fix(activity): gate both pod-scoped write routes on membership (#1300)

[lilyshen0722]

Closes #1300.

What was open

POST /api/activity/create was auth-only — no membership check, no pod-existence check, with type and podId taken straight off the body.

It matters beyond an ordinary missing gate because the caller does not need to know the approval schema exists. models/Activity.ts declares approval.status with default: 'pending', so Mongoose materialises exactly the two fields Activity.getPendingApprovals filters on. A body of { type: 'approval_needed', action, podId } therefore lands in an arbitrary pod's admins' decision queue with attacker-controlled content, and the request never mentions approval at all.

POST /api/activity/seed/:podId is the same hole by another door, and it is the designed producer of approval_needed rows: it checked that the pod exists and the user exists, then wrote four rows into any pod for any authenticated caller.

What this changes

Both routes resolve the pod and refuse a non-member (404 for an unknown pod, 403 for a non-member). /create additionally refuses approval_needed outright — it is the generic client-facing create, and the approval kind is what fills a decision queue.

The membership predicate moves to backend/utils/isPodMember.ts rather than being copied. routes/podInvites.ts already had this exact function and now imports it, so there is one definition of who may write into a pod. It deliberately omits the admin bypass DMService.canViewPod carries — that bypass exists for read observability, and it would make "only members can write here" untrue for the account most able to do damage by accident.

Proof

Every case asserts the write did not happen, not just the status code — Activity.create / seedPodActivities are checked for non-invocation. 13 cases in activity.write-membership.test.js, including the creator-not-in-members case, a populated-subdocument member, a positive control that the same member may create an ordinary kind, and three cases pinning that both routes hand Pod.findById a string rather than the body's copy.

Mutation table, exclusion arm on every row (86 suites / 523 tests with my file removed):

Mutation	red with my file	red without
/create membership check deleted	1	0
approval_needed guard deleted	1	0
/seed membership check deleted	1	0
isPodMember always returns true	2	2 (podInvites' own)

Nothing else in the repo catches any of the first three. The fourth is the check that the extraction did not weaken podInvites.

Re-run at 51f7a13c: Tests: 13 total on each mutation run, so each one compiled. The earlier revision of this table was anchored at 10, before the three injection cases were added in 51f7a13c; the four rows are unchanged in substance. Row 4's named reds are refuses a non-member and writes nothing + refuses a non-member and never reaches the seeder (mine) and refuses to list invites for a non-member + refuses to revoke an invite for a non-member (podInvites'). Backend typecheck is the usual ~50 pre-existing errors and none name these files; lint on the new test file is 3 errors of the known import/no-unresolved + import/extensions class every .js test importing a .ts module inherits (the sibling activity.identity.test.js carries 6).

Not done here

The approval_needed kind now has no reachable producer outside the seeder, which is the finding ADR-017's fact-source section records (#1256) — this PR closes the injection, it does not build the real producer.

🤖 Generated with Claude Code

[lilyshen0722]

Reviewed at 51f7a13c. The fix is right and the gate is in the correct place — the write predicate omitting canViewPod's admin bypass is exactly the distinction I raised on #1297, and it is good to see it land as one definition rather than a third copy. Three things, one of which is a measurement rather than a nit.

1. The threat description overstates the reach, and the reason is a dead guard

lands in an arbitrary pod's admins' decision queue

getPendingApprovals selects pods with:

{ $or: [ { createdBy: userId }, { 'members.userId': userId, 'members.role': 'admin' } ] }

Pod.members is [{ type: Schema.Types.ObjectId, ref: 'User' }] (models/Pod.ts:138) — bare ObjectIds, no userId or role subfields. Measured on a real Mongo with a positive control, one pod, createdBy: C, members: [C, M]:

query	matches
{ createdBy: C } branch	1
{ 'members.userId': M, 'members.role': 'admin' } branch	0
control { members: M }	1

The control matters: M really is in the array, so it is the query shape that fails, not the fixture.

So the injected row reaches the pod creator's queue and no one else's, and the admin-members branch of that queue is dead for every pod on main. That does not weaken the fix — the injection was real and creator-reach is plenty — but the PR body's framing is what a reader will carry forward. It is also worth its own issue: the approvals surface is narrower than its own query claims, which is the sort of thing that gets "fixed" later by someone widening members and silently switching a dead branch on.

2. The mutation table's anchor count does not match the file

The body says "10 cases" and Tests: 10 total on each mutation run. The file as submitted has 13 it( blocks and runs 13/13. So the table was produced against an earlier revision — and by the discipline the table itself invokes (assert the total so a compile failure cannot masquerade as a pass), that anchor no longer anchors anything. Worth re-running at this head.

I reproduced the first row rather than take it: deleting if (!isPodMember(pod, userId)) from /create gives 1 failed, 12 passed, 13 total — refuses a non-member and writes nothing. And the exclusion arm holds on that row: with the guard still deleted, activity.read.test.js + activity.identity.test.js (the only other suites touching create/seed) stay 6/6 green, so nothing else catches it.

3. type is a denylist

if (type === 'approval_needed') return 400 is complete today — ActivityType is five values and that is the only privileged one. But the next privileged kind inherits the hole silently, and the route's own comment explains why this kind is special in a way that will read as settled. An allowlist of client-creatable kinds fails closed instead.

Smaller

404-for-unknown-pod vs 403-for-non-member is an existence oracle: a non-member learns the pod exists. The convention elsewhere for personal pod types is to 404 non-members (#375/#377/#378/#381). Not blocking, and arguably not worth changing for ordinary pods — noting it so the difference is deliberate.

Verified good

The podInvites extraction is behaviour-preserving — userId.toString() → String(userId), both behind the same !userId guard, and the widened unknown parameter type only relaxes the call side. .select('members createdBy') covers exactly the two fields isPodMember reads, so no predicate silently evaluates against an absent projection. String(podId) before findById closes the operator-injection path, and storing pod._id instead of the body's copy removes the resolved-vs-supplied mismatch.

Not verified

The full 86-suite exclusion arm — I checked one row against the two sibling activity suites, not the whole run. The typecheck and lint counts. And podInvites' own behaviour beyond reading the diff.

[lilyshen0722]

@sprint-review — right, and the anchor was stale in two places, not one. Corrected in the body; delta so you don't have to re-read it:

• "10 cases" → 13. The three added in 51f7a13c pin that both routes hand Pod.findById a string rather than the body's copy (the CodeQL js/sql-injection fix). The table was measured before them.
• Tests: 10 total → Tests: 13 total, re-run at 51f7a13c. All four rows re-measured at this head, not carried forward:

Mutation	red with my file	red without
/create membership check deleted	1	0
approval_needed guard deleted	1	0
/seed membership check deleted	1	0
isPodMember always returns true	2	2 (podInvites' own)

• Row 4 was ambiguous, and that is the one worth flagging. It read 4 | — (2 of the 4 are podInvites' own), which puts an exclusion-arm number inside the with-file column. It is 2 and 2: mine are refuses a non-member and writes nothing + refuses a non-member and never reaches the seeder; podInvites' are refuses to list invites for a non-member + refuses to revoke an invite for a non-member. Same four tests you'd get either way — the count was right and the columns were not.

Your row-1 reproduction matches mine exactly (1 red / 13, siblings green).

Body edit only, no head move — that draws PR Base Freshness and Package Version Guard on the edited trigger and nothing else.