From 123f5d3eabfd8c708da26489386534dcfa072fff Mon Sep 17 00:00:00 2001 From: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com> Date: Fri, 31 Jul 2026 16:52:08 +0800 Subject: [PATCH] fix(spec,plugins): sweep the auth/session slot lookups (#4251) Batch B2 of the #4251 sweep: every service-lookup erasure in the auth/session family. plugin-auth/auth-plugin.ts (20), plugin-hono-server/current-user-endpoints.ts (10) and plugin-security/security-plugin.ts (1) now pass the slot's contract type; the ratchet baseline drops 171 -> 140 sites, 40 -> 37 files, and all three files leave the grandfather list. The yield: POST /admin/import-users resolved the `metadata` slot and probed `metadataService?.getMetaItem` to decide whether to pass the import's field-coercion dependency. `getMetaItem` is a PROTOCOL method (ObjectStackProtocolImplementation, registered under `protocol`); MetadataManager, which occupies `metadata`, has never had it. The probe was false on every deployment and the dep was never passed -- imported rows reached sys_user uncoerced, with the branch claiming otherwise sitting right there. Same shape as #4127's dead automation.trigger. The route reads `protocol` now. /me/apps reached ObjectQL's PRIVATE `_registry` through `as any` while /auth/me/permissions, two handlers up in the same file, read the public `registry` getter over the same field of the same object. Both read the public accessor now; the one test that stubbed `_registry` was pinning that private reach and stubs `registry`. Contract, from evidence: IDataEngine's read methods (find / findOne / count / aggregate) declare the trailing `options?: BaseEngineOptions` argument they have always accepted. ObjectQL's own doc explains why it exists -- the same `{ context }` object was correct as insert's 3rd argument and SILENTLY DROPPED as find's, so an intended isSystem bypass just vanished. The contract exposed only query.context, so callers using the trailing channel could reach it only by erasing the lookup. Adding an optional trailing parameter breaks no implementor or caller; BaseEngineOptions was already exported but sat unused under the legacy/deprecated heading, and moves up with the rationale attached. One new spec test pins the argument at the call site. Where the contract does not reach, the escape hatch is named rather than `any`: PermissionEvaluatorSurface (security.permissions), SettingsReadSurface (settings), EngineRegistrySurface / EngineExtensionSurface (objectql beyond IDataEngine). That last one is deliberate scope -- the standing record on getObjectQL in @objectstack/ runtime says ObjectQL is genuinely wider than IDataEngine and typing the whole thing IDataEngine would be the more comfortable-looking lie. These declarations are what that contract gets written from, and what it deletes. Verified: full-repo typecheck 110/110; spec 7157/278, objectql 1394/87, runtime 951/66, plugin-auth 579/26, plugin-security 677/32, plugin-hono-server 135/12; eslint clean on every changed file; check:slot-lookup 140 sites in 37 files. Co-Authored-By: Claude Opus 5 --- .changeset/slot-lookup-sweep-auth-session.md | 61 ++++++++ .../plugins/plugin-auth/src/auth-plugin.ts | 142 +++++++++++++++--- ...urrent-user-endpoints-multi-tenant.test.ts | 4 +- .../src/current-user-endpoints.ts | 109 +++++++++++--- .../plugin-security/src/security-plugin.ts | 3 +- .../spec/src/contracts/data-engine.test.ts | 46 ++++++ packages/spec/src/contracts/data-engine.ts | 25 ++- packages/spec/src/data/data-engine.zod.ts | 19 ++- scripts/slot-lookup-baseline.json | 3 - 9 files changed, 362 insertions(+), 50 deletions(-) create mode 100644 .changeset/slot-lookup-sweep-auth-session.md diff --git a/.changeset/slot-lookup-sweep-auth-session.md b/.changeset/slot-lookup-sweep-auth-session.md new file mode 100644 index 0000000000..34c9530d73 --- /dev/null +++ b/.changeset/slot-lookup-sweep-auth-session.md @@ -0,0 +1,61 @@ +--- +"@objectstack/spec": patch +"@objectstack/plugin-auth": patch +"@objectstack/plugin-hono-server": patch +"@objectstack/plugin-security": patch +--- + +fix(spec,plugins): sweep the auth/session slot lookups — 31 sites typed, and the user-import metadata reader was pointed at a service that never had the method (#4251) + +Batch B2 of the #4251 sweep: every service-lookup erasure in the auth/session +family. `plugin-auth/auth-plugin.ts` (20), `plugin-hono-server/current-user-endpoints.ts` +(10) and `plugin-security/security-plugin.ts` (1) now pass the slot's contract +type; the ratchet baseline drops **171 → 140 sites, 40 → 37 files**. + +**The yield.** `POST /admin/import-users` resolved the `metadata` slot and probed +`metadataService?.getMetaItem` to decide whether to pass the import's field-coercion +dependency. `getMetaItem` is a **protocol** method — `ObjectStackProtocolImplementation`, +registered by MetadataProtocolPlugin under the `protocol` slot. `MetadataManager`, +which occupies `metadata`, has never had it. So the probe was false on every +deployment and the dep was never passed: imported rows reached `sys_user` +uncoerced, with the branch that says otherwise sitting right there. This is the +same shape as #4127's dead `automation.trigger` and #4321's `registerInMemory` +probes — a capability the code advertises and the runtime cannot deliver, kept +invisible by the `any`. Typing the lookup to `IMetadataService` is what turned it +into a compile error. The route reads `protocol` now. + +`/me/apps` reached ObjectQL's **private** `_registry` through `as any` while +`/auth/me/permissions`, two handlers up in the same file, read the public +`registry` getter over the same field of the same object. Both read the public +accessor now; the one test that stubbed `_registry` was pinning the private reach +and stubs `registry` instead. + +**Contract, from evidence.** `IDataEngine`'s read methods (`find` / `findOne` / +`count` / `aggregate`) declare the trailing `options?: BaseEngineOptions` +argument they have always accepted. ObjectQL's own doc explains why it exists: +reads once took their context inside the query while writes took it in trailing +`options.context`, so the same `{ context }` object was correct as `insert`'s 3rd +argument and **silently dropped** as `find`'s — "an intended `isSystem` bypass +just vanished". The engine accepts both channels; the contract exposed only the +query one, so callers using the trailing channel — the current-user endpoints' +permission-set loader among them — could only reach it by erasing the lookup. +Adding an optional trailing parameter breaks no implementor (the existing +minimal-implementation test proves it) and no caller. `BaseEngineOptions` was +already exported, sitting unused under the "legacy/deprecated" heading, which is +why the contract went looking and did not find it; it moves up beside the other +QueryAST-aligned types with the rationale attached. One new spec test pins the +trailing argument at the call site — the position where the old contract rejected it. + +**Where the contract does not reach, the escape hatch is named.** Three slots +resist a spec type today and each gets a narrow, documented local interface +instead of `any`: `security.permissions` (plugin-security's `PermissionEvaluator` +— plugin-hono-server must not depend on an optional plugin), `settings` +(service-settings' resolver, same reason), and ObjectQL beyond `IDataEngine` +(`registry` / `getSchema` / `registerHook` / `registerMiddleware`). That last one +is deliberate scope: the standing record on `getObjectQL` in `@objectstack/runtime` +says ObjectQL is genuinely wider than `IDataEngine` and nobody has written the +wider contract, so typing the whole thing `IDataEngine` would be "the more +comfortable-looking lie". These declarations are what that contract gets written +from, and what it deletes. + +No behavior changes beyond the two fixes above. diff --git a/packages/plugins/plugin-auth/src/auth-plugin.ts b/packages/plugins/plugin-auth/src/auth-plugin.ts index a34867707b..0628721199 100644 --- a/packages/plugins/plugin-auth/src/auth-plugin.ts +++ b/packages/plugins/plugin-auth/src/auth-plugin.ts @@ -2,7 +2,14 @@ import { Plugin, PluginContext, IHttpServer } from '@objectstack/core'; import type { BetterAuthOptions } from 'better-auth'; -import { AuthConfig, type SocialProviderConfig, SystemObjectName, SystemUserId } from '@objectstack/spec/system'; +import { + AuthConfig, + type SocialProviderConfig, + type SettingsChangeHandler, + type SettingsUnsubscribe, + SystemObjectName, + SystemUserId, +} from '@objectstack/spec/system'; import { // ADR-0048 — the Setup/Studio/Account apps moved to their own packages // (@objectstack/{setup,studio,account}); plugin-auth no longer registers them. @@ -12,6 +19,7 @@ import { import { SysOrganizationDetailPage, SysUserDetailPage } from '@objectstack/platform-objects/pages'; import { resolveTenancyPosture } from '@objectstack/types'; import { postureEnforcesWall, type OrgScopingEntitlement } from '@objectstack/spec/security'; +import type { IDataEngine, IEmailService, ISmsService } from '@objectstack/spec/contracts'; import { AuthManager, resolveOidcProviderEnabled, @@ -36,6 +44,87 @@ import { authPluginManifestHeader, } from './manifest.js'; +/** + * The `objectql` slot BEYOND `IDataEngine` — the hook and middleware seams this + * plugin installs on the engine. + * + * [#4251] The slot's ledger entry is `IDataEngine` and it covers every read and + * write below; it does not cover `registerHook` / `registerMiddleware`, and no + * contract has been written for the wider ObjectQL surface yet (the standing + * record of why is on `getObjectQL` in `@objectstack/runtime`'s + * `DomainHandlerContext`). Declared here, narrow and named, so the extension is + * legible instead of hidden under `any` — and so it is deleted, not migrated, + * when that contract lands. + * + * Both members are optional and every call site already guards with + * `typeof … === 'function'`: the slot is satisfiable by engines that implement + * neither (mock mode), and this plugin degrades rather than fails there. + */ +interface EngineExtensionSurface { + registerHook?( + event: string, + handler: (context: any) => Promise | void, + options?: { object?: string | string[]; priority?: number; packageId?: string }, + ): void; + registerMiddleware?( + fn: (opCtx: any, next: () => Promise) => Promise, + options?: { object?: string }, + ): void; +} + +/** The engine as this plugin uses it: the data contract plus those two seams. */ +type AuthEngine = IDataEngine & EngineExtensionSurface; + +/** + * The `settings` slot, as this plugin reads it. + * + * [#4251] `service-settings` registers its `SettingsService` here and the slot + * carries no `packages/spec` contract, so this declares the two resolver methods + * this plugin calls. Structural on purpose — plugin-auth must not depend on + * service-settings, which is optional (both readers below already return early + * when the slot is empty or the method is missing). + */ +interface SettingsReadSurface { + /** Resolve one key; `source` distinguishes a stored/env value from a manifest default. */ + get( + namespace: string, + key: string, + ctx?: Record, + ): Promise<{ value?: unknown; source?: string } | undefined>; + /** Resolve a whole namespace as `key → { value, source }`. */ + getNamespace( + namespace: string, + ctx?: Record, + ): Promise<{ values: Record }>; + /** + * Re-run a binding when the namespace changes — how the brand name, SMS + * locale and auth namespace stay live without a redeploy. Optional: an + * implementation without a change bus leaves the initial read in place, which + * is what the `typeof … === 'function'` guards at each call site already say. + * + * The handler and unsubscribe types are the SPEC's + * (`@objectstack/spec/system`), not re-declared here — the change bus has a + * published contract even though the slot does not. + */ + subscribe?( + namespace: string | undefined, + handler: SettingsChangeHandler, + ): SettingsUnsubscribe; +} + +/** + * The `protocol` slot's metadata reader, as the user-import route uses it. + * + * [#4251] `protocol` is a deliberately UNCONTRACTED slot (see + * `UNCONTRACTED_SLOTS` in `eslint.config.mjs`), so the one method this route + * needs is declared here rather than erased. It reads `sys_user`'s field + * definitions so imported values are coerced to their declared types — + * best-effort by contract: without it the import still runs, uncoerced. + */ +interface MetaItemReaderSurface { + getMetaItem(ref: { type: string; name: string }): Promise; +} + /** * Auth Plugin Options * Extends AuthConfig from spec with additional runtime options @@ -222,7 +311,7 @@ export class AuthPlugin implements Plugin { // that advertises a tolerance the composition forbids is worse than no // branch — it reads as a supported degraded mode (#4187). AuthManager keeps // its own `dataEngine?` guards because it is usable outside this plugin. - const dataEngine = ctx.getService('data'); + const dataEngine = ctx.getService('data'); const authConfig: AuthManagerOptions & AuthPluginOptions = { ...this.options, @@ -451,8 +540,8 @@ export class AuthPlugin implements Plugin { if (this.authManager) { await this.bindAuthSettings(ctx); - let emailSvc: any; - try { emailSvc = ctx.getService('email'); } catch { emailSvc = undefined; } + let emailSvc: IEmailService | undefined; + try { emailSvc = ctx.getService('email'); } catch { emailSvc = undefined; } if (emailSvc) { this.authManager.setEmailService(emailSvc); ctx.logger.info('Auth: email service wired (transactional mail enabled)'); @@ -480,8 +569,8 @@ export class AuthPlugin implements Plugin { // SMS-invite path can deliver. Same lazy-resolution contract as // the email service: absent ⇒ OTP endpoints keep failing loudly // (NOT_SUPPORTED) while phone+password sign-in still works. - let smsSvc: any; - try { smsSvc = ctx.getService('sms'); } catch { smsSvc = undefined; } + let smsSvc: ISmsService | undefined; + try { smsSvc = ctx.getService('sms'); } catch { smsSvc = undefined; } if (smsSvc) { this.authManager.setSmsService(smsSvc); if (this.authManager.isPhoneNumberEnabled()) { @@ -503,7 +592,7 @@ export class AuthPlugin implements Plugin { // the override so the deployment's `appName` (e.g. `OS_APP_NAME`) // keeps precedence. Mirrors EmailServicePlugin's settings binding. try { - const settings = ctx.getService('settings'); + const settings = ctx.getService('settings'); if (settings && typeof settings.get === 'function') { const applyBrand = async () => { try { @@ -628,7 +717,7 @@ export class AuthPlugin implements Plugin { // whose rows already carry an issuer costs one empty query. ctx.hook('kernel:ready', async () => { try { - const ql: any = ctx.getService('objectql'); + const ql = ctx.getService('objectql'); if (!ql) return; const { backfillAccountIssuer } = await import('./backfill-account-issuer.js'); await backfillAccountIssuer(ql, { @@ -656,7 +745,7 @@ export class AuthPlugin implements Plugin { if (this.options.autoDefaultOrganization !== false && !postureEnforcesWall(resolveTenancyPosture())) { const runEnsure = async () => { try { - const ql: any = ctx.getService('objectql'); + const ql = ctx.getService('objectql'); if (!ql) return; const res = await ensureDefaultOrganization(ql, { logger: ctx.logger }); if (res.defaultOrgCreated) { @@ -675,7 +764,7 @@ export class AuthPlugin implements Plugin { // to platform admin" case where kernel:ready fired before any user // existed (same wiring the multi-org bootstrap uses). try { - const ql: any = ctx.getService('objectql'); + const ql = ctx.getService('objectql'); if (ql && typeof ql.registerMiddleware === 'function') { ql.registerMiddleware(async (opCtx: any, next: () => Promise) => { await next(); @@ -708,7 +797,7 @@ export class AuthPlugin implements Plugin { const runBackfill = (source: string): Promise => { backfillChain = backfillChain.then(async () => { try { - const ql: any = ctx.getService('objectql'); + const ql = ctx.getService('objectql'); const tenancy = this.tenancy; if (!ql || !tenancy) return; const res = await backfillMemberships(ql, { @@ -752,7 +841,7 @@ export class AuthPlugin implements Plugin { try { // Use the kernel's ObjectQL engine (available + hookable at kernel:ready); // the auth manager's getDataEngine() is not yet wired this early. - const engine: any = ctx.getService('objectql'); + const engine = ctx.getService('objectql'); if (!engine || typeof engine.registerHook !== 'function') return; const SYSTEM_CTX = { isSystem: true, roles: [], permissions: [] }; engine.registerHook('afterInsert', async (hookCtx: any) => { @@ -794,7 +883,7 @@ export class AuthPlugin implements Plugin { // bypass — see identity-write-guard.ts for the full contract. ctx.hook('kernel:ready', async () => { try { - const engine: any = ctx.getService('objectql'); + const engine = ctx.getService('objectql'); if (!engine || typeof engine.registerHook !== 'function') return; registerManagedUpdateWhitelist(SystemObjectName.USER, SYS_USER_PROFILE_EDIT_FIELDS); // [ADR-0105 D7] Extension fields ObjectStack adds to better-auth-managed @@ -821,7 +910,7 @@ export class AuthPlugin implements Plugin { // Register auth middleware on ObjectQL engine (if available) try { - const ql = ctx.getService('objectql'); + const ql = ctx.getService('objectql'); if (ql && typeof ql.registerMiddleware === 'function') { ql.registerMiddleware(async (opCtx: any, next: () => Promise) => { // If context already has userId or isSystem, skip auth resolution @@ -850,9 +939,9 @@ export class AuthPlugin implements Plugin { private async bindAuthSettings(ctx: PluginContext): Promise { if (!this.authManager) return; - let settings: any; + let settings: SettingsReadSurface | undefined; try { - settings = ctx.getService('settings'); + settings = ctx.getService('settings'); } catch { return; } @@ -1136,8 +1225,8 @@ export class AuthPlugin implements Plugin { const password = process.env.OS_SEED_ADMIN_PASSWORD?.trim() || 'admin123'; const name = process.env.OS_SEED_ADMIN_NAME?.trim() || 'Dev Admin'; - let ql: any; - try { ql = ctx.getService('objectql'); } catch { /* unavailable */ } + let ql: IDataEngine | undefined; + try { ql = ctx.getService('objectql'); } catch { /* unavailable */ } if (!ql || typeof ql.find !== 'function') return; try { @@ -1627,15 +1716,24 @@ export class AuthPlugin implements Plugin { const actor = await gateAdmin(c); if (actor instanceof Response) return actor; const { runAdminImportUsers } = await import('./admin-import-users.js'); - const metadataService: any = (() => { - try { return ctx.getService?.('metadata'); } catch { return undefined; } + // [#4251] Resolved from `protocol`, not `metadata`. `getMetaItem` is a + // PROTOCOL method (`ObjectStackProtocolImplementation`, registered by + // MetadataProtocolPlugin); the `metadata` slot holds MetadataManager, + // which has never had it. So `metadataService?.getMetaItem` was always + // falsy and the field-coercion dep was never passed — an import row's + // values reached `sys_user` uncoerced, with the branch that says + // otherwise sitting right here. Invisible while the lookup was `any`: + // typing it to the slot's contract is what turned the dead probe into + // a compile error. + const metaReader = (() => { + try { return ctx.getService?.('protocol'); } catch { return undefined; } })(); const { status, body } = await runAdminImportUsers( { getAuthApi: () => this.authManager!.getApi() as any, getDataEngine: () => this.authManager!.getDataEngine(), - ...(metadataService?.getMetaItem - ? { getMetaItem: (ref: { type: string; name: string }) => metadataService.getMetaItem(ref) } + ...(typeof metaReader?.getMetaItem === 'function' + ? { getMetaItem: (ref: { type: string; name: string }) => metaReader.getMetaItem(ref) } : {}), phoneNumberEnabled: () => this.authManager!.isPhoneNumberEnabled(), emailServiceAvailable: () => this.authManager!.isEmailServiceAvailable(), diff --git a/packages/plugins/plugin-hono-server/src/current-user-endpoints-multi-tenant.test.ts b/packages/plugins/plugin-hono-server/src/current-user-endpoints-multi-tenant.test.ts index 681ce4f0af..b4ff8dde0b 100644 --- a/packages/plugins/plugin-hono-server/src/current-user-endpoints-multi-tenant.test.ts +++ b/packages/plugins/plugin-hono-server/src/current-user-endpoints-multi-tenant.test.ts @@ -96,7 +96,9 @@ describe('the answer comes from the kernel that OWNS the request (cloud#927)', ( it('routes /auth/me/localization and /me/apps through the same resolution', async () => { const envKernel = kernelWith({ auth: authFor('usr_env'), - objectql: { _registry: { getAllApps: () => [{ name: 'env_app' }] } }, + // [#4251] `registry`, the public accessor — this stub pinned the + // private `_registry` the handler used to reach through `as any`. + objectql: { registry: { getAllApps: () => [{ name: 'env_app' }] } }, }); const { app } = mount({}, async () => envKernel); diff --git a/packages/plugins/plugin-hono-server/src/current-user-endpoints.ts b/packages/plugins/plugin-hono-server/src/current-user-endpoints.ts index 94a58ed672..9085197655 100644 --- a/packages/plugins/plugin-hono-server/src/current-user-endpoints.ts +++ b/packages/plugins/plugin-hono-server/src/current-user-endpoints.ts @@ -45,7 +45,7 @@ import { type EnableLike, } from '@objectstack/spec/data'; import type { ExecutionContext } from '@objectstack/spec/kernel'; -import type { Logger } from '@objectstack/spec/contracts'; +import type { IAuthService, IMetadataService, Logger } from '@objectstack/spec/contracts'; import { allowPerfDisclosure, isPerfDisclosurePrincipal } from '@objectstack/observability'; /** API prefix these endpoints mount under unless the host overrides it. */ @@ -261,6 +261,71 @@ export function foldWildcardSuperUser(objects: Record): void { } } +/** + * The `objectql` slot BEYOND `IDataEngine` — the schema registry these + * endpoints read. + * + * [#4251] The slot's ledger entry is `IDataEngine` (ObjectQL registers the SAME + * instance under `data` and `objectql`), and that covers the reads below. It + * does NOT cover `registry` / `getSchema`, and the honest record of why lives + * on `getObjectQL` in `@objectstack/runtime`'s `DomainHandlerContext`: ObjectQL + * is genuinely wider than `IDataEngine`, nobody has written a contract for the + * wider part, and typing the whole thing `IDataEngine` would be "the more + * comfortable-looking lie". So the extra surface is declared here, named and + * narrow, instead of the lookup being erased to `any` — the wider contract, when + * someone writes it, absorbs this and the declaration is deleted. + * + * Every member is optional and every call site probes with `?.`: the slot is + * satisfied by engines with no registry at all (test fakes, remote engines), and + * these endpoints degrade rather than fail when it is absent. + */ +interface EngineRegistrySurface { + /** + * The engine's schema registry — the PUBLIC accessor. ObjectQL exposes it as + * `get registry()` over the private `_registry` field; `_registry` is what + * `/me/apps` used to reach through `as any`, two handlers away from the + * `/auth/me/permissions` reach for the public one, for the same object. + */ + readonly registry?: { + getAllObjects?(): ApiExposureSchemaLike[]; + getAllApps?(): unknown[]; + }; + getSchema?(objectName: string): unknown; +} + +/** + * The `security.permissions` slot, as these two handlers use it. + * + * [#4251] plugin-security registers its `PermissionEvaluator` under this name; + * the slot has no `packages/spec` contract, so this declares the ONE method both + * handlers call rather than erasing the lookup. Structural on purpose — + * plugin-hono-server must not take a runtime dependency on plugin-security, + * which is OPTIONAL in the stacks these endpoints serve (the `!evaluator` + * branches below are exactly its absence). + * + * The parameter types are the loose shapes this caller passes, not the + * evaluator's own: it takes `metadataService: any` and resolves to parsed + * `PermissionSet`s, while the DB loader here yields the projected subset the + * merges below read. Declaring what is passed and read keeps the claim honest. + */ +interface PermissionEvaluatorSurface { + resolvePermissionSets( + identifiers: string[], + metadataService: unknown, + bootstrapPermissionSets?: unknown[], + dbLoader?: (unresolved: string[]) => Promise, + ): Promise; +} + +/** The permission-set fields the two handlers merge out of a resolution. */ +interface ResolvedPermissionSetLike { + name?: string; + objects?: Record; + fields?: Record; + systemPermissions?: unknown; + tabPermissions?: unknown; +} + /** Minimal schema shape the managed-write clamp needs. */ export interface ManagedSchemaLike { managedBy?: string; @@ -426,9 +491,9 @@ export function makeExecutionContextResolver(ctx: CurrentUserEndpointsContext) { // promotion seeded by `bootstrapPlatformAdmin`. const resolveCtx = async (c: any): Promise => { try { - const authService: any = ctx.getService('auth'); + const authService = ctx.getService('auth'); if (!authService) return undefined; - let api: any = authService.api; + let api = authService.api; if (!api && typeof authService.getApi === 'function') { api = await authService.getApi(); } @@ -685,11 +750,12 @@ export function registerCurrentUserEndpoints( // logged as "/auth/me/permissions failed", which reads as a // fault on every console navigation instead of the deliberate // `!evaluator` branch right below. - const metadata: any = (() => { - try { return ctx.getService('metadata'); } catch { return null; } + const metadata = (() => { + try { return ctx.getService('metadata') ?? null; } catch { return null; } })(); - const evaluator: any = (() => { - try { return ctx.getService('security.permissions'); } catch { return null; } + const evaluator = (() => { + try { return ctx.getService('security.permissions') ?? null; } + catch { return null; } })(); const bootstrap: any[] = (() => { try { return ctx.getService('security.bootstrapPermissionSets') ?? []; } @@ -702,8 +768,9 @@ export function registerCurrentUserEndpoints( // DB loader: surfaces user-defined permission sets // (created via the admin UI as `sys_permission_set` // rows) that aren't in metadata or bootstrap. - const ql: any = (() => { - try { return ctx.getService('objectql'); } catch { return null; } + const ql = (() => { + try { return ctx.getService('objectql') ?? null; } + catch { return null; } })(); const dbLoader = ql ? async (names: string[]) => { @@ -763,7 +830,7 @@ export function registerCurrentUserEndpoints( ...(execCtx.positions ?? []), ...(execCtx.permissions ?? []), ]; - let resolved: any[] = await evaluator + let resolved: ResolvedPermissionSetLike[] = await evaluator .resolvePermissionSets(requested, metadata, bootstrap, dbLoader) .catch(() => []); if (resolved.length === 0 && fallbackName) { @@ -831,7 +898,7 @@ export function registerCurrentUserEndpoints( // here must never drop the whole response. try { const allSchemas: ApiExposureSchemaLike[] = (() => { - try { return (ql as any)?.registry?.getAllObjects?.() ?? []; } + try { return ql?.registry?.getAllObjects?.() ?? []; } catch { return []; } })(); seedSuperUserRestrictedObjects(objects, allSchemas); @@ -909,13 +976,17 @@ export function registerCurrentUserEndpoints( try { const byName = new Map(); try { - const registry: any = (ctx.getService('objectql') as any)?._registry; + // The PUBLIC accessor. This read used to reach ObjectQL's + // private `_registry` while `/auth/me/permissions` read the + // `registry` getter over the same field, on the same object, + // in the same file — visible only once both were typed. + const registry = ctx.getService('objectql')?.registry; for (const app of registry?.getAllApps?.() ?? []) { - if (app?.name) byName.set(String(app.name), app); + if ((app as { name?: unknown })?.name) byName.set(String((app as { name: unknown }).name), app); } } catch { /* registry unavailable — fall through to metadata */ } try { - const metadata: any = ctx.getService('metadata'); + const metadata = ctx.getService('metadata'); for (const app of ((await metadata?.list?.('app')) ?? []) as any[]) { if (app?.name && !byName.has(String(app.name))) byName.set(String(app.name), app); } @@ -929,10 +1000,10 @@ export function registerCurrentUserEndpoints( const tabs: Record = { ...((execCtx as any).tabPermissions ?? {}) }; let failOpen = true; try { - const evaluator: any = ctx.getService('security.permissions'); + const evaluator = ctx.getService('security.permissions'); failOpen = !evaluator; if (evaluator) { - const metadata: any = ctx.getService('metadata'); + const metadata = ctx.getService('metadata'); const bootstrap: any[] = (() => { try { return ctx.getService('security.bootstrapPermissionSets') ?? []; } catch { return []; } @@ -945,7 +1016,9 @@ export function registerCurrentUserEndpoints( ...((execCtx as any).positions ?? []), ...((execCtx as any).permissions ?? []), ]; - const qlSvc: any = (() => { try { return ctx.getService('objectql'); } catch { return null; } })(); + const qlSvc = (() => { + try { return ctx.getService('objectql') ?? null; } catch { return null; } + })(); const dbLoader = qlSvc ? async (names: string[]) => { let rows: any; @@ -968,7 +1041,7 @@ export function registerCurrentUserEndpoints( })); } : undefined; - let resolved: any[] = await evaluator + let resolved: ResolvedPermissionSetLike[] = await evaluator .resolvePermissionSets(requested, metadata, bootstrap, dbLoader) .catch(() => []); if (resolved.length === 0 && fallbackName) { diff --git a/packages/plugins/plugin-security/src/security-plugin.ts b/packages/plugins/plugin-security/src/security-plugin.ts index 1bf8bd29d0..8a4d1eaf10 100644 --- a/packages/plugins/plugin-security/src/security-plugin.ts +++ b/packages/plugins/plugin-security/src/security-plugin.ts @@ -17,6 +17,7 @@ import { intersectFieldMasks, } from './explain-engine.js'; import type { ExplainDecision, ExplainOperation } from '@objectstack/spec/security'; +import type { II18nService } from '@objectstack/spec/contracts'; import { bootstrapDeclaredPositions } from './bootstrap-declared-positions.js'; import { bootstrapDeclaredPermissions, upsertPackagePermissionSet, readDeclared } from './bootstrap-declared-permissions.js'; import { applyManagedWriteDenies } from './managed-object-write-denies.js'; @@ -451,7 +452,7 @@ export class SecurityPlugin implements Plugin { if (typeof (ctx as any).hook === 'function') { (ctx as any).hook('kernel:ready', async () => { try { - const i18n = ctx.getService('i18n'); + const i18n = ctx.getService('i18n'); if (i18n && typeof i18n.loadTranslations === 'function') { const { SecurityTranslations } = await import('./translations/index.js'); for (const [locale, data] of Object.entries(SecurityTranslations)) { diff --git a/packages/spec/src/contracts/data-engine.test.ts b/packages/spec/src/contracts/data-engine.test.ts index 301dd3475f..8c920f200c 100644 --- a/packages/spec/src/contracts/data-engine.test.ts +++ b/packages/spec/src/contracts/data-engine.test.ts @@ -90,6 +90,52 @@ describe('Data Engine Contract', () => { expect(count).toBe(2); }); + it('takes the execution context in a trailing options argument on every read', async () => { + // [#4251] The read methods' 3rd argument. It is what ObjectQL has taken + // since reads and writes were unified on "context goes in the trailing + // options" — passing it as the 3rd arg to `insert` was correct while the + // same object as the 3rd arg to `find` was SILENTLY DROPPED, and an + // intended `isSystem` bypass just vanished. The contract declared only + // `query.context`, so callers reaching the trailing channel had to erase + // the lookup to `any` to do it. Pinned here in the position that matters: + // at the CALL, which is where the old contract rejected it. + const seen: Array<{ method: string; isSystem?: boolean }> = []; + const engine: IDataEngine = { + find: async (_obj, _query, options) => { + seen.push({ method: 'find', isSystem: options?.context?.isSystem }); + return []; + }, + findOne: async (_obj, _query, options) => { + seen.push({ method: 'findOne', isSystem: options?.context?.isSystem }); + return null; + }, + insert: async (_obj, data) => data, + update: async (_obj, data) => data, + delete: async () => ({}), + count: async (_obj, _query, options) => { + seen.push({ method: 'count', isSystem: options?.context?.isSystem }); + return 0; + }, + aggregate: async (_obj, _query, options) => { + seen.push({ method: 'aggregate', isSystem: options?.context?.isSystem }); + return []; + }, + }; + + const sys = { context: { isSystem: true } }; + await engine.find('sys_permission_set', { where: {} }, sys); + await engine.findOne('sys_user', { where: {} }, sys); + await engine.count('sys_account', { where: {} }, sys); + await engine.aggregate('sys_user', {} as any, sys); + + expect(seen).toEqual([ + { method: 'find', isSystem: true }, + { method: 'findOne', isSystem: true }, + { method: 'count', isSystem: true }, + { method: 'aggregate', isSystem: true }, + ]); + }); + it('should support optional vectorFind', async () => { const engine: IDataEngine = { find: async () => [], diff --git a/packages/spec/src/contracts/data-engine.ts b/packages/spec/src/contracts/data-engine.ts index f24b6e0344..571d57704d 100644 --- a/packages/spec/src/contracts/data-engine.ts +++ b/packages/spec/src/contracts/data-engine.ts @@ -1,6 +1,7 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import { + BaseEngineOptions, EngineQueryOptions, DataEngineInsertOptions, EngineUpdateOptions, @@ -50,13 +51,29 @@ export interface WriteObservabilityOptions { */ export interface IDataEngine { - find(objectName: string, query?: EngineQueryOptions): Promise; - findOne(objectName: string, query?: EngineQueryOptions): Promise; + /** + * Read methods take the execution context in a TRAILING options argument, + * the same position the write methods take theirs. + * + * [#4251] Declared because the implementation and its callers were already + * there: ObjectQL's `find`/`findOne`/`count`/`aggregate` have taken this + * argument since the read/write split was unified, and its own doc explains + * why — the same `{ context }` object was correct as the 3rd arg to `insert` + * but SILENTLY DROPPED as the 3rd arg to `find`, so an intended `isSystem` + * bypass just vanished (control-plane reads coming back empty once + * org-scoping hooks landed). The contract kept only `query.context`, so + * every caller passing the trailing one — the current-user endpoints' + * permission-set loader among them — had to reach it through `any`, which is + * exactly the erasure this issue is sweeping. `query.context` remains + * supported; when both are given, `options.context` wins. + */ + find(objectName: string, query?: EngineQueryOptions, options?: BaseEngineOptions): Promise; + findOne(objectName: string, query?: EngineQueryOptions, options?: BaseEngineOptions): Promise; insert(objectName: string, data: any | any[], options?: DataEngineInsertOptions & WriteObservabilityOptions): Promise; update(objectName: string, data: any, options?: EngineUpdateOptions & WriteObservabilityOptions): Promise; delete(objectName: string, options?: EngineDeleteOptions): Promise; - count(objectName: string, query?: EngineCountOptions): Promise; - aggregate(objectName: string, query: EngineAggregateOptions): Promise; + count(objectName: string, query?: EngineCountOptions, options?: BaseEngineOptions): Promise; + aggregate(objectName: string, query: EngineAggregateOptions, options?: BaseEngineOptions): Promise; /** * Vector Search (AI/RAG) diff --git a/packages/spec/src/data/data-engine.zod.ts b/packages/spec/src/data/data-engine.zod.ts index f3d1c4c453..f55cf2c738 100644 --- a/packages/spec/src/data/data-engine.zod.ts +++ b/packages/spec/src/data/data-engine.zod.ts @@ -704,6 +704,24 @@ export const DataEngineRequestSchema = lazySchema(() => z.discriminatedUnion('me // ========================================================================== // --- New: QueryAST-aligned types (preferred) --- +/** + * Trailing options for the READ methods (`find` / `findOne` / `count` / + * `aggregate`) — the execution context, and nothing else. + * + * [#4251] The schema always existed; the exported type did not, so + * `IDataEngine`'s read methods could not declare the trailing argument their + * implementation has taken since the split was unified. Reads once took their + * context INSIDE the query while writes took it in trailing `options.context`, + * and passing the write shape to a read SILENTLY DROPPED it — an intended + * `isSystem` bypass just vanished. The engine accepts both channels now + * (`options.context` wins), but a caller typed to the contract could not reach + * the trailing one at all, so the callers that use it were reaching it through + * `any`. Same shape as {@link BaseEngineOptionsSchema} by construction: this is + * naming what is already there, not widening it. (The type itself is not new — + * it sat unused under the "legacy/deprecated" heading below, which is why the + * contract went looking for it and did not find it.) + */ +export type BaseEngineOptions = z.infer; export type EngineQueryOptions = z.infer; export type EngineUpdateOptions = z.infer; export type DroppedFieldsEvent = z.infer; @@ -715,7 +733,6 @@ export type EngineCountOptions = z.infer; export type DataEngineFilter = z.infer; /** @deprecated Use standard `SortNode[]` from QueryAST instead. */ export type DataEngineSort = z.infer; -export type BaseEngineOptions = z.infer; /** @deprecated Use `EngineQueryOptions` instead. */ export type DataEngineQueryOptions = z.infer; export type DataEngineInsertOptions = z.infer; diff --git a/scripts/slot-lookup-baseline.json b/scripts/slot-lookup-baseline.json index 937ed74e55..df999668b9 100644 --- a/scripts/slot-lookup-baseline.json +++ b/scripts/slot-lookup-baseline.json @@ -13,12 +13,9 @@ "packages/plugins/plugin-approvals/src/approvals-plugin.ts": 7, "packages/plugins/plugin-approvals/src/status-mirror-cascade.integration.test.ts": 2, "packages/plugins/plugin-audit/src/audit-plugin.ts": 1, - "packages/plugins/plugin-auth/src/auth-plugin.ts": 20, "packages/plugins/plugin-email/src/email-plugin.ts": 3, - "packages/plugins/plugin-hono-server/src/current-user-endpoints.ts": 10, "packages/plugins/plugin-pinyin-search/src/pinyin-search-plugin.ts": 2, "packages/plugins/plugin-reports/src/reports-plugin.ts": 5, - "packages/plugins/plugin-security/src/security-plugin.ts": 1, "packages/plugins/plugin-sharing/src/sharing-plugin.ts": 7, "packages/plugins/plugin-webhooks/src/webhook-outbox-plugin.ts": 1, "packages/qa/dogfood/test/showcase-agent-intersection.dogfood.test.ts": 1,