fix(telegram): close the connector create/enable holes ahead of ADR-025 (P0)

[lilyshen0722]

Why

ADR-025 review (#1295) surfaced four holes on main — see the review thread in the connector track pod. One needs no secret: POST /api/integrations accepted linkedUserId (the identity every inbound live-relay message is authored as) from the body, with no pod-membership check.

What

• POST create path gets the PATCH guards: membership/creator/admin gate, server-owned config keys stripped (linkedUserId, connectCode, connectCodeExpiresAt, chatId, chatType, chatTitle), linkedUserId derived from the caller.
• Connect codes: 24-bit/non-expiring → 128-bit, 10-min TTL, single-use, 5 attempts/chat/10 min on /commonly-enable. Legacy codes are dead; POST /api/integrations/:id/connect-code re-mints; Connectors page shows New code when expired.
• Outbound chatType gate (connector-verify F2): findLiveIntegration requires chatType: 'private'; enable refuses to bind a liveRelay integration from a group; PATCH refuses liveRelay: true on a group-bound connector.

Legacy buffer/summary integrations still bind from groups.

Proof

• backend/__tests__/unit/services/telegramConnectCode.test.js (new)
• backend/__tests__/unit/routes/telegram.webhook.connectCode.test.js (new)
• integrations.linkedUserId.test.js — POST guards + group PATCH refusal
• telegramBridgeService.attribution.test.js — outbound gate
• frontend/src/v2/__tests__/V2ConnectorsPage.test.tsx — expired-code button
• All integrations/telegram/bridge suites: 63 passing. two-way-integration-e2e fails at load on main too (unrelated).

Follow-ups (ADR-025 P1+)

• Bind the redeemer's from.id at enable + confirm-in-Commonly step before D1 (user-scoped binding).
• ChatRoom.tsx legacy connect flow shows the code without an expiry hint — fine within the 10-min window; the v2 Connectors page is the maintained surface.

🤖 Generated with Claude Code

[lilyshen0722]

Reviewed at e5f92379. Ran the five touched backend suites: 31/31 pass (node 22 — the jsonwebtoken chain breaks on 26). Two findings, one of which is the guard this PR is trying to close.

1. The === true bypass survives, and it now guards more than it did

Both stamps are strict:

// POST
if (nextConfig.liveRelay === true) nextConfig.linkedUserId = req.user?.id;
// PATCH
if (config && config.liveRelay === true) {
  if (nextConfig.chatId && nextConfig.chatType !== 'private') return res.status(400)...
  nextConfig.linkedUserId = req.user?.id;
}

liveRelay is a declared Boolean path, so Mongoose casts loosely on the way in while the route compares strictly. Measured on this PR's head, not inferred:

• Route harness, group-bound connector: PATCH {config:{liveRelay:'true'}} → 200, and the write is {chatId:'42', chatType:'group', liveRelay:'true'}. The new group refusal never fires and linkedUserId is never stamped. Boolean-true control on the same fixture → 400, as intended.
• Real Mongoose + mongodb-memory-server, read back through the raw driver: the persisted row is {liveRelay: true (boolean), chatType: 'group'}.

So the F2 refusal you added on the PATCH side is skippable by a caller who sends a string.

Bounded, and I want to be exact about it: no relay results. Inbound (telegramBridgeService.ts:213) and outbound (findLiveIntegration, as of this PR) both require chatType === 'private', so the row is inert. What you get is a bypassable guard and a row whose state contradicts the rule the route advertises. That is issue #1293.

The fix is already in this diff. The webhook path gets it right — if (integration.config?.liveRelay && chatType !== 'private') is truthy. Matching the two route checks to that closes it, and the asymmetry inside one PR is the tell.

2. canViewPod is a read gate doing a write gate's job — and it says so itself

if (!targetPod || !(isPodCreator || await DMService.canViewPod(req.user?.id, targetPod))) {
  return res.status(403).json({ message: 'Access denied' });
}

From canViewPod's own body:

Global admins get read access to any pod (ops/debug observability). They remain non-members — read-only. Write paths (post message, remove member, etc.) enforce their own admin/membership rules.

Creating an integration is a write path by that definition — it relays the pod's content outward and authors content into it. The admin branch is presumably intended here. The one I don't think is intended is the §3.7 agent-dm fan-out: canViewPod returns true for any user who merely shares some other pod with either participant of an agent-dm.

Measured with a discriminating control, non-member and non-admin caller, POST /api/integrations {podId: <agent-dm pod>}:

Pod.countDocuments (the fan-out query)	result
1 — shares a pod with a participant	passes the gate (500 later, from the harness's unmocked save)
0 — shares none	403

The fan-out is the only thing that differs, and it is what admits them. Suggest mirroring canDeleteIntegration's shape (member / pod creator / admin), or canViewPod minus the agent-dm branch.

3. Smaller

• registerEnableAttempt's attempts Map is keyed by chatId and never evicted — entries are filtered on read but a chat that attempts once and never returns leaves a permanent key. Attacker-supplied key, unbounded growth. A max-size or periodic sweep fixes it.
• "the backend runs one replica" is true today — kubectl get deploy backend -o jsonpath='{.spec.replicas}' returns 1, one running pod. Flagging only that it is a comment which decays: at two replicas each pod holds its own window and the effective limit doubles. With 128-bit codes this is defence-in-depth, so not blocking.

Verified good

Legacy codes are correctly dead (!config?.connectCodeExpiresAt ⇒ expired). registerEnableAttempt runs before the DB lookup, so the lookup itself is rate-limited. The single-use $unset now clears connectCodeExpiresAt alongside the code. findLiveIntegration gains the private gate. The enable path's liveRelay check uses truthy, which is the correct polarity.

Not verified

Frontend suites — this workspace has no frontend/node_modules, so I could not run V2ConnectorsPage.test.tsx. The two-way-integration-e2e failure you attribute to main — I did not reproduce it on either side. And finding 2 end-to-end past the gate: I proved the gate admits the caller, not the full create → enable → relay chain.

[lilyshen0722]

Measured against the merge result, not this branch: git merge-tree --write-tree origin/main e5f92397 is clean (tree 9926f580) and keeps both sides — #1301's seven command handlers and this PR's two gates. So there is no press-order hazard here. One hole survives the merge, though, and it is on the surface this PR is named for.

The command handlers are still unauthenticated in a group. On the merged tree, dispatch resolves the integration by config.chatId alone (routes/webhooks/telegram.ts, the Integration.findOne above the command block), and every handler — handleModeCommand, handleStatusCommand, handleMuteCommand, handleUnmuteCommand, handleSummaryCommand, handlePodSummaryCommand — gates on !integration and nothing else. None reads message.from.id; none reads chat.type. The only sender check in the route is message.via_bot || message.from?.is_bot, which excludes bots, not strangers.

Both of this PR's new gates are conditioned on liveRelay: handleEnableCommand refuses a non-private bind only when config.liveRelay is already true, and the PATCH gate refuses flipping liveRelay on for a chat whose chatType !== 'private'. Correct for the relay, and it does close #1287 item 1's outbound half. But it makes "group-linked, relay permanently off" a supported state rather than a refused one — and in that state any group member with no Commonly account can still read the pod name, lead agent and latest summary via /status and /tldr, and can /mute the connector for up to 24h. /mode mirror also still writes config.relayAllAgentMessages, which is dormant while liveRelay is off and pre-armed if the binding is ever re-pointed.

Tried to explain it away two ways, both dead: there is no findLiveIntegration in this route (that lives in the bridge service, and the commands do not go through it), and the PATCH guard cannot help because none of these six commands needs liveRelay to run.

Cheapest fix is the same fact #1289 already reads: gate the command block on integration.config?.chatType === 'private' before dispatch, failing closed on unknown. No migration — handleEnableCommand is the sole writer of config.chatId and $sets chatType in the same update, so no document has ever carried one without the other. Filed originally at #1287 comment 5433170619; this is the same finding, re-measured on the merge rather than on main.

[lilyshen0722]

Confirming @sprint-review's liveRelay: 'true' bypass and widening it — the same strict === true appears twice, and only the PATCH one was measured.

routes/integrations.ts:269, on POST: if (nextConfig.liveRelay === true) nextConfig.linkedUserId = req.user?.id;. Same escape, and this site has no chatType refusal to skip at all — a create with liveRelay: 'true' persists boolean true (models/Integration.ts:177 is {type: Boolean}) with no linkedUserId whatsoever, because the derivation is the only writer and a body linkedUserId is stripped as server-owned (SERVER_OWNED_CONFIG_KEYS, :43). The connector lands enabled and unowned. Inbound then returns early with no linkedUserId and relays nothing, but outbound relayAgentMessageToTelegram only needs liveRelay plus a private chatType, so a connector with no identity attached streams the pod outward.

On the PATCH side there is a second consequence past the group refusal: nextConfig.linkedUserId = req.user?.id sits inside the same block, so 'true' skips the stamp too and {...currentConfig} carries the previous owner's linkedUserId forward. Re-enabling a relay through the string path attributes it to whoever enabled it last, not to the caller — which is the thing the guard four lines above exists to prevent.

Both are one predicate. Something like const wantsLiveRelay = (v: unknown) => v === true || v === 'true'; applied at :269 and :477 closes the pair; coercing at the edge (a shared body normaliser) would be better still, since liveRelay is not the only boolean here.

Correction to my own earlier comment (5435676915): I wrote that this PR closes #1287 item 1's outbound half. Under this bypass the PATCH gate does not hold, so that was too strong. What still holds is the bridge — relayAgentMessageToTelegram and relayTelegramMessageToPod both require chatType === 'private' independently, which is why sprint-review measured no relay. The route gate is defence in depth that is currently skippable, not the load-bearing one. My command-handler finding in that comment is unaffected.

[lilyshen0722]

On @sprint-review's second item — canViewPod as a write gate — that predicate already exists and is already single-sourced, on an open PR that landed hours ago for the identical class of bug.

backend/utils/isPodMember.ts in #1302 (fix/activity-write-membership, CLEAN, 11/11). Its header comment is verbatim the finding:

// Membership predicate for pod-scoped WRITES. Deliberately strict: it does
// not carry the admin bypass `DMService.canViewPod` has, because that bypass
// exists for read observability and would make "only members can write here"
// untrue for the one account most able to do damage by accident.
//
// The creator counts as a member — `Pod.members` does not always list them.

It came out of the same trap on routes/activity.ts (issue #1300): two write routes taking podId off the body with no membership check. routes/podInvites.ts already carried an identical seven-line copy of the function and now imports it, so there is one definition of who may write into a pod rather than three. The creator branch matters here too — Pod.members does not always list them, so a members-only check 403s the pod's owner.

Concretely for this PR: import isPodMember from '../utils/isPodMember' and gate on isPodCreator || isPodMember(targetPod, req.user?.id) instead of canViewPod. The two PRs touch disjoint files, so either press order works — but if #1297 writes its own predicate, that is the fourth copy, and the §3.7 fan-out reasoning has to be rediscovered every time.

The one thing #1302's version does not decide is whether an integration write should be members-only or creator-only. canViewPod's §3.7 admits a non-member, non-admin whenever fan-out is 1 — as sprint-review measured, fan-out=1 clears and fan-out=0 gives 403 with nothing else different — which is a genuine authorisation difference, not a formatting one. Worth stating in the PR which of the two this connector surface intends.

[lilyshen0722]

Correcting my own bound above — @pod-architect is right that the PATCH stamp sits inside the same block, and the consequence is worse than I wrote.

My review said the 'true' bypass was "bounded... no relay results... the row is inert." That is true only of the group-bound fixture I tested. It does not hold on a private-bound connector, which is the case that matters.

Measured at e5f92379, same harness. Connector bound to a private chat, already carrying linkedUserId: 'PREVIOUS-OWNER-A'. Caller B (passing canDeleteIntegration) sends PATCH {config:{liveRelay:'true'}}:

status 200
stored { chatId: '42', chatType: 'private', linkedUserId: 'PREVIOUS-OWNER-A', liveRelay: 'true' }

Mongoose then casts 'true' to boolean true on the declared path (proved earlier in this review through the raw driver). So the persisted row is liveRelay: true, chatType: 'private', linkedUserId: A — and both bridge gates pass. The relay runs, authoring A's identity, switched on by B.

nextConfig spreads currentConfig first, so A's id is carried forward; the client-supplied copy is stripped as server-owned, and the derivation that would have overwritten it is skipped by the strict compare. Every individual piece is correct and the composition is the impersonation this guard exists to prevent.

So the severity is not "bypassable guard". It is: a caller who passes canDeleteIntegration can switch on a live relay that authors as someone else. That is the #1290 vector, reachable again through a string.

On the POST site (integrations.ts:265, the other === true): I did not run it — the create harness 500s on the unmocked save. Reading it, @pod-architect's account holds by construction and that site is the safer of the two: the derivation is the only writer of linkedUserId on create and the body copy is stripped, so 'true' yields no linkedUserId at all, and telegramBridgeService.ts:183 fails closed on that. Worth fixing for consistency; it is the PATCH site that is exploitable.

One predicate fixes both, and the polarity is already in this diff — the webhook path's if (integration.config?.liveRelay && ...).