From 6644f2280551aecff77d5762498e5558f0bd0f3a Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 27 Jul 2026 09:01:51 +0000 Subject: [PATCH 1/2] fix(approvals)!: scope approval requests to their participants, not the tenant (#3590) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit getRequest / listRequests / countRequests deliberately query with SYSTEM_CTX to bypass RLS — as their own comments say, the approver-visibility rule spans identity forms RLS cannot model cleanly, so it has to be expressed in the service. Only the TENANT half of that rule was ever written. The comment names the participant half; the code never applied it. So any authenticated user could read any approval request in their tenant: its payload snapshot, its full decision history, and — once decision attachments derived their access from the request (#3580) — its files. `approverId` on listRequests is a FILTER, not authorization. Omitting it returned the whole tenant. A caller now sees a request when they participate in it: the submitter, a current approver (via the normalized approver index, so every identity form the write path recorded is covered), or someone who already acted on it — a past approver whose slot has moved on, a commenter. Override admins keep the unrestricted view the "all requests" console surface depends on; a tokenless context sees nothing. Keying on the concrete user id is sufficient rather than an approximation, and that is worth stating because under-granting here would silently hide approvals someone must act on: position/team/manager/field approvers are resolved to concrete user ids at open time, and the `type:value` literal is only the fallback for a spec that resolved to NOBODY — a slot no one can act on either way, since can_act is a plain membership test over the resolved ids. A write path's own result is NOT re-gated. Every operation echoes back the request it just changed via getRequest; the operation authorized itself before writing, and re-asking answers a settled question — wrongly, for a context that carries no userId (a flow-driven resume, a service-to-service call), turning a successful write into a null result. Four existing tests caught exactly that, which is why the echo now goes through readBackRequest instead. Verified in the browser on app-showcase: a non-admin approver's unfiltered list drops from 3 requests to the 1 she is on, an admin still sees all 3, her decision attachment still downloads (200, real PDF bytes — #3580 composes), and a request she has nothing to do with reads back empty. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01SHpGw3GBA9aFpfwVArRWfd --- .changeset/approval-participant-visibility.md | 39 +++++ .../src/approval-service.test.ts | 118 +++++++++++++- .../plugin-approvals/src/approval-service.ts | 153 ++++++++++++++++-- 3 files changed, 296 insertions(+), 14 deletions(-) create mode 100644 .changeset/approval-participant-visibility.md diff --git a/.changeset/approval-participant-visibility.md b/.changeset/approval-participant-visibility.md new file mode 100644 index 000000000..4a7ec6bb1 --- /dev/null +++ b/.changeset/approval-participant-visibility.md @@ -0,0 +1,39 @@ +--- +"@objectstack/plugin-approvals": patch +--- + +fix(approvals)!: an approval request is visible to its participants, not to the whole tenant (#3590) + +`getRequest` / `listRequests` / `countRequests` deliberately query with +`SYSTEM_CTX` to bypass RLS — as the code comments say, the approver-visibility +rule spans identity forms RLS cannot model cleanly, so it has to be expressed in +the service. Only the **tenant** half of that rule was ever applied. The +participant half was named in the comment and never written, so **any +authenticated user could read any approval request in their tenant** — its +payload snapshot, its full decision history, and (once decision attachments +derived their access from the request, #3580) its files. + +`approverId` on `listRequests` is a *filter*, not authorization: omitting it +returned the whole tenant. + +A caller now sees a request when they are a participant — the submitter, a +current approver (via the normalized approver index, so every identity form the +write path recorded is covered), or someone who has already acted on it (a past +approver whose slot has moved on, a commenter). Admins with override authority +keep the unrestricted view the "all requests" console surface depends on, and a +tokenless context sees nothing. + +Keying on the concrete user id is sufficient rather than an approximation: +position/team/manager/field approvers are resolved to concrete user ids at open +time, and the `type:value` literal is only the fallback for a spec that resolved +to *nobody* — a slot no one can act on either way. So this cannot hide a request +from someone who could actually act on it. + +**A write path's own result is not re-gated.** Every operation echoes back the +request it just changed; the operation already authorized itself, and re-asking +would answer wrong for a context carrying no `userId` (a flow-driven resume, a +service-to-service call), turning a successful write into `null`. + +Marked breaking because a client that listed requests without an `approverId` +filter and expected the whole tenant will now receive only its own — which is +the point. diff --git a/packages/plugins/plugin-approvals/src/approval-service.test.ts b/packages/plugins/plugin-approvals/src/approval-service.test.ts index 6af59cee8..fe4b419ac 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.test.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.test.ts @@ -740,8 +740,10 @@ describe('ApprovalService (node era)', () => { expect(asApprover!.viewer).toEqual({ can_act: true, is_submitter: false, can_override: false }); const asSubmitter = await svc.getRequest(req.id, { userId: 'u1', tenantId: 't1' } as any); expect(asSubmitter!.viewer).toEqual({ can_act: false, is_submitter: true, can_override: false }); - const asOther = await svc.getRequest(req.id, { userId: 'u_stranger', tenantId: 't1' } as any); - expect(asOther!.viewer).toEqual({ can_act: false, is_submitter: false, can_override: false }); + // #3590: a same-tenant stranger participates in nothing, so the request is + // not readable at all — previously it came back with an all-false viewer + // block, which meant every authenticated user could read every request. + expect(await svc.getRequest(req.id, { userId: 'u_stranger', tenantId: 't1' } as any)).toBeNull(); }); it('getRequest: viewer.can_act drops to false once the request is finalized', async () => { @@ -1477,12 +1479,22 @@ describe('ApprovalService — admin override (#3424)', () => { const req = await svc.openNodeRequest(stuckInput(), CTX); const asAdmin = await svc.getRequest(req.id, TENANT_ADMIN); expect(asAdmin!.viewer).toMatchObject({ can_act: false, can_override: true }); - const asMember = await svc.getRequest(req.id, MEMBER); - expect(asMember!.viewer!.can_override).toBe(false); + // Someone who CAN see the request but holds no override privilege — the + // submitter. `can_override` is about the privilege, not about access, so + // the check needs a participant rather than a stranger. + const asSubmitter = await svc.getRequest(req.id, CTX); + expect(asSubmitter!.viewer!.can_override).toBe(false); await svc.decide(req.id, { decision: 'approve', actorId: 'owner' }, TENANT_ADMIN); const after = await svc.getRequest(req.id, TENANT_ADMIN); expect(after!.viewer!.can_override).toBe(false); }); + + // #3590: a plain member who participates in nothing now sees nothing — a + // request is no longer readable merely because you are in the same tenant. + it('a non-participant member cannot read the request at all', async () => { + const req = await svc.openNodeRequest(stuckInput(), CTX); + expect(await svc.getRequest(req.id, MEMBER)).toBeNull(); + }); }); describe('record-lock hook (node era)', () => { @@ -2114,3 +2126,101 @@ describe('ApprovalService — authorizeFileRead delegate (ADR-0104 D3 wave 2)', expect((SysApprovalAction as any).fileAccessDelegate).toBe('approvals'); }); }); + +// ── Participant visibility (#3590) ─────────────────────────────────── +// +// getRequest/listRequests deliberately query with SYSTEM_CTX (the +// approver-visibility rule spans identity forms RLS cannot model), but only +// the TENANT half of that rule was ever applied — so any authenticated user +// could read any request in their tenant, and after #3580 its decision +// attachments too. These lock in the participant half. +describe('ApprovalService — participant visibility (#3590)', () => { + const svcFor = (engine: any) => { + let n = 0; + return new ApprovalService({ engine, clock: { now: () => new Date(1757000000000 + (n++) * 1000) } }); + }; + const asUser = (userId: string) => ({ userId, tenantId: 't1', positions: [], permissions: [] } as any); + const ADMIN = { userId: 'root', tenantId: 't1', positions: [], permissions: ['admin_full_access'] } as any; + + it('the submitter and a pending approver can read it; a same-tenant stranger cannot', async () => { + const engine = makeFakeEngine(); + const svc = svcFor(engine); + const req = await svc.openNodeRequest(openInput(['u9']), CTX); // submitter u1, approver u9 + + expect(await svc.getRequest(req.id, asUser('u1'))).not.toBeNull(); + expect(await svc.getRequest(req.id, asUser('u9'))).not.toBeNull(); + expect(await svc.getRequest(req.id, asUser('u_stranger'))).toBeNull(); + }); + + it('someone who already acted keeps access after their slot moves on', async () => { + const engine = makeFakeEngine(); + const svc = svcFor(engine); + const req = await svc.openNodeRequest(openInput(['u9']), CTX); + await svc.decideNode(req.id, { decision: 'approve', actorId: 'u9' }, SYS); + + // u9 is no longer a pending approver, but the decision is theirs — the + // audit trail (and its attachments) must not vanish from under them. + expect(await svc.getRequest(req.id, asUser('u9'))).not.toBeNull(); + }); + + it('an override admin keeps the unrestricted view the console depends on', async () => { + const engine = makeFakeEngine(); + const svc = svcFor(engine); + const req = await svc.openNodeRequest(openInput(['u9']), CTX); + + expect(await svc.getRequest(req.id, ADMIN)).not.toBeNull(); + }); + + it('a tokenless context sees nothing — the gate fails closed', async () => { + const engine = makeFakeEngine(); + const svc = svcFor(engine); + const req = await svc.openNodeRequest(openInput(['u9']), CTX); + + expect(await svc.getRequest(req.id, { tenantId: 't1', positions: [], permissions: [] } as any)).toBeNull(); + }); + + it('listRequests no longer returns the whole tenant when no approverId filter is passed', async () => { + const engine = makeFakeEngine(); + const svc = svcFor(engine); + const mine = await svc.openNodeRequest(openInput(['u9']), CTX); // submitter u1 + await svc.openNodeRequest(openInput(['u7'], { recordId: 'opp2', record: { id: 'opp2' } }), asUser('u_other')); // unrelated to u1 + + // The old behaviour: omit approverId and receive every request in the + // tenant. `approverId` is a filter, never authorization. + const seen = await svc.listRequests(undefined, asUser('u1')); + expect(seen.map(r => r.id)).toEqual([mine.id]); + + const strangerSees = await svc.listRequests(undefined, asUser('u_stranger')); + expect(strangerSees).toEqual([]); + + // The admin console still sees everything. + expect((await svc.listRequests(undefined, ADMIN)).length).toBeGreaterThanOrEqual(2); + }); + + it('countRequests agrees with the list it paginates', async () => { + const engine = makeFakeEngine(); + const svc = svcFor(engine); + await svc.openNodeRequest(openInput(['u9']), CTX); + await svc.openNodeRequest(openInput(['u7'], { recordId: 'opp2', record: { id: 'opp2' } }), asUser('u_other')); + + expect(await svc.countRequests(undefined, asUser('u_stranger'))).toBe(0); + expect(await svc.countRequests(undefined, asUser('u1'))).toBe(1); + }); + + it('a write path still echoes back its own result when the context has no userId', async () => { + const engine = makeFakeEngine(); + const svc = svcFor(engine); + const req = await svc.openNodeRequest(openInput(['u9']), CTX); + + // Flow-driven resumes and service-to-service calls carry no userId. The + // operation authorized itself; re-gating the echo would turn a successful + // write into a null result. + const res = await svc.decideNode( + req.id, + { decision: 'approve', actorId: 'u9' }, + { isSystem: false, positions: [], permissions: [] } as any, + ); + expect(res.request).not.toBeNull(); + expect(res.request.status).toBe('approved'); + }); +}); diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index e73533d57..9aa825114 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -1377,7 +1377,7 @@ export class ApprovalService implements IApprovalService { } : {}), }, { context: SYSTEM_CTX }); await this.syncApproverIndex(requestId, stillPending, org, now); - const fresh = await this.getRequest(requestId, context); + const fresh = await this.readBackRequest(requestId, context); return { request: fresh!, runId, nodeId, finalized: false, decision: input.decision }; } } @@ -1400,7 +1400,7 @@ export class ApprovalService implements IApprovalService { if (config.approvalStatusField) { await this.mirrorStatusField(raw.object_name, raw.record_id, config.approvalStatusField, finalStatus); } - const fresh = await this.getRequest(requestId, context); + const fresh = await this.readBackRequest(requestId, context); return { request: fresh!, runId, nodeId, finalized: true, decision: input.decision, outputs: mergedOutputs }; } @@ -1535,7 +1535,7 @@ export class ApprovalService implements IApprovalService { } } - const fresh = await this.getRequest(requestId, context); + const fresh = await this.readBackRequest(requestId, context); return { request: fresh!, runId, resumed }; } @@ -1634,7 +1634,7 @@ export class ApprovalService implements IApprovalService { }, }); } - const fresh = await this.getRequest(requestId, context); + const fresh = await this.readBackRequest(requestId, context); return { request: fresh!, runId, resumed, autoRejected: true }; } @@ -1675,7 +1675,7 @@ export class ApprovalService implements IApprovalService { }); } - const fresh = await this.getRequest(requestId, context); + const fresh = await this.readBackRequest(requestId, context); return { request: fresh!, runId, resumed }; } @@ -1748,7 +1748,7 @@ export class ApprovalService implements IApprovalService { } } - const fresh = await this.getRequest(requestId, context); + const fresh = await this.readBackRequest(requestId, context); return { request: fresh!, runId, resumed }; } @@ -1876,7 +1876,7 @@ export class ApprovalService implements IApprovalService { }, }); - const fresh = await this.getRequest(requestId, context); + const fresh = await this.readBackRequest(requestId, context); return { request: fresh! }; } @@ -1959,7 +1959,7 @@ export class ApprovalService implements IApprovalService { }); } - const fresh = await this.getRequest(requestId, context); + const fresh = await this.readBackRequest(requestId, context); return { request: fresh!, notified }; } @@ -2098,7 +2098,7 @@ export class ApprovalService implements IApprovalService { }); } - const fresh = await this.getRequest(requestId, context); + const fresh = await this.readBackRequest(requestId, context); return { request: fresh! }; } @@ -2140,7 +2140,7 @@ export class ApprovalService implements IApprovalService { }, }); - const fresh = await this.getRequest(requestId, context); + const fresh = await this.readBackRequest(requestId, context); return { request: fresh! }; } @@ -2691,6 +2691,98 @@ export class ApprovalService implements IApprovalService { return [...new Set(list.map(r => String(r.request_id)))]; } + /** + * The request ids this caller is a PARTICIPANT of, or `null` for a caller + * who may see everything in scope (#3590). + * + * These reads deliberately run with `SYSTEM_CTX` to bypass RLS — the + * approver-visibility rule spans several identity forms that RLS cannot model + * cleanly, which is why it has to be expressed here. Until now only the + * TENANT half of that rule was applied, so any authenticated user could read + * any request in their tenant (and, once attachments derived their access + * from the request, its files too). This adds the participant half. + * + * A participant is the submitter, a current approver, or someone who has + * already acted on the request (a past approver whose slot has moved on, a + * commenter). Admins with override authority keep the unrestricted view the + * "all requests" console surface depends on. + * + * Keying on the concrete user id is sufficient rather than an approximation: + * position/team/manager/field approvers are resolved to concrete user ids at + * open time, and the `type:value` literal is only the fallback for a spec + * that resolved to NOBODY — a slot no one can act on either way (`can_act` + * is a plain membership test over the resolved ids). So this cannot hide a + * request from someone who could actually act on it. + */ + private async visibleRequestIds( + context: SharingExecutionContext, + tenantOrg: string | null, + ): Promise | null> { + if (this.isOverrideActor(context, tenantOrg)) return null; + const uid = (context as any)?.userId != null ? String((context as any).userId) : ''; + // A tokenless/anonymous caller participates in nothing. Fail closed. + if (!uid) return new Set(); + + const ids = new Set(); + const cap = ApprovalService.APPROVER_INDEX_CAP; + const add = (rows: unknown, key: string) => { + const list: any[] = Array.isArray(rows) ? rows : []; + for (const r of list) if (r?.[key] != null) ids.add(String(r[key])); + if (list.length >= cap) { + this.logger?.warn?.( + '[approvals] participant-visibility probe hit its window — some requests may be hidden from a legitimate participant', + { cap, key }, + ); + } + }; + + try { + // Current approver — via the normalized index, so every identity form + // the write path recorded is covered. + for (const id of (await this.approverRequestIds([uid], tenantOrg)) ?? []) ids.add(id); + + const orgWhere = tenantOrg ? { organization_id: tenantOrg } : {}; + add( + await this.engine.find('sys_approval_request', { + where: { submitter_id: uid, ...orgWhere }, + fields: ['id'], limit: cap, context: SYSTEM_CTX, + }), + 'id', + ); + // Already acted on it: a past approver whose slot has moved on, or a + // commenter. They saw it legitimately; keep it that way. + add( + await this.engine.find('sys_approval_action', { + where: { actor_id: uid }, + fields: ['request_id'], limit: cap, context: SYSTEM_CTX, + }), + 'request_id', + ); + } catch (err) { + // Never widen on error: a failed probe yields whatever was collected. + this.logger?.warn?.('[approvals] participant-visibility probe failed', { + error: err instanceof Error ? err.message : String(err), + }); + } + return ids; + } + + /** Intersect an existing `where.id` constraint with the participant set. */ + private applyVisibility(where: any, visible: Set | null): boolean { + if (!visible) return true; + if (visible.size === 0) return false; + let allowed = [...visible]; + const current = where.id; + if (typeof current === 'string') allowed = allowed.filter((x) => x === current); + else if (current && typeof current === 'object' && Array.isArray(current.$in)) { + const set = new Set(current.$in.map((v: unknown) => String(v))); + allowed = allowed.filter((x) => set.has(x)); + } + if (allowed.length === 0) return false; + where.id = allowed.length === 1 ? allowed[0] : { $in: allowed }; + return true; + } + async listRequests( filter: { object?: string; @@ -2718,6 +2810,11 @@ export class ApprovalService implements IApprovalService { where.id = ids.length === 1 ? ids[0] : { $in: ids }; } + // #3590: the caller-supplied `approverId` is a FILTER, not authorization — + // omitting it used to return every request in the tenant. Intersect with + // what this caller actually participates in. + if (!this.applyVisibility(where, await this.visibleRequestIds(context, tenantOrg))) return []; + const findOpts: any = { where, orderBy: [{ field: 'created_at', order: 'desc' }], @@ -2753,6 +2850,9 @@ export class ApprovalService implements IApprovalService { where.id = ids.length === 1 ? ids[0] : { $in: ids }; } + // #3590 — the count must agree with the list it paginates. + if (!this.applyVisibility(where, await this.visibleRequestIds(context, tenantOrg))) return 0; + const countFn = (this.engine as any).count; if (typeof countFn === 'function') { try { @@ -2769,7 +2869,33 @@ export class ApprovalService implements IApprovalService { return Array.isArray(rows) ? rows.length : 0; } + /** + * Read the request a write path just changed, to echo back as its result. + * + * NOT participant-gated (#3590), deliberately: the operation authorized + * itself by its own rule before writing, so re-asking "may you see this?" + * for the echo answers a question that has already been settled — and would + * answer it WRONG for a caller context that carries no `userId` (a + * flow-driven resume, a service-to-service call), turning a successful write + * into a `null` result. Gating belongs on the read API, not on an + * operation's own return value. + */ + private async readBackRequest( + requestId: string, + context: SharingExecutionContext, + ): Promise { + return this.loadRequest(requestId, context, false); + } + async getRequest(requestId: string, context: SharingExecutionContext): Promise { + return this.loadRequest(requestId, context, true); + } + + private async loadRequest( + requestId: string, + context: SharingExecutionContext, + enforceVisibility: boolean, + ): Promise { if (!requestId) return null; const where: any = { id: requestId }; const tenantOrg = (context as any)?.organizationId ?? (context as any)?.tenantId; @@ -2778,6 +2904,13 @@ export class ApprovalService implements IApprovalService { where, limit: 1, context: SYSTEM_CTX, }); if (!Array.isArray(rows) || !rows[0]) return null; + // #3590: tenant scoping alone let any authenticated user read any request + // — and, once decision attachments derived their access from the request + // (#3580), its files too. Participation is the rest of the rule. + if (enforceVisibility) { + const visible = await this.visibleRequestIds(context, tenantOrg ?? null); + if (visible && !visible.has(String(rows[0].id))) return null; + } const row = rowFromRequest(rows[0]); await this.enrichRows([row]); await this.attachFlowSteps(row); From 042f91feeef59bb331aadbba666c5bcb3c1bd93b Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 27 Jul 2026 09:13:17 +0000 Subject: [PATCH 2/2] docs(approvals): state who can see a request, now that it is participant-scoped MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The drift check flagged approvals.mdx, and rightly: it documented `approverId` as one filter among several and never said who may see what — so a reader would reasonably assume omitting it returns the whole tenant, which is exactly what this PR stops being true. Also records that a decision's attachments inherit the same rule, since that is the non-obvious half: the file is as readable as the decision, never more. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01SHpGw3GBA9aFpfwVArRWfd --- content/docs/automation/approvals.mdx | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/content/docs/automation/approvals.mdx b/content/docs/automation/approvals.mdx index 78ec3b770..b803113df 100644 --- a/content/docs/automation/approvals.mdx +++ b/content/docs/automation/approvals.mdx @@ -244,6 +244,20 @@ a person's identities in one call. Other filters: `status`, `object`, `recordId`, `submitterId`, `q`, `limit`, `offset`. + +**`approverId` is a filter, not authorization.** What you may see is decided +separately: a request is visible to its **participants** — the submitter, a +current approver, and anyone who has already acted on it (a past approver whose +slot has moved on, a commenter). So omitting `approverId` returns *your* +requests, not every request in the tenant. Admins with override authority +(`admin_full_access`, or `organization_admin` within their org) see all of them +— that is what the "all requests" view is for. + +The same rule governs a decision's files: an attachment is exactly as readable +as the decision it hangs off, never more (`sys_approval_action` delegates that +question back to this service via `fileAccessDelegate`). + + **Opening a request notifies nobody.** There is no built-in "you have an approval waiting" message today — an approver only discovers it by looking at