Break CodeQL taint flow in /paths/compare so path-injection alerts resolve

apps/local/src/index.ts

@@ -33,8 +33,8 @@ import {
 } from '@studio/common/lib/connected-sites';
 import {
 	arePathsEqual,
+	confineToRoot,
 	isEmptyDir,
-	isPathWithin,
 	isWordPressDirectory,
 	recursiveCopyDirectory,
 } from '@studio/common/lib/fs-utils';
@@ -542,14 +542,11 @@ export async function startLocalServer( options: LocalServerOptions ): Promise<
 
 	api.post( '/paths/compare', ( req: Request, res: Response ) => {
 		const { path1, path2 } = req.body as { path1?: string; path2?: string };
-		// Confine both operands to `sitesRoot`: nothing outside it can be a site, and
-		// this keeps untrusted input from reaching statSync (in arePathsEqual).
-		const equal =
-			!! path1 &&
-			!! path2 &&
-			isPathWithin( sitesRoot, path1 ) &&
-			isPathWithin( sitesRoot, path2 ) &&
-			arePathsEqual( path1, path2 );
+		// Confine both operands to `sitesRoot` before any filesystem access; nothing
+		// outside it can be a site, so a non-match is the correct answer.
+		const confined1 = path1 ? confineToRoot( sitesRoot, path1 ) : null;
+		const confined2 = path2 ? confineToRoot( sitesRoot, path2 ) : null;
+		const equal = !! confined1 && !! confined2 && arePathsEqual( confined1, confined2 );
 		res.json( { equal } );
 	} );
 

packages/common/lib/fs-utils.ts

@@ -72,14 +72,19 @@ export function isWordPressDirectory( projectPath: string ): boolean {
 	);
 }
 
-// True when `candidate` resolves to `root` or a descendant. Used to confine
-// untrusted paths to a safe root, guarding against `../` traversal escapes.
-export function isPathWithin( root: string, candidate: string ): boolean {
+// Resolve `candidate` under `root`, returning the normalized path when it is `root`
+// or a descendant, or `null` when it escapes (e.g. via `../`). Callers with
+// untrusted input must use the returned value for filesystem access, not the raw input.
+export function confineToRoot( root: string, candidate: string ): string | null {
 	const resolvedRoot = path.resolve( root );
-	const resolvedCandidate = path.resolve( candidate );
-	return (
-		resolvedCandidate === resolvedRoot || resolvedCandidate.startsWith( resolvedRoot + path.sep )
-	);
+	const resolvedCandidate = path.resolve( resolvedRoot, candidate );
+	if (
+		resolvedCandidate === resolvedRoot ||
+		resolvedCandidate.startsWith( resolvedRoot + path.sep )
+	) {
+		return resolvedCandidate;
+	}
+	return null;
 }
 
 // Compare paths, preferring inode comparison when both paths exist on disk.

packages/common/lib/tests/fs-utils.test.ts

@@ -2,7 +2,7 @@ import fs from 'fs';
 import os from 'os';
 import path from 'path';
 import { vi } from 'vitest';
-import { calculateDirectorySizeForArchive, isPathWithin } from '@studio/common/lib/fs-utils';
+import { calculateDirectorySizeForArchive, confineToRoot } from '@studio/common/lib/fs-utils';
 
 describe( 'calculateDirectorySizeForArchive', () => {
 	let tempDir: string;
@@ -40,22 +40,29 @@ describe( 'calculateDirectorySizeForArchive', () => {
 	} );
 } );
 
-describe( 'isPathWithin', () => {
+describe( 'confineToRoot', () => {
 	const root = path.join( os.tmpdir(), 'studio-sites' );
 
-	it( 'accepts the root itself and its descendants', () => {
-		expect( isPathWithin( root, root ) ).toBe( true );
-		expect( isPathWithin( root, path.join( root, 'my-site' ) ) ).toBe( true );
-		expect( isPathWithin( root, path.join( root, 'my-site', 'wp-content' ) ) ).toBe( true );
+	it( 'returns the resolved path for the root itself and its descendants', () => {
+		expect( confineToRoot( root, root ) ).toBe( path.resolve( root ) );
+		expect( confineToRoot( root, path.join( root, 'my-site' ) ) ).toBe(
+			path.resolve( root, 'my-site' )
+		);
+	} );
+
+	it( 'normalizes traversal segments that stay inside the root', () => {
+		expect( confineToRoot( root, path.join( root, 'a', '..', 'b' ) ) ).toBe(
+			path.resolve( root, 'b' )
+		);
 	} );
 
-	it( 'rejects traversal escapes and unrelated paths', () => {
-		expect( isPathWithin( root, path.join( root, '..', 'secret' ) ) ).toBe( false );
-		expect( isPathWithin( root, path.join( root, '..', '..', 'etc', 'passwd' ) ) ).toBe( false );
-		expect( isPathWithin( root, '/etc/passwd' ) ).toBe( false );
+	it( 'returns null for traversal escapes and unrelated paths', () => {
+		expect( confineToRoot( root, path.join( root, '..', 'secret' ) ) ).toBeNull();
+		expect( confineToRoot( root, '/etc/passwd' ) ).toBeNull();
+		expect( confineToRoot( root, `${ root }-other` ) ).toBeNull();
 	} );
 
-	it( 'rejects a sibling directory sharing the root as a name prefix', () => {
-		expect( isPathWithin( root, `${ root }-other` ) ).toBe( false );
+	it( 'resolves relative candidates against the root, not the cwd', () => {
+		expect( confineToRoot( root, 'my-site' ) ).toBe( path.resolve( root, 'my-site' ) );
 	} );
 } );