Repository navigation
fix(authz): the PG chat path reads Mongo membership, not its own mirror (TASK-162) - #1942
Merged
Merged
Conversation
…or (TASK-162)
A member who left a pod kept read and write on `/api/pg/messages` because the PG
`pod_members` row was consulted first and a row alone concluded membership. The
mirror cannot be authoritative: `PGPod.create` inserts the owner unconditionally
and `syncPodFromMongo` backfills Mongo's `createdBy`, so a leave plus any later
backfill re-creates the row. Measured read-only on production (Vera 74648, 727
rows): 77 rows present in PG and absent from their pod's Mongo `members` — 36 of
them the pod's own creator — plus 140 rows for a pod id Mongo does not have,
which passed because the fallback never read Mongo at all.
Both platform readers now take Mongo `members` through the one predicate
TASK-161 placed beside `isPodMember` (`isListedPodMember`): the PG chat
controller, for reads and writes, and `callerHasPodWriteAccess` (reactions,
thread state). That predicate is the rule `createMessage` runs, so the two write
paths cannot disagree.
No data write and no migration: the stored rows stop deciding access the moment
the readers stop trusting them. The 77 + 140 row cleanup stays the separate
dry-run-first script on the operator's word, as the row requires.
Disclosed narrowing: `callerHasPodWriteAccess`'s agent fallback also accepted
`{ userId }`-shaped member entries, which the model does not store
(`models/Pod.ts:157` is ObjectId[]) and `createMessage` refuses. Both branches
refuse that shape now, with an arm that states it rather than leaving it implied.
This was referenced Sep 27, 2026
Merged
Merged
lilyshen0722
added a commit
to thantiklermcirony/commonly
that referenced
this pull request
Oct 9, 2026
…e, not a copy (TASK-165) (Team-Commonly#1943) `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 Team-Commonly#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.
lilyshen0722
added a commit
to thantiklermcirony/commonly
that referenced
this pull request
Oct 9, 2026
…reader, and give the dead rows a script (TASK-167) (Team-Commonly#1948) * chore(pg): witness the swallowed mirror write, delete the membership reader, and give the dead rows a script (TASK-167) Three follow-ups from Team-Commonly#1942, none of them a live defect. (1) The swallowed catch in `isPodMemberInMongo` is load-bearing and untested: removing the inner `try`/`catch` around `PGPod.addMember` left every suite green, yet that catch is what keeps a rejected cache write from reaching the outer catch, which answers 401 to a member Mongo lists. The rejection is ordinary — the FK on `pod_members.pod_id` fires when the pod has no PG row yet. The new arm drives it and asserts the member is admitted. (2) `PGPod.isMember` had zero production callers and three test arms. Deleted rather than relabelled: a method with that name on the PG model is what the next author reaches for, and the answer it gives authorises nothing. A source assertion with a positive control covers the absence, since no execution can show a method is gone. Its test arms went with it, and the four mirror mocks in the PG message suites were dropped — they mocked a reader that no longer exists. (3) `scripts/cleanup-ghost-pod-members.ts` removes the two dead row classes and names what it leaves. Dry run by default; `--apply` to write; the operator decides when. Built to the gate's three requirements: two numbers reconciled against the observed post-state, the survivor as the discriminating observation, and the orphan predicate separate from the ghost one with a read failure failing closed. Run only on the operator's word — nothing runs it. * fix(pg-script): a failed reconciliation must not exit 0 (TASK-167 gate) The report already carried `reconciled: false`; `main()` printed it and returned success. So the more serious of the two conditions — the one that can only be discovered *after* rows are gone — reported success to anything reading the exit status, while `refused` (nothing written, re-run is safe) reported failure. The decision now lives in one exported function, so a new condition cannot be added to the log without being added to the status: refused -> 2 (nothing was written) reconciled === false -> 3 (something was written; inspect first) otherwise -> 0 Distinct codes so an operator reading a status can tell those apart without parsing logs; refusal wins if both are somehow true. `main(argv = process.argv)` so the status can be witnessed by a test without mutating the test runner's own argv. Arms (4): the diverged store exits 3; a clean store exits 0 (control, so 3 is not a blanket non-zero); a refused store exits 2; and the mapping table, including the both-true precedence. The first three call `main` for real with mongo mocked — the row's point is what the *process* reports, and the report had been right all along. Ledger against this head, 3 mutations each alone, no survivors: M8 process.exitCode hardcoded to 0 -> diverged arm + refused arm M9 divergence branch dropped -> diverged arm + mapping arm M10 refusal/divergence codes swapped -> all three arms The seven witnesses Vera already cleared were re-run against this head (23 -> 27 tests) and reproduced, with two new couplings worth noting: M3 (read failure classified as orphan) now also reddens the refused-exit arm, and M6 (reconciliation always true) also reddens the diverged-exit arm — the divergence is now witnessed in the report and in the status. One claim I wrote into a comment and then measured instead of trusting: a leaked `process.exitCode` does NOT make jest fail a green run (jest assigns its own status at teardown, exit 0 measured with the reset removed). The reset stays, to stop one arm reading a previous arm's status, and the comment now says what was measured rather than what was convenient.
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
7f1c7e4f(main, after #1940 merged into it — this PR importsisListedPodMemberfrommain, not from a stacked branch). Backend only: no UI, no versioned package, no data write and no migration.The defect
/api/pg/messages/:podId(routes/pg-messages.ts:61,authonly, live atserver.ts:399) gated reads and writes onisMemberWithFallback, which asked the PGpod_membersmirror first and returned true on a row alone. The mirror cannot be authoritative:PGPod.create(models/pg/Pod.ts:37-38) inserts the owner unconditionally andsyncPodFromMongo(:49-56) backfills Mongo'screatedBy, whileleavePodfilters Mongomembersonly and never deletes the PG row.Measured read-only on production (Vera 74648, 727
pod_membersrows):members; 36 of them the pod's own creator.The second reader was the same shape:
callerHasPodWriteAccess(services/podWriteAccessService.ts) checkedpod_membersfirst for human callers, so a departed member kept reacting and writing thread state, and its agent fallback carried a third copy of the membership test inline.The fix
Both readers take Mongo
membersthrough the one predicate TASK-161 placed besideisPodMember:pgMessageController.isPodMemberInMongo(renamed fromisMemberWithFallbackbecause it no longer falls back to the mirror) — reads the pod, appliesisListedPodMember, and warms the PG row afterwards for the listing surfaces; a stale row never decides.callerHasPodWriteAccess— the PG query is gone entirely. The agent branch keeps itsAgentInstallationshort-circuit (posting rule) and then uses the same predicate instead of its inline copy.isListedPodMemberis the rulecreateMessageruns (pod.membersalone), which is the row's requirement: PG must not admit anyone the pod's own write path would refuse.The stored rows become inert rather than deleted. The 77 + 140 row cleanup stays its own dry-run-first script on the operator's word — deliberately not in this PR, and not inline in a deploy.
Witnesses
The row's witness as written ("leave a pod, then POST → 403") passes without the fix once the mirror is cleaned, and would pass under a mirror-on-leave fix as well. The arm that discriminates is the survivor: the stale row still present and saying yes, Mongo membership gone.
refuses a post whose PG pod_members row survived a leaveservice/pgMessages.test.js)PGPod.isMembermocked true and Mongo lists nobody → 401,PGMessage.createnot calledrefuses a post from a member whose PG row survived their departurePGPod.isMemberasserted not calledrefuses a read from the same stale row, so the ghost does not leak historyPGMessage.findByPodIdnot calledrefuses a departed CREATOR whose PG row is presentrefuses a caller for a pod Mongo no longer has, whatever PG holdshuman IN mongo pod.members with no PG row is still allowedhuman caller is admitted from Mongo members, and no PG pod_members row is readpool.querywas called once — the message lookup, not membershipMutation ledger
Baseline and restore 40/40 green; each mutation reverted before the next.
pgMessageControllerpodWriteAccessService{userId}-object shape is honoured again at the agent fallbackThree disclosures from that table:
refuses a caller whose PG row outlived their membership,...departed CREATOR...,...pod Mongo no longer has..., and the orphan control). Stated because a "9 red" presented alone would overstate what the reaction arms prove.failedcounts both.Verification
unit/controllers,unit/services,unit/models/threadStateReadContract,service/pgMessages).reactionController.test.jsandservice/pgMessages.test.jsgranted access through the PG row (memberLookup(1)/PGPod.isMember.mockResolvedValue(true)) and had no Mongo fixture. Each now names the caller in the pod Mongo returns; the unusedmemberLookuphelper is deleted; one arm's title changed from "human caller hits the pg pod_members path" to the contract that replaced it, withpool.queryasserted called once..tsfiles carry 0 eslint diagnostics at HEAD and atHEAD~1. The touched/new.jssuites carry the ambientimport/no-unresolvedclass (the corpus is un-gated).PGPod.isMembernow has no non-test caller — left in place rather than deleted, so this diff stays a permission change and not a model refactor.callerHasPodWriteAccessused to treat{ userId: <id> }member entries as membership.models/Pod.ts:157stores ObjectId[] andcreateMessagecomparesmemberId.toString(), so such an entry is refused by the primary path already. If productionpods.memberscontains any such entries ($elemMatch: { userId: { $exists: true } }), this PR is a new 403 for those callers and that data needs its own row; the arm added for it is written so it can be inverted with the census.