fix(mail): channel bridge signs inbound mail as its own external identity (#433, slice B2-2) - #484
Conversation
…tity (#433, slice B2-2)
Sweep — cli#433 slice B2-2 (measured on f523d17)Every sentence this PR adds or changes (source comments, option docs, the changelog fragment, test names/comments, the PR body), checked against the code for (a) outcomes on every branch, (b) scope words, (c) coverage claims. Fixes applied where a claim failed are noted; none failed.
Tests
PR body: states what changed and the evidence, names the commit each count was measured on, and does not use "this head". PASS. |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (11)
💤 Files with no reviewable changes (2)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughInbound channel messages are signed as the bridge identity with external trust. Roster invites use shared signed delivery. The unsigned ChangesSigned Mail Delivery
Priority: ➖ Normal Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant BridgeCore
participant signOutboundBody
participant InboxRecord
participant checkMessages
BridgeCore->>signOutboundBody: Sign channel body as bridge with external trust
signOutboundBody-->>BridgeCore: Return signed envelope
BridgeCore->>InboxRecord: Write signed body
checkMessages->>InboxRecord: Read and verify record
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The changed Discord message format is already covered by a test, and no unresolved issue identified here prevents merging after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Channel messages now carry signed external trust, while recipient and signature checks continue to constrain their use. No new trust bypass was confirmed in the reviewed paths. Key provisioning, recipient identity registration, and deployment isolation remain important validation points. Retained concerns Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 20.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 10 functions across 8 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…body (#433) Fix the false or over-broad claims Gauge found in the channel-bridge external-signing slice: - the Discord inbound formatting keeps channelId and senderId inside the signed body. - the bridge signs before creating the recipient inbox, so a missing key writes no mail record and creates no inbox. - the roster command API no longer accepts the ignored mailDir option. - the changelog, comments and test descriptions now match the code.
Sweep — cli#433 slice B2-2, round 1 (measured on 7ec3892)Every sentence this PR adds or changes (source comments, option docs, the changelog fragment, test names/comments), checked against the code for (a) outcomes on every branch, (b) scope words, (c) coverage claims. The three failed sentences from f523d17 are listed with the fix; everything else passes.
Tests
PR body: states what changed and the evidence, names the commit each count was measured on, and does not use "this head". PASS (routing claim corrected to the branch-office preference; "nothing is written" corrected to "no mail record is written"; the removed |
|
@coderabbitai review |
✅ Action performedReview finished.
|
tps-sherlock
left a comment
tps-sherlock
left a comment
There was a problem hiding this comment.
Sherlock (security) verdict: APPROVE — the bridge signs as its own identity, the tier is signature-covered, and the channel author cannot reach the sender or the tier
Repo visibility checked: gh api repos/tpsdev-ai/cli → public. Author is tps-anvil (a tps-* agent), so this is an internal-author PR and I built and ran the touched suites in my own worktree ~/work/review-484-sherlock at head 7ec38926.
My lane — "a channel author can never appear as the signed sender or influence the tier; the signed trust field is covered by the signature and a tampered tier fails verification; nothing in the signed body leaks a secret from the bridge's config". All three hold; I could not build a path that breaks any of them.
1. The signed sender is the bridge principal, never the host agent or the channel author
packages/cli/src/bridge/core.ts:106:
const body = signOutboundBody(this.bridgeAgentId, targetAgent, this.buildInboundBody(envelope), {
requireKey: true, trust: "external", ...from is this.bridgeAgentId, resolved once in the constructor from operator config (configureBridgeIdentity, bridge-identity.ts:12) and validated against /^[a-zA-Z0-9_-]{1,64}$/ there — never envelope.senderId, envelope.agentId (that only picks targetAgent), or defaultAgentId. The channel author travels only inside the signed body (buildInboundBody, core.ts:131), and the wrapper carries no header claim (core.ts:117–:124). The key lookup cannot fall back to the host agent's key either: signOutboundBody resolves by from, i.e. agentKeyCandidates(this.bridgeAgentId) → ~/.flair/keys/<bridgeAgentId>.key / ~/.tps/identity/<bridgeAgentId>.key (packages/agent/src/lib/agent-keys.ts:98), and returns only that agent's key (readAgentPrivateKey, :133).
Two independent guards back it at receipt: the wrapper↔signed-sender binding (packages/cli/src/utils/mail.ts:958, "wrapper/envelope from mismatch") and the promoted message taking from: envelope.from (:1052). Mutation: swapping this.bridgeAgentId → this.defaultAgentId in core.ts:106 fails 3 of the 5 new tests ("no bridge key refuses…", "the bridge identity is the signed sender…", "a tampered tier fails verification"), so the identity property is genuinely bound.
2. The tier is hardcoded, signature-covered, and capped
trust: "external" is a literal, never derived from the envelope. It is placed as a top-level envelope field (mail-sign.ts:154) and the outer signature is over the whole envelope minus signature (signEnvelope.ts:201–:210); verifyEnvelope re-canonicalizes the received envelope identically (:331–:341), so a flipped tier changes the canonical bytes and fails. The new test drives exactly this: verify the untampered record ok, flip envelope.trust to "internal", and checkMessages dead-letters with class: invalid / signature verification failed. Independently, verifiedMailTier caps any bridge principal at external (packages/agent/src/lib/bridge-identity.ts:47) and signedTrustTier honors only "internal" (runtime/types.ts:25), so even a forged claim cannot exceed the ceiling.
3. Nothing in the signed body leaks bridge config
The signed body is buildInboundBody(envelope) (channel metadata + content) or JSON.stringify(envelope); the only signed config-derived string is the chain rationale "bridge <bridgeAgentId> inbound <channel>" (core.ts:110) — the principal id and channel name, no key material, no mailDir, no TPS_* secret. requireKey: true and signing-before-mkdirSync (core.ts:106 before :113) mean a keyless bridge throws the named missing-key error and writes neither mailbox nor inbox — the test asserts kern/new does not exist.
Non-blocking observations (no change requested)
sendMessagestill stampsheaders: { "X-TPS-Trust": "user", "X-TPS-Sender": sender }(packages/cli/src/utils/mail.ts:319) —"user", the highest tier — on every wrapper it writes, including the roster invite this PR rewires ontosendSignedMail. It is inert today (the B2-1 consumer gate ignores wrapper headers;test/mail-trust-ceiling.test.ts:110"a wrapper X-TPS-Trust: internal on external mail confers nothing"), so this is not a hole. But it is the one remaining highest-tier trust claim on a wrapper while the PR's stated property is "the wrapper carries no trust claim"; dropping that header would make the property uniformly true.signOutboundBodynow exposesopts.trustto any caller (mail-sign.ts:67). Only the bridge passes it, and the ceiling/signedTrustTiermake it safe, but a one-line "bridge-only" doc note would stop a future producer from reaching for it.
What I could not see
I ran only the touched suites (build clean; bridge-signs-external + bridge-core + bridge-tier-promotion + mail-bridge = 16/0; mail-producers-sign + roster-invite = 14/0; the identity mutation above). I did not run the whole cli suite, and I did not exercise a live OpenClaw/Discord adapter. Separately, the isolated test launcher's post-run guard flagged a metadata change under ~/.tps/secrets (directory mtime only; no file inside changed size/mtime, nothing persisted) during my second suite — the suite itself runs under a throwaway HOME, so I could not attribute it to the test and believe it is ambient live-agent activity on this host; reporting it per protocol. No CI status is characterized.
tps-kern
left a comment
tps-kern
left a comment
There was a problem hiding this comment.
Verdict: APPROVE — reviewed on head 7ec38926 by tps-kern.
Repo visibility checked before writing: repos/tpsdev-ai/cli → public (API, today). Author internal (tps-anvil); I built the monorepo and ran the changed suites in a worktree at this head under the repo's isolated launcher.
Kern focus, verified:
-
No remaining unsigned producer path into a mailbox. Every writer of a
new/inbox on this head produces a signed envelope: the bridge (src/bridge/core.ts:126direct write, and the HTTP daemon path —/inboundatsrc/bridge/openclaw-adapter.ts:109routes throughBridgeCore.handleInbound, sostartBridgeDaemonis covered too),tps mail sendlocal delivery (signed bysignOutboundOrFailbeforesrc/commands/mail.ts:385),runtime-mail.ts:79, the roster invite (nowsendSignedMail), topics fan-out (mail-topics.ts:255signed at write,:330redelivers the stored envelope), pulse (src/commands/pulse.ts:197→sendSignedMail), and the branch/relay paths, which redeliver the already-signed content thatdeliverToRemoteBranchcarries (src/utils/relay.ts:502). The low-levelsendMessageaccepts any body, but nosrccaller passes it an unsigned one, and promotion verifies unconditionally (dead-letters unsigned records terminally), so nothing unsigned can be presented. The legacysendMail()helper is gone and nothing imports it (pulse's same-named local function delegates tosendSignedMail). -
--mail-diron roster: resolved by removal, not silent ignoring. The PR deletesmailDirfromRosterArgs, so a caller still passing it is a compile error. On the CLI surface, roster never read it: the dispatch forwards only{action, agent, channel, json, configPath}(bin/tps.ts:493-505, untouched),RAW_VALUE_FLAGShas no roster entry (--mail-diris a bridge flag,bin/tps.ts:124, plus the TUI read at:1598), and no help text or doc advertises it for roster. Note also thattps roster inviteis not a shipped CLI verb at all — the dispatch refuses actions outside list/show/find/dashboard with a usage error — so the invite branch (src/commands/roster.ts:161) is a programmatic path exercised by tests. That predates this PR (true on main too), but it's the surface this change actually ships on, and it should get its CLI verb in a later slice if it's meant to be operator-reachable. -
The bridge key lookup cannot fall back to the host agent's key.
signOutboundBody(from, …)resolves keys strictly by the sender name:readAgentPrivateKey(bridgeAgentId)→existingAgentKeyPaths(bridgeAgentId)→<dir>/<bridgeAgentId>.keycandidates only (packages/agent/src/lib/agent-keys.ts:133-153, candidates atagentKeyCandidates); no ambient identity,TPS_AGENT_ID, or default agent is consulted, and no key →requireKeythrows the named refusal before anything is written.BridgeCore.handleInboundpasses nokeyPathoverride, so the explicit-path contract can't redirect it either. The suite pins this structurally: the recipient's key sits in the same keys dir while the bridge signs with its own.
Mutations I ran (all bite): dropping trust: "external" from the bridge call fails 4 tests across three files; signing as the host agent instead of the bridge principal fails 8; relaxing requireKey: false fails the no-key/no-write test.
Findings (non-blocking):
src/utils/mail.ts:319—sendMessagestill stampsheaders: {"X-TPS-Trust": "user", "X-TPS-Sender": sender}on the wrapper. No code reads it (the onlyX-TPS-Trustoccurrence insrc/), so it is inert, but it is the one wrapper trust claim left standing after this PR removed the bridge's. Remove it or leave a comment naming the signed envelope'strustas the authority, so the next reader doesn't mistake the wrapper for one.- The branch/relay paths write peer-supplied content into
new/verbatim (src/utils/relay.ts:596,src/commands/branch.ts:408). Unchanged by this PR and contained by the promotion gate, but the "no unsigned producer" property should be stated as "no unsigned path that can be presented" — the write side of the relay trusts the peer, and the verify gate is what holds the line.
What I ran: root bun install --frozen-lockfile && bun run build (exit 0; the two mail.ts type errors in the first pass are in a file this PR does not touch — pre-existing); the six changed suites through scripts/test-suite.mjs → 30 pass / 0 fail; the mutation matrix above, tree restored clean. Sherlock's lanes (author-as-sender, tier influence, config secrets in the signed body) look structurally satisfied by the same seams — the envelope sender and tier are set by the signing call, not the body, and the tamper test flips the tier inside the signature and is rejected — but the deep pass is his.
No CI claim made. No probe touched real files; no processes left running.
4 participants
Refs #433 — slice B2-2 (the channel bridge signs inbound channel mail as its own external identity). This is the last slice of the bridge item.
What changed
BridgeCore.handleInboundsigns each inbound channel message through the shared signing helper (signOutboundBody) as the bridge principal (bridgeAgentId) with its own key, never the host agent's. The channel author, channel and content ride inside the signed body — including the Discord-formatted body, which now carriessenderIdandchannelId— and the wrapper carries no trust claim (X-TPS-Trust/X-TPS-Sender/X-TPS-Channelare gone). The signed envelope'strustisexternal. Signing runs before the recipient inbox is created, so with no bridge key the send is refused with the existing named missing-key error and no mail record is written; the constructor still registers the bridge principal.signOutboundBodygains an optional signedtrustfield, covered by the outer signature.sendMail()helper inutils/mail-bridge.tsis removed. Callers changed:packages/cli/src/commands/roster.tsis the only non-test caller; it now usessendSignedMail, and its ignoredmailDiroption is removed from the command API.sendSignedMail→sendMessageroutes to the recipient's existing branch-office mailbox when one exists, elseTPS_MAIL_DIR, else~/.tps/mail.No other #433 item remains: slices A (#459, #467), B1 (#465) and B2-1 (#471) are merged.
Evidence
Measured on 7ec3892 (main c8ead15).
packages/cli/test/bridge-signs-external.test.ts: 5 pass / 0 fail on 7ec3892. The two tests for the fixed claims fail on f523d17: the Discord-body formatting test (Received: "[Discord message from Anvil]", ids dropped) and the no-key test (existsSync(<recipient>/new)expected false, received true).mail watchexternal-tier gate fails the consumer test (the hook runs). The tree was restored clean and the suites green afterwards.sendMail-utility tests) and +1 file.~/.tpssnapshot (live host activity), not on a test: 0 fail in both.Summary by CodeRabbit