From edeea847b1939329d2f01ac24e7410b98b1da6ec Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 8 Aug 2026 12:27:26 +0000 Subject: [PATCH] fix(reports): owner-gate the saved-report schedule routes (#2980) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `unscheduleReport` and `listSchedules` took the caller context as `_context` and never consulted it, querying under the RLS-bypassing system context. Any authenticated caller could delete another owner's report schedule (a cross-owner destructive write) or list another owner's schedules (leaking recipients + cron) by supplying an id, even though the sibling read/run/delete routes are all owner-isolated. Both now resolve the schedule's parent report and require the caller to own it, mirroring the sibling routes: - unscheduleReport: loads the schedule, then its report, and deletes only when canAccessReport holds; a cross-owner attempt throws REPORT_NOT_FOUND (mapped to 404 in the REST layer — deny-as-404, anti-enumeration), while a genuinely-absent schedule stays idempotent. Create was already gated via getReport, so only the delete/list doors were open. - listSchedules: returns an empty list to any non-system caller who cannot access the report it is scoped to — the same non-leaking posture as listReports. The scheduler's system context still sees every schedule. Tests: 5 new owner-gate cases in report-service.test.ts (cross-owner delete denied + schedule survives, unknown-id idempotent, cross-owner list empty, system context still lists). No authoring-surface or metadata change. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01L8aEBrJVxRnA5XVRkeVft9 --- .changeset/report-schedule-owner-gate.md | 30 ++++++++++++++++++ .../plugin-reports/src/report-service.test.ts | 28 +++++++++++++++++ .../plugin-reports/src/report-service.ts | 31 +++++++++++++++++-- packages/rest/src/rest-server.ts | 1 + 4 files changed, 88 insertions(+), 2 deletions(-) create mode 100644 .changeset/report-schedule-owner-gate.md diff --git a/.changeset/report-schedule-owner-gate.md b/.changeset/report-schedule-owner-gate.md new file mode 100644 index 0000000000..7bb21355ef --- /dev/null +++ b/.changeset/report-schedule-owner-gate.md @@ -0,0 +1,30 @@ +--- +"@objectstack/plugin-reports": patch +"@objectstack/rest": patch +--- + +fix(reports): owner-gate the saved-report schedule routes (#2980) + +The report read/run/delete routes are owner-isolated (a caller may only touch a +report they own, denied as `REPORT_NOT_FOUND` to avoid leaking that the id +exists), but the two schedule routes bypassed that gate: `unscheduleReport` and +`listSchedules` took the caller `context` as `_context` and never consulted it, +querying under the system context (RLS-bypassing). Any authenticated caller +could therefore delete another owner's report schedule — a cross-owner +destructive write — or list another owner's schedules (leaking recipient +addresses and cron), by supplying an id. + +Both now resolve the schedule's parent report and require the caller to own it, +mirroring the sibling routes: + +- **`unscheduleReport`** loads the schedule, then its report, and deletes only + when `canAccessReport` holds; a cross-owner attempt throws `REPORT_NOT_FOUND` + (mapped to `404` by the REST layer, deny-as-404 anti-enumeration), while a + genuinely-absent schedule stays idempotent. `scheduleReport` (create) was + already gated via `getReport`, so only the delete/list doors were open. +- **`listSchedules`** returns an empty list to any non-system caller who cannot + access the report it is scoped to — the same non-leaking posture as + `listReports`. The scheduler's system context still sees every schedule. + +No authoring-surface or metadata change; existing owner-path behavior is +unchanged. diff --git a/packages/plugins/plugin-reports/src/report-service.test.ts b/packages/plugins/plugin-reports/src/report-service.test.ts index 0006ce0869..ccb8bc4a3f 100644 --- a/packages/plugins/plugin-reports/src/report-service.test.ts +++ b/packages/plugins/plugin-reports/src/report-service.test.ts @@ -443,6 +443,34 @@ describe('ReportService', () => { expect(all.length).toBe(2); }); + it('unscheduleReport: a non-owner cannot delete another user\'s schedule', async () => { + const r = await svc.saveReport({ name: 'Mine', object: 'lead', query: {} }, CTX); + const s = await svc.scheduleReport({ reportId: r.id, recipients: ['x@t'] }, CTX); + // stranger is denied as not-found and the schedule survives untouched + await expect(svc.unscheduleReport(s.id, OTHER)).rejects.toThrow(/REPORT_NOT_FOUND/); + expect(engine._tables['sys_report_schedule'].length).toBe(1); + // owner can + await svc.unscheduleReport(s.id, CTX); + 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(); + }); + + it('listSchedules: a non-owner cannot see another user\'s schedules', async () => { + const r = await svc.saveReport({ name: 'Mine', object: 'lead', query: {} }, CTX); + await svc.scheduleReport({ reportId: r.id, recipients: ['x@t'] }, CTX); + expect((await svc.listSchedules({ reportId: r.id }, OTHER)).length).toBe(0); // stranger sees nothing + expect((await svc.listSchedules({ reportId: r.id }, CTX)).length).toBe(1); // owner sees it + }); + + it('listSchedules: system context (dispatcher) still sees schedules', async () => { + const r = await svc.saveReport({ name: 'Mine', object: 'lead', query: {} }, CTX); + await svc.scheduleReport({ reportId: r.id, recipients: ['x@t'] }, CTX); + expect((await svc.listSchedules({ reportId: r.id }, { isSystem: true } as any)).length).toBe(1); + }); + it('dispatchDue: fails closed (no RLS bypass) when no owner resolver is configured', async () => { const noResolver = new ReportService({ engine: engine as any, email, clock: { now: () => now } }); const r = await noResolver.saveReport({ name: 'A', object: 'lead', query: {} }, CTX); diff --git a/packages/plugins/plugin-reports/src/report-service.ts b/packages/plugins/plugin-reports/src/report-service.ts index 5c9b682d1f..00d49641b5 100644 --- a/packages/plugins/plugin-reports/src/report-service.ts +++ b/packages/plugins/plugin-reports/src/report-service.ts @@ -312,6 +312,14 @@ export class ReportService implements IReportService { return Array.isArray(rows) && rows[0] ? rows[0] : null; } + /** Raw metadata read of a report schedule by id (no authz — callers gate). */ + private async loadScheduleRow(scheduleId: string): Promise { + const rows = await this.engine.find('sys_report_schedule', { + where: { id: scheduleId }, limit: 1, context: SYSTEM_CTX, + }); + return Array.isArray(rows) && rows[0] ? rows[0] : null; + } + // ── Report CRUD ──────────────────────────────────────────────── async saveReport(input: SaveReportInput, context: SharingExecutionContext): Promise { @@ -532,15 +540,34 @@ export class ReportService implements IReportService { return rowFromSchedule(row); } - async unscheduleReport(scheduleId: string, _context: SharingExecutionContext): Promise { + async unscheduleReport(scheduleId: string, context: SharingExecutionContext): 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); + if (!this.canAccessReport(report, context)) { + throw new Error(`REPORT_NOT_FOUND: ${scheduleId}`); + } await this.engine.delete('sys_report_schedule', { where: { id: scheduleId }, context: SYSTEM_CTX }); } async listSchedules( filter: { reportId?: string } | undefined, - _context: SharingExecutionContext, + context: SharingExecutionContext, ): Promise { + // Schedules are owned through their report (#2980): a non-system caller may + // only list the schedules of a report they can access. The route always + // supplies the parent report id; a caller who cannot see that report gets an + // empty list — never another owner's recipients/cron — the same non-leaking + // posture as listReports. System/tooling (the dispatcher) still sees all. + if (!context?.isSystem) { + if (!filter?.reportId) return []; + if (!(await this.getReport(filter.reportId, context))) return []; + } const f: any = {}; if (filter?.reportId) f.report_id = filter.reportId; const rows = await this.engine.find('sys_report_schedule', { diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index 48d9e9ff2e..f0d4387c58 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -8451,6 +8451,7 @@ export class RestServer { await svc.unscheduleReport(req.params.scheduleId, context ?? {}); res.status(204).end(); } catch (error: any) { + if (handleValidation(res, error)) return; // REPORT_NOT_FOUND → 404 (deny-as-404, anti-enumeration) logError('[REST] Unschedule report error:', error); res.status(500).json({ code: 'SCHEDULE_DELETE_FAILED', error: String(error?.message ?? error).slice(0, 500) }); }