From 1efc7fb570d22de76810d530fa43c585d4cd7a7e Mon Sep 17 00:00:00 2001 From: Bohdan Triapitsyn Date: Tue, 18 Aug 2026 19:15:11 +0300 Subject: [PATCH] fix(fs): open files through workspace symlinks --- packages/vscode/src/DOCUMENTATION.md | 1 + .../vscode/src/bridge-fs-helpers-runtime.ts | 8 ++--- packages/web/server/lib/fs/DOCUMENTATION.md | 1 + packages/web/server/lib/fs/routes.js | 36 +++---------------- packages/web/server/lib/fs/routes.test.js | 18 ++++++++++ 5 files changed, 26 insertions(+), 38 deletions(-) diff --git a/packages/vscode/src/DOCUMENTATION.md b/packages/vscode/src/DOCUMENTATION.md index e3447e21..72823844 100644 --- a/packages/vscode/src/DOCUMENTATION.md +++ b/packages/vscode/src/DOCUMENTATION.md @@ -40,6 +40,7 @@ Keep `bridge.ts` as a thin orchestration layer that delegates message handling t - active-directory selection across multi-root workspaces - dropped-file parsing and attachment reading - models metadata fetch helper + - Read paths are authorized in the requested workspace path space before symlink resolution, matching the web runtime; directly requested outside-workspace paths remain denied. The webview CSP permits `blob:` only for `worker-src` so shared UI parsers can run bounded local decompression off the main thread. Blob scripts remain disallowed by `script-src`. diff --git a/packages/vscode/src/bridge-fs-helpers-runtime.ts b/packages/vscode/src/bridge-fs-helpers-runtime.ts index b304778d..d2017405 100644 --- a/packages/vscode/src/bridge-fs-helpers-runtime.ts +++ b/packages/vscode/src/bridge-fs-helpers-runtime.ts @@ -583,12 +583,8 @@ export const resolveFileReadPath = async (targetPath: string, requestedRoot?: st } try { - const [canonicalPath, canonicalBase] = await Promise.all([ - fs.promises.realpath(resolved), - fs.promises.realpath(baseRoot).catch(() => path.resolve(baseRoot)), - ]); - - if (!isPathInside(canonicalPath, canonicalBase)) { + const canonicalPath = await fs.promises.realpath(resolved); + if (!isPathInside(resolved, path.resolve(baseRoot))) { return { ok: false, status: 403, error: 'Access to file denied' }; } diff --git a/packages/web/server/lib/fs/DOCUMENTATION.md b/packages/web/server/lib/fs/DOCUMENTATION.md index 4629135b..f184e8c1 100644 --- a/packages/web/server/lib/fs/DOCUMENTATION.md +++ b/packages/web/server/lib/fs/DOCUMENTATION.md @@ -35,5 +35,6 @@ Own filesystem API behavior for the web server runtime, including workspace-boun ## Notes for contributors - Keep filesystem policy (workspace root checks, error mapping, exec timeout behavior) inside this module, not in the composition root. - Filesystem `EPERM`/`EACCES` failures use the stable `reason: "os-permission"` response marker. Policy denials such as workspace-boundary or missing-grant failures must not use that marker because a native folder picker cannot remediate them. +- Read-only routes authorize the requested path against the workspace before resolving symlinks. A symlink reached through the workspace may therefore target a file outside it, while a directly requested outside path still requires an exact-path grant. Write routes keep canonical-target boundary checks. - If adding new `/api/fs/*` endpoints, add them in `routes.js` and extend this document. - `GET /api/fs/list` may resolve symlinks with `realpath` to read directory contents, but the response `path` and each entry `path` must stay in the caller's requested path space (`path.join(requestedPath, name)`). Returning real paths breaks file-tree expansion for directories reached through workspace symlinks. diff --git a/packages/web/server/lib/fs/routes.js b/packages/web/server/lib/fs/routes.js index 10842ef3..20cc32b7 100644 --- a/packages/web/server/lib/fs/routes.js +++ b/packages/web/server/lib/fs/routes.js @@ -758,14 +758,7 @@ export const registerFsRoutes = (app, dependencies) => { return res.status(400).json({ error: resolved.error }); } - const [canonicalPath, canonicalBase] = await Promise.all([ - fsPromises.realpath(resolved.resolved), - fsPromises.realpath(resolved.base).catch(() => path.resolve(resolved.base)), - ]); - - if (!isPathWithinRoot(canonicalPath, canonicalBase, path, os)) { - return res.status(403).json({ error: 'Access to file denied' }); - } + const canonicalPath = await fsPromises.realpath(resolved.resolved); const stats = await fsPromises.stat(canonicalPath); if (!stats.isFile()) { @@ -815,14 +808,7 @@ export const registerFsRoutes = (app, dependencies) => { return res.status(400).json({ error: resolved.error }); } - const [canonicalPath, canonicalBase] = await Promise.all([ - fsPromises.realpath(resolved.resolved), - fsPromises.realpath(resolved.base).catch(() => path.resolve(resolved.base)), - ]); - - if (!isPathWithinRoot(canonicalPath, canonicalBase, path, os)) { - return res.status(403).json({ error: 'Access to file denied' }); - } + const canonicalPath = await fsPromises.realpath(resolved.resolved); const stats = await fsPromises.stat(canonicalPath); if (!stats.isFile()) { @@ -886,14 +872,7 @@ export const registerFsRoutes = (app, dependencies) => { return res.status(400).json({ error: resolved.error }); } - const [canonicalPath, canonicalBase] = await Promise.all([ - fsPromises.realpath(resolved.resolved), - fsPromises.realpath(resolved.base).catch(() => path.resolve(resolved.base)), - ]); - - if (!isPathWithinRoot(canonicalPath, canonicalBase, path, os)) { - return res.status(403).json({ error: 'Access to file denied' }); - } + const canonicalPath = await fsPromises.realpath(resolved.resolved); const stats = await fsPromises.stat(canonicalPath); if (!stats.isFile()) { @@ -971,14 +950,7 @@ export const registerFsRoutes = (app, dependencies) => { return res.status(400).json({ error: resolved.error }); } - const [canonicalPath, canonicalBase] = await Promise.all([ - fsPromises.realpath(resolved.resolved), - fsPromises.realpath(resolved.base).catch(() => path.resolve(resolved.base)), - ]); - - if (!isPathWithinRoot(canonicalPath, canonicalBase, path, os)) { - return res.status(403).json({ error: 'Access to file denied' }); - } + const canonicalPath = await fsPromises.realpath(resolved.resolved); const stats = await fsPromises.stat(canonicalPath); if (!stats.isFile()) { diff --git a/packages/web/server/lib/fs/routes.test.js b/packages/web/server/lib/fs/routes.test.js index 1112e333..b238c559 100644 --- a/packages/web/server/lib/fs/routes.test.js +++ b/packages/web/server/lib/fs/routes.test.js @@ -341,6 +341,24 @@ describe('fs write', () => { }); describe('fs read', () => { + it('reads workspace files through symlinks that resolve outside the workspace', async () => { + const fsPromises = { + realpath: vi.fn(async (targetPath) => { + if (targetPath === '/repo/link.txt') return '/shared/target.txt'; + return targetPath; + }), + stat: vi.fn(async () => ({ isFile: () => true, size: 6 })), + readFile: vi.fn(async () => 'shared'), + }; + const handler = registerRead(fsPromises); + + const res = await callRead(handler, { path: '/repo/link.txt' }); + + expect(res.statusCode).toBe(200); + expect(res.body).toBe('shared'); + expect(fsPromises.readFile).toHaveBeenCalledWith('/shared/target.txt', 'utf8'); + }); + it('rejects outside workspace reads without a grant', async () => { const warn = vi.spyOn(console, 'warn').mockImplementation(() => {}); const fsPromises = {