Activity approve/reject routes have no membership or owner check — any authenticated user can resolve any approval

[lilyshen0722]

POST /api/activity/:activityId/approve and /reject carry no authorization beyond auth. Any authenticated user who knows an activity id can approve or reject an approval_needed activity in a pod they are not a member of.

Measured at origin/main 9ee1fac6d, not read off the source.

Reproduction

Caller is a member of no pod; the activity belongs to pod-1, whose createdBy and members are someone-else.

APPROVE status=200 approveCalled=1 body={"success":true,"status":"approved"}
CONTROL/create status=403 createCalled=0 body={"error":"Only pod members can create activities in a pod"}

The /create line is the positive control, run in the same harness against the same mocked pod and caller. It refuses and never reaches the model — so the harness can see a guard, and approve's 200 is the route's behaviour rather than a bypassed mock. activity.approve was called with the attacker's own userId, so the row records them as the approver.

Where the gap is

routes/activity.ts requires isPodMember and applies it on exactly two routes:

• :231 — POST /seed/:podId
• :258 — POST /create

:197 /:activityId/approve and :211 /:activityId/reject are auth-only. The service behind them does not compensate: ActivityService.approveActivity / rejectActivity do Activity.findById, check activity.type !== 'approval_needed', and call activity.approve(userId, notes). Neither loads the pod.

__tests__/unit/routes/activity.write-membership.test.js covers /create and /seed/:podId only — the file's own header names the create hole as the one that mattered, and the approve/reject pair was not in that sweep.

Why it matters

This is the resolve verb for ADR-020 cards, whose D3 states cards are actionable by the workspace owner only, and TASK-095 v1.1 is currently being specced with resolveApproval as the single resolve verb for clickable options. A ruling surface that anyone can press is worth closing before more UI points at it.

Not caused by, but adjacent to, #1451

#1451 is a correct fix to a separate dead-predicate bug in getPendingApprovals and should not be held for this. It does widen which users see pending approvals (creator-only → creator + members), which surfaces more activity ids to more people; those people are legitimate pod members, so it is exposure rather than escalation. The hole here is independent of it and predates it.

Suggested fix

Load the activity's pod and apply the same isPodMember check the sibling write routes use — plus whatever narrower owner rule ADR-020 D3 actually intends, since "workspace owner only" is stricter than pod membership. Pin both with a non-member test and a member positive control.

[lilyshen0722]

Refining reachability, since severity turns on it. Re-measured at 6a7cde848.

@pod-architect measured that getPendingApprovals reads Activity rows while ADR-020 cards are ApprovalAction rows, and that approvalActionService.ts holds zero Activity references — I confirm all three independently. So the routes in this issue govern the Activity surface, not the card surface, and the two stores are genuinely disjoint today.

Their accompanying claim that seedPodActivities is the only producer of approval_needed Activity rows needs one correction. There is a second: Activity.createApprovalRequest (models/Activity.ts:175), which builds a real agent-access-request row — type: 'approval_needed', approval: { status: 'pending', requestedBy, requestedScopes } — and is wrapped by ActivityService.createApprovalRequest (services/activityService.ts:1251). I enumerated every reference repo-wide excluding tests; the only four hits are the interface declaration, the implementation, the wrapper, and the wrapper's call into it. No caller. It is dead code, not missing code — a literal-type: grep does not surface it, which is how it was missed.

Two consequences for this issue:

1. It is exploitable today, not merely latent. POST /api/activity/seed/:podId is live and isPodMember-gated (routes/activity.ts:231), and seedPodActivities writes approval_needed rows into whatever real pod is named. So any pod member can create the target, and — per the reproduction above — any authenticated user can then resolve it. Impact is currently bounded by the rows being demo copy ("An agent is requesting access to the Production pod").

2. That bound is one call site deep. createApprovalRequest is clearly intended to be reached — it is the agent-access-request path. The moment anything wires it up, the queue carries genuine authorization requests and the unguarded resolve stops being low-value.

None of this changes the defect or the fix; it argues against deferring on "the queue is only seed data".

[lilyshen0722]

Severity update — I understated this, and #1470 changes the recommended fix.

Re-measured at origin/main = 12e76a223.

This is not a latent route. The shipped UI presses it. frontend/src/v2/components/V2ActivityPage.tsx:160 posts to /api/activity/${item.id}/${action}, and the approve/reject buttons at :337/:340 call it. The backend path behind it has no authorization of any kind:

• routes/activity.ts — POST /:activityId/approve and /reject carry auth only. No isPodMember, no owner check. Compare /seed/:podId and /create in the same file, which both do if (!isPodMember(pod, userId)) return res.status(403).
• services/activityService.ts — approveActivity is Activity.findById(activityId) → type !== 'approval_needed' → activity.approve(userId, notes). The only argument that reaches a check is the activity id.

So any authenticated user who knows or guesses an activity id can resolve any approval_needed row in the instance.

#1470 fixes the exploitable path and orphans the routes. At head 2da56971 the component posts to /api/approvals/:id/resolve instead — resolveApproval, which gates ownerUserId !== callerUserId → 403 and refuses bots. Its decision queue reads ApprovalAction.find({ ownerUserId: userId, status: 'flagged' }), so the queue filter and the resolve gate share one owner scope, and legacy Activity approval rows are deliberately excluded.

After #1470 lands, git grep finds no production caller of /:activityId/approve|reject — only the route definitions themselves.

Recommendation changed: delete the routes rather than gate them. Adding a membership check would preserve a second, weaker approval surface next to the ADR-020 one and re-create the two-store split that made this defect possible. Deleting them removes the class. ActivityService.approveActivity/rejectActivity and Activity.createApprovalRequest (which has zero callers, per ADR-017 line 289) go with them.

Sequencing: this should follow #1470, not precede it — the routes are load-bearing for the current UI until that merges.