Skip to content

Commit ee264b2

Browse files
hotlongclaude
andauthored
fix(rest): refuse an unknown ?status on /security/suggested-bindings (#7678) (#7979)
`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. Claude-Session: https://claude.ai/code/session_01Xc86SFVAgZHc52YF9iLxCc Co-authored-by: Claude <noreply@anthropic.com>
1 parent be37f85 commit ee264b2

6 files changed

Lines changed: 343 additions & 26 deletions

File tree

Lines changed: 44 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,44 @@
1+
---
2+
"@objectstack/core": patch
3+
"@objectstack/runtime": patch
4+
"@objectstack/rest": patch
5+
---
6+
7+
fix(rest): refuse an unknown `?status` on `/security/suggested-bindings` instead of answering an empty list (#7678)
8+
9+
`GET /api/v1/security/suggested-bindings?status=garbage` returned **200 with an
10+
empty list**. That is worse than an error: an empty list is a plausible,
11+
actionable-looking answer, so the response reads as *"there are no suggestions"*
12+
rather than *"your filter was not a status"*. An admin checking whether a package
13+
still has pending audience-binding suggestions got a clean, wrong all-clear.
14+
15+
The route (`registerSecurityEndpoints`) forwarded `req.query.status` straight into
16+
`listAudienceBindingSuggestions`, whose contract — `AudienceBindingSuggestionFilter`
17+
— declares exactly three values (`pending`, `confirmed`, `dismissed`). Anything
18+
else was not an injection (the `where` clause is structured, never interpolated),
19+
it simply matched no row.
20+
21+
**The rule already existed; only one of its two seams had it.** The runtime
22+
dispatcher's `/security` domain has refused unknown statuses since the filter was
23+
first tightened, carrying a comment describing precisely the empty-list arm above.
24+
The live REST route is a second seam onto the same service call and never got it —
25+
a dispatcher-vs-REST divergence pointing the opposite way from the earlier `/meta`
26+
cases, where routes existed on the dispatcher but were never mounted on REST.
27+
28+
So this is a **convergence, not a second implementation**. The vocabulary, the
29+
predicate and the refusal wording move to `@objectstack/core`'s security barrel
30+
(`isAudienceBindingSuggestionStatus`, alongside `shouldDenyAnonymous` and the other
31+
decisions shared by every HTTP seam), and both callers import it. The accepted
32+
values stay keyed *by* the contract type, so adding a status to
33+
`AudienceBindingSuggestionFilter` leaves a key missing and fails to compile rather
34+
than silently drifting.
35+
36+
An unknown `?status` is now refused with **400** and the ADR-0112 envelope
37+
(`{ error: { code: 'VALIDATION_ERROR', message } }`) — matching the repeated-query-
38+
parameter guard already on this route — and the service is not called at all. The
39+
vocabulary is case-sensitive, so `?status=PENDING` is refused like any other
40+
non-status.
41+
42+
Unchanged: every declared status still returns its list, omitting `?status`
43+
entirely still returns the unfiltered list, `?packageId` is untouched, and the
44+
dispatcher seam answers exactly as it did before.
Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,59 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* [#7678] The `?status=` vocabulary of the audience-binding suggestion list
5+
* (ADR-0090 D5/D9) — ONE owner for a rule that had exactly one implementation
6+
* and two seams needing it.
7+
*
8+
* The predicate was written for the runtime dispatcher's `/security` domain and
9+
* lived there, private. The **live** REST route
10+
* (`rest-server.ts` → `registerSecurityEndpoints`) is a second seam onto the
11+
* same service call and never had it, so `?status=garbage` reached the service,
12+
* matched no row, and answered **200 with an empty list** — which reads as
13+
* "there are no suggestions", a plausible and actionable-looking answer, rather
14+
* than "your filter was not a status". That silent arm is the defect; the two
15+
* seams disagreeing about one contract is the cause.
16+
*
17+
* So this module is the convergence, not a copy: `domains/security.ts` and
18+
* `rest-server.ts` both import from here, and the vocabulary — including the
19+
* refusal wording — exists once.
20+
*
21+
* The record is keyed BY the contract type on purpose (carried over from the
22+
* original): adding a status to `AudienceBindingSuggestionFilter` leaves a key
23+
* missing here and renaming one leaves a key excess, and either way this fails
24+
* to compile. A plain `['pending', …]` array would silently drift.
25+
*/
26+
27+
import type { AudienceBindingSuggestionFilter } from '@objectstack/spec/contracts';
28+
29+
/** The `status` arm of {@link AudienceBindingSuggestionFilter}, named. */
30+
export type AudienceBindingSuggestionStatus = NonNullable<AudienceBindingSuggestionFilter['status']>;
31+
32+
/** The accepted `?status=` values, keyed by the contract type (see module note). */
33+
export const AUDIENCE_BINDING_SUGGESTION_STATUSES: Record<AudienceBindingSuggestionStatus, true> = {
34+
pending: true,
35+
confirmed: true,
36+
dismissed: true,
37+
};
38+
39+
/**
40+
* The same vocabulary as a list — for refusal messages, and for tests that must
41+
* enumerate every valid value FROM the type rather than hand-picking one.
42+
*/
43+
export const AUDIENCE_BINDING_SUGGESTION_STATUS_VALUES = Object.keys(
44+
AUDIENCE_BINDING_SUGGESTION_STATUSES,
45+
) as readonly AudienceBindingSuggestionStatus[];
46+
47+
/**
48+
* Is `value` one of the three statuses the contract declares? Case-sensitive on
49+
* purpose — the contract's values are lowercase, so `PENDING` is not a status
50+
* and gets the same refusal as `garbage`.
51+
*/
52+
export const isAudienceBindingSuggestionStatus = (
53+
value: string,
54+
): value is AudienceBindingSuggestionStatus =>
55+
Object.prototype.hasOwnProperty.call(AUDIENCE_BINDING_SUGGESTION_STATUSES, value);
56+
57+
/** The refusal wording, shared so both seams answer an unknown status identically. */
58+
export const unknownAudienceBindingSuggestionStatusMessage = (value: string): string =>
59+
`Unknown status filter '${value}' — expected one of: ${AUDIENCE_BINDING_SUGGESTION_STATUS_VALUES.join(', ')}`;

packages/core/src/security/index.ts

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -137,6 +137,16 @@ export {
137137
// ADR-0091 D1/D2 — grant validity windows, the shared resolution-time predicate.
138138
export { isGrantActive, isGrantExpired, type GrantValidityWindow } from './grant-validity.js';
139139

140+
// [#7678] ADR-0090 D5/D9 — the audience-binding suggestion `?status=` vocabulary,
141+
// shared by the runtime dispatcher's `/security` domain and the live REST route.
142+
export {
143+
AUDIENCE_BINDING_SUGGESTION_STATUSES,
144+
AUDIENCE_BINDING_SUGGESTION_STATUS_VALUES,
145+
isAudienceBindingSuggestionStatus,
146+
unknownAudienceBindingSuggestionStatusMessage,
147+
type AudienceBindingSuggestionStatus,
148+
} from './audience-binding-suggestion-status.js';
149+
140150
// #7284 — the `__` operation-private-key convention, the CONSUMER half of the
141151
// ExecutionContext lifecycle `assemble-execution-context.ts` opens. One owner
142152
// for the rule three packages had hand-copied (#7141 / #7145 / #7204).
Lines changed: 198 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,198 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* [#7678] `GET /api/v1/security/suggested-bindings?status=` — the LIVE REST
5+
* route's `?status=` vocabulary (ADR-0090 D5/D9).
6+
*
7+
* ## The defect these cases pin
8+
*
9+
* `registerSecurityEndpoints` forwarded `req.query.status` straight into
10+
* `listAudienceBindingSuggestions`, whose contract
11+
* (`AudienceBindingSuggestionFilter`) declares exactly three values. An unknown
12+
* one was not rejected anywhere — it simply matched no row, so
13+
* `?status=garbage` answered **200 with an empty list**. That is worse than an
14+
* error: an empty list is a plausible, actionable-looking answer, and it reads
15+
* as "there are no suggestions" rather than "your filter was not a status". So
16+
* a `not.toBe(200)` assertion would be worth nothing here — the unfixed code's
17+
* whole symptom IS a 200 — and every refusal case below asserts the ADR-0112
18+
* pair: the HTTP `status` AND the nested `body.error.code`.
19+
*
20+
* The rule itself is not new. The runtime dispatcher's `/security` domain has
21+
* refused unknown statuses since #4127, with a comment describing precisely the
22+
* empty-list arm above; the live REST route is a second seam onto the same
23+
* service call and never got it. The fix is therefore a CONVERGENCE — both
24+
* seams now call `isAudienceBindingSuggestionStatus` from `@objectstack/core` —
25+
* and the vocabulary is imported here rather than retyped, so a status added to
26+
* the contract is exercised by these cases automatically.
27+
*
28+
* ## The negatives are load-bearing
29+
*
30+
* A guard that 400s everything satisfies the refusal cases and breaks the
31+
* route. The bottom half pins the other direction: every declared status still
32+
* reaches the service, and omitting `?status` still lists unfiltered. Both
33+
* assert the ARGUMENT the service was handed, not merely that a 200 came back.
34+
*/
35+
36+
import { describe, it, expect, vi } from 'vitest';
37+
// `.js` on purpose — NodeNext resolution requires the extension, and this
38+
// package's TEST_DEBT ceiling has no margin for another TS2835 (#7248).
39+
import { RestServer } from './rest-server.js';
40+
import {
41+
AUDIENCE_BINDING_SUGGESTION_STATUS_VALUES,
42+
unknownAudienceBindingSuggestionStatusMessage,
43+
} from '@objectstack/core';
44+
45+
const SUGGESTED_BINDINGS = '/api/v1/security/suggested-bindings';
46+
47+
function mockServer() {
48+
return {
49+
get: vi.fn(), post: vi.fn(), put: vi.fn(), delete: vi.fn(), patch: vi.fn(),
50+
use: vi.fn(),
51+
listen: vi.fn().mockResolvedValue(undefined),
52+
close: vi.fn().mockResolvedValue(undefined),
53+
};
54+
}
55+
56+
function mockRes() {
57+
const res: any = {
58+
statusCode: 200,
59+
json: vi.fn(function (this: any, body: any) { this._body = body; return this; }),
60+
send: vi.fn(function (this: any) { return this; }),
61+
setHeader: vi.fn(function (this: any) { return this; }),
62+
status: vi.fn(function (this: any, code: number) { this.statusCode = code; return this; }),
63+
header: vi.fn(function (this: any) { return this; }),
64+
};
65+
return res;
66+
}
67+
68+
/** The rows the stub service answers with, so "listed" is distinguishable from "empty". */
69+
const SUGGESTIONS = [{ id: 's1', status: 'pending', package_id: 'com.example.crm' }];
70+
71+
function boot() {
72+
const listAudienceBindingSuggestions = vi.fn().mockResolvedValue({
73+
suggestions: SUGGESTIONS,
74+
sync: { created: 0, confirmedObserved: 0, pruned: 0 },
75+
});
76+
77+
const rest = new RestServer(
78+
mockServer() as any,
79+
{ getDiscovery: vi.fn().mockResolvedValue({ version: 'v0', routes: {} }) } as any,
80+
{ api: { requireAuth: false } } as any,
81+
);
82+
// `isSystem` clears the auth gates that run BEFORE the query is read, so
83+
// every request below reaches the status rule it is named after.
84+
(rest as any).resolveExecCtx = async () => ({ isSystem: true, userId: 'u1' });
85+
(rest as any).securityServiceProvider = async () => ({ listAudienceBindingSuggestions });
86+
rest.registerRoutes();
87+
88+
const route = (rest as any).getRoutes().find(
89+
(r: any) => r.method === 'GET' && r.path === SUGGESTED_BINDINGS,
90+
);
91+
if (!route) throw new Error(`route not registered: GET ${SUGGESTED_BINDINGS}`);
92+
93+
const drive = async (query: Record<string, unknown>) => {
94+
const res = mockRes();
95+
await route.handler(
96+
{ method: 'GET', path: SUGGESTED_BINDINGS, params: {}, query, headers: {}, body: {} } as any,
97+
res,
98+
);
99+
return { status: res.statusCode, body: res.json.mock.calls.at(-1)?.[0] };
100+
};
101+
102+
return { drive, listAudienceBindingSuggestions };
103+
}
104+
105+
// ─────────────────────────────────────────────────────────────────────────────
106+
// 1. REFUSAL — the silent-empty-list arm, closed
107+
// ─────────────────────────────────────────────────────────────────────────────
108+
109+
describe('#7678 — an unknown ?status is REFUSED, not answered with an empty list', () => {
110+
it('?status=garbage → 400 VALIDATION_ERROR (was: 200 with an empty list)', async () => {
111+
const { drive, listAudienceBindingSuggestions } = boot();
112+
const answer = await drive({ status: 'garbage' });
113+
114+
// Both halves, per ADR-0112. `status` alone would pass on any 400 the
115+
// route emits for another reason; `code` alone would pass on the 200
116+
// this route used to answer if a code ever appeared in a success body.
117+
expect(
118+
answer.status,
119+
`expected 400 for an unknown ?status, got ${answer.status} with body ${JSON.stringify(answer.body)}`,
120+
).toBe(400);
121+
expect(answer.body?.error?.code).toBe('VALIDATION_ERROR');
122+
// `error` is the object, not a bare string — the dialect #7035 retired.
123+
expect(typeof answer.body?.error).toBe('object');
124+
// The wording is the SHARED one, so the two seams cannot drift apart
125+
// while both still refusing.
126+
expect(answer.body?.error?.message).toBe(
127+
unknownAudienceBindingSuggestionStatusMessage('garbage'),
128+
);
129+
expect(
130+
listAudienceBindingSuggestions,
131+
'the refused filter must not reach the service at all',
132+
).not.toHaveBeenCalled();
133+
});
134+
135+
it('?status=PENDING → 400: the vocabulary is lowercase, so wrong case is not a status', async () => {
136+
// The card names this one explicitly. Measured, it is NOT accepted: the
137+
// contract's values are lowercase and the predicate is case-sensitive,
138+
// so `PENDING` is refused exactly like `garbage` rather than silently
139+
// filtering to nothing.
140+
const { drive, listAudienceBindingSuggestions } = boot();
141+
const answer = await drive({ status: 'PENDING' });
142+
143+
expect(answer.status).toBe(400);
144+
expect(answer.body?.error?.code).toBe('VALIDATION_ERROR');
145+
expect(listAudienceBindingSuggestions).not.toHaveBeenCalled();
146+
});
147+
});
148+
149+
// ─────────────────────────────────────────────────────────────────────────────
150+
// 2. PRESERVATION — a guard that 400s everything would pass §1 and break the route
151+
// ─────────────────────────────────────────────────────────────────────────────
152+
153+
describe('#7678 — every declared status, and no status at all, still list', () => {
154+
// Enumerated FROM the contract type, never hand-picked: a status added to
155+
// `AudienceBindingSuggestionFilter` is covered here the day it is declared.
156+
it.each(AUDIENCE_BINDING_SUGGESTION_STATUS_VALUES)(
157+
'?status=%s reaches the service and returns its list',
158+
async (status) => {
159+
const { drive, listAudienceBindingSuggestions } = boot();
160+
const answer = await drive({ status });
161+
162+
expect(
163+
answer.status,
164+
`a valid ?status=${status} must not be refused`,
165+
).toBe(200);
166+
expect(answer.body?.data?.suggestions).toEqual(SUGGESTIONS);
167+
// The argument, not just the status code — "still 200" is what the
168+
// defect looked like.
169+
expect(listAudienceBindingSuggestions).toHaveBeenCalledWith(
170+
expect.anything(),
171+
{ status, packageId: undefined },
172+
);
173+
},
174+
);
175+
176+
it('no ?status at all still returns the unfiltered list', async () => {
177+
const { drive, listAudienceBindingSuggestions } = boot();
178+
const answer = await drive({});
179+
180+
expect(answer.status).toBe(200);
181+
expect(answer.body?.data?.suggestions).toEqual(SUGGESTIONS);
182+
expect(listAudienceBindingSuggestions).toHaveBeenCalledWith(
183+
expect.anything(),
184+
{ status: undefined, packageId: undefined },
185+
);
186+
});
187+
188+
it('an unrelated filter (?packageId) is untouched by the status rule', async () => {
189+
const { drive, listAudienceBindingSuggestions } = boot();
190+
const answer = await drive({ packageId: 'com.example.crm' });
191+
192+
expect(answer.status).toBe(200);
193+
expect(listAudienceBindingSuggestions).toHaveBeenCalledWith(
194+
expect.anything(),
195+
{ status: undefined, packageId: 'com.example.crm' },
196+
);
197+
});
198+
});

packages/rest/src/rest-server.ts

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,9 @@ import {
44
IHttpServer, resolveAuthzContext, resolveLocalizationContext, isAuthGateAllowlisted,
55
assembleExecutionContext, normalizeAuthGate, type AuthGate,
66
shouldDenyAnonymous, ANONYMOUS_DENY_BODY, ANONYMOUS_DENY_STATUS,
7+
// [#7678] ADR-0090 D5/D9 suggested-binding `?status=` vocabulary — the one
8+
// owner, shared with the runtime dispatcher's `/security` domain.
9+
isAudienceBindingSuggestionStatus, unknownAudienceBindingSuggestionStatusMessage,
710
} from '@objectstack/core';
811
import {
912
isMcpServerEnabled,
@@ -9515,8 +9518,26 @@ export class RestServer {
95159518
// [#6877] Both are `String(array)` joins — `?status=a&status=b`
95169519
// filtered on the single status `'a,b'` and returned nothing.
95179520
if (refuseRepeatedQueryParams(req, res, ['status', 'packageId'])) return;
9521+
// [#7678] …and a single well-formed but UNKNOWN `?status=`
9522+
// did the same thing one layer on: the service's contract
9523+
// declares exactly three values, anything else matched no
9524+
// row, and the caller got 200 with an empty list — which
9525+
// reads as "there are no suggestions" rather than "your
9526+
// filter was not a status". The runtime dispatcher's twin of
9527+
// this route had refused it since #4127; this live route
9528+
// never did. Same predicate, imported — not a second copy of
9529+
// the vocabulary.
9530+
const status = req.query?.status ? String(req.query.status) : undefined;
9531+
if (status !== undefined && !isAudienceBindingSuggestionStatus(status)) {
9532+
return res.status(400).json({
9533+
error: {
9534+
code: 'VALIDATION_ERROR',
9535+
message: unknownAudienceBindingSuggestionStatusMessage(status),
9536+
},
9537+
});
9538+
}
95189539
const result = await svc.listAudienceBindingSuggestions(context ?? {}, {
9519-
status: req.query?.status ? String(req.query.status) : undefined,
9540+
status,
95209541
packageId: req.query?.packageId ? String(req.query.packageId) : undefined,
95219542
});
95229543
res.json({ data: result });

0 commit comments

Comments
 (0)