fix(rest): stop DELETE /reports/:id revealing whether a report id exists (#7523) - #7562
Conversation
…xists (#7523) `DELETE /api/v1/reports/:id` answered `500 REPORT_DELETE_FAILED` for another owner's report but `204 No Content` for an id that does not exist. The split is an enumeration oracle over other users' saved-report ids: an authenticated caller probes ids and reads existence straight off the status code. The service layer was already correct. `deleteReport()` returns early for an unknown id and throws `REPORT_NOT_FOUND` for a cross-owner id, with the intent written down — "others get a not-found so the delete neither fires nor reveals the report's existence". The route discarded it: its catch went straight to `res.status(500)` and never reached the file-local `handleValidation`, which maps `REPORT_NOT_FOUND*` to 404. The sibling `DELETE /reports/schedules/:id` in the same file does call it, which is why that route answers correctly. Rewiring that catch is necessary but not sufficient — cross-owner 404 against an unknown-id 204 discriminates on existence exactly as well as 500-vs-204 did. So both deny arms are now answered by ONE response, before the delete fires, via the call this surface already keeps blind to the difference: `getReport()` returns null for an unknown id and for another owner's id alike (#2980). The response is emitted by `handleValidation` from a synthesised REPORT_NOT_FOUND — the same code path the thrown arm takes — so status and body cannot drift apart. Both arms now also do identical work (one visibility read, no delete, no `logError`), where cross-owner previously threw and logged and unknown did not. Deleting a report you own still answers 204. Deleting an id you cannot see is now 404 instead of a silent idempotent 204 — the cost of closing the oracle, and in line with cross-owner GET / run / upsert-overwrite / unschedule, which all already answer 404. Tests assert the two arms' responses are EQUAL rather than pinning each arm's status separately, so the plausible half-fix cannot pass through them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018MPZfgGSLM2jHwD7vBqzKd
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
📓 Docs Drift CheckThis PR changes 1 package(s): 9 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
⛔ 3 release-owned page(s) also reference the affected code. These are read-only:
|
…extension `check:type-check-debt` measures the test layer that the package's own `tsc --noEmit` excludes, and the new file's `from './rest-server'` was one TS2835 under `moduleResolution: nodenext` — pushing @objectstack/rest's TEST_DEBT from its ledgered 155 to 156. The ledger is a ratchet that may only shrink, so this fixes the error rather than raising the entry. `.js` is what the package's newer test files already use (direct-mount-*.test.ts). @objectstack/rest TEST_DEBT re-measures at 155 again; the 6 tests stay green. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_018MPZfgGSLM2jHwD7vBqzKd
Fixes #7523
What was leaking
DELETE /api/v1/reports/:idanswered differently depending on whether the target id existed, which let any authenticated caller enumerate other users' saved reports by probing ids and reading the status code.500 REPORT_DELETE_FAILED404 REPORT_NOT_FOUND204 No Content404 REPORT_NOT_FOUND204 No Content204 No Content— unchangedRoot cause, and why the one-line fix is not the fix
The service layer was never wrong.
deleteReport()returns early for an unknown id and throwsREPORT_NOT_FOUNDfor a report the caller does not own, with the intent written down in the source: "others get a not-found so the delete neither fires nor reveals the report's existence". The route discarded it — its catch went straight tores.status(500)and never reached the file-localhandleValidation, which mapsREPORT_NOT_FOUND*to 404. The siblingDELETE /reports/schedules/:scheduleIdin the same file does call it, which is why that route was already correct; the shape here is established, not invented.Rewiring that catch is necessary but not sufficient. It maps cross-owner to 404 while an unknown id still answers 204 — and 404-vs-204 discriminates on existence exactly as well as 500-vs-204 did. The oracle would survive in a quieter costume.
So the two deny arms are now answered by one response, emitted before the delete fires, using the call this surface already keeps blind to the difference:
getReport()returns null for an unknown id and for another owner's id alike (#2980). That response is produced byhandleValidationfrom a synthesisedREPORT_NOT_FOUND— literally the same code path the thrown arm takes — so status and body cannot drift apart. The catch is still routed throughhandleValidationas well: that arm is now reachable only for anIReportServicethat gates indeleteReport()without also blindinggetReport(), and routing it through the same helper keeps that implementation's arms identical too.Chosen status: 404 for both arms. 204 for both was the alternative and is wrong — it would mean answering "no content, done" to a caller whose delete never fired, and it would put
DELETEout of step with cross-ownerGET/run/ upsert-overwrite / unschedule, which all already answer 404 for the same input.No residual tells. Same status, same
code, sameerrorstring modulo the id the caller typed. Both arms also now do identical work — one visibility read, no delete, nologError— where cross-owner previously threw and logged and the unknown id did neither.Behaviour change for existing clients
Deleting a report you own still answers
204; the SDK'sreports.delete()is untouched on that path (client.test.ts's "delete tolerates 204" pin stays green). What changes is deleting an id you cannot see: previously a silent idempotent204, now404 REPORT_NOT_FOUND. A client that re-issues a delete for a report already deleted now sees an error where it saw success. That is the price of closing the oracle, and it is stated in the changeset.One knowingly-accepted micro-change: an empty id would previously have surfaced
400 VALIDATION_FAILEDfromdeleteReport()and now yields 404 from the visibility probe. It is not reachable through this route —/api/v1/reports/:iddoes not match an empty segment — so no branch was added to preserve it.Tests — and why they assert equality, not statuses
packages/rest/src/reports-delete-enumeration-oracle.test.ts(6 tests). A test that pins each arm's status on its own line cannot fail on the half-fix, so none of these do that. They record the whole response — everystatus()/json()/end()call, in order, with arguments — and assert the transcripts are equal.Both arms are driven with the same id against two worlds that differ only in whether the report is there, which is the prober's actual experiment and makes the comparison literal — nothing is normalised away. (Normalisation is where an oracle hides: whatever you normalise, you stop testing.)
Mutation table — every test proven able to fail
Each mutation applied to the source, suite run, then reverted.
handleValidation, no visibility probehandleValidationfrom the catch, keep the probeif (true)) — "fix" the oracle by breaking the featuredeleteReport()on the invisible arm ("keep it idempotent")T1 identical responses · T2 reproduces 2× · T3 service that only gates in
deleteReport()· T4 owner still deletes · T5 genuine fault stays 500 · T6 same service calls on both arms.M2 is the one the card asked for. The equality assertion catches it with exactly the diff that names the surviving oracle:
M5 is why T6 exists: it leaves every response identical and is caught only by the work-shape assertion.
Gates
pnpm lint— clean (exit 0)pnpm typecheck/@objectstack/rest— cleanpackages/rest— 83 files, 1347 tests, all passingpackages/plugins/plugin-reports— 3 files, 68 tests passingpnpm check:route-envelope,check:error-code-casing,check:empty-changeset,check:changeset-gate-self-tests— all pass (the rest-server envelope ratchet did not tick up: the fix adds no new hand-built error body, it reuseshandleValidation)packages/clientsuites fail to resolve@objectstack/plugin-hono-server— a missing local build in this container, present before the change and unrelated to it.client.test.ts, which holds thereports.*pins, passes.Scope
Confined to the reports-delete region of
packages/rest/src/rest-server.ts(+24 lines, no deletions), its test file, and a changeset.mapDataErroruntouched;/metaroute mounting untouched — #7525 and #7526 are unblocked by this landing.Sibling finding — not fixed here, filing to the PM
DELETE /reports/schedules/:scheduleIdhas the same oracle in the 404-vs-204 form this card warns about.unscheduleReport()returns early for an unknown scheduleId (if (!schedule) return; // idempotent) → route answers 204, but throwsREPORT_NOT_FOUNDfor a cross-owner schedule →handleValidation→ 404. The QA run saw only the cross-owner arm and read it as correct.rest.test.ts:1651currently pins the unknown arm green at 204. This is a different handler in a contended file, so it is deliberately not touched here — it wants its own card, after #7525/#7526.Generated by Claude Code