GHSA-p7mm-r948-4q3q: Paperclip: Approval decision attribution spoofing via client-controlled `decidedByUserId` in paperclip server
SummaryThe approval-resolution endpoints (`POST /approvals/:id/approve`, `/reject`, `/request-revision`) accept a client-supplied `decidedByUserId` field in the request body and write it verbatim into the authoritative `approvals.decidedByUserId` column — without cross-checking it against the authenticated actor. Any board user who can access an approval's company can record the decision as having been made by another user (e.g. the CEO), forging the governance audit trail. For `hire_agent` approvals with a monthly budget, the same attacker-controlled string is also stamped onto the resulting `budget_policies` row as `createdByUserId`/`updatedByUserId`.Details**Entry point** — `server/src/routes/approvals.ts:130`:**Authorization check** — `server/src/routes/authz.ts:4`:`assertBoard` only checks that the caller is some board user; it never ties `req.body.decidedByUserId` to `req.actor.userId`. `requireApprovalAccess`/`assertCompanyAccess` only verify the attacker is allowed to touch the approval's company, which every board user in that company already is.**Validator** — `packages/shared/src/validators/approval.ts:13`:The Zod schema accepts any string for `decidedByUserId` — no UUID check, no membership check, no binding to the session.**Sink** — `server/src/services/approvals.ts:54`:**Secondary sink (budget policies)** — `server/src/services/approvals.ts:147-156`, reached when a `hire_agent` approval with `budgetMonthlyCents > 0` is approved:`budgets.upsertPolicy` uses that `actorUserId` to populate `createdByUserId`/`updatedByUserId` on the `budget_policies` row, extending the forgery to budget-policy audit columns.**Same pattern in `reject` and `request-revision`** — `server/src/routes/approvals.ts:229` and `:257`:`approvalService.reject()` and `requestRevision()` (`approvals.ts:175` and `:201`) both write `decidedByUserId` directly into the approvals row.**Why `logActivity` is not a mitigation**: the route handlers correctly use `req.actor.userId ?? "board"` when writing to `activity_log` (e.g. `approvals.ts:151`, `175`, `190`, `212`, `246`, `276`), which shows the developer intent was that the deciding user equals the authenticated user. But the authoritative `approvals.decidedByUserId` column — the value shown to anyone reviewing the approval — is still sourced from the client, so the two records are allowed to diverge and the user-visible attribution is the forged one.**Why this is reachable from a non-admin attacker**: `actorMiddleware` (`server/src/middleware/auth.ts:62-98`) populates `req.actor` as `type: "board"` for any authenticated user (session cookie or board API key); `isInstanceAdmin` is not consulted by `assertBoard`. In a multi-user `authenticated` deployment, any board member of a company can spoof the attribution of any other board member for approvals within that company. In `local_trusted` deployments there is only a single implicit `local-board` user, so the exploit has no target — but the code is shipped for both deployment modes.PoCPrerequisite: a pending `hire_agent` approval `$APPROVAL_ID` in a company where both `attacker@corp` and `ceo@corp` are board members of the `authenticated` deployment. Attacker authenticates with their own session cookie / board API key.Attacker approves as the CEO:Verify the forged attribution is stored on the authoritative row:For `hire_agent` approvals with `budgetMonthlyCents > 0`, confirm the budget-policy row is also stamped with the forged user (direct DB read, or via an endpoint that surfaces `budget_policies.createdByUserId`):The same body works against `/approvals/$APPROVAL_ID/reject` and `/approvals/$APPROVAL_ID/request-revision`.Note: the `activity_log` row written alongside the approval still shows the real attacker's `userId` (correctly taken from `req.actor.userId`), so a defender who looks at `activity_log` will see the discrepancy — but the approval UI, the approvals API, and the budget_policies audit columns all display the forged user.Impact**Forged governance audit trail.** Any board user with access to a company can record approval, rejection, or revision-request decisions under any arbitrary user identifier — including other legitimate board users of that company. Approvals gate security-sensitive actions (agent hiring, which grants execution privileges and assigns a monthly spend budget), and the `approvals.decidedByUserId` column is the authoritative record of who authorized each decision.**Budget-policy attribution forgery.** For `hire_agent` approvals that carry a monthly budget, `budget_policies.createdByUserId` / `updatedByUserId` are also populated from the same attacker-controlled string, spreading the forgery to spend-authorization audit columns.**Non-repudiation break.** A board user can frame another board user for approving/rejecting a hire, undermining accountability for governance actions. The parallel `activity_log` entry does preserve the true actor, but any reviewer inspecting the approval itself (not the activity log) will see the forged attribution as fact.**Scope.** Limited to board users who already have company access; does not escalate privileges, does not leak data, and does not change whether the decision itself gets applied. Integrity impact is Low (attribution only, not decision content); confidentiality and availability are unaffected.Recommended FixDrop `decidedByUserId` from the request schema entirely and derive it server-side from the authenticated actor. Treat `req.body.decidedByUserId` as untrusted and ignore it.**`packages/shared/src/validators/approval.ts`:****`server/src/routes/approvals.ts`** (apply to `/approve`, `/reject`, `/request-revision`):Repeat the same `const decidedBy = req.actor.userId ?? "board";` substitution at `approvals.ts:238` (`/reject`) and `:269` (`/request-revision`). No change is needed inside `approvalService` — it already accepts the value as a parameter — and this also ensures the forged value cannot reach `budgets.upsertPolicy` at `approvals.ts:155`. Existing callers that currently pass a body `decidedByUserId` can be updated to stop sending it (it is already effectively redundant with the session).
CVSS v3.1: 4.3