diff --git a/apps/local/src/index.ts b/apps/local/src/index.ts index 4f53aa4197..95c2107f42 100644 --- a/apps/local/src/index.ts +++ b/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 } ); } ); diff --git a/packages/common/lib/fs-utils.ts b/packages/common/lib/fs-utils.ts index 063586093d..ddaefe4777 100644 --- a/packages/common/lib/fs-utils.ts +++ b/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. diff --git a/packages/common/lib/tests/fs-utils.test.ts b/packages/common/lib/tests/fs-utils.test.ts index 9faf54f398..da3884d064 100644 --- a/packages/common/lib/tests/fs-utils.test.ts +++ b/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' ) ); } ); } );