fix(rest): stop DELETE /reports/:id revealing whether a report id exists (#7523)

.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.

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<typeof reportsService>, 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<typeof reportsService>) => ({
+ 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 });
+ });
+});

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) });
  }