Skip to content

fix(authz): the connector owner routes require listed membership, not just createdBy (TASK-168) - #1946

Merged
lilyshen0722 merged 1 commit into
mainfrom
kai/task168-connector-owner-routes
Sep 27, 2026
Merged

lilyshen0722 merged 1 commit into
mainfrom
kai/task168-connector-owner-routes

Conversation

@lilyshen0722

@lilyshen0722 lilyshen0722 commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

Cut from f77c18e5 (main after #1943). Backend only, no version bump.

Cleared by @vera at 358427ac (74705): each of the six sites reddens its own arm alone, lint 0 errors and 0 on any touched line. Body-only edits since — head unchanged.

The defect

createdBy records who made the pod and survives leavePod — leaving filters members, not the field. Five owner routes gated on the field alone:

route what it exposed
POST /:id/connect re-attach the pod's channel
POST /:id/disconnect tear the pod's channel down
GET /:id/stats the channel's telemetry
GET /:id/messages read the pod's Discord channel (DiscordService.fetchMessages)
POST /:id/send post into the pod's Discord channel (DiscordService.sendMessage)

After TASK-161/162 closed every relay and chat path to a departed creator, these five still answered to one — the same hole, through a route that calls no predicate at all, which is also why #1940's census walked past them.

canDeleteIntegration had the same shape on its pod-creator arm, and that function is the write gate for six further call sites (both ingest-token routes, the connect code, the PATCH, and DELETE).

The fix

isListedPodMember(pod, callerId) beside the creator check at each of the five routes, and on canDeleteIntegration's pod-creator arm. No new import, and no new spelling to pick wrong: this file already imports the strict rule at line 38 — require('../services/connectorRelayPolicy'), which re-exports utils/isPodMember's strict rule and says why in its own header ("every connector site reads one definition through this module"). The row's spec pointed at utils/isPodMember; that is where the rule is defined, and this file reaches it through the connector layer's re-export. The admin and integration-creator arms are untouched: neither ever claimed pod membership.

Why the exposure is exactly "a creator who left"

models/Pod.ts:194 pushes createdBy into members for every new pod, so a pod's creator is listed from birth and createdBy ∉ members means they left. That is what makes this shape a defect rather than a tautology — and it caught my first version of the arms, which declared members: [bystander] on a creator's pod and passed without ever reaching the predicate: the hook silently repaired the fixture into a listed creator. The fixtures now $pull the creator after creation, which is what leavePod does and which the hook's isNew guard leaves alone. A future author adding an arm here will hit the same trap; it is commented in the test file.

Exposure count — zero today, measured from both directions

Vera ran it on live Mongo, which my lane cannot reach: 59 pods whose creator is not in members, carrying 0 integrations, and of the 14 integration rows, 13 have a podId and 0 of those point at an unlisted creator's pod. Latent, like TASK-164 — the routes are unreachable for the population that exists today, and the two-directional census is what makes that a measurement rather than a hope. (Query asked of Vera: pods where createdBy is absent from members ∩ active integrations, grouped by type.)

"A creator no longer listed" — not "a creator who left"

My build note argued that models/Pod.ts:194 (the pre-save hook that pushes createdBy into members on every new pod) makes createdBy ∉ members mean the creator left. @vera corrected the reason, and it matters for scope: leavePod has no client caller (wren, 74691), so the 59 pods we measured were not produced by leaving. They were produced by the agent-side removals — agentIdentityService.ts:710 and agentInstallationCleanupService.ts:299 $pull an agent user, plus three one-shot bot migrations — consistent with all 29 unlisted creators being bots. The leavePod guard (#1945) bounds the human set at one; the bot set keeps being produced by agent cleanup, guard or no guard. So the guard is right and it is not what protects these five routes: the membership read is, which is why it sits on every pod-creator arm rather than on the leave path.

The direction this fix fails in silently

All six sites call Pod.findById(...) with no select, and isListedPodMember needs pod.members — but the TS cast on those lines types the document as { createdBy?: … } only. A future reader "tightening" that to .select('createdBy') would refuse every creator, listed or not, at all five routes. @vera applied exactly that projection to the messages route and it reddened a listed creator is unaffected: all five owner routes still act, so the requirement is witnessed by a control rather than by a refusal arm. If you are editing one of those casts, that arm is the one that will tell you.

Witnesses

Nine arms, real Pod/Integration/User rows on memory Mongo. The two proxy routes assert the side effect did not happen (fetchMessages / sendMessage not called), not only the status — a 403 reached after the call would be no fix at all.

# arm red
M1 /:id/connect added term removed refuses /connect … and never connects
M2 /:id/disconnect term removed refuses /disconnect … and never disconnects
M3 /:id/stats term removed refuses /stats … and never reads stats
M4 /:id/messages term removed refuses /messages … and never reads the channel
M5 /:id/send term removed refuses /send … and never posts to the channel
M6 canDeleteIntegration pod arm term removed refuses DELETE for a pod creator who is no longer listed, and removes nothing
M7 canDeleteIntegration integration-creator arm removed (control liveness) the integration-creator arm is untouched: a departed pod creator who created the connector still deletes it
M8 seam: the shared predicate regains its creator clause all six refusal arms, 3 controls still green

Every mutation ran alone and reddened exactly its named arm — no survivors. Two are controls rather than site mutations and are labelled as such: M7 proves the third arm is what admits the handover case, so my "untouched" claim is witnessed rather than asserted; M8 mutates the shared rule instead of a call site, which is why it reddens six arms at once and is expected to.

Three control arms keep the refusals honest: a listed creator still acts on all five routes (one arm walking all five), a listed pod creator still deletes a connector someone else made, and the integration-creator arm still admits a departed pod creator who created the connector. Without them, "refuse the creator" would pass by refusing every pod that names one.

Runs and lint

  • __tests__/unit/routes — 140 suites / 1068 tests, green (this adds one suite and nine tests).
  • The same tree with this file excluded — 3262 passed, 24 skipped, so nothing else depends on the old behaviour.
  • routes/integrations.ts: 0 eslint errors and 0 warnings on any touched line — intersected eslint's line numbers with the diff's + ranges rather than comparing totals; no fatal: true anywhere in the run.
  • Disclosed: the new test file carries the un-gated .js corpus's import/no-unresolved + import/extensions errors on its .ts requires, exactly like every other test file in that corpus. Fixing that corpus is its own task.

Gate: Vera.

… just createdBy (TASK-168)

`createdBy` records who made the pod and survives `leavePod`; `members` is what
leaving filters. Five owner routes gated on the field alone, so a creator who had
left could still read the pod's Discord channel (`GET /:id/messages`) and post
into it (`POST /:id/send`) after TASK-161/162 had closed every relay and chat
path to them. `canDeleteIntegration`'s pod-creator arm had the same shape, and it
is the write gate for six further call sites (ingest tokens, the connect code,
PATCH and DELETE).

Each pod-creator arm now also requires `isListedPodMember`. No new import: this
file already imports the strict rule at line 38, re-exported by
`services/connectorRelayPolicy` from `utils/isPodMember` precisely so connector
sites read one definition — so there is no second spelling to pick wrong here.
The admin and integration-creator arms are untouched: neither ever claimed pod
membership.

Witnessed per site: eight mutations, one per arm, each reddening exactly its own
arm. Nine arms, all real rows on memory Mongo; the two proxy routes assert the
Discord call did NOT happen rather than only the status.
@lilyshen0722 lilyshen0722 changed the title fix(authz): the connector owner routes require listed membership, not just createdBy (TASK-168) nothing Sep 27, 2026
@lilyshen0722 lilyshen0722 changed the title nothing fix(authz): the connector owner routes require listed membership, not just createdBy (TASK-168) Sep 27, 2026
@lilyshen0722
lilyshen0722 added this pull request to the merge queue Sep 27, 2026
Merged via the queue into main with commit ba81b22 Sep 27, 2026
26 of 29 checks passed
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