diff --git a/.changeset/report-delete-enumeration-oracle.md b/.changeset/report-delete-enumeration-oracle.md new file mode 100644 index 0000000000..855227e88d --- /dev/null +++ b/.changeset/report-delete-enumeration-oracle.md @@ -0,0 +1,45 @@ +--- +"@objectstack/rest": patch +--- + +fix(rest): `DELETE /api/v1/reports/:id` stops telling a caller whether a report id exists + +`DELETE /api/v1/reports/:id` answered 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: + +| Target | Before | After | +| --- | --- | --- | +| Another owner's report id | `500 REPORT_DELETE_FAILED` | `404 REPORT_NOT_FOUND` | +| An id that does not exist | `204 No Content` | `404 REPORT_NOT_FOUND` | +| Your own report | `204 No Content` | `204 No Content` (unchanged) | + +The service layer was never wrong. `deleteReport()` returns early for an unknown +id and throws `REPORT_NOT_FOUND` for 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 to `res.status(500)` and never reached the file-local +`handleValidation`, which maps `REPORT_NOT_FOUND*` to 404 — the sibling +`DELETE /reports/schedules/:scheduleId` in the same file does call it, which is +why that route was already correct. + +Rewiring that catch is necessary but **not sufficient**: it maps the cross-owner +arm to 404 while an unknown id still answers 204, which is the same oracle in a +quieter costume — 404-vs-204 discriminates on existence exactly as well as +500-vs-204 did. So the two deny arms are now answered by **one** response, 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 emitted by `handleValidation` from a +synthesised `REPORT_NOT_FOUND` — the same code path the thrown arm takes — so the +status and the body cannot drift apart. Both arms also now do identical work (one +visibility read, no delete, no `logError`), where the cross-owner arm previously +threw and logged and the unknown one did neither. + +**Behaviour change for existing clients.** Deleting a report you own still answers +`204`, and the SDK's `reports.delete()` is unaffected on that path. What changes +is deleting an id you *cannot see*: previously a silent, idempotent `204`, now a +`404 REPORT_NOT_FOUND` — so a client that re-issues a delete for a report already +deleted (or never present) now sees an error where it saw success. That is the +cost of closing the oracle, and it puts delete in line with the rest of the +surface: cross-owner `GET`, `run`, upsert-overwrite and unschedule all already +answer 404 for the same input. diff --git a/packages/rest/src/reports-delete-enumeration-oracle.test.ts b/packages/rest/src/reports-delete-enumeration-oracle.test.ts new file mode 100644 index 0000000000..54083efd9b --- /dev/null +++ b/packages/rest/src/reports-delete-enumeration-oracle.test.ts @@ -0,0 +1,252 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// [#7523] `DELETE /api/v1/reports/:id` must not tell a caller whether a report +// id EXISTS. +// +// The service layer was already right: `deleteReport()` returns early for an id +// that does not exist and throws `REPORT_NOT_FOUND` for a report the caller does +// not own — "others get a not-found so the delete neither fires nor reveals the +// report's existence". The route threw that away. Its catch went straight to +// `res.status(500)` with `REPORT_DELETE_FAILED` and never reached the file-local +// `handleValidation`, so the two arms surfaced as: +// +// another owner's report id → 500 REPORT_DELETE_FAILED +// an id that does not exist → 204 No Content +// +// which is an enumeration oracle over other users' saved-report ids: an +// authenticated caller probes ids and reads existence off the status code. The +// sibling `DELETE .../reports/schedules/:scheduleId` in the same file does call +// `handleValidation`, which is why that route was already correct. +// +// The half-fix is the trap this file is built around. Rewiring the catch alone +// makes cross-owner answer 404 while the unknown id still answers 204 — the same +// oracle in a quieter costume, 404-vs-204 instead of 500-vs-204. So the tests +// below never assert the two arms' statuses SEPARATELY. They record the whole +// response — every `status()`/`json()`/`end()` call, in order, with arguments — +// and assert the two transcripts are EQUAL. A test that pins each arm's status +// on its own line cannot fail on a half-fix; an equality assertion cannot pass +// through one. +// +// Reverse verification, direction predicted BEFORE running (see the PR body for +// the mutation table): correcting only ONE arm — cross-owner mapped to 404 while +// the unknown id keeps its 204, i.e. exactly the plausible half-fix — turns the +// equality tests RED and leaves the owner's-own-delete test GREEN. + +import { describe, it, expect, vi } from 'vitest'; +import { RestServer } from './rest-server.js'; + +// --------------------------------------------------------------------------- +// Harness +// --------------------------------------------------------------------------- + +const ANON_API = { api: { requireAuth: false } }; + +function createMockServer() { + return { + get: vi.fn(), post: vi.fn(), put: vi.fn(), delete: vi.fn(), patch: vi.fn(), + use: vi.fn(), listen: vi.fn(), close: vi.fn(), + }; +} + +const PROTOCOL = { + getDiscovery: async () => ({ version: 'v0', routes: { data: '', metadata: '', ui: '', auth: '/auth' } }), + getMetaTypes: async () => [], getMetaItems: async () => [], getMetaItem: async () => ({}), + findData: async () => [], getData: async () => ({}), createData: async () => ({ id: '1' }), + updateData: async () => ({}), deleteData: async () => ({ success: true }), +}; + +/** + * A response double that RECORDS rather than asserts. + * + * The oracle lives in the difference between two responses, so the test's unit + * of comparison has to be a whole response, not a status code. `calls` is the + * ordered transcript of everything the handler did to `res` — including the + * argument objects — which is what the two deny arms have to agree on. + */ +function recordingRes() { + const calls: Array<[string, unknown[]]> = []; + const res: any = { + status: (...a: unknown[]) => { calls.push(['status', a]); return res; }, + json: (...a: unknown[]) => { calls.push(['json', a]); return res; }, + end: (...a: unknown[]) => { calls.push(['end', a]); return res; }, + }; + return { res, calls }; +} + +/** + * An `IReportService` double with the REAL ownership semantics of + * `packages/plugins/plugin-reports`' `ReportService` — the three behaviours + * this route sits on top of, each pinned by that package's own suite: + * + * - `getReport()` returns null for an unknown id AND for another owner's id + * alike (#2980: "unauthorized reads are indistinguishable from a genuine + * miss"). + * - `deleteReport()` returns early — silently, no throw — for an unknown id + * ("idempotent — nothing to drop"). + * - `deleteReport()` throws `REPORT_NOT_FOUND` for a report the caller does + * not own (report-service.test.ts: "a non-owner cannot delete another + * user's report"). + * + * Copied rather than imported: `@objectstack/rest` must not take a dependency on + * a plugin package to test its own route. The behaviours, not the code, are what + * this route is contracted against. + */ +function reportsService(rows: Array<{ id: string; ownerId: string }>) { + const deleted: string[] = []; + const owned = (id: string, ctx: any) => rows.find(r => r.id === id && r.ownerId === ctx?.userId); + return { + deleted, + getReport: vi.fn(async (id: string, ctx: any) => owned(id, ctx) ?? null), + deleteReport: vi.fn(async (id: string, ctx: any) => { + const row = rows.find(r => r.id === id); + if (!row) return; // unknown id — idempotent + if (row.ownerId !== ctx?.userId) throw new Error(`REPORT_NOT_FOUND: ${id}`); + deleted.push(id); + }), + }; +} + +/** The route under test, wired for `callerId` as the authenticated principal. */ +function deleteRoute(svc: any, callerId: string) { + const rest: any = new RestServer( + createMockServer() as any, PROTOCOL as any, ANON_API as any, + undefined, undefined, undefined, undefined, undefined, undefined, undefined, + async () => svc, + ); + rest.resolveExecCtx = async () => ({ userId: callerId }); + rest.registerRoutes(); + const route = rest.getRoutes().find( + (r: any) => r.method === 'DELETE' && r.path === '/api/v1/reports/:id', + ); + expect(route).toBeDefined(); + return route; +} + +/** Drive the route once as `callerId` against `id`; return the transcript. */ +async function deleteAs(svc: any, callerId: string, id: string) { + const { res, calls } = recordingRes(); + await deleteRoute(svc, callerId).handler({ params: { id } } as any, res); + return calls; +} + +/** + * The prober's experiment, stated exactly. + * + * A prober sends ONE id and watches what comes back; the question is whether + * the answer depends on whether that id exists. So both arms are driven with + * the SAME id, against two worlds that differ only in whether the report is + * there — which makes the two responses comparable byte-for-byte, with no + * normalising away of an id that differed between the runs. (Normalisation is + * where an oracle hides: whatever you normalise, you stop testing.) + */ +async function probe(id: string, ownerId: string, callerId: string) { + const exists = reportsService([{ id, ownerId }]); + const absent = reportsService([]); + return { + exists, absent, + whenItExists: await deleteAs(exists, callerId, id), + whenItDoesNot: await deleteAs(absent, callerId, id), + }; +} + +// --------------------------------------------------------------------------- +// The oracle, closed +// --------------------------------------------------------------------------- + +describe('[#7523] DELETE /reports/:id does not discriminate on report existence', () => { + // The card's own reproduction: A owns two reports, B is a different owner. + const A_REPORTS = [ + { id: 'rpt_owned_by_a', ownerId: 'user-a' }, + { id: 'rpt_owned_by_a_2', ownerId: 'user-a' }, + ]; + + it("another owner's report and a nonexistent id produce IDENTICAL responses", async () => { + const { whenItExists, whenItDoesNot, exists, absent } = + await probe('rpt_owned_by_a', 'user-a', 'user-b'); + + // The whole response, not just its status — and the same id on both + // sides, so this is literal equality with nothing normalised away. This + // is the assertion the half-fix (cross-owner → 404 while the unknown id + // keeps its 204) cannot survive. + expect(whenItExists).toEqual(whenItDoesNot); + + // ...and the response they agree on is the deny, not an accidental + // agreement on 204 that would mean the owner gate stopped working. + expect(whenItExists).toEqual([ + ['status', [404]], + ['json', [{ code: 'REPORT_NOT_FOUND', error: 'REPORT_NOT_FOUND: rpt_owned_by_a' }]], + ]); + + // The delete never fired for either arm. + expect(exists.deleted).toEqual([]); + expect(absent.deleted).toEqual([]); + }); + + it('reproduces 2× — a second report owned by A answers the same way', async () => { + // The card reproduced on two distinct reports; so does the closure. + for (const { id } of A_REPORTS) { + const { whenItExists, whenItDoesNot, exists } = await probe(id, 'user-a', 'user-b'); + expect(whenItExists).toEqual(whenItDoesNot); + expect(whenItExists[0]).toEqual(['status', [404]]); + expect(exists.deleted).toEqual([]); + } + }); + + it('is blind to WHICH service call gates — a service that only gates in deleteReport() also gets one response', async () => { + // Defence in depth for the catch arm. An `IReportService` that leaves + // `getReport()` unblinded still reaches the route's catch on a + // cross-owner delete; routing that catch through `handleValidation` is + // what keeps ITS two arms identical too. + const unblind = (svc: ReturnType, rows: Array<{ id: string }>) => { + svc.getReport = vi.fn(async (id: string) => rows.find(r => r.id === id) ?? null) as any; + return svc; + }; + const rows = [{ id: 'rpt_owned_by_a', ownerId: 'user-a' }]; + const exists = unblind(reportsService(rows), rows); + const absent = unblind(reportsService([]), []); + + const whenItExists = await deleteAs(exists, 'user-b', 'rpt_owned_by_a'); + const whenItDoesNot = await deleteAs(absent, 'user-b', 'rpt_owned_by_a'); + + expect(whenItExists).toEqual(whenItDoesNot); + expect(whenItExists[0]).toEqual(['status', [404]]); // never 500 REPORT_DELETE_FAILED + expect(exists.deleted).toEqual([]); + }); + + it('does not do equal work by refusing everyone — the owner still deletes their own report', async () => { + // The cheap way to make two responses equal is to break the feature. + const svc = reportsService([...A_REPORTS]); + + const owner = await deleteAs(svc, 'user-a', 'rpt_owned_by_a'); + + expect(owner).toEqual([['status', [204]], ['end', []]]); + expect(svc.deleted).toEqual(['rpt_owned_by_a']); + expect(svc.deleteReport).toHaveBeenCalledWith('rpt_owned_by_a', expect.anything()); + }); + + it('keeps a genuine fault a 500 — the deny mapping did not swallow REPORT_DELETE_FAILED', async () => { + // The other overreach: routing the catch through `handleValidation` + // must not turn an unrelated failure into a 404. + const svc = reportsService([{ id: 'rpt_owned_by_a', ownerId: 'user-a' }]); + svc.deleteReport = vi.fn(async () => { throw new Error('connection reset by peer'); }) as any; + + const boom = await deleteAs(svc, 'user-a', 'rpt_owned_by_a'); + + expect(boom[0]).toEqual(['status', [500]]); + expect((boom[1][1][0] as any).code).toBe('REPORT_DELETE_FAILED'); + }); + + it('performs the same service calls on both deny arms — no work-shaped tell', async () => { + const { exists, absent } = await probe('rpt_owned_by_a', 'user-a', 'user-b'); + const work = (svc: ReturnType) => ({ + get: svc.getReport.mock.calls.length, + del: svc.deleteReport.mock.calls.length, + }); + + // One visibility read, no delete — on BOTH arms. Anything else is a + // difference in work done between "exists" and "does not", which is the + // shape a timing side channel would take. + expect(work(exists)).toEqual(work(absent)); + expect(work(exists)).toEqual({ get: 1, del: 0 }); + }); +}); diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index 973b446155..f34641a000 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -9027,9 +9027,33 @@ export class RestServer { if (this.enforceAuth(req, res, context)) return; const svc = await resolveService(environmentId); if (!svc) return respond501(res); + // [#7523] Deny-as-404, with the two deny arms collapsed onto ONE + // response. `deleteReport()` is silently idempotent for an id that + // does not exist but throws REPORT_NOT_FOUND for a report the + // caller does not own — two shapes that used to reach the caller + // as 204-vs-500 and let an authenticated prober read another + // owner's report ids straight off the status code. Splitting them + // 204-vs-404 would only re-dress the same oracle, so both arms are + // answered here, before the delete fires, by the one call the + // 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, i.e. the exact code path + // the thrown arm takes below — one emitter, so status and body + // cannot drift apart. + const visible = await svc.getReport(req.params.id, context ?? {}); + if (!visible) { + handleValidation(res, new Error(`REPORT_NOT_FOUND: ${req.params.id}`)); + return; + } await svc.deleteReport(req.params.id, context ?? {}); res.status(204).end(); } catch (error: any) { + // REPORT_NOT_FOUND → 404, VALIDATION_FAILED → 400. Reached only + // when an IReportService gates in `deleteReport()` without also + // blinding `getReport()`; routing it through the same helper keeps + // that implementation's arms indistinguishable too. + if (handleValidation(res, error)) return; logError('[REST] Delete report error:', error); res.status(500).json({ code: 'REPORT_DELETE_FAILED', error: String(error?.message ?? error).slice(0, 500) }); }