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