diff --git a/desktop/src/components/chat/AssistantMessage.images.test.tsx b/desktop/src/components/chat/AssistantMessage.images.test.tsx index 5feb63ae..f172b028 100644 --- a/desktop/src/components/chat/AssistantMessage.images.test.tsx +++ b/desktop/src/components/chat/AssistantMessage.images.test.tsx @@ -73,6 +73,8 @@ describe('AssistantMessage · Markdown pictures on disk', () => { ['a file:// URL with a drive letter', '![chart](file:///C:/Users/me/chart.png)', 'C:/Users/me/chart.png'], ['a Windows path', '![chart](C:\\Users\\me\\chart.png)', 'C:/Users/me/chart.png'], ['a home-relative path', '![chart](~/Pictures/chart.png)', '~/Pictures/chart.png'], + ['the QA-003 home-relative path to /tmp', '![chart](~/../../tmp/qa/sample.png)', '~/../../tmp/qa/sample.png'], + ['an encoded home-relative path', '![chart](~/%2e%2e/%2e%2e/tmp/My%20Pics/sample.png)', '~/../../tmp/My Pics/sample.png'], ['a file:// URL', '![chart](file:///Users/me/chart.png)', '/Users/me/chart.png'], ])('shows a picture written as %s', (_label, markdown, path) => { const { container } = renderMessage(markdown) @@ -127,6 +129,15 @@ describe('AssistantMessage · looking closer at a picture', () => { expect(dialog.querySelector('img')).toHaveAttribute('src', `${BASE}/preview-fs/s1//repo/two.png`) }) + it('opens the QA-003 home-relative picture in the viewer', () => { + const { container } = renderMessage('![tilde](~/../../tmp/qa/sample.png)') + + fireEvent.click(proseImages(container)[0]!) + + expect(screen.getByRole('dialog', { name: 'tilde' }).querySelector('img')) + .toHaveAttribute('src', filesystem('~/../../tmp/qa/sample.png')) + }) + it('moves between the pictures of the reply', () => { const { container } = renderMessage(TWO) fireEvent.click(proseImages(container)[0]!) diff --git a/desktop/src/lib/markdownImages.test.ts b/desktop/src/lib/markdownImages.test.ts index 4f06194f..45e99234 100644 --- a/desktop/src/lib/markdownImages.test.ts +++ b/desktop/src/lib/markdownImages.test.ts @@ -179,10 +179,15 @@ describe('createAssistantMarkdownImageResolver with the session workdir known', expect(unknown('~/Pictures/chart.png')).toBe(filesystem('~/Pictures/chart.png')) }) - it('reads ../ under the home directory lexically, and refuses to climb out of it', () => { + it('normalizes home paths but leaves leading parents for the server to expand and authorize', () => { expect(resolve('~/Pictures/../Desktop/chart.png')).toBe(filesystem('~/Desktop/chart.png')) - expect(resolve('~/../chart.png')).toBeNull() - expect(resolve('~/../../etc/x.png')).toBeNull() + // QA-003: the home alias is not a sandbox root. This can name an allowed + // /tmp image; only the server knows where HOME is and which roots are allowed. + expect(resolve('~/../../tmp/qa/sample.png')).toBe(filesystem('~/../../tmp/qa/sample.png')) + expect(resolve('~/../Pictures/../../chart.png')).toBe(filesystem('~/../../chart.png')) + expect(resolve('~/%2e%2e/%2e%2e/tmp/qa/sample.png')).toBe(filesystem('~/../../tmp/qa/sample.png')) + expect(resolve('~/../../etc/x.png')).toBe(filesystem('~/../../etc/x.png')) + expect(resolve('../outside.png')).toBeNull() }) it('takes a bare Windows drive path, as it does the shape the renderer writes', () => { diff --git a/desktop/src/lib/markdownImages.ts b/desktop/src/lib/markdownImages.ts index 2d625bc6..1e817fb3 100644 --- a/desktop/src/lib/markdownImages.ts +++ b/desktop/src/lib/markdownImages.ts @@ -57,9 +57,10 @@ type LocalImagePath = | { root: 'drive'; drive: string; segments: string[] } /** - * `null` when the path cannot be a stable local file: a relative path (or one under - * `~`) that climbs out of where it starts. Above the root of an absolute path `..` - * stays at the root, as it does on disk. + * `null` when a workspace-relative path climbs out of where it starts. The home + * alias is expanded on the server, so leading parents must survive until then; + * the server authorizes the resulting canonical path. Above an absolute root, + * `..` stays at the root, as it does on disk. */ function parseLocalImagePath(value: string): LocalImagePath | null { const slashed = value.replace(/\\/g, '/') @@ -74,9 +75,11 @@ function parseLocalImagePath(value: string): LocalImagePath | null { for (const segment of body.split('/')) { if (!segment || segment === '.') continue if (segment === '..') { - if (segments.length > 0) segments.pop() - // Below a relative or home root it would leave the place it is relative to. - else if (root === 'relative' || root === 'home') return null + if (segments.length > 0 && segments.at(-1) !== '..') segments.pop() + else if (root === 'relative') return null + // HOME is an alias, not an authorization root. ~/../../tmp can be allowed, + // while an expanded path outside the server's roots is still rejected. + else if (root === 'home') segments.push('..') continue } segments.push(segment) diff --git a/src/server/__tests__/markdown-images.test.ts b/src/server/__tests__/markdown-images.test.ts new file mode 100644 index 00000000..f50d625a --- /dev/null +++ b/src/server/__tests__/markdown-images.test.ts @@ -0,0 +1,141 @@ +import { describe, expect, it } from 'bun:test' +import * as fs from 'node:fs/promises' +import * as os from 'node:os' +import * as path from 'node:path' +import { createSandboxedTestEnvironment } from '../../../scripts/pr/test-environment' + +// Bun caches os.homedir() on startup. Give each child its HOME before imports; +// changing process.env.HOME in a test would still exercise the original home. +async function withTemporaryHome(scenario: string): Promise { + const fixture = await fs.mkdtemp(path.join(os.tmpdir(), 'qa003-markdown-images-')) + const temporaryHome = path.join(fixture, 'home', 'user') + await fs.mkdir(temporaryHome, { recursive: true }) + const setup = ` + import assert from 'node:assert/strict' + import * as fs from 'node:fs/promises' + import * as os from 'node:os' + import * as path from 'node:path' + import { pathToFileURL } from 'node:url' + import { createAssistantMarkdownImageResolver, normalizeMarkdownImageDestination } from './desktop/src/lib/markdownImages' + import { handleFilesystemRoute } from './src/server/api/filesystem' + import { registerFilesystemAccessRoot } from './src/server/services/filesystemAccessRoots' + const fixture = ${JSON.stringify(fixture)} + assert.equal(os.homedir(), ${JSON.stringify(temporaryHome)}) + const PNG = Buffer.from('89504e470d0a1a0a0000000d4948445200000001000000010802000000907753de0000000c49444154789c63606060000000040001f61738550000000049454e44ae426082', 'hex') + await fs.mkdir(path.join(os.homedir(), 'Pictures', '测试 pics'), { recursive: true }) + await fs.writeFile(path.join(os.homedir(), 'Pictures', '测试 pics', 'sample.png'), PNG) + await fs.mkdir(path.join(fixture, 'tmp')) + await fs.writeFile(path.join(fixture, 'tmp', 'sample.png'), PNG) + registerFilesystemAccessRoot(fixture) + const resolve = createAssistantMarkdownImageResolver({ + baseUrl: 'http://localhost:3456', sessionId: 'qa003', workDir: path.join(fixture, 'workspace'), + }) + function imageUrl(destination) { + const src = resolve(normalizeMarkdownImageDestination(destination)) + // QA-003 dropped the image here, before serving/authorization. + assert.notEqual(src, null, destination) + const url = new URL(src) + assert.equal(url.pathname, '/api/filesystem/file') + return url + } + async function serve(destination) { + const url = imageUrl(destination) + return handleFilesystemRoute(url.pathname, url) + } + ` + try { + const proc = Bun.spawn(['bun', '--no-env-file', '-e', setup + scenario], { + cwd: path.resolve(import.meta.dir, '../../..'), + env: createSandboxedTestEnvironment(temporaryHome), + stdout: 'pipe', + stderr: 'pipe', + }) + const [stdout, stderr, exitCode] = await Promise.all([ + new Response(proc.stdout).text(), new Response(proc.stderr).text(), proc.exited, + ]) + expect(exitCode, stdout + stderr).toBe(0) + } finally { + await fs.rm(fixture, { recursive: true, force: true }) + } +} + +describe('assistant Markdown image paths → filesystem authorization', () => { + it('serves a normal home subdirectory, its absolute path and file URL with encoded names', async () => { + await withTemporaryHome(` + const absolute = path.join(os.homedir(), 'Pictures', '测试 pics', 'sample.png') + for (const destination of [ + '~/Pictures/%E6%B5%8B%E8%AF%95%20pics/sample.png', + absolute.split(path.sep).map(encodeURIComponent).join('/'), + pathToFileURL(absolute).href, + ]) { + const response = await serve(destination) + assert.equal(response.status, 200) + assert.equal(response.headers.get('Content-Type'), 'image/png') + assert.deepEqual(Buffer.from(await response.arrayBuffer()), PNG) + } + `) + }) + + it('serves the original ~/../../tmp shape after expanding a temporary HOME', async () => { + await withTemporaryHome(` + const response = await serve('~/../../tmp/sample.png') + assert.equal(response.status, 200) + assert.deepEqual(Buffer.from(await response.arrayBuffer()), PNG) + `) + }) + + it('keeps registered-root and canonical symlink checks after home expansion', async () => { + if (process.platform === 'win32') return + await withTemporaryHome(` + const external = await fs.mkdtemp('/var/tmp/qa003-markdown-images-') + try { + const allowed = path.join(external, 'allowed') + await fs.mkdir(allowed) + const image = path.join(allowed, 'sample.png') + const secret = path.join(external, 'secret.png') + await fs.writeFile(image, PNG) + await fs.writeFile(secret, PNG) + await fs.symlink(secret, path.join(allowed, 'escape.png')) + const homePath = (target) => '~/' + path.relative(os.homedir(), target) + assert.equal((await serve(homePath(image))).status, 403) + registerFilesystemAccessRoot(allowed) + assert.equal((await serve(homePath(image))).status, 200) + assert.equal((await serve(homePath(secret))).status, 403) + assert.equal((await serve(homePath(path.join(allowed, 'escape.png')))).status, 403) + } finally { + await fs.rm(external, { recursive: true, force: true }) + } + `) + }) + + it('decodes once and preserves missing-file and type refusals', async () => { + await withTemporaryHome(` + await fs.writeFile(path.join(os.homedir(), 'Pictures', 'a%20b.png'), PNG) + assert.equal((await serve('~/Pictures/a%2520b.png')).status, 200) + assert.equal((await serve('~/Pictures/missing.png')).status, 404) + assert.equal(resolve('~/Pictures/note.txt'), null) + `) + }) + + it('keeps home images behind H5 pairing and serves them through the authenticated boundary', async () => { + await withTemporaryHome(` + const { shouldRequireH5Token } = await import('./src/server/h5AccessPolicy') + const { requireH5Token } = await import('./src/server/middleware/auth') + const { H5AccessService } = await import('./src/server/services/h5AccessService') + const { token } = await new H5AccessService().enable() + for (const destination of ['~/Pictures/测试%20pics/sample.png', '~/../../tmp/sample.png']) { + const url = imageUrl(destination) + const request = new Request(url, { headers: { Origin: 'https://paired-phone.invalid' } }) + assert.equal(shouldRequireH5Token({ + request, url, h5Enabled: true, context: { clientAddress: '192.0.2.44' }, + }), true) + assert.equal((await requireH5Token(request)).status, 401) + request.headers.set('Authorization', 'Bearer invalid-fixture-token') + assert.equal((await requireH5Token(request)).status, 401) + request.headers.set('Authorization', 'Bearer ' + token) + assert.equal(await requireH5Token(request), null) + assert.equal((await handleFilesystemRoute(url.pathname, url)).status, 200) + } + `) + }) +})