Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 68 additions & 0 deletions .changeset/service-lookup-any-guard.md
Original file line number Diff line number Diff line change
@@ -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<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.

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.
25 changes: 24 additions & 1 deletion content/docs/kernel/contracts/auth-service.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -29,10 +29,33 @@ export interface IAuthService {

/** Resolve the current user from a request. Optional. */
getCurrentUser?(request: Request): Promise<AuthUser | undefined>;

/** 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<AuthSessionApi | undefined>;

/** 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.

---

Expand Down
26 changes: 26 additions & 0 deletions content/docs/kernel/contracts/metadata-service.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,11 @@ export interface IMetadataService {
getObject(name: string): Promise<unknown | undefined>;
listObjects(): Promise<unknown[]>;

// Loader reads (optional)
load?<T>(type: string, name: string, options?): Promise<T | null>;
loadDiagnosed?<T>(type: string, name: string, options?):
Promise<{ data: T | null; degraded: boolean; errors: string[] }>;

// Convenience accessors for UI metadata (optional)
getView?(name: string): Promise<unknown | undefined>;
listViews?(object?: string): Promise<unknown[]>;
Expand Down Expand Up @@ -112,6 +117,27 @@ const has = await metadataService.exists('object', 'task');
`get` / `getObject` resolve to `undefined` (not `null`) when an item does not exist.
</Callout>

### 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)`.
Expand Down
72 changes: 72 additions & 0 deletions eslint.config.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -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}'],
Expand Down Expand Up @@ -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<any> => …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,
},
],
},
},
];
33 changes: 30 additions & 3 deletions packages/runtime/src/action-execution.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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<K extends keyof ServiceSlotContracts>(name: K, environmentId?: string): Promise<ServiceSlotContract<K> | undefined>;
resolveService(name: string, environmentId?: string): any;
getObjectQL(environmentId?: string): Promise<any>;
}
Expand Down Expand Up @@ -329,7 +340,11 @@ export function headlessActionTypeError(deps: ActionExecutionDeps, action: any,
*/
export async function resolveAutomationService(deps: ActionExecutionDeps, envId?: string): Promise<any | null> {
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
Expand Down Expand Up @@ -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');
Expand Down Expand Up @@ -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 };
Expand Down
9 changes: 8 additions & 1 deletion packages/runtime/src/dispatcher-plugin.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -1140,7 +1141,13 @@ export function createDispatcherPlugin(config: DispatcherPluginConfig = {}): Plu
// hits to protected routes are rejected upstream.
const resolveRequestUser = async (headers: Record<string, any>): Promise<any | undefined> => {
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') {
Expand Down
Loading
Loading