From 042b2201afaffe83dfb8476a293464b14aafb9a4 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 11 Aug 2026 10:13:34 +0000 Subject: [PATCH] fix(plugin-reports): stop `DELETE /reports/schedules/:scheduleId` revealing whether a schedule id exists (#7603) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `DELETE /api/v1/reports/schedules/:scheduleId` answered `404 REPORT_NOT_FOUND` for another owner's schedule but `204 No Content` for a schedule id that does not exist. The caller can delete neither, yet still reads which of the two they hit straight off the status code — an enumeration oracle over other owners' report schedules. This is the defect #7523 closed on the sibling `DELETE /reports/:id`, in the costume that card explicitly warned about: there the split was 500-vs-204 and loud, here 404-vs-204 and quiet. The route was in fact cited by #7523's investigation as the example of the RIGHT shape, because it does route its catch through `handleValidation` — which is why the cross-owner arm is a clean 404 rather than a 500. Only that arm was ever probed (QA run #7515); the unknown-id arm was not, so the surviving half went unseen and `rest.test.ts:1651` pinned its 204 green. `unscheduleReport()` carried the intent — "others get a not-found so the delete neither fires nor reveals the schedule's existence" — and a hole one line wide above it: `if (!schedule) return; // idempotent`. Idempotence is only harmless where every caller may see the row; with a cross-owner arm that throws, resolving quietly IS the tell. Both deny arms are now one decision, taken before the delete fires, by the predicate already blind to the difference between them: `canAccessReport` is false for a schedule that does not exist, for one whose report is gone, and for one owned by somebody else alike. A single throw site means a single message, so the route's single `handleValidation` call emits a single response — status and body cannot drift apart. Unlike `deleteReport`, this could NOT be pre-empted in the route. That one collapses its arms with `getReport()`, already blind to the same difference (#2980); the caller here presents a scheduleId and `IReportService` exposes no by-id schedule read to be blind with (`listSchedules` is keyed by reportId). The blinding therefore lives in the service, and `IReportService.unscheduleReport` now states it as a contract obligation rather than leaving each implementation to rediscover it. The route keeps its half of the composition: ONE emitter for whatever the service throws. Deleting a schedule you own still answers 204. Deleting one you cannot see is now 404 instead of a silent 204 — the cost of closing the oracle, and in line with the cross-owner GET / run / upsert-overwrite / delete arms, which all already answer 404. A system context deleting an id with no row now gets REPORT_NOT_FOUND too; the route is the only production caller. Two pins asserted the leaking arm and are superseded IN PLACE, same input, opposite assertion — `rest.test.ts`'s "DELETE /reports/schedules/:scheduleId returns 204" and `report-service.test.ts`'s "an unknown schedule id is idempotent, not a leak", whose title stated the conclusion backwards. Tests assert the two deny arms' whole responses are EQUAL rather than pinning each arm's status separately: a mutation answering both arms 404 with different bodies leaves every per-arm status assertion green and turns the equality assertions red. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Q4Zy4fY7Kj8t27QAEzr9G1 --- .../schedule-delete-enumeration-oracle.md | 60 ++++ .../plugin-reports/src/report-service.test.ts | 57 +++- .../plugin-reports/src/report-service.ts | 27 +- packages/rest/src/rest-server.ts | 18 ++ packages/rest/src/rest.test.ts | 33 ++- ...schedule-delete-enumeration-oracle.test.ts | 267 ++++++++++++++++++ packages/spec/src/contracts/report-service.ts | 19 +- 7 files changed, 474 insertions(+), 7 deletions(-) create mode 100644 .changeset/schedule-delete-enumeration-oracle.md create mode 100644 packages/rest/src/schedule-delete-enumeration-oracle.test.ts diff --git a/.changeset/schedule-delete-enumeration-oracle.md b/.changeset/schedule-delete-enumeration-oracle.md new file mode 100644 index 0000000000..7906753abd --- /dev/null +++ b/.changeset/schedule-delete-enumeration-oracle.md @@ -0,0 +1,60 @@ +--- +"@objectstack/plugin-reports": patch +"@objectstack/spec": patch +"@objectstack/rest": patch +--- + +fix(plugin-reports): `DELETE /api/v1/reports/schedules/:scheduleId` stops telling a caller whether a schedule id exists + +`DELETE /api/v1/reports/schedules/:scheduleId` answered differently depending on +whether the target id **existed**, which let any authenticated caller enumerate +other owners' report schedules by probing ids and reading the status code: + +| Target | Before | After | +| --- | --- | --- | +| Another owner's schedule id | `404 REPORT_NOT_FOUND` | `404 REPORT_NOT_FOUND` (unchanged) | +| A schedule id that does not exist | `204 No Content` | `404 REPORT_NOT_FOUND` | +| A schedule whose report row is gone | `404 REPORT_NOT_FOUND` | `404 REPORT_NOT_FOUND` (unchanged) | +| Your own schedule | `204 No Content` | `204 No Content` (unchanged) | + +This is the same defect #7523 closed on the sibling `DELETE /reports/:id`, in the +costume that card explicitly warned about: there the split was 500-vs-204 and +loud, here it was 404-vs-204 and read as correct. The route was in fact cited by +#7523's investigation as the example of the *right* shape, because it does route +its catch through `handleValidation` — which is why the cross-owner arm is a +clean 404 rather than a 500. Only the cross-owner arm was ever probed (QA run +#7515); the unknown-id arm was not, so the surviving half went unseen and +`packages/rest/src/rest.test.ts` pinned its `204` green. + +`ReportService.unscheduleReport()` carried the intent — *"others get a not-found +so the delete neither fires nor reveals the schedule's existence"* — and a hole +one line wide above it: `if (!schedule) return; // idempotent`. Idempotence is +only harmless where every caller may see the row; with a cross-owner arm that +throws, resolving quietly *is* the tell. + +Both deny arms are now one decision, taken before the delete fires, by the +predicate already blind to the difference between them: `canAccessReport` is +false for a schedule that does not exist, for one whose report is gone, and for +one owned by somebody else alike. A single throw site means a single message, so +the route's single `handleValidation` call emits a single response — status and +body cannot drift apart. + +Unlike `deleteReport`, this could not be pre-empted in the route. That one +collapses its arms with `getReport()`, which is already blind to the same +difference (#2980); the caller here presents a `scheduleId`, and `IReportService` +exposes no by-id schedule read to be blind with (`listSchedules` is keyed by +`reportId`). The blinding therefore lives in the service, and +`IReportService.unscheduleReport` now states it as a contract obligation rather +than leaving each implementation to rediscover it. + +Deleting a schedule you own still answers `204`. Deleting one you cannot see is +now `404` instead of a silent `204` — the cost of closing the oracle, and in line +with the cross-owner GET / run / upsert-overwrite / delete arms, which all +already answer 404. A system/dispatcher context deleting an id with no row now +gets `REPORT_NOT_FOUND` too, where it previously resolved; no caller in the repo +relies on that (the route is the only production caller). + +Tests assert the two deny arms' responses are **EQUAL** rather than pinning each +arm's status separately, so the plausible half-fix cannot pass through them — a +mutation that answers both arms 404 with different bodies leaves every per-arm +status assertion green and turns the equality assertions red. diff --git a/packages/plugins/plugin-reports/src/report-service.test.ts b/packages/plugins/plugin-reports/src/report-service.test.ts index c4458ea77f..3b87e83d43 100644 --- a/packages/plugins/plugin-reports/src/report-service.test.ts +++ b/packages/plugins/plugin-reports/src/report-service.test.ts @@ -454,8 +454,61 @@ describe('ReportService', () => { expect(engine._tables['sys_report_schedule'].length).toBe(0); }); - it('unscheduleReport: an unknown schedule id is idempotent, not a leak', async () => { - await expect(svc.unscheduleReport('rsch_nope', OTHER)).resolves.toBeUndefined(); + // [#7603] SUPERSEDES `unscheduleReport: an unknown schedule id is + // idempotent, not a leak`, which asserted on this same input + // (`'rsch_nope'` as OTHER) that the call `resolves.toBeUndefined()`. + // + // Its title stated the conclusion backwards. Idempotence is only "not a + // leak" where every caller may see the row; here the sibling arm — another + // owner's schedule — threw REPORT_NOT_FOUND, so resolving quietly was the + // one behaviour that told a stranger apart "no such schedule" from "not + // yours". The route turns that into 204-vs-404, which is an enumeration + // oracle over other owners' schedule ids (#7523's defect on `deleteReport`, + // in a quieter costume). Same input, opposite assertion: the unknown id now + // throws, exactly as the cross-owner id does. + it('unscheduleReport: an unknown schedule id is denied as not-found, not silently idempotent', async () => { + await expect(svc.unscheduleReport('rsch_nope', OTHER)).rejects.toThrow(/REPORT_NOT_FOUND/); + }); + + // The assertion that actually closes the oracle, and the one to keep if any + // of these ever have to be merged: it compares the two deny arms to EACH + // OTHER instead of pinning each one's outcome separately. #7523's mutation + // table showed per-arm assertions cannot fail on the plausible half-fix + // (one arm corrected, the other left alone) — an equality cannot pass + // through one. + it('unscheduleReport: the unknown-id and cross-owner deny arms are indistinguishable', async () => { + const r = await svc.saveReport({ name: 'Mine', object: 'lead', query: {} }, CTX); + const s = await svc.scheduleReport({ reportId: r.id, recipients: ['x@t'] }, CTX); + + // A prober holds one id at a time and can only compare what comes back. + const outcome = async (scheduleId: string) => { + try { + await svc.unscheduleReport(scheduleId, OTHER); + return { threw: false, message: null as string | null }; + } catch (err) { + // The whole message, not a pattern: the route puts it in the 404 body + // verbatim, so any difference here is a difference on the wire. + return { threw: true, message: (err as Error).message }; + } + }; + + // Same id on both sides, so this is literal equality with nothing + // normalised away — the id that does not exist is `s.id` itself, in a + // world where the schedule was never created. + const crossOwner = await outcome(s.id); + await svc.unscheduleReport(s.id, CTX); // owner drops it for real + const unknownId = await outcome(s.id); // same id, now nonexistent + + expect(unknownId).toEqual(crossOwner); + expect(crossOwner).toEqual({ threw: true, message: `REPORT_NOT_FOUND: ${s.id}` }); + }); + + it('unscheduleReport: does not buy equal denials by refusing the owner too', async () => { + // The cheap way to make two arms agree is to break the feature. + const r = await svc.saveReport({ name: 'Mine', object: 'lead', query: {} }, CTX); + const s = await svc.scheduleReport({ reportId: r.id, recipients: ['x@t'] }, CTX); + await expect(svc.unscheduleReport(s.id, CTX)).resolves.toBeUndefined(); + expect(engine._tables['sys_report_schedule'].length).toBe(0); }); it('listSchedules: a non-owner cannot see another user\'s schedules', async () => { diff --git a/packages/plugins/plugin-reports/src/report-service.ts b/packages/plugins/plugin-reports/src/report-service.ts index 3dd5e53d44..34a28426a5 100644 --- a/packages/plugins/plugin-reports/src/report-service.ts +++ b/packages/plugins/plugin-reports/src/report-service.ts @@ -598,13 +598,34 @@ export class ReportService implements IReportService { async unscheduleReport(scheduleId: string, context: ExecutionContext): Promise { if (!scheduleId) throw new Error('VALIDATION_FAILED: scheduleId is required'); - const schedule = await this.loadScheduleRow(scheduleId); - if (!schedule) return; // idempotent — nothing to drop (mirrors deleteReport) // A schedule is owned through its report (#2980): a caller may only delete // the schedules of a report they own. Others get a not-found so the delete // neither fires nor reveals the schedule's existence — deny-as-404, never a // cross-owner 2xx. - const report = await this.loadReportRow(schedule.report_id); + // + // [#7603] That intent used to have a hole one line wide. An id with no row + // behind it returned early and silently — `if (!schedule) return; // + // idempotent` — while another owner's id threw. The route maps those to 204 + // and 404, so a caller who could delete neither still learned which of the + // two they had hit: an enumeration oracle over other owners' schedule ids, + // the same one #7523 closed on `DELETE /reports/:id` in its 500-vs-204 + // costume. Idempotence is only harmless where every caller may see the row; + // here it was the tell. + // + // Both deny arms are now ONE decision, taken before the delete fires, by the + // predicate that is already blind to the difference between them: + // `canAccessReport` is false for a schedule that does not exist, for one + // whose report is gone, and for one owned by somebody else alike. A single + // throw site means a single message, so the route's single `handleValidation` + // call emits a single response — status and body cannot drift apart. + // + // Unlike `deleteReport`, this cannot be pre-empted in the route: the caller + // presents a scheduleId, and `IReportService` exposes no by-id schedule read + // to be blind with (`listSchedules` is keyed by reportId). The blinding has + // to live here, which is why the contract now states it as an obligation + // rather than leaving it to each implementation. + const schedule = await this.loadScheduleRow(scheduleId); + const report = schedule ? await this.loadReportRow(schedule.report_id) : null; if (!this.canAccessReport(report, context)) { throw new Error(`REPORT_NOT_FOUND: ${scheduleId}`); } diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index 2eda71f636..d84f8e1bb0 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -9533,6 +9533,24 @@ export class RestServer { if (this.enforceAuth(req, res, context)) return; const svc = await resolveService(environmentId); if (!svc) return respond501(res); + // [#7603] Both deny arms — an unknown scheduleId and another + // owner's — reach the caller as the one 404 emitted by the + // single `handleValidation` call below, because + // `unscheduleReport` is contracted to throw the SAME + // `REPORT_NOT_FOUND: ` for both, before the delete + // fires. It used to resolve silently for the unknown id, which + // landed here as a 204 and let a prober read another owner's + // schedule ids off the status code (#7523's oracle, in the + // 404-vs-204 costume its card warned about). + // + // Unlike the sibling `DELETE /reports/:id`, this route cannot + // pre-empt the two arms itself: that one collapses them with + // `getReport()`, already blind to the difference (#2980), + // whereas the caller here presents a scheduleId and + // `IReportService` exposes no by-id schedule read to be blind + // with — `listSchedules` is keyed by reportId. So the blinding + // is the service's obligation (stated on the contract), and the + // route's job is to keep ONE emitter for whatever it throws. await svc.unscheduleReport(req.params.scheduleId, context ?? {}); res.status(204).end(); } catch (error: any) { diff --git a/packages/rest/src/rest.test.ts b/packages/rest/src/rest.test.ts index 0103a6e4e5..3aeb34d284 100644 --- a/packages/rest/src/rest.test.ts +++ b/packages/rest/src/rest.test.ts @@ -1648,7 +1648,38 @@ describe('RestServer', () => { expect(res.status).toHaveBeenCalledWith(201); }); - it('DELETE /reports/schedules/:scheduleId returns 204', async () => { + // [#7603] SUPERSEDES the former test `DELETE /reports/schedules/:scheduleId + // returns 204`, which stood right here and drove this same input — + // `{ scheduleId: 'rsch_1' }` against a service whose `unscheduleReport` + // resolved — asserting 204 as the route's answer for ANY schedule id. + // + // That expectation was wrong, and load-bearing in the wrong direction: + // resolving quietly was precisely what `unscheduleReport` did for an id that + // DOES NOT EXIST, while another owner's id threw REPORT_NOT_FOUND. So the 204 + // pinned under the old title was one arm of an enumeration oracle — a caller + // who could delete neither schedule still read which of the two they had hit + // straight off the status code — and this pin held it green. + // + // Same input, opposite assertion. Post-#7603 the two deny arms are a single + // throw in the service, so the route answers 404 to both; a resolving + // `unscheduleReport` now means only "the caller owned it and it is gone", + // which is the second test below. That the two deny arms agree on the WHOLE + // response — body included, not just the status — is asserted in + // schedule-delete-enumeration-oracle.test.ts: a pair of per-arm status + // assertions like these cannot fail on a half-fix, which is why that file + // exists alongside this one. + it('DELETE /reports/schedules/:scheduleId returns 404 for a schedule the caller cannot see', async () => { + const unscheduleReport = vi.fn(async () => { throw new Error('REPORT_NOT_FOUND: rsch_1'); }); + const rest = makeRest(async () => ({ unscheduleReport })); + const { unschedule } = getReportRoutes(rest); + const res = { json: vi.fn(), status: vi.fn().mockReturnThis(), end: vi.fn() }; + await unschedule!.handler({ params: { scheduleId: 'rsch_1' } } as any, res as any); + expect(unscheduleReport).toHaveBeenCalledWith('rsch_1', expect.anything()); + expect(res.status).toHaveBeenCalledWith(404); + expect(res.json).toHaveBeenCalledWith(expect.objectContaining({ code: 'REPORT_NOT_FOUND' })); + }); + + it('DELETE /reports/schedules/:scheduleId returns 204 when the caller owned the schedule', async () => { const unscheduleReport = vi.fn(async () => undefined); const rest = makeRest(async () => ({ unscheduleReport })); const { unschedule } = getReportRoutes(rest); diff --git a/packages/rest/src/schedule-delete-enumeration-oracle.test.ts b/packages/rest/src/schedule-delete-enumeration-oracle.test.ts new file mode 100644 index 0000000000..d03bf0b1ff --- /dev/null +++ b/packages/rest/src/schedule-delete-enumeration-oracle.test.ts @@ -0,0 +1,267 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// [#7603] `DELETE /api/v1/reports/schedules/:scheduleId` must not tell a caller +// whether a schedule id EXISTS. +// +// This is #7523's oracle wearing the costume that card warned about. There, the +// two deny arms surfaced as 500-vs-204; here they surfaced as: +// +// another owner's schedule id → 404 REPORT_NOT_FOUND +// a schedule id that does not exist → 204 No Content +// +// A caller can delete neither, yet still reads which of the two they hit off the +// status code — an enumeration oracle over other owners' schedule ids. The route +// was read as CORRECT precisely because it already routes its catch through +// `handleValidation` (that is why the cross-owner arm is a clean 404 and not a +// 500), and #7523's investigation cited it as the example of the right shape. +// The unknown-id arm was simply never probed on this route: `unscheduleReport` +// returned early and silently, and `rest.test.ts` pinned the resulting 204. +// +// The fix does NOT live in this package. `deleteReport`'s arms can be pre-empted +// in the route because `getReport()` is already blind to the difference (#2980); +// the caller here presents a scheduleId, and `IReportService` exposes no by-id +// schedule read to be blind with — `listSchedules` is keyed by reportId. So the +// blinding is the service's obligation, stated on the contract and implemented +// in `packages/plugins/plugin-reports`, whose own suite pins the real code +// (`report-service.test.ts`, "the unknown-id and cross-owner deny arms are +// indistinguishable"). What THIS file pins is the half of the composition that +// belongs to the route: given a contract-conforming service, the two deny arms +// reach the caller as one and the same response. +// +// The half-fix is the trap the file is built around, 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. +// Equality alone is not enough either (two arms that agree on 500 are equal and +// still broken), so the deny transcript is also pinned in full. +// +// Reverse verification, directions predicted BEFORE running — see the PR body +// for the mutation table. + +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 carrying the schedule-delete semantics the contract + * now requires, in the structure `packages/plugins/plugin-reports` implements + * them: + * + * - a schedule is owned THROUGH its report (#2980) — there is no `owner_id` + * test on the schedule itself; + * - one access predicate decides all three miss shapes alike (no schedule, no + * report behind it, someone else's report), so the deny is a single throw + * site with a single message; + * - that decision is taken BEFORE the delete fires. + * + * Copied rather than imported: `@objectstack/rest` must not take a dependency on + * a plugin package to test its own route (the same call #7523's file made). The + * behaviours, not the code, are what this route is contracted against — which is + * why the plugin's own suite pins the real implementation separately, and why a + * mutation to the fix has to be applied here too for this file to see it. + */ +function reportsService( + schedules: Array<{ id: string; reportId: string }>, + reports: Array<{ id: string; ownerId: string }>, +) { + const deleted: string[] = []; + return { + deleted, + unscheduleReport: vi.fn(async (scheduleId: string, ctx: any) => { + if (!scheduleId) throw new Error('VALIDATION_FAILED: scheduleId is required'); + const schedule = schedules.find(s => s.id === scheduleId); + const report = schedule ? reports.find(r => r.id === schedule.reportId) ?? null : null; + if (!report || report.ownerId !== ctx?.userId) { + throw new Error(`REPORT_NOT_FOUND: ${scheduleId}`); + } + deleted.push(scheduleId); + }), + }; +} + +/** The route under test, wired for `callerId` as the authenticated principal. */ +function unscheduleRoute(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/schedules/:scheduleId', + ); + expect(route).toBeDefined(); + return route; +} + +/** Drive the route once as `callerId` against `scheduleId`; return the transcript. */ +async function unscheduleAs(svc: any, callerId: string, scheduleId: string) { + const { res, calls } = recordingRes(); + await unscheduleRoute(svc, callerId).handler({ params: { scheduleId } } as any, res); + return calls; +} + +/** + * The prober's experiment, stated exactly. + * + * A prober sends ONE schedule 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 schedule + * 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(scheduleId: string, reportId: string, ownerId: string, callerId: string) { + const reports = [{ id: reportId, ownerId }]; + const exists = reportsService([{ id: scheduleId, reportId }], reports); + const absent = reportsService([], reports); + return { + exists, absent, + whenItExists: await unscheduleAs(exists, callerId, scheduleId), + whenItDoesNot: await unscheduleAs(absent, callerId, scheduleId), + }; +} + +// --------------------------------------------------------------------------- +// The oracle, closed +// --------------------------------------------------------------------------- + +describe('[#7603] DELETE /reports/schedules/:scheduleId does not discriminate on schedule existence', () => { + // A owns a report with two schedules on it; B is a different owner. + const A_SCHEDULES = [ + { id: 'rsch_owned_by_a', reportId: 'rpt_a' }, + { id: 'rsch_owned_by_a_2', reportId: 'rpt_a' }, + ]; + + it("another owner's schedule and a nonexistent id produce IDENTICAL responses", async () => { + const { whenItExists, whenItDoesNot, exists, absent } = + await probe('rsch_owned_by_a', 'rpt_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 — nor + // an agreement on 500, which equality alone would happily accept. + expect(whenItExists).toEqual([ + ['status', [404]], + ['json', [{ code: 'REPORT_NOT_FOUND', error: 'REPORT_NOT_FOUND: rsch_owned_by_a' }]], + ]); + + // The delete never fired for either arm. + expect(exists.deleted).toEqual([]); + expect(absent.deleted).toEqual([]); + }); + + it('reproduces 2× — a second schedule on the same report answers the same way', async () => { + for (const { id, reportId } of A_SCHEDULES) { + const { whenItExists, whenItDoesNot, exists } = await probe(id, reportId, 'user-a', 'user-b'); + expect(whenItExists).toEqual(whenItDoesNot); + expect(whenItExists[0]).toEqual(['status', [404]]); + expect(exists.deleted).toEqual([]); + } + }); + + it('a schedule whose report is gone denies identically too — the third miss shape', async () => { + // `canAccessReport` is false for a schedule that exists but whose parent + // report does not, and the collapse has to cover that arm as well or it + // becomes the next tell: it is reachable whenever a report row is removed + // out from under its schedules. + const orphan = reportsService([{ id: 'rsch_orphan', reportId: 'rpt_gone' }], []); + const absent = reportsService([], []); + + const whenOrphaned = await unscheduleAs(orphan, 'user-b', 'rsch_orphan'); + const whenItDoesNot = await unscheduleAs(absent, 'user-b', 'rsch_orphan'); + + expect(whenOrphaned).toEqual(whenItDoesNot); + expect(whenOrphaned[0]).toEqual(['status', [404]]); + expect(orphan.deleted).toEqual([]); + }); + + it('does not do equal work by refusing everyone — the owner still deletes their own schedule', async () => { + // The cheap way to make two responses equal is to break the feature. + const svc = reportsService([...A_SCHEDULES], [{ id: 'rpt_a', ownerId: 'user-a' }]); + + const owner = await unscheduleAs(svc, 'user-a', 'rsch_owned_by_a'); + + expect(owner).toEqual([['status', [204]], ['end', []]]); + expect(svc.deleted).toEqual(['rsch_owned_by_a']); + expect(svc.unscheduleReport).toHaveBeenCalledWith('rsch_owned_by_a', expect.anything()); + }); + + it('keeps a genuine fault a 500 — the deny mapping did not swallow SCHEDULE_DELETE_FAILED', async () => { + // The overreach in the other direction: routing the catch through + // `handleValidation` must not turn an unrelated failure into a 404. + const svc = reportsService([...A_SCHEDULES], [{ id: 'rpt_a', ownerId: 'user-a' }]); + svc.unscheduleReport = vi.fn(async () => { throw new Error('connection reset by peer'); }) as any; + + const boom = await unscheduleAs(svc, 'user-a', 'rsch_owned_by_a'); + + expect(boom[0]).toEqual(['status', [500]]); + expect((boom[1][1][0] as any).code).toBe('SCHEDULE_DELETE_FAILED'); + }); + + it('still maps VALIDATION_FAILED to 400 — the one emitter serves both codes', async () => { + const svc = reportsService([], []); + const bad = await unscheduleAs(svc, 'user-a', ''); + + expect(bad[0]).toEqual(['status', [400]]); + expect((bad[1][1][0] as any).code).toBe('VALIDATION_FAILED'); + }); + + it('performs the same service calls on both deny arms — no work-shaped tell', async () => { + const { exists, absent } = await probe('rsch_owned_by_a', 'rpt_a', 'user-a', 'user-b'); + const work = (svc: ReturnType) => ({ + unschedule: svc.unscheduleReport.mock.calls.length, + deleted: svc.deleted.length, + }); + + // One call, 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({ unschedule: 1, deleted: 0 }); + }); +}); diff --git a/packages/spec/src/contracts/report-service.ts b/packages/spec/src/contracts/report-service.ts index a944f54eac..9c3249e81d 100644 --- a/packages/spec/src/contracts/report-service.ts +++ b/packages/spec/src/contracts/report-service.ts @@ -147,7 +147,24 @@ export interface IReportService { /** Create or update a schedule. */ scheduleReport(input: ScheduleReportInput, context: ExecutionContext): Promise; - /** Remove a schedule by id. */ + /** + * Remove a schedule by id. + * + * MUST reject every schedule the caller cannot see with the SAME + * `REPORT_NOT_FOUND: ` error — one that does not exist and one + * owned by somebody else alike — and MUST take that decision before the + * delete fires. Resolving quietly for the unknown id (the "idempotent + * delete" reflex) while throwing for the cross-owner id is what makes + * `DELETE /reports/schedules/:scheduleId` an enumeration oracle over other + * owners' schedule ids: the caller can delete neither, but learns which of + * the two they hit (#7603 — #7523 is the same defect on `deleteReport`). + * + * The obligation is stated here because it cannot be enforced at the route. + * `deleteReport`'s two arms are pre-empted by `getReport`, which is already + * blind to the same difference (#2980); this contract has no by-id schedule + * read to do that with — `listSchedules` is keyed by reportId — so a caller + * holding only a scheduleId depends on the implementation to blind itself. + */ unscheduleReport(scheduleId: string, context: ExecutionContext): Promise; /** List schedules — optionally filtered by report. */