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