Skip to content

fix(rest): refuse unknown query parameters on GET /approvals/requests instead of returning every request - #7607

Merged
os-help merged 1 commit into
mainfrom
claude/issue-7527-approvals-unknown-filter
Aug 11, 2026
Merged

fix(rest): refuse unknown query parameters on GET /approvals/requests instead of returning every request#7607
os-help merged 1 commit into
mainfrom
claude/issue-7527-approvals-unknown-filter

Conversation

@os-help

@os-help os-help commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

Fixes #7527

The defect

GET /api/v1/approvals/requests?assignedToMe=true answered 200 with every request the caller could see. The handler reads the keys it knows off the query string and ignores the remainder, so a caller who believes they asked for "the requests assigned to me" is handed the unfiltered list — and cannot tell, because an unfiltered result is shaped exactly like a genuinely broad match. No status, header or field distinguishes "your filter matched everything" from "your filter was thrown away".

Same anti-pattern as #7463's defect 2 (tracked as #7534), pointed the other way: there an unrecognised key silently NARROWS to zero, here an unrecognised parameter silently WIDENS to everything.

The fix — refuse, not alias

The route declares a closed parameter set and refuses anything outside it with a located 400 carrying the ADR-0112 nested envelope, { error: { code: 'VALIDATION_ERROR', message } } — the same position and code the repeated-parameter refusal (#6877) on this same handler already answers with, so one route never speaks two dialects for two flavours of "this request is malformed". The message names the parameters that were not understood and lists every one that is supported, so a caller can fix the request from the response alone.

assignedToMe is refused rather than implemented, per the #5714 / #5931 / #7463 family norm. The capability it reaches for already exists and is already reachable — the Console asks exactly this question as approverId=…,role:user, which the approverId multi-identity arm was built for. A second spelling for a question that already has one is surface with no pull behind it and has to be carried forever; refusing costs one error path and makes every future typo self-reporting.

Premise verified before implementing: nothing consumes assignedToMe. grep over the whole worktree — all packages, all of examples/**, the packages/console prebuilt-SPA package — returns zero hits in any file type. The client SDK's approvals.listRequests sends only object / recordId / status / approverId / submitterId (packages/client/src/index.ts), a strict subset of the closed set.

The closed set was MEASURED, not guessed

APPROVAL_REQUEST_LIST_PARAMS is read off the handler's own reads, and is exported so the pin tests assert against it rather than a hand-copied second list that can drift:

kind names
filters object, recordId, status, approverId, submitterId
free text q
paging limit, offset
aliases the handler honours record_id, approver_id, submitter_id

Paging is inside the set on purpose — a whitelist built from the filters alone would have traded a silent-widening bug for a loud paging outage. Auth is header-based (computeExecCtx reads no query key) and the Hono adapter copies searchParams verbatim with no synthetic keys, so nothing else legitimately arrives here.

Where the code actually lives — a falsified dispatch assumption

The dispatch expected the route in packages/plugins/plugin-approvals/src/, and the card said the same ("Not located in this run"). It is not there. GET /api/v1/approvals/requests is registered by registerApprovalsEndpoints in packages/rest/src/rest-server.ts (approx. line 9200); plugin-approvals supplies the service behind it (listRequests) and declares the per-record action targets, but owns no route registration. The fix is therefore in packages/rest, and plugin-approvals is unmodified. Its suite is run below as a regression check only.

Tests

New packages/rest/src/rest-server-approvals-unknown-filter.test.ts — 13 cases. These handlers send, they do not throw, and on the unfixed code the answer was an ordinary 200, so a toThrow-shaped assertion would be worthless and a status-only one is half a test: the defect is that listRequests ran and its rows were returned. Each refusal case asserts the ADR-0112 pair (status AND nested body.error.code), the located message, and that the service was never asked.

  • §1 refusal — assignedToMe; the located message; four other plausible misspellings (assigned_to_me, assignee, mine, approver, ApproverId) failing identically, since the bug is the class and not the one name; multiple unknowns named in deterministic sorted order; unknown-parameter refusal outranking the repeated-parameter one.
  • §2 preservation — the card's own control approverId=u_42,role:user still narrowing (asserted on the argument handed to the service, not the status); the unparameterised call still returning the full list; paging reaching the paged branch (total present); the snake_case aliases; and every name in the exported set accepted.
  • §3 the helper itself — absent/null query, all-recognised, singular vs plural phrasing.

Reverse verification — direction predicted first

Prediction: removing the gate turns the §1 cases red while §2 and §3 stay green (they do not depend on it). Measured, with the fix taken out via Edit and restored the same way:

Tests  5 failed | 8 passed (13)

× ?assignedToMe=true is refused, and the whole list is NOT returned
  → expected a 400 refusal for assignedToMe, got 200 with body
    {"data":[{"id":"req_1"},{"id":"req_2"}]}
× an unknown parameter outranks a repeated known one
  → expected 'The "status" query parameter was supp…'
       to be 'The "assignedToMe" query parameter is…'
✓ the card control: approverId=u_42,role:user still narrows
✓ a request with no parameters still returns the full list

The red output reproduces the reported defect verbatim — a 200 carrying both rows — rather than failing generically, which is what makes it a pin on this bug and not on "something changed".

Suites and gates

command result
pnpm --filter '@objectstack/rest^...' build (build closure first) success
pnpm --filter @objectstack/rest test 83 files / 1357 tests passed
new file, verbose 13/13 passed
pnpm --filter @objectstack/rest typecheck clean (tsc --noEmit, no output)
pnpm --filter @objectstack/plugin-approvals test 21 files / 458 tests passed — the #7592 baseline, held
pnpm check:docs-audit-scope pass (56 + 22 self-tests, 179 docs, 9 release pages read-only)
node scripts/check-nul-bytes.mjs OK, 7023 files; plus a widened self-scan over both new files — clean

The plugin-approvals suite first failed 5 files at collection with Failed to resolve entry for package "@objectstack/service-automation" — the unbuilt-dependency trap, not this change. Green at the full 21/458 baseline after pnpm --filter '@objectstack/plugin-approvals^...' build.

Scope

Deliberately not here: the card's wider suggestion to reject unknown query parameters across all endpoints. That is a cross-lane REST-ingress policy decision with a real breaking-change surface, not an approvals bug — filed for triage as #7606 (searched first; no duplicate). The helper it would reuse is introduced here and proven on one route.


Generated by Claude Code

…#7527)

`?assignedToMe=true` answered 200 with every request the caller could see —
the handler read the keys it knew and dropped the rest, so a caller who
believed they asked for "the requests assigned to me" got the unfiltered
list and could not tell, because an unfiltered result is shaped exactly
like a genuinely broad match.

The route now declares a closed parameter set, measured from the handler's
own reads (five filters, `q`, the paging pair, and the snake_case aliases it
honours), and refuses anything outside it with a located 400 carrying the
ADR-0112 nested envelope — the same position and code the repeated-parameter
refusal on this same handler already answers with. The message names the
unrecognised parameters and lists the supported ones.

`assignedToMe` is refused rather than implemented: the Console already asks
this question as `approverId=<id>,role:user`, so a second spelling would be
surface with no pull behind it.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015fkdTyGmMD5s8ZtEifvuGy
@vercel

vercel Bot commented Aug 11, 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 11, 2026 7:40am

Request Review

@github-actions

Copy link
Copy Markdown
Contributor

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/rest.

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

  • content/docs/ai/connect-mcp.mdx (via @objectstack/rest)
  • content/docs/api/error-handling-server.mdx (via @objectstack/rest)
  • content/docs/api/index.mdx (via @objectstack/rest)
  • content/docs/permissions/authentication.mdx (via @objectstack/rest)
  • content/docs/permissions/system-context.mdx (via packages/rest)
  • content/docs/plugins/index.mdx (via @objectstack/rest)
  • content/docs/plugins/packages.mdx (via @objectstack/rest)
  • content/docs/protocol/kernel/http-protocol.mdx (via @objectstack/rest)
  • content/docs/protocol/kernel/i18n-standard.mdx (via packages/rest)

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

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

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.

@github-actions github-actions Bot added documentation Improvements or additions to documentation tests tooling labels Aug 11, 2026
@os-help
os-help marked this pull request as ready for review August 11, 2026 08:19
@os-help
os-help added this pull request to the merge queue Aug 11, 2026
Merged via the queue into main with commit 59768f7 Aug 11, 2026
27 checks passed
@os-help
os-help deleted the claude/issue-7527-approvals-unknown-filter branch August 11, 2026 08:35
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.

[approvals] assignedToMe=true is not a supported list filter on /api/v1/approvals/requests — silently ignored, returns every request

2 participants