diff --git a/clients/claude-code/hooks-manifest.test.ts b/clients/claude-code/hooks-manifest.test.ts index ccfd326..037dc48 100644 --- a/clients/claude-code/hooks-manifest.test.ts +++ b/clients/claude-code/hooks-manifest.test.ts @@ -72,6 +72,7 @@ const BOUND_HTTP_CALLERS = [ 'subscriptions', 'unreact', 'unsubscribe', + 'uploadFile', ] as const /** @@ -95,6 +96,37 @@ const BOUND_HTTP_CALLERS = [ */ const BOUND_INBOX_VERBS = ['subscribe', 'subscriptions', 'unsubscribe'] as const +/** + * Adapter ATTACHMENT verbs that reach `boundHttp` (comms-qpup). An upload + * writes an `Attachment` row with an owner, and Zulip grants a reader access to + * it only when that owner is also the sender of the referencing message — so + * the upload goes out under the seat's own bot, like every other write. + * + * Held apart from the two sets above for the same reason they are held apart + * from each other: the tool layer reaches these through a THIRD receiver. It + * never names `adapter.uploadFile` — it receives a closure as a dep — which is + * why the trace needs {@link ATTACHMENT_DEP_ADAPTER_MEMBER} to see this path at + * all. + */ +const BOUND_ATTACHMENT_VERBS = ['uploadFile'] as const + +/** + * Which adapter member backs each attachment dep in `RegisterToolsDeps`, as + * `server.ts` wires them. + * + * PINNED, NOT PARSED, and the limit of that is worth stating: nothing here + * reads `server.ts`, so a rewire of `upload` onto a different member would + * leave this map lying. Every other pin in this file carries the same limit + * (see `BOUND_VERBS`), and resolving a closure across a second source file is a + * different kind of test than this one. What the map buys is the thing that was + * missing: a tool whose only route to `boundHttp` is a dep now sits INSIDE the + * compared set instead of outside it. + */ +const ATTACHMENT_DEP_ADAPTER_MEMBER: Readonly> = { + upload: 'uploadFile', + downloadFile: 'downloadFile', +} + /** * Tools that reach `boundHttp` through an inbox verb while sitting outside the * matcher, so the hook never stamps them and the bind seam sees no session id. @@ -126,9 +158,10 @@ const G5ZH3_MATCHER_PENDING = [] as const const TWW6_EXCEPTIONS = ['resolve_thread', 'set_channel_description', 'unresolve_thread'] as const /** - * Every tool that accepts a host-supplied `session_id`. SEVEN, which is wider - * than the five in the PreToolUse matcher: `subscribe` and `unsubscribe` are - * here for a non-CC ephemeral host that supplies the UUID itself, and a + * Every tool that accepts a host-supplied `session_id`. EIGHT, the same eight + * the PreToolUse matcher stamps today — but the two sets answer different + * questions and are allowed to diverge again: `subscribe` and `unsubscribe` + * are here for a non-CC ephemeral host that supplies the UUID itself, and a * listen-first seat reaches an identity through no other tool. Do not read a * statement about one of these sets as a statement about the other. * @@ -143,6 +176,7 @@ const SESSION_ID_RECEIVING_TOOLS = [ 'subscribe', 'unreact', 'unsubscribe', + 'upload_file', ] as const /** Enclosing declaration names in the adapter source that call `boundHttp()`. */ @@ -170,15 +204,35 @@ function adapterVerbsReachingBoundHttp(source: string): ReadonlySet { interface ToolFacts { readonly verbs: ReadonlySet readonly inboxVerbs: ReadonlySet + readonly attachmentVerbs: ReadonlySet readonly receivesSessionId: boolean readonly advertisesSessionId: boolean } /** - * Per-tool: which publisher verbs and which inbox verbs its handler calls, - * whether it accepts a host-supplied `session_id`, and whether it advertises - * one on its `inputSchema`. The last is traced only so the assertions can show - * it is nowhere — the two facts are separate and stay separately measured. + * `const = deps.` bindings in the tools source — the hop that + * hides an attachment tool's adapter path from a line-wise scan. `upload_file`'s + * handler calls `upload(path)`; only this binding says `upload` is + * `deps.upload`, and only {@link ATTACHMENT_DEP_ADAPTER_MEMBER} says + * `deps.upload` is `adapter.uploadFile`. + */ +function depAliases(source: string): ReadonlyMap { + const aliases = new Map() + for (const line of source.split('\n')) { + const m = line.match(/^ {4}const ([a-zA-Z]+) = deps\.([a-zA-Z]+)$/) + const alias = m?.[1] + const field = m?.[2] + if (alias !== undefined && field !== undefined) aliases.set(alias, field) + } + return aliases +} + +/** + * Per-tool: which publisher verbs, inbox verbs and attachment verbs its handler + * reaches, whether it accepts a host-supplied `session_id`, and whether it + * advertises one on its `inputSchema`. The last is traced only so the + * assertions can show it is nowhere — the two facts are separate and stay + * separately measured. */ function toolFactsFromToolsSource(source: string): ReadonlyMap { const facts = new Map< @@ -186,10 +240,12 @@ function toolFactsFromToolsSource(source: string): ReadonlyMap inboxVerbs: Set + attachmentVerbs: Set receivesSessionId: boolean advertisesSessionId: boolean } >() + const aliases = depAliases(source) let current: string | undefined for (const line of source.split('\n')) { const named = line.match(/^ {6}name: '([a-z_]+)',$/)?.[1] @@ -198,6 +254,7 @@ function toolFactsFromToolsSource(source: string): ReadonlyMap [name, { ...e, verbs: e.verbs, inboxVerbs: e.inboxVerbs }]), + [...facts].map(([name, e]) => [ + name, + { ...e, verbs: e.verbs, inboxVerbs: e.inboxVerbs, attachmentVerbs: e.attachmentVerbs }, + ]), ) } @@ -266,7 +339,7 @@ test('the set of adapter declarations reaching boundHttp is the pinned one', asy // them reads "no tool violates this", which a scan that matches nothing // satisfies trivially — so assert first that the scan finds the set it is // supposed to find. -test('the tools accepting a host-supplied session_id are exactly the pinned seven', async () => { +test('the tools accepting a host-supplied session_id are exactly the pinned eight', async () => { const facts = toolFactsFromToolsSource(await toolsSource()) const receiving = [...facts] .filter(([, f]) => f.receivesSessionId) @@ -312,6 +385,55 @@ test('every tool whose adapter path reaches boundHttp is in the PreToolUse match expect(missing).toEqual([]) }) +// The same rule again, over the ATTACHMENT verbs that began binding with +// comms-qpup. Its own assertion with its own named set, for the reason the +// inbox rule has one: a rule that compares over a set which no longer covers +// every binding path goes green by not looking. +test('every tool whose adapter path reaches boundHttp via an attachment dep receives session_id', async () => { + const facts = toolFactsFromToolsSource(await toolsSource()) + const offenders = [...facts] + .filter( + ([, f]) => + [...f.attachmentVerbs].some((v) => + (BOUND_ATTACHMENT_VERBS as ReadonlyArray).includes(v), + ) && !f.receivesSessionId, + ) + .map(([name]) => name) + .sort() + expect(offenders).toEqual([]) +}) + +test('every tool whose adapter path reaches boundHttp via an attachment dep is in the matcher', async () => { + const facts = toolFactsFromToolsSource(await toolsSource()) + const matched = alternationToolsFromMatcher(injectSessionIdMatcher(hooksManifest)) + const missing = [...facts] + .filter(([, f]) => + [...f.attachmentVerbs].some((v) => + (BOUND_ATTACHMENT_VERBS as ReadonlyArray).includes(v), + ), + ) + .map(([name]) => name) + .filter((name) => !matched.has(name)) + .sort() + expect(missing).toEqual([]) +}) + +// The pin that keeps the two rules above from holding by not looking. The +// attachment trace runs through an alias hop, so it has more ways to stop +// matching than the other two — assert it still finds the tool it is for. +test('the tools reaching a bound attachment verb are exactly upload_file', async () => { + const facts = toolFactsFromToolsSource(await toolsSource()) + const reaching = [...facts] + .filter(([, f]) => + [...f.attachmentVerbs].some((v) => + (BOUND_ATTACHMENT_VERBS as ReadonlyArray).includes(v), + ), + ) + .map(([name]) => name) + .sort() + expect(reaching).toEqual(['upload_file']) +}) + // The same rule, stated over the INBOX verbs that began binding with // comms-g5zh.2/.3. Kept as its own assertion with its own named list so the // publisher-side rule above cannot go green on a set that no longer covers @@ -354,12 +476,15 @@ test('the matcher carries no tool that never reaches boundHttp and never binds', .filter((name) => { const f = facts.get(name) if (f === undefined) return true - // Either receiver counts. A tool binds through the publisher verbs or - // through the inbox verbs; asking only about the first would call a - // legitimately-stamped `subscribe` an orphan. + // Any receiver counts. A tool binds through the publisher verbs, the + // inbox verbs or an attachment dep; asking only about the first would + // call a legitimately-stamped `subscribe` an orphan. return ( ![...f.verbs].some((v) => (BOUND_VERBS as ReadonlyArray).includes(v)) && - ![...f.inboxVerbs].some((v) => (BOUND_INBOX_VERBS as ReadonlyArray).includes(v)) + ![...f.inboxVerbs].some((v) => (BOUND_INBOX_VERBS as ReadonlyArray).includes(v)) && + ![...f.attachmentVerbs].some((v) => + (BOUND_ATTACHMENT_VERBS as ReadonlyArray).includes(v), + ) ) }) .sort() @@ -398,17 +523,25 @@ test('toolFactsFromToolsSource attributes verbs and session_id to the enclosing session_id: sessionIdField, }, }, + const upload = deps.upload + name: 'epsilon', + hostSuppliedArgs: hostSuppliedSessionId, + handler: async (args) => { + const result = await run(upload(path)) + }, ` const facts = toolFactsFromToolsSource(synthetic) expect(facts.get('alpha')).toEqual({ verbs: new Set(['post']), inboxVerbs: new Set(), + attachmentVerbs: new Set(), receivesSessionId: true, advertisesSessionId: false, }) expect(facts.get('beta')).toEqual({ verbs: new Set(), inboxVerbs: new Set(), + attachmentVerbs: new Set(), receivesSessionId: false, advertisesSessionId: false, }) @@ -417,17 +550,32 @@ test('toolFactsFromToolsSource attributes verbs and session_id to the enclosing expect(facts.get('gamma')).toEqual({ verbs: new Set(), inboxVerbs: new Set(['subscribe']), + attachmentVerbs: new Set(), receivesSessionId: false, advertisesSessionId: false, }) // The advertise-side trace catches a schema property coming back, and does - // not confuse it with the accept-side marker. + // not confuse it with the accept-side marker. `delta` also sits immediately + // before the `const upload = deps.upload` binding, so its empty + // `attachmentVerbs` is what proves the binding line is not attributed to + // whichever tool the scan is currently inside. expect(facts.get('delta')).toEqual({ verbs: new Set(), inboxVerbs: new Set(), + attachmentVerbs: new Set(), receivesSessionId: false, advertisesSessionId: true, }) + // The alias hop, end to end: `upload` resolves through the binding to + // `deps.upload`, and `ATTACHMENT_DEP_ADAPTER_MEMBER` resolves that to + // `adapter.uploadFile` — the member the tool source never names. + expect(facts.get('epsilon')).toEqual({ + verbs: new Set(), + inboxVerbs: new Set(), + attachmentVerbs: new Set(['uploadFile']), + receivesSessionId: true, + advertisesSessionId: false, + }) }) test('alternationToolsFromMatcher splits the trailing parenthesised group', () => { diff --git a/clients/claude-code/hooks/hooks.json b/clients/claude-code/hooks/hooks.json index 21e9ec3..09f6200 100644 --- a/clients/claude-code/hooks/hooks.json +++ b/clients/claude-code/hooks/hooks.json @@ -2,7 +2,7 @@ "hooks": { "PreToolUse": [ { - "matcher": "mcp__plugin_commy_commy__(post|edit_message|react|unreact|current_identity|subscribe|unsubscribe)", + "matcher": "mcp__plugin_commy_commy__(post|edit_message|react|unreact|current_identity|subscribe|unsubscribe|upload_file)", "hooks": [ { "type": "command", diff --git a/docs/self-hosting.md b/docs/self-hosting.md index 739456f..41af7ff 100644 --- a/docs/self-hosting.md +++ b/docs/self-hosting.md @@ -244,9 +244,10 @@ Two supported ways to avoid it, both already documented above: - Pass a UUID `session_id` in the tool-call arguments, which is the binding an ephemeral non-CC host has (`docs/claude-channel-inbound-contract.md`). The tools accepting it are `post`, `edit_message`, `react`, `unreact`, - `current_identity`, `subscribe` and `unsubscribe`. It is **not** advertised on - their `inputSchema` — a session id is the host's to supply, not something a - model could fill, so it is documented here rather than shown to the agent. + `current_identity`, `subscribe`, `unsubscribe` and `upload_file`. It is + **not** advertised on their `inputSchema` — a session id is the host's to + supply, not something a model could fill, so it is documented here rather + than shown to the agent. Anything that fails UUID validation is treated as missing: the server returns the unbound-stub error rather than minting a malformed `cc-*` identity. diff --git a/packages/mcp/tools.ts b/packages/mcp/tools.ts index 09a5635..c6e3e49 100644 --- a/packages/mcp/tools.ts +++ b/packages/mcp/tools.ts @@ -505,9 +505,10 @@ const buildToolDefs = (deps: RegisterToolsDeps, cache: InternalCache): ReadonlyA const projectForCwd = deps.projectForCwd ?? (() => Effect.succeed(undefined)) /** * The tools that accept a host-supplied `session_id` (see - * {@link ToolDef.hostSuppliedArgs}). SEVEN tools carry it, not the five in - * Claude Code's PreToolUse matcher: `subscribe` and `unsubscribe` are here - * for the non-CC ephemeral host that supplies the UUID itself, which is a + * {@link ToolDef.hostSuppliedArgs}). EIGHT tools carry it. That is the same + * eight Claude Code's PreToolUse matcher stamps today, but the two sets are + * separate questions and are allowed to differ: this one also serves the + * non-CC ephemeral host that supplies the UUID itself, which is a * listen-first seat's only route to an identity. A host stamping the arg on * a tool CC never stamps must not be told the argument is unknown. * @@ -1293,10 +1294,16 @@ const buildToolDefs = (deps: RegisterToolsDeps, cache: InternalCache): ReadonlyA type: 'string', description: 'Absolute path to the local file to upload (e.g. /tmp/chart.png)', }, + cwd: cwdField, }, required: ['path'], additionalProperties: false, }, + // An upload binds (comms-qpup). The file it writes carries an owner, and + // Zulip grants readers access to it only when that owner is also the + // account that sends the referencing message — so the upload has to go out + // under the calling session's own bot, which needs the session id here. + hostSuppliedArgs: hostSuppliedSessionId, handler: async (args) => { const run = runFor(args) const { path } = await run(Schema.decodeUnknown(UploadFileArgs)(args)) diff --git a/packages/zulip/adapter.ts b/packages/zulip/adapter.ts index a24f68c..6e5347e 100644 --- a/packages/zulip/adapter.ts +++ b/packages/zulip/adapter.ts @@ -2194,6 +2194,31 @@ export const zulipAdapter = ( inbox, history, directory, + // Stays on the MINTER, so a seat that only ever reads never binds. Once + // `uploadFile` moved to the bound bot below, the minter stopped owning the + // files it reads, so this now rests on two grounds — NEITHER OF WHICH THIS + // REPO CONTROLS, and both of which are realm state rather than code. + // + // Zulip's `validate_attachment_request` admits a reader on the first of + // three tests it passes: owns it, `is_realm_public`, or holds a + // `UserMessage` for a message referencing it. The two grounds map to the + // last two. `is_realm_public` is stamped onto the `Attachment` row at claim + // time from `stream.is_public()`, so it holds while no channel is + // invite-only. The `UserMessage` test holds while the minter is subscribed + // to the channel the attachment was posted in — measured by + // `cc-homelab-cbb5b9c3` on 2026-08-19 as 1194 active subscriptions against + // 1182 non-deactivated channels, i.e. everything. + // + // BOTH GROUNDS ARE ARGUED FROM REALM STATE AND ZULIP'S SOURCE, NOT + // ESTABLISHED BY A DISCRIMINATING TEST. They overlap completely on this + // realm, so no download run against it can say which one admitted the + // reader, or whether either would still admit it alone. What would + // discriminate: an invite-only channel the minter is NOT subscribed to, + // carrying an attachment posted by another account. Building that mutates + // the realm, so it is the operator's call, not this test suite's. + // + // If either ground goes away, this read fails and the fix is to move it + // onto `boundHttp()` — which costs the never-bind property above. downloadFile: (ref: AttachmentRef) => decodeUserUploadPath(ref).pipe( Effect.flatMap((urlPath) => @@ -2203,8 +2228,16 @@ export const zulipAdapter = ( ), Effect.mapError((cause) => new AttachmentError({ operation: 'download', cause })), ), + // An upload writes an `Attachment` row with an owner, so it belongs on + // the mint seam with the other attribution-producing verbs. It also has + // to be there for the reference to work at all: `do_claim_attachments` + // validates each attachment against the MESSAGE SENDER, so an upload + // owned by the minter and posted by the bound bot fails that check. Zulip + // logs a warning, skips the row that grants read access, and sends the + // message anyway — the link renders and nobody can open it. uploadFile: (filename: string, data: Uint8Array) => - minterHttp.uploadRaw(filename, data).pipe( + boundHttp().pipe( + Effect.flatMap((http) => http.uploadRaw(filename, data)), Effect.flatMap((upload) => decodeAttachmentRef(upload.url).pipe( Effect.map((ref) => ({ diff --git a/packages/zulip/bot-dm-guard.test.ts b/packages/zulip/bot-dm-guard.test.ts index e3184bd..dda1fe3 100644 --- a/packages/zulip/bot-dm-guard.test.ts +++ b/packages/zulip/bot-dm-guard.test.ts @@ -257,3 +257,41 @@ effectTest( expect(yield* messagesPosted(stub)).toBe(0) }), ) + +/** + * `BotHttp` carries `uploadRaw` because an upload has to go out under the same + * account that sends the referencing message — Zulip validates a message's + * attachments against the SENDER (comms-qpup). Widening a security-adjacent + * type is the move that gets accepted once and then cited for the next member, + * so the reasoning is pinned here rather than left in a PR body: THE WALL THIS + * WRAPPER ENFORCES IS A RECIPIENT RULE ON `POST /messages`, and an upload + * addresses no recipient. A member that can address one does not belong here on + * this precedent. + * + * One test, two assertions, and the second is the one that matters: the same + * wrapped client that just uploaded still refuses a bot-to-bot direct message. + */ +effectTest('uploadRaw passes through, and the DM wall still stands on the same client', () => + Effect.gen(function* () { + const stub = yield* makeStubHttpClient + yield* stub.respond('POST', '/api/v1/user_uploads', { + body: { url: '/user_uploads/2/ab/token/chart.svg', filename: 'chart.svg' }, + }) + const http = wrapBotHttp(yield* buildHttp(stub), directoryStub, SELF_ID) + + const uploaded = yield* http + .uploadRaw('chart.svg', new Uint8Array([1, 2, 3])) + .pipe(Effect.orDie) + expect(uploaded.filename).toBe('chart.svg') + expect(yield* requestsTo(stub, 'POST', '/api/v1/user_uploads')).toBe(1) + + yield* expectBotDirectMessageDefect( + http.post('/messages', sentMessageSchema, { + type: 'private', + to: JSON.stringify([OTHER_BOT_ID]), + content: 'hi', + }), + ) + expect(yield* messagesPosted(stub)).toBe(0) + }), +) diff --git a/packages/zulip/bot-dm-guard.ts b/packages/zulip/bot-dm-guard.ts index aadd1f0..789d27a 100644 --- a/packages/zulip/bot-dm-guard.ts +++ b/packages/zulip/bot-dm-guard.ts @@ -32,7 +32,14 @@ export interface RecipientDirectory { readonly byId: ReadonlyMap } -export type BotHttp = Pick +/** + * The bot-authenticated slice of {@link ZulipHttp}. `uploadRaw` is here + * because an upload writes an `Attachment` row owned by the account that + * sends the request, and Zulip's `do_claim_attachments` validates a message's + * attachments against the SENDER — so the upload has to go out under the same + * credential as the post that references it. + */ +export type BotHttp = Pick const decodeUserIds = Schema.decodeUnknownEither(Schema.parseJson(Schema.Array(Schema.Int))) @@ -99,4 +106,7 @@ export const wrapBotHttp = ( inner.patch(path, schema, body), delete: (path: string, schema: Schema.Schema, body?: ZulipParams) => inner.delete(path, schema, body), + // Nothing to guard: the wall this wrapper enforces is a recipient rule on + // `POST /messages`, and an upload addresses no recipient. + uploadRaw: (filename: string, data: Uint8Array) => inner.uploadRaw(filename, data), }) diff --git a/packages/zulip/realm.live.test.ts b/packages/zulip/realm.live.test.ts index fb70498..60d055b 100644 --- a/packages/zulip/realm.live.test.ts +++ b/packages/zulip/realm.live.test.ts @@ -712,3 +712,72 @@ describeLive('zulip live upload round-trip — zulip.example.com', () => { }), )) }) + +describeLiveChannel('zulip live attachment claim — zulip.example.com', () => { + // The regression comms-qpup exists to catch. `uploadFile` used to go out + // through the MINTER while `post` went out through the bound bot, so Zulip's + // `do_claim_attachments` — which validates each attachment against the + // MESSAGE SENDER — skipped the row that grants read access, logged a warning, + // and sent the message anyway. The link rendered and nobody could open it. + // + // WHY THE READER IS A SECOND BOT AND NOT THE ADAPTER'S OWN `downloadFile`. + // `downloadFile` reads through the minter, and the minter is subscribed to + // every channel on this realm, so it holds a `UserMessage` for the + // referencing message and its read succeeds on a ground this fix does not + // touch. That instrument cannot tell a claimed attachment from an unclaimed + // one. A FRESHLY MINTED BOT CAN: it owns nothing and is subscribed to + // nothing, so it fails the ownership test and the `UserMessage` test, and + // `is_realm_public` is its only remaining route — and that flag is stamped + // onto the `Attachment` row only when the claim SUCCEEDS. + // + // So this reads 403 before the fix and returns the bytes after it. + test( + 'a file uploaded and posted by one bot is readable by a second bot that owns nothing and is subscribed to nothing', + () => + Effect.runPromise( + Effect.gen(function* () { + const channel = decodeChannelNameSync(liveChannelName ?? '') + const thread = decodeThreadNameSync( + `cc-live-attach-${Date.now().toString(36)}-${Math.random().toString(36).slice(2, 8)}`, + ) + // Four bytes. The upload is permanent realm state, so the test keeps + // its own footprint to the smallest thing that still proves a + // round-trip. + const bytes = new Uint8Array([0x71, 0x70, 0x75, 0x70]) + const filename = `cc-live-attach-${Date.now()}.bin` + + const owner = yield* buildAdapter() + const uploaded = yield* Effect.acquireUseRelease( + pacedAcquire(owner, decodeBotNameSync(uniqueName('attach-owner'))), + () => + Effect.gen(function* () { + const up = yield* owner.uploadFile(filename, bytes) + // The post is what triggers the claim, so it is part of the + // setup rather than an assertion — an upload nobody references + // is never claimed by anyone. + yield* owner.publisher.post( + channel, + decodeMessageBodySync(`attachment claim probe ${up.reference}`), + { thread }, + ) + return up + }), + () => pacedRelease(owner), + ) + + const reader = yield* buildAdapter() + yield* Effect.acquireUseRelease( + pacedAcquire(reader, decodeBotNameSync(uniqueName('attach-reader'))), + (acquired) => + Effect.gen(function* () { + const http = yield* botHttp(liveEnv(), credentialsOf(acquired.credentials)) + const back = yield* http.downloadRaw(uploaded.ref) + expect(new Uint8Array(back.data)).toEqual(bytes) + }), + () => pacedRelease(reader), + ) + }), + ), + 60_000, + ) +})