From fd03b893336397728a09fa850473d001432e4063 Mon Sep 17 00:00:00 2001 From: Adam Date: Fri, 24 Apr 2026 15:44:19 +1000 Subject: [PATCH] fix(agent-route): reject unsafe attachment filenames to prevent path traversal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Filenames in forwardAttachedFiles arrived from the source agent's messages_out content and were used directly in path.join on both source outbox read and target inbox write. A value like `../evil.sh` could escape `inbox//` on the target session (and similarly the source outbox on read), breaking session isolation — an adversarial or hallucinating sub-agent could overwrite files in a sibling session. Adds isSafeAttachmentName(name) — exported so it's unit-testable — which rejects empty, `.`, `..`, anything containing `/`, `\`, or NUL, and anything path.basename would strip. Guard runs before any I/O. Unsafe names are dropped with a warning log, same pattern as missing-source-file handling; a bad filename in one attachment doesn't kill the whole route's text delivery. Addresses Codex Review P1 on qwibitai/nanoclaw#1967. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../agent-to-agent/agent-route.test.ts | 46 +++++++++++++++++++ src/modules/agent-to-agent/agent-route.ts | 33 +++++++++++-- 2 files changed, 76 insertions(+), 3 deletions(-) create mode 100644 src/modules/agent-to-agent/agent-route.test.ts diff --git a/src/modules/agent-to-agent/agent-route.test.ts b/src/modules/agent-to-agent/agent-route.test.ts new file mode 100644 index 0000000..4d48f6f --- /dev/null +++ b/src/modules/agent-to-agent/agent-route.test.ts @@ -0,0 +1,46 @@ +import { describe, expect, it } from 'vitest'; + +import { isSafeAttachmentName } from './agent-route.js'; + +/** + * `forwardAttachedFiles` has a filesystem side that's awkward to unit-test + * without mocking DATA_DIR. The guarantee worth pinning is that the + * filename validator rejects everything that could escape the inbox dir — + * `forwardAttachedFiles` runs this guard before any I/O, so traversal is + * impossible as long as this matrix holds. + */ +describe('isSafeAttachmentName', () => { + it('accepts plain filenames', () => { + expect(isSafeAttachmentName('baby-duck.png')).toBe(true); + expect(isSafeAttachmentName('file with spaces.pdf')).toBe(true); + expect(isSafeAttachmentName('report.v2.docx')).toBe(true); + expect(isSafeAttachmentName('.hidden')).toBe(true); // leading dot is fine, just not `.` / `..` + }); + + it('rejects empty / sentinel values', () => { + expect(isSafeAttachmentName('')).toBe(false); + expect(isSafeAttachmentName('.')).toBe(false); + expect(isSafeAttachmentName('..')).toBe(false); + }); + + it('rejects path separators', () => { + expect(isSafeAttachmentName('../evil.png')).toBe(false); + expect(isSafeAttachmentName('/etc/passwd')).toBe(false); + expect(isSafeAttachmentName('nested/file.txt')).toBe(false); + expect(isSafeAttachmentName('windows\\path.exe')).toBe(false); + }); + + it('rejects NUL bytes', () => { + expect(isSafeAttachmentName('clean\0.png')).toBe(false); + }); + + it('rejects anything path.basename would strip', () => { + expect(isSafeAttachmentName('a/b')).toBe(false); + expect(isSafeAttachmentName('./thing')).toBe(false); + }); + + it('rejects non-string input', () => { + expect(isSafeAttachmentName(null as unknown as string)).toBe(false); + expect(isSafeAttachmentName(undefined as unknown as string)).toBe(false); + }); +}); diff --git a/src/modules/agent-to-agent/agent-route.ts b/src/modules/agent-to-agent/agent-route.ts index faec2b9..812cb8e 100644 --- a/src/modules/agent-to-agent/agent-route.ts +++ b/src/modules/agent-to-agent/agent-route.ts @@ -36,6 +36,26 @@ export interface ForwardedAttachment { localPath: string; } +/** + * Is `name` safe to use as the last segment of a path inside the target + * agent's inbox directory? Filenames arrive in messages_out content from + * the source agent — under a multi-agent setup with heterogenous providers + * (or a compromised / hallucinating sub-agent) they can't be trusted. + * + * Rejects: + * - empty string + * - `.` / `..` (traversal sentinels that path.basename returns as-is) + * - anything containing a path separator (`/` or `\`) or NUL + * - any value where `path.basename(name) !== name`, catching OS-specific + * separators and covering drives/prefixes on Windows runtimes + */ +export function isSafeAttachmentName(name: string): boolean { + if (typeof name !== 'string' || name.length === 0) return false; + if (name === '.' || name === '..') return false; + if (/[\\/\0]/.test(name)) return false; + return path.basename(name) === name; +} + /** * Copy file attachments from the source agent's outbox into the target * agent's inbox. Returns attachments using the formatter's existing @@ -43,9 +63,9 @@ export interface ForwardedAttachment { * as relative to `/workspace/`, matching how channel-inbound attachments * are surfaced today. * - * Missing source files are skipped with a warning rather than failing - * the whole route — a bad filename reference shouldn't kill the - * accompanying text. + * Missing source files and unsafe (path-traversal) filenames are skipped + * with a warning rather than failing the whole route — a bad filename + * reference shouldn't kill the accompanying text. */ export function forwardAttachedFiles( source: { agentGroupId: string; sessionId: string; messageId: string; filenames: string[] }, @@ -67,6 +87,13 @@ export function forwardAttachedFiles( const attachments: ForwardedAttachment[] = []; for (const filename of source.filenames) { + if (!isSafeAttachmentName(filename)) { + log.warn('agent-route: rejecting unsafe attachment filename (path traversal attempt?)', { + sourceMsgId: source.messageId, + filename, + }); + continue; + } const src = path.join(sourceDir, filename); if (!fs.existsSync(src)) { log.warn('agent-route: referenced file missing in source outbox, skipped', {