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

`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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Q4Zy4fY7Kj8t27QAEzr9G1

Author: claude

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