|
| 1 | +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. |
| 2 | +// |
| 3 | +// [#7523] `DELETE /api/v1/reports/:id` must not tell a caller whether a report |
| 4 | +// id EXISTS. |
| 5 | +// |
| 6 | +// The service layer was already right: `deleteReport()` returns early for an id |
| 7 | +// that does not exist and throws `REPORT_NOT_FOUND` for a report the caller does |
| 8 | +// not own — "others get a not-found so the delete neither fires nor reveals the |
| 9 | +// report's existence". The route threw that away. Its catch went straight to |
| 10 | +// `res.status(500)` with `REPORT_DELETE_FAILED` and never reached the file-local |
| 11 | +// `handleValidation`, so the two arms surfaced as: |
| 12 | +// |
| 13 | +// another owner's report id → 500 REPORT_DELETE_FAILED |
| 14 | +// an id that does not exist → 204 No Content |
| 15 | +// |
| 16 | +// which is an enumeration oracle over other users' saved-report ids: an |
| 17 | +// authenticated caller probes ids and reads existence off the status code. The |
| 18 | +// sibling `DELETE .../reports/schedules/:scheduleId` in the same file does call |
| 19 | +// `handleValidation`, which is why that route was already correct. |
| 20 | +// |
| 21 | +// The half-fix is the trap this file is built around. Rewiring the catch alone |
| 22 | +// makes cross-owner answer 404 while the unknown id still answers 204 — the same |
| 23 | +// oracle in a quieter costume, 404-vs-204 instead of 500-vs-204. So the tests |
| 24 | +// below never assert the two arms' statuses SEPARATELY. They record the whole |
| 25 | +// response — every `status()`/`json()`/`end()` call, in order, with arguments — |
| 26 | +// and assert the two transcripts are EQUAL. A test that pins each arm's status |
| 27 | +// on its own line cannot fail on a half-fix; an equality assertion cannot pass |
| 28 | +// through one. |
| 29 | +// |
| 30 | +// Reverse verification, direction predicted BEFORE running (see the PR body for |
| 31 | +// the mutation table): correcting only ONE arm — cross-owner mapped to 404 while |
| 32 | +// the unknown id keeps its 204, i.e. exactly the plausible half-fix — turns the |
| 33 | +// equality tests RED and leaves the owner's-own-delete test GREEN. |
| 34 | + |
| 35 | +import { describe, it, expect, vi } from 'vitest'; |
| 36 | +import { RestServer } from './rest-server.js'; |
| 37 | + |
| 38 | +// --------------------------------------------------------------------------- |
| 39 | +// Harness |
| 40 | +// --------------------------------------------------------------------------- |
| 41 | + |
| 42 | +const ANON_API = { api: { requireAuth: false } }; |
| 43 | + |
| 44 | +function createMockServer() { |
| 45 | + return { |
| 46 | + get: vi.fn(), post: vi.fn(), put: vi.fn(), delete: vi.fn(), patch: vi.fn(), |
| 47 | + use: vi.fn(), listen: vi.fn(), close: vi.fn(), |
| 48 | + }; |
| 49 | +} |
| 50 | + |
| 51 | +const PROTOCOL = { |
| 52 | + getDiscovery: async () => ({ version: 'v0', routes: { data: '', metadata: '', ui: '', auth: '/auth' } }), |
| 53 | + getMetaTypes: async () => [], getMetaItems: async () => [], getMetaItem: async () => ({}), |
| 54 | + findData: async () => [], getData: async () => ({}), createData: async () => ({ id: '1' }), |
| 55 | + updateData: async () => ({}), deleteData: async () => ({ success: true }), |
| 56 | +}; |
| 57 | + |
| 58 | +/** |
| 59 | + * A response double that RECORDS rather than asserts. |
| 60 | + * |
| 61 | + * The oracle lives in the difference between two responses, so the test's unit |
| 62 | + * of comparison has to be a whole response, not a status code. `calls` is the |
| 63 | + * ordered transcript of everything the handler did to `res` — including the |
| 64 | + * argument objects — which is what the two deny arms have to agree on. |
| 65 | + */ |
| 66 | +function recordingRes() { |
| 67 | + const calls: Array<[string, unknown[]]> = []; |
| 68 | + const res: any = { |
| 69 | + status: (...a: unknown[]) => { calls.push(['status', a]); return res; }, |
| 70 | + json: (...a: unknown[]) => { calls.push(['json', a]); return res; }, |
| 71 | + end: (...a: unknown[]) => { calls.push(['end', a]); return res; }, |
| 72 | + }; |
| 73 | + return { res, calls }; |
| 74 | +} |
| 75 | + |
| 76 | +/** |
| 77 | + * An `IReportService` double with the REAL ownership semantics of |
| 78 | + * `packages/plugins/plugin-reports`' `ReportService` — the three behaviours |
| 79 | + * this route sits on top of, each pinned by that package's own suite: |
| 80 | + * |
| 81 | + * - `getReport()` returns null for an unknown id AND for another owner's id |
| 82 | + * alike (#2980: "unauthorized reads are indistinguishable from a genuine |
| 83 | + * miss"). |
| 84 | + * - `deleteReport()` returns early — silently, no throw — for an unknown id |
| 85 | + * ("idempotent — nothing to drop"). |
| 86 | + * - `deleteReport()` throws `REPORT_NOT_FOUND` for a report the caller does |
| 87 | + * not own (report-service.test.ts: "a non-owner cannot delete another |
| 88 | + * user's report"). |
| 89 | + * |
| 90 | + * Copied rather than imported: `@objectstack/rest` must not take a dependency on |
| 91 | + * a plugin package to test its own route. The behaviours, not the code, are what |
| 92 | + * this route is contracted against. |
| 93 | + */ |
| 94 | +function reportsService(rows: Array<{ id: string; ownerId: string }>) { |
| 95 | + const deleted: string[] = []; |
| 96 | + const owned = (id: string, ctx: any) => rows.find(r => r.id === id && r.ownerId === ctx?.userId); |
| 97 | + return { |
| 98 | + deleted, |
| 99 | + getReport: vi.fn(async (id: string, ctx: any) => owned(id, ctx) ?? null), |
| 100 | + deleteReport: vi.fn(async (id: string, ctx: any) => { |
| 101 | + const row = rows.find(r => r.id === id); |
| 102 | + if (!row) return; // unknown id — idempotent |
| 103 | + if (row.ownerId !== ctx?.userId) throw new Error(`REPORT_NOT_FOUND: ${id}`); |
| 104 | + deleted.push(id); |
| 105 | + }), |
| 106 | + }; |
| 107 | +} |
| 108 | + |
| 109 | +/** The route under test, wired for `callerId` as the authenticated principal. */ |
| 110 | +function deleteRoute(svc: any, callerId: string) { |
| 111 | + const rest: any = new RestServer( |
| 112 | + createMockServer() as any, PROTOCOL as any, ANON_API as any, |
| 113 | + undefined, undefined, undefined, undefined, undefined, undefined, undefined, |
| 114 | + async () => svc, |
| 115 | + ); |
| 116 | + rest.resolveExecCtx = async () => ({ userId: callerId }); |
| 117 | + rest.registerRoutes(); |
| 118 | + const route = rest.getRoutes().find( |
| 119 | + (r: any) => r.method === 'DELETE' && r.path === '/api/v1/reports/:id', |
| 120 | + ); |
| 121 | + expect(route).toBeDefined(); |
| 122 | + return route; |
| 123 | +} |
| 124 | + |
| 125 | +/** Drive the route once as `callerId` against `id`; return the transcript. */ |
| 126 | +async function deleteAs(svc: any, callerId: string, id: string) { |
| 127 | + const { res, calls } = recordingRes(); |
| 128 | + await deleteRoute(svc, callerId).handler({ params: { id } } as any, res); |
| 129 | + return calls; |
| 130 | +} |
| 131 | + |
| 132 | +/** |
| 133 | + * The prober's experiment, stated exactly. |
| 134 | + * |
| 135 | + * A prober sends ONE id and watches what comes back; the question is whether |
| 136 | + * the answer depends on whether that id exists. So both arms are driven with |
| 137 | + * the SAME id, against two worlds that differ only in whether the report is |
| 138 | + * there — which makes the two responses comparable byte-for-byte, with no |
| 139 | + * normalising away of an id that differed between the runs. (Normalisation is |
| 140 | + * where an oracle hides: whatever you normalise, you stop testing.) |
| 141 | + */ |
| 142 | +async function probe(id: string, ownerId: string, callerId: string) { |
| 143 | + const exists = reportsService([{ id, ownerId }]); |
| 144 | + const absent = reportsService([]); |
| 145 | + return { |
| 146 | + exists, absent, |
| 147 | + whenItExists: await deleteAs(exists, callerId, id), |
| 148 | + whenItDoesNot: await deleteAs(absent, callerId, id), |
| 149 | + }; |
| 150 | +} |
| 151 | + |
| 152 | +// --------------------------------------------------------------------------- |
| 153 | +// The oracle, closed |
| 154 | +// --------------------------------------------------------------------------- |
| 155 | + |
| 156 | +describe('[#7523] DELETE /reports/:id does not discriminate on report existence', () => { |
| 157 | + // The card's own reproduction: A owns two reports, B is a different owner. |
| 158 | + const A_REPORTS = [ |
| 159 | + { id: 'rpt_owned_by_a', ownerId: 'user-a' }, |
| 160 | + { id: 'rpt_owned_by_a_2', ownerId: 'user-a' }, |
| 161 | + ]; |
| 162 | + |
| 163 | + it("another owner's report and a nonexistent id produce IDENTICAL responses", async () => { |
| 164 | + const { whenItExists, whenItDoesNot, exists, absent } = |
| 165 | + await probe('rpt_owned_by_a', 'user-a', 'user-b'); |
| 166 | + |
| 167 | + // The whole response, not just its status — and the same id on both |
| 168 | + // sides, so this is literal equality with nothing normalised away. This |
| 169 | + // is the assertion the half-fix (cross-owner → 404 while the unknown id |
| 170 | + // keeps its 204) cannot survive. |
| 171 | + expect(whenItExists).toEqual(whenItDoesNot); |
| 172 | + |
| 173 | + // ...and the response they agree on is the deny, not an accidental |
| 174 | + // agreement on 204 that would mean the owner gate stopped working. |
| 175 | + expect(whenItExists).toEqual([ |
| 176 | + ['status', [404]], |
| 177 | + ['json', [{ code: 'REPORT_NOT_FOUND', error: 'REPORT_NOT_FOUND: rpt_owned_by_a' }]], |
| 178 | + ]); |
| 179 | + |
| 180 | + // The delete never fired for either arm. |
| 181 | + expect(exists.deleted).toEqual([]); |
| 182 | + expect(absent.deleted).toEqual([]); |
| 183 | + }); |
| 184 | + |
| 185 | + it('reproduces 2× — a second report owned by A answers the same way', async () => { |
| 186 | + // The card reproduced on two distinct reports; so does the closure. |
| 187 | + for (const { id } of A_REPORTS) { |
| 188 | + const { whenItExists, whenItDoesNot, exists } = await probe(id, 'user-a', 'user-b'); |
| 189 | + expect(whenItExists).toEqual(whenItDoesNot); |
| 190 | + expect(whenItExists[0]).toEqual(['status', [404]]); |
| 191 | + expect(exists.deleted).toEqual([]); |
| 192 | + } |
| 193 | + }); |
| 194 | + |
| 195 | + it('is blind to WHICH service call gates — a service that only gates in deleteReport() also gets one response', async () => { |
| 196 | + // Defence in depth for the catch arm. An `IReportService` that leaves |
| 197 | + // `getReport()` unblinded still reaches the route's catch on a |
| 198 | + // cross-owner delete; routing that catch through `handleValidation` is |
| 199 | + // what keeps ITS two arms identical too. |
| 200 | + const unblind = (svc: ReturnType<typeof reportsService>, rows: Array<{ id: string }>) => { |
| 201 | + svc.getReport = vi.fn(async (id: string) => rows.find(r => r.id === id) ?? null) as any; |
| 202 | + return svc; |
| 203 | + }; |
| 204 | + const rows = [{ id: 'rpt_owned_by_a', ownerId: 'user-a' }]; |
| 205 | + const exists = unblind(reportsService(rows), rows); |
| 206 | + const absent = unblind(reportsService([]), []); |
| 207 | + |
| 208 | + const whenItExists = await deleteAs(exists, 'user-b', 'rpt_owned_by_a'); |
| 209 | + const whenItDoesNot = await deleteAs(absent, 'user-b', 'rpt_owned_by_a'); |
| 210 | + |
| 211 | + expect(whenItExists).toEqual(whenItDoesNot); |
| 212 | + expect(whenItExists[0]).toEqual(['status', [404]]); // never 500 REPORT_DELETE_FAILED |
| 213 | + expect(exists.deleted).toEqual([]); |
| 214 | + }); |
| 215 | + |
| 216 | + it('does not do equal work by refusing everyone — the owner still deletes their own report', async () => { |
| 217 | + // The cheap way to make two responses equal is to break the feature. |
| 218 | + const svc = reportsService([...A_REPORTS]); |
| 219 | + |
| 220 | + const owner = await deleteAs(svc, 'user-a', 'rpt_owned_by_a'); |
| 221 | + |
| 222 | + expect(owner).toEqual([['status', [204]], ['end', []]]); |
| 223 | + expect(svc.deleted).toEqual(['rpt_owned_by_a']); |
| 224 | + expect(svc.deleteReport).toHaveBeenCalledWith('rpt_owned_by_a', expect.anything()); |
| 225 | + }); |
| 226 | + |
| 227 | + it('keeps a genuine fault a 500 — the deny mapping did not swallow REPORT_DELETE_FAILED', async () => { |
| 228 | + // The other overreach: routing the catch through `handleValidation` |
| 229 | + // must not turn an unrelated failure into a 404. |
| 230 | + const svc = reportsService([{ id: 'rpt_owned_by_a', ownerId: 'user-a' }]); |
| 231 | + svc.deleteReport = vi.fn(async () => { throw new Error('connection reset by peer'); }) as any; |
| 232 | + |
| 233 | + const boom = await deleteAs(svc, 'user-a', 'rpt_owned_by_a'); |
| 234 | + |
| 235 | + expect(boom[0]).toEqual(['status', [500]]); |
| 236 | + expect((boom[1][1][0] as any).code).toBe('REPORT_DELETE_FAILED'); |
| 237 | + }); |
| 238 | + |
| 239 | + it('performs the same service calls on both deny arms — no work-shaped tell', async () => { |
| 240 | + const { exists, absent } = await probe('rpt_owned_by_a', 'user-a', 'user-b'); |
| 241 | + const work = (svc: ReturnType<typeof reportsService>) => ({ |
| 242 | + get: svc.getReport.mock.calls.length, |
| 243 | + del: svc.deleteReport.mock.calls.length, |
| 244 | + }); |
| 245 | + |
| 246 | + // One visibility read, no delete — on BOTH arms. Anything else is a |
| 247 | + // difference in work done between "exists" and "does not", which is the |
| 248 | + // shape a timing side channel would take. |
| 249 | + expect(work(exists)).toEqual(work(absent)); |
| 250 | + expect(work(exists)).toEqual({ get: 1, del: 0 }); |
| 251 | + }); |
| 252 | +}); |
0 commit comments