Skip to content

Commit edeea84

Browse files
committed
fix(reports): owner-gate the saved-report schedule routes (#2980)
`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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01L8aEBrJVxRnA5XVRkeVft9
1 parent b3efeb7 commit edeea84

4 files changed

Lines changed: 88 additions & 2 deletions

File tree

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
---
2+
"@objectstack/plugin-reports": patch
3+
"@objectstack/rest": patch
4+
---
5+
6+
fix(reports): owner-gate the saved-report schedule routes (#2980)
7+
8+
The report read/run/delete routes are owner-isolated (a caller may only touch a
9+
report they own, denied as `REPORT_NOT_FOUND` to avoid leaking that the id
10+
exists), but the two schedule routes bypassed that gate: `unscheduleReport` and
11+
`listSchedules` took the caller `context` as `_context` and never consulted it,
12+
querying under the system context (RLS-bypassing). Any authenticated caller
13+
could therefore delete another owner's report schedule — a cross-owner
14+
destructive write — or list another owner's schedules (leaking recipient
15+
addresses and cron), by supplying an id.
16+
17+
Both now resolve the schedule's parent report and require the caller to own it,
18+
mirroring the sibling routes:
19+
20+
- **`unscheduleReport`** loads the schedule, then its report, and deletes only
21+
when `canAccessReport` holds; a cross-owner attempt throws `REPORT_NOT_FOUND`
22+
(mapped to `404` by the REST layer, deny-as-404 anti-enumeration), while a
23+
genuinely-absent schedule stays idempotent. `scheduleReport` (create) was
24+
already gated via `getReport`, so only the delete/list doors were open.
25+
- **`listSchedules`** returns an empty list to any non-system caller who cannot
26+
access the report it is scoped to — the same non-leaking posture as
27+
`listReports`. The scheduler's system context still sees every schedule.
28+
29+
No authoring-surface or metadata change; existing owner-path behavior is
30+
unchanged.

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

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -443,6 +443,34 @@ describe('ReportService', () => {
443443
expect(all.length).toBe(2);
444444
});
445445

446+
it('unscheduleReport: a non-owner cannot delete another user\'s schedule', async () => {
447+
const r = await svc.saveReport({ name: 'Mine', object: 'lead', query: {} }, CTX);
448+
const s = await svc.scheduleReport({ reportId: r.id, recipients: ['x@t'] }, CTX);
449+
// stranger is denied as not-found and the schedule survives untouched
450+
await expect(svc.unscheduleReport(s.id, OTHER)).rejects.toThrow(/REPORT_NOT_FOUND/);
451+
expect(engine._tables['sys_report_schedule'].length).toBe(1);
452+
// owner can
453+
await svc.unscheduleReport(s.id, CTX);
454+
expect(engine._tables['sys_report_schedule'].length).toBe(0);
455+
});
456+
457+
it('unscheduleReport: an unknown schedule id is idempotent, not a leak', async () => {
458+
await expect(svc.unscheduleReport('rsch_nope', OTHER)).resolves.toBeUndefined();
459+
});
460+
461+
it('listSchedules: a non-owner cannot see another user\'s schedules', async () => {
462+
const r = await svc.saveReport({ name: 'Mine', object: 'lead', query: {} }, CTX);
463+
await svc.scheduleReport({ reportId: r.id, recipients: ['x@t'] }, CTX);
464+
expect((await svc.listSchedules({ reportId: r.id }, OTHER)).length).toBe(0); // stranger sees nothing
465+
expect((await svc.listSchedules({ reportId: r.id }, CTX)).length).toBe(1); // owner sees it
466+
});
467+
468+
it('listSchedules: system context (dispatcher) still sees schedules', async () => {
469+
const r = await svc.saveReport({ name: 'Mine', object: 'lead', query: {} }, CTX);
470+
await svc.scheduleReport({ reportId: r.id, recipients: ['x@t'] }, CTX);
471+
expect((await svc.listSchedules({ reportId: r.id }, { isSystem: true } as any)).length).toBe(1);
472+
});
473+
446474
it('dispatchDue: fails closed (no RLS bypass) when no owner resolver is configured', async () => {
447475
const noResolver = new ReportService({ engine: engine as any, email, clock: { now: () => now } });
448476
const r = await noResolver.saveReport({ name: 'A', object: 'lead', query: {} }, CTX);

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

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -312,6 +312,14 @@ export class ReportService implements IReportService {
312312
return Array.isArray(rows) && rows[0] ? rows[0] : null;
313313
}
314314

315+
/** Raw metadata read of a report schedule by id (no authz — callers gate). */
316+
private async loadScheduleRow(scheduleId: string): Promise<any | null> {
317+
const rows = await this.engine.find('sys_report_schedule', {
318+
where: { id: scheduleId }, limit: 1, context: SYSTEM_CTX,
319+
});
320+
return Array.isArray(rows) && rows[0] ? rows[0] : null;
321+
}
322+
315323
// ── Report CRUD ────────────────────────────────────────────────
316324

317325
async saveReport(input: SaveReportInput, context: SharingExecutionContext): Promise<SavedReport> {
@@ -532,15 +540,34 @@ export class ReportService implements IReportService {
532540
return rowFromSchedule(row);
533541
}
534542

535-
async unscheduleReport(scheduleId: string, _context: SharingExecutionContext): Promise<void> {
543+
async unscheduleReport(scheduleId: string, context: SharingExecutionContext): Promise<void> {
536544
if (!scheduleId) throw new Error('VALIDATION_FAILED: scheduleId is required');
545+
const schedule = await this.loadScheduleRow(scheduleId);
546+
if (!schedule) return; // idempotent — nothing to drop (mirrors deleteReport)
547+
// A schedule is owned through its report (#2980): a caller may only delete
548+
// the schedules of a report they own. Others get a not-found so the delete
549+
// neither fires nor reveals the schedule's existence — deny-as-404, never a
550+
// cross-owner 2xx.
551+
const report = await this.loadReportRow(schedule.report_id);
552+
if (!this.canAccessReport(report, context)) {
553+
throw new Error(`REPORT_NOT_FOUND: ${scheduleId}`);
554+
}
537555
await this.engine.delete('sys_report_schedule', { where: { id: scheduleId }, context: SYSTEM_CTX });
538556
}
539557

540558
async listSchedules(
541559
filter: { reportId?: string } | undefined,
542-
_context: SharingExecutionContext,
560+
context: SharingExecutionContext,
543561
): Promise<ReportSchedule[]> {
562+
// Schedules are owned through their report (#2980): a non-system caller may
563+
// only list the schedules of a report they can access. The route always
564+
// supplies the parent report id; a caller who cannot see that report gets an
565+
// empty list — never another owner's recipients/cron — the same non-leaking
566+
// posture as listReports. System/tooling (the dispatcher) still sees all.
567+
if (!context?.isSystem) {
568+
if (!filter?.reportId) return [];
569+
if (!(await this.getReport(filter.reportId, context))) return [];
570+
}
544571
const f: any = {};
545572
if (filter?.reportId) f.report_id = filter.reportId;
546573
const rows = await this.engine.find('sys_report_schedule', {

packages/rest/src/rest-server.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8451,6 +8451,7 @@ export class RestServer {
84518451
await svc.unscheduleReport(req.params.scheduleId, context ?? {});
84528452
res.status(204).end();
84538453
} catch (error: any) {
8454+
if (handleValidation(res, error)) return; // REPORT_NOT_FOUND → 404 (deny-as-404, anti-enumeration)
84548455
logError('[REST] Unschedule report error:', error);
84558456
res.status(500).json({ code: 'SCHEDULE_DELETE_FAILED', error: String(error?.message ?? error).slice(0, 500) });
84568457
}

0 commit comments

Comments
 (0)