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..cfed61463 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,10 +31,25 @@ interface ResolvedFile { const HTML_EXTENSIONS = ['.html', '.htm'] -async function statFile(abs: string): Promise { +/** + * 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 { + return realpath(absDir).then(normalize, () => absDir) +} + +/** + * 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()) + const real = normalize(await realpath(abs)) + if (!s.isFile() || (real !== realRoot && !real.startsWith(realRoot + sep))) return null return { abs, size: s.size, mtime: s.mtime } } @@ -45,6 +60,7 @@ async function statFile(abs: string): Promise { async function resolveTarget( absDir: string, + realRoot: string, urlPath: string, indexNames: string[], single: boolean, @@ -67,7 +83,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 +91,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 +106,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 +114,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 +215,9 @@ export function serveStaticHandler( return serveRemoteAssetsHandler(source) const absDir = resolve(source) const opts = normalizeOptions(options) + // 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') { @@ -206,7 +225,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 realRoot, event.url.pathname, opts.indexNames, opts.single) if (!file) { event.res.status = 404 return '' @@ -250,6 +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) + const realRoot = absDir === undefined ? undefined : canonicalRoot(absDir) return (req, res, next) => { void (async () => { const method = req.method @@ -282,7 +302,7 @@ export function serveStaticNodeMiddleware( return } - const file = await resolveTarget(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/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..8e1feda1a 100644 --- a/plugins/assets/src/node/context.ts +++ b/plugins/assets/src/node/context.ts @@ -19,7 +19,12 @@ 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. Symlink-aware containment for reads and mutations lives in + * `node/paths` (`resolveAssetReadPath` / `assertAssetMutationPath`), which + * the RPC handlers call directly with {@link AssetsContext.dir}. + */ resolvePath: (relativePath: string) => string } diff --git a/plugins/assets/src/node/paths.ts b/plugins/assets/src/node/paths.ts index 1e3c480d3..df5290341 100644 --- a/plugins/assets/src/node/paths.ts +++ b/plugins/assets/src/node/paths.ts @@ -1,17 +1,67 @@ -import { resolve } from 'pathe' +import fsp from 'node:fs/promises' +import { normalize, resolve } from 'pathe' import { diagnostics } from '../diagnostics' +/** realpath, pathe-normalized, or `null` when the path doesn't exist. */ +async function realpath(path: string): Promise { + try { + return normalize(await fsp.realpath(path)) + } + catch { + return null + } +} + /** * 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. + * 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). */ export function resolveAssetPath(root: string, relativePath: string): string { - const cleaned = relativePath.replace(/^[/\\]+/, '') const normalizedRoot = resolve(root) - const absolute = resolve(normalizedRoot, cleaned) + 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. 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 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. 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) + 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 }) + } + 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..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() @@ -26,7 +27,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 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 026ac7c5f..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,11 +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 = assets.resolvePath(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 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 d4901bccf..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(assets.resolvePath(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 6363289e1..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(assets.resolvePath(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 8abe2b259..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' @@ -47,8 +48,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 assertAssetMutationPath(assets.dir, path) + const to = await assertAssetMutationPath(assets.dir, nextRelPath) if (from === to) { const stat = await fsp.lstat(from) @@ -60,6 +63,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 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 ab1a99eee..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,8 +50,11 @@ export const upload = defineAssetsRpc({ }) } - const absolute = assets.resolvePath(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 assertAssetMutationPath(assets.dir, 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..777aee819 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' @@ -77,26 +94,33 @@ 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)) + 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 it - * lands inside one of the allowed roots, or throw. + * 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. */ - 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) - 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 } 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 +128,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())