Skip to content

fix(authz): the PG chat path reads Mongo membership, not its own mirror (TASK-162) - #1942

Merged
lilyshen0722 merged 1 commit into
mainfrom
kai/task162-pg-membership-truth
Sep 27, 2026
Merged

lilyshen0722 merged 1 commit into
mainfrom
kai/task162-pg-membership-truth

Conversation

@lilyshen0722

Copy link
Copy Markdown
Contributor

Cut from 7f1c7e4f (main, after #1940 merged into it — this PR imports isListedPodMember from main, 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, auth only, live at server.ts:399) gated reads and writes on isMemberWithFallback, which asked the PG pod_members mirror 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 and syncPodFromMongo (:49-56) backfills Mongo's createdBy, while leavePod filters Mongo members only and never deletes the PG row.

Measured read-only on production (Vera 74648, 727 pod_members rows):

  • 77 ghost rows — present in PG, absent from that pod's Mongo members; 36 of them the pod's own creator.
  • 140 rows referencing a pod id Mongo does not have, which still passed membership because the fallback never consulted Mongo.

The second reader was the same shape: callerHasPodWriteAccess (services/podWriteAccessService.ts) checked pod_members first 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 members through the one predicate TASK-161 placed beside isPodMember:

  • pgMessageController.isPodMemberInMongo (renamed from isMemberWithFallback because it no longer falls back to the mirror) — reads the pod, applies isListedPodMember, 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 its AgentInstallation short-circuit (posting rule) and then uses the same predicate instead of its inline copy.

isListedPodMember is the rule createMessage runs (pod.members alone), 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.

arm tier what it holds
refuses a post whose PG pod_members row survived a leave route (service/pgMessages.test.js) PGPod.isMember mocked true and Mongo lists nobody → 401, PGMessage.create not called
refuses a post from a member whose PG row survived their departure controller 401; PGPod.isMember asserted not called
refuses a read from the same stale row, so the ghost does not leak history controller 401; PGMessage.findByPodId not called
refuses a departed CREATOR whose PG row is present controller + write gate 36 of the 77 ghosts are creators — the creator clause and the stale row reinforce each other
refuses a caller for a pod Mongo no longer has, whatever PG holds write gate the 140-row orphan class
human IN mongo pod.members with no PG row is still allowed controller + write gate the inverse direction, and the reason Mongo decides: the mirror lags, so an absent row is not evidence against membership (the 2026-07-24 65/66 incident)
human caller is admitted from Mongo members, and no PG pod_members row is read controller asserts pool.query was called once — the message lookup, not membership

Mutation ledger

Baseline and restore 40/40 green; each mutation reverted before the next.

mutation red
M1 the PG row is trusted again in pgMessageController 7
M2 the PG row is trusted again in podWriteAccessService 9
M3 the {userId}-object shape is honoured again at the agent fallback 1, its arm alone
M4 the permissive creator clause returns at both sites 2 (one name, both suites)
M5 control: a blanket grant at the human gate 5

Three disclosures from that table:

  • M2's 9 is not 9 contract arms. Five of them are reaction-suite arms that fail because the reverted code queries a PG mock which, since this PR, has no membership response queued — they show the mirror is consulted, not that the rule holds. The four that assert the contract are the write-gate arms (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.
  • M4 reddens one name in two suites — the two departed-creator arms share a title, so the name list dedupes while failed counts both.
  • M3 was a survivor on the first run, and that is the most useful line here. The arm added for the narrowing covered the human path only; the agent branch's inline copy stayed green. The surviving mutation is what found the missing arm, not the reasoning about it.

Verification

  • Targeted suites: 40/40 across the four touched suites; the wider set 173 suites / 1923 tests green (unit/controllers, unit/services, unit/models/threadStateReadContract, service/pgMessages).
  • Fixture fallout, disclosed rather than tidied: 12 arms across reactionController.test.js and service/pgMessages.test.js granted 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 unused memberLookup helper is deleted; one arm's title changed from "human caller hits the pg pod_members path" to the contract that replaced it, with pool.query asserted called once.
  • All three touched .ts files carry 0 eslint diagnostics at HEAD and at HEAD~1. The touched/new .js suites carry the ambient import/no-unresolved class (the corpus is un-gated).
  • Not done, deliberately: the row's PGPod.isMember now has no non-test caller — left in place rather than deleted, so this diff stays a permission change and not a model refactor.
  • Open question for the gate, with the query shape: callerHasPodWriteAccess used to treat { userId: <id> } member entries as membership. models/Pod.ts:157 stores ObjectId[] and createMessage compares memberId.toString(), so such an entry is refused by the primary path already. If production pods.members contains 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.

…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.
@lilyshen0722
lilyshen0722 added this pull request to the merge queue Sep 27, 2026
Merged via the queue into main with commit 67d8e25 Sep 27, 2026
14 checks passed
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.
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