diff --git a/.changeset/approval-actor-is-the-authenticated-caller.md b/.changeset/approval-actor-is-the-authenticated-caller.md new file mode 100644 index 0000000000..d1b087c129 --- /dev/null +++ b/.changeset/approval-actor-is-the-authenticated-caller.md @@ -0,0 +1,61 @@ +--- +"@objectstack/plugin-approvals": minor +"@objectstack/rest": patch +--- + +fix(approvals): an approval action is recorded against the authenticated caller, never a body field (#3800) + +Every mutating approvals entrypoint takes an `actorId`, and the REST routes +filled it from `body.actorId ?? body.actor_id ?? context.userId` — so the body +won. The service then authorized *that value*: `pending_approvers.includes( +input.actorId)` for a decision, `submitter_id === actorId` for a recall. It never +checked that the value named the caller. + +So any authenticated user could POST `{"actorId": ""}` and have +that person's approval recorded, the request finalized, and the owning flow run +resumed down the `approve` edge — or name a request's submitter and recall it. +With `api.requireAuth` unset the anonymous-deny never fires either, so an +unauthenticated request could do the same. + +#3783 drew this line for the *data-write* identity and called the audit-row half +"tolerable". It was not: the same unchecked string was the authorization key, so +naming someone else was not a mislabelled audit row, it was how you got through +the door. + +The actor is now resolved server-side (`ApprovalService.resolveActor`) on all +nine entrypoints — `decide` / `decideNode`, `recall`, `sendBack`, `resubmit`, +`reassign`, `remind`, `requestInfo`, `comment`. + +**The rule is not "`actorId` must equal `context.userId`."** A slot can +legitimately be keyed by something else: the approver resolver stores the +`type:value` literal when a graph lookup finds no holders, and the Console picks +from the caller's own identity list — user id, email, or `role:`. The rule is +**"the actor must be an identity the server can prove belongs to the caller"**: + +- A **system** context keeps its explicit actor. The SLA sweep's reserved + `system:sla` sentinel and the ADR-0043 action link — whose single-use hashed + token binds exactly one approver — are unchanged. They are the only callers + holding a trustworthy actor with no session behind them. +- A caller with **no identity at all** is now refused. This is the anonymous case + above. +- **No `actorId`, or one naming the caller**, resolves to the caller. This is the + common path and what the Console already sends. +- **Any other value** is accepted only when the server can prove the caller holds + it — `position:

` / `role:

` against the positions on the resolved authz + context, or the caller's own email (one lazy `sys_user` read, taken only when + nothing cheaper matched). Otherwise `FORBIDDEN`. + +REST still forwards the body value; it is now a *hint* the service validates, +which is what keeps the email and `type:value` slot cases working. + +**Upgrade note.** A client that deliberately sent another user's `actorId` now +gets `403 FORBIDDEN` instead of silently succeeding. Send the action as the +acting user's own session — the field can be omitted entirely, and the caller is +used. Server-to-server callers that legitimately act for someone else should +present a system context, as the SLA sweep and the action link already do. + +This also makes two existing claims true that were previously aspirational: the +approval object's declared actions say "`actorId` defaults to the caller +server-side… the service remains the authority on who may act", and +`attachViewers` documents `can_act` as mirroring "the exact authorization the +decision methods enforce". diff --git a/packages/plugins/plugin-approvals/src/approval-actor-impersonation.test.ts b/packages/plugins/plugin-approvals/src/approval-actor-impersonation.test.ts new file mode 100644 index 0000000000..0406a937df --- /dev/null +++ b/packages/plugins/plugin-approvals/src/approval-actor-impersonation.test.ts @@ -0,0 +1,330 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The acting identity on an approval is the AUTHENTICATED CALLER, never a + * request-body field. + * + * Every mutating entrypoint on the service takes an `actorId` and — before this + * suite — authorized *that value* rather than the caller behind it. The REST + * routes fill it from `body.actorId ?? body.actor_id ?? context.userId`, so the + * body won. An authenticated user could therefore name any pending approver and + * have that approver's decision recorded, finalized, and the owning flow resumed + * — or name a request's submitter and recall it. + * + * #3783 drew exactly this line for the *data-write* identity (see the + * `actingUserId` docblock in the service) and left the authorization side + * body-driven, calling a mislabelled audit row "tolerable". It is not merely a + * label: `pending_approvers.includes(input.actorId)` is the authorization gate + * itself, so naming someone else does not just misattribute the row — it is how + * you get through the door. + * + * The tests below are all "mallory is logged in, names someone else". Each one + * must be FORBIDDEN. The final block is the load-bearing negative: the two + * legitimate callers that supply an actor with no session behind them — the SLA + * sweep (a reserved sentinel) and the ADR-0043 action link (a single-use token + * cryptographically bound to one approver) — must keep working, or the fix has + * simply broken the feature instead of securing it. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { ApprovalService, SLA_ACTOR_ID } from './approval-service.js'; + +interface FakeRow { [k: string]: any } + +/** Equality/`$in`/`$ne`/`$contains` WHERE matcher — mirrors approval-service.test.ts. */ +function makeFakeEngine() { + const tables: Record = {}; + const ensure = (n: string) => (tables[n] ??= []); + + function matches(row: FakeRow, filter: any): boolean { + if (!filter || typeof filter !== 'object') return true; + for (const [k, v] of Object.entries(filter)) { + if (k === '$or') { + if (!(v as any[]).some(sub => matches(row, sub))) return false; + continue; + } + const rv = row[k]; + if (v != null && typeof v === 'object' && '$in' in (v as any)) { + if (!(v as any).$in.includes(rv)) return false; + continue; + } + if (v != null && typeof v === 'object' && '$ne' in (v as any)) { + if (rv === (v as any).$ne) return false; + continue; + } + if (v != null && typeof v === 'object' && '$contains' in (v as any)) { + if (!String(rv ?? '').includes(String((v as any).$contains))) return false; + continue; + } + if (rv !== v) return false; + } + return true; + } + + return { + _tables: tables, + async find(object: string, options?: any) { + const rows = ensure(object).filter(r => matches(r, options?.filter ?? options?.where)); + if (options?.orderBy?.[0]) { + const { field, order } = options.orderBy[0]; + rows.sort((a, b) => { + const av = a[field]; const bv = b[field]; + if (av === bv) return 0; + const cmp = av > bv ? 1 : -1; + return order === 'desc' ? -cmp : cmp; + }); + } + const start = options?.offset ?? 0; + return rows.slice(start, start + (options?.limit ?? 1000)); + }, + async insert(object: string, data: any) { + ensure(object).push({ ...data }); + return { ...data }; + }, + async update(object: string, idOrData: any, _opts?: any) { + const data = typeof idOrData === 'object' ? idOrData : _opts; + const id = typeof idOrData === 'object' ? idOrData.id : idOrData; + const table = ensure(object); + const i = table.findIndex(r => r.id === id); + if (i >= 0) table[i] = { ...table[i], ...data }; + return table[i]; + }, + async delete(object: string, options?: any) { + const table = ensure(object); + const id = options?.where?.id ?? options?.id; + const i = table.findIndex(r => r.id === id); + if (i >= 0) table.splice(i, 1); + return { id }; + }, + registerHook() {}, + unregisterHooksByPackage() { return 0; }, + }; +} + +/** The submitter, and the approver whose slot is up for grabs. */ +const SUBMITTER = 'alice'; +const APPROVER = 'bob'; +/** Authenticated, ordinary, and on neither side of the request. */ +const MALLORY = { userId: 'mallory', tenantId: 't1', positions: [], permissions: [] } as any; +const ALICE = { userId: SUBMITTER, tenantId: 't1', positions: [], permissions: [] } as any; +const SYS = { isSystem: true, positions: [], permissions: [] } as any; + +function nodeConfig(approvers: string[], extra: Record = {}) { + return { + approvers: approvers.map(v => ({ type: 'user' as const, value: v })), + behavior: 'first_response' as const, + ...extra, + }; +} + +function openInput(approvers: string[], extra: Record = {}, configExtra: Record = {}) { + return { + object: 'opportunity', + recordId: 'opp1', + runId: 'run_1', + nodeId: 'approve_step', + flowName: 'deal_approval', + config: nodeConfig(approvers, configExtra), + record: { id: 'opp1', amount: 100 }, + ...extra, + }; +} + +/** + * Enough automation surface for the send-back path's ADR-0044 guards to pass, + * so a `sendBack` / `resubmit` test fails on the ACTOR check or not at all — + * never on a missing `revise` out-edge. + */ +function makeAutomationStub() { + const resumed: any[] = []; + const cancelled: string[] = []; + return { + resumed, + cancelled, + async getFlow() { + return { + name: 'deal_approval', + nodes: [{ id: 'approve_step', type: 'approval' }, { id: 'wait_revision', type: 'wait' }], + edges: [ + { id: 'e1', source: 'approve_step', target: 'ok', label: 'approve' }, + { id: 'e2', source: 'approve_step', target: 'no', label: 'reject' }, + { id: 'e3', source: 'approve_step', target: 'wait_revision', label: 'revise' }, + { id: 'e4', source: 'wait_revision', target: 'approve_step', label: 'resubmit', type: 'back' }, + ], + }; + }, + async resume(runId: string, signal: any) { resumed.push({ runId, signal }); }, + async cancelRun(runId: string) { cancelled.push(runId); }, + }; +} + +describe('approvals: the actor is the authenticated caller, not a body field', () => { + let engine: ReturnType; + let svc: ApprovalService; + let n = 0; + const baseTime = new Date('2026-01-15T10:00:00Z').getTime(); + + beforeEach(() => { + engine = makeFakeEngine(); + n = 0; + svc = new ApprovalService({ + engine: engine as any, + clock: { now: () => new Date(baseTime + (n++) * 1000) }, + }); + }); + + /** Open a request submitted by alice and pending on bob. */ + const open = (approvers = [APPROVER], configExtra: Record = {}) => + svc.openNodeRequest(openInput(approvers, {}, configExtra), ALICE); + + // ── the decision itself ───────────────────────────────────────── + + it('decideNode: mallory cannot approve by naming the pending approver', async () => { + const req = await open(); + await expect( + svc.decideNode(req.id, { decision: 'approve', actorId: APPROVER }, MALLORY), + ).rejects.toThrow(/FORBIDDEN/); + }); + + it('decideNode: a refused impersonation writes no audit row and leaves the request pending', async () => { + const req = await open(); + await expect( + svc.decideNode(req.id, { decision: 'approve', actorId: APPROVER }, MALLORY), + ).rejects.toThrow(/FORBIDDEN/); + + const decisions = (engine._tables['sys_approval_action'] ?? []) + .filter((a: any) => a.action === 'approve' || a.action === 'reject'); + expect(decisions).toHaveLength(0); + expect(engine._tables['sys_approval_request'][0].status).toBe('pending'); + }); + + it('decideNode: mallory cannot reject by naming the pending approver', async () => { + const req = await open(); + await expect( + svc.decideNode(req.id, { decision: 'reject', actorId: APPROVER, comment: 'no' }, MALLORY), + ).rejects.toThrow(/FORBIDDEN/); + }); + + it('decide: the flow is not resumed by an impersonated decision', async () => { + const resumed: any[] = []; + svc.attachAutomation({ async resume(runId: string, signal: any) { resumed.push({ runId, signal }); } } as any); + const req = await open(); + await expect( + svc.decide(req.id, { decision: 'approve', actorId: APPROVER }, MALLORY), + ).rejects.toThrow(/FORBIDDEN/); + expect(resumed).toHaveLength(0); + }); + + it('decideNode: a unanimous slate cannot be filled by one user naming the others', async () => { + const req = await open(['bob', 'carol'], { behavior: 'unanimous' }); + const asBob = { userId: 'bob', tenantId: 't1', positions: [], permissions: [] } as any; + const first = await svc.decideNode(req.id, { decision: 'approve', actorId: 'bob' }, asBob); + expect(first.finalized).toBe(false); + // Bob holds a slot, so he clears the "is a pending approver" gate — but the + // slot he clears it with is his own, not carol's. + await expect( + svc.decideNode(req.id, { decision: 'approve', actorId: 'carol' }, asBob), + ).rejects.toThrow(/FORBIDDEN/); + expect(engine._tables['sys_approval_request'][0].status).toBe('pending'); + }); + + // ── the submitter-only moves ──────────────────────────────────── + + it('recall: mallory cannot withdraw the request by naming its submitter', async () => { + const req = await open(); + await expect( + svc.recall(req.id, { actorId: SUBMITTER, comment: 'gone' }, MALLORY), + ).rejects.toThrow(/FORBIDDEN/); + expect(engine._tables['sys_approval_request'][0].status).toBe('pending'); + }); + + it('resubmit: mallory cannot resubmit a returned request by naming its submitter', async () => { + svc.attachAutomation(makeAutomationStub() as any); + const req = await open(); + const asBob = { userId: APPROVER, tenantId: 't1', positions: [], permissions: [] } as any; + await svc.sendBack(req.id, { actorId: APPROVER, comment: 'fix it' }, asBob); + expect(engine._tables['sys_approval_request'][0].status).toBe('returned'); + await expect( + svc.resubmit(req.id, { actorId: SUBMITTER, comment: 'done' }, MALLORY), + ).rejects.toThrow(/FORBIDDEN/); + }); + + it('sendBack: mallory cannot return the request by naming the pending approver', async () => { + const req = await open(); + await expect( + svc.sendBack(req.id, { actorId: APPROVER, comment: 'revise' }, MALLORY), + ).rejects.toThrow(/FORBIDDEN/); + expect(engine._tables['sys_approval_request'][0].status).toBe('pending'); + }); + + // ── the thread moves ──────────────────────────────────────────── + + it('reassign: mallory cannot move the slot by naming its holder', async () => { + const req = await open(); + await expect( + svc.reassign(req.id, { actorId: APPROVER, to: 'mallory' }, MALLORY), + ).rejects.toThrow(/FORBIDDEN/); + expect(engine._tables['sys_approval_request'][0].pending_approvers).toBe(APPROVER); + }); + + it('requestInfo: mallory cannot post as the pending approver', async () => { + const req = await open(); + await expect( + svc.requestInfo(req.id, { actorId: APPROVER, comment: 'send the contract' }, MALLORY), + ).rejects.toThrow(/FORBIDDEN/); + }); + + it('comment: mallory cannot post to the thread as the submitter', async () => { + const req = await open(); + await expect( + svc.comment(req.id, { actorId: SUBMITTER, comment: 'looks fine to me' }, MALLORY), + ).rejects.toThrow(/FORBIDDEN/); + }); + + it('remind: mallory cannot nudge as the submitter', async () => { + const req = await open(); + await expect( + svc.remind(req.id, { actorId: SUBMITTER }, MALLORY), + ).rejects.toThrow(/FORBIDDEN/); + }); + + // ── the legitimate paths must survive ─────────────────────────── + // + // Without these, "reject every actorId that isn't the caller" would pass the + // suite above by breaking the SLA sweep and the emailed action link — the two + // callers that hold a trustworthy actor with no session behind it. + + it('the real approver still decides their own slot', async () => { + const req = await open(); + const asBob = { userId: APPROVER, tenantId: 't1', positions: [], permissions: [] } as any; + const out = await svc.decideNode(req.id, { decision: 'approve', actorId: APPROVER }, asBob); + expect(out.finalized).toBe(true); + expect(out.request.status).toBe('approved'); + const row = (engine._tables['sys_approval_action'] ?? []).find((a: any) => a.action === 'approve'); + expect(row.actor_id).toBe(APPROVER); + }); + + it('the real submitter still recalls their own request', async () => { + const req = await open(); + const out = await svc.recall(req.id, { actorId: SUBMITTER }, ALICE); + expect(out.request.status).toBe('recalled'); + }); + + it('a system context may still name an actor with no session behind it (SLA sweep)', async () => { + const req = await open(); + const out = await svc.decideNode(req.id, { decision: 'approve', actorId: SLA_ACTOR_ID }, SYS); + expect(out.finalized).toBe(true); + const row = (engine._tables['sys_approval_action'] ?? []).find((a: any) => a.action === 'approve'); + expect(row.actor_id).toBe(SLA_ACTOR_ID); + }); + + it('a privileged admin may still override a stuck request', async () => { + const req = await open(['position:cfo']); + const admin = { + userId: 'root', tenantId: 't1', positions: [], permissions: ['admin_full_access'], + } as any; + const out = await svc.decideNode(req.id, { decision: 'approve', actorId: 'root' }, admin); + expect(out.finalized).toBe(true); + expect(out.request.status).toBe('approved'); + }); +}); diff --git a/packages/plugins/plugin-approvals/src/approval-revise.test.ts b/packages/plugins/plugin-approvals/src/approval-revise.test.ts index 6c56b9ea83..8e0feaa02a 100644 --- a/packages/plugins/plugin-approvals/src/approval-revise.test.ts +++ b/packages/plugins/plugin-approvals/src/approval-revise.test.ts @@ -20,7 +20,14 @@ import { registerApprovalNode } from './approval-node.js'; import { bindApprovalLockHook, APPROVALS_HOOK_PACKAGE } from './lifecycle-hooks.js'; const SYSTEM_CTX = { isSystem: true, positions: [], permissions: [] } as any; -const USER_CTX = { isSystem: false, positions: [], permissions: [] } as any; + +/** + * The signed-in caller. An approval action is recorded against the + * AUTHENTICATED caller (#3800), so each call below presents the context of the + * person it names — an identity-less context can no longer act by naming one. + */ +const asUser = (userId: string) => + ({ isSystem: false, userId, positions: [], permissions: [] }) as any; const noopLogger = { info() {}, warn() {}, error() {}, debug() {} }; @@ -156,7 +163,7 @@ describe('Send back for revision (ADR-0044)', () => { registerReviseFlow(); const { runId, req } = await startFlow(); - const sent = await service.sendBack(req.id, { actorId: 'u1', comment: 'fix the totals' }, USER_CTX); + const sent = await service.sendBack(req.id, { actorId: 'u1', comment: 'fix the totals' }, asUser('u1')); expect(sent.resumed).toBe(true); expect(sent.autoRejected).toBeUndefined(); expect(sent.request.status).toBe('returned'); @@ -166,7 +173,7 @@ describe('Send back for revision (ADR-0044)', () => { expect(automation.listSuspendedRuns()).toMatchObject([{ runId, nodeId: 'wait_revision' }]); expect(await actionsOf(req.id)).toEqual(['submit', 'revise']); - const re = await service.resubmit(req.id, { actorId: 'submitter', comment: 'totals fixed' }, USER_CTX); + const re = await service.resubmit(req.id, { actorId: 'submitter', comment: 'totals fixed' }, asUser('submitter')); expect(re.resumed).toBe(true); expect(await actionsOf(req.id)).toEqual(['submit', 'revise', 'resubmit']); @@ -189,13 +196,13 @@ describe('Send back for revision (ADR-0044)', () => { const { req } = await startFlow(); expect((await service.getRequest(req.id, SYSTEM_CTX))?.round).toBeUndefined(); // round 1 - await service.sendBack(req.id, { actorId: 'u1' }, USER_CTX); - await service.resubmit(req.id, { actorId: 'submitter' }, USER_CTX); + await service.sendBack(req.id, { actorId: 'u1' }, asUser('u1')); + await service.resubmit(req.id, { actorId: 'submitter' }, asUser('submitter')); const round2 = await pendingReq(); expect((await service.getRequest(round2.id, SYSTEM_CTX))?.round).toBe(2); - await service.sendBack(round2.id, { actorId: 'u1' }, USER_CTX); - await service.resubmit(round2.id, { actorId: 'submitter' }, USER_CTX); + await service.sendBack(round2.id, { actorId: 'u1' }, asUser('u1')); + await service.resubmit(round2.id, { actorId: 'submitter' }, asUser('submitter')); const round3 = await pendingReq(); expect((await service.getRequest(round3.id, SYSTEM_CTX))?.round).toBe(3); }); @@ -205,12 +212,12 @@ describe('Send back for revision (ADR-0044)', () => { const { req } = await startFlow(); // Send-back #1 fits the budget. - await service.sendBack(req.id, { actorId: 'u1' }, USER_CTX); - await service.resubmit(req.id, { actorId: 'submitter' }, USER_CTX); + await service.sendBack(req.id, { actorId: 'u1' }, asUser('u1')); + await service.resubmit(req.id, { actorId: 'submitter' }, asUser('submitter')); const round2 = await pendingReq(); // Send-back #2 exceeds it → auto-reject, flow takes the reject branch. - const out = await service.sendBack(round2.id, { actorId: 'u1', comment: 'still wrong' }, USER_CTX); + const out = await service.sendBack(round2.id, { actorId: 'u1', comment: 'still wrong' }, asUser('u1')); expect(out.autoRejected).toBe(true); expect(out.resumed).toBe(true); expect(out.request.status).toBe('rejected'); @@ -224,7 +231,7 @@ describe('Send back for revision (ADR-0044)', () => { it('maxRevisions 0 disables send-back (immediate auto-reject)', async () => { registerReviseFlow({ maxRevisions: 0 }); const { req } = await startFlow(); - const out = await service.sendBack(req.id, { actorId: 'u1' }, USER_CTX); + const out = await service.sendBack(req.id, { actorId: 'u1' }, asUser('u1')); expect(out.autoRejected).toBe(true); expect(marks).toEqual(['on_rejected']); }); @@ -246,7 +253,7 @@ describe('Send back for revision (ADR-0044)', () => { await automation.execute('no_revise', { object: 'fin_expense', record: { id: 'x2' }, userId: 'submitter' }); const req = await pendingReq(); - await expect(service.sendBack(req.id, { actorId: 'u1' }, USER_CTX)).rejects.toThrow(/no 'revise' out-edge/); + await expect(service.sendBack(req.id, { actorId: 'u1' }, asUser('u1'))).rejects.toThrow(/no 'revise' out-edge/); // Nothing moved: still pending, no revise audit row. expect((await fake.find('sys_approval_request', { where: { id: req.id } }))[0].status).toBe('pending'); expect(await actionsOf(req.id)).toEqual(['submit']); @@ -264,10 +271,10 @@ describe('Send back for revision (ADR-0044)', () => { expect(first.finalized).toBe(false); // u2 sends back instead: finalizes despite u1's earlier approval. - const sent = await service.sendBack(req.id, { actorId: 'u2', comment: 'rework' }, USER_CTX); + const sent = await service.sendBack(req.id, { actorId: 'u2', comment: 'rework' }, asUser('u2')); expect(sent.request.status).toBe('returned'); - await service.resubmit(req.id, { actorId: 'submitter' }, USER_CTX); + await service.resubmit(req.id, { actorId: 'submitter' }, asUser('submitter')); const round2 = await pendingReq(); // Fresh slate: BOTH approvers pending again — prior approvals are stale. expect((round2.pending_approvers as string).split(',').sort()).toEqual(['u1', 'u2']); @@ -292,21 +299,21 @@ describe('Send back for revision (ADR-0044)', () => { }); await expect(editAttempt()).rejects.toThrow(/RECORD_LOCKED/); // pending → locked - await service.sendBack(req.id, { actorId: 'u1' }, USER_CTX); + await service.sendBack(req.id, { actorId: 'u1' }, asUser('u1')); await expect(editAttempt()).resolves.toBeUndefined(); // returned → unlocked - await service.resubmit(req.id, { actorId: 'submitter' }, USER_CTX); + await service.resubmit(req.id, { actorId: 'submitter' }, asUser('submitter')); await expect(editAttempt()).rejects.toThrow(/RECORD_LOCKED/); // round 2 pending → re-locked }); it('recall crossing the revise window cancels the run (returned → recalled)', async () => { registerReviseFlow(); const { runId, req } = await startFlow(); - await service.sendBack(req.id, { actorId: 'u1' }, USER_CTX); + await service.sendBack(req.id, { actorId: 'u1' }, asUser('u1')); // Only the submitter may abandon the revision. - await expect(service.recall(req.id, { actorId: 'u1' }, USER_CTX)).rejects.toThrow(/FORBIDDEN/); + await expect(service.recall(req.id, { actorId: 'u1' }, asUser('u1'))).rejects.toThrow(/FORBIDDEN/); - const out = await service.recall(req.id, { actorId: 'submitter' }, USER_CTX); + const out = await service.recall(req.id, { actorId: 'submitter' }, asUser('submitter')); expect(out.request.status).toBe('recalled'); expect(out.resumed).toBe(false); // The run was terminally cancelled, not resumed down any branch. @@ -317,13 +324,13 @@ describe('Send back for revision (ADR-0044)', () => { expect(log.id).toBe(runId); // The window is closed: resubmit is no longer possible. - await expect(service.resubmit(req.id, { actorId: 'submitter' }, USER_CTX)).rejects.toThrow(/INVALID_STATE/); + await expect(service.resubmit(req.id, { actorId: 'submitter' }, asUser('submitter'))).rejects.toThrow(/INVALID_STATE/); }); it('refuses resubmit while another pending request collides on the record (run stays resumable)', async () => { registerReviseFlow(); const { runId, req } = await startFlow(); - await service.sendBack(req.id, { actorId: 'u1' }, USER_CTX); + await service.sendBack(req.id, { actorId: 'u1' }, asUser('u1')); // Simulate a record-change trigger re-firing off an edit made inside the // revise window: a second, unrelated run opened its own pending request. @@ -334,38 +341,38 @@ describe('Send back for revision (ADR-0044)', () => { created_at: new Date().toISOString(), }); - await expect(service.resubmit(req.id, { actorId: 'submitter' }, USER_CTX)).rejects.toThrow(/DUPLICATE_REQUEST/); + await expect(service.resubmit(req.id, { actorId: 'submitter' }, asUser('submitter'))).rejects.toThrow(/DUPLICATE_REQUEST/); // The refusal happened BEFORE the suspension was consumed — clearing the // collision makes the same resubmit succeed. expect(automation.listSuspendedRuns().some(r => r.runId === runId)).toBe(true); await fake.delete('sys_approval_request', { where: { id: 'areq_collider' } }); - const re = await service.resubmit(req.id, { actorId: 'submitter' }, USER_CTX); + const re = await service.resubmit(req.id, { actorId: 'submitter' }, asUser('submitter')); expect(re.resumed).toBe(true); }); it('a superseded returned request can neither resubmit again nor be recalled', async () => { registerReviseFlow(); const { req } = await startFlow(); - await service.sendBack(req.id, { actorId: 'u1' }, USER_CTX); - await service.resubmit(req.id, { actorId: 'submitter' }, USER_CTX); + await service.sendBack(req.id, { actorId: 'u1' }, asUser('u1')); + await service.resubmit(req.id, { actorId: 'submitter' }, asUser('submitter')); // Round 2 is the live frontier; the round-1 row is history. - await expect(service.resubmit(req.id, { actorId: 'submitter' }, USER_CTX)).rejects.toThrow(/supersedes/); - await expect(service.recall(req.id, { actorId: 'submitter' }, USER_CTX)).rejects.toThrow(/supersedes/); + await expect(service.resubmit(req.id, { actorId: 'submitter' }, asUser('submitter'))).rejects.toThrow(/supersedes/); + await expect(service.recall(req.id, { actorId: 'submitter' }, asUser('submitter'))).rejects.toThrow(/supersedes/); }); it('enforces the actor matrix: only pending approvers send back, only the submitter resubmits', async () => { registerReviseFlow(); const { req } = await startFlow(); - await expect(service.sendBack(req.id, { actorId: 'intruder' }, USER_CTX)).rejects.toThrow(/FORBIDDEN/); - await expect(service.sendBack(req.id, { actorId: 'submitter' }, USER_CTX)).rejects.toThrow(/FORBIDDEN/); + await expect(service.sendBack(req.id, { actorId: 'intruder' }, asUser('intruder'))).rejects.toThrow(/FORBIDDEN/); + await expect(service.sendBack(req.id, { actorId: 'submitter' }, asUser('submitter'))).rejects.toThrow(/FORBIDDEN/); - await service.sendBack(req.id, { actorId: 'u1' }, USER_CTX); - await expect(service.resubmit(req.id, { actorId: 'u1' }, USER_CTX)).rejects.toThrow(/FORBIDDEN/); + await service.sendBack(req.id, { actorId: 'u1' }, asUser('u1')); + await expect(service.resubmit(req.id, { actorId: 'u1' }, asUser('u1'))).rejects.toThrow(/FORBIDDEN/); // Resubmit only applies to returned requests — a pending one rejects. const fresh = await startFlowSecondRecord(); - await expect(service.resubmit(fresh.id, { actorId: 'submitter' }, USER_CTX)).rejects.toThrow(/INVALID_STATE/); + await expect(service.resubmit(fresh.id, { actorId: 'submitter' }, asUser('submitter'))).rejects.toThrow(/INVALID_STATE/); }); /** A second record's pending request, to probe resubmit-on-pending. */ @@ -403,9 +410,9 @@ describe('Send back for revision (ADR-0044)', () => { const mirror = async () => (await fake.find('fin_expense', { where: { id: 'm1' } }))[0].approval_status; expect(await mirror()).toBe('pending'); - await service.sendBack(req.id, { actorId: 'u1' }, USER_CTX); + await service.sendBack(req.id, { actorId: 'u1' }, asUser('u1')); expect(await mirror()).toBe('returned'); - await service.resubmit(req.id, { actorId: 'submitter' }, USER_CTX); + await service.resubmit(req.id, { actorId: 'submitter' }, asUser('submitter')); expect(await mirror()).toBe('pending'); // round 2 re-mirrors }); }); diff --git a/packages/plugins/plugin-approvals/src/approval-service.test.ts b/packages/plugins/plugin-approvals/src/approval-service.test.ts index 6c2c7a0cb3..2bb4a55eb6 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.test.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.test.ts @@ -118,6 +118,15 @@ function makeFakeEngine() { const CTX = { userId: 'u1', tenantId: 't1', positions: [], permissions: [] } as any; const SYS = { isSystem: true, positions: [], permissions: [] } as any; +/** + * The signed-in caller, when it is someone other than {@link CTX}'s `u1`. + * An approval action is recorded against the AUTHENTICATED caller (#3800), so a + * test that acts as `u9` has to present `u9`'s context — naming them in + * `actorId` while calling as `u1` is the impersonation the service now refuses. + */ +const asUser = (userId: string) => + ({ userId, tenantId: 't1', positions: [], permissions: [] }) as any; + function nodeConfig(approvers: string[], extra: Record = {}) { return { approvers: approvers.map(v => ({ type: 'user' as const, value: v })), @@ -908,7 +917,7 @@ describe('ApprovalService (node era)', () => { it('reassign: hands the slot to a new approver and audits the move', async () => { const req = await svc.openNodeRequest(openInput(['u9', 'u2']), CTX); - const out = await svc.reassign(req.id, { actorId: 'u9', to: 'u7' }, CTX); + const out = await svc.reassign(req.id, { actorId: 'u9', to: 'u7' }, asUser('u9')); expect(out.request.pending_approvers).toEqual(['u7', 'u2']); const actions = await svc.listActions(req.id, SYS); expect(actions.at(-1)).toMatchObject({ action: 'reassign', actor_id: 'u9', comment: 'u9 → u7' }); @@ -918,15 +927,15 @@ describe('ApprovalService (node era)', () => { const emitted: any[] = []; svc.attachMessaging({ async emit(input) { emitted.push(input); } }); const req = await svc.openNodeRequest(openInput(['u9']), CTX); - await svc.reassign(req.id, { actorId: 'u9', to: 'u7' }, CTX); + await svc.reassign(req.id, { actorId: 'u9', to: 'u7' }, asUser('u9')); expect(emitted).toHaveLength(1); expect(emitted[0]).toMatchObject({ topic: 'approval.reassigned', audience: ['u7'] }); }); it('reassign: blocks a non-holder and duplicate targets', async () => { const req = await svc.openNodeRequest(openInput(['u9', 'u2']), CTX); - await expect(svc.reassign(req.id, { actorId: 'intruder', to: 'u7' }, CTX)).rejects.toThrow(/FORBIDDEN/); - await expect(svc.reassign(req.id, { actorId: 'u9', to: 'u2' }, CTX)).rejects.toThrow(/VALIDATION_FAILED/); + await expect(svc.reassign(req.id, { actorId: 'intruder', to: 'u7' }, asUser('intruder'))).rejects.toThrow(/FORBIDDEN/); + await expect(svc.reassign(req.id, { actorId: 'u9', to: 'u2' }, asUser('u9'))).rejects.toThrow(/VALIDATION_FAILED/); }); it('remind: notifies pending approvers, audits, and throttles repeats', async () => { @@ -975,7 +984,7 @@ describe('ApprovalService (node era)', () => { const emitted: any[] = []; svc.attachMessaging({ async emit(input) { emitted.push(input); } }); const req = await svc.openNodeRequest(openInput(['u9']), CTX); - const out = await svc.requestInfo(req.id, { actorId: 'u9', comment: 'Need the Q3 numbers' }, CTX); + const out = await svc.requestInfo(req.id, { actorId: 'u9', comment: 'Need the Q3 numbers' }, asUser('u9')); expect(out.request.status).toBe('pending'); expect(out.request.pending_approvers).toEqual(['u9']); expect(emitted[0]).toMatchObject({ topic: 'approval.request_info', audience: ['u1'] }); @@ -986,8 +995,8 @@ describe('ApprovalService (node era)', () => { it('comment: submitter and approver may reply; outsiders may not', async () => { const req = await svc.openNodeRequest(openInput(['u9']), CTX); await svc.comment(req.id, { actorId: 'u1', comment: 'Numbers attached.' }, CTX); - await svc.comment(req.id, { actorId: 'u9', comment: 'Thanks, reviewing.' }, CTX); - await expect(svc.comment(req.id, { actorId: 'outsider', comment: 'hi' }, { positions: [], permissions: [] } as any)) + await svc.comment(req.id, { actorId: 'u9', comment: 'Thanks, reviewing.' }, asUser('u9')); + await expect(svc.comment(req.id, { actorId: 'outsider', comment: 'hi' }, asUser('outsider'))) .rejects.toThrow(/FORBIDDEN/); const actions = await svc.listActions(req.id, SYS); expect(actions.filter(a => a.action === 'comment')).toHaveLength(2); @@ -1039,7 +1048,7 @@ describe('ApprovalService (node era)', () => { expect(await svc.redeemActionToken(short.approve)).toMatchObject({ ok: false, reason: 'expired' }); const live = await svc.issueActionTokens(req.id, 'u9'); - await svc.reassign(req.id, { actorId: 'u9', to: 'u7' }, CTX); + await svc.reassign(req.id, { actorId: 'u9', to: 'u7' }, asUser('u9')); expect(await svc.redeemActionToken(live.approve)).toMatchObject({ ok: false, reason: 'not_approver' }); const forU7 = await svc.issueActionTokens(req.id, 'u7'); @@ -2405,19 +2414,29 @@ describe('ApprovalService — participant visibility (#3590)', () => { 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 () => { + it('a write path still echoes back its own result to the user who made it', 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, - ); + // Approving CLEARS `pending_approvers`, so the approver stops being a + // participant the instant their own write lands. The operation authorized + // itself; re-gating the echo would turn a successful write into a null + // result for the very person who made it. + const res = await svc.decideNode(req.id, { decision: 'approve', actorId: 'u9' }, asUser('u9')); + expect(res.request).not.toBeNull(); + expect(res.request.status).toBe('approved'); + }); + + it('a service-to-service write echoes back too (no session at all)', async () => { + const engine = makeFakeEngine(); + const svc = svcFor(engine); + const req = await svc.openNodeRequest(openInput(['u9']), CTX); + + // Flow-driven resumes and the SLA sweep carry no user. Since #3800 that is + // expressible only as a SYSTEM context — a user-less non-system caller can + // no longer act by naming an approver. + const res = await svc.decideNode(req.id, { decision: 'approve', actorId: 'u9' }, SYS); expect(res.request).not.toBeNull(); expect(res.request.status).toBe('approved'); }); @@ -2513,7 +2532,7 @@ describe('in-band transitions finalise before they resume (#3456 invariant)', () it('sendBack finalises before resuming', async () => { const req = await open(); - await svc.sendBack(req.id, { actorId: 'u9', comment: 'fix the totals' }, CTX); + await svc.sendBack(req.id, { actorId: 'u9', comment: 'fix the totals' }, asUser('u9')); expectCleanHandoffs(); }); @@ -2521,14 +2540,14 @@ describe('in-band transitions finalise before they resume (#3456 invariant)', () // `maxRevisions: 0` takes the ADR-0044 loop-guard branch on the first // send-back — a separate resume site from the normal path above. const req = await open({ maxRevisions: 0 }); - const out = await svc.sendBack(req.id, { actorId: 'u9' }, CTX); + const out = await svc.sendBack(req.id, { actorId: 'u9' }, asUser('u9')); expect(out.autoRejected, 'expected the auto-reject branch').toBe(true); expectCleanHandoffs(); }); it('recall inside the revise window cancels the run without a pending request', async () => { const req = await open(); - await svc.sendBack(req.id, { actorId: 'u9' }, CTX); + await svc.sendBack(req.id, { actorId: 'u9' }, asUser('u9')); handoffs = []; // isolate the recall's own hand-back await svc.recall(req.id, { actorId: 'u1' }, CTX); expect(handoffs.map(h => h.hook)).toContain('cancelRun'); @@ -2537,7 +2556,7 @@ describe('in-band transitions finalise before they resume (#3456 invariant)', () it('resubmit re-enters the node without leaving the old request pending', async () => { const req = await open(); - await svc.sendBack(req.id, { actorId: 'u9' }, CTX); + await svc.sendBack(req.id, { actorId: 'u9' }, asUser('u9')); handoffs = []; // isolate the resubmit's own hand-back await svc.resubmit(req.id, { actorId: 'u1' }, CTX); expectCleanHandoffs(); @@ -2633,14 +2652,19 @@ describe('status mirror identity (#3783)', () => { }); it('never takes the identity from the caller-supplied actorId', async () => { - // `actorId` arrives in the REST body (`body.actorId ?? context.userId`) and - // is only checked against the pending slate, never against the caller. It is - // fine on an audit row; making it the identity of an RLS-scoped write would - // let any authenticated caller borrow a slot holder's identity. + // `actorId` arrives in the REST body (`body.actorId ?? context.userId`). + // #3783 kept it out of the mirror identity; #3800 then stopped it from + // reaching the slate check too, so borrowing a slot holder's identity now + // fails outright rather than merely mislabelling the write. The mirror is + // the second line here, not the first — assert both, so neither can regress + // silently behind the other. const req = await open(); const someoneElse = { ...CTX, userId: 'intruder' }; - await svc.decideNode(req.id, { decision: 'approve', actorId: 'u9' }, someoneElse as any); - expect(mirrorContext()?.userId).toBe('intruder'); + await expect( + svc.decideNode(req.id, { decision: 'approve', actorId: 'u9' }, someoneElse as any), + ).rejects.toThrow(/FORBIDDEN/); + expect(engine._tables['opportunity'][0].approval_status).toBe('pending'); + expect(mirrorContext()?.userId).not.toBe('u9'); }); it('SLA auto-decision: stays user-less — no human did it', async () => { diff --git a/packages/plugins/plugin-approvals/src/approval-service.ts b/packages/plugins/plugin-approvals/src/approval-service.ts index ba489c534d..0aac090636 100644 --- a/packages/plugins/plugin-approvals/src/approval-service.ts +++ b/packages/plugins/plugin-approvals/src/approval-service.ts @@ -147,11 +147,17 @@ const SYSTEM_CTX = { isSystem: true, positions: [], permissions: [] } as const; * Who is acting, for the purpose of a data write made on their behalf (#3783). * * Reads the AUTHENTICATED principal off the execution context — deliberately not - * `input.actorId`. Every public entrypoint takes `actorId` from the request body - * (`body.actorId ?? context.userId`, see the REST approval routes) and the - * service only checks that it names a pending approver, never that it is the - * caller. Tolerable on an audit row; promoting it to the identity of an - * RLS-scoped write would turn a mislabelled audit trail into identity spoofing. + * `input.actorId`. When this was written the two could still disagree: every + * public entrypoint took `actorId` from the request body (`body.actorId ?? + * context.userId`, see the REST approval routes) and the service only checked + * that it named a pending approver, never that it was the caller. That was + * called tolerable on an audit row — but the same unchecked value was the + * authorization key, so it was in fact impersonation, and #3800 closed it: + * {@link ApprovalService.resolveActor} now pins the actor to an identity the + * server can prove belongs to the caller. This helper stays the separate, + * stricter answer for a DATA WRITE, which wants the bare human id and never a + * `type:value` slot literal or a machine sentinel. + * * A caller holding a trustworthy actor with no session behind it — the ADR-0043 * action link, whose token cryptographically binds exactly one approver — puts * that actor ON the context instead of relying on this. @@ -509,6 +515,81 @@ export class ApprovalService implements IApprovalService { return requestOrg == null || (actorTenant != null && String(requestOrg) === String(actorTenant)); } + /** + * Pin the acting identity to the AUTHENTICATED CALLER (#3800). + * + * Every public entrypoint accepts an `actorId`, and the REST routes fill it + * from `body.actorId ?? body.actor_id ?? context.userId` — so before this + * gate the body won. The authorization checks downstream all read that value + * (`pending_approvers.includes(input.actorId)`, `submitter_id === actorId`), + * which made the body-supplied string not merely the audit label but the key + * that opens the door: any authenticated user could name a pending approver + * and have that approver's decision recorded and the owning flow resumed. + * #3783 drew this line for the data-write identity ({@link actingUserId}); + * this closes the authorization half. + * + * A caller may still name an identity OTHER than their bare user id, because + * a slot legitimately can be keyed by one: `resolveApproverSpec` stores the + * `type:value` literal when a graph lookup yields nothing, and an author may + * write an email as a `user` approver. So the rule is not "actorId must equal + * userId" — it is **"actorId must be an identity the SERVER can prove belongs + * to the caller"**. Anything else is `FORBIDDEN`. + * + * A system context is exempt and keeps its explicit actor: the SLA sweep + * passes the reserved {@link SLA_ACTOR_ID} sentinel, and the ADR-0043 action + * link passes the approver its single-use token is cryptographically bound to + * (having also put them on the context). Those are the only two callers that + * hold a trustworthy actor with no session behind them. + * + * A caller with NO identity at all cannot act. That case is reachable: the + * REST anonymous-deny only fires when `api.requireAuth` is set, so without it + * an anonymous request previously decided approvals outright by naming one. + */ + private async resolveActor( + actorId: string | undefined, + context: SharingExecutionContext, + ): Promise { + // The machine callers — their actor is server-minted, not caller-supplied. + if (context?.isSystem) { + if (!actorId) throw new Error('VALIDATION_FAILED: actorId is required'); + return actorId; + } + const uid = actingUserId(context); + if (!uid) { + throw new Error('FORBIDDEN: an approval action requires an authenticated caller'); + } + // The common case: no actor named, or the caller named themselves. + if (!actorId || String(actorId) === uid) return uid; + + // Named something else — allow it ONLY if the server can prove the caller + // holds that identity. `positions` is resolved by the shared authz resolver + // (never client-supplied); `role:` is the ADR-0090 D3 deprecated spelling + // that 15.x-era slots and the Console's own identity list still carry. + const named = String(actorId); + for (const position of context.positions ?? []) { + if (named === `position:${position}` || named === `role:${position}`) return named; + } + // Email last — it costs a read, so only when nothing cheaper matched. + if (named.includes('@') && await this.callerHasEmail(uid, named)) return named; + + throw new Error( + `FORBIDDEN: cannot act as '${named}' — an approval action is recorded against the authenticated caller`, + ); + } + + /** Does `userId`'s own account carry `email`? (Slots keyed by email, #3800.) */ + private async callerHasEmail(userId: string, email: string): Promise { + try { + const rows = await this.engine.find('sys_user', { + where: { id: userId }, limit: 1, context: SYSTEM_CTX, + }); + const row: any = Array.isArray(rows) ? rows[0] : null; + return !!row?.email && String(row.email).toLowerCase() === email.toLowerCase(); + } catch { + return false; + } + } + /** * Expand the approvers on an Approval node into user IDs by querying the * graph tables for `team:` / `department:` / `position:` / @@ -1356,7 +1437,7 @@ export class ApprovalService implements IApprovalService { context: SharingExecutionContext, ): Promise<{ request: ApprovalRequestRow; runId: string | null; nodeId: string | null; finalized: boolean; decision: 'approve' | 'reject'; outputs?: Record }> { if (!requestId) throw new Error('VALIDATION_FAILED: requestId is required'); - if (!input?.actorId) throw new Error('VALIDATION_FAILED: actorId is required'); + const actorId = await this.resolveActor(input?.actorId, context); if (input.decision !== 'approve' && input.decision !== 'reject') { throw new Error('VALIDATION_FAILED: decision must be approve|reject'); } @@ -1374,9 +1455,9 @@ export class ApprovalService implements IApprovalService { // hold no slot — the escape hatch for an approval routed to an unstaffed // position or to approvers who have all left. const isOverride = this.isOverrideActor(context, raw.organization_id ?? null); - const isSlotHolder = pendingApprovers.includes(input.actorId); + const isSlotHolder = pendingApprovers.includes(actorId); if (!isSlotHolder && !isOverride) { - throw new Error(`FORBIDDEN: actor '${input.actorId}' is not a pending approver`); + throw new Error(`FORBIDDEN: actor '${actorId}' is not a pending approver`); } const config = parseJson(raw.node_config_json, { approvers: [], behavior: 'first_response' } as any); @@ -1424,7 +1505,7 @@ export class ApprovalService implements IApprovalService { await this.engine.insert('sys_approval_action', { id: uid('aact'), request_id: requestId, organization_id: org, step_name: nodeId, step_index: 0, action: input.decision, - actor_id: input.actorId, comment: input.comment ?? null, + actor_id: actorId, comment: input.comment ?? null, attachments: input.attachments?.length ? input.attachments : null, created_at: now, }, { context: SYSTEM_CTX }); @@ -1572,7 +1653,7 @@ export class ApprovalService implements IApprovalService { context: SharingExecutionContext, ): Promise { if (!requestId) throw new Error('VALIDATION_FAILED: requestId is required'); - if (!input?.actorId) throw new Error('VALIDATION_FAILED: actorId is required'); + const actorId = await this.resolveActor(input?.actorId, context); const rawRows = await this.engine.find('sys_approval_request', { where: { id: requestId }, limit: 1, context: SYSTEM_CTX, @@ -1586,7 +1667,7 @@ export class ApprovalService implements IApprovalService { // The submitter withdraws their own request; a privileged admin may recall // any pending request to release a stuck record (#3424). if (!this.isOverrideActor(context, raw.organization_id ?? null) - && raw.submitter_id && String(raw.submitter_id) !== String(input.actorId)) { + && raw.submitter_id && String(raw.submitter_id) !== String(actorId)) { throw new Error(`FORBIDDEN: only the submitter may recall this request`); } // A returned request is only recallable while it is still the run's live @@ -1602,7 +1683,7 @@ export class ApprovalService implements IApprovalService { await this.engine.insert('sys_approval_action', { id: uid('aact'), request_id: requestId, organization_id: org, step_name: nodeId, step_index: 0, action: 'recall', - actor_id: input.actorId, comment: input.comment ?? null, created_at: now, + actor_id: actorId, comment: input.comment ?? null, created_at: now, }, { context: SYSTEM_CTX }); await this.engine.update('sys_approval_request', { @@ -1667,11 +1748,11 @@ export class ApprovalService implements IApprovalService { input: ApprovalSendBackInput, context: SharingExecutionContext, ): Promise { - if (!input?.actorId) throw new Error('VALIDATION_FAILED: actorId is required'); + const actorId = await this.resolveActor(input?.actorId, context); const raw = await this.loadPendingRow(requestId); const pending = csvSplit(raw.pending_approvers); - if (!context.isSystem && !pending.includes(input.actorId)) { - throw new Error(`FORBIDDEN: actor '${input.actorId}' is not a pending approver`); + if (!context.isSystem && !pending.includes(actorId)) { + throw new Error(`FORBIDDEN: actor '${actorId}' is not a pending approver`); } const config = parseJson(raw.node_config_json, { approvers: [], behavior: 'first_response' } as any); @@ -1697,7 +1778,7 @@ export class ApprovalService implements IApprovalService { await this.engine.insert('sys_approval_action', { id: uid('aact'), request_id: requestId, organization_id: org, step_name: nodeId, step_index: 0, action: 'revise', - actor_id: input.actorId, comment: input.comment ?? null, created_at: now, + actor_id: actorId, comment: input.comment ?? null, created_at: now, }, { context: SYSTEM_CTX }); if (priorSendBacks >= maxRevisions) { @@ -1705,7 +1786,7 @@ export class ApprovalService implements IApprovalService { await this.engine.insert('sys_approval_action', { id: uid('aact'), request_id: requestId, organization_id: org, step_name: nodeId, step_index: 0, action: 'reject', - actor_id: input.actorId, + actor_id: actorId, comment: `Auto-rejected: revision limit (${maxRevisions}) exceeded`, created_at: now, }, { context: SYSTEM_CTX }); await this.engine.update('sys_approval_request', { @@ -1736,7 +1817,7 @@ export class ApprovalService implements IApprovalService { await this.notify({ topic: 'approval.returned', audience: [String(raw.submitter_id)], - actorId: input.actorId, + actorId: actorId, source: { object: 'sys_approval_request', id: requestId }, payload: { title: 'Approval auto-rejected', @@ -1779,7 +1860,7 @@ export class ApprovalService implements IApprovalService { await this.notify({ topic: 'approval.returned', audience: [String(raw.submitter_id)], - actorId: input.actorId, + actorId: actorId, source: { object: 'sys_approval_request', id: requestId }, payload: { title: 'Sent back for revision', @@ -1805,7 +1886,7 @@ export class ApprovalService implements IApprovalService { input: ApprovalResubmitInput, context: SharingExecutionContext, ): Promise { - if (!input?.actorId) throw new Error('VALIDATION_FAILED: actorId is required'); + const actorId = await this.resolveActor(input?.actorId, context); const rawRows = await this.engine.find('sys_approval_request', { where: { id: requestId }, limit: 1, context: SYSTEM_CTX, }); @@ -1814,7 +1895,7 @@ export class ApprovalService implements IApprovalService { if (raw.status !== 'returned') { throw new Error(`INVALID_STATE: request is ${raw.status} (resubmit applies to returned requests)`); } - if (!context.isSystem && raw.submitter_id && String(raw.submitter_id) !== String(input.actorId)) { + if (!context.isSystem && raw.submitter_id && String(raw.submitter_id) !== String(actorId)) { throw new Error('FORBIDDEN: only the submitter may resubmit'); } await this.assertLatestForRun(raw); @@ -1842,7 +1923,7 @@ export class ApprovalService implements IApprovalService { await this.engine.insert('sys_approval_action', { id: uid('aact'), request_id: requestId, organization_id: org, step_name: nodeId, step_index: 0, action: 'resubmit', - actor_id: input.actorId, comment: input.comment ?? null, created_at: now, + actor_id: actorId, comment: input.comment ?? null, created_at: now, }, { context: SYSTEM_CTX }); // The next round only exists if this resume lands — surface `resumed` @@ -1924,7 +2005,7 @@ export class ApprovalService implements IApprovalService { input: { actorId: string; to: string; from?: string; comment?: string }, context: SharingExecutionContext, ): Promise<{ request: ApprovalRequestRow }> { - if (!input?.actorId) throw new Error('VALIDATION_FAILED: actorId is required'); + const actorId = await this.resolveActor(input?.actorId, context); const to = String(input?.to ?? '').trim(); if (!to) throw new Error('VALIDATION_FAILED: `to` (new approver) is required'); const raw = await this.loadPendingRow(requestId); @@ -1934,13 +2015,13 @@ export class ApprovalService implements IApprovalService { throw new Error(`VALIDATION_FAILED: '${to}' is already a pending approver`); } const isOverride = this.isOverrideActor(context, raw.organization_id ?? null); - const from = String(input.from ?? input.actorId).trim(); + const from = String(input.from ?? actorId).trim(); let next: string[]; if (pending.includes(from)) { // Normal hand-off: the actor holds the slot being moved (or is a // system/admin caller acting on a real holder's slot). - if (!context.isSystem && !isOverride && input.actorId !== from && !pending.includes(input.actorId)) { - throw new Error(`FORBIDDEN: actor '${input.actorId}' is not a pending approver`); + if (!context.isSystem && !isOverride && actorId !== from && !pending.includes(actorId)) { + throw new Error(`FORBIDDEN: actor '${actorId}' is not a pending approver`); } next = pending.map(a => (a === from ? to : a)); } else if (isOverride) { @@ -1957,7 +2038,7 @@ export class ApprovalService implements IApprovalService { await this.engine.insert('sys_approval_action', { id: uid('aact'), request_id: requestId, organization_id: raw.organization_id ?? null, step_name: raw.flow_node_id ?? raw.current_step ?? null, step_index: 0, action: 'reassign', - actor_id: input.actorId, comment: input.comment ?? `${from} → ${to}`, created_at: now, + actor_id: actorId, comment: input.comment ?? `${from} → ${to}`, created_at: now, }, { context: SYSTEM_CTX }); // per_group / quorum (#3266): carry the delegated slot's group membership to // the new approver in the snapshot, so their approval still counts for the @@ -1980,7 +2061,7 @@ export class ApprovalService implements IApprovalService { await this.notify({ topic: 'approval.reassigned', audience: [to], - actorId: input.actorId, + actorId: actorId, source: { object: 'sys_approval_request', id: requestId }, dedupKey: `approval-reassign-${requestId}-${to}`, payload: { @@ -2003,9 +2084,9 @@ export class ApprovalService implements IApprovalService { input: { actorId: string; comment?: string }, context: SharingExecutionContext, ): Promise<{ request: ApprovalRequestRow; notified: number }> { - if (!input?.actorId) throw new Error('VALIDATION_FAILED: actorId is required'); + const actorId = await this.resolveActor(input?.actorId, context); const raw = await this.loadPendingRow(requestId); - if (!context.isSystem && raw.submitter_id && String(raw.submitter_id) !== String(input.actorId)) { + if (!context.isSystem && raw.submitter_id && String(raw.submitter_id) !== String(actorId)) { throw new Error('FORBIDDEN: only the submitter may send reminders'); } @@ -2024,7 +2105,7 @@ export class ApprovalService implements IApprovalService { await this.engine.insert('sys_approval_action', { id: uid('aact'), request_id: requestId, organization_id: raw.organization_id ?? null, step_name: raw.flow_node_id ?? raw.current_step ?? null, step_index: 0, action: 'remind', - actor_id: input.actorId, comment: input.comment ?? null, created_at: nowIso, + actor_id: actorId, comment: input.comment ?? null, created_at: nowIso, }, { context: SYSTEM_CTX }); // Per-approver fan-out: concrete identities (user ids / emails) each get @@ -2039,7 +2120,7 @@ export class ApprovalService implements IApprovalService { notified += await this.notify({ topic: 'approval.reminder', audience: [approver], - actorId: input.actorId, + actorId: actorId, source: { object: 'sys_approval_request', id: requestId }, dedupKey: `approval-remind-${requestId}-${nowIso}-${approver}`, payload: { @@ -2062,7 +2143,7 @@ export class ApprovalService implements IApprovalService { notified += await this.notify({ topic: 'approval.reminder', audience: literals, - actorId: input.actorId, + actorId: actorId, source: { object: 'sys_approval_request', id: requestId }, dedupKey: `approval-remind-${requestId}-${nowIso}`, payload: { @@ -2190,26 +2271,26 @@ export class ApprovalService implements IApprovalService { input: { actorId: string; comment: string }, context: SharingExecutionContext, ): Promise<{ request: ApprovalRequestRow }> { - if (!input?.actorId) throw new Error('VALIDATION_FAILED: actorId is required'); + const actorId = await this.resolveActor(input?.actorId, context); if (!input?.comment?.trim()) throw new Error('VALIDATION_FAILED: comment is required'); const raw = await this.loadPendingRow(requestId); const pending = csvSplit(raw.pending_approvers); - if (!context.isSystem && !pending.includes(input.actorId)) { - throw new Error(`FORBIDDEN: actor '${input.actorId}' is not a pending approver`); + if (!context.isSystem && !pending.includes(actorId)) { + throw new Error(`FORBIDDEN: actor '${actorId}' is not a pending approver`); } const now = this.clock.now().toISOString(); await this.engine.insert('sys_approval_action', { id: uid('aact'), request_id: requestId, organization_id: raw.organization_id ?? null, step_name: raw.flow_node_id ?? raw.current_step ?? null, step_index: 0, action: 'request_info', - actor_id: input.actorId, comment: input.comment.trim(), created_at: now, + actor_id: actorId, comment: input.comment.trim(), created_at: now, }, { context: SYSTEM_CTX }); if (raw.submitter_id) { await this.notify({ topic: 'approval.request_info', audience: [String(raw.submitter_id)], - actorId: input.actorId, + actorId: actorId, source: { object: 'sys_approval_request', id: requestId }, payload: { title: 'More information requested', @@ -2229,20 +2310,20 @@ export class ApprovalService implements IApprovalService { input: { actorId: string; comment: string; attachments?: string[] }, context: SharingExecutionContext, ): Promise<{ request: ApprovalRequestRow }> { - if (!input?.actorId) throw new Error('VALIDATION_FAILED: actorId is required'); + const actorId = await this.resolveActor(input?.actorId, context); if (!input?.comment?.trim()) throw new Error('VALIDATION_FAILED: comment is required'); const raw = await this.loadPendingRow(requestId); const pending = csvSplit(raw.pending_approvers); - const isSubmitter = raw.submitter_id && String(raw.submitter_id) === String(input.actorId); - if (!context.isSystem && !isSubmitter && !pending.includes(input.actorId)) { - throw new Error(`FORBIDDEN: actor '${input.actorId}' is not on this request`); + const isSubmitter = raw.submitter_id && String(raw.submitter_id) === String(actorId); + if (!context.isSystem && !isSubmitter && !pending.includes(actorId)) { + throw new Error(`FORBIDDEN: actor '${actorId}' is not on this request`); } const now = this.clock.now().toISOString(); await this.engine.insert('sys_approval_action', { id: uid('aact'), request_id: requestId, organization_id: raw.organization_id ?? null, step_name: raw.flow_node_id ?? raw.current_step ?? null, step_index: 0, action: 'comment', - actor_id: input.actorId, comment: input.comment.trim(), + actor_id: actorId, comment: input.comment.trim(), attachments: input.attachments?.length ? input.attachments : null, created_at: now, }, { context: SYSTEM_CTX }); @@ -2252,7 +2333,7 @@ export class ApprovalService implements IApprovalService { await this.notify({ topic: 'approval.comment', audience, - actorId: input.actorId, + actorId: actorId, source: { object: 'sys_approval_request', id: requestId }, payload: { title: 'New comment on an approval', diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index 11cb9329ab..10838cd4f3 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -6270,6 +6270,14 @@ export class RestServer { // through the contract's `decide()`, which finalizes the request and // resumes the owning flow run down the matching `approve` / `reject` // edge. + // + // On the `actorId` these routes forward (#3800): it is a HINT, not the + // acting identity. The service pins the actor to the authenticated + // caller and accepts a body value only when it can prove the caller + // holds that identity — a slot keyed by a `type:value` literal or by + // the caller's email, which the Console legitimately sends. It is + // forwarded rather than dropped for exactly those cases; naming anyone + // else is `FORBIDDEN` at the service, never here. const decisionRoute = (decision: 'approve' | 'reject') => { this.routeManager.register({ method: 'POST',