approvals: the acting identity must be resolved server-side, not taken from the request payload

[os-zhuang]

Found while reviewing #3783's blast radius. Hardening task.

Detail is deliberately withheld here. This is a public repo with published
packages; the full analysis, the verification and the impact assessment live in a
private draft advisory under the repo's Security tab, per this repo's
split-disclosure convention. This issue tracks the requirement and the design.

Requirement

On every ApprovalService entrypoint the acting identity must be resolved
server-side from the authenticated execution context. Today it is taken from
the request payload (actorId), which REST forwards verbatim, and the service's
own checks then read that same value. An identity the caller supplies must never
be the thing that satisfies the check that gates the action.

#3783 touched the adjacent seam and pinned the data-write identity to the
context (actingUserId), but scoped itself there and characterised the remaining
audit-row use as tolerable. That characterisation was too generous: the same
field is read by the authorization checks, so it needs the same treatment.

Why the rule cannot be "actorId must equal context.userId"

A slot is not always keyed by a user id, so strict equality would reject
legitimate approvers:

• resolveApproverSpec stores the type:value literal when a graph lookup
  yields nothing, so a slot can read position:finance.
• The showcase documents deciding with exactly that form.
• The Console picks from the caller's own identity list — id, email, or
  role:<r> — and gates its buttons on that set client-side.

Design

The rule is "the actor must be an identity the server can prove belongs to the
caller", implemented as ApprovalService.resolveActor:

• system context → keeps its explicit actor (the SLA sentinel, the ADR-0043
  action-link token, both of which derive their actor server-side already).
  Unchanged.
• no identity at all → FORBIDDEN.
• no actorId, or actorId === context.userId → the caller. The common path.
• anything else → allowed only when it matches position:<p> / role:<p>
  for a position on the server-resolved context, or the caller's own email (one
  lazy sys_user read, only when nothing cheaper matched). Otherwise
  FORBIDDEN.

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

This also makes two existing claims true that were previously aspirational:
sys-approval-request.object.ts ("actorId defaults to the caller server-side…
The service remains the authority on who may act"), and attachViewers' claim
that can_act "mirrors the exact authorization the decision methods enforce."

Note for reviewers of the fix

A number of existing tests fail against it, every one because it acted with a
context that named nobody, or named someone other than the actor it claimed —
approval-revise.test.ts shared one context with no userId across all its
calls, and approval-service.test.ts called as one user while passing another
as actorId. Each was updated to present the acting user's own context. Same
shape as #3760, where tests had likewise pinned the old behaviour.

Refs #3783, #3760. Fix in #3805.

[os-zhuang]

Follow-up observation from the fix (PR #3805), left deliberately out of scope there.

attachViewers computes viewer.can_act from the bare context.userId:

// approval-service.ts:3241
can_act: row.status === 'pending' && !!uid && pending.includes(uid),

Its docblock claims this "mirrors the exact authorization the decision methods enforce". After #3805 that is true for the common case but still under-reports for a slot keyed by something other than the bare user id — a position:<p> literal or the caller's email — where resolveActor now legitimately lets the caller act while can_act says false.

The Console papers over this by computing its own client-side identity set (buildApproverIdentities → id / email / role:<r>) and gating buttons on that instead of on viewer.can_act (ApprovalsInboxPage.tsx:800-804). So the two ends already disagree about who may act, and the client is the one that happens to be right.

Worth converging on one server-side answer: have attachViewers ask the same resolver resolveActor uses, and let the Console drop buildApproverIdentities in favour of viewer.can_act. That would also delete a duplicated identity vocabulary that has to be kept in sync across two repos.

Not urgent — it fails closed (a button hidden that should be shown), which is why it went unnoticed.