From bc41ad3c60863a3ec6d1b97fff3134a70b58e689 Mon Sep 17 00:00:00 2001 From: DavidBabinec Date: Sun, 6 Sep 2026 10:42:22 +0200 Subject: [PATCH] fix(runtime): decide path containment with relative(), not a string prefix The runtime package server tested containment with absPath.startsWith(`${nodeModulesDir}/`). The separator is hard-coded to POSIX, so on Windows hosts, where resolve() yields backslashes, the check was false for every legitimate path and the endpoint 404'd all runtime package assets. It fails closed, so this is an availability bug rather than a bypass, but the code is wrong either way. Containment now goes through a shared isPathWithin helper that decides with path.relative(), which is separator-correct on both platforms. assertPathWithin delegates to it, and the '..' test compares a whole segment so a child legitimately named '..foo' is no longer read as an escape. The same sweep found the inverse spelling in virtualSiteWorkspace, a prefix check with no separator at all, which would accept a sibling directory whose name merely begins with the root. Routed through the same helper. Reported as GHSA-hwp9-vc7h-gvvf. --- server/publish/runtime/packageServer.ts | 7 +- .../publish/runtime/virtualSiteWorkspace.ts | 5 +- server/util/pathWithin.ts | 39 +++++++--- src/__tests__/server/pathWithin.test.ts | 76 +++++++++++++++++++ 4 files changed, 114 insertions(+), 13 deletions(-) create mode 100644 src/__tests__/server/pathWithin.test.ts diff --git a/server/publish/runtime/packageServer.ts b/server/publish/runtime/packageServer.ts index 15ad2854c..9f61410f4 100644 --- a/server/publish/runtime/packageServer.ts +++ b/server/publish/runtime/packageServer.ts @@ -32,6 +32,7 @@ */ import { existsSync } from 'node:fs' import { resolve as resolvePath } from 'node:path' +import { isPathWithin } from '../../util/pathWithin' import { nodeModulesDirForHash, sentinelPathForHash } from './dependencyCache' const RUNTIME_PACKAGE_PREFIX = '/_instatic/runtime/cache/' @@ -76,8 +77,10 @@ function resolveCacheFilePath(pathname: string): { hash: string; absPath: string const absPath = resolvePath(nodeModulesDir, subPath) // Final containment check — the resolved path must live inside - // node_modules/. Any escape attempt returns null. - if (!absPath.startsWith(`${nodeModulesDir}/`)) return null + // node_modules/. Any escape returns null. Decided by `relative()`, not a + // string prefix: a hard-coded `/` rejected every legitimate path on Windows + // (GHSA-hwp9), where `resolvePath` yields `\` separators. + if (!isPathWithin(nodeModulesDir, absPath)) return null return { hash, absPath } } diff --git a/server/publish/runtime/virtualSiteWorkspace.ts b/server/publish/runtime/virtualSiteWorkspace.ts index 053d802a9..93e60115d 100644 --- a/server/publish/runtime/virtualSiteWorkspace.ts +++ b/server/publish/runtime/virtualSiteWorkspace.ts @@ -3,6 +3,7 @@ import { tmpdir } from 'node:os' import { dirname, join, resolve } from 'node:path' import type { SiteDocument } from '@core/page-tree' import { isSafePath, normalizePath } from '@core/files/pathValidation' +import { isPathWithin } from '../../util/pathWithin' interface SiteScriptWorkspace { rootDir: string @@ -23,7 +24,9 @@ export async function materializeSiteScriptWorkspace(site: SiteDocument): Promis if (!isSafePath(normalized)) continue const absolutePath = resolve(rootDir, normalized) - if (!absolutePath.startsWith(rootDir)) continue + // Bare `startsWith(rootDir)` would also accept a sibling directory whose + // name merely begins with the root. Use the shared containment rule. + if (!isPathWithin(rootDir, absolutePath)) continue await mkdir(dirname(absolutePath), { recursive: true }) await writeFile(absolutePath, file.content, 'utf8') diff --git a/server/util/pathWithin.ts b/server/util/pathWithin.ts index 812ec2a1d..6d4eab14c 100644 --- a/server/util/pathWithin.ts +++ b/server/util/pathWithin.ts @@ -1,19 +1,38 @@ -import { isAbsolute, relative } from 'node:path' +import { isAbsolute, relative, sep } from 'node:path' /** - * Defense-in-depth filesystem path containment. Schema-level patterns may - * exclude `..` segments and absolute paths, but filesystem sinks recompose - * paths via `path.join` — so re-assert the resolved `child` stays strictly - * under `rootDir` after composition. Throws on the root itself, any `..` - * escape, or an absolute path that lands outside the root. + * Filesystem path containment — the one rule, in two ergonomics. * - * Used by every untrusted-path write sink: plugin asset extraction + * Schema-level patterns may exclude `..` segments and absolute paths, but + * filesystem sinks recompose paths via `path.join` / `path.resolve` — so + * re-assert the resolved `child` stays strictly under `rootDir` after + * composition. The root itself does not count as "within". + * + * Containment is decided by `relative()`, never by a string prefix. A prefix + * test has to hard-code a separator, which is wrong twice over: it says `/` on + * Windows (where `relative` and `resolve` produce `\`, so every legitimate path + * is rejected — GHSA-hwp9), and without a separator it accepts a sibling whose + * name merely starts with the root (`/srv/site-evil` under `/srv/site`). + * `relative()` is platform-aware and handles both. + * + * Used by every untrusted-path sink: plugin asset extraction * (`server/plugins/runtime.ts`, `pack.ts`), the plugin admin upload route, - * and site-bundle media import. + * site-bundle media import, the runtime package server, and the site-script + * workspace. */ -export function assertPathWithin(rootDir: string, child: string): void { +export function isPathWithin(rootDir: string, child: string): boolean { const rel = relative(rootDir, child) - if (rel === '' || rel.startsWith('..') || isAbsolute(rel)) { + // '' means child IS the root; an absolute result means different roots + // entirely (separate drives on Windows). + if (rel === '' || isAbsolute(rel)) return false + // Compare against a full segment, so a child legitimately named `..foo` + // is not mistaken for an escape. + return rel !== '..' && !rel.startsWith(`..${sep}`) +} + +/** Throwing form of {@link isPathWithin}, for sinks that treat an escape as fatal. */ +export function assertPathWithin(rootDir: string, child: string): void { + if (!isPathWithin(rootDir, child)) { throw new Error(`Path "${child}" escapes root "${rootDir}"`) } } diff --git a/src/__tests__/server/pathWithin.test.ts b/src/__tests__/server/pathWithin.test.ts new file mode 100644 index 000000000..5549c3e73 --- /dev/null +++ b/src/__tests__/server/pathWithin.test.ts @@ -0,0 +1,76 @@ +import { describe, expect, it } from 'bun:test' +import { readFile } from 'node:fs/promises' +import { fileURLToPath } from 'node:url' +import { join, dirname } from 'node:path' +import { isPathWithin, assertPathWithin } from '../../../server/util/pathWithin' + +const ROOT = join(dirname(fileURLToPath(import.meta.url)), '..', '..', '..') + +/** + * Path containment is decided by `relative()`, never by a string prefix. + * Both prefix spellings are wrong in a different direction: + * + * `startsWith(`${root}/`)` — hard-codes a POSIX separator, so on Windows + * (where `resolve` yields `\`) it rejects every + * legitimate path. That was GHSA-hwp9: the + * runtime package server 404'd all assets on + * Windows hosts. + * `startsWith(root)` — no separator at all, so a sibling directory + * whose name merely begins with the root slips + * through. + */ +describe('isPathWithin', () => { + it('accepts a real descendant', () => { + expect(isPathWithin('/srv/site', '/srv/site/build/app.js')).toBe(true) + expect(isPathWithin('/srv/site', '/srv/site/a')).toBe(true) + }) + + it('rejects the root itself', () => { + expect(isPathWithin('/srv/site', '/srv/site')).toBe(false) + }) + + it('rejects a traversal escape', () => { + expect(isPathWithin('/srv/site', '/srv/site/../secrets')).toBe(false) + expect(isPathWithin('/srv/site', '/srv')).toBe(false) + expect(isPathWithin('/srv/site', '/etc/passwd')).toBe(false) + }) + + it('rejects a sibling that merely shares the root as a name prefix', () => { + // The bare-`startsWith(root)` failure mode. + expect(isPathWithin('/srv/site', '/srv/site-evil/app.js')).toBe(false) + expect(isPathWithin('/srv/site', '/srv/sitedata')).toBe(false) + }) + + it('accepts a descendant whose name legitimately begins with dots', () => { + // A naive `rel.startsWith('..')` would call this an escape. + expect(isPathWithin('/srv/site', '/srv/site/..foo')).toBe(true) + expect(isPathWithin('/srv/site', '/srv/site/...bar/baz')).toBe(true) + }) + + it('assertPathWithin throws exactly when isPathWithin is false', () => { + expect(() => assertPathWithin('/srv/site', '/srv/site/ok')).not.toThrow() + expect(() => assertPathWithin('/srv/site', '/srv/site-evil')).toThrow(/escapes root/) + expect(() => assertPathWithin('/srv/site', '/srv/site')).toThrow(/escapes root/) + }) +}) + +describe('no separator-naive containment checks remain (GHSA-hwp9)', () => { + const SINKS = [ + 'server/publish/runtime/packageServer.ts', + 'server/publish/runtime/virtualSiteWorkspace.ts', + 'server/util/pathWithin.ts', + ] + + it('containment sinks use the shared helper, not a string prefix', async () => { + for (const relPath of SINKS) { + const source = await readFile(join(ROOT, relPath), 'utf-8') + const code = source + .replace(/\/\*[\s\S]*?\*\//g, '') + .replace(/\/\/[^\n]*/g, '') + // A containment decision written as `startsWith(`${someDir}/`)` or + // `startsWith(someDir)` is the bug this suite exists to prevent. + expect(code).not.toMatch(/startsWith\(\s*`\$\{\w*[Dd]ir\w*\}/) + expect(code).not.toMatch(/startsWith\(\s*\w*(?:Dir|Root|rootDir)\w*\s*\)/) + } + }) +})