fix(rest): refuse an unknown ?status on /security/suggested-bindings instead of answering an empty list (#7678) - #7979
Conversation
…7678) `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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xc86SFVAgZHc52YF9iLxCc
…gested-bindings-status-validation
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
📓 Docs Drift CheckThis PR changes 3 package(s): 36 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
⛔ 4 release-owned page(s) also reference the affected code. These are read-only:
|
|
PM review — The ruling was "reuse the existing The home was chosen for a compile-time guarantee, not for tidiness. The valid-status test enumerates from the contract type ( The reverse verification shows the right thing: not "the test went red" but a 200 carrying a list — the defect itself. Two things beyond the ask:
Docs need no change and the PR explains why — #7981 correctly filed rather than fixed: three mutually incompatible error-envelope shapes across Generated by Claude Code |
Fixes #7678
The defect
GET /api/v1/security/suggested-bindings?status=garbagereturned 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.
registerSecurityEndpointsforwardedreq.query.statusstraight intolistAudienceBindingSuggestions, whose contract (AudienceBindingSuggestionFilter) declares exactly three values (pending,confirmed,dismissed). An unknown one was not an injection — thewhereclause is structured, never interpolated — it simply matched no row.The docs already promised the fixed behaviour.
content/docs/permissions/permission-sets.mdx:207-210documents this route as "statusacceptspending,confirmedordismissed… Any other value is rejected with a400naming 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'." True of the dispatcher, false of the route users actually hit. So this is a declared≠enforced closure (Prime Directive #10), not a new policy — no doc change is needed, the doc simply becomes true.Why this is a convergence, not a second implementation
The rule already existed. The runtime dispatcher's
/securitydomain 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/metacases, where routes existed on the dispatcher but were never mounted on REST.So the vocabulary, the predicate and the refusal wording move to
@objectstack/core's security barrel — besideshouldDenyAnonymousand the other decisions shared by every HTTP seam — and both callers import the one definition:packages/core/src/security/audience-binding-suggestion-status.ts(new):AUDIENCE_BINDING_SUGGESTION_STATUSES,AUDIENCE_BINDING_SUGGESTION_STATUS_VALUES,isAudienceBindingSuggestionStatus,unknownAudienceBindingSuggestionStatusMessage.packages/runtime/src/domains/security.ts: local copy deleted, imports the shared one. Behaviour byte-identical — the shared message builder reproduces the wording it already emitted.packages/rest/src/rest-server.ts: theregisterSecurityEndpointsLIST handler now refuses before the service call.packages/corewas the smallest home that works: it already depends on@objectstack/spec(so the record stays keyed byAudienceBindingSuggestionFilter— adding a status leaves a key missing and fails to compile rather than silently drifting), and bothrestandruntimealready depend on it.The refusal
400, ADR-0112 envelope, matching the repeated-query-parameter guard already on this same route:
{ "error": { "code": "VALIDATION_ERROR", "message": "Unknown status filter 'garbage' — expected one of: pending, confirmed, dismissed" } }The service is not called at all. The vocabulary is case-sensitive, so
?status=PENDING— which the card names — is measured as refused too, not accepted.VALIDATION_ERRORis not a new code and needs no ledger entry: it is in the closed standard catalog, and it is also what the dispatcher seam already answers, sincedeps.error(msg, 400)→buildApiError→standardErrorCodeForHttpStatus(400)resolves to exactly that. The two seams therefore agree on status and code, not just on refusing.Tests
New:
packages/rest/src/rest-server-suggested-bindings-status.test.ts(7 cases).Refusals assert the ADR-0112 pair — HTTP
statusand the nestedbody.error.code— never one alone. Anot.toBe(200)assertion would be worth nothing here: the unfixed code's whole symptom is a 200.The preservation half is load-bearing, because a guard that 400s everything would satisfy the refusals and break the route:
AUDIENCE_BINDING_SUGGESTION_STATUS_VALUES, never hand-picked, so a status added to the contract is exercised the day it is declared;?statusentirely still returns the unfiltered list;?packageIdis untouched.Both assert the argument the service was handed, not merely that a 200 came back.
Reverse verification
With only the
rest-server.tsguard reverted (test and shared helper intact), the two refusal cases go red for the right reason and the five preservation cases stay green:Not a compile error, not a 404 — a 200 carrying a list, which is the defect itself. Restored from the commit and re-run: 7/7 green.
Verification
pnpm -w typecheck— 126/126 clean.pnpm check:type-check-debt— re-measured, none above its recorded number; no ledger entry raised.@objectstack/rest1531 ✓ ·@objectstack/runtime2128 ✓ ·@objectstack/core773 ✓ — all re-run after mergingorigin/main.packages/specmoved on the incoming side, so it was rebuilt andcheck:generatedre-run: all 13 artifacts up to date.Changeset:
.changeset/suggested-bindings-status-validation.md(patch × 3).Out of scope
/meta/:typecatch-all — the route audit can't see this class because it treats the ledger as ground truth for what's mounted #7526) covering behaviour divergence rather than mount presence — the card argues for it and is right, but it is not built or extended here.registerSecurityEndpointsanswers THREE different error-envelope shapes on the same three routes (ADR-0112) #7981 (filed from this work):registerSecurityEndpointsanswers three mutually incompatible error-envelope shapes across its arms, including the bare-stringerrordialect finding:rest-server.ts里三个相邻/metahandler 的错误信封是三种不同形状,其中两种不符合 ADR-0112 #7035 retired. Same region, deliberately not fixed here.Generated by Claude Code