diff --git a/src/mcp/security/pathPolicy.ts b/src/mcp/security/pathPolicy.ts index f16437a2..3797cbe5 100644 --- a/src/mcp/security/pathPolicy.ts +++ b/src/mcp/security/pathPolicy.ts @@ -79,10 +79,18 @@ export function assertPathAllowed(filePath: string, allowedPaths: string[]): voi if ( resolvedAllowed.length > 0 && !resolvedAllowed.some((base) => { - const baseKey = normalizeForCompare(base); - // Strip a trailing separator so roots like '/' or 'C:\' don't produce '//' or 'C:\\' - const baseKeyNorm = baseKey.endsWith(path.sep) ? baseKey.slice(0, -1) : baseKey; - return resolvedKey === baseKey || resolvedKey.startsWith(baseKeyNorm + path.sep); + const rawBaseKey = normalizeForCompare(base); + // Strip trailing separator unless base is a filesystem root (/ on Unix, C:\ on Windows). + // A trailing sep from user input like "/tmp/" would otherwise cause double-sep prefix + // checks ("startsWith('/tmp//')") and equality mismatches ("/tmp" !== "/tmp/"). + const isRoot = rawBaseKey === path.sep || (isWindows && /^[a-z]:[/\\]$/.test(rawBaseKey)); + if (isRoot) { + // Root path already ends with its own separator (/ or C:\). + // Appending path.sep would produce // or C:\\, breaking startsWith for all children. + return resolvedKey.startsWith(rawBaseKey); + } + const baseKey = rawBaseKey.endsWith(path.sep) ? rawBaseKey.slice(0, -1) : rawBaseKey; + return resolvedKey === baseKey || resolvedKey.startsWith(baseKey + path.sep); }) ) { throw new PathPolicyError( diff --git a/src/mcp/tools/testPlanTools.ts b/src/mcp/tools/testPlanTools.ts index d93d7253..1dd8352e 100644 --- a/src/mcp/tools/testPlanTools.ts +++ b/src/mcp/tools/testPlanTools.ts @@ -259,9 +259,23 @@ export function registerTestPlanAddInstance(server: McpServer, config: ServerCon }; } - // Resolve testcase absolute path — normalize backslashes so Windows-style paths work on macOS/Linux. - // Check for '..' before path.join() normalizes them away; otherwise traversal goes undetected - // when allowedPaths is empty (unrestricted mode). Then enforce containment on the resolved path. + // Reject absolute test_case_path: path.join(root, absPath) ignores root on Windows (drive-letter paths) + // and Unix, allowing the caller to escape the project directory entirely. + if (path.isAbsolute(test_case_path)) { + return { + isError: true, + content: [ + { + type: 'text' as const, + text: JSON.stringify( + makeError('INVALID_PATH', 'test_case_path must be relative to project_path, not absolute', requestId) + ), + }, + ], + }; + } + + // Resolve testcase absolute path — normalize backslashes so Windows-style paths work on macOS/Linux const normalizedTestCasePath = toForwardSlashes(test_case_path); if (normalizedTestCasePath.split('/').some((seg) => seg === '..')) { throw new PathPolicyError('PATH_TRAVERSAL', `Path traversal detected in test_case_path: ${test_case_path}`); diff --git a/test/unit/mcp/pathPolicy.test.ts b/test/unit/mcp/pathPolicy.test.ts index 38aab73c..35bbe6ff 100644 --- a/test/unit/mcp/pathPolicy.test.ts +++ b/test/unit/mcp/pathPolicy.test.ts @@ -44,6 +44,22 @@ describe('pathPolicy', () => { assert.doesNotThrow(() => assertPathAllowed(path.join(tmp, 'a', 'b', 'c', 'file.xml'), [tmp])); }); + it('allows path when allowed root has a trailing separator', () => { + assert.doesNotThrow(() => assertPathAllowed(path.join(tmp, 'foo.java'), [tmp + path.sep])); + }); + + it('allows exact match when allowed root has a trailing separator', () => { + assert.doesNotThrow(() => assertPathAllowed(tmp, [tmp + path.sep])); + }); + + it('allows child paths when allowed root is the filesystem root', () => { + // path.parse gives the drive root on Windows (C:\) or / on Unix. + // Regression: prior code appended path.sep to the root, producing "//" or "C:\\\\", + // causing startsWith to fail for all children. + const fsRoot = path.parse(tmp).root; + assert.doesNotThrow(() => assertPathAllowed(path.join(tmp, 'foo.java'), [fsRoot])); + }); + it('rejects sibling directories that share a prefix', () => { const allowed = path.join(tmp, 'myproject'); const sibling = path.join(tmp, 'myproject-evil', 'secret.txt'); diff --git a/test/unit/mcp/testPlanTools.test.ts b/test/unit/mcp/testPlanTools.test.ts index 0193ebcf..ceb09eff 100644 --- a/test/unit/mcp/testPlanTools.test.ts +++ b/test/unit/mcp/testPlanTools.test.ts @@ -369,6 +369,25 @@ describe('provar_testplan_add-instance', () => { assert.equal(errorCode(result), 'FILE_NOT_FOUND'); }); + it('returns INVALID_PATH when test_case_path is absolute', () => { + makeProject(projectDir); + makePlan(projectDir, 'MyPlan'); + + // Construct the absolute path from the filesystem root so the test is robust + // regardless of whether projectDir happens to be absolute or relative. + const absolutePath = path.join(path.parse(projectDir).root, 'escape', 'MyTest.testcase'); + const result = server.call('provar_testplan_add-instance', { + project_path: projectDir, + test_case_path: absolutePath, + plan_name: 'MyPlan', + overwrite: false, + dry_run: false, + }); + + assert.equal(isError(result), true); + assert.equal(errorCode(result), 'INVALID_PATH'); + }); + it('returns INVALID_PATH when test_case_path does not end with .testcase', () => { makeProject(projectDir); // Create the file but with wrong extension