fix(plugin-reports): stop DELETE /reports/schedules/:scheduleId revealing whether a schedule id exists (#7603)

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

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 () => {

packages/plugins/plugin-reports/src/report-service.ts

@@ -598,13 +598,34 @@ export class ReportService implements IReportService {
 
   async unscheduleReport(scheduleId: string, context: ExecutionContext): Promise<void> {
     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}`);
     }

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: <scheduleId>` 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) {

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