refactor(authz): the socket write path runs the shared membership rule, not a copy (TASK-165) - #1943
Merged
Merged
Conversation
…e, not a copy (TASK-165) `server.ts` defined its own `isPodMember` for the socket post path and exported it, so the strict rule had two definitions in `backend/`. The copy had also drifted from the rule it exists to mirror: it compared `member.toString()`, which on a member document Mongo has populated renders `[object Object]` — so a populated member was admitted by `createMessage` and refused by the socket path beside it. The socket write path now calls `isListedPodMember` from `utils/isPodMember` — the export TASK-161 placed beside the permissive predicate, and the one #1942's two readers import — and this module no longer exports a membership predicate of its own, so the rule has one home and no second name. Witnesses sit at the call site, not the definition: a departed creator (`createdBy` present, `members` empty, which is the shape `leavePod` leaves) is refused on the socket write path, and a populated member document is admitted — the second arm is the one the removed copy failed, so it reddens if a local copy returns, and the first reddens if the write path is pointed at the permissive predicate instead. The helper arm that read the removed export now reads the rule where it lives. No behaviour changes for string or ObjectId members.
samxu01
pushed a commit
that referenced
this pull request
Sep 27, 2026
#1943 was open when this entry was written; the past tense asserted a merge that hasn't happened. Entry-only, one sentence.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Cut from
6a52978b(main after #1941). Backend only, no version bump.What this closes
server.ts:503defined its ownisPodMemberfor the socket write path and exported it at:814, so the strict membership rule had two definitions:utils/isPodMember.ts'sisListedPodMember(whatcreateMessageand, since #1940/#1942, the connector and PG readers run) and this copy. Two definitions of a load-bearing rule is the shape the last three PRs kept collapsing, and this was the last one left.The copy had also already drifted, in the opposite direction from the one TASK-161 was about:
createMessage/ shared predicate'user-1'(ObjectId){ toString: () => 'user-1' }(the test double){ _id: ObjectId('user-1') }(Mongo populated the doc)member.toString()is[object Object]So the copy's contract and the HTTP write path beside it disagreed about a populated member document. Whether that could bite in production is now measured, and it could not:
authorizeSocketPodAccessdoes a barePod.findById(podId)with no.populate(), andmodels/Pod.tscarries no autopopulate plugin and no populate hook (grep clean, with a positive control in the same file), somembersarrives asObjectId[]andmember.toString()yields the hex — the copy never refused a real member. The[object Object]defect was real in the predicate's contract and unreachable at this call site, which is what makes the populated-document arm a contract guard against a copy returning rather than evidence of a live divergence. (Measured by @vera, 74692.)The change
server.tsimports{ isListedPodMember }from./utils/isPodMemberand calls it on the write branch ofauthorizeSocketPodAccess; the local definition is deleted.isPodMemberis removed from this module's exports rather than re-exported as an alias: one rule, one name (the only importer was the helper arm inserver.test.js, which now reads the rule where it lives).isListedPodMemberis the strict form —pod.membersalone — so the socket path is exactly as strict as it was for ObjectId members, and no stricter or looser thancreateMessage.Witnesses at the call site, not the definition
server.test.js, throughauthorizeSocketPodAccess(socket, podId, 'post'):refuses a departed creator on the socket write pathcreatedBypresent,members: []→null+Not authorized to post for this pod. Reddens if this path is pointed at the permissive predicate.admits a populated member document, so the socket path runs the shared predicatemembers: [{ _id: { toString } }]→ the pod is returned. This is the arm the deleted copy failed — it reddens if a local copy returns.treats string and ObjectId-like members as valid pod members, and no creatorutils/isPodMemberdirectly: member shapes admitted, creator-not-listed refused.The second arm is the reason this row was worth a PR rather than a comment, and @vera's 74672 point is why both arms exist: a non-member refusal cannot distinguish the shared predicate from a local copy, and neither can a departed-creator refusal — both copies are strict today. What distinguishes them is drift, so the populated-document arm is the drift guard and the departed-creator arm is the wiring guard. Neither is redundant with the other, and the populated-document arm should not be deleted as "the same as the member arm" — it is the only arm that fails when this module grows a copy again.
Mutation ledger
Baseline/restore 11/11,
--forceExit(the suite importsserverand never exits on its own).admits a populated member document…, alonerefuses a departed creator…, aloneNo survivors. M3 does not redden the member arms by construction (they are members, so
trueadmits them), which is why M4 exists as the other half of the control.Verification
serverdirectly; onlyserver.test.jsimported the removed export. The other two destructure{ app }alone —middleware/rateLimitIpKeySeparation.test.js:64androutes/mcpGrants.noRedirect.test.js:96— and were run green on this head rather than reasoned about (2 suites / 8 tests). Same conclusion, with a number that survives checking (measured by @vera, 74692).server.ts: 0 errors, 4 warnings — byte-identical to the warning set atHEAD(lines 234/264/265/587, all pre-existingmax-len), none on a line this PR touched.server.test.js: 0 eslint problems. Un-gated.jscorpus as before.TASK-164(the scheduled feed writer reads no membership at all — a read plus a pause rule, not a swap) and the permissive callers onactivity.ts/podInvites.ts/activityService.ts/decisionRequestService.ts, which are the product question on TASK-166, not this row.