mirror of
https://github.com/Ark0N/Codeman.git
synced 2026-10-03 05:59:43 +02:00
fix(review): harden Codex generated-artifact attachment pipeline (PR #150)
- Pass the attachment request `source` through the server deps lambda and make it a required param on SessionListenerDeps.registerAttachment + the wiring event type (the 2-arg lambda silently dropped `source`, force-confining every codex-generated artifact — the feature never worked outside the workspace); new test/session-listener-wiring.test.ts asserts the pass-through - Gate the Codex `Saved to: file://` scanner on mode === 'codex' via a codexArtifacts option threaded from the session call site; magic links stay mode-agnostic; tests assert claude/shell sessions never emit codex-generated requests - Decide the generated-artifact trust policy on the realpath-RESOLVED path (unresolvable → force-confined) and anchor the ~/.codex marker dirs to os.homedir() prefixes with startsWith instead of substring matching; symlink escape + unanchored-marker regression tests added - Run the Codex scanner on stripAnsi'd data so trailing SGR sequences don't ride into the captured URL; styled 'Saved to:' test added - Extend generateFirstPageThumbnail with jpg/jpeg/gif/webp passthrough and per-extension content types (mirrors the png passthrough) so the PR's new image formats render real thumbnails instead of 204 letter-tiles Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
@@ -58,7 +58,8 @@ describe('attachment magic links', () => {
|
||||
|
||||
it('extracts Codex generated image file URLs from saved-to terminal output', () => {
|
||||
const requests = parseTerminalAttachmentRequests(
|
||||
'Saved to: file:///Users/aamer/.codex-personal/generated_images/mockup%20one.png'
|
||||
'Saved to: file:///Users/aamer/.codex-personal/generated_images/mockup%20one.png',
|
||||
{ codexArtifacts: true }
|
||||
);
|
||||
|
||||
expect(requests).toEqual([
|
||||
@@ -69,6 +70,28 @@ describe('attachment magic links', () => {
|
||||
]);
|
||||
});
|
||||
|
||||
it('ignores Codex saved-to output unless the codex scanner is enabled', () => {
|
||||
const requests = parseTerminalAttachmentRequests(
|
||||
'Saved to: file:///Users/aamer/.codex-personal/generated_images/mockup.png'
|
||||
);
|
||||
|
||||
expect(requests).toEqual([]);
|
||||
});
|
||||
|
||||
it('strips ANSI styling around Codex saved-to lines before capturing the URL', () => {
|
||||
const requests = parseTerminalAttachmentRequests(
|
||||
'\x1b[1mSaved to:\x1b[0m file:///Users/aamer/.codex/generated_images/mockup.png\x1b[0m\r\n',
|
||||
{ codexArtifacts: true }
|
||||
);
|
||||
|
||||
expect(requests).toEqual([
|
||||
{
|
||||
path: '/Users/aamer/.codex/generated_images/mockup.png',
|
||||
source: 'codex-generated',
|
||||
},
|
||||
]);
|
||||
});
|
||||
|
||||
it('emits generated artifact requests from Codex saved-to output', () => {
|
||||
const session = new Session({ id: 'session-generated-artifact-test', workingDir: '/tmp', mode: 'codex' });
|
||||
const requested: Array<{ path: string; source?: string }> = [];
|
||||
@@ -88,6 +111,20 @@ describe('attachment magic links', () => {
|
||||
]);
|
||||
});
|
||||
|
||||
it('does not emit codex-generated requests from non-codex session modes', () => {
|
||||
for (const mode of ['claude', 'shell'] as const) {
|
||||
const session = new Session({ id: `session-generated-artifact-${mode}`, workingDir: '/tmp', mode });
|
||||
const requested: Array<{ path: string }> = [];
|
||||
session.on('attachmentRequested', (event: { path: string }) => requested.push(event));
|
||||
|
||||
(session as unknown as { _handleTerminalOutput(data: string): void })._handleTerminalOutput(
|
||||
'Saved to: file:///Users/aamer/.codex-personal/generated_images/output.png'
|
||||
);
|
||||
|
||||
expect(requested).toEqual([]);
|
||||
}
|
||||
});
|
||||
|
||||
it('supports generated image attachment extensions beyond png', () => {
|
||||
expect(isSupportedAttachmentExtension('jpg')).toBe(true);
|
||||
expect(isSupportedAttachmentExtension('jpeg')).toBe(true);
|
||||
|
||||
@@ -71,6 +71,22 @@ describe('document-thumbnailer', () => {
|
||||
);
|
||||
});
|
||||
|
||||
it('passes through generated image formats with per-extension content types', async () => {
|
||||
const expectations: Array<[string, string]> = [
|
||||
['jpg', 'image/jpeg'],
|
||||
['jpeg', 'image/jpeg'],
|
||||
['gif', 'image/gif'],
|
||||
['webp', 'image/webp'],
|
||||
['png', 'image/png'],
|
||||
];
|
||||
|
||||
for (const [ext, contentType] of expectations) {
|
||||
const result = await generateFirstPageThumbnail(`/tmp/mockup.${ext}`, ext);
|
||||
expect(result).toEqual({ content: Buffer.from('large thumbnail'), contentType });
|
||||
}
|
||||
expect(mockedExecFile).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('renders Office thumbnails from the cached converted PDF after conversion cleanup', async () => {
|
||||
mockedMkdtemp.mockImplementation(async (prefix) =>
|
||||
String(prefix).includes('codeman-document-preview-cache')
|
||||
|
||||
@@ -1,15 +1,78 @@
|
||||
import { describe, expect, it } from 'vitest';
|
||||
import { isAllowedGeneratedArtifactPath } from '../src/generated-artifact-attachments.js';
|
||||
import { afterEach, describe, expect, it } from 'vitest';
|
||||
import fs from 'node:fs/promises';
|
||||
import { homedir, tmpdir } from 'node:os';
|
||||
import { join } from 'node:path';
|
||||
import {
|
||||
isAllowedGeneratedArtifactPath,
|
||||
registerGeneratedArtifactAttachment,
|
||||
} from '../src/generated-artifact-attachments.js';
|
||||
import { attachmentRegistry } from '../src/attachment-registry.js';
|
||||
|
||||
describe('generated artifact attachments', () => {
|
||||
it('allows workspace artifacts and known Codex generated image directories', () => {
|
||||
it('allows workspace artifacts and home-anchored Codex generated image directories', () => {
|
||||
const home = homedir();
|
||||
expect(isAllowedGeneratedArtifactPath('/repo/out/mockup.png', '/repo')).toBe(true);
|
||||
expect(isAllowedGeneratedArtifactPath('/Users/aamer/.codex-personal/generated_images/mockup.png', '/repo')).toBe(
|
||||
expect(
|
||||
isAllowedGeneratedArtifactPath(join(home, '.codex-personal', 'generated_images', 'mockup.png'), '/repo')
|
||||
).toBe(true);
|
||||
expect(isAllowedGeneratedArtifactPath(join(home, '.codex', 'generated_artifacts', 'report.pdf'), '/repo')).toBe(
|
||||
true
|
||||
);
|
||||
expect(isAllowedGeneratedArtifactPath('/etc/secret.png', '/repo')).toBe(false);
|
||||
expect(
|
||||
isAllowedGeneratedArtifactPath('/Users/aamer/.codex-personal/generated_images/../../.ssh/id_rsa.png', '/repo')
|
||||
isAllowedGeneratedArtifactPath(
|
||||
join(home, '.codex-personal', 'generated_images', '..', '..', '.ssh', 'id_rsa.png'),
|
||||
'/repo'
|
||||
)
|
||||
).toBe(false);
|
||||
});
|
||||
|
||||
it('rejects .codex marker directories that are not anchored at the user home', () => {
|
||||
expect(isAllowedGeneratedArtifactPath('/var/tmp/staging/.codex/generated_images/leak.png', '/repo')).toBe(false);
|
||||
expect(isAllowedGeneratedArtifactPath('/var/tmp/.codex-personal/generated_artifacts/leak.md', '/repo')).toBe(false);
|
||||
});
|
||||
|
||||
describe('symlink resolution', () => {
|
||||
let workspaceDir: string | undefined;
|
||||
let outsideDir: string | undefined;
|
||||
const sessionId = 'generated-artifact-symlink-test';
|
||||
|
||||
afterEach(async () => {
|
||||
attachmentRegistry.clearSession(sessionId);
|
||||
for (const dir of [workspaceDir, outsideDir]) {
|
||||
if (dir) await fs.rm(dir, { recursive: true, force: true });
|
||||
}
|
||||
workspaceDir = undefined;
|
||||
outsideDir = undefined;
|
||||
});
|
||||
|
||||
it('confines on the resolved path: a workspace symlink to an outside file is rejected', async () => {
|
||||
// realpath so a symlinked tmpdir (e.g. macOS /var -> /private/var) can't skew containment checks
|
||||
workspaceDir = await fs.realpath(await fs.mkdtemp(join(tmpdir(), 'codeman-genart-ws-')));
|
||||
outsideDir = await fs.realpath(await fs.mkdtemp(join(tmpdir(), 'codeman-genart-out-')));
|
||||
const outsideFile = join(outsideDir, 'private-notes.md');
|
||||
await fs.writeFile(outsideFile, 'secret');
|
||||
const linkPath = join(workspaceDir, 'x.md');
|
||||
await fs.symlink(outsideFile, linkPath);
|
||||
|
||||
await expect(
|
||||
registerGeneratedArtifactAttachment({ sessionId, filePath: linkPath, sessionWorkingDir: workspaceDir })
|
||||
).rejects.toMatchObject({ statusCode: 403 });
|
||||
});
|
||||
|
||||
it('registers a real workspace file', async () => {
|
||||
workspaceDir = await fs.realpath(await fs.mkdtemp(join(tmpdir(), 'codeman-genart-ws-')));
|
||||
const filePath = join(workspaceDir, 'mockup.png');
|
||||
await fs.writeFile(filePath, 'png-bytes');
|
||||
|
||||
const event = await registerGeneratedArtifactAttachment({
|
||||
sessionId,
|
||||
filePath,
|
||||
sessionWorkingDir: workspaceDir,
|
||||
});
|
||||
|
||||
expect(event.fileName).toBe('mockup.png');
|
||||
expect(event.attachmentType).toBe('image');
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -0,0 +1,23 @@
|
||||
import { describe, expect, it, vi } from 'vitest';
|
||||
import { Session } from '../src/session.js';
|
||||
import { createSessionListeners } from '../src/web/session-listener-wiring.js';
|
||||
|
||||
describe('session listener wiring', () => {
|
||||
it('forwards the attachment request source through registerAttachment', async () => {
|
||||
const session = new Session({ id: 'wiring-attach-source-test', workingDir: '/tmp', mode: 'codex' });
|
||||
const registerAttachment = vi.fn(async () => undefined);
|
||||
const deps = { registerAttachment } as unknown as Parameters<typeof createSessionListeners>[1];
|
||||
|
||||
const refs = createSessionListeners(session, deps);
|
||||
refs.attachmentRequested({ path: '/tmp/mockup.png', source: 'codex-generated' });
|
||||
refs.attachmentRequested({ path: '/tmp/report.pdf', source: 'external' });
|
||||
|
||||
expect(registerAttachment).toHaveBeenNthCalledWith(
|
||||
1,
|
||||
'wiring-attach-source-test',
|
||||
'/tmp/mockup.png',
|
||||
'codex-generated'
|
||||
);
|
||||
expect(registerAttachment).toHaveBeenNthCalledWith(2, 'wiring-attach-source-test', '/tmp/report.pdf', 'external');
|
||||
});
|
||||
});
|
||||
Reference in New Issue
Block a user