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*\)/) + } + }) +})