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.
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