From 9abfc3f3c1097b05795929d6fca3f931585b22ac Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 12 Aug 2026 08:55:03 +0000 Subject: [PATCH] fix(rest): refuse an unknown ?status on /security/suggested-bindings (#7678) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `GET /api/v1/security/suggested-bindings?status=garbage` answered 200 with an empty list — which reads as "there are no suggestions" rather than "your filter was not a status". The live REST route forwarded `req.query.status` into `listAudienceBindingSuggestions`, whose contract declares exactly three values, so an unknown one simply matched no row. The rule already existed on the runtime dispatcher's `/security` domain, whose comment describes precisely that empty-list arm; the REST route is a second seam onto the same service call and never had it. This converges the two rather than growing a second copy: the vocabulary, the predicate and the refusal wording move to `@objectstack/core`'s security barrel — beside `shouldDenyAnonymous` and the other decisions shared by every HTTP seam — and both callers import them. The accepted values stay keyed BY `AudienceBindingSuggestionFilter`, so a new status leaves a key missing and fails to compile instead of drifting. The refusal is 400 with the ADR-0112 envelope (`{ error: { code: 'VALIDATION_ERROR', message } }`), matching the repeated-query-parameter guard already on this route, and the service is not called at all. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Xc86SFVAgZHc52YF9iLxCc --- .../suggested-bindings-status-validation.md | 44 ++++ .../audience-binding-suggestion-status.ts | 59 ++++++ packages/core/src/security/index.ts | 10 + ...t-server-suggested-bindings-status.test.ts | 198 ++++++++++++++++++ packages/rest/src/rest-server.ts | 23 +- packages/runtime/src/domains/security.ts | 35 +--- 6 files changed, 343 insertions(+), 26 deletions(-) create mode 100644 .changeset/suggested-bindings-status-validation.md create mode 100644 packages/core/src/security/audience-binding-suggestion-status.ts create mode 100644 packages/rest/src/rest-server-suggested-bindings-status.test.ts diff --git a/.changeset/suggested-bindings-status-validation.md b/.changeset/suggested-bindings-status-validation.md new file mode 100644 index 0000000000..e4ac00299e --- /dev/null +++ b/.changeset/suggested-bindings-status-validation.md @@ -0,0 +1,44 @@ +--- +"@objectstack/core": patch +"@objectstack/runtime": patch +"@objectstack/rest": patch +--- + +fix(rest): refuse an unknown `?status` on `/security/suggested-bindings` instead of answering an empty list (#7678) + +`GET /api/v1/security/suggested-bindings?status=garbage` returned **200 with an +empty list**. That is worse than an error: an empty list is a plausible, +actionable-looking answer, so the response reads as *"there are no suggestions"* +rather than *"your filter was not a status"*. An admin checking whether a package +still has pending audience-binding suggestions got a clean, wrong all-clear. + +The route (`registerSecurityEndpoints`) forwarded `req.query.status` straight into +`listAudienceBindingSuggestions`, whose contract — `AudienceBindingSuggestionFilter` +— declares exactly three values (`pending`, `confirmed`, `dismissed`). Anything +else was not an injection (the `where` clause is structured, never interpolated), +it simply matched no row. + +**The rule already existed; only one of its two seams had it.** The runtime +dispatcher's `/security` domain has refused unknown statuses since the filter was +first tightened, carrying a comment describing precisely the empty-list arm above. +The live REST route is a second seam onto the same service call and never got it — +a dispatcher-vs-REST divergence pointing the opposite way from the earlier `/meta` +cases, where routes existed on the dispatcher but were never mounted on REST. + +So this is a **convergence, not a second implementation**. The vocabulary, the +predicate and the refusal wording move to `@objectstack/core`'s security barrel +(`isAudienceBindingSuggestionStatus`, alongside `shouldDenyAnonymous` and the other +decisions shared by every HTTP seam), and both callers import it. The accepted +values stay keyed *by* the contract type, so adding a status to +`AudienceBindingSuggestionFilter` leaves a key missing and fails to compile rather +than silently drifting. + +An unknown `?status` is now refused with **400** and the ADR-0112 envelope +(`{ error: { code: 'VALIDATION_ERROR', message } }`) — matching the repeated-query- +parameter guard already on this route — and the service is not called at all. The +vocabulary is case-sensitive, so `?status=PENDING` is refused like any other +non-status. + +Unchanged: every declared status still returns its list, omitting `?status` +entirely still returns the unfiltered list, `?packageId` is untouched, and the +dispatcher seam answers exactly as it did before. diff --git a/packages/core/src/security/audience-binding-suggestion-status.ts b/packages/core/src/security/audience-binding-suggestion-status.ts new file mode 100644 index 0000000000..d6e5b849fe --- /dev/null +++ b/packages/core/src/security/audience-binding-suggestion-status.ts @@ -0,0 +1,59 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#7678] The `?status=` vocabulary of the audience-binding suggestion list + * (ADR-0090 D5/D9) — ONE owner for a rule that had exactly one implementation + * and two seams needing it. + * + * The predicate was written for the runtime dispatcher's `/security` domain and + * lived there, private. The **live** REST route + * (`rest-server.ts` → `registerSecurityEndpoints`) is a second seam onto the + * same service call and never had it, so `?status=garbage` reached the service, + * matched no row, and answered **200 with an empty list** — which reads as + * "there are no suggestions", a plausible and actionable-looking answer, rather + * than "your filter was not a status". That silent arm is the defect; the two + * seams disagreeing about one contract is the cause. + * + * So this module is the convergence, not a copy: `domains/security.ts` and + * `rest-server.ts` both import from here, and the vocabulary — including the + * refusal wording — exists once. + * + * The record is keyed BY the contract type on purpose (carried over from the + * original): 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 silently drift. + */ + +import type { AudienceBindingSuggestionFilter } from '@objectstack/spec/contracts'; + +/** The `status` arm of {@link AudienceBindingSuggestionFilter}, named. */ +export type AudienceBindingSuggestionStatus = NonNullable; + +/** The accepted `?status=` values, keyed by the contract type (see module note). */ +export const AUDIENCE_BINDING_SUGGESTION_STATUSES: Record = { + pending: true, + confirmed: true, + dismissed: true, +}; + +/** + * The same vocabulary as a list — for refusal messages, and for tests that must + * enumerate every valid value FROM the type rather than hand-picking one. + */ +export const AUDIENCE_BINDING_SUGGESTION_STATUS_VALUES = Object.keys( + AUDIENCE_BINDING_SUGGESTION_STATUSES, +) as readonly AudienceBindingSuggestionStatus[]; + +/** + * Is `value` one of the three statuses the contract declares? Case-sensitive on + * purpose — the contract's values are lowercase, so `PENDING` is not a status + * and gets the same refusal as `garbage`. + */ +export const isAudienceBindingSuggestionStatus = ( + value: string, +): value is AudienceBindingSuggestionStatus => + Object.prototype.hasOwnProperty.call(AUDIENCE_BINDING_SUGGESTION_STATUSES, value); + +/** The refusal wording, shared so both seams answer an unknown status identically. */ +export const unknownAudienceBindingSuggestionStatusMessage = (value: string): string => + `Unknown status filter '${value}' — expected one of: ${AUDIENCE_BINDING_SUGGESTION_STATUS_VALUES.join(', ')}`; diff --git a/packages/core/src/security/index.ts b/packages/core/src/security/index.ts index f069ca1ad7..d2bd7b1a29 100644 --- a/packages/core/src/security/index.ts +++ b/packages/core/src/security/index.ts @@ -137,6 +137,16 @@ export { // ADR-0091 D1/D2 — grant validity windows, the shared resolution-time predicate. export { isGrantActive, isGrantExpired, type GrantValidityWindow } from './grant-validity.js'; +// [#7678] ADR-0090 D5/D9 — the audience-binding suggestion `?status=` vocabulary, +// shared by the runtime dispatcher's `/security` domain and the live REST route. +export { + AUDIENCE_BINDING_SUGGESTION_STATUSES, + AUDIENCE_BINDING_SUGGESTION_STATUS_VALUES, + isAudienceBindingSuggestionStatus, + unknownAudienceBindingSuggestionStatusMessage, + type AudienceBindingSuggestionStatus, +} from './audience-binding-suggestion-status.js'; + // #7284 — the `__` operation-private-key convention, the CONSUMER half of the // ExecutionContext lifecycle `assemble-execution-context.ts` opens. One owner // for the rule three packages had hand-copied (#7141 / #7145 / #7204). diff --git a/packages/rest/src/rest-server-suggested-bindings-status.test.ts b/packages/rest/src/rest-server-suggested-bindings-status.test.ts new file mode 100644 index 0000000000..7219b55499 --- /dev/null +++ b/packages/rest/src/rest-server-suggested-bindings-status.test.ts @@ -0,0 +1,198 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#7678] `GET /api/v1/security/suggested-bindings?status=` — the LIVE REST + * route's `?status=` vocabulary (ADR-0090 D5/D9). + * + * ## The defect these cases pin + * + * `registerSecurityEndpoints` forwarded `req.query.status` straight into + * `listAudienceBindingSuggestions`, whose contract + * (`AudienceBindingSuggestionFilter`) declares exactly three values. An unknown + * one was not rejected anywhere — it simply matched no row, so + * `?status=garbage` answered **200 with an empty list**. That is worse than an + * error: an empty list is a plausible, actionable-looking answer, and it reads + * as "there are no suggestions" rather than "your filter was not a status". So + * a `not.toBe(200)` assertion would be worth nothing here — the unfixed code's + * whole symptom IS a 200 — and every refusal case below asserts the ADR-0112 + * pair: the HTTP `status` AND the nested `body.error.code`. + * + * The rule itself is not new. The runtime dispatcher's `/security` domain has + * refused unknown statuses since #4127, with a comment describing precisely the + * empty-list arm above; the live REST route is a second seam onto the same + * service call and never got it. The fix is therefore a CONVERGENCE — both + * seams now call `isAudienceBindingSuggestionStatus` from `@objectstack/core` — + * and the vocabulary is imported here rather than retyped, so a status added to + * the contract is exercised by these cases automatically. + * + * ## The negatives are load-bearing + * + * A guard that 400s everything satisfies the refusal cases and breaks the + * route. The bottom half pins the other direction: every declared status still + * reaches the service, and omitting `?status` still lists unfiltered. Both + * assert the ARGUMENT the service was handed, not merely that a 200 came back. + */ + +import { describe, it, expect, vi } from 'vitest'; +// `.js` on purpose — NodeNext resolution requires the extension, and this +// package's TEST_DEBT ceiling has no margin for another TS2835 (#7248). +import { RestServer } from './rest-server.js'; +import { + AUDIENCE_BINDING_SUGGESTION_STATUS_VALUES, + unknownAudienceBindingSuggestionStatusMessage, +} from '@objectstack/core'; + +const SUGGESTED_BINDINGS = '/api/v1/security/suggested-bindings'; + +function mockServer() { + return { + get: vi.fn(), post: vi.fn(), put: vi.fn(), delete: vi.fn(), patch: vi.fn(), + use: vi.fn(), + listen: vi.fn().mockResolvedValue(undefined), + close: vi.fn().mockResolvedValue(undefined), + }; +} + +function mockRes() { + const res: any = { + statusCode: 200, + json: vi.fn(function (this: any, body: any) { this._body = body; return this; }), + send: vi.fn(function (this: any) { return this; }), + setHeader: vi.fn(function (this: any) { return this; }), + status: vi.fn(function (this: any, code: number) { this.statusCode = code; return this; }), + header: vi.fn(function (this: any) { return this; }), + }; + return res; +} + +/** The rows the stub service answers with, so "listed" is distinguishable from "empty". */ +const SUGGESTIONS = [{ id: 's1', status: 'pending', package_id: 'com.example.crm' }]; + +function boot() { + const listAudienceBindingSuggestions = vi.fn().mockResolvedValue({ + suggestions: SUGGESTIONS, + sync: { created: 0, confirmedObserved: 0, pruned: 0 }, + }); + + const rest = new RestServer( + mockServer() as any, + { getDiscovery: vi.fn().mockResolvedValue({ version: 'v0', routes: {} }) } as any, + { api: { requireAuth: false } } as any, + ); + // `isSystem` clears the auth gates that run BEFORE the query is read, so + // every request below reaches the status rule it is named after. + (rest as any).resolveExecCtx = async () => ({ isSystem: true, userId: 'u1' }); + (rest as any).securityServiceProvider = async () => ({ listAudienceBindingSuggestions }); + rest.registerRoutes(); + + const route = (rest as any).getRoutes().find( + (r: any) => r.method === 'GET' && r.path === SUGGESTED_BINDINGS, + ); + if (!route) throw new Error(`route not registered: GET ${SUGGESTED_BINDINGS}`); + + const drive = async (query: Record) => { + const res = mockRes(); + await route.handler( + { method: 'GET', path: SUGGESTED_BINDINGS, params: {}, query, headers: {}, body: {} } as any, + res, + ); + return { status: res.statusCode, body: res.json.mock.calls.at(-1)?.[0] }; + }; + + return { drive, listAudienceBindingSuggestions }; +} + +// ───────────────────────────────────────────────────────────────────────────── +// 1. REFUSAL — the silent-empty-list arm, closed +// ───────────────────────────────────────────────────────────────────────────── + +describe('#7678 — an unknown ?status is REFUSED, not answered with an empty list', () => { + it('?status=garbage → 400 VALIDATION_ERROR (was: 200 with an empty list)', async () => { + const { drive, listAudienceBindingSuggestions } = boot(); + const answer = await drive({ status: 'garbage' }); + + // Both halves, per ADR-0112. `status` alone would pass on any 400 the + // route emits for another reason; `code` alone would pass on the 200 + // this route used to answer if a code ever appeared in a success body. + expect( + answer.status, + `expected 400 for an unknown ?status, got ${answer.status} with body ${JSON.stringify(answer.body)}`, + ).toBe(400); + expect(answer.body?.error?.code).toBe('VALIDATION_ERROR'); + // `error` is the object, not a bare string — the dialect #7035 retired. + expect(typeof answer.body?.error).toBe('object'); + // The wording is the SHARED one, so the two seams cannot drift apart + // while both still refusing. + expect(answer.body?.error?.message).toBe( + unknownAudienceBindingSuggestionStatusMessage('garbage'), + ); + expect( + listAudienceBindingSuggestions, + 'the refused filter must not reach the service at all', + ).not.toHaveBeenCalled(); + }); + + it('?status=PENDING → 400: the vocabulary is lowercase, so wrong case is not a status', async () => { + // The card names this one explicitly. Measured, it is NOT accepted: the + // contract's values are lowercase and the predicate is case-sensitive, + // so `PENDING` is refused exactly like `garbage` rather than silently + // filtering to nothing. + const { drive, listAudienceBindingSuggestions } = boot(); + const answer = await drive({ status: 'PENDING' }); + + expect(answer.status).toBe(400); + expect(answer.body?.error?.code).toBe('VALIDATION_ERROR'); + expect(listAudienceBindingSuggestions).not.toHaveBeenCalled(); + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// 2. PRESERVATION — a guard that 400s everything would pass §1 and break the route +// ───────────────────────────────────────────────────────────────────────────── + +describe('#7678 — every declared status, and no status at all, still list', () => { + // Enumerated FROM the contract type, never hand-picked: a status added to + // `AudienceBindingSuggestionFilter` is covered here the day it is declared. + it.each(AUDIENCE_BINDING_SUGGESTION_STATUS_VALUES)( + '?status=%s reaches the service and returns its list', + async (status) => { + const { drive, listAudienceBindingSuggestions } = boot(); + const answer = await drive({ status }); + + expect( + answer.status, + `a valid ?status=${status} must not be refused`, + ).toBe(200); + expect(answer.body?.data?.suggestions).toEqual(SUGGESTIONS); + // The argument, not just the status code — "still 200" is what the + // defect looked like. + expect(listAudienceBindingSuggestions).toHaveBeenCalledWith( + expect.anything(), + { status, packageId: undefined }, + ); + }, + ); + + it('no ?status at all still returns the unfiltered list', async () => { + const { drive, listAudienceBindingSuggestions } = boot(); + const answer = await drive({}); + + expect(answer.status).toBe(200); + expect(answer.body?.data?.suggestions).toEqual(SUGGESTIONS); + expect(listAudienceBindingSuggestions).toHaveBeenCalledWith( + expect.anything(), + { status: undefined, packageId: undefined }, + ); + }); + + it('an unrelated filter (?packageId) is untouched by the status rule', async () => { + const { drive, listAudienceBindingSuggestions } = boot(); + const answer = await drive({ packageId: 'com.example.crm' }); + + expect(answer.status).toBe(200); + expect(listAudienceBindingSuggestions).toHaveBeenCalledWith( + expect.anything(), + { status: undefined, packageId: 'com.example.crm' }, + ); + }); +}); diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index 40b5a41783..d4193fe220 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -4,6 +4,9 @@ import { IHttpServer, resolveAuthzContext, resolveLocalizationContext, isAuthGateAllowlisted, assembleExecutionContext, normalizeAuthGate, type AuthGate, shouldDenyAnonymous, ANONYMOUS_DENY_BODY, ANONYMOUS_DENY_STATUS, + // [#7678] ADR-0090 D5/D9 suggested-binding `?status=` vocabulary — the one + // owner, shared with the runtime dispatcher's `/security` domain. + isAudienceBindingSuggestionStatus, unknownAudienceBindingSuggestionStatusMessage, } from '@objectstack/core'; import { isMcpServerEnabled, @@ -9515,8 +9518,26 @@ export class RestServer { // [#6877] Both are `String(array)` joins — `?status=a&status=b` // filtered on the single status `'a,b'` and returned nothing. if (refuseRepeatedQueryParams(req, res, ['status', 'packageId'])) return; + // [#7678] …and a single well-formed but UNKNOWN `?status=` + // did the same thing one layer on: the service's contract + // declares exactly three values, anything else matched no + // row, and the caller got 200 with an empty list — which + // reads as "there are no suggestions" rather than "your + // filter was not a status". The runtime dispatcher's twin of + // this route had refused it since #4127; this live route + // never did. Same predicate, imported — not a second copy of + // the vocabulary. + const status = req.query?.status ? String(req.query.status) : undefined; + if (status !== undefined && !isAudienceBindingSuggestionStatus(status)) { + return res.status(400).json({ + error: { + code: 'VALIDATION_ERROR', + message: unknownAudienceBindingSuggestionStatusMessage(status), + }, + }); + } const result = await svc.listAudienceBindingSuggestions(context ?? {}, { - status: req.query?.status ? String(req.query.status) : undefined, + status, packageId: req.query?.packageId ? String(req.query.packageId) : undefined, }); res.json({ data: result }); diff --git a/packages/runtime/src/domains/security.ts b/packages/runtime/src/domains/security.ts index 8101d8e77e..367adc6d70 100644 --- a/packages/runtime/src/domains/security.ts +++ b/packages/runtime/src/domains/security.ts @@ -22,30 +22,11 @@ import { shouldDenyAnonymous, ANONYMOUS_DENY_STATUS, ANONYMOUS_DENY_CODE, ANONYMOUS_DENY_MESSAGE, + isAudienceBindingSuggestionStatus, unknownAudienceBindingSuggestionStatusMessage, } 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; - -/** - * [#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 = { - 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', @@ -113,14 +94,18 @@ export async function handleSecurityRequest( // 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. + // + // [#7678] The predicate and its wording moved to + // `@objectstack/core`'s security barrel unchanged: the LIVE REST + // route (`rest-server.ts` → `registerSecurityEndpoints`) is a second + // seam onto this same service call and was still forwarding + // `?status=` unvalidated, so the empty-list arm described above was + // reachable there. One owner, two callers — not two copies. const rawStatus = query?.status ? String(query.status) : undefined; - if (rawStatus !== undefined && !isSuggestionStatus(rawStatus)) { + if (rawStatus !== undefined && !isAudienceBindingSuggestionStatus(rawStatus)) { return { handled: true, - response: deps.error( - `Unknown status filter '${rawStatus}' — expected one of: ${Object.keys(SUGGESTION_STATUSES).join(', ')}`, - 400, - ), + response: deps.error(unknownAudienceBindingSuggestionStatusMessage(rawStatus), 400), }; } const packageId = query?.packageId ? String(query.packageId) : undefined;