From a50885694401915cc16a006388f992d19e1b963f Mon Sep 17 00:00:00 2001 From: Anket Pratap Singh Date: Sat, 29 Aug 2026 13:10:20 +0530 Subject: [PATCH] Resolve both operands in isUnderRoot so containment works on Windows isUnderRoot() applied path.resolve() to filePath but only path.normalize() to repoRoot. On Windows resolve() prepends the current drive letter and normalize() does not, so a genuinely contained file compared "C:\repo\src\a.ts" against "\repo" and was rejected. The function is duplicated verbatim in travsr-lsif-ts and travsr-lsif-py; both copies are fixed. Not exploitable in the current call path (walker.ts always passes a root from resolveRoot(), which is already absolute), but the red test made npm test unrunnable on Windows dev machines, and the containment filter's correctness silently depended on an unstated caller precondition. It now holds for any input, and the doc comment records why. The function takes an optional pathImpl parameter (defaulting to the host path module) so the tests can pin its Windows AND POSIX behaviour from any host: new tests inject path.win32 and path.posix explicitly with fully-qualified inputs, making the platform matrix part of every test run instead of only surfacing on whichever OS CI happens to use. Production callers never pass it. Fixes #806 --- packages/travsr-lsif-py/src/security.ts | 20 ++++++-- .../travsr-lsif-py/src/test/security.test.ts | 46 +++++++++++++++++++ packages/travsr-lsif-ts/src/security.ts | 20 ++++++-- .../travsr-lsif-ts/src/test/security.test.ts | 46 +++++++++++++++++++ 4 files changed, 124 insertions(+), 8 deletions(-) diff --git a/packages/travsr-lsif-py/src/security.ts b/packages/travsr-lsif-py/src/security.ts index a79b76f3..d5ce9cb2 100644 --- a/packages/travsr-lsif-py/src/security.ts +++ b/packages/travsr-lsif-py/src/security.ts @@ -46,11 +46,23 @@ export function assertPathsContained(filePaths: string[], repoRoot: string): voi * Pure in-memory containment check — no I/O, safe to call in hot loops. * Does NOT follow symlinks; use assertPathsContained for the authoritative check. * Used as a belt-and-suspenders filter before emitting each file's ranges. + * + * Both operands are resolved before comparing (#806). Resolving only filePath + * put the two sides in different namespaces on Windows, where resolve() + * prepends the current drive letter and normalize() does not, so a genuinely + * contained file compared "C:\repo\src\a.ts" against "\repo" and was dropped. + * + * pathImpl exists so the tests can pin this function's Windows and POSIX + * behaviour from any host; production callers never pass it. */ -export function isUnderRoot(filePath: string, repoRoot: string): boolean { - const normalized = path.normalize(path.resolve(filePath)); - const root = path.normalize(repoRoot); - return normalized.startsWith(root + path.sep) || normalized === root; +export function isUnderRoot( + filePath: string, + repoRoot: string, + pathImpl: typeof path = path +): boolean { + const normalized = pathImpl.normalize(pathImpl.resolve(filePath)); + const root = pathImpl.normalize(pathImpl.resolve(repoRoot)); + return normalized.startsWith(root + pathImpl.sep) || normalized === root; } // ── Internal helpers ─────────────────────────────────────────────────────────── diff --git a/packages/travsr-lsif-py/src/test/security.test.ts b/packages/travsr-lsif-py/src/test/security.test.ts index 631ea51c..ce2e8319 100644 --- a/packages/travsr-lsif-py/src/test/security.test.ts +++ b/packages/travsr-lsif-py/src/test/security.test.ts @@ -45,6 +45,52 @@ test('isUnderRoot: correctly rejects escaped paths', () => { assert.ok(!isUnderRoot('/etc/passwd', root)); assert.ok(!isUnderRoot('/', root)); }); +// #806: isUnderRoot resolved filePath but not repoRoot. On Windows resolve() +// prepends the current drive letter and normalize() does not, so the two sides +// compared "C:\...\foo.py" against "\home\user\project" and every +// contained file was rejected. The platform-native test above only catches that +// when run ON Windows; these two pin both platforms' semantics from any host by +// injecting the flavour explicitly. Inputs are fully qualified — a +// drive-relative path would pick up the host's cwd and stop being +// deterministic. +test('isUnderRoot: Windows semantics, verified from any host (#806)', () => { + const root = 'C:\\home\\user\\project'; + assert.ok(isUnderRoot('C:\\home\\user\\project\\src\\foo.py', root, path.win32)); + assert.ok(isUnderRoot('C:\\home\\user\\project', root, path.win32)); + assert.ok( + isUnderRoot('C:/home/user/project/src/foo.py', root, path.win32), + 'forward slashes normalize to backslashes' + ); + assert.ok(!isUnderRoot('C:\\home\\user\\other\\foo.py', root, path.win32)); + assert.ok( + !isUnderRoot('C:\\home\\user\\project-evil\\foo.py', root, path.win32), + 'prefix match is not enough' + ); + assert.ok( + !isUnderRoot('D:\\home\\user\\project\\src\\foo.py', root, path.win32), + 'same path on another drive is outside' + ); + assert.ok( + !isUnderRoot('C:\\home\\user\\project\\src\\..\\..\\..\\etc\\passwd', root, path.win32), + 'traversal collapses before comparing' + ); +}); + +test('isUnderRoot: POSIX semantics, verified from any host (#806)', () => { + const root = '/home/user/project'; + assert.ok(isUnderRoot('/home/user/project/src/foo.py', root, path.posix)); + assert.ok(isUnderRoot('/home/user/project', root, path.posix)); + assert.ok(!isUnderRoot('/home/user/other/foo.py', root, path.posix)); + assert.ok( + !isUnderRoot('/home/user/project-evil/foo.py', root, path.posix), + 'prefix match is not enough' + ); + assert.ok( + !isUnderRoot('/home/user/project/../project-evil/foo.py', root, path.posix), + 'traversal collapses before comparing' + ); +}); + test('assertPathsContained: accepts files inside root', () => { const root = resolveRoot('/tmp'); diff --git a/packages/travsr-lsif-ts/src/security.ts b/packages/travsr-lsif-ts/src/security.ts index 7eed7eac..e4769db3 100644 --- a/packages/travsr-lsif-ts/src/security.ts +++ b/packages/travsr-lsif-ts/src/security.ts @@ -134,11 +134,23 @@ export function assertFilesContained( * Pure in-memory containment check — no I/O, safe to call in hot filter loops. * Does NOT follow symlinks; use assertFilesContained for the authoritative check. * Used as a belt-and-suspenders filter on program.getSourceFiles(). + * + * Both operands are resolved before comparing (#806). Resolving only filePath + * put the two sides in different namespaces on Windows, where resolve() + * prepends the current drive letter and normalize() does not, so a genuinely + * contained file compared "C:\repo\src\a.ts" against "\repo" and was dropped. + * + * pathImpl exists so the tests can pin this function's Windows and POSIX + * behaviour from any host; production callers never pass it. */ -export function isUnderRoot(filePath: string, repoRoot: string): boolean { - const normalized = path.normalize(path.resolve(filePath)); - const root = path.normalize(repoRoot); - return normalized.startsWith(root + path.sep) || normalized === root; +export function isUnderRoot( + filePath: string, + repoRoot: string, + pathImpl: typeof path = path +): boolean { + const normalized = pathImpl.normalize(pathImpl.resolve(filePath)); + const root = pathImpl.normalize(pathImpl.resolve(repoRoot)); + return normalized.startsWith(root + pathImpl.sep) || normalized === root; } // ── Internal helpers ────────────────────────────────────────────────────────── diff --git a/packages/travsr-lsif-ts/src/test/security.test.ts b/packages/travsr-lsif-ts/src/test/security.test.ts index d2d890d7..0328872c 100644 --- a/packages/travsr-lsif-ts/src/test/security.test.ts +++ b/packages/travsr-lsif-ts/src/test/security.test.ts @@ -115,6 +115,52 @@ test('isUnderRoot: correctly identifies contained and escaped paths', () => { assert.ok(!isUnderRoot('/home/user/other/foo.ts', root)); assert.ok(!isUnderRoot('/home/user/project-evil/foo.ts', root), 'prefix match is not enough'); }); +// #806: isUnderRoot resolved filePath but not repoRoot. On Windows resolve() +// prepends the current drive letter and normalize() does not, so the two sides +// compared "C:\...\foo.ts" against "\home\user\project" and every +// contained file was rejected. The platform-native test above only catches that +// when run ON Windows; these two pin both platforms' semantics from any host by +// injecting the flavour explicitly. Inputs are fully qualified — a +// drive-relative path would pick up the host's cwd and stop being +// deterministic. +test('isUnderRoot: Windows semantics, verified from any host (#806)', () => { + const root = 'C:\\home\\user\\project'; + assert.ok(isUnderRoot('C:\\home\\user\\project\\src\\foo.ts', root, path.win32)); + assert.ok(isUnderRoot('C:\\home\\user\\project', root, path.win32)); + assert.ok( + isUnderRoot('C:/home/user/project/src/foo.ts', root, path.win32), + 'forward slashes normalize to backslashes' + ); + assert.ok(!isUnderRoot('C:\\home\\user\\other\\foo.ts', root, path.win32)); + assert.ok( + !isUnderRoot('C:\\home\\user\\project-evil\\foo.ts', root, path.win32), + 'prefix match is not enough' + ); + assert.ok( + !isUnderRoot('D:\\home\\user\\project\\src\\foo.ts', root, path.win32), + 'same path on another drive is outside' + ); + assert.ok( + !isUnderRoot('C:\\home\\user\\project\\src\\..\\..\\..\\etc\\passwd', root, path.win32), + 'traversal collapses before comparing' + ); +}); + +test('isUnderRoot: POSIX semantics, verified from any host (#806)', () => { + const root = '/home/user/project'; + assert.ok(isUnderRoot('/home/user/project/src/foo.ts', root, path.posix)); + assert.ok(isUnderRoot('/home/user/project', root, path.posix)); + assert.ok(!isUnderRoot('/home/user/other/foo.ts', root, path.posix)); + assert.ok( + !isUnderRoot('/home/user/project-evil/foo.ts', root, path.posix), + 'prefix match is not enough' + ); + assert.ok( + !isUnderRoot('/home/user/project/../project-evil/foo.ts', root, path.posix), + 'traversal collapses before comparing' + ); +}); + // ── Integration tests against malicious tsconfig fixtures ─────────────────────