Skip to content

fix(rest): refuse an unknown ?status on /security/suggested-bindings instead of answering an empty list (#7678) - #7979

Merged
hotlong merged 2 commits into
mainfrom
claude/issue-7678-suggested-bindings-status-validation
Aug 12, 2026
Merged

fix(rest): refuse an unknown ?status on /security/suggested-bindings instead of answering an empty list (#7678)#7979
hotlong merged 2 commits into
mainfrom
claude/issue-7678-suggested-bindings-status-validation

Conversation

@hotlong

@hotlong hotlong commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Fixes #7678

The defect

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.

registerSecurityEndpoints forwarded req.query.status straight into listAudienceBindingSuggestions, whose contract (AudienceBindingSuggestionFilter) declares exactly three values (pending, confirmed, dismissed). An unknown one was not an injection — the where clause is structured, never interpolated — it simply matched no row.

The docs already promised the fixed behaviour. content/docs/permissions/permission-sets.mdx:207-210 documents this route as "status accepts pending, confirmed or dismissed … 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'." 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 /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 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 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: the registerSecurityEndpoints LIST handler now refuses before the service call.

packages/core was the smallest home that works: it already depends on @objectstack/spec (so the record stays keyed by AudienceBindingSuggestionFilter — adding a status leaves a key missing and fails to compile rather than silently drifting), and both rest and runtime already 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_ERROR is 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, since deps.error(msg, 400)buildApiErrorstandardErrorCodeForHttpStatus(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 status and the nested body.error.code — never one alone. A not.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:

  • every declared status still lists — enumerated from the contract type via AUDIENCE_BINDING_SUGGESTION_STATUS_VALUES, never hand-picked, so a status added to the contract is exercised the day it is declared;
  • omitting ?status entirely still returns the unfiltered list;
  • ?packageId is untouched.

Both assert the argument the service was handed, not merely that a 200 came back.

Reverse verification

With only the rest-server.ts guard reverted (test and shared helper intact), the two refusal cases go red for the right reason and the five preservation cases stay green:

 ❯ src/rest-server-suggested-bindings-status.test.ts (7 tests | 2 failed)
   × ?status=garbage → 400 VALIDATION_ERROR (was: 200 with an empty list)
   × ?status=PENDING → 400: the vocabulary is lowercase, so wrong case is not a status

AssertionError: expected 400 for an unknown ?status, got 200 with body
{"data":{"suggestions":[{"id":"s1","status":"pending","package_id":"com.example.crm"}],
"sync":{"created":0,"confirmedObserved":0,"pruned":0}}}
- Expected  400
+ Received  200

 Tests  2 failed | 5 passed (7)

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/rest 1531 ✓ · @objectstack/runtime 2128 ✓ · @objectstack/core 773 ✓ — all re-run after merging origin/main.
  • packages/spec moved on the incoming side, so it was rebuilt and check:generated re-run: all 13 artifacts up to date.

Changeset: .changeset/suggested-bindings-status-validation.md (patch × 3).

Out of scope


Generated by Claude Code

claude added 2 commits August 12, 2026 08:55
…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
@vercel

vercel Bot commented Aug 12, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
objectstack Ignored Ignored Aug 12, 2026 9:19am

Request Review

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 3 package(s): @objectstack/core, @objectstack/rest, @objectstack/runtime.

36 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:

  • content/docs/ai/actions-as-tools.mdx (via @objectstack/core)
  • content/docs/ai/connect-mcp.mdx (via @objectstack/rest)
  • content/docs/ai/knowledge-rag.mdx (via @objectstack/core)
  • content/docs/ai/natural-language-queries.mdx (via @objectstack/core)
  • content/docs/api/client-sdk.mdx (via packages/runtime)
  • content/docs/api/error-handling-server.mdx (via @objectstack/rest)
  • content/docs/api/index.mdx (via @objectstack/rest, @objectstack/runtime)
  • content/docs/api/wire-format.mdx (via @objectstack/runtime)
  • content/docs/automation/hook-bodies.mdx (via @objectstack/runtime)
  • content/docs/automation/webhooks.mdx (via @objectstack/core)
  • content/docs/concepts/metadata-lifecycle.mdx (via @objectstack/runtime)
  • content/docs/concepts/north-star.mdx (via packages/core, packages/runtime)
  • content/docs/data-modeling/drivers.mdx (via @objectstack/runtime)
  • content/docs/deployment/index.mdx (via @objectstack/runtime)
  • content/docs/deployment/migration-from-objectql.mdx (via @objectstack/core)
  • content/docs/deployment/production-readiness.mdx (via @objectstack/runtime)
  • content/docs/deployment/single-project-mode.mdx (via @objectstack/runtime)
  • content/docs/deployment/vercel.mdx (via @objectstack/runtime)
  • content/docs/getting-started/your-first-project.mdx (via @objectstack/runtime)
  • content/docs/kernel/cluster.mdx (via @objectstack/runtime)
  • content/docs/kernel/contracts/index.mdx (via @objectstack/core)
  • content/docs/kernel/runtime-services/examples.mdx (via @objectstack/core)
  • content/docs/kernel/services-checklist.mdx (via @objectstack/core)
  • content/docs/kernel/services.mdx (via @objectstack/core)
  • content/docs/permissions/authentication.mdx (via @objectstack/core, @objectstack/rest, @objectstack/runtime)
  • content/docs/permissions/authorization.mdx (via packages/core, packages/runtime)
  • content/docs/permissions/system-context.mdx (via packages/rest, packages/runtime)
  • content/docs/plugins/anatomy.mdx (via @objectstack/core)
  • content/docs/plugins/development.mdx (via @objectstack/core)
  • content/docs/plugins/index.mdx (via @objectstack/core, @objectstack/rest)
  • content/docs/plugins/packages.mdx (via @objectstack/core, @objectstack/rest, @objectstack/runtime)
  • content/docs/protocol/kernel/http-protocol.mdx (via @objectstack/rest, @objectstack/runtime)
  • content/docs/protocol/kernel/i18n-standard.mdx (via packages/rest)
  • content/docs/protocol/kernel/index.mdx (via @objectstack/core, @objectstack/runtime)
  • content/docs/protocol/kernel/lifecycle.mdx (via @objectstack/core, @objectstack/runtime)
  • content/docs/protocol/kernel/plugin-spec.mdx (via @objectstack/core)

4 release-owned page(s) also reference the affected code. These are read-only:

  • content/docs/releases/implementation-status.mdx (via @objectstack/core, @objectstack/rest, @objectstack/runtime)
  • content/docs/releases/v12.mdx (via @objectstack/core, @objectstack/rest)
  • content/docs/releases/v15.mdx (via @objectstack/core)
  • content/docs/releases/v17.mdx (via @objectstack/core, @objectstack/rest, @objectstack/runtime)

content/docs/releases/ is RELEASE-OWNED (AGENTS.md "Documentation Guardrails"): release
notes are written centrally at release time, and a code PR that edits them is the exact PR
that guardrail exists to stop. They are still audited — read-only. If one of them is actually
wrong, file an issue or open a dedicated docs-only PR; do not edit it here.

Advisory only. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs origin/main → pass the list as args.docs.

hotlong commented Aug 12, 2026

Copy link
Copy Markdown
Contributor Author

PM review — domain:cli seat (#6024). Verdict: accept. Auto-merge on.

The ruling was "reuse the existing isSuggestionStatus, ⛔ do not write a second copy." This delivers the stronger version: not reuse, but convergence to one definition — the runtime's local copy is deleted, the vocabulary/predicate/message move to @objectstack/core's security barrel beside shouldDenyAnonymous, and both seams import it. After this there is no second copy to drift.

The home was chosen for a compile-time guarantee, not for tidiness. packages/core already depends on @objectstack/spec, so the record stays keyed by AudienceBindingSuggestionFilter — adding a status leaves a key missing and fails to compile. That is the drift that produced this bug, made unrepresentable rather than merely fixed. Fifth one-rule-two-implementations case this lane closed today (#7616 / #7652 / #7602 / #7749), and the first to close the door behind it.

The valid-status test enumerates from the contract type (AUDIENCE_BINDING_SUGGESTION_STATUS_VALUES), not a hand-picked list — so a status added to the contract is exercised the day it is declared. Both halves assert the argument the service was handed, not merely that a 200 came back.

The reverse verification shows the right thing: not "the test went red" but a 200 carrying a list — the defect itself. not.toBe(200) would have been worthless here precisely because the symptom is a 200, and the PR says so.

Two things beyond the ask:

  • ?status=PENDING is measured as refused. The card named case-sensitivity; it would have been easy to leave the vocabulary case-insensitive and quietly widen the contract.
  • The two seams now agree on code, not just on refusing. VALIDATION_ERROR is what the dispatcher already answers via standardErrorCodeForHttpStatus(400), so convergence reaches the envelope and not only the status.

Docs need no change and the PR explains whycontent/docs/permissions/permission-sets.mdx:207-210 already documents the rejection, true of the dispatcher and false of the route users hit. A declared≠enforced closure (Prime Directive #10): the doc simply becomes true. Same shape as PR #7958's release note earlier today, where releases/v15.mdx had been claiming a 401 the handler was not delivering.

#7981 correctly filed rather than fixed: three mutually incompatible error-envelope shapes across registerSecurityEndpoints' arms, including the bare-string dialect #7035 retired. Same region, right call to leave it.


Generated by Claude Code

@hotlong
hotlong added this pull request to the merge queue Aug 12, 2026
Merged via the queue into main with commit ee264b2 Aug 12, 2026
27 checks passed
@hotlong
hotlong deleted the claude/issue-7678-suggested-bindings-status-validation branch August 12, 2026 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation size/m tests tooling

Projects

None yet

Development

Successfully merging this pull request may close these issues.

suggested-binding-loop (b): unknown ?status on /security/suggested-bindings returns 200 empty instead of 400 (live REST route skips validation)

2 participants