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/<group>) 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 <noreply@anthropic.com>
This commit is contained in:
@@ -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);
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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' }]);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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<string, string> = {
|
||||
'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 }> = [];
|
||||
|
||||
Reference in New Issue
Block a user