refactor: tighten symlink-containment helpers

Author: antfubot

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<string> {
-  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<ResolvedFile | null> {
   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,18 +215,17 @@ 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<string> | undefined
-  const getRealRoot = (): Promise<string> => (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') {
       event.res.status = 405
       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<string> | undefined
-  const getRealRoot = (dir: string): Promise<string> => (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()

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<string> {
-  const normalizedRoot = resolve(root)
+/** realpath, pathe-normalized, or `null` when the path doesn't exist. */
+async function realpath(path: string): Promise<string | null> {
   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<string> {
-  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<string> {
   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<string> {
+  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 })
   }

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<string[]> | undefined
-      const canonicalRoots = (): Promise<string[]> => (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<string> {
         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))
         })