From 7bbc587d4c97b7ff02dd261cd07a412f7207529f Mon Sep 17 00:00:00 2001 From: "Anthony Fu (via agent)" Date: Tue, 1 Sep 2026 06:18:52 +0000 Subject: [PATCH 1/4] fix: enforce symlink-aware filesystem roots --- .../devframe/src/utils/serve-static.test.ts | 63 ++++++++++- packages/devframe/src/utils/serve-static.ts | 51 +++++++-- plans/README.md | 2 +- plugins/assets/src/node/context.ts | 17 ++- plugins/assets/src/node/paths.ts | 103 +++++++++++++++++- plugins/assets/src/node/scanner.ts | 8 +- plugins/assets/src/rpc/functions/delete.ts | 3 +- plugins/assets/src/rpc/functions/mkdir.ts | 5 +- .../src/rpc/functions/read-image-meta.ts | 2 +- plugins/assets/src/rpc/functions/read-text.ts | 2 +- plugins/assets/src/rpc/functions/rename.ts | 10 +- plugins/assets/src/rpc/functions/upload.ts | 5 +- plugins/assets/test/assets.test.ts | 74 +++++++++++++ services/open/src/index.ts | 46 ++++++-- services/open/test/service.test.ts | 32 +++++- .../plugin-assets/node.snapshot.d.ts | 2 + 16 files changed, 393 insertions(+), 32 deletions(-) diff --git a/packages/devframe/src/utils/serve-static.test.ts b/packages/devframe/src/utils/serve-static.test.ts index 24ed9b08e..738231e55 100644 --- a/packages/devframe/src/utils/serve-static.test.ts +++ b/packages/devframe/src/utils/serve-static.test.ts @@ -1,9 +1,10 @@ import type { AddressInfo } from 'node:net' import type { ServeStaticOptions } from './serve-static' -import { mkdirSync, mkdtempSync, writeFileSync } from 'node:fs' +import { mkdirSync, mkdtempSync, symlinkSync, writeFileSync } from 'node:fs' import { createServer } from 'node:http' import { tmpdir } from 'node:os' import { join } from 'node:path' +import process from 'node:process' import { H3, toNodeHandler } from 'h3' import { afterEach, describe, expect, it } from 'vitest' import { mountStaticHandler, serveStaticHandler, serveStaticNodeMiddleware } from './serve-static' @@ -216,6 +217,66 @@ describe('mountStaticHandler', () => { }) }) +// Symlinks require privileges on Windows that hosted CI runners lack, so +// gate the symlink-containment suite off that platform. +describe.skipIf(process.platform === 'win32')('serveStaticHandler symlink containment', () => { + let fx: Fixture | undefined + + afterEach(async () => { + await fx?.close() + fx = undefined + }) + + it('returns 404 for a file symlink escaping the served root', async () => { + const dir = makeTmp('devframe-serve-link-') + const outside = makeTmp('devframe-serve-outside-') + writeFileSync(join(outside, 'secret.txt'), 'top secret', 'utf-8') + symlinkSync(join(outside, 'secret.txt'), join(dir, 'leak.txt')) + writeFileSync(join(dir, 'ok.txt'), 'in root', 'utf-8') + fx = await startH3(dir, { single: false }) + + const leak = await fetch(`${fx.baseUrl}/leak.txt`) + expect(leak.status).toBe(404) + // Ordinary in-root files still serve. + const ok = await fetch(`${fx.baseUrl}/ok.txt`) + expect(ok.status).toBe(200) + expect(await ok.text()).toBe('in root') + }) + + it('returns 404 for a file reached through an escaping directory symlink', async () => { + const dir = makeTmp('devframe-serve-link-') + const outside = makeTmp('devframe-serve-outside-') + writeFileSync(join(outside, 'secret.txt'), 'top secret', 'utf-8') + symlinkSync(outside, join(dir, 'escape')) + fx = await startH3(dir, { single: false }) + + const res = await fetch(`${fx.baseUrl}/escape/secret.txt`) + expect(res.status).toBe(404) + }) + + it('serves a symlink whose canonical target stays inside the served root', async () => { + const dir = makeTmp('devframe-serve-link-') + writeFileSync(join(dir, 'real.txt'), 'contained', 'utf-8') + symlinkSync(join(dir, 'real.txt'), join(dir, 'alias.txt')) + fx = await startH3(dir, { single: false }) + + const res = await fetch(`${fx.baseUrl}/alias.txt`) + expect(res.status).toBe(200) + expect(await res.text()).toBe('contained') + }) + + it('returns 404 through the Node middleware for an escaping symlink', async () => { + const dir = makeTmp('devframe-serve-link-') + const outside = makeTmp('devframe-serve-outside-') + writeFileSync(join(outside, 'secret.txt'), 'top secret', 'utf-8') + symlinkSync(join(outside, 'secret.txt'), join(dir, 'leak.txt')) + fx = await startMw(dir, { single: false }) + + const res = await fetch(`${fx.baseUrl}/leak.txt`) + expect(res.status).toBe(404) + }) +}) + describe('serveStaticNodeMiddleware', () => { let fx: Fixture | undefined diff --git a/packages/devframe/src/utils/serve-static.ts b/packages/devframe/src/utils/serve-static.ts index c24e698bd..be15d6f23 100644 --- a/packages/devframe/src/utils/serve-static.ts +++ b/packages/devframe/src/utils/serve-static.ts @@ -3,7 +3,7 @@ import type { IncomingMessage, ServerResponse } from 'node:http' import type { ReadableStream as NodeWebReadableStream } from 'node:stream/web' import type { RemoteAssetsErrorMessage, RemoteAssetsStore } from '../types/remote-assets' import { createReadStream } from 'node:fs' -import { stat } from 'node:fs/promises' +import { realpath, stat } from 'node:fs/promises' import { Readable } from 'node:stream' import { defineHandler, H3 } from 'h3' import { lookup } from 'mrmime' @@ -31,11 +31,39 @@ interface ResolvedFile { const HTML_EXTENSIONS = ['.html', '.htm'] -async function statFile(abs: string): Promise { +/** `child === root` or a path nested beneath it, using pathe's `/` separator. */ +function isWithin(child: string, root: string): boolean { + return child === root || child.startsWith(root + sep) +} + +/** + * The canonical (symlink-resolved) served root. Falls back to the lexical + * path when the directory does not exist yet so an empty deployment simply + * serves nothing rather than throwing. + */ +async function canonicalRoot(absDir: string): Promise { + try { + return normalize(await realpath(absDir)) + } + catch { + return absDir + } +} + +/** + * Stat a candidate file and confirm its canonical target stays inside the + * canonical served root, so a symlink inside the root can only resolve to a + * file that is still within the root. A symlink escaping the root reads as a + * miss (`null`), not a leak. + */ +async function statFile(abs: string, realRoot: string): Promise { try { const s = await stat(abs) if (!s.isFile()) return null + const real = normalize(await realpath(abs)) + if (!isWithin(real, realRoot)) + return null return { abs, size: s.size, mtime: s.mtime } } catch { @@ -45,6 +73,7 @@ async function statFile(abs: string): Promise { async function resolveTarget( absDir: string, + realRoot: string, urlPath: string, indexNames: string[], single: boolean, @@ -67,7 +96,7 @@ async function resolveTarget( if (abs !== absDir && !abs.startsWith(absDir + sep)) return null - const direct = await statFile(abs) + const direct = await statFile(abs, realRoot) if (direct) return direct @@ -75,7 +104,7 @@ async function resolveTarget( const s = await stat(abs) if (s.isDirectory()) { for (const name of indexNames) { - const candidate = await statFile(join(abs, name)) + const candidate = await statFile(join(abs, name), realRoot) if (candidate) return candidate } @@ -90,7 +119,7 @@ async function resolveTarget( // fallback so pretty-URL deployments resolve to the right page. if (!extname(cleaned)) { for (const ext of HTML_EXTENSIONS) { - const candidate = await statFile(abs + ext) + const candidate = await statFile(abs + ext, realRoot) if (candidate) return candidate } @@ -98,7 +127,7 @@ async function resolveTarget( const fallbackIndex = indexNames[0] if (single && fallbackIndex && !/\.[a-z0-9]+$/i.test(cleaned)) { - const indexFile = await statFile(join(absDir, fallbackIndex)) + const indexFile = await statFile(join(absDir, fallbackIndex), realRoot) if (indexFile) return indexFile } @@ -199,6 +228,10 @@ export function serveStaticHandler( return serveRemoteAssetsHandler(source) const absDir = resolve(source) const opts = normalizeOptions(options) + // Canonicalize the served root once and reuse it — the containment check + // compares every candidate's canonical path against this. + let realRootPromise: Promise | undefined + const getRealRoot = (): Promise => (realRootPromise ??= canonicalRoot(absDir)) return defineHandler(async (event) => { const method = event.req.method if (method !== 'GET' && method !== 'HEAD') { @@ -206,7 +239,7 @@ export function serveStaticHandler( event.res.headers.set('Allow', 'GET, HEAD') return '' } - const file = await resolveTarget(absDir, event.url.pathname, opts.indexNames, opts.single) + const file = await resolveTarget(absDir, await getRealRoot(), event.url.pathname, opts.indexNames, opts.single) if (!file) { event.res.status = 404 return '' @@ -250,6 +283,8 @@ export function serveStaticNodeMiddleware( ): (req: IncomingMessage, res: ServerResponse, next?: (err?: Error) => void) => void { const absDir = typeof source === 'string' ? resolve(source) : undefined const opts = normalizeOptions(options) + let realRootPromise: Promise | undefined + const getRealRoot = (dir: string): Promise => (realRootPromise ??= canonicalRoot(dir)) return (req, res, next) => { void (async () => { const method = req.method @@ -282,7 +317,7 @@ export function serveStaticNodeMiddleware( return } - const file = await resolveTarget(absDir, url, opts.indexNames, opts.single) + const file = await resolveTarget(absDir, await getRealRoot(absDir), url, opts.indexNames, opts.single) if (!file) { if (next) { next() diff --git a/plans/README.md b/plans/README.md index c263bd731..8356e1a5a 100644 --- a/plans/README.md +++ b/plans/README.md @@ -12,7 +12,7 @@ Generated by the improve skill on 2026-09-01 at commit `2d978f84`. Execute in th | 004 | Contain remote asset materialization | P1 | S | - | TODO | | 005 | Block Data Inspector prototype-chain writes | P1 | S | - | TODO | | 006 | Validate request-derived authentication-link origins | P1 | M | - | TODO | -| 007 | Reject pre-existing symlink escapes from filesystem roots | P2 | M | - | TODO | +| 007 | Reject pre-existing symlink escapes from filesystem roots | P2 | M | - | DONE | Status values: TODO | IN PROGRESS | DONE | BLOCKED (with reason) | REJECTED (with rationale) diff --git a/plugins/assets/src/node/context.ts b/plugins/assets/src/node/context.ts index 23b47f107..0b5e7f917 100644 --- a/plugins/assets/src/node/context.ts +++ b/plugins/assets/src/node/context.ts @@ -1,5 +1,5 @@ import type { DevframeNodeContext, RpcStreamingChannel } from 'devframe' -import { resolveAssetPath } from './paths' +import { assertAssetMutationPath, resolveAssetPath, resolveAssetReadPath } from './paths' export interface AssetsConfig { /** Directory this devframe manages. */ @@ -19,8 +19,19 @@ export interface AssetsConfig { } export interface AssetsContext extends AssetsConfig { - /** Resolve a root-relative path to an absolute one, rejecting escapes. */ + /** Resolve a root-relative path to an absolute one, rejecting lexical escapes. */ resolvePath: (relativePath: string) => string + /** + * Resolve a path for a read, allowing an in-root symlink only when its + * canonical target stays inside the managed root. + */ + resolveReadPath: (relativePath: string) => Promise + /** + * Resolve a path for a mutation, rejecting every pre-existing symlink + * component. Call again after creating directories and right before the + * mutating I/O. + */ + assertMutationPath: (relativePath: string) => Promise } const configs = new WeakMap() @@ -54,6 +65,8 @@ export function getAssetsContext(ctx: DevframeNodeContext): AssetsContext { baseURL: config?.baseURL ?? '/', uploadChannel: config?.uploadChannel, resolvePath: (relativePath: string) => resolveAssetPath(dir, relativePath), + resolveReadPath: (relativePath: string) => resolveAssetReadPath(dir, relativePath), + assertMutationPath: (relativePath: string) => assertAssetMutationPath(dir, relativePath), } contexts.set(ctx, built) return built diff --git a/plugins/assets/src/node/paths.ts b/plugins/assets/src/node/paths.ts index 1e3c480d3..107323dae 100644 --- a/plugins/assets/src/node/paths.ts +++ b/plugins/assets/src/node/paths.ts @@ -1,11 +1,17 @@ -import { resolve } from 'pathe' +import fsp from 'node:fs/promises' +import { dirname, normalize, resolve } from 'pathe' import { diagnostics } from '../diagnostics' /** * Resolve a client-supplied, root-relative path against the managed * directory, rejecting anything that would escape it (`..` traversal, a - * rogue absolute path, etc.). Every RPC handler that touches the - * filesystem goes through this — never trust a path from the wire. + * rogue absolute path, etc.). This is the lexical guard every RPC handler + * that touches the filesystem goes through first — never trust a path from + * the wire. + * + * Lexical checks alone cannot see symlinks: use {@link resolveAssetReadPath} + * (reads) or {@link assertAssetMutationPath} (mutations) to also close + * pre-existing symlink escapes. */ export function resolveAssetPath(root: string, relativePath: string): string { const cleaned = relativePath.replace(/^[/\\]+/, '') @@ -15,3 +21,94 @@ export function resolveAssetPath(root: string, relativePath: string): string { throw diagnostics.DP_ASSETS_0001({ path: relativePath }) return absolute } + +/** `child === root` or a path nested beneath it, using pathe's `/` separator. */ +function isWithin(child: string, root: string): boolean { + return child === root || child.startsWith(`${root}/`) +} + +/** + * The canonical (symlink-resolved) managed root. Falls back to the lexical + * path when the directory does not exist yet. + */ +async function canonicalRoot(root: string): Promise { + const normalizedRoot = resolve(root) + try { + return normalize(await fsp.realpath(normalizedRoot)) + } + catch { + return normalizedRoot + } +} + +/** + * Canonical path of the nearest existing ancestor of `absolute` (the target + * itself when it exists), with every symlink along the way resolved. + */ +async function nearestExistingCanonical(absolute: string): Promise { + let current = absolute + for (;;) { + try { + return normalize(await fsp.realpath(current)) + } + catch { + const parent = dirname(current) + if (parent === current) + return current + current = parent + } + } +} + +/** + * Resolve a path for a **read**, allowing a symlink only when its canonical + * target stays inside the canonical managed root. Lexical escapes and + * symlinks whose canonical target leaves the root both throw + * `DP_ASSETS_0001`. + */ +export async function resolveAssetReadPath(root: string, relativePath: string): Promise { + const absolute = resolveAssetPath(root, relativePath) + const canonRoot = await canonicalRoot(root) + const nearest = await nearestExistingCanonical(absolute) + if (!isWithin(nearest, canonRoot)) + throw diagnostics.DP_ASSETS_0001({ path: relativePath }) + return absolute +} + +/** + * Resolve a path for a **mutation**, rejecting every pre-existing symlink + * among the path components from the managed root down to the target — + * including in-root symlinks — so a mutation can never follow a symlink out + * of (or around) the root. Walks only components that already exist, so it + * is safe for not-yet-created upload/mkdir targets; call it again after + * creating directories and immediately before the mutating I/O to re-check + * the freshly materialized components. + * + * This closes deterministic, pre-existing symlink escapes; it does not + * defeat a concurrent local process swapping a component between this check + * and the I/O. + */ +export async function assertAssetMutationPath(root: string, relativePath: string): Promise { + const absolute = resolveAssetPath(root, relativePath) + const canonRoot = await canonicalRoot(root) + const lexicalRoot = resolve(root) + const rel = absolute === lexicalRoot ? '' : absolute.slice(lexicalRoot.length + 1) + const segments = rel ? rel.split('/') : [] + + let current = canonRoot + for (const segment of segments) { + current = `${current}/${segment}` + let stat + try { + stat = await fsp.lstat(current) + } + catch { + // This component does not exist yet — nothing deeper can either, so + // there is no pre-existing symlink left to reject. + break + } + if (stat.isSymbolicLink()) + throw diagnostics.DP_ASSETS_0001({ path: relativePath }) + } + return absolute +} diff --git a/plugins/assets/src/node/scanner.ts b/plugins/assets/src/node/scanner.ts index 01b212373..e6979834b 100644 --- a/plugins/assets/src/node/scanner.ts +++ b/plugins/assets/src/node/scanner.ts @@ -50,11 +50,17 @@ export function statToAssetInfo(dir: string, baseURL: string, relPath: string, s /** Recursively lists every file under `dir`, sorted alphabetically by path. */ export async function scanAssets(dir: string, baseURL: string, includeFsPath = false): Promise { - const files = await glob(['**/*'], { cwd: dir, onlyFiles: true, dot: false }) + // Never traverse into or across symlinks — a symlink inside the managed + // directory must not expose files (or whole trees) that live outside it. + const files = await glob(['**/*'], { cwd: dir, onlyFiles: true, dot: false, followSymbolicLinks: false }) const infos = await Promise.all(files.map(async (relPath): Promise => { try { const stat = await fsp.lstat(join(dir, relPath)) + // `lstat` describes the link itself; drop any symlink entry so the + // listing only ever names real files contained in the root. + if (stat.isSymbolicLink()) + return undefined return statToAssetInfo(dir, baseURL, relPath, stat, includeFsPath) } catch { diff --git a/plugins/assets/src/rpc/functions/delete.ts b/plugins/assets/src/rpc/functions/delete.ts index 00812f7a3..5ddb4a378 100644 --- a/plugins/assets/src/rpc/functions/delete.ts +++ b/plugins/assets/src/rpc/functions/delete.ts @@ -26,7 +26,8 @@ export const deleteAssets = defineAssetsRpc({ handler: (async ({ paths }: { paths: string[] }): Promise<{ deleted: string[] }> => { const deleted: string[] = [] for (const path of paths) { - const absolute = assets.resolvePath(path) + // Reject any pre-existing symlink component right before unlinking. + const absolute = await assets.assertMutationPath(path) try { await fsp.unlink(absolute) deleted.push(path) diff --git a/plugins/assets/src/rpc/functions/mkdir.ts b/plugins/assets/src/rpc/functions/mkdir.ts index 026ac7c5f..73d48fef4 100644 --- a/plugins/assets/src/rpc/functions/mkdir.ts +++ b/plugins/assets/src/rpc/functions/mkdir.ts @@ -24,11 +24,14 @@ export const mkdir = defineAssetsRpc({ return { // See `list.ts` for why the async handler is cast. handler: (async ({ path }: { path: string }): Promise => { - const absolute = assets.resolvePath(path) + const absolute = await assets.assertMutationPath(path) const stat = await fsp.stat(absolute).catch(() => undefined) if (stat && !stat.isDirectory()) throw diagnostics.DP_ASSETS_0005({ path }) await fsp.mkdir(absolute, { recursive: true }) + // Re-check after creation: reject any symlink component that + // materialized under the root before anything follows this path. + await assets.assertMutationPath(path) }) as any, } }, diff --git a/plugins/assets/src/rpc/functions/read-image-meta.ts b/plugins/assets/src/rpc/functions/read-image-meta.ts index d4901bccf..25f72afa0 100644 --- a/plugins/assets/src/rpc/functions/read-image-meta.ts +++ b/plugins/assets/src/rpc/functions/read-image-meta.ts @@ -30,7 +30,7 @@ export const readImageMeta = defineAssetsRpc({ // See `list.ts` for why the async handler is cast. handler: (async (path: string): Promise => { try { - const buffer = await fsp.readFile(assets.resolvePath(path)) + const buffer = await fsp.readFile(await assets.resolveReadPath(path)) const meta = imageMeta(buffer) return { width: meta.width, height: meta.height, orientation: meta.orientation } } diff --git a/plugins/assets/src/rpc/functions/read-text.ts b/plugins/assets/src/rpc/functions/read-text.ts index 6363289e1..c65e2599e 100644 --- a/plugins/assets/src/rpc/functions/read-text.ts +++ b/plugins/assets/src/rpc/functions/read-text.ts @@ -26,7 +26,7 @@ export const readText = defineAssetsRpc({ // See `list.ts` for why the async handler is cast. handler: (async (path: string, limit: number = DEFAULT_LIMIT): Promise => { try { - const content = await fsp.readFile(assets.resolvePath(path), 'utf-8') + const content = await fsp.readFile(await assets.resolveReadPath(path), 'utf-8') return content.slice(0, limit) } catch { diff --git a/plugins/assets/src/rpc/functions/rename.ts b/plugins/assets/src/rpc/functions/rename.ts index 8abe2b259..4d5f0d4b1 100644 --- a/plugins/assets/src/rpc/functions/rename.ts +++ b/plugins/assets/src/rpc/functions/rename.ts @@ -47,8 +47,10 @@ export const rename = defineAssetsRpc({ const folder = dirname(path) const nextName = `${trimmed}${extname(path)}` const nextRelPath = folder === '.' ? nextName : `${folder}/${nextName}` - const from = assets.resolvePath(path) - const to = assets.resolvePath(nextRelPath) + // Reject a pre-existing symlink component on either side before we + // touch the filesystem. + const from = await assets.assertMutationPath(path) + const to = await assets.assertMutationPath(nextRelPath) if (from === to) { const stat = await fsp.lstat(from) @@ -60,6 +62,10 @@ export const rename = defineAssetsRpc({ throw diagnostics.DP_ASSETS_0003({ path: nextRelPath }) await fsp.mkdir(dirname(to), { recursive: true }) + // Re-check both sides after creating the destination's parents and + // immediately before the rename. + await assets.assertMutationPath(path) + await assets.assertMutationPath(nextRelPath) try { await fsp.rename(from, to) } diff --git a/plugins/assets/src/rpc/functions/upload.ts b/plugins/assets/src/rpc/functions/upload.ts index ab1a99eee..dcb028b3a 100644 --- a/plugins/assets/src/rpc/functions/upload.ts +++ b/plugins/assets/src/rpc/functions/upload.ts @@ -49,8 +49,11 @@ export const upload = defineAssetsRpc({ }) } - const absolute = assets.resolvePath(path) + const absolute = await assets.assertMutationPath(path) await fsp.mkdir(dirname(absolute), { recursive: true }) + // Re-check after the parent dirs are created and reject any + // pre-existing symlink component before we open the write stream. + await assets.assertMutationPath(path) const channel = assets.uploadChannel if (!channel) diff --git a/plugins/assets/test/assets.test.ts b/plugins/assets/test/assets.test.ts index 5a619de71..3059c3836 100644 --- a/plugins/assets/test/assets.test.ts +++ b/plugins/assets/test/assets.test.ts @@ -6,6 +6,8 @@ import process from 'node:process' import { afterAll, afterEach, beforeEach, describe, expect, it } from 'vitest' import { bootClient, call, cleanupTempDir, createTempDir, startAssetsServer } from './_utils' +const describeSymlinks = process.platform === 'win32' ? describe.skip : describe + // A minimal, valid 1x1 PNG. const ONE_PIXEL_PNG = Buffer.from( 'iVBORw0KGgoAAAANSUhEUgAAAAEAAAABAQAAAAA3bvkkAAAACklEQVR4nGNgAAIAAAUAAen63NgAAAAASUVORK5CYII=', @@ -146,6 +148,78 @@ describe('assets plugin', () => { ).rejects.toThrow(/outside the managed directory/) }) + describeSymlinks('symlink containment', () => { + let outside: string + + beforeEach(async () => { + outside = await createTempDir() + tempDirs.push(outside) + await fsp.writeFile(join(outside, 'secret.txt'), 'top secret', 'utf-8') + }) + + it('does not read through an escaping ancestor-directory symlink', async () => { + await fsp.symlink(outside, join(dir, 'escape')) + server = await startAssetsServer(dir, { watch: false }) + client = bootClient(server.port) + + await expect( + call(client, 'devframes:plugin:assets:read-text', 'escape/secret.txt'), + ).resolves.toBeNull() + }) + + it('reads a symlink whose canonical target stays inside the managed root', async () => { + await fsp.writeFile(join(dir, 'real.txt'), 'contained', 'utf-8') + await fsp.symlink(join(dir, 'real.txt'), join(dir, 'alias.txt')) + server = await startAssetsServer(dir, { watch: false }) + client = bootClient(server.port) + + await expect( + call(client, 'devframes:plugin:assets:read-text', 'alias.txt'), + ).resolves.toBe('contained') + }) + + it('omits symlink entries from the listing', async () => { + await fsp.writeFile(join(dir, 'real.txt'), 'x', 'utf-8') + await fsp.symlink(join(dir, 'real.txt'), join(dir, 'alias.txt')) + await fsp.symlink(outside, join(dir, 'escape')) + server = await startAssetsServer(dir, { watch: false }) + client = bootClient(server.port) + + const list = await call(client, 'devframes:plugin:assets:list') + expect(list.map((a: { path: string }) => a.path)).toEqual(['real.txt']) + }) + + it('rejects a mutation through an escaping ancestor-directory symlink', async () => { + await fsp.symlink(outside, join(dir, 'escape')) + server = await startAssetsServer(dir, { watch: false }) + client = bootClient(server.port) + + await expect( + call(client, 'devframes:plugin:assets:upload', { path: 'escape/evil.txt' }), + ).rejects.toThrow(/outside the managed directory/) + await expect( + call(client, 'devframes:plugin:assets:mkdir', { path: 'escape/sub' }), + ).rejects.toThrow(/outside the managed directory/) + await expect( + call(client, 'devframes:plugin:assets:delete', { paths: ['escape/secret.txt'] }), + ).rejects.toThrow(/outside the managed directory/) + }) + + it('rejects a mutation onto a symlink even when its target stays in-root', async () => { + await fsp.writeFile(join(dir, 'real.txt'), 'x', 'utf-8') + await fsp.symlink(join(dir, 'real.txt'), join(dir, 'alias.txt')) + server = await startAssetsServer(dir, { watch: false }) + client = bootClient(server.port) + + await expect( + call(client, 'devframes:plugin:assets:delete', { paths: ['alias.txt'] }), + ).rejects.toThrow(/outside the managed directory/) + await expect( + call(client, 'devframes:plugin:assets:rename', { path: 'alias.txt', newName: 'renamed' }), + ).rejects.toThrow(/outside the managed directory/) + }) + }) + it('rejects uploads with a disallowed extension', async () => { server = await startAssetsServer(dir, { uploadExtensions: ['png'], watch: false }) client = bootClient(server.port) diff --git a/services/open/src/index.ts b/services/open/src/index.ts index b4017f2e4..9c342da8f 100644 --- a/services/open/src/index.ts +++ b/services/open/src/index.ts @@ -1,12 +1,29 @@ import type { KnownEditor } from 'devframe/recipes/common-rpc-functions' import type { DevframeServiceDefinition } from 'devframe/types' +import { realpath } from 'node:fs/promises' import { defineRpcFunction } from 'devframe' import { KNOWN_EDITORS } from 'devframe/recipes/common-rpc-functions' import { s } from 'devframe/utils/simple-schema' -import { isAbsolute, relative, resolve } from 'pathe' +import { dirname, isAbsolute, normalize, relative, resolve } from 'pathe' import pkg from '../package.json' with { type: 'json' } import { diagnostics } from './diagnostics' +/** Canonical path of the nearest existing ancestor of `absolute`. */ +async function nearestExistingCanonical(absolute: string): Promise { + let current = absolute + for (;;) { + try { + return normalize(await realpath(current)) + } + catch { + const parent = dirname(current) + if (parent === current) + return current + current = parent + } + } +} + export const OPEN_SERVICE_PACKAGE = '@devframes/service-open' export const OPEN_SERVICE_SCOPE = 'devframes:service:open' @@ -79,14 +96,27 @@ export function createOpenService(options?: OpenServiceOptions): DevframeService setup(ctx, { options }) { const allowedRoots = [ctx.workspaceRoot, ...(options?.roots ?? [])].map(root => resolve(root)) + // Canonicalize the allowed roots once (resolving any symlink in the + // root paths themselves) so containment compares canonical to + // canonical. + let canonicalRootsPromise: Promise | undefined + const canonicalRoots = (): Promise => (canonicalRootsPromise ??= Promise.all( + allowedRoots.map(async root => nearestExistingCanonical(root)), + )) + /** - * Resolve `path` (relative paths against `workspaceRoot`) and assert it - * lands inside one of the allowed roots, or throw. + * Resolve `path` (relative paths against `workspaceRoot`) and assert + * its canonical location lands inside one of the allowed roots, or + * throw. Canonicalizing the nearest existing ancestor rejects a + * symlink that would redirect the open outside every allowed root, + * while still allowing not-yet-existing files under a root. */ - function assertAllowedPath(path: string): string { + async function assertAllowedPath(path: string): Promise { const resolved = isAbsolute(path) ? resolve(path) : resolve(ctx.workspaceRoot, path) - const contained = allowedRoots.some((root) => { - const rel = relative(root, resolved) + const roots = await canonicalRoots() + const canonical = await nearestExistingCanonical(resolved) + const contained = roots.some((root) => { + const rel = relative(root, canonical) return rel === '' || (!rel.startsWith('..') && !isAbsolute(rel)) }) if (!contained) @@ -96,7 +126,7 @@ export function createOpenService(options?: OpenServiceOptions): DevframeService const api: OpenServiceApi = { async openInEditor(input) { - const path = assertAllowedPath(input.path) + const path = await assertAllowedPath(input.path) const target = input.line != null ? `${path}:${input.line}${input.column != null ? `:${input.column}` : ''}` : path @@ -104,7 +134,7 @@ export function createOpenService(options?: OpenServiceOptions): DevframeService launchEditor(target, input.editor ?? options?.editor) }, async openInFinder(input) { - const path = assertAllowedPath(input.path) + const path = await assertAllowedPath(input.path) const { open } = await import('devframe/utils/open') await open(path) }, diff --git a/services/open/test/service.test.ts b/services/open/test/service.test.ts index ff1857c68..c444e3f4a 100644 --- a/services/open/test/service.test.ts +++ b/services/open/test/service.test.ts @@ -1,6 +1,7 @@ import type { DevframeHost } from 'devframe/types' -import { mkdtempSync, rmSync } from 'node:fs' +import { mkdtempSync, rmSync, symlinkSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' +import process from 'node:process' import { createHostContext } from 'devframe/node' // `pathe` (not `node:path`) so the expected paths use the same normalized // forward-slash form the service resolves to on every OS. @@ -99,6 +100,35 @@ describe('@devframes/service-open', () => { expect(launchEditor).toHaveBeenCalledWith(join(extra, 'b.ts'), 'zed') }) + it.skipIf(process.platform === 'win32')('refuses a symlink that escapes every allowed root', async () => { + const { ctx, dir } = await createCtx() + const outside = mkdtempSync(join(tmpdir(), 'devframe-service-open-outside-')) + tempDirs.push(outside) + writeFileSync(join(outside, 'secret.txt'), 'top secret', 'utf-8') + // A symlink inside the workspace root pointing at a file outside it. + symlinkSync(join(outside, 'secret.txt'), join(dir, 'leak.txt')) + + const install = ctx.services.install(createOpenService()) + await ctx.services.ready() + const api = await install + + await expect(api!.openInFinder({ path: join(dir, 'leak.txt') })).rejects.toThrowError(/outside the workspace root/) + expect(open).not.toHaveBeenCalled() + }) + + it.skipIf(process.platform === 'win32')('allows a symlink whose canonical target stays inside an allowed root', async () => { + const { ctx, dir } = await createCtx() + writeFileSync(join(dir, 'real.txt'), 'contained', 'utf-8') + symlinkSync(join(dir, 'real.txt'), join(dir, 'alias.txt')) + + const install = ctx.services.install(createOpenService()) + await ctx.services.ready() + const api = await install + + await api!.openInFinder({ path: join(dir, 'alias.txt') }) + expect(open).toHaveBeenCalledWith(join(dir, 'alias.txt')) + }) + it('rejects unknown editor commands at the RPC boundary', async () => { const { ctx, dir } = await createCtx() void ctx.services.install(createOpenService()) diff --git a/tests/__snapshots__/tsnapi/@devframes/plugin-assets/node.snapshot.d.ts b/tests/__snapshots__/tsnapi/@devframes/plugin-assets/node.snapshot.d.ts index bccb69ad3..09fb75f37 100644 --- a/tests/__snapshots__/tsnapi/@devframes/plugin-assets/node.snapshot.d.ts +++ b/tests/__snapshots__/tsnapi/@devframes/plugin-assets/node.snapshot.d.ts @@ -32,5 +32,7 @@ interface AssetsConfig { } interface AssetsContext extends AssetsConfig { resolvePath: (_: string) => string; + resolveReadPath: (_: string) => Promise; + assertMutationPath: (_: string) => Promise; } // #endregion \ No newline at end of file From 9f72642475cda94f09d31bf7c1a7668380d29f31 Mon Sep 17 00:00:00 2001 From: "Anthony Fu (via agent)" Date: Tue, 1 Sep 2026 06:29:17 +0000 Subject: [PATCH 2/4] refactor: keep symlink resolvers internal to asset handlers --- plugins/assets/src/node/context.ts | 20 ++++++------------- plugins/assets/src/rpc/functions/delete.ts | 3 ++- plugins/assets/src/rpc/functions/mkdir.ts | 5 +++-- .../src/rpc/functions/read-image-meta.ts | 3 ++- plugins/assets/src/rpc/functions/read-text.ts | 3 ++- plugins/assets/src/rpc/functions/rename.ts | 9 +++++---- plugins/assets/src/rpc/functions/upload.ts | 5 +++-- .../plugin-assets/node.snapshot.d.ts | 2 -- 8 files changed, 23 insertions(+), 27 deletions(-) diff --git a/plugins/assets/src/node/context.ts b/plugins/assets/src/node/context.ts index 0b5e7f917..8e1feda1a 100644 --- a/plugins/assets/src/node/context.ts +++ b/plugins/assets/src/node/context.ts @@ -1,5 +1,5 @@ import type { DevframeNodeContext, RpcStreamingChannel } from 'devframe' -import { assertAssetMutationPath, resolveAssetPath, resolveAssetReadPath } from './paths' +import { resolveAssetPath } from './paths' export interface AssetsConfig { /** Directory this devframe manages. */ @@ -19,19 +19,13 @@ export interface AssetsConfig { } export interface AssetsContext extends AssetsConfig { - /** Resolve a root-relative path to an absolute one, rejecting lexical escapes. */ - resolvePath: (relativePath: string) => string - /** - * Resolve a path for a read, allowing an in-root symlink only when its - * canonical target stays inside the managed root. - */ - resolveReadPath: (relativePath: string) => Promise /** - * Resolve a path for a mutation, rejecting every pre-existing symlink - * component. Call again after creating directories and right before the - * mutating I/O. + * Resolve a root-relative path to an absolute one, rejecting lexical + * escapes. Symlink-aware containment for reads and mutations lives in + * `node/paths` (`resolveAssetReadPath` / `assertAssetMutationPath`), which + * the RPC handlers call directly with {@link AssetsContext.dir}. */ - assertMutationPath: (relativePath: string) => Promise + resolvePath: (relativePath: string) => string } const configs = new WeakMap() @@ -65,8 +59,6 @@ export function getAssetsContext(ctx: DevframeNodeContext): AssetsContext { baseURL: config?.baseURL ?? '/', uploadChannel: config?.uploadChannel, resolvePath: (relativePath: string) => resolveAssetPath(dir, relativePath), - resolveReadPath: (relativePath: string) => resolveAssetReadPath(dir, relativePath), - assertMutationPath: (relativePath: string) => assertAssetMutationPath(dir, relativePath), } contexts.set(ctx, built) return built diff --git a/plugins/assets/src/rpc/functions/delete.ts b/plugins/assets/src/rpc/functions/delete.ts index 5ddb4a378..51d7c1a83 100644 --- a/plugins/assets/src/rpc/functions/delete.ts +++ b/plugins/assets/src/rpc/functions/delete.ts @@ -3,6 +3,7 @@ import fsp from 'node:fs/promises' import { createDefineWrapperWithContext } from 'devframe/rpc' import { s } from 'devframe/utils/simple-schema' import { getAssetsContext } from '../../node/context' +import { assertAssetMutationPath } from '../../node/paths' const defineAssetsRpc = createDefineWrapperWithContext() @@ -27,7 +28,7 @@ export const deleteAssets = defineAssetsRpc({ const deleted: string[] = [] for (const path of paths) { // Reject any pre-existing symlink component right before unlinking. - const absolute = await assets.assertMutationPath(path) + const absolute = await assertAssetMutationPath(assets.dir, path) try { await fsp.unlink(absolute) deleted.push(path) diff --git a/plugins/assets/src/rpc/functions/mkdir.ts b/plugins/assets/src/rpc/functions/mkdir.ts index 73d48fef4..12ff42854 100644 --- a/plugins/assets/src/rpc/functions/mkdir.ts +++ b/plugins/assets/src/rpc/functions/mkdir.ts @@ -4,6 +4,7 @@ import { createDefineWrapperWithContext } from 'devframe/rpc' import { s } from 'devframe/utils/simple-schema' import { diagnostics } from '../../diagnostics' import { getAssetsContext } from '../../node/context' +import { assertAssetMutationPath } from '../../node/paths' const defineAssetsRpc = createDefineWrapperWithContext() @@ -24,14 +25,14 @@ export const mkdir = defineAssetsRpc({ return { // See `list.ts` for why the async handler is cast. handler: (async ({ path }: { path: string }): Promise => { - const absolute = await assets.assertMutationPath(path) + const absolute = await assertAssetMutationPath(assets.dir, path) const stat = await fsp.stat(absolute).catch(() => undefined) if (stat && !stat.isDirectory()) throw diagnostics.DP_ASSETS_0005({ path }) await fsp.mkdir(absolute, { recursive: true }) // Re-check after creation: reject any symlink component that // materialized under the root before anything follows this path. - await assets.assertMutationPath(path) + await assertAssetMutationPath(assets.dir, path) }) as any, } }, diff --git a/plugins/assets/src/rpc/functions/read-image-meta.ts b/plugins/assets/src/rpc/functions/read-image-meta.ts index 25f72afa0..510fac06e 100644 --- a/plugins/assets/src/rpc/functions/read-image-meta.ts +++ b/plugins/assets/src/rpc/functions/read-image-meta.ts @@ -5,6 +5,7 @@ import { createDefineWrapperWithContext } from 'devframe/rpc' import { s } from 'devframe/utils/simple-schema' import { imageMeta } from 'image-meta' import { getAssetsContext } from '../../node/context' +import { resolveAssetReadPath } from '../../node/paths' const defineAssetsRpc = createDefineWrapperWithContext() @@ -30,7 +31,7 @@ export const readImageMeta = defineAssetsRpc({ // See `list.ts` for why the async handler is cast. handler: (async (path: string): Promise => { try { - const buffer = await fsp.readFile(await assets.resolveReadPath(path)) + const buffer = await fsp.readFile(await resolveAssetReadPath(assets.dir, path)) const meta = imageMeta(buffer) return { width: meta.width, height: meta.height, orientation: meta.orientation } } diff --git a/plugins/assets/src/rpc/functions/read-text.ts b/plugins/assets/src/rpc/functions/read-text.ts index c65e2599e..2a583f921 100644 --- a/plugins/assets/src/rpc/functions/read-text.ts +++ b/plugins/assets/src/rpc/functions/read-text.ts @@ -3,6 +3,7 @@ import fsp from 'node:fs/promises' import { createDefineWrapperWithContext } from 'devframe/rpc' import { s } from 'devframe/utils/simple-schema' import { getAssetsContext } from '../../node/context' +import { resolveAssetReadPath } from '../../node/paths' const defineAssetsRpc = createDefineWrapperWithContext() @@ -26,7 +27,7 @@ export const readText = defineAssetsRpc({ // See `list.ts` for why the async handler is cast. handler: (async (path: string, limit: number = DEFAULT_LIMIT): Promise => { try { - const content = await fsp.readFile(await assets.resolveReadPath(path), 'utf-8') + const content = await fsp.readFile(await resolveAssetReadPath(assets.dir, path), 'utf-8') return content.slice(0, limit) } catch { diff --git a/plugins/assets/src/rpc/functions/rename.ts b/plugins/assets/src/rpc/functions/rename.ts index 4d5f0d4b1..e2e2395c6 100644 --- a/plugins/assets/src/rpc/functions/rename.ts +++ b/plugins/assets/src/rpc/functions/rename.ts @@ -6,6 +6,7 @@ import { s } from 'devframe/utils/simple-schema' import { dirname, extname } from 'pathe' import { diagnostics } from '../../diagnostics' import { getAssetsContext } from '../../node/context' +import { assertAssetMutationPath } from '../../node/paths' import { statToAssetInfo } from '../../node/scanner' import { assetInfoSchema } from './list' @@ -49,8 +50,8 @@ export const rename = defineAssetsRpc({ const nextRelPath = folder === '.' ? nextName : `${folder}/${nextName}` // Reject a pre-existing symlink component on either side before we // touch the filesystem. - const from = await assets.assertMutationPath(path) - const to = await assets.assertMutationPath(nextRelPath) + const from = await assertAssetMutationPath(assets.dir, path) + const to = await assertAssetMutationPath(assets.dir, nextRelPath) if (from === to) { const stat = await fsp.lstat(from) @@ -64,8 +65,8 @@ export const rename = defineAssetsRpc({ await fsp.mkdir(dirname(to), { recursive: true }) // Re-check both sides after creating the destination's parents and // immediately before the rename. - await assets.assertMutationPath(path) - await assets.assertMutationPath(nextRelPath) + await assertAssetMutationPath(assets.dir, path) + await assertAssetMutationPath(assets.dir, nextRelPath) try { await fsp.rename(from, to) } diff --git a/plugins/assets/src/rpc/functions/upload.ts b/plugins/assets/src/rpc/functions/upload.ts index dcb028b3a..d7c253e95 100644 --- a/plugins/assets/src/rpc/functions/upload.ts +++ b/plugins/assets/src/rpc/functions/upload.ts @@ -6,6 +6,7 @@ import { s } from 'devframe/utils/simple-schema' import { dirname, extname } from 'pathe' import { diagnostics } from '../../diagnostics' import { getAssetsContext } from '../../node/context' +import { assertAssetMutationPath } from '../../node/paths' const defineAssetsRpc = createDefineWrapperWithContext() @@ -49,11 +50,11 @@ export const upload = defineAssetsRpc({ }) } - const absolute = await assets.assertMutationPath(path) + const absolute = await assertAssetMutationPath(assets.dir, path) await fsp.mkdir(dirname(absolute), { recursive: true }) // Re-check after the parent dirs are created and reject any // pre-existing symlink component before we open the write stream. - await assets.assertMutationPath(path) + await assertAssetMutationPath(assets.dir, path) const channel = assets.uploadChannel if (!channel) diff --git a/tests/__snapshots__/tsnapi/@devframes/plugin-assets/node.snapshot.d.ts b/tests/__snapshots__/tsnapi/@devframes/plugin-assets/node.snapshot.d.ts index 09fb75f37..bccb69ad3 100644 --- a/tests/__snapshots__/tsnapi/@devframes/plugin-assets/node.snapshot.d.ts +++ b/tests/__snapshots__/tsnapi/@devframes/plugin-assets/node.snapshot.d.ts @@ -32,7 +32,5 @@ interface AssetsConfig { } interface AssetsContext extends AssetsConfig { resolvePath: (_: string) => string; - resolveReadPath: (_: string) => Promise; - assertMutationPath: (_: string) => Promise; } // #endregion \ No newline at end of file From 5348f39240904828f6a8fd0c76ecc1977aeab5b5 Mon Sep 17 00:00:00 2001 From: "Anthony Fu (via agent)" Date: Tue, 1 Sep 2026 06:35:49 +0000 Subject: [PATCH 3/4] refactor: tighten symlink-containment helpers --- packages/devframe/src/utils/serve-static.ts | 43 +++----- plugins/assets/src/node/paths.ts | 115 ++++++-------------- services/open/src/index.ts | 23 ++-- 3 files changed, 57 insertions(+), 124 deletions(-) diff --git a/packages/devframe/src/utils/serve-static.ts b/packages/devframe/src/utils/serve-static.ts index be15d6f23..cfed61463 100644 --- a/packages/devframe/src/utils/serve-static.ts +++ b/packages/devframe/src/utils/serve-static.ts @@ -31,38 +31,25 @@ interface ResolvedFile { const HTML_EXTENSIONS = ['.html', '.htm'] -/** `child === root` or a path nested beneath it, using pathe's `/` separator. */ -function isWithin(child: string, root: string): boolean { - return child === root || child.startsWith(root + sep) -} - /** - * The canonical (symlink-resolved) served root. Falls back to the lexical - * path when the directory does not exist yet so an empty deployment simply - * serves nothing rather than throwing. + * The canonical (symlink-resolved) served root, falling back to the lexical + * path when the directory doesn't exist yet (an empty deployment then serves + * nothing rather than throwing). */ async function canonicalRoot(absDir: string): Promise { - try { - return normalize(await realpath(absDir)) - } - catch { - return absDir - } + return realpath(absDir).then(normalize, () => absDir) } /** - * Stat a candidate file and confirm its canonical target stays inside the - * canonical served root, so a symlink inside the root can only resolve to a - * file that is still within the root. A symlink escaping the root reads as a - * miss (`null`), not a leak. + * Stat a candidate file, confirming its canonical target stays inside the + * canonical served root — a symlink inside the root can only resolve to a + * file still within it; one escaping the root reads as a miss, not a leak. */ async function statFile(abs: string, realRoot: string): Promise { try { const s = await stat(abs) - if (!s.isFile()) - return null const real = normalize(await realpath(abs)) - if (!isWithin(real, realRoot)) + if (!s.isFile() || (real !== realRoot && !real.startsWith(realRoot + sep))) return null return { abs, size: s.size, mtime: s.mtime } } @@ -228,10 +215,9 @@ export function serveStaticHandler( return serveRemoteAssetsHandler(source) const absDir = resolve(source) const opts = normalizeOptions(options) - // Canonicalize the served root once and reuse it — the containment check - // compares every candidate's canonical path against this. - let realRootPromise: Promise | undefined - const getRealRoot = (): Promise => (realRootPromise ??= canonicalRoot(absDir)) + // Canonicalize the served root once; the containment check compares every + // candidate's canonical path against it. + const realRoot = canonicalRoot(absDir) return defineHandler(async (event) => { const method = event.req.method if (method !== 'GET' && method !== 'HEAD') { @@ -239,7 +225,7 @@ export function serveStaticHandler( event.res.headers.set('Allow', 'GET, HEAD') return '' } - const file = await resolveTarget(absDir, await getRealRoot(), event.url.pathname, opts.indexNames, opts.single) + const file = await resolveTarget(absDir, await realRoot, event.url.pathname, opts.indexNames, opts.single) if (!file) { event.res.status = 404 return '' @@ -283,8 +269,7 @@ export function serveStaticNodeMiddleware( ): (req: IncomingMessage, res: ServerResponse, next?: (err?: Error) => void) => void { const absDir = typeof source === 'string' ? resolve(source) : undefined const opts = normalizeOptions(options) - let realRootPromise: Promise | undefined - const getRealRoot = (dir: string): Promise => (realRootPromise ??= canonicalRoot(dir)) + const realRoot = absDir === undefined ? undefined : canonicalRoot(absDir) return (req, res, next) => { void (async () => { const method = req.method @@ -317,7 +302,7 @@ export function serveStaticNodeMiddleware( return } - const file = await resolveTarget(absDir, await getRealRoot(absDir), url, opts.indexNames, opts.single) + const file = await resolveTarget(absDir, await realRoot!, url, opts.indexNames, opts.single) if (!file) { if (next) { next() diff --git a/plugins/assets/src/node/paths.ts b/plugins/assets/src/node/paths.ts index 107323dae..df5290341 100644 --- a/plugins/assets/src/node/paths.ts +++ b/plugins/assets/src/node/paths.ts @@ -1,112 +1,65 @@ import fsp from 'node:fs/promises' -import { dirname, normalize, resolve } from 'pathe' +import { normalize, resolve } from 'pathe' import { diagnostics } from '../diagnostics' -/** - * Resolve a client-supplied, root-relative path against the managed - * directory, rejecting anything that would escape it (`..` traversal, a - * rogue absolute path, etc.). This is the lexical guard every RPC handler - * that touches the filesystem goes through first — never trust a path from - * the wire. - * - * Lexical checks alone cannot see symlinks: use {@link resolveAssetReadPath} - * (reads) or {@link assertAssetMutationPath} (mutations) to also close - * pre-existing symlink escapes. - */ -export function resolveAssetPath(root: string, relativePath: string): string { - const cleaned = relativePath.replace(/^[/\\]+/, '') - const normalizedRoot = resolve(root) - const absolute = resolve(normalizedRoot, cleaned) - if (absolute !== normalizedRoot && !absolute.startsWith(`${normalizedRoot}/`)) - throw diagnostics.DP_ASSETS_0001({ path: relativePath }) - return absolute -} - -/** `child === root` or a path nested beneath it, using pathe's `/` separator. */ -function isWithin(child: string, root: string): boolean { - return child === root || child.startsWith(`${root}/`) -} - -/** - * The canonical (symlink-resolved) managed root. Falls back to the lexical - * path when the directory does not exist yet. - */ -async function canonicalRoot(root: string): Promise { - const normalizedRoot = resolve(root) +/** realpath, pathe-normalized, or `null` when the path doesn't exist. */ +async function realpath(path: string): Promise { try { - return normalize(await fsp.realpath(normalizedRoot)) + return normalize(await fsp.realpath(path)) } catch { - return normalizedRoot + return null } } /** - * Canonical path of the nearest existing ancestor of `absolute` (the target - * itself when it exists), with every symlink along the way resolved. + * Resolve a client-supplied, root-relative path against the managed + * directory, rejecting anything that would escape it lexically (`..` + * traversal, a rogue absolute path). The first guard every RPC handler runs; + * symlink-aware containment is layered on by {@link resolveAssetReadPath} + * (reads) and {@link assertAssetMutationPath} (mutations). */ -async function nearestExistingCanonical(absolute: string): Promise { - let current = absolute - for (;;) { - try { - return normalize(await fsp.realpath(current)) - } - catch { - const parent = dirname(current) - if (parent === current) - return current - current = parent - } - } +export function resolveAssetPath(root: string, relativePath: string): string { + const normalizedRoot = resolve(root) + const absolute = resolve(normalizedRoot, relativePath.replace(/^[/\\]+/, '')) + if (absolute !== normalizedRoot && !absolute.startsWith(`${normalizedRoot}/`)) + throw diagnostics.DP_ASSETS_0001({ path: relativePath }) + return absolute } /** * Resolve a path for a **read**, allowing a symlink only when its canonical - * target stays inside the canonical managed root. Lexical escapes and - * symlinks whose canonical target leaves the root both throw - * `DP_ASSETS_0001`. + * target stays inside the canonical managed root. A target resolving outside + * throws `DP_ASSETS_0001`; a missing target is left for the caller's own read + * to fail. */ export async function resolveAssetReadPath(root: string, relativePath: string): Promise { const absolute = resolveAssetPath(root, relativePath) - const canonRoot = await canonicalRoot(root) - const nearest = await nearestExistingCanonical(absolute) - if (!isWithin(nearest, canonRoot)) + const real = await realpath(absolute) + const canonRoot = (await realpath(root)) ?? resolve(root) + if (real && real !== canonRoot && !real.startsWith(`${canonRoot}/`)) throw diagnostics.DP_ASSETS_0001({ path: relativePath }) return absolute } /** * Resolve a path for a **mutation**, rejecting every pre-existing symlink - * among the path components from the managed root down to the target — - * including in-root symlinks — so a mutation can never follow a symlink out - * of (or around) the root. Walks only components that already exist, so it - * is safe for not-yet-created upload/mkdir targets; call it again after - * creating directories and immediately before the mutating I/O to re-check - * the freshly materialized components. - * - * This closes deterministic, pre-existing symlink escapes; it does not - * defeat a concurrent local process swapping a component between this check - * and the I/O. + * among the path components from the managed root down to the target + * (including in-root symlinks) so a mutation can never follow a symlink out + * of, or around, the root. Only existing components are inspected, so it is + * safe for not-yet-created upload/mkdir targets — call it again after + * creating directories and right before the I/O. This closes deterministic, + * pre-existing symlink escapes, not concurrent component-swap races. */ export async function assertAssetMutationPath(root: string, relativePath: string): Promise { + const lexRoot = resolve(root) const absolute = resolveAssetPath(root, relativePath) - const canonRoot = await canonicalRoot(root) - const lexicalRoot = resolve(root) - const rel = absolute === lexicalRoot ? '' : absolute.slice(lexicalRoot.length + 1) - const segments = rel ? rel.split('/') : [] - - let current = canonRoot - for (const segment of segments) { - current = `${current}/${segment}` - let stat - try { - stat = await fsp.lstat(current) - } - catch { - // This component does not exist yet — nothing deeper can either, so - // there is no pre-existing symlink left to reject. + let current = (await realpath(root)) ?? lexRoot + for (const segment of absolute.slice(lexRoot.length).split('/').filter(Boolean)) { + current += `/${segment}` + const stat = await fsp.lstat(current).catch(() => null) + if (!stat) break - } if (stat.isSymbolicLink()) throw diagnostics.DP_ASSETS_0001({ path: relativePath }) } diff --git a/services/open/src/index.ts b/services/open/src/index.ts index 9c342da8f..ed880c3b6 100644 --- a/services/open/src/index.ts +++ b/services/open/src/index.ts @@ -94,28 +94,23 @@ export function createOpenService(options?: OpenServiceOptions): DevframeService // Option sets from multiple installers merge via devframe's default // deep-merge: `roots` union, `editor` last-wins. setup(ctx, { options }) { - const allowedRoots = [ctx.workspaceRoot, ...(options?.roots ?? [])].map(root => resolve(root)) - - // Canonicalize the allowed roots once (resolving any symlink in the - // root paths themselves) so containment compares canonical to - // canonical. - let canonicalRootsPromise: Promise | undefined - const canonicalRoots = (): Promise => (canonicalRootsPromise ??= Promise.all( - allowedRoots.map(async root => nearestExistingCanonical(root)), - )) + // Canonicalize each allowed root once (resolving symlinks in the root + // paths themselves) so containment compares canonical to canonical. + const allowedRoots = Promise.all( + [ctx.workspaceRoot, ...(options?.roots ?? [])].map(r => nearestExistingCanonical(resolve(r))), + ) /** * Resolve `path` (relative paths against `workspaceRoot`) and assert * its canonical location lands inside one of the allowed roots, or - * throw. Canonicalizing the nearest existing ancestor rejects a - * symlink that would redirect the open outside every allowed root, - * while still allowing not-yet-existing files under a root. + * throw. Canonicalizing the nearest existing ancestor rejects a symlink + * that would redirect the open outside every allowed root, while still + * allowing not-yet-existing files under a root. */ async function assertAllowedPath(path: string): Promise { const resolved = isAbsolute(path) ? resolve(path) : resolve(ctx.workspaceRoot, path) - const roots = await canonicalRoots() const canonical = await nearestExistingCanonical(resolved) - const contained = roots.some((root) => { + const contained = (await allowedRoots).some((root) => { const rel = relative(root, canonical) return rel === '' || (!rel.startsWith('..') && !isAbsolute(rel)) }) From db52cc5138515a98b772f810a259f970177aa0e4 Mon Sep 17 00:00:00 2001 From: "Anthony Fu (via agent)" Date: Tue, 1 Sep 2026 08:15:03 +0000 Subject: [PATCH 4/4] fix: keep lexical containment in open service allowed-root check --- services/open/src/index.ts | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/services/open/src/index.ts b/services/open/src/index.ts index ed880c3b6..777aee819 100644 --- a/services/open/src/index.ts +++ b/services/open/src/index.ts @@ -94,27 +94,26 @@ export function createOpenService(options?: OpenServiceOptions): DevframeService // Option sets from multiple installers merge via devframe's default // deep-merge: `roots` union, `editor` last-wins. setup(ctx, { options }) { - // Canonicalize each allowed root once (resolving symlinks in the root - // paths themselves) so containment compares canonical to canonical. - const allowedRoots = Promise.all( - [ctx.workspaceRoot, ...(options?.roots ?? [])].map(r => nearestExistingCanonical(resolve(r))), - ) + const allowedRoots = [ctx.workspaceRoot, ...(options?.roots ?? [])].map(r => resolve(r)) + // Canonical forms of the same roots (symlinks in the root paths + // resolved), computed once for the symlink-aware pass. + const canonicalRoots = Promise.all(allowedRoots.map(r => nearestExistingCanonical(r))) + + const within = (roots: string[], p: string): boolean => roots.some((root) => { + const rel = relative(root, p) + return rel === '' || (!rel.startsWith('..') && !isAbsolute(rel)) + }) /** - * Resolve `path` (relative paths against `workspaceRoot`) and assert - * its canonical location lands inside one of the allowed roots, or - * throw. Canonicalizing the nearest existing ancestor rejects a symlink - * that would redirect the open outside every allowed root, while still + * Resolve `path` (relative paths against `workspaceRoot`) and assert it + * lands inside one of the allowed roots, or throw. The lexical pass + * rejects plain `..`/absolute escapes; the canonical pass rejects a + * symlink that would redirect the open outside every root, while still * allowing not-yet-existing files under a root. */ async function assertAllowedPath(path: string): Promise { const resolved = isAbsolute(path) ? resolve(path) : resolve(ctx.workspaceRoot, path) - const canonical = await nearestExistingCanonical(resolved) - const contained = (await allowedRoots).some((root) => { - const rel = relative(root, canonical) - return rel === '' || (!rel.startsWith('..') && !isAbsolute(rel)) - }) - if (!contained) + if (!within(allowedRoots, resolved) || !within(await canonicalRoots, await nearestExistingCanonical(resolved))) throw diagnostics.DS_OPEN_0002({ path }) return resolved }