From 8a93311941d6be56a53adf5929ff9b62e6b2e117 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 16:09:12 +0000 Subject: [PATCH 1/2] =?UTF-8?q?fix(runtime,spec):=20guard=20the=20service-?= =?UTF-8?q?lookup=20typing=20with=20a=20lint=20rule=20=E2=80=94=20which=20?= =?UTF-8?q?found=20the=20project-membership=20gate=20not=20gating=20(#4127?= =?UTF-8?q?)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Batch 4 of the #4127 gate. #4168/#4176/#4202 made a slot lookup return the slot's contract. Nothing protected that: an `any` annotation on the RESULT switches the checking back off for that call site, silently, with no test failing and no visual difference from code that has it. Three such sites already existed and were found by grep — the same unrepeatable sweep this work replaced. The rule bans `: any` / `as any` on a `resolveService` / `getService` / `getRequestKernelService` result. Slots with no written contract (`protocol`, `mcp`, `kernel-resolver`, `scope-manager`) are exempted BY NAME, CENTRALLY, in eslint.config.mjs — not by inline disables, because `pnpm lint` runs `--no-inline-config` and ignores those on purpose. The effect is the one worth having: a deliberate gap is a reviewed line in one file, a careless one is a build failure, and they stop looking identical in the code. ITS FIRST RUN FOUND A LIVE FAIL-OPEN. `enforceProjectMembership` read the session as `authService?.api?.getSession?.(...)` with no `getApi()` fallback — the only one of the codebase's three `.api` readers without it. `plugin-auth` registers `AuthManager`, which has NO `.api` member at all. So the read yielded `undefined`, `userId` stayed unset, and the function returned at its "anonymous — upstream auth will decide" line BEFORE ever querying `sys_environment_member`. A signed-in non-member passed the gate, on every deployment with project scoping on — which is where the flag defaults to true. Anonymous callers were still denied elsewhere (#2567/#3963), so this was specifically the signed-in non-member case. The existing test for that gate mocked auth as `{ api: { getSession } }` — the legacy shape the shipped provider does not have — so it was green throughout. That is the FOURTH test in this work line found encoding a contract nobody implements, after batch 1's three `auth.handler` mocks and batch 3's `status: 'open'`. The new test uses the `getApi()` shape and fails against the pre-fix code. Also found by the rule, all the same #4127 shape (implemented, called, undeclared) and all now declared: IAuthService gains `api`, `getApi`, `isAuthGateActive` and `verifyMcpAccessToken`; IMetadataService gains `load` and `loadDiagnosed`. `getApi`'s return type is the EVIDENCED SUBSET — `getSession({headers})` and the three fields callers read — not a re-declaration of better-auth's handle, which belongs to that library. And the pattern's real root: the lookup facade returning `any` was re-declared in THREE places. Batches 1-3 typed `DomainHandlerDeps` and left `ActionExecutionDeps` and resolve-execution-context's `ResolveOptions` still saying `any` — so the copy that stayed untyped was the way around all the others, and it is where the auth reads lived. All three are typed now. Completing the interface: `getRequestKernelService` gets the same overload split (its one caller resolves the same `objectql` slot the `resolveService` fallback beside it does, so the two arms of one expression had different types), and share-links' `getEngine` loses a `Promise` return annotation — a THIRD erasure syntax after `: any` and `as any`, and one this AST rule cannot see. That residual is documented in the config. `getObjectQL` STAYS `any`, deliberately, with the reason recorded: it exists to reach ObjectQL's surface beyond IDataEngine (`registry`, `executeAction`), which has no contract. Typing it IDataEngine would be the comfortable-looking lie. Refs #4127 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015T5CeitwV1jXLvsL1A84tw --- .changeset/service-lookup-any-guard.md | 68 ++++++++++++++++++ eslint.config.mjs | 72 +++++++++++++++++++ packages/runtime/src/action-execution.ts | 33 ++++++++- packages/runtime/src/dispatcher-plugin.ts | 9 ++- .../runtime/src/domain-handler-registry.ts | 27 +++++++ packages/runtime/src/domains/actions.ts | 3 + packages/runtime/src/domains/mcp.ts | 2 - packages/runtime/src/domains/share-links.ts | 7 +- .../src/error-envelope.conformance.test.ts | 34 +++++++++ packages/runtime/src/http-dispatcher.ts | 37 ++++++++-- .../src/security/resolve-execution-context.ts | 27 ++++++- packages/spec/api-surface.json | 1 + packages/spec/src/contracts/auth-service.ts | 63 ++++++++++++++++ .../spec/src/contracts/metadata-service.ts | 38 ++++++++++ 14 files changed, 406 insertions(+), 15 deletions(-) create mode 100644 .changeset/service-lookup-any-guard.md diff --git a/.changeset/service-lookup-any-guard.md b/.changeset/service-lookup-any-guard.md new file mode 100644 index 0000000000..da014082c7 --- /dev/null +++ b/.changeset/service-lookup-any-guard.md @@ -0,0 +1,68 @@ +--- +"@objectstack/spec": minor +"@objectstack/runtime": patch +--- + +fix(runtime,spec): guard the service-lookup typing with a lint rule — which immediately found the project-membership gate not gating (#4127) + +Batch 4 of the #4127 gate. #4168/#4176/#4202 made a slot lookup return the +slot's contract. Nothing protected that: an `any` annotation on the **result** +switches the checking back off for that call site, silently, with no test +failing and no visual difference from code that has it. Three such sites already +existed and were found by grep — the same unrepeatable sweep this work replaced. + +**The rule** bans `: any` / `as any` on a `resolveService` / `getService` / +`getRequestKernelService` result. Slots with no written contract (`protocol`, +`mcp`, `kernel-resolver`, `scope-manager`) are exempted **by name, centrally**, +in `eslint.config.mjs` — not by inline disables, because `pnpm lint` runs +`--no-inline-config` and ignores those on purpose. The effect is the one worth +having: a deliberate gap is a reviewed line in one file, a careless one is a +build failure, and they stop looking identical in the code. + +**Its first run found a live fail-open.** `enforceProjectMembership` read the +session as `authService?.api?.getSession?.(…)` with no `getApi()` fallback — the +only one of the codebase's three `.api` readers without it. `plugin-auth` +registers `AuthManager`, which has **no `.api` member at all**. So the read +yielded `undefined`, `userId` stayed unset, and the function returned at its +"anonymous — upstream auth will decide" line **before ever querying +`sys_environment_member`**. A signed-in non-member passed the gate, on every +deployment with project scoping on — which is where the flag defaults to true. +Anonymous callers were still denied elsewhere (#2567/#3963), so this was +specifically the signed-in-non-member case. + +The existing test for that gate mocked auth as `{ api: { getSession } }` — the +legacy shape the shipped provider does not have — so it was green throughout. +That is the **fourth** test in this work line found encoding a contract nobody +implements, after batch 1's three `auth.handler` mocks and batch 3's +`status: 'open'`. The new test uses the `getApi()` shape and fails against the +pre-fix code. + +**Also found by the rule**, all the same #4127 shape (implemented, called, +undeclared) and all now declared: `IAuthService` gains `api`, `getApi`, +`isAuthGateActive` and `verifyMcpAccessToken`; `IMetadataService` gains `load` +and `loadDiagnosed`. `getApi`'s return type is the **evidenced subset** — +`getSession({headers})` and the three fields callers read — not a re-declaration +of better-auth's handle, which belongs to that library. + +**And the pattern's real root:** the lookup facade returning `any` was +re-declared in **three** places. Batches 1-3 typed `DomainHandlerDeps` and left +`ActionExecutionDeps` and `resolve-execution-context`'s `ResolveOptions` still +saying `any` — so the copy that stayed untyped was the way around all the +others, and it is where the auth reads lived. All three are typed now. + +Completing the interface: `getRequestKernelService` gets the same overload split +(its one caller resolves the same `objectql` slot the `resolveService` fallback +beside it does, so the two arms of one expression had different types), and +share-links' `getEngine` loses a `Promise` return annotation — a **third** +erasure syntax after `: any` and `as any`, and one this AST rule cannot see. +That residual is documented in the config. + +`getObjectQL` **stays** `any`, deliberately, with the reason recorded: it exists +to reach ObjectQL's surface beyond `IDataEngine` (`registry`, `executeAction`), +which has no contract. Typing it `IDataEngine` would be the comfortable-looking +lie. + +Verified: `@objectstack/runtime` **952 tests / 67 files**, `@objectstack/spec` +**7147 / 275**, plugin-auth **579**, rest **512**; `tsc --noEmit` on spec, +runtime, downstream-contract and all four examples; `pnpm lint` (with +`--no-inline-config`); all nine `check:*` gates. diff --git a/eslint.config.mjs b/eslint.config.mjs index d0f9bf3b7e..8740323df1 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -47,6 +47,29 @@ const DOMAIN_RULE_MESSAGE = 'instead of a bare `: Type` literal. The factory validates at parse time and a ' + 'broken value import fails loudly instead of degrading to `any` — see issue #2035.'; +// The dispatcher's service-lookup methods whose result carries the slot's +// contract (#4127). `getObjectQL` is NOT here: it reaches ObjectQL's surface +// beyond IDataEngine (`registry`, `executeAction`), which has no contract, so +// its `any` is correct and permanent until someone writes one. +const SLOT_LOOKUPS = ['resolveService', 'getService', 'getRequestKernelService'].join('|'); + +// Slots with no written contract. A lookup naming one of these legitimately +// yields `any`, so the rule exempts it BY NAME rather than by an inline +// disable — this repo lints with `--no-inline-config`, which ignores +// eslint-disable comments on purpose: exceptions belong in one reviewable +// place, not sprinkled through the code. Deleting a name from this list is how +// the exemption ends once that contract gets written. +const UNCONTRACTED_SLOTS = ['protocol', 'mcp', 'kernel-resolver', 'scope-manager'].join('|'); + +const SLOT_LOOKUP_ANY_MESSAGE = + 'Do not annotate a service-lookup result as `any` — the lookup already returns ' + + 'the slot\'s contract (#4168/#4176/#4202), and this switches that checking off ' + + 'for the call site while looking identical to code that has it. Every such ' + + 'annotation found so far was hiding a real gap, including a project-membership ' + + 'gate that silently stopped gating. If the slot genuinely has no contract, add ' + + 'its name to UNCONTRACTED_SLOTS in eslint.config.mjs with a note, so the ' + + 'exemption is reviewed once and visible in one place — see issue #4127.'; + export default [ { files: ['**/*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}'], @@ -155,4 +178,53 @@ export default [ ], }, }, + // issue #4127 — service-lookup `any` guard. #4168/#4176/#4202 made a slot + // lookup return the slot's contract, so a domain calling a method nobody + // declares is a compile error. An `any` annotation on the RESULT silently + // switches that back off for that call site: nothing fails, no test breaks, + // and the code looks exactly like the checked kind. Three such sites already + // existed and were found by grep, which is the sweep this work replaced — + // #4087 shipped for months because a sweep is not repeatable. + // + // The `any` is not always wrong, so the exemptions are declared above — + // by SLOT NAME, and centrally. That is deliberate: `pnpm lint` runs with + // `--no-inline-config`, so an `eslint-disable` comment would be ignored and + // the escape has to live in config anyway. The effect is the one worth + // having — a deliberate gap is a reviewed line in this file, a careless one + // is a build failure, and the two stop looking identical in the code. + // + // KNOWN RESIDUAL: a wrapper whose own return type is annotated + // (`const getEngine = async (): Promise => …resolveService(…)`) erases + // the slot type just as effectively, and this selector cannot see it — the + // annotation is on the enclosing function, not on the call. One such site + // existed (share-links `getEngine`, fixed in batch 4). Catching that shape + // needs type information, so it belongs to a typed-lint pass, not here. + { + files: ['packages/runtime/**/*.{ts,mts,cts}'], + ignores: ['**/node_modules/**', '**/dist/**'], + languageOptions: { + parser: tsParser, + parserOptions: { ecmaVersion: 'latest', sourceType: 'module' }, + }, + rules: { + 'no-restricted-syntax': ['error', + { + // `const svc: any = await deps.resolveService('auth', env)` + selector: + 'VariableDeclarator[id.typeAnnotation.typeAnnotation.type="TSAnyKeyword"]' + + `:has(CallExpression[callee.property.name=/^(${SLOT_LOOKUPS})$/]` + + `:not(:has(Literal[value=/^(${UNCONTRACTED_SLOTS})$/])))`, + message: SLOT_LOOKUP_ANY_MESSAGE, + }, + { + // `await deps.resolveService('security', env) as any` + selector: + 'TSAsExpression[typeAnnotation.type="TSAnyKeyword"]' + + `:has(CallExpression[callee.property.name=/^(${SLOT_LOOKUPS})$/]` + + `:not(:has(Literal[value=/^(${UNCONTRACTED_SLOTS})$/])))`, + message: SLOT_LOOKUP_ANY_MESSAGE, + }, + ], + }, + }, ]; diff --git a/packages/runtime/src/action-execution.ts b/packages/runtime/src/action-execution.ts index 0b614f0c11..6cab8f4952 100644 --- a/packages/runtime/src/action-execution.ts +++ b/packages/runtime/src/action-execution.ts @@ -17,6 +17,7 @@ import { validateActionParams, type ResolvedActionParam } from '@objectstack/spec/ui'; import type { ExecutionContext } from '@objectstack/spec/kernel'; +import type { ServiceSlotContract, ServiceSlotContracts } from '@objectstack/spec/contracts'; import { checkApiExposure } from './api-exposure.js'; import { GLOBAL_ACTION_OBJECT_KEY, @@ -57,8 +58,18 @@ function warnActionParamsOnce(key: string, message: string): void { console.warn(message); } -/** The dispatcher facilities the action subsystem may touch. */ +/** + * The dispatcher facilities the action subsystem may touch. + * + * [#4127 batch 4] `resolveService` is split the same way as the one on + * `DomainHandlerDeps`. This is a NARROWER re-declaration of the same facility + * and it kept returning `any` after the main one stopped — the third copy of + * the pattern, alongside `ResolveOptions` in security/resolve-execution-context. + * A lookup facade has to be typed everywhere it is re-declared, or the copy + * that still says `any` becomes the way around all the others. + */ export interface ActionExecutionDeps { + resolveService(name: K, environmentId?: string): Promise | undefined>; resolveService(name: string, environmentId?: string): any; getObjectQL(environmentId?: string): Promise; } @@ -329,7 +340,11 @@ export function headlessActionTypeError(deps: ActionExecutionDeps, action: any, */ export async function resolveAutomationService(deps: ActionExecutionDeps, envId?: string): Promise { try { - const svc: any = await deps.resolveService('automation', envId); + // [#4127 batch 4] Was `: any`, which voided the gate here. `execute` is + // declared on IAutomationService, so this needed no contract work — only + // for someone to notice, and three grep sweeps over `domains/*.ts` never + // reached this file. The lint rule did. + const svc = await deps.resolveService('automation', envId); return svc && typeof svc.execute === 'function' ? svc : null; } catch { return null; // no automation service on this kernel @@ -810,6 +825,9 @@ export async function invokeBusinessAction(deps: ActionExecutionDeps, } // ── script/body dispatch via the engine's executeAction ── + // [#4127] `executeAction` is + // ObjectQL's own surface, outside IDataEngine; `getObjectQL` exists to reach + // exactly that. Closing this needs ObjectQL's contract written, not a cast. const ql: any = await deps.getObjectQL(envId); if (!ql || typeof ql.executeAction !== 'function') { throw new Error('Data engine not available for action dispatch'); @@ -1060,7 +1078,16 @@ export async function resolveRouteActionDeclaration(deps: ActionExecutionDeps, let degraded = false; let reason: string | undefined; try { - const meta: any = await deps.resolveService('metadata', envId); + // [#4127 FINDING, batch 5] + // `metadata` IS contracted, so this `any` is not legitimate — but + // `loadDiagnosed` is not on IMetadataService, though MetadataManager + // implements it. Same shape as every gap this line of work has found: + // call site and implementation agree, the contract is what nobody wrote. + // Recorded rather than fixed here — adding it changes a public contract + // and belongs in the batch that adds the four undeclared auth members. + // [#4127 batch 4] `loadDiagnosed` is on IMetadataService now, so this + // reads the contract instead of guessing at it. + const meta = await deps.resolveService('metadata', envId); if (meta && typeof meta.loadDiagnosed === 'function') { const diag: any = await meta.loadDiagnosed('action', actionName); if (diag?.data && ownsRoute(diag.data)) return { action: diag.data, obj }; diff --git a/packages/runtime/src/dispatcher-plugin.ts b/packages/runtime/src/dispatcher-plugin.ts index af123d906c..7575e3e1f1 100644 --- a/packages/runtime/src/dispatcher-plugin.ts +++ b/packages/runtime/src/dispatcher-plugin.ts @@ -3,6 +3,7 @@ import { Plugin, PluginContext, IHttpServer } from '@objectstack/core'; import { looksLikeInternalErrorLeak, INTERNAL_ERROR_MESSAGE } from '@objectstack/types'; import { DispatcherErrorCode } from '@objectstack/spec/api'; +import type { IAuthService } from '@objectstack/spec/contracts'; import { HttpDispatcher, HttpDispatcherResult } from './http-dispatcher.js'; import { isServiceServeable } from './service-serveable.js'; import { validationFailureDetails, VALIDATION_FAILED_STATUS } from './validation-failure.js'; @@ -1140,7 +1141,13 @@ export function createDispatcherPlugin(config: DispatcherPluginConfig = {}): Plu // hits to protected routes are rejected upstream. const resolveRequestUser = async (headers: Record): Promise => { try { - const authService: any = ctx.getService('auth'); + // [#4127 batch 4] The KERNEL's `ctx.getService` is a + // different, still-untyped surface — typing it is its own + // change. Naming the contract in the cast is the point: the + // claim being made is the ledger's (`auth` -> IAuthService), + // written where a reader can check it, instead of `any`, + // which claims the same thing while saying nothing. + const authService = ctx.getService('auth') as IAuthService | undefined; if (!authService) return undefined; let api: any = authService.api; if (!api && typeof authService.getApi === 'function') { diff --git a/packages/runtime/src/domain-handler-registry.ts b/packages/runtime/src/domain-handler-registry.ts index 212f98edec..85cbd6616c 100644 --- a/packages/runtime/src/domain-handler-registry.ts +++ b/packages/runtime/src/domain-handler-registry.ts @@ -129,6 +129,25 @@ export interface DomainHandlerDeps { * (resolves the `objectql` service and returns it only when it exposes * `.registry`; null otherwise). The data-plane domains (/keys today, * /data /meta when they migrate) depend on this. + * + * [#4127 batch 4] **Deliberately still `any`, and this is the record of why.** + * It looks like it should be `IDataEngine` now — batch 3 evidenced + * `objectql: IDataEngine` and this method resolves exactly that slot. It is + * not, because this accessor exists to reach the surface `IDataEngine` does + * NOT cover: it gates on `.registry`, and its callers use `executeAction`. + * Neither is declared on the data-engine contract; `insert`, the third + * method they call, is. + * + * So the slot has two truths and both are real: ObjectQL implements + * `IDataEngine` (the plugin registers it as `data` saying exactly that), and + * ObjectQL is also wider than `IDataEngine`, with no contract written for + * the wider part. Typing this as `IDataEngine` would be the more + * comfortable-looking lie — it would force casts at `.registry` and + * `executeAction` and bury the gap under them. + * + * The concrete input for whoever writes ObjectQL's own contract: + * `registry` and `executeAction` are what the dispatcher actually needs + * from it beyond the data engine. */ getObjectQL(environmentId?: string): Promise; /** @@ -138,7 +157,15 @@ export interface DomainHandlerDeps { * share-links: the token row and the shared record sit next to the * `shareLinks` service's engine) read through this and fall back to * `resolveService` themselves. + * + * [#4127 batch 4] The third and last lookup path on this contract, split by + * the same rule as `resolveService`. It resolves the SAME slots off a + * different kernel, so a slot's contract cannot depend on which path + * reached it — and its one caller proves the point: `share-links` tries + * this path and falls back to `resolveService` for the same `'objectql'` + * slot, so before this the two arms of one expression had different types. */ + getRequestKernelService(name: K): Promise | undefined>; getRequestKernelService(name: string): Promise; /** Standard success envelope. */ success(data: any, meta?: any): { status: number; body: any }; diff --git a/packages/runtime/src/domains/actions.ts b/packages/runtime/src/domains/actions.ts index 2a1410b7f3..3f8349a19c 100644 --- a/packages/runtime/src/domains/actions.ts +++ b/packages/runtime/src/domains/actions.ts @@ -124,6 +124,9 @@ export async function handleActionsRequest(deps: DomainHandlerDeps, path: string } const projectQl: any = await deps.resolveProjectKernelObjectQL(_context); + // [#4127] Same as the + // action-execution site: `executeAction` is outside IDataEngine, and + // ObjectQL's wider surface has no contract yet. const ql: any = projectQl ?? await deps.getObjectQL(_context?.environmentId); if (!ql || typeof ql.executeAction !== 'function') { return { handled: true, response: deps.error('Data engine not available', 503) }; diff --git a/packages/runtime/src/domains/mcp.ts b/packages/runtime/src/domains/mcp.ts index 40840be0a6..e6cf3ce179 100644 --- a/packages/runtime/src/domains/mcp.ts +++ b/packages/runtime/src/domains/mcp.ts @@ -51,7 +51,6 @@ export async function handleMcpRequest(deps: DomainHandlerDeps, body: any, conte if (!isMcpServerEnabled()) { return { handled: true, response: deps.error('MCP server is not enabled for this environment', 404) }; } - const mcp: any = await deps.resolveService('mcp', context.environmentId); if (!mcp || typeof mcp.handleHttpRequest !== 'function') { return { handled: true, response: deps.error('MCP server is not available', 501) }; @@ -181,7 +180,6 @@ export async function handleMcpSkillRequest(deps: DomainHandlerDeps, method: str }, }; } - const mcp: any = await deps.resolveService('mcp', context.environmentId); if (!mcp || typeof mcp.renderSkill !== 'function') { return { handled: true, response: deps.error('MCP server is not available', 501) }; diff --git a/packages/runtime/src/domains/share-links.ts b/packages/runtime/src/domains/share-links.ts index 6cb0e192fd..0c1ebd344f 100644 --- a/packages/runtime/src/domains/share-links.ts +++ b/packages/runtime/src/domains/share-links.ts @@ -85,7 +85,12 @@ export async function handleShareLinksRequest( // request's RESOLVED (per-env) kernel first: `resolveService('objectql', // env)` can hand back a different (host/scoped) engine that lacks the // per-env rows. - const getEngine = async (): Promise => { + // [#4127 batch 4] The `Promise` return annotation was a THIRD way the + // slot type got erased, after `const x: any =` and `as any` in batches 2-3. + // Both arms resolve the same `objectql` slot and both are typed now; the + // wrapper's own annotation flattened them back to `any` on the way out. + // Inferred instead, so `engine.find` below is checked against IDataEngine. + const getEngine = async () => { try { const e = await deps.getRequestKernelService('objectql'); if (e) return e; diff --git a/packages/runtime/src/error-envelope.conformance.test.ts b/packages/runtime/src/error-envelope.conformance.test.ts index dacdebfbe0..713ab2b570 100644 --- a/packages/runtime/src/error-envelope.conformance.test.ts +++ b/packages/runtime/src/error-envelope.conformance.test.ts @@ -131,6 +131,40 @@ describe('#3842 — every dispatcher error exit answers in the declared envelope expect(error.details).toEqual({ environmentId: 'proj-private', userId: 'user-1' }); }); + // [#4127 batch 4] The case above mocks auth as `{ api: { getSession } }` — + // the LEGACY direct-mount shape. `plugin-auth` registers `AuthManager`, + // which has no `.api` at all and exposes `getApi()` instead. So this gate + // was green in tests and open in production: the session read yielded + // undefined, `userId` stayed unset, and the handler returned at its + // "anonymous — upstream auth will decide" line without ever querying + // `sys_environment_member`. A signed-in NON-MEMBER passed it, on every + // deployment with project scoping on. This pins the shipped shape. + it('denies a non-member when the auth service exposes getApi() rather than .api (the shipped shape)', async () => { + const ql = { find: vi.fn().mockResolvedValue([]) }; // no membership row + const kernel: any = { + context: { + getService: (name: string) => { + // No `.api` member at all — exactly like AuthManager. + if (name === 'auth') { + return { getApi: async () => ({ getSession: async () => ({ user: { id: 'user-1' } }) }) }; + } + if (name === 'objectql') return ql; + return null; + }, + }, + }; + const dispatcher = new HttpDispatcher(kernel, undefined, { enforceProjectMembership: true }); + const response = await (dispatcher as any).enforceProjectMembership( + { request: { headers: {} }, environmentId: 'proj-private' }, + '/api/v1/environments/proj-private/data/task', + ); + + const error = expectConformantError(response); + expect(error.code).toBe('PROJECT_MEMBERSHIP_REQUIRED'); + expect(error.details).toEqual({ environmentId: 'proj-private', userId: 'user-1' }); + expect(ql.find).toHaveBeenCalled(); // the membership query actually ran + }); + it('lifts a thrown error’s own code into the declared field', async () => { const thrown = Object.assign(new Error('publish backend unavailable'), { code: 'CONNECTOR_UPSTREAM_UNAVAILABLE', diff --git a/packages/runtime/src/http-dispatcher.ts b/packages/runtime/src/http-dispatcher.ts index 636b96798a..b070428cd9 100644 --- a/packages/runtime/src/http-dispatcher.ts +++ b/packages/runtime/src/http-dispatcher.ts @@ -235,7 +235,9 @@ export class HttpDispatcher { getObjectQL: (environmentId) => this.getObjectQLService(environmentId), // Reads off the per-request RESOLVED kernel (`this.kernel` is set by // dispatch() before any handler runs) — see the deps contract note. - getRequestKernelService: async (name) => { + // [#4127 batch 4] Annotated for the same reason as the two above: an + // arrow cannot be contextually typed against an overloaded signature. + getRequestKernelService: async (name: string) => { const k: any = this.kernel; return typeof k?.getServiceAsync === 'function' ? k.getServiceAsync(name) @@ -714,7 +716,13 @@ export class HttpDispatcher { private async enforceAuthGate(context: any, cleanPath: string): Promise { try { if (isAuthGateAllowlisted(cleanPath)) return null; - const authService: any = await this.resolveService('auth', context.environmentId); + // [#4127 FINDING, batch 5] + // `auth` IS contracted, so this `any` is not legitimate. + // `isAuthGateActive` is implemented (auth-manager.ts) and load-bearing + // — the whole auth gate turns on it — but IAuthService never declared + // it. `packages/rest` probes it the same way, so two independent + // callers agree on a member the contract does not mention. + const authService = await this.resolveService('auth', context.environmentId); if (!authService || typeof authService.isAuthGateActive !== 'function' || !authService.isAuthGateActive()) { return null; } @@ -770,8 +778,25 @@ export class HttpDispatcher { let userId: string | undefined; let activeOrganizationId: string | undefined; try { - const authService: any = await this.resolveService(CoreServiceName.enum.auth); - const sessionData = await authService?.api?.getSession?.({ + // [#4127 batch 4] This read was `authService?.api?.getSession?.(…)` + // with NO `getApi()` fallback — the only one of the three `.api` + // readers in the codebase without it (resolve-execution-context and + // dispatcher-plugin both have it, and their comments explain why: + // `.api` is the LEGACY direct mount, `getApi()` the lazy-plugin + // accessor). `plugin-auth` registers `AuthManager` as the `auth` + // service, and AuthManager has no `.api` at all. + // + // So against the shipped auth plugin this resolved to `undefined`, + // `userId` stayed unset, and the function returned at the + // "anonymous — upstream auth will decide" line BEFORE ever querying + // `sys_environment_member`. The project-membership gate did not + // gate: a signed-in NON-MEMBER passed it, on every deployment with + // project scoping on, which is where the flag defaults to true. + // Anonymous callers were still denied elsewhere (#2567/#3963), so + // this was specifically the signed-in non-member case. + const authService = await this.resolveService(CoreServiceName.enum.auth); + const api = authService?.api ?? (typeof authService?.getApi === 'function' ? await authService.getApi() : undefined); + const sessionData = await api?.getSession?.({ headers: context.request?.headers, }); userId = sessionData?.user?.id ?? sessionData?.session?.userId; @@ -1281,7 +1306,9 @@ export class HttpDispatcher { */ private async resolveActiveOrganizationId(context: HttpProtocolContext): Promise { try { - const authService: any = await this.resolveService(CoreServiceName.enum.auth); + // [#4127 FINDING, batch 5] + // Third `.api` reader on the auth service; see the note above. + const authService = await this.resolveService(CoreServiceName.enum.auth); const rawHeaders = context.request?.headers; let headers: any = rawHeaders; if (rawHeaders && typeof rawHeaders === 'object' && typeof (rawHeaders as any).get !== 'function') { diff --git a/packages/runtime/src/security/resolve-execution-context.ts b/packages/runtime/src/security/resolve-execution-context.ts index fc0d270421..93327a5188 100644 --- a/packages/runtime/src/security/resolve-execution-context.ts +++ b/packages/runtime/src/security/resolve-execution-context.ts @@ -20,6 +20,7 @@ */ import type { ExecutionContext } from '@objectstack/spec/kernel'; +import type { ServiceSlotContract, ServiceSlotContracts } from '@objectstack/spec/contracts'; import { scopesToAgentPermissionSets, MCP_OAUTH_SCOPE_ACTIONS } from '@objectstack/spec/ai'; import { preferredLocaleFromHeader } from '@objectstack/spec/system'; @@ -28,9 +29,21 @@ import { resolveLocalizationContext, } from '@objectstack/core'; +/** + * [#4127 batch 4] The lookup facade, typed by slot like the ones on + * `DomainHandlerDeps` and `ActionExecutionDeps`. This was the THIRD copy still + * returning `any`, and the one that mattered most: both of its `'auth'` callers + * read members — `verifyMcpAccessToken`, `api`, `getApi` — that nothing + * declared, on the path that decides who a request is. + */ +interface KernelServiceLookup { + (name: K): Promise | undefined>; + (name: string): Promise | any; +} + interface ResolveOptions { /** Function returning a service from the active kernel (or undefined). */ - getService: (name: string) => Promise | any; + getService: KernelServiceLookup; /** Function returning the data engine (ObjectQL) for the active scope. */ getQl: () => Promise | any; /** The raw incoming HTTP request (Fetch Request, Node IncomingMessage, …). */ @@ -102,7 +115,12 @@ export async function resolveExecutionContext(opts: ResolveOptions): Promise Promise | any` + // is a second, smaller instance of the same problem — a lookup facade + // that returns `any` — and typing it is part of that batch too. + const authService = await opts.getService('auth'); const verified = await authService?.verifyMcpAccessToken?.(jwtBearer); if (verified?.userId && Array.isArray(verified.scopes)) { oauthPrincipal = verified; @@ -118,7 +136,10 @@ export async function resolveExecutionContext(opts: ResolveOptions): Promise { try { - const authService: any = await opts.getService('auth'); + // [#4127 FINDING, batch 5] + // Reaches BOTH undeclared shapes in two lines — `.api` and the `getApi()` + // fallback — which is the clearest statement of the gap in the codebase. + const authService = await opts.getService('auth'); let api: any = authService?.api; if (!api && typeof authService?.getApi === 'function') api = await authService.getApi(); return await api?.getSession?.({ headers: h }); diff --git a/packages/spec/api-surface.json b/packages/spec/api-surface.json index d424f6e66d..be8b62a3fd 100644 --- a/packages/spec/api-surface.json +++ b/packages/spec/api-surface.json @@ -3567,6 +3567,7 @@ "AudienceBindingSuggestionSync (interface)", "AuthResult (interface)", "AuthSession (interface)", + "AuthSessionApi (interface)", "AuthUser (interface)", "AutomationContext (interface)", "AutomationResult (interface)", diff --git a/packages/spec/src/contracts/auth-service.ts b/packages/spec/src/contracts/auth-service.ts index e7db6c7cf5..e835e0b70e 100644 --- a/packages/spec/src/contracts/auth-service.ts +++ b/packages/spec/src/contracts/auth-service.ts @@ -116,4 +116,67 @@ export interface IAuthService { * @returns The metadata URL, or `null` when the OAuth track is off */ getMcpResourceMetadataUrl?(): string | null; + + /** + * The underlying session API, when the provider mounts it DIRECTLY on + * itself (the legacy shape). Prefer {@link getApi} — the shipped + * `plugin-auth` registers an `AuthManager`, which has no `api` member at + * all, so a caller that reads only this one gets `undefined` on every + * current deployment. + * + * [#4127 batch 4] Declared because reading it without the `getApi()` + * fallback silently disabled the project-membership gate: the read yielded + * `undefined`, the caller treated that as "anonymous", and + * `sys_environment_member` was never queried. Both shapes are on the + * contract now so the two-step read is a checked expression rather than a + * pair of guesses. + */ + api?: AuthSessionApi; + + /** + * Resolve the session API, creating the underlying auth instance on first + * use (the lazy-plugin shape `AuthManager` implements). This is the + * accessor callers should reach for; {@link api} is its legacy twin. + */ + getApi?(): Promise; + + /** + * Whether the deployment's auth gate is live, i.e. whether unauthenticated + * requests should be challenged at all. Synchronous and cheap by contract — + * the implementation keeps a TTL-refreshed snapshot precisely so every + * request can ask. + * + * [#4127 batch 4] Implemented by `AuthManager` and probed identically by + * the dispatcher and `packages/rest` — two independent callers agreeing on + * a member the contract never mentioned. + */ + isAuthGateActive?(): boolean; + + /** + * Verify an OAuth 2.1 access token issued by this deployment's embedded + * authorization server, for the MCP surface (#2698). Resolves to the + * token's principal, or a falsy value when the token is + * unknown/expired/revoked or carries the wrong audience — callers fail + * CLOSED on anything but a verified result. + * + * [#4127 batch 4] Implemented by `AuthManager`; the execution-context + * resolver has always called it through `any`. + */ + verifyMcpAccessToken?(token: string): Promise<{ userId: string; scopes: string[]; clientId?: string } | undefined>; +} + +/** + * The slice of the session API the platform actually uses. + * + * [#4127 batch 4] Deliberately NOT a re-declaration of better-auth's handle, + * which is far wider and belongs to that library. Every dispatcher-side reader + * calls exactly `getSession({ headers })` and reads exactly the fields below, + * so that is what is declared. Widening this is for whoever needs more, with + * the call site to prove it. + */ +export interface AuthSessionApi { + getSession?(input: { headers: unknown }): Promise<{ + user?: { id?: string }; + session?: { userId?: string; activeOrganizationId?: string }; + } | undefined>; } diff --git a/packages/spec/src/contracts/metadata-service.ts b/packages/spec/src/contracts/metadata-service.ts index f03b4cfb61..71c6cba803 100644 --- a/packages/spec/src/contracts/metadata-service.ts +++ b/packages/spec/src/contracts/metadata-service.ts @@ -485,4 +485,42 @@ export interface IMetadataService { * @returns Diff result with changes */ diff?(type: string, name: string, version1: number, version2: number): Promise; + + /** + * Load one metadata item through the registered loaders, or `null`. + * + * [#4127 batch 4] Declared alongside {@link loadDiagnosed}, which the same + * call site reaches for first. `null` here is AMBIGUOUS by construction — + * `MetadataManager` defines this as `(await loadDiagnosed(…)).data`, so a + * miss and a total loader outage are the same answer. Prefer + * `loadDiagnosed` wherever the difference could change a decision. + */ + load?( + type: string, + name: string, + options?: Record, + ): Promise; + + /** + * Load one metadata item, and say whether the answer can be trusted as + * complete. A MISS and an OUTAGE are different facts with opposite security + * meanings — a loader that throws is warn-logged and skipped, so a plain + * lookup cannot tell "this action does not exist" from "the store holding + * it is down" (ADR-0110 D3). + * + * `degraded` is true when at least one loader failed, with their messages + * in `errors`. A caller deciding whether something is ABSENT must check it: + * treating a degraded read as an absence is how an outage becomes an + * authorization answer. + * + * [#4127 batch 4] Implemented by `MetadataManager` — which defines plain + * `load` as `(await loadDiagnosed(…)).data` — and reached by the action + * resolver through `any`. Declared so the callers that need the + * distinction can see that it exists. + */ + loadDiagnosed?( + type: string, + name: string, + options?: Record, + ): Promise<{ data: T | null; degraded: boolean; errors: string[] }>; } From 07368921e004ccf7e72758227a010d98ca4c8f81 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 30 Jul 2026 16:14:24 +0000 Subject: [PATCH 2/2] docs(kernel): the auth and metadata contract pages list the members the contracts actually have (#4127) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both pages reproduce their interface source, and both had fallen behind. The auth page stopped at `getCurrentUser` and still called the interface "intentionally minimal" — it was already missing the two MCP members batch 2 added, which I did not catch when that shipped, and this batch added four more. The metadata page was missing `load`/`loadDiagnosed`. Documented with the part a caller can get wrong, not just the signatures: `api` vs `getApi()` (the shipped provider has no `api` member, so reading only that one yields undefined everywhere — the fail-open this batch fixed), and `load` vs `loadDiagnosed` (a null from `load` means either absent or every loader down, and treating the second as the first turns an outage into an authorization answer). Refs #4127 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_015T5CeitwV1jXLvsL1A84tw --- .../docs/kernel/contracts/auth-service.mdx | 25 +++++++++++++++++- .../kernel/contracts/metadata-service.mdx | 26 +++++++++++++++++++ 2 files changed, 50 insertions(+), 1 deletion(-) diff --git a/content/docs/kernel/contracts/auth-service.mdx b/content/docs/kernel/contracts/auth-service.mdx index 26d8963c94..a08d7993b1 100644 --- a/content/docs/kernel/contracts/auth-service.mdx +++ b/content/docs/kernel/contracts/auth-service.mdx @@ -29,10 +29,33 @@ export interface IAuthService { /** Resolve the current user from a request. Optional. */ getCurrentUser?(request: Request): Promise; + + /** The session API, when the provider mounts it directly (legacy shape). Optional. */ + api?: AuthSessionApi; + + /** Resolve the session API, creating the auth instance on first use. Optional. */ + getApi?(): Promise; + + /** Whether this deployment's auth gate is live. Optional. */ + isAuthGateActive?(): boolean; + + /** Verify an OAuth 2.1 access token from the embedded AS (MCP surface). Optional. */ + verifyMcpAccessToken?(token: string): Promise<{ userId: string; scopes: string[]; clientId?: string } | undefined>; + + /** The MCP resource identifier (RFC 8707 `resource` / token `aud`). Optional. */ + getMcpResourceUrl?(): string; + + /** RFC 9728 protected-resource metadata URL, or `null` when OAuth is off. Optional. */ + getMcpResourceMetadataUrl?(): string | null; } ``` -The interface is intentionally minimal: it follows the Dependency Inversion Principle so that plugins depend on the contract rather than a concrete provider. Concrete implementations (better-auth, custom, LDAP, etc.) supply the actual logic. `handleRequest` works directly with the standard `Request`/`Response` types; `logout` and `getCurrentUser` are optional. +The two required members are the core: it follows the Dependency Inversion Principle so that plugins depend on the contract rather than a concrete provider. Concrete implementations (better-auth, custom, LDAP, etc.) supply the actual logic, and `handleRequest` works directly with the standard `Request`/`Response` types. + +Everything after `verify` is optional, and each optional member describes a capability a provider may or may not have — a caller must probe before use. Two pairs are worth reading together: + +- **`api` and `getApi()`** are the same session handle by two routes. `api` is the legacy direct mount; `getApi()` is the lazy accessor, and it is the one to prefer — the shipped `plugin-auth` registers an `AuthManager`, which has **no `api` member at all**, so a caller that reads only `api` gets `undefined` on every current deployment. Read `api ?? await getApi?.()`. +- **`getMcpResourceUrl()` and `getMcpResourceMetadataUrl()`** both describe the MCP OAuth surface; the second returns `null` (rather than being absent) when the OAuth track is off for the deployment. --- diff --git a/content/docs/kernel/contracts/metadata-service.mdx b/content/docs/kernel/contracts/metadata-service.mdx index 3dc7f924b8..eaa791400f 100644 --- a/content/docs/kernel/contracts/metadata-service.mdx +++ b/content/docs/kernel/contracts/metadata-service.mdx @@ -40,6 +40,11 @@ export interface IMetadataService { getObject(name: string): Promise; listObjects(): Promise; + // Loader reads (optional) + load?(type: string, name: string, options?): Promise; + loadDiagnosed?(type: string, name: string, options?): + Promise<{ data: T | null; degraded: boolean; errors: string[] }>; + // Convenience accessors for UI metadata (optional) getView?(name: string): Promise; listViews?(object?: string): Promise; @@ -112,6 +117,27 @@ const has = await metadataService.exists('object', 'task'); `get` / `getObject` resolve to `undefined` (not `null`) when an item does not exist. +### load / loadDiagnosed + +Both read one item through the registered loaders. The difference is what a +missing answer means. + +`load` returns `null` for **both** "no loader has this item" and "every loader +failed" — a loader that throws is warn-logged and skipped, so the two collapse +into the same value. `loadDiagnosed` returns the same data plus `degraded` and +`errors`, which is what tells them apart (ADR-0110 D3). + +Reach for `loadDiagnosed` whenever the difference could change a decision. +Treating a degraded read as an absence is how a store being down turns into an +authorization answer. + +```typescript +const { data, degraded, errors } = await metadataService.loadDiagnosed?.('action', name) ?? {}; +if (degraded) { + // The store is unreachable — do NOT conclude the action does not exist. +} +``` + ### register / unregister `register` saves (creates or replaces) the full definition for a `(type, name)`.