From 9c46cf676155dca4ff4de1700c1b320cd932883e Mon Sep 17 00:00:00 2001 From: Codex Date: Fri, 12 Jun 2026 00:39:46 +0900 Subject: [PATCH] fix: relocate out-of-allowed outbound attachments so files actually send Agent-generated files written to an arbitrary working path (e.g. TTS audio under /home/claude/jarvis-tts) were rejected by validateOutboundAttachments as "outside-allowed-dirs". The rejection was only logged; the MEDIA: directive had already been stripped from the text, so the user got a message claiming a file was attached with no file and no error. - Stage attachments outside the room's allowed dirs into a safe per-group dir (data/attachments/outbound/) at the universal delivery choke point, so the path delivery uses and revalidates is one the validator accepts. Files already inside an allowed dir are untouched, preserving isolation checks. - Surface any still-rejected attachment in the visible Discord body via appendRejectionNotice, so delivery can never again silently drop a file. Verified: outbound-attachments 22/22, discord 46/46 (incl. new integration test asserting the notice lands in the sent body), final-delivery 5/5, tsc clean. Co-Authored-By: Claude Opus 4.7 --- src/channels/discord.test.ts | 22 +++++ src/channels/discord.ts | 11 ++- src/message-runtime-final-delivery.ts | 21 ++++- src/outbound-attachments.test.ts | 111 +++++++++++++++++++++- src/outbound-attachments.ts | 128 +++++++++++++++++++++++++- 5 files changed, 285 insertions(+), 8 deletions(-) diff --git a/src/channels/discord.test.ts b/src/channels/discord.test.ts index b1e4aaa..cdf4fc6 100644 --- a/src/channels/discord.test.ts +++ b/src/channels/discord.test.ts @@ -977,6 +977,28 @@ describe('sendMessage', () => { fs.rmSync(dir, { recursive: true, force: true }); }); + it('surfaces rejected attachments in the sent message instead of dropping them silently', async () => { + const opts = createTestOpts(); + const channel = new DiscordChannel('test-token', opts); + await channel.connect(); + const mockChannel = { + send: vi.fn().mockResolvedValue({ id: 'discord-message-1' }), + sendTyping: vi.fn(), + }; + currentClient().channels.fetch.mockResolvedValue(mockChannel); + + await channel.sendMessage('dc:1234567890123456', '샘플을 첨부합니다.', { + attachments: [{ path: '/home/claude/missing-sample.wav' }], + }); + + expect(mockChannel.send).toHaveBeenCalledTimes(1); + const sent = mockChannel.send.mock.calls[0][0]; + expect(sent.files).toBeUndefined(); + expect(sent.content).toContain('샘플을 첨부합니다.'); + expect(sent.content).toContain('첨부 1건을 전송하지 못했습니다'); + expect(sent.content).toContain('missing-sample.wav (파일을 찾을 수 없음)'); + }); + it('uses legacy image tags as Discord attachment fallback', async () => { const opts = createTestOpts(); const channel = new DiscordChannel('test-token', opts); diff --git a/src/channels/discord.ts b/src/channels/discord.ts index 24c692a..91c1e73 100644 --- a/src/channels/discord.ts +++ b/src/channels/discord.ts @@ -14,7 +14,10 @@ import { import { CACHE_DIR } from '../config.js'; import { getEnv } from '../env.js'; import { logger } from '../logger.js'; -import { validateOutboundAttachments } from '../outbound-attachments.js'; +import { + appendRejectionNotice, + validateOutboundAttachments, +} from '../outbound-attachments.js'; import { formatOutbound } from '../router.js'; import { hasReviewerLease } from '../service-routing.js'; import type { @@ -418,6 +421,12 @@ export class DiscordChannel implements Channel { cleaned = cleaned.replaceAll('@눈쟁이', '<@216851709744513024>'); cleaned = formatOutbound(cleaned); + // Surface rejected attachments in the visible message. The MEDIA: + // directive was already stripped from the text, so without this the user + // would see a message that claims a file was attached while none was + // delivered. Making the failure loud prevents that silent mismatch. + cleaned = appendRejectionNotice(cleaned, validation.rejected); + // Discord has a 2000 character limit per message and 10 attachments per message const MAX_LENGTH = 2000; const MAX_ATTACHMENTS = 10; diff --git a/src/message-runtime-final-delivery.ts b/src/message-runtime-final-delivery.ts index f5c15bb..aec805e 100644 --- a/src/message-runtime-final-delivery.ts +++ b/src/message-runtime-final-delivery.ts @@ -2,6 +2,10 @@ import { createProducedWorkItem } from './db.js'; import { resolveRuntimeAttachmentBaseDirs } from './attachment-base-dirs.js'; import { logger } from './logger.js'; import { deliverOpenWorkItem } from './message-runtime-delivery.js'; +import { + getOutboundStageDir, + stageOutboundAttachments, +} from './outbound-attachments.js'; import type { AgentType, Channel, @@ -50,6 +54,19 @@ export async function deliverMessageRuntimeFinalText(args: { return true; } + const attachmentBaseDirs = resolveRuntimeAttachmentBaseDirs(args.group); + // Relocate attachments that live outside the room's allowed directories into + // a safe staging dir before persisting the work item, so the path delivery + // actually uses (and revalidates) is one the validator accepts. Without this, + // agent-generated files written to an arbitrary working path are rejected as + // "outside-allowed-dirs" and silently dropped. + const attachments = args.attachments?.length + ? stageOutboundAttachments(args.attachments, { + baseDirs: attachmentBaseDirs, + stageDir: getOutboundStageDir(args.group.folder), + }) + : args.attachments; + const workItem = createProducedWorkItem({ group_folder: args.group.folder, chat_jid: args.chatJid, @@ -59,14 +76,14 @@ export async function deliverMessageRuntimeFinalText(args: { start_seq: args.startSeq, end_seq: args.endSeq, result_payload: args.text, - attachments: args.attachments, + attachments, }); return deliverOpenWorkItem({ channel: args.channel, item: workItem, log: logger, - attachmentBaseDirs: resolveRuntimeAttachmentBaseDirs(args.group), + attachmentBaseDirs, replaceMessageId: args.replaceMessageId, isDuplicateOfLastBotFinal: args.isDuplicateOfLastBotFinal, openContinuation: args.openContinuation, diff --git a/src/outbound-attachments.test.ts b/src/outbound-attachments.test.ts index 5428013..bd0c206 100644 --- a/src/outbound-attachments.test.ts +++ b/src/outbound-attachments.test.ts @@ -4,7 +4,12 @@ import path from 'path'; import { afterEach, describe, expect, it, vi } from 'vitest'; -import { validateOutboundAttachments } from './outbound-attachments.js'; +import { + appendRejectionNotice, + describeRejectedAttachments, + stageOutboundAttachments, + validateOutboundAttachments, +} from './outbound-attachments.js'; const ONE_PIXEL_PNG = Buffer.from( 'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mP8/x8AAwMCAO+/p9sAAAAASUVORK5CYII=', @@ -14,6 +19,11 @@ const MINIMAL_MP4 = Buffer.from([ 0x00, 0x00, 0x00, 0x18, 0x66, 0x74, 0x79, 0x70, 0x69, 0x73, 0x6f, 0x6d, 0x00, 0x00, 0x02, 0x00, 0x69, 0x73, 0x6f, 0x6d, 0x69, 0x73, 0x6f, 0x32, ]); +const WAV_BYTES = Buffer.concat([ + Buffer.from('RIFF', 'ascii'), + Buffer.from([0x24, 0x00, 0x00, 0x00]), + Buffer.from('WAVE', 'ascii'), +]); const MINIMAL_PDF = Buffer.from('%PDF-1.4\n', 'ascii'); const MINIMAL_ZIP = Buffer.from([0x50, 0x4b, 0x03, 0x04, 0x14, 0x00]); @@ -325,3 +335,102 @@ describe('validateOutboundAttachments policy checks', () => { }); }); }); + +describe('describeRejectedAttachments', () => { + it('returns an empty string when nothing was rejected', () => { + expect(describeRejectedAttachments([])).toBe(''); + }); + + it('summarizes rejected attachments by basename and reason', () => { + const note = describeRejectedAttachments([ + { path: '/home/claude/jarvis-tts/sample.wav', reason: 'outside-allowed-dirs' }, + { path: '/tmp/missing.png', reason: 'not-found' }, + ]); + + expect(note).toContain('첨부 2건을 전송하지 못했습니다'); + expect(note).toContain('sample.wav (허용된 폴더 밖의 경로)'); + expect(note).toContain('missing.png (파일을 찾을 수 없음)'); + }); + + it('falls back to the raw reason code when no label is mapped', () => { + const note = describeRejectedAttachments([ + { path: '/tmp/x.bin', reason: 'some-new-reason' }, + ]); + + expect(note).toContain('x.bin (some-new-reason)'); + }); +}); + +describe('appendRejectionNotice', () => { + it('leaves the body untouched when nothing was rejected', () => { + expect(appendRejectionNotice('보고서입니다', [])).toBe('보고서입니다'); + }); + + it('appends the failure notice to the visible body', () => { + const body = appendRejectionNotice('샘플을 첨부합니다', [ + { path: '/home/claude/jarvis-tts/a.wav', reason: 'outside-allowed-dirs' }, + ]); + + expect(body).toContain('샘플을 첨부합니다'); + expect(body).toContain('첨부 1건을 전송하지 못했습니다'); + expect(body).toContain('a.wav (허용된 폴더 밖의 경로)'); + }); + + it('returns the notice alone when the body would otherwise be empty', () => { + const body = appendRejectionNotice('', [ + { path: '/tmp/missing.png', reason: 'not-found' }, + ]); + + expect(body).toBe(describeRejectedAttachments([ + { path: '/tmp/missing.png', reason: 'not-found' }, + ])); + }); +}); + +describe('stageOutboundAttachments', () => { + it('copies attachments outside the allowed dirs into the stage dir and keeps them deliverable', () => { + const sourceDir = makeTempDir(process.cwd(), '.ejclaw-external-src-'); + const stageDir = makeTempDir(os.tmpdir(), 'ejclaw-outbound-stage-'); + const sourcePath = writeFile(sourceDir, 'sample.wav', WAV_BYTES); + + const [staged] = stageOutboundAttachments([{ path: sourcePath }], { + stageDir, + }); + + expect(staged.path.startsWith(stageDir)).toBe(true); + expect(staged.path).not.toBe(sourcePath); + expect(fs.readFileSync(staged.path)).toEqual(WAV_BYTES); + + // The relocated copy is now accepted by the validator (delivery succeeds), + // which is the regression the silent drop used to break. + const validation = validateOutboundAttachments([{ path: staged.path }], { + baseDirs: [stageDir], + }); + expect(validation.rejected).toEqual([]); + expect(validation.files).toHaveLength(1); + }); + + it('leaves attachments already inside an allowed dir untouched', () => { + const allowedDir = makeTempDir(os.tmpdir(), 'ejclaw-attachment-'); + const stageDir = makeTempDir(os.tmpdir(), 'ejclaw-outbound-stage-'); + const sourcePath = writeFile(allowedDir, 'inside.png', ONE_PIXEL_PNG); + + const [staged] = stageOutboundAttachments([{ path: sourcePath }], { + baseDirs: [allowedDir], + stageDir, + }); + + expect(staged.path).toBe(sourcePath); + }); + + it('returns missing or relative paths unchanged so validation can reject them', () => { + const stageDir = makeTempDir(os.tmpdir(), 'ejclaw-outbound-stage-'); + + expect( + stageOutboundAttachments( + [{ path: '/does/not/exist.png' }, { path: 'relative.png' }], + { stageDir }, + ), + ).toEqual([{ path: '/does/not/exist.png' }, { path: 'relative.png' }]); + }); +}); diff --git a/src/outbound-attachments.ts b/src/outbound-attachments.ts index 08258a5..2de9e9c 100644 --- a/src/outbound-attachments.ts +++ b/src/outbound-attachments.ts @@ -1,3 +1,4 @@ +import crypto from 'crypto'; import fs from 'fs'; import os from 'os'; import path from 'path'; @@ -92,6 +93,13 @@ export function getDefaultAttachmentBaseDirs(): string[] { .filter((dir): dir is string => Boolean(dir)); } +function resolveAllowedBaseDirs(baseDirs?: string[]): string[] { + return unique([ + ...getDefaultAttachmentBaseDirs(), + ...(baseDirs ?? []).map(resolveExistingDir), + ]).filter((dir): dir is string => Boolean(dir)); +} + function isWithinBaseDir(realPath: string, baseDir: string): boolean { const relative = path.relative(baseDir, realPath); return ( @@ -235,14 +243,126 @@ function normalizeAttachmentName( return candidate || 'attachment'; } +const REJECTION_REASON_LABELS: Record = { + 'outside-allowed-dirs': '허용된 폴더 밖의 경로', + 'not-found': '파일을 찾을 수 없음', + 'not-absolute': '절대경로가 아님', + 'not-file': '파일이 아님', + 'too-large': '8MB 용량 초과', + 'unsupported-extension': '지원하지 않는 형식', + 'invalid-image-signature': '이미지 형식 손상 또는 불일치', + 'invalid-file-signature': '파일 형식 손상 또는 불일치', + 'mime-mismatch': '형식 불일치', + 'validation-error': '검증 오류', +}; + +/** + * Builds a concise, user-facing note describing attachments that failed to + * send. Returning a non-empty string lets the delivery layer surface the + * failure in the visible message instead of dropping it silently — otherwise a + * stripped MEDIA: directive leaves the user with a message that claims a file + * was attached when none was delivered. + */ +export function describeRejectedAttachments( + rejected: Array<{ path: string; reason: string }>, +): string { + if (rejected.length === 0) return ''; + const lines = rejected.map(({ path: rejectedPath, reason }) => { + const label = REJECTION_REASON_LABELS[reason] ?? reason; + return `• ${path.basename(rejectedPath)} (${label})`; + }); + return `⚠️ 첨부 ${rejected.length}건을 전송하지 못했습니다:\n${lines.join('\n')}`; +} + +/** + * Folds a rejected-attachment notice into the visible outbound text so the + * delivery layer never sends a message that silently dropped its attachments. + * Returns the notice on its own when the body would otherwise be empty. + */ +export function appendRejectionNotice( + cleanText: string, + rejected: Array<{ path: string; reason: string }>, +): string { + const note = describeRejectedAttachments(rejected); + if (!note) return cleanText; + return cleanText ? `${cleanText}\n\n${note}` : note; +} + +export function isAttachmentWithinAllowedDirs( + realPath: string, + baseDirs?: string[], +): boolean { + const allowed = resolveAllowedBaseDirs(baseDirs); + const tempDir = resolveExistingDir(os.tmpdir()); + return ( + matchesAllowedBaseDir(realPath, allowed) || + isWithinTempDir(realPath, tempDir) + ); +} + +export function getOutboundStageDir(groupFolder: string): string { + const safe = groupFolder.replace(/[^a-zA-Z0-9._-]+/g, '-') || 'group'; + return path.join(DATA_DIR, 'attachments', 'outbound', safe); +} + +/** + * Relocates outbound attachments that live outside the room's allowed + * directories into a safe staging directory inside DATA_DIR, returning the + * rewritten attachment list. This is what makes agent-generated files (e.g. TTS + * audio written to an arbitrary working path) actually deliverable: the staged + * copy lands inside an allowlisted directory, so validateOutboundAttachments + * accepts it while still enforcing every signature/size/type check on the copy. + * + * Attachments already inside an allowed directory are returned untouched so the + * existing isolation checks (including symlink-escape rejection) keep applying. + * Anything that cannot be staged is returned as-is; validation then rejects it + * and appendRejectionNotice surfaces the failure visibly instead of silently. + */ +export function stageOutboundAttachments( + attachments: OutboundAttachment[] | undefined, + options: { baseDirs?: string[]; stageDir: string }, +): OutboundAttachment[] { + if (!attachments || attachments.length === 0) return []; + return attachments.map((attachment) => { + try { + const requested = attachment.path; + if (!path.isAbsolute(requested) || !fs.existsSync(requested)) { + return attachment; + } + const realPath = fs.realpathSync(requested); + if (!fs.statSync(realPath).isFile()) return attachment; + if (isAttachmentWithinAllowedDirs(realPath, options.baseDirs)) { + return attachment; + } + fs.mkdirSync(options.stageDir, { recursive: true }); + const hash = crypto + .createHash('sha1') + .update(realPath) + .digest('hex') + .slice(0, 10); + const targetPath = path.join( + options.stageDir, + `${hash}-${path.basename(realPath)}`, + ); + fs.copyFileSync(realPath, targetPath); + return { + ...attachment, + path: targetPath, + name: attachment.name + ? path.basename(attachment.name) + : path.basename(realPath), + }; + } catch { + return attachment; + } + }); +} + export function validateOutboundAttachments( attachments: OutboundAttachment[] | undefined, options: { baseDirs?: string[] } = {}, ): ValidateOutboundAttachmentsResult { - const baseDirs = unique([ - ...getDefaultAttachmentBaseDirs(), - ...(options.baseDirs ?? []).map(resolveExistingDir), - ]).filter((dir): dir is string => Boolean(dir)); + const baseDirs = resolveAllowedBaseDirs(options.baseDirs); const defaultTempDir = resolveExistingDir(os.tmpdir()); const files: ValidatedOutboundAttachment[] = []; const rejected: Array<{ path: string; reason: string }> = [];