fix(spec,runtime): 槽契约账本越过 CoreServiceName，/security 不再把未校验输入交给安全服务 (#4127)

.changeset/slot-contract-ledger-beyond-the-enum.md

@@ -0,0 +1,73 @@
+---
+"@objectstack/spec": minor
+"@objectstack/runtime": patch
+---
+
+fix(spec,runtime): the slot→contract ledger extends past `CoreServiceName`, and `/security` stops passing unvalidated input to the security service (#4127)
+
+Batch 3 of the #4127 gate, after #4168 (`getService`) and #4176 (`resolveService`).
+
+Three slots — `security`, `shareLinks`, `objectql` — each had a written
+contract, a provider registering them, and call sites already inside the
+contract. The only missing link was that the slot name was not a
+`CoreServiceName` member, so nothing could connect them and all three sat behind
+`as any`.
+
+**The ledger extends past the enum rather than the enum growing.** The two
+answer different questions, and conflating them is what left these untyped:
+`CoreServiceName` answers *"what happens at boot when this slot is empty?"* — it
+sits beside `ServiceCriticality` and drives startup orchestration and discovery,
+so adding a member changes runtime behaviour and is effectively permanent. The
+ledger answers *"what shape occupies this slot?"* — pure type information. These
+three need only the second, so `ServiceSlotContracts extends CoreServiceContracts`
+adds them there and `resolveService` keys on `keyof ServiceSlotContracts`. Zero
+runtime effect. If one is later promoted to a genuine core service, its entry
+moves up and nothing else changes.
+
+Evidence, as always, before an entry: `plugin-security` registers `security` and
+`ISecurityService`'s own doc names that registration; `plugin-sharing` registers
+`ShareLinkService`, which declares `implements IShareLinkService`; and `objectql`
+is an **alias of `data`** — `packages/objectql`'s plugin registers the *same
+instance* under both names two lines apart, so one object was resolving as
+`IDataEngine` through one name and `any` through the other. `protocol` (22 call
+sites) and `mcp` have no written contract and stay unmapped.
+
+**Turning it on found four things, all on the `/security` admin surface:**
+
+1. **Request input reached the security service unvalidated.** `?status=` was
+   `String(query.status)` — any string — handed to a method whose contract
+   declares exactly three values, and from there into the query's `where`
+   clause. Not an injection (the `where` is structured, never interpolated), but
+   `?status=garbage` matched no row and returned an empty list, which reads as
+   "there are no suggestions" rather than "that is not a status". Now a 400.
+
+2. **A test pinned that bug as expected behaviour.** The existing case asserted
+   `status: 'open'` — not one of the three declared values — reached the service
+   and returned 200. It proved the delegate carried *a filter* and nothing about
+   that filter being a status. Same shape as batch 1's `auth.handler` mocks:
+   coverage in appearance, a wrong contract in substance.
+
+3. **and 4. Two writes could not prove they had a caller.**
+   `confirmAudienceBindingSuggestion`/`dismissAudienceBindingSuggestion` declare
+   `callerContext: SecurityContext` non-optionally — deliberately, since the
+   read beside them declares it optional — and the domain passed a possibly-
+   `undefined` execution context.
+
+   **This was not a live hole**, and the distinction matters: with no execution
+   context `shouldDenyAnonymous` already denied, because it sees no
+   `userId`/`isSystem` and its allowlist arm needs a non-empty `path` this seam
+   never passes, so it fell through to `return true`. What it never did was
+   narrow `ec` itself — it only read `ec?.userId`. Checking `ec` directly is
+   behaviour-preserving and makes the invariant legible to the compiler and the
+   next reader.
+
+The `?status=` rejection is the one **behaviour change**: an unknown status was
+a silent empty list and is now a 400 naming the accepted values. The accepted
+set is a `Record` keyed on the contract type, so adding a status to the contract
+leaves a key missing and renaming one leaves a key excess — either fails to
+compile, where a plain array would have drifted silently.
+
+Verified: `@objectstack/runtime` **945 tests / 66 files** (+8), `@objectstack/spec`
+**7141 / 274** (+29), plugin-security **677**, plugin-sharing **225**;
+`tsc --noEmit` on spec, runtime, downstream-contract and all four examples;
+`pnpm lint`; all nine `check:*` gates.

content/docs/permissions/permission-sets.mdx

@@ -204,6 +204,11 @@ POST /api/v1/security/suggested-bindings/:id/confirm      # create the everyone
 POST /api/v1/security/suggested-bindings/:id/dismiss      # decline the prompt
 ```
 
+`status` accepts `pending`, `confirmed` or `dismissed`, and may be omitted to
+list every suggestion. Any other value is rejected with a `400` naming the
+accepted set — it previously reached the query as a filter nothing matched, so
+a mistyped status returned an empty list that read as "no suggestions".
+
 All three require a tenant-level administrator (the anchors are tenant-level
 only — D12). Confirm writes the `sys_position_permission_set` row **as the
 caller**, so the audience-anchor gate re-checks the forbidden-bits predicate

packages/runtime/src/domain-handler-registry.test.ts

@@ -227,12 +227,49 @@ describe('HttpDispatcher extracted domains (PR-2)', () => {
             dismissAudienceBindingSuggestion: vi.fn(),
         };
         // Direct delegate call for the same reason as the notifications case.
+        //
+        // [#4127 batch 3] This asserted `status: 'open'` — not one of the three
+        // values `AudienceBindingSuggestionFilter` declares. The test pinned the
+        // unvalidated pass-through as EXPECTED, so it proved the delegate
+        // carried a filter through and nothing about that filter being a status.
+        // Same shape as the `auth.handler` mock in batch 1: coverage in
+        // appearance, a wrong contract in substance. Now a real status.
+        const context: any = { executionContext: { userId: 'admin-1' } };
+        const result = await makeDispatcher({ security }).handleSecurity('/suggested-bindings', 'GET', undefined, { status: 'pending' }, context);
+        expect(result.response?.status).toBe(200);
+        expect(security.listAudienceBindingSuggestions).toHaveBeenCalledWith(
+            expect.objectContaining({ userId: 'admin-1' }),
+            expect.objectContaining({ status: 'pending' }),
+        );
+    });
+
+    it('/security rejects a status filter that is not a declared status, without calling the service', async () => {
+        const security = {
+            listAudienceBindingSuggestions: vi.fn().mockResolvedValue([{ id: 's1' }]),
+            confirmAudienceBindingSuggestion: vi.fn(),
+            dismissAudienceBindingSuggestion: vi.fn(),
+        };
         const context: any = { executionContext: { userId: 'admin-1' } };
         const result = await makeDispatcher({ security }).handleSecurity('/suggested-bindings', 'GET', undefined, { status: 'open' }, context);
+        // Previously this reached the service and became `where.status = 'open'`,
+        // which matched no row — an empty list that reads as "no suggestions"
+        // rather than "that is not a status".
+        expect(result.response?.status).toBe(400);
+        expect(security.listAudienceBindingSuggestions).not.toHaveBeenCalled();
+    });
+
+    it('/security omits the status filter entirely when the query has none', async () => {
+        const security = {
+            listAudienceBindingSuggestions: vi.fn().mockResolvedValue([]),
+            confirmAudienceBindingSuggestion: vi.fn(),
+            dismissAudienceBindingSuggestion: vi.fn(),
+        };
+        const context: any = { executionContext: { userId: 'admin-1' } };
+        const result = await makeDispatcher({ security }).handleSecurity('/suggested-bindings', 'GET', undefined, {}, context);
         expect(result.response?.status).toBe(200);
         expect(security.listAudienceBindingSuggestions).toHaveBeenCalledWith(
             expect.objectContaining({ userId: 'admin-1' }),
-            expect.objectContaining({ status: 'open' }),
+            expect.objectContaining({ status: undefined }),
         );
     });
 

packages/runtime/src/domain-handler-registry.ts

@@ -35,7 +35,7 @@
 
 import type { HttpProtocolContext, HttpDispatcherResult } from './http-dispatcher.js';
 import type { CoreServiceName } from '@objectstack/spec/system';
-import type { CoreServiceContract } from '@objectstack/spec/contracts';
+import type { CoreServiceContract, ServiceSlotContract, ServiceSlotContracts } from '@objectstack/spec/contracts';
 
 /**
  * The normalized request slice a domain handler receives. `path` is the
@@ -86,18 +86,24 @@ export interface DomainHandlerDeps {
      * its call sites already passed a `CoreServiceName`. This one is mixed, and
      * the overloads split it exactly where the evidence does:
      *
-     *  - A **`CoreServiceName`** — however it is written. 17 call sites address a
+     *  - An **evidenced slot** — however it is written. 17 call sites address a
      *    core slot with a bare literal (`'metadata'` ×10, `'automation'` ×3,
      *    `'auth'` ×3, `'ai'`) rather than `CoreServiceName.enum.*`, so the same
-     *    slot was being addressed two ways; both resolve to the contract now.
-     *  - **Anything else** — `protocol`, `objectql`, `mcp`, `kernel-resolver`,
-     *    `security`, `scope-manager`. These are real services with no
-     *    `CoreServiceName` entry and no written contract, so they keep today's
-     *    `any` rather than being given a shape here that nothing verifies.
-     *    Writing their contracts is the next batch; until then the `any` marks
-     *    where the ledger ends.
+     *    slot was being addressed two ways; both resolve to the contract.
+     *  - **Anything else** — `protocol`, `mcp`, `kernel-resolver`,
+     *    `scope-manager`. Real services with no written contract, so they keep
+     *    today's `any` rather than being given a shape here that nothing
+     *    verifies. That `any` is where the ledger honestly ends.
+     *
+     * [batch 3] The key is `keyof ServiceSlotContracts`, not `CoreServiceName`.
+     * `security`, `shareLinks` and `objectql` each had a contract, a provider
+     * registering them, and call sites already within the contract — the only
+     * missing link was that the slot name was not in the enum, so nothing could
+     * connect them. Widening the *enum* to fix that would have paid for a type
+     * answer with a change to the boot/criticality vocabulary; the ledger
+     * extends past the enum instead. See {@link ServiceSlotContracts}.
      */
-    resolveService<K extends CoreServiceName>(name: K, environmentId?: string): Promise<CoreServiceContract<K> | undefined>;
+    resolveService<K extends keyof ServiceSlotContracts>(name: K, environmentId?: string): Promise<ServiceSlotContract<K> | undefined>;
     resolveService(name: string, environmentId?: string): any;
     /**
      * Unscoped service lookup on the current kernel, typed by the slot.

packages/runtime/src/domains/packages.ts

@@ -623,13 +623,12 @@ names: string[],
 organizationId: string | undefined,
 _context: HttpProtocolContext,
 ): Promise<{ success: boolean; inserted?: number; updated?: number; errors?: unknown[]; error?: string }> {
+    // [#4127] `protocol` keeps its `any` — no written contract, so this is where
+    // the ledger honestly ends. `metadata` and `ql` are both evidenced now,
+    // `objectql` as of batch 3: it is the same instance the `data` slot holds.
     const protocol: any = await deps.resolveService('protocol');
-    // [#4127] `metadata` was annotated `: any`, which erased the slot type even
-    // after the lookup started returning `IMetadataService`. `protocol` and
-    // `ql` keep theirs: neither is a `CoreServiceName` slot and neither has a
-    // written contract, so their `any` is where the ledger honestly ends.
     const metadata = await deps.getService(CoreServiceName.enum.metadata);
-    const ql: any = await deps.resolveService('objectql');
+    const ql = await deps.resolveService('objectql');
     if (!protocol || typeof protocol.getMetaItem !== 'function' || !ql || !metadata) {
         return { success: false, error: 'seed apply: required services unavailable' };
     }

packages/runtime/src/domains/security.ts

@@ -23,9 +23,29 @@
 import {
     shouldDenyAnonymous, ANONYMOUS_DENY_STATUS, ANONYMOUS_DENY_CODE, ANONYMOUS_DENY_MESSAGE,
 } from '@objectstack/core';
+import type { AudienceBindingSuggestionFilter } from '@objectstack/spec/contracts';
 import type { HttpProtocolContext, HttpDispatcherResult } from '../http-dispatcher.js';
 import type { DomainHandlerDeps, DomainRoute } from '../domain-handler-registry.js';
 
+type SuggestionStatus = NonNullable<AudienceBindingSuggestionFilter['status']>;
+
+/**
+ * [#4127 batch 3] The accepted `?status=` values, keyed BY the contract type so
+ * the correspondence is mechanical: adding a status to
+ * `AudienceBindingSuggestionFilter` leaves a key missing here and renaming one
+ * leaves a key excess, and either way this fails to compile. A plain
+ * `['pending', …]` array would have silently drifted, which is the failure mode
+ * this whole work line exists to remove.
+ */
+const SUGGESTION_STATUSES: Record<SuggestionStatus, true> = {
+    pending: true,
+    confirmed: true,
+    dismissed: true,
+};
+
+const isSuggestionStatus = (value: string): value is SuggestionStatus =>
+    Object.prototype.hasOwnProperty.call(SUGGESTION_STATUSES, value);
+
 export function createSecurityDomain(deps: DomainHandlerDeps): DomainRoute {
     return {
         prefix: '/security',
@@ -44,7 +64,11 @@ export async function handleSecurityRequest(
     query: any,
     context: HttpProtocolContext,
 ): Promise<HttpDispatcherResult> {
-    const service = await deps.resolveService('security', context.environmentId) as any;
+    // [#4127 batch 3] The `as any` was the only thing between this call and
+    // `ISecurityService`. The contract was written, `plugin-security` registers
+    // the slot, and all three methods used below were already declared — the
+    // slot name simply was not in the ledger, so nothing connected them.
+    const service = await deps.resolveService('security', context.environmentId);
     if (!service || typeof service.listAudienceBindingSuggestions !== 'function') {
         return { handled: true, response: deps.error('Security service not available', 503) };
     }
@@ -54,7 +78,18 @@ export async function handleSecurityRequest(
     // even before the opt-out was retired this seam never honoured it, so an
     // anonymous caller could never list or confirm audience bindings. Shares
     // the decision + body with every other HTTP seam.
-    if (shouldDenyAnonymous({ userId: ec?.userId, isSystem: ec?.isSystem })) {
+    //
+    // [#4127 batch 3] The `!ec` arm is BEHAVIOUR-PRESERVING, not a new gate: with
+    // no execution context, `shouldDenyAnonymous` already denied — it sees no
+    // `userId`/`isSystem`, and its allowlist arm needs a non-empty `path` this
+    // seam never passes, so it fell through to `return true`. What it did not do
+    // is narrow `ec` itself, because it only ever read `ec?.userId`. So the
+    // contract's requirement — `confirmAudienceBindingSuggestion(callerContext:
+    // SecurityContext, …)`, non-optional precisely because a WRITE needs a
+    // caller identity, unlike the optional one on the read — could not be seen
+    // to hold even though it did. Checking `ec` directly makes the invariant
+    // legible to the compiler and to the next reader.
+    if (!ec || shouldDenyAnonymous({ userId: ec.userId, isSystem: ec.isSystem })) {
         return {
             handled: true,
             response: deps.error(ANONYMOUS_DENY_MESSAGE, ANONYMOUS_DENY_STATUS, { code: ANONYMOUS_DENY_CODE }),
@@ -70,9 +105,26 @@ export async function handleSecurityRequest(
     try {
         // GET /security/suggested-bindings
         if (parts.length === 1 && m === 'GET') {
-            const status = query?.status ? String(query.status) : undefined;
+            // [#4127 batch 3] `status` was `String(query.status)` — any request
+            // string, handed to a service whose contract declares exactly three
+            // values, and from there straight into the query's `where` clause.
+            // Not an injection (the `where` is structured, never interpolated),
+            // but `?status=garbage` matched no row and returned an empty list,
+            // which reads as "there are no suggestions" rather than "your filter
+            // was not a status". Rejecting is the honest answer, and the only
+            // one that keeps the call inside the contract.
+            const rawStatus = query?.status ? String(query.status) : undefined;
+            if (rawStatus !== undefined && !isSuggestionStatus(rawStatus)) {
+                return {
+                    handled: true,
+                    response: deps.error(
+                        `Unknown status filter '${rawStatus}' — expected one of: ${Object.keys(SUGGESTION_STATUSES).join(', ')}`,
+                        400,
+                    ),
+                };
+            }
             const packageId = query?.packageId ? String(query.packageId) : undefined;
-            const result = await service.listAudienceBindingSuggestions(ec, { status, packageId });
+            const result = await service.listAudienceBindingSuggestions(ec, { status: rawStatus, packageId });
             return { handled: true, response: deps.success(result) };
         }
 

packages/runtime/src/domains/share-links.ts

@@ -56,7 +56,10 @@ export async function handleShareLinksRequest(
     query: any,
     context: HttpProtocolContext,
 ): Promise<HttpDispatcherResult> {
-    const svc: any = await deps.resolveService('shareLinks', context.environmentId);
+    // [#4127 batch 3] `plugin-sharing` registers `ShareLinkService`, which
+    // declares `implements IShareLinkService`; the four methods called below
+    // were all already on that contract. Only the ledger entry was missing.
+    const svc = await deps.resolveService('shareLinks', context.environmentId);
     if (!svc) {
         return { handled: true, response: deps.error('Sharing is not configured for this environment', 501) };
     }

packages/spec/api-surface.json

@@ -3764,6 +3764,8 @@
     "SendSmsInput (interface)",
     "SendSmsResult (interface)",
     "SendTemplateInput (interface)",
+    "ServiceSlotContract (type)",
+    "ServiceSlotContracts (interface)",
     "ShareAccessLevel (type)",
     "ShareLink (interface)",
     "ShareLinkAudience (type)",