Skip to content

Commit 65a3a84

Browse files
authored
fix(runtime,spec): guard the service-lookup typing with a lint rule — which found the project-membership gate not gating (#4127) (#4214)
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<any>` 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. The auth and metadata contract doc pages are brought back in step with the interfaces — the auth page still called itself "intentionally minimal" while missing six members, two of them from batch 2. Refs #4127
1 parent 6c87cc9 commit 65a3a84

16 files changed

Lines changed: 456 additions & 16 deletions
Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,68 @@
1+
---
2+
"@objectstack/spec": minor
3+
"@objectstack/runtime": patch
4+
---
5+
6+
fix(runtime,spec): guard the service-lookup typing with a lint rule — which immediately found the project-membership gate not gating (#4127)
7+
8+
Batch 4 of the #4127 gate. #4168/#4176/#4202 made a slot lookup return the
9+
slot's contract. Nothing protected that: an `any` annotation on the **result**
10+
switches the checking back off for that call site, silently, with no test
11+
failing and no visual difference from code that has it. Three such sites already
12+
existed and were found by grep — the same unrepeatable sweep this work replaced.
13+
14+
**The rule** bans `: any` / `as any` on a `resolveService` / `getService` /
15+
`getRequestKernelService` result. Slots with no written contract (`protocol`,
16+
`mcp`, `kernel-resolver`, `scope-manager`) are exempted **by name, centrally**,
17+
in `eslint.config.mjs` — not by inline disables, because `pnpm lint` runs
18+
`--no-inline-config` and ignores those on purpose. The effect is the one worth
19+
having: a deliberate gap is a reviewed line in one file, a careless one is a
20+
build failure, and they stop looking identical in the code.
21+
22+
**Its first run found a live fail-open.** `enforceProjectMembership` read the
23+
session as `authService?.api?.getSession?.(…)` with no `getApi()` fallback — the
24+
only one of the codebase's three `.api` readers without it. `plugin-auth`
25+
registers `AuthManager`, which has **no `.api` member at all**. So the read
26+
yielded `undefined`, `userId` stayed unset, and the function returned at its
27+
"anonymous — upstream auth will decide" line **before ever querying
28+
`sys_environment_member`**. A signed-in non-member passed the gate, on every
29+
deployment with project scoping on — which is where the flag defaults to true.
30+
Anonymous callers were still denied elsewhere (#2567/#3963), so this was
31+
specifically the signed-in-non-member case.
32+
33+
The existing test for that gate mocked auth as `{ api: { getSession } }` — the
34+
legacy shape the shipped provider does not have — so it was green throughout.
35+
That is the **fourth** test in this work line found encoding a contract nobody
36+
implements, after batch 1's three `auth.handler` mocks and batch 3's
37+
`status: 'open'`. The new test uses the `getApi()` shape and fails against the
38+
pre-fix code.
39+
40+
**Also found by the rule**, all the same #4127 shape (implemented, called,
41+
undeclared) and all now declared: `IAuthService` gains `api`, `getApi`,
42+
`isAuthGateActive` and `verifyMcpAccessToken`; `IMetadataService` gains `load`
43+
and `loadDiagnosed`. `getApi`'s return type is the **evidenced subset**
44+
`getSession({headers})` and the three fields callers read — not a re-declaration
45+
of better-auth's handle, which belongs to that library.
46+
47+
**And the pattern's real root:** the lookup facade returning `any` was
48+
re-declared in **three** places. Batches 1-3 typed `DomainHandlerDeps` and left
49+
`ActionExecutionDeps` and `resolve-execution-context`'s `ResolveOptions` still
50+
saying `any` — so the copy that stayed untyped was the way around all the
51+
others, and it is where the auth reads lived. All three are typed now.
52+
53+
Completing the interface: `getRequestKernelService` gets the same overload split
54+
(its one caller resolves the same `objectql` slot the `resolveService` fallback
55+
beside it does, so the two arms of one expression had different types), and
56+
share-links' `getEngine` loses a `Promise<any>` return annotation — a **third**
57+
erasure syntax after `: any` and `as any`, and one this AST rule cannot see.
58+
That residual is documented in the config.
59+
60+
`getObjectQL` **stays** `any`, deliberately, with the reason recorded: it exists
61+
to reach ObjectQL's surface beyond `IDataEngine` (`registry`, `executeAction`),
62+
which has no contract. Typing it `IDataEngine` would be the comfortable-looking
63+
lie.
64+
65+
Verified: `@objectstack/runtime` **952 tests / 67 files**, `@objectstack/spec`
66+
**7147 / 275**, plugin-auth **579**, rest **512**; `tsc --noEmit` on spec,
67+
runtime, downstream-contract and all four examples; `pnpm lint` (with
68+
`--no-inline-config`); all nine `check:*` gates.

content/docs/kernel/contracts/auth-service.mdx

Lines changed: 24 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,10 +29,33 @@ export interface IAuthService {
2929

3030
/** Resolve the current user from a request. Optional. */
3131
getCurrentUser?(request: Request): Promise<AuthUser | undefined>;
32+
33+
/** The session API, when the provider mounts it directly (legacy shape). Optional. */
34+
api?: AuthSessionApi;
35+
36+
/** Resolve the session API, creating the auth instance on first use. Optional. */
37+
getApi?(): Promise<AuthSessionApi | undefined>;
38+
39+
/** Whether this deployment's auth gate is live. Optional. */
40+
isAuthGateActive?(): boolean;
41+
42+
/** Verify an OAuth 2.1 access token from the embedded AS (MCP surface). Optional. */
43+
verifyMcpAccessToken?(token: string): Promise<{ userId: string; scopes: string[]; clientId?: string } | undefined>;
44+
45+
/** The MCP resource identifier (RFC 8707 `resource` / token `aud`). Optional. */
46+
getMcpResourceUrl?(): string;
47+
48+
/** RFC 9728 protected-resource metadata URL, or `null` when OAuth is off. Optional. */
49+
getMcpResourceMetadataUrl?(): string | null;
3250
}
3351
```
3452

35-
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.
53+
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.
54+
55+
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:
56+
57+
- **`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?.()`.
58+
- **`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.
3659

3760
---
3861

content/docs/kernel/contracts/metadata-service.mdx

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,11 @@ export interface IMetadataService {
4040
getObject(name: string): Promise<unknown | undefined>;
4141
listObjects(): Promise<unknown[]>;
4242

43+
// Loader reads (optional)
44+
load?<T>(type: string, name: string, options?): Promise<T | null>;
45+
loadDiagnosed?<T>(type: string, name: string, options?):
46+
Promise<{ data: T | null; degraded: boolean; errors: string[] }>;
47+
4348
// Convenience accessors for UI metadata (optional)
4449
getView?(name: string): Promise<unknown | undefined>;
4550
listViews?(object?: string): Promise<unknown[]>;
@@ -112,6 +117,27 @@ const has = await metadataService.exists('object', 'task');
112117
`get` / `getObject` resolve to `undefined` (not `null`) when an item does not exist.
113118
</Callout>
114119

120+
### load / loadDiagnosed
121+
122+
Both read one item through the registered loaders. The difference is what a
123+
missing answer means.
124+
125+
`load` returns `null` for **both** "no loader has this item" and "every loader
126+
failed" — a loader that throws is warn-logged and skipped, so the two collapse
127+
into the same value. `loadDiagnosed` returns the same data plus `degraded` and
128+
`errors`, which is what tells them apart (ADR-0110 D3).
129+
130+
Reach for `loadDiagnosed` whenever the difference could change a decision.
131+
Treating a degraded read as an absence is how a store being down turns into an
132+
authorization answer.
133+
134+
```typescript
135+
const { data, degraded, errors } = await metadataService.loadDiagnosed?.('action', name) ?? {};
136+
if (degraded) {
137+
// The store is unreachable — do NOT conclude the action does not exist.
138+
}
139+
```
140+
115141
### register / unregister
116142

117143
`register` saves (creates or replaces) the full definition for a `(type, name)`.

eslint.config.mjs

Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,29 @@ const DOMAIN_RULE_MESSAGE =
4747
'instead of a bare `: Type` literal. The factory validates at parse time and a ' +
4848
'broken value import fails loudly instead of degrading to `any` — see issue #2035.';
4949

50+
// The dispatcher's service-lookup methods whose result carries the slot's
51+
// contract (#4127). `getObjectQL` is NOT here: it reaches ObjectQL's surface
52+
// beyond IDataEngine (`registry`, `executeAction`), which has no contract, so
53+
// its `any` is correct and permanent until someone writes one.
54+
const SLOT_LOOKUPS = ['resolveService', 'getService', 'getRequestKernelService'].join('|');
55+
56+
// Slots with no written contract. A lookup naming one of these legitimately
57+
// yields `any`, so the rule exempts it BY NAME rather than by an inline
58+
// disable — this repo lints with `--no-inline-config`, which ignores
59+
// eslint-disable comments on purpose: exceptions belong in one reviewable
60+
// place, not sprinkled through the code. Deleting a name from this list is how
61+
// the exemption ends once that contract gets written.
62+
const UNCONTRACTED_SLOTS = ['protocol', 'mcp', 'kernel-resolver', 'scope-manager'].join('|');
63+
64+
const SLOT_LOOKUP_ANY_MESSAGE =
65+
'Do not annotate a service-lookup result as `any` — the lookup already returns ' +
66+
'the slot\'s contract (#4168/#4176/#4202), and this switches that checking off ' +
67+
'for the call site while looking identical to code that has it. Every such ' +
68+
'annotation found so far was hiding a real gap, including a project-membership ' +
69+
'gate that silently stopped gating. If the slot genuinely has no contract, add ' +
70+
'its name to UNCONTRACTED_SLOTS in eslint.config.mjs with a note, so the ' +
71+
'exemption is reviewed once and visible in one place — see issue #4127.';
72+
5073
export default [
5174
{
5275
files: ['**/*.{ts,tsx,mts,cts,js,jsx,mjs,cjs}'],
@@ -155,4 +178,53 @@ export default [
155178
],
156179
},
157180
},
181+
// issue #4127 — service-lookup `any` guard. #4168/#4176/#4202 made a slot
182+
// lookup return the slot's contract, so a domain calling a method nobody
183+
// declares is a compile error. An `any` annotation on the RESULT silently
184+
// switches that back off for that call site: nothing fails, no test breaks,
185+
// and the code looks exactly like the checked kind. Three such sites already
186+
// existed and were found by grep, which is the sweep this work replaced —
187+
// #4087 shipped for months because a sweep is not repeatable.
188+
//
189+
// The `any` is not always wrong, so the exemptions are declared above —
190+
// by SLOT NAME, and centrally. That is deliberate: `pnpm lint` runs with
191+
// `--no-inline-config`, so an `eslint-disable` comment would be ignored and
192+
// the escape has to live in config anyway. The effect is the one worth
193+
// having — a deliberate gap is a reviewed line in this file, a careless one
194+
// is a build failure, and the two stop looking identical in the code.
195+
//
196+
// KNOWN RESIDUAL: a wrapper whose own return type is annotated
197+
// (`const getEngine = async (): Promise<any> => …resolveService(…)`) erases
198+
// the slot type just as effectively, and this selector cannot see it — the
199+
// annotation is on the enclosing function, not on the call. One such site
200+
// existed (share-links `getEngine`, fixed in batch 4). Catching that shape
201+
// needs type information, so it belongs to a typed-lint pass, not here.
202+
{
203+
files: ['packages/runtime/**/*.{ts,mts,cts}'],
204+
ignores: ['**/node_modules/**', '**/dist/**'],
205+
languageOptions: {
206+
parser: tsParser,
207+
parserOptions: { ecmaVersion: 'latest', sourceType: 'module' },
208+
},
209+
rules: {
210+
'no-restricted-syntax': ['error',
211+
{
212+
// `const svc: any = await deps.resolveService('auth', env)`
213+
selector:
214+
'VariableDeclarator[id.typeAnnotation.typeAnnotation.type="TSAnyKeyword"]' +
215+
`:has(CallExpression[callee.property.name=/^(${SLOT_LOOKUPS})$/]` +
216+
`:not(:has(Literal[value=/^(${UNCONTRACTED_SLOTS})$/])))`,
217+
message: SLOT_LOOKUP_ANY_MESSAGE,
218+
},
219+
{
220+
// `await deps.resolveService('security', env) as any`
221+
selector:
222+
'TSAsExpression[typeAnnotation.type="TSAnyKeyword"]' +
223+
`:has(CallExpression[callee.property.name=/^(${SLOT_LOOKUPS})$/]` +
224+
`:not(:has(Literal[value=/^(${UNCONTRACTED_SLOTS})$/])))`,
225+
message: SLOT_LOOKUP_ANY_MESSAGE,
226+
},
227+
],
228+
},
229+
},
158230
];

packages/runtime/src/action-execution.ts

Lines changed: 30 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@
1717

1818
import { validateActionParams, type ResolvedActionParam } from '@objectstack/spec/ui';
1919
import type { ExecutionContext } from '@objectstack/spec/kernel';
20+
import type { ServiceSlotContract, ServiceSlotContracts } from '@objectstack/spec/contracts';
2021
import { checkApiExposure } from './api-exposure.js';
2122
import {
2223
GLOBAL_ACTION_OBJECT_KEY,
@@ -57,8 +58,18 @@ function warnActionParamsOnce(key: string, message: string): void {
5758
console.warn(message);
5859
}
5960

60-
/** The dispatcher facilities the action subsystem may touch. */
61+
/**
62+
* The dispatcher facilities the action subsystem may touch.
63+
*
64+
* [#4127 batch 4] `resolveService` is split the same way as the one on
65+
* `DomainHandlerDeps`. This is a NARROWER re-declaration of the same facility
66+
* and it kept returning `any` after the main one stopped — the third copy of
67+
* the pattern, alongside `ResolveOptions` in security/resolve-execution-context.
68+
* A lookup facade has to be typed everywhere it is re-declared, or the copy
69+
* that still says `any` becomes the way around all the others.
70+
*/
6171
export interface ActionExecutionDeps {
72+
resolveService<K extends keyof ServiceSlotContracts>(name: K, environmentId?: string): Promise<ServiceSlotContract<K> | undefined>;
6273
resolveService(name: string, environmentId?: string): any;
6374
getObjectQL(environmentId?: string): Promise<any>;
6475
}
@@ -329,7 +340,11 @@ export function headlessActionTypeError(deps: ActionExecutionDeps, action: any,
329340
*/
330341
export async function resolveAutomationService(deps: ActionExecutionDeps, envId?: string): Promise<any | null> {
331342
try {
332-
const svc: any = await deps.resolveService('automation', envId);
343+
// [#4127 batch 4] Was `: any`, which voided the gate here. `execute` is
344+
// declared on IAutomationService, so this needed no contract work — only
345+
// for someone to notice, and three grep sweeps over `domains/*.ts` never
346+
// reached this file. The lint rule did.
347+
const svc = await deps.resolveService('automation', envId);
333348
return svc && typeof svc.execute === 'function' ? svc : null;
334349
} catch {
335350
return null; // no automation service on this kernel
@@ -810,6 +825,9 @@ export async function invokeBusinessAction(deps: ActionExecutionDeps,
810825
}
811826

812827
// ── script/body dispatch via the engine's executeAction ──
828+
// [#4127] `executeAction` is
829+
// ObjectQL's own surface, outside IDataEngine; `getObjectQL` exists to reach
830+
// exactly that. Closing this needs ObjectQL's contract written, not a cast.
813831
const ql: any = await deps.getObjectQL(envId);
814832
if (!ql || typeof ql.executeAction !== 'function') {
815833
throw new Error('Data engine not available for action dispatch');
@@ -1060,7 +1078,16 @@ export async function resolveRouteActionDeclaration(deps: ActionExecutionDeps,
10601078
let degraded = false;
10611079
let reason: string | undefined;
10621080
try {
1063-
const meta: any = await deps.resolveService('metadata', envId);
1081+
// [#4127 FINDING, batch 5]
1082+
// `metadata` IS contracted, so this `any` is not legitimate — but
1083+
// `loadDiagnosed` is not on IMetadataService, though MetadataManager
1084+
// implements it. Same shape as every gap this line of work has found:
1085+
// call site and implementation agree, the contract is what nobody wrote.
1086+
// Recorded rather than fixed here — adding it changes a public contract
1087+
// and belongs in the batch that adds the four undeclared auth members.
1088+
// [#4127 batch 4] `loadDiagnosed` is on IMetadataService now, so this
1089+
// reads the contract instead of guessing at it.
1090+
const meta = await deps.resolveService('metadata', envId);
10641091
if (meta && typeof meta.loadDiagnosed === 'function') {
10651092
const diag: any = await meta.loadDiagnosed('action', actionName);
10661093
if (diag?.data && ownsRoute(diag.data)) return { action: diag.data, obj };

packages/runtime/src/dispatcher-plugin.ts

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
import { Plugin, PluginContext, IHttpServer } from '@objectstack/core';
44
import { looksLikeInternalErrorLeak, INTERNAL_ERROR_MESSAGE } from '@objectstack/types';
55
import { DispatcherErrorCode } from '@objectstack/spec/api';
6+
import type { IAuthService } from '@objectstack/spec/contracts';
67
import { HttpDispatcher, HttpDispatcherResult } from './http-dispatcher.js';
78
import { isServiceServeable } from './service-serveable.js';
89
import { validationFailureDetails, VALIDATION_FAILED_STATUS } from './validation-failure.js';
@@ -1140,7 +1141,13 @@ export function createDispatcherPlugin(config: DispatcherPluginConfig = {}): Plu
11401141
// hits to protected routes are rejected upstream.
11411142
const resolveRequestUser = async (headers: Record<string, any>): Promise<any | undefined> => {
11421143
try {
1143-
const authService: any = ctx.getService('auth');
1144+
// [#4127 batch 4] The KERNEL's `ctx.getService` is a
1145+
// different, still-untyped surface — typing it is its own
1146+
// change. Naming the contract in the cast is the point: the
1147+
// claim being made is the ledger's (`auth` -> IAuthService),
1148+
// written where a reader can check it, instead of `any`,
1149+
// which claims the same thing while saying nothing.
1150+
const authService = ctx.getService('auth') as IAuthService | undefined;
11441151
if (!authService) return undefined;
11451152
let api: any = authService.api;
11461153
if (!api && typeof authService.getApi === 'function') {

0 commit comments

Comments
 (0)