From b5bb19c59c51108bc2aee06f02e5a751555d7938 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Alejandro=20Guti=C3=A9rrez?= <35082514+alezmad@users.noreply.github.com> Date: Sat, 26 Sep 2026 20:54:23 +0100 Subject: [PATCH] fix(cli): never inject undecryptable ciphertext into a session (bug2) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A sealed DM (nonce + senderPubkey) that no key opened fell through to a base64 "plaintext" fallback. Buffer#toString never throws — invalid bytes become U+FFFD — so the raw ciphertext reached Claude as «21e425db: ��a.�tH�/…» (flexicar report 2026-09-26). - daemon/inbound: the base64 fallback only applies to unsealed v1 broadcasts, decoded with a fatal TextDecoder + renderable-text check. An undecryptable topic post renders an honest placeholder pointing at `topic tail`; anything else is dropped: logged `inbound_decrypt_failed`, acked (stops redelivery), no inbox row, no bus event. - mcp/server: second guard — a body with U+FFFD or C0 controls is never emitted as a channel notification. - utils/text: isRenderableText + decodeBase64Utf8. Test fails 3/6 on the old inbound.ts, passes on the new one. Spec: .artifacts/specs/2026-09-26-session-identity-and-routing.md §1 Co-Authored-By: Claude Opus 5.5 --- ...2026-09-26-session-identity-and-routing.md | 76 ++++++++++++ apps/cli/src/daemon/inbound.ts | 30 ++++- apps/cli/src/mcp/server.ts | 10 +- apps/cli/src/utils/index.ts | 1 + apps/cli/src/utils/text.ts | 19 +++ .../tests/unit/inbound-undecryptable.test.ts | 109 ++++++++++++++++++ 6 files changed, 238 insertions(+), 7 deletions(-) create mode 100644 .artifacts/specs/2026-09-26-session-identity-and-routing.md create mode 100644 apps/cli/src/utils/text.ts create mode 100644 apps/cli/tests/unit/inbound-undecryptable.test.ts diff --git a/.artifacts/specs/2026-09-26-session-identity-and-routing.md b/.artifacts/specs/2026-09-26-session-identity-and-routing.md new file mode 100644 index 0000000..67d49d1 --- /dev/null +++ b/.artifacts/specs/2026-09-26-session-identity-and-routing.md @@ -0,0 +1,76 @@ +# Session identity & routing hardening (CLI 1.38.0) + +Source: flexicar-orchestrator bug report 2026-09-26 (9 bugs, CLI 1.37.0) + user report +"peers spread messages to the wrong peers, creates noise in context". Investigated at `59610b5`. + +## Root thread +The member key is used where a session key belongs, at three points that feed each other: +1. a session that lost its daemon registration silently signs sends with the **member** key; +2. the channel notification exposes `from_pubkey` = member key, so the receiver replies to the member; +3. the broker delivers a member-key DM to *any/every* connection of that member. +Result: replies fan out to sibling sessions → noise. Fix all three; each alone is insufficient. + +## Decisions (per bug, ordered by blast radius) + +### §1 bug2 — never inject undecryptable bytes +- `daemon/inbound.ts`: base64 "plaintext" fallback only when the push is **not** a sealed DM + (no `nonce`+`senderPubkey`), decoded with `TextDecoder("utf-8",{fatal:true})`. +- A sealed payload that fails every key → body `null`. If `topic` is known → render placeholder + `[encrypted # message — claudemesh topic tail ]`; otherwise **drop**: log + `inbound_decrypt_failed {broker_message_id, sender}`, ack (stop redelivery), no inbox row, no bus event. +- MCP (`mcp/server.ts`) second guard: never emit a channel body containing U+FFFD or C0 controls + (except \t\n\r). +- Broker drained push (`index.ts maybePushQueuedMessages`) carries `senderName`, + `senderMemberPubkey`, `topic`, `subtype` like the live push already does. +- Deferred (own feature, not effort): daemon-side v2 topic decrypt needs a REST key mint per topic + in the daemon hot path; placeholder is honest until then. + +### §2 bug1 — no silent member-key identity +- `ipc/server.ts`: a request carrying a session header that doesn't resolve → **401 session_unknown** + (not a silent `session=null`). +- `send`: when the process is inside a launched session (`CLAUDEMESH_SESSION_ID` set) but the + daemon doesn't know the session → error with fix hint, unless `--as-member`. +- Re-attach: `POST /v1/sessions/reattach {session_id, mesh, display_name, pid}` — daemon loads the + persisted keypair (never mints), signs the attestation (same as boot reload), registers with the + caller's pid. Called by the MCP server at startup when its token is missing/unknown. +- Token lives in a stable per-session path (`~/.claudemesh/sessions//.token`), not the + launch tmpdir, so a resumed claude finds it. +- The 24 h absolute cap is removed for entries whose pid+start-time still match. +- Tokenless MCP subscribes to **no** message events (today: all of them). + +### §3 wrong-peer delivery +- Broker: a DM whose target is a member pubkey of a member with ≥1 live session is rejected + `member_target_requires_fanout` unless the envelope sets `fanout:true`. Rollout: log-only via + env `CLAUDEMESH_MEMBER_DM_POLICY=warn` (default), `reject` after one release. +- CLI `send`: refuses a member-key target (other member) unless `--fanout`. +- Channel meta: `from_pubkey`/`from_id` = sender **session** pubkey; new `from_member_pubkey`. + Skill text updated: reply to `from_pubkey`. +- Name / prefix resolution (daemon + cold path): >1 match → error listing candidates; control-plane + rows and member keys excluded. +- `--self`: requires a registered session; scoped to the current mesh. +- Inbox dedupe key = (client_message_id, recipient_pubkey). + +### §4 bug3 — keys don't change behind a session's back +- `launch`: mint the UUID first; the same id drives keypair + registration; no throwaway keypair + when a session id exists (non-UUID ids persisted under sha256). +- Boot reload: load-only; a missing key file → skip + log, never mint. +- Supervisor: a session client that ends `replaced`/closed while its registry entry is alive is rebuilt. + +### §5 bug4 — shipped in `dd351a7` (1.37.1, unpublished) — releases with 1.38.0. + +### §6 the rest +- bug5 `topic history` decrypts via the same topic-key path as `tail` (shared `decryptForRender`). +- bug6 author label: history/tail return the session pubkey + a `sender_display_name` captured at write. +- bug7 non-TTY confirm → exit non-zero with `--yes` hint; `[-y]` in kick/ban/disconnect help. +- bug8 `message status` matches broker id OR `client_message_id` (hyphens ignored). +- bug9 `send` to a control-plane row says so (not "ephemeral"); `peer list --all` tags control-plane. + +## Smoke tests +| case | expected | +|---|---| +| push with sealed DM + wrong key | no inbox row, no channel event, `inbound_decrypt_failed` logged, acked | +| broadcast base64 of invalid UTF-8 | dropped, not rendered | +| resumed session (`claude --resume`) | same session pubkey, sends signed by session key | +| DM to a member pubkey (policy reject) | broker error `member_target_requires_fanout` | +| reply via channel `from_pubkey` | lands in exactly one session | +| `message status <8 chars printed by send>` | resolves | diff --git a/apps/cli/src/daemon/inbound.ts b/apps/cli/src/daemon/inbound.ts index 65ed261..424ad2c 100644 --- a/apps/cli/src/daemon/inbound.ts +++ b/apps/cli/src/daemon/inbound.ts @@ -8,6 +8,7 @@ import type { SqliteDb } from "./db/sqlite.js"; import { insertIfNew } from "./db/inbox.js"; import type { EventBus } from "./events.js"; import { decryptDirect } from "~/services/crypto/facade.js"; +import { decodeBase64Utf8 } from "~/utils/text.js"; export interface InboundContext { db: SqliteDb; @@ -96,9 +97,26 @@ export async function handleBrokerPush(msg: Record, ctx: Inboun const subtype = stringOrNull(msg.subtype); // Forward-compat: Sprint 7 brokers will send client_message_id alongside. const clientMessageId = stringOrNull(msg.client_message_id) ?? brokerMessageId ?? randomUUID(); - const body = await decryptOrFallback({ + const decrypted = await decryptOrFallback({ ciphertext, nonce, senderPubkey, ctx, }); + // 1.38.0 (spec 2026-09-26 §1): an undecryptable payload is never + // rendered. Topic posts we can't open yet get an honest placeholder; + // anything else is dropped (acked so the broker stops redelivering). + let body: string | null = decrypted; + if (decrypted === null && ciphertext) { + if (topic) { + body = `[encrypted #${topic} message — read it with: claudemesh topic tail ${topic}]`; + } else { + ctx.log?.("warn", "inbound_decrypt_failed", { + broker_message_id: brokerMessageId, + sender: senderPubkey.slice(0, 16), + has_nonce: Boolean(nonce), + }); + ctx.ackClientMessage?.(clientMessageId, brokerMessageId); + return; + } + } const id = randomUUID(); const inserted = insertIfNew(ctx.db, { @@ -178,11 +196,11 @@ async function decryptOrFallback(args: { } } - // Fallback: broadcast/topic posts are base64 plaintext (existing CLI - // pre-encryption convention for `*` and `@topic`). Sprint 7+ adds per- - // topic symmetric keys. - try { return Buffer.from(ciphertext, "base64").toString("utf8"); } - catch (e) { ctx.log?.("warn", "inbound_b64_decode_failed", { err: String(e) }); return null; } + // A sealed payload (nonce + sender) that no key opened is NOT base64 + // plaintext — decoding it produced the raw-binary injection of the + // 2026-09-26 report. Only unsealed v1 broadcasts take the fallback. + if (nonce && senderPubkey) return null; + return decodeBase64Utf8(ciphertext); } function stringOrNull(v: unknown): string | null { diff --git a/apps/cli/src/mcp/server.ts b/apps/cli/src/mcp/server.ts index 13aac6f..c21ddbe 100644 --- a/apps/cli/src/mcp/server.ts +++ b/apps/cli/src/mcp/server.ts @@ -36,6 +36,7 @@ import { join } from "node:path"; import { DAEMON_PATHS } from "~/daemon/paths.js"; import { VERSION } from "~/constants/urls.js"; +import { isRenderableText } from "~/utils/text.js"; import { readConfig } from "~/services/config/facade.js"; import { BrokerClient } from "~/services/broker/facade.js"; @@ -439,7 +440,14 @@ export async function startMcpServer(): Promise { const d = ev.data; const fromName = String(d.sender_name ?? "unknown"); const fromMember = String(d.sender_member_pubkey ?? d.sender_pubkey ?? ""); - const body = String(d.body ?? "(decrypt failed)"); + const rawBody = typeof d.body === "string" ? d.body : null; + // 1.38.0 (spec 2026-09-26 §1): second guard — never hand Claude + // binary garbage, whatever the daemon's inbox holds. + if (rawBody === null || !isRenderableText(rawBody)) { + mcpLog("channel_dropped_unrenderable", { mesh: String(d.mesh ?? ""), id: String(d.id ?? "") }); + return; + } + const body = rawBody; const priority = String(d.priority ?? "next"); const prioBadge = priority === "now" ? "[URGENT] " : priority === "low" ? "[low] " : ""; const topicTag = d.topic ? ` (#${d.topic})` : ""; diff --git a/apps/cli/src/utils/index.ts b/apps/cli/src/utils/index.ts index 1433c27..66f660c 100644 --- a/apps/cli/src/utils/index.ts +++ b/apps/cli/src/utils/index.ts @@ -4,3 +4,4 @@ export { isInviteUrl, extractInviteCode } from "./url.js"; export { formatBytes, formatDuration } from "./format.js"; export { isNewer } from "./semver.js"; export { retry } from "./retry.js"; +export { isRenderableText, decodeBase64Utf8 } from "./text.js"; diff --git a/apps/cli/src/utils/text.ts b/apps/cli/src/utils/text.ts new file mode 100644 index 0000000..1a66ee3 --- /dev/null +++ b/apps/cli/src/utils/text.ts @@ -0,0 +1,19 @@ +/** No U+FFFD replacement chars, no C0/DEL controls other than tab, newline, CR. + * Used to refuse rendering undecryptable payloads as text (spec 2026-09-26 §1). */ +export function isRenderableText(text: string): boolean { + // eslint-disable-next-line no-control-regex + return !/[\uFFFD\u0000-\u0008\u000B\u000C\u000E-\u001F\u007F]/.test(text); +} + +/** Strict base64 → UTF-8. Buffer#toString never throws (invalid bytes + * become U+FFFD), so validate with a fatal TextDecoder instead. */ +export function decodeBase64Utf8(b64: string): string | null { + try { + const bytes = Buffer.from(b64, "base64"); + if (bytes.length === 0) return null; + const text = new TextDecoder("utf-8", { fatal: true }).decode(bytes); + return isRenderableText(text) ? text : null; + } catch { + return null; + } +} diff --git a/apps/cli/tests/unit/inbound-undecryptable.test.ts b/apps/cli/tests/unit/inbound-undecryptable.test.ts new file mode 100644 index 0000000..aa91289 --- /dev/null +++ b/apps/cli/tests/unit/inbound-undecryptable.test.ts @@ -0,0 +1,109 @@ +/** + * Spec 2026-09-26 §1 (bug2): an undecryptable push must never reach a + * Claude session as text. Before 1.38.0 a sealed DM that no key opened + * was base64-decoded and rendered as raw binary («21e425db: ��a.�tH�/…»). + */ + +import { afterEach, beforeEach, describe, expect, it } from "vitest"; +import { mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { randomBytes } from "node:crypto"; + +import { openSqlite, type SqliteDb } from "~/daemon/db/sqlite.js"; +import { migrateInbox } from "~/daemon/db/inbox.js"; +import { EventBus } from "~/daemon/events.js"; +import { handleBrokerPush, type InboundContext } from "~/daemon/inbound.js"; +import { encryptDirect, generateKeypair } from "~/services/crypto/facade.js"; +import { decodeBase64Utf8, isRenderableText } from "~/utils/text.js"; + +describe("inbound: undecryptable payloads", () => { + let dir: string; + let db: SqliteDb; + let events: Array>; + let logs: string[]; + let acks: string[]; + let ctx: InboundContext; + + beforeEach(async () => { + dir = mkdtempSync(join(tmpdir(), "cmh-inbound-")); + db = await openSqlite(join(dir, "inbox.db")); + migrateInbox(db); + events = []; + logs = []; + acks = []; + const bus = new EventBus(); + bus.subscribe((e) => { if (e.kind === "message") events.push(e.data); }); + const me = await generateKeypair(); + ctx = { + db, bus, meshSlug: "test", + recipientSecretKeyHex: me.secretKey, + ackClientMessage: (cid) => acks.push(cid), + log: (_l, msg) => logs.push(msg), + }; + }); + + afterEach(() => { + try { db.close(); } catch { /* ignore */ } + rmSync(dir, { recursive: true, force: true }); + }); + + it("drops a sealed DM no key opens: no event, no row, logged, acked", async () => { + const sender = await generateKeypair(); + const stranger = await generateKeypair(); + // Sealed for somebody else — our key can't open it. + const env = await encryptDirect("secret", stranger.publicKey, sender.secretKey); + await handleBrokerPush({ + type: "push", messageId: "m1", senderPubkey: sender.publicKey, + nonce: env.nonce, ciphertext: env.ciphertext, + }, ctx); + expect(events).toHaveLength(0); + expect(db.prepare("SELECT COUNT(*) AS n FROM inbox").get()).toEqual({ n: 0 }); + expect(logs).toContain("inbound_decrypt_failed"); + expect(acks).toEqual(["m1"]); + }); + + it("renders a placeholder for a topic post it can't open", async () => { + const sender = await generateKeypair(); + await handleBrokerPush({ + type: "push", messageId: "m2", senderPubkey: sender.publicKey, topic: "compras-orq", + nonce: Buffer.from(randomBytes(24)).toString("base64"), + ciphertext: Buffer.from(randomBytes(64)).toString("base64"), + }, ctx); + expect(events).toHaveLength(1); + expect(String(events[0]!.body)).toContain("claudemesh topic tail compras-orq"); + }); + + it("still decrypts a DM sealed for us", async () => { + const sender = await generateKeypair(); + const me = await generateKeypair(); + ctx.recipientSecretKeyHex = me.secretKey; + const env = await encryptDirect("hola", me.publicKey, sender.secretKey); + await handleBrokerPush({ + type: "push", messageId: "m3", senderPubkey: sender.publicKey, + nonce: env.nonce, ciphertext: env.ciphertext, + }, ctx); + expect(events.map((e) => e.body)).toEqual(["hola"]); + }); + + it("keeps the v1 base64 broadcast path for valid text", async () => { + await handleBrokerPush({ + type: "push", messageId: "m4", ciphertext: Buffer.from("hello mesh").toString("base64"), + }, ctx); + expect(events.map((e) => e.body)).toEqual(["hello mesh"]); + }); + + it("drops a v1 broadcast whose bytes aren't text", async () => { + await handleBrokerPush({ + type: "push", messageId: "m5", ciphertext: Buffer.from([0xff, 0xfe, 0x00, 0x9c]).toString("base64"), + }, ctx); + expect(events).toHaveLength(0); + }); + + it("text helpers", () => { + expect(isRenderableText("línea\ncon\ttab")).toBe(true); + expect(isRenderableText("bad �")).toBe(false); + expect(isRenderableText("bell \u0007")).toBe(false); + expect(decodeBase64Utf8(Buffer.from([0xc3, 0x28]).toString("base64"))).toBeNull(); + }); +});