fix(approvals): an approval action is recorded against the authenticated caller (#3800) (#3805)

The approvals REST routes filled the acting identity from
`body.actorId ?? body.actor_id ?? context.userId`, and the service then
authorized *that value* — `pending_approvers.includes(input.actorId)` for a
decision, `submitter_id === actorId` for a recall — never checking that it named
the caller. Any authenticated user could POST `{"actorId": "<someone else>"}`
and have that person's approval recorded, the request finalized and the owning
flow resumed; with `api.requireAuth` unset, so could an anonymous one.

#3783 scoped this correctly 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 how you got through the door, not merely how you
mislabelled the row.

`ApprovalService.resolveActor` now pins the actor on all nine entrypoints. The
rule is not "actorId must equal context.userId": a slot can legitimately be
keyed by a `type:value` literal or by the caller's email, and the Console sends
those. It is "the actor must be an identity the server can prove belongs to the
caller" — the caller's own id, a `position:`/`role:` token backed by the
resolved authz context, or their own email. A system context keeps its explicit
actor, so the SLA sweep's sentinel and the ADR-0043 action link are unchanged.
A caller with no identity at all is refused.

REST keeps forwarding the body value as a hint the service validates, which is
what preserves the email and `type:value` slot cases.

Tests: `approval-actor-impersonation.test.ts` — 16 cases, 12 of which passed the
impersonation before this change. 24 existing tests also failed against the fix,
every one because it acted with a context naming nobody (`approval-revise.ts`
used one identity-less `USER_CTX` for all 32 calls) or someone other than the
actor it claimed; each now presents the acting user's context.

Filed the adjacent bypass separately as #3801: the generic
`runs/:runId/resume` route advances a suspended approval node down the `approve`
edge with no approver check and no audit row.

Co-authored-by: Jack Zhuang <277994282+os-zhuang@users.noreply.github.com>
Co-authored-by: Claude <noreply@anthropic.com>

Author: os-zhuang

.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": "<someone else>"}` 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:<r>`. 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:<p>` / `role:<p>` 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".

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<string, FakeRow[]> = {};
+  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<string, any> = {}) {
+  return {
+    approvers: approvers.map(v => ({ type: 'user' as const, value: v })),
+    behavior: 'first_response' as const,
+    ...extra,
+  };
+}
+
+function openInput(approvers: string[], extra: Record<string, any> = {}, configExtra: Record<string, any> = {}) {
+  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<typeof makeFakeEngine>;
+  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<string, any> = {}) =>
+    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');
+  });
+});