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/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)`. 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[] }>; }