Skip to content

fix(authz): createdBy stops counting as membership, and the creator cannot leave (TASK-166) - #1945

Merged
lilyshen0722 merged 1 commit into
mainfrom
kai/task166-ownership-after-leave
Sep 27, 2026
Merged

lilyshen0722 merged 1 commit into
mainfrom
kai/task166-ownership-after-leave

Conversation

@lilyshen0722

Copy link
Copy Markdown
Contributor

Cut from 67d8e252 (main after #1942). Backend only, no version bump.

Wren's ruling (pod 74691): leavePod refuses a creator's leave, createdBy stays as the record of who made the pod, the permissive sites move to the strict predicate, and both sides of the activity pair move — the read rule and the write gate that mirrored it.

What changed

Thirteen predicate terms across five files stop reading createdBy as membership, plus one guard.

file sites what it gates
controllers/podController.ts leavePod new guard: creator → 409 creator_cannot_leave
routes/podInvites.ts :62 create, :104 list, :144 revoke invite management
routes/activity.ts :338 seed, :365 create activity writes
services/decisionRequestService.ts :373 chooseDecision a human ruling
services/activityService.ts getRecap, getUserFeed, getDecisionHistory query + filter, getPodFeed, requireActivityApprovalMember, getPendingApprovals pod list, ambient feed, settled history, pod feed, legacy approval gate, approvals queue

All four route/service files bound require('../utils/isPodMember') — the module default, which is the permissive rule (utils/isPodMember exports the permissive predicate twice, as default and as named isPodMember, and the strict one once, as isListedPodMember). They now import the strict one by name. That export shape is a trap in its own right and wants a row; it is not this PR's.

Why the guard: createdBy is written only at creation and nothing transfers it — a repo-wide search for transferOwner|changeOwner|reassignOwner returns zero — while removeMember is gated on createdBy with no admin fallback ("Only pod admin can remove members"). A creator who left would strand the pod with nobody able to remove a member. Refused rather than stripped, so the field keeps meaning what its name says.

Why both sides of the activity pair: requireActivityApprovalMember's docstring said its read rule was "pod membership (with the creator fallback for old rows), so write authorization must use the identical predicate". That was accurate — which is exactly why narrowing one side alone would have broken read/write agreement on the rows the comment is about. Both moved, and the docstring now says what the rule is.

A live defect this surfaced, and the arm that caught it

getPodFeed carried a third hand-rolled copy of the membership rule:

const isMember = String(pod.createdBy) === String(userId)          // the term this PR narrows
  || (pod.members || []).some((m) => (String(m.userId) || String(m)) === String(userId));

String(undefined) is the non-empty string 'undefined', so || String(m) was unreachable and no member listed as a plain ObjectId or string ever matched. In production only the createdBy term beside it ever matched (Vera's census: 0 of 424 pods carry a members.userId entry), which made GET /api/activity/pods/:podId creator-only.

So narrowing that term without touching the copy would have refused every caller, members included. The control arm — "still serves a listed member" — is what failed and made me look; the copy now calls isListedPodMember, which also leaves this function with one definition rather than two. Blast radius, measured rather than assumed: nothing else in the repo requests that path (grep for activity/pods across frontend/, cli/ and backend/, excluding the route definition, is empty), which is why a read path that refused all non-creator members went unnoticed.

Witnesses — one arm per site

mutation arm red
M1 leavePod guard removed leavePod refuses the creator with 409 creator_cannot_leave and keeps them listed
M2 podInvites create clause back refuses to create an invite for a creator who left
M3 podInvites list clause back refuses to list invites for a creator who left
M4 podInvites revoke clause back refuses to revoke an invite for a creator who left
M5 activity seed clause back refuses a creator who is no longer listed, and never reaches the seeder
M6 activity create clause back refuses a creator who is no longer listed, and writes nothing
M7 getRecap term back builds the viewer's pod list from membership, not from createdBy
M8 getUserFeed term back selects the viewer's pods by membership, not by createdBy
M9 getDecisionHistory query term back reads settled history by membership only
M10 getDecisionHistory filter term back refuses a creator who left the pod, although createdBy still names them
M11 getPodFeed clause back refuses a creator who left the pod
M12 approval gate clause back fails closed when the pod's creator has left it
M13 getPendingApprovals term back queries approvals from listed membership only
M14 chooseDecision clause back refuses a creator who left the pod before claiming the decision

Baseline and restore: 9 suites / 95 tests, exit 0. Fourteen mutations, each applied alone, each reddening exactly its named arm and nothing else — no survivors.

Three things about the arms worth keeping:

  • Every inverted arm carries its control. activity.write-membership previously asserted the creator was admitted with members: []; the file still has an arm for a creator who is listed, so the inversion cannot pass by refusing every pod that names a creator. Same shape in podInvites (a listed creator still manages invites) and getPodFeed (a listed member still gets the feed — which is the arm that found the third copy).
  • Where the decision is in the query, the arm asserts the query and says so. getRecap / getUserFeed / getDecisionHistory query / getPendingApprovals filter MongoDB-side; the Pod mock returns whichever fixture it is handed, so a row assertion would have been satisfied by the mock. The comment on each says that, rather than implying a row was checked.
  • getDecisionHistory needed two arms, not one. Its query drops the term (M9) and its in-process filter re-checks the returned rows (M10); mutating either alone reddens one arm, so neither witness is doing the other's work.

Wider runs and lint

  • __tests__/unit/routes — 139 suites / 1065 tests, green.
  • __tests__/unit/services — 160 suites / 1737, green.
  • __tests__/unit/controllers — 14 suites / 175, green.
  • Changed .ts files: 0 eslint errors and — checked by intersecting eslint's line numbers with the diff's + ranges rather than comparing counts — 0 warnings on any touched line (fatal: true checked while there: none). The one warning the change introduced (activity.ts max-len, from the longer strict ident) was fixed by wrapping the guard, so nothing lands on a touched line.

Disclosed, not hidden: the changed .js test files carry eslint errors from the un-gated .js corpus (~2,279 repo-wide) — import/no-unresolved + import/extensions on every .js test file's .ts requires, including the two new files. They follow the neighbouring files' convention exactly; I did not add a third style to that corpus, and the burn-down is its own task.

Gate: Vera.

…annot leave (TASK-166)

Wren's ruling: `leavePod` refuses a creator's leave, `createdBy` stays as the
record of who made the pod, and the permissive sites move to the strict
predicate. Both sides of the activity pair move — the read rule and the write
gate that mirrored it.

Thirteen predicate terms across five files stop reading `createdBy` as
membership, and one guard is added:

- `controllers/podController.ts` — `leavePod` refuses the creator with 409
  `creator_cannot_leave`. `createdBy` is written only at creation and nothing
  transfers it, while `removeMember` is gated on it with no admin fallback, so a
  creator who left would strand the pod with nobody able to remove a member.
- `routes/podInvites.ts` (3 sites), `routes/activity.ts` (2) and
  `services/decisionRequestService.ts` (1) — the strict predicate, imported from
  `utils/isPodMember` rather than the module's permissive default. All four files
  bound the default, which is the permissive rule.
- `services/activityService.ts` (7 terms) — the `createdBy` arm is dropped from
  `getRecap`, `getUserFeed`, `getDecisionHistory` (query and filter),
  `getPodFeed`, the legacy approval gate, and `getPendingApprovals`, so the read
  rule and the write gate that mirrors it are the same rule.

One live defect surfaced while doing it, and it is the reason the `getPodFeed`
control arm exists: that function carried a THIRD hand-rolled copy of the rule,
`String(member.userId) || String(m)`. `String(undefined)` is the non-empty
string 'undefined', so the fallback was unreachable and no member listed as a
plain ObjectId ever matched. In production only the `createdBy` term beside it
ever matched, which made `GET /api/activity/pods/:podId` creator-only. Narrowing
that term without the fix would have refused every caller; the arm caught it, and
the copy now calls `isListedPodMember`.

Fourteen mutations, one per term, each alone: every one reddens exactly its named
arm and nothing else. Baseline 9 suites / 95 tests, and the wider run is green —
routes 139 / 1065, services 160 / 1737, controllers 14 / 175.
@lilyshen0722
lilyshen0722 added this pull request to the merge queue Sep 27, 2026
samxu01 pushed a commit that referenced this pull request Sep 27, 2026
…ure shape, a mutation that never applied, and load-red

All three came out of the TASK-166/#1945 verification on 2026-09-27 and all
three arrive looking like a result.

Fixture shape: a `DocumentArray`'s cast is part of the measurement.
`doc.members.includes('<hex>')` is true on a hydrated document and false on the
bare array of the same values — while `JSON.parse(JSON.stringify(doc)).members
.includes(hex)` is also true, because serialisation turned the values into
strings. A green `true` has two mechanisms behind it and the test cannot tell
them apart, so the assertion names which one it is.

A mutation is evidence only once the replacement applied: an old-string built by
a shell pipeline came out empty, matched 11,445 sites, and reported a normal
green run — indistinguishable from a surviving mutant unless the edit counts its
matches.

And the load case: a timeout in a cold parallel run is contention before it is a
regression. Nine booting `MongoMemoryServer`s pushed a suite past the global 30 s
with the diff untouched; the same suites then passed 95/95 three ways.

Docs only; no code, no version bump.
Merged via the queue into main with commit b95ec1e Sep 27, 2026
14 checks passed
samxu01 pushed a commit that referenced this pull request Sep 27, 2026
…ure shape, a mutation that never applied, and load-red

All three came out of the TASK-166/#1945 verification on 2026-09-27 and all
three arrive looking like a result.

Fixture shape: a `DocumentArray`'s cast is part of the measurement.
`doc.members.includes('<hex>')` is true on a hydrated document and false on the
bare array of the same values — while `JSON.parse(JSON.stringify(doc)).members
.includes(hex)` is also true, because serialisation turned the values into
strings. A green `true` has two mechanisms behind it and the test cannot tell
them apart, so the assertion names which one it is.

A mutation is evidence only once the replacement applied: an old-string built by
a shell pipeline came out empty, matched 11,445 sites, and reported a normal
green run — indistinguishable from a surviving mutant unless the edit counts its
matches.

And the load case: a timeout in a cold parallel run is contention before it is a
regression. Nine booting `MongoMemoryServer`s pushed a suite past the global 30 s
with the diff untouched; the same suites then passed 95/95 three ways.

Docs only; no code, no version bump.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant