From b672e8271ef7c71236d8dd27a7888944475facf7 Mon Sep 17 00:00:00 2001 From: Doug Daniels Date: Sun, 26 Apr 2026 22:17:09 -0400 Subject: [PATCH] feat(signal): support outbound attachments via signal-cli `attachments` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The native Signal adapter previously logged-and-dropped any OutboundFile entries with a TODO. This wires through to signal-cli's already-supported `send` JSON-RPC `attachments` parameter: - New `sendAttachments(platformId, files)` helper writes each OutboundFile.data Buffer to a temp file in os.tmpdir(), passes the paths to `tcp.rpc('send', { attachments: [...] })`, then cleans up the temp files in finally{}. - `deliver()` no longer drops files — sends accompanying text first via the existing sendText (preserving chunking + textStyle), then attachments as a single send call. - Filename sanitization replaces `/`, `\`, and `\0` with `_` so operator-supplied filenames can't escape tmpdir (CWE-22). - Tests cover: single attachment, text+attachment ordering, multiple attachments in one send, group destinations, and a structural invariant proving sanitized paths stay inside tmpdir(). Total signal tests: 33 → 37 (one stale "drops files" test replaced by the positive-behavior tests). Co-Authored-By: Claude Opus 4.7 (1M context) --- src/channels/signal.test.ts | 163 +++++++++++++++++++++++++++++------- src/channels/signal.ts | 69 +++++++++++---- 2 files changed, 189 insertions(+), 43 deletions(-) diff --git a/src/channels/signal.test.ts b/src/channels/signal.test.ts index e82be6066..5d8d36948 100644 --- a/src/channels/signal.test.ts +++ b/src/channels/signal.test.ts @@ -548,6 +548,141 @@ describe('SignalAdapter', () => { }); }); + // --- Outbound attachments --- + + describe('deliver — attachments', () => { + // Real fs writes happen in tmpdir(); confirm the bytes round-trip and + // are cleaned up after deliver returns. + it('sends a single attachment via attachments[] param', async () => { + const fs = await import('node:fs'); + const adapter = createAdapter(); + await adapter.setup(createMockSetup()); + tcpRef.fakeSocket.write.mockClear(); + + await adapter.deliver('+15555550123', null, { + kind: 'file', + content: {}, + files: [{ filename: 'report.md', data: Buffer.from('# Report\n\nbody') }], + }); + + const sendCalls = getRpcCallsForMethod('send'); + expect(sendCalls.length).toBe(1); + const params = sendCalls[0].params as Record; + expect(params.recipient).toEqual(['+15555550123']); + expect(params.account).toBe('+15551234567'); + expect(params.message).toBeUndefined(); + const paths = params.attachments as string[]; + expect(paths).toHaveLength(1); + expect(paths[0]).toMatch(/signal-out-\d+-[a-z0-9]+-report\.md$/); + // Temp file should no longer exist — finally{} cleanup ran + expect(fs.existsSync(paths[0])).toBe(false); + + await adapter.teardown(); + }); + + it('sends text first, then attachment, when both are present', async () => { + const adapter = createAdapter(); + await adapter.setup(createMockSetup()); + tcpRef.fakeSocket.write.mockClear(); + + await adapter.deliver('+15555550123', null, { + kind: 'file', + content: { text: 'Here is the digest' }, + files: [{ filename: 'digest.md', data: Buffer.from('content') }], + }); + + const sendCalls = getRpcCallsForMethod('send'); + expect(sendCalls).toHaveLength(2); + // First call: text message + expect(sendCalls[0].params).toEqual( + expect.objectContaining({ message: 'Here is the digest', recipient: ['+15555550123'] }), + ); + expect((sendCalls[0].params as Record).attachments).toBeUndefined(); + // Second call: attachment, no message + expect(sendCalls[1].params).toEqual( + expect.objectContaining({ recipient: ['+15555550123'] }), + ); + const attachments = (sendCalls[1].params as Record).attachments as string[]; + expect(attachments).toHaveLength(1); + + await adapter.teardown(); + }); + + it('sends multiple attachments in a single send call', async () => { + const adapter = createAdapter(); + await adapter.setup(createMockSetup()); + tcpRef.fakeSocket.write.mockClear(); + + await adapter.deliver('+15555550123', null, { + kind: 'file', + content: {}, + files: [ + { filename: 'a.txt', data: Buffer.from('a') }, + { filename: 'b.png', data: Buffer.from([0x89, 0x50, 0x4e, 0x47]) }, + ], + }); + + const sendCalls = getRpcCallsForMethod('send'); + expect(sendCalls).toHaveLength(1); + const attachments = (sendCalls[0].params as Record).attachments as string[]; + expect(attachments).toHaveLength(2); + expect(attachments[0]).toMatch(/-a\.txt$/); + expect(attachments[1]).toMatch(/-b\.png$/); + + await adapter.teardown(); + }); + + it('uses groupId for group destinations', async () => { + const adapter = createAdapter(); + await adapter.setup(createMockSetup()); + tcpRef.fakeSocket.write.mockClear(); + + await adapter.deliver('group:abc123', null, { + kind: 'file', + content: {}, + files: [{ filename: 'pic.jpg', data: Buffer.from('jpg') }], + }); + + const sendCalls = getRpcCallsForMethod('send'); + expect(sendCalls).toHaveLength(1); + const params = sendCalls[0].params as Record; + expect(params.groupId).toBe('abc123'); + expect(params.recipient).toBeUndefined(); + + await adapter.teardown(); + }); + + /** + * Defensive test: `OutboundFile.filename` is operator-supplied data, so + * the implementation must not let a filename containing path separators + * escape the temp directory. We feed an attempt-to-traverse filename and + * assert the resolved path stays strictly inside `tmpdir()`. + */ + it('keeps temp paths inside tmpdir even when filename contains path separators', async () => { + const path = await import('node:path'); + const os = await import('node:os'); + const adapter = createAdapter(); + await adapter.setup(createMockSetup()); + tcpRef.fakeSocket.write.mockClear(); + + await adapter.deliver('+15555550123', null, { + kind: 'file', + content: {}, + files: [{ filename: '../sneaky.txt', data: Buffer.from('x') }], + }); + + const sendCalls = getRpcCallsForMethod('send'); + const paths = (sendCalls[0].params as Record).attachments as string[]; + const resolvedTmp = path.resolve(os.tmpdir()); + const resolvedResult = path.resolve(paths[0]); + // path.resolve normalizes away any "../"; if sanitization failed, the + // result would resolve to tmpdir's parent. + expect(resolvedResult.startsWith(resolvedTmp + path.sep)).toBe(true); + + await adapter.teardown(); + }); + }); + // --- Text styles --- describe('text styles', () => { @@ -784,34 +919,6 @@ describe('SignalAdapter', () => { }); }); - // --- Outbound files --- - - describe('outbound files', () => { - it('logs a warning and drops unsupported file attachments', async () => { - const { log } = await import('../log.js'); - const warnMock = log.warn as unknown as ReturnType; - - const adapter = createAdapter(); - await adapter.setup(createMockSetup()); - warnMock.mockClear(); - - await adapter.deliver('+15555550123', null, { - kind: 'text', - content: { text: 'with an attachment' }, - files: [{ filename: 'hi.txt', data: Buffer.from('hi') }], - }); - - const sendCalls = getRpcCallsForMethod('send'); - expect(sendCalls.length).toBeGreaterThan(0); - expect(warnMock).toHaveBeenCalledWith( - 'Signal: outbound files not supported, dropping', - expect.objectContaining({ platformId: '+15555550123', count: 1 }), - ); - - await adapter.teardown(); - }); - }); - // --- setTyping --- describe('setTyping', () => { diff --git a/src/channels/signal.ts b/src/channels/signal.ts index 2aed22e80..1f490c18b 100644 --- a/src/channels/signal.ts +++ b/src/channels/signal.ts @@ -8,9 +8,9 @@ * Ported from v1 — see v1 source for commit history. */ import { execFileSync, execSync, spawn } from 'node:child_process'; -import { existsSync, readFileSync, unlinkSync } from 'node:fs'; +import { existsSync, readFileSync, unlinkSync, writeFileSync } from 'node:fs'; import { createConnection, type Socket } from 'node:net'; -import { homedir } from 'node:os'; +import { homedir, tmpdir } from 'node:os'; import { join } from 'node:path'; import type { ChannelAdapter, ChannelSetup, InboundMessage, OutboundMessage } from './adapter.js'; @@ -744,6 +744,51 @@ export function createSignalAdapter(config: { log.info('Signal message sent', { platformId, length: text.length }); } + /** + * Send one or more file attachments via signal-cli's `send` JSON-RPC, which + * accepts an `attachments` array of host filesystem paths. The OutboundFile + * Buffer is materialized to an OS temp file so signal-cli can read it, then + * removed in the finally block. + * + * Caption text, if any, is sent first via `sendText` (which handles chunking + * + textStyles) — keeps this function single-purpose and avoids a long + * caption colliding with signal-cli's per-message size limits. + */ + async function sendAttachments(platformId: string, files: { filename: string; data: Buffer }[]): Promise { + if (!connected || !tcp) return; + if (files.length === 0) return; + + const tempPaths: string[] = []; + for (const file of files) { + const safeName = file.filename.replace(/[/\\\0]/g, '_'); + const tempPath = join(tmpdir(), `signal-out-${Date.now()}-${Math.random().toString(36).slice(2, 8)}-${safeName}`); + writeFileSync(tempPath, file.data); + tempPaths.push(tempPath); + } + + try { + const params: Record = { attachments: tempPaths }; + if (config.account) params.account = config.account; + if (platformId.startsWith('group:')) { + params.groupId = platformId.slice('group:'.length); + } else { + params.recipient = [platformId]; + } + await tcp.rpc('send', params); + log.info('Signal attachments sent', { platformId, count: files.length, filenames: files.map((f) => f.filename) }); + } catch (err) { + log.error('Signal: attachment send failed', { platformId, count: files.length, err }); + } finally { + for (const p of tempPaths) { + try { + unlinkSync(p); + } catch { + /* best-effort cleanup */ + } + } + } + } + async function waitForDaemon(): Promise { const maxWait = 30_000; const pollInterval = 1000; @@ -847,17 +892,6 @@ export function createSignalAdapter(config: { }, async deliver(platformId: string, _threadId: string | null, message: OutboundMessage): Promise { - if (message.files && message.files.length > 0) { - // Native adapter doesn't yet forward file uploads to signal-cli's - // `send --attachment`. Don't silently swallow — operators need to see - // that an attachment was requested but not sent. - log.warn('Signal: outbound files not supported, dropping', { - platformId, - count: message.files.length, - filenames: message.files.map((f) => f.filename), - }); - } - const content = message.content as Record | string | undefined; let text: string | null = null; if (typeof content === 'string') { @@ -865,9 +899,14 @@ export function createSignalAdapter(config: { } else if (content && typeof content === 'object' && typeof content.text === 'string') { text = content.text; } - if (!text) return undefined; - await sendText(platformId, text); + const files = message.files ?? []; + + // Send accompanying text first so it lands above the attachment(s) in + // the recipient's chat. Both branches no-op cleanly if their input is + // empty, so any combination of (text, files) works. + if (text) await sendText(platformId, text); + if (files.length > 0) await sendAttachments(platformId, files); return undefined; },