From eeda92129cd8e68173299d3370e492d4befa5b8a Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Wed, 6 May 2026 15:22:32 -0500 Subject: [PATCH 1/2] PDX-0: fix(mcp): address PR #145 review comments for security hardening RCA: Copilot review on PR #145 identified two security gaps: (1) pathPolicy.ts did not strip trailing separators from allowed-root keys, causing double-sep prefix checks ("/tmp//") and equality mismatches that incorrectly reject valid child paths when users pass roots like "/tmp/"; (2) testPlanTools.ts accepted absolute test_case_path values, allowing path.join(projectRoot, absolutePath) to silently discard projectRoot on Windows (drive-letter) and Unix, enabling reads outside the project. Fix: Strip trailing path.sep from allowed-root keys in pathPolicy (except filesystem roots "/" and "C:\"). Reject absolute test_case_path values in add-instance handler before path.join. Add two new tests: trailing-sep cases in pathPolicy.test.ts and an absolute-path rejection in testPlanTools.test.ts. Co-Authored-By: Claude Sonnet 4.6 --- src/mcp/security/pathPolicy.ts | 7 ++++++- src/mcp/tools/testPlanTools.ts | 16 ++++++++++++++++ test/unit/mcp/pathPolicy.test.ts | 8 ++++++++ test/unit/mcp/testPlanTools.test.ts | 16 ++++++++++++++++ 4 files changed, 46 insertions(+), 1 deletion(-) diff --git a/src/mcp/security/pathPolicy.ts b/src/mcp/security/pathPolicy.ts index c7a46560..04d25f18 100644 --- a/src/mcp/security/pathPolicy.ts +++ b/src/mcp/security/pathPolicy.ts @@ -79,7 +79,12 @@ export function assertPathAllowed(filePath: string, allowedPaths: string[]): voi if ( resolvedAllowed.length > 0 && !resolvedAllowed.some((base) => { - const baseKey = normalizeForCompare(base); + 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)); + const baseKey = !isRoot && rawBaseKey.endsWith(path.sep) ? rawBaseKey.slice(0, -1) : rawBaseKey; return resolvedKey === baseKey || resolvedKey.startsWith(baseKey + path.sep); }) ) { diff --git a/src/mcp/tools/testPlanTools.ts b/src/mcp/tools/testPlanTools.ts index c53e65a4..aa781982 100644 --- a/src/mcp/tools/testPlanTools.ts +++ b/src/mcp/tools/testPlanTools.ts @@ -259,6 +259,22 @@ export function registerTestPlanAddInstance(server: McpServer, config: ServerCon }; } + // 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); const absoluteTestCasePath = path.join(projectRoot, normalizedTestCasePath); diff --git a/test/unit/mcp/pathPolicy.test.ts b/test/unit/mcp/pathPolicy.test.ts index 38aab73c..2c0ac3c3 100644 --- a/test/unit/mcp/pathPolicy.test.ts +++ b/test/unit/mcp/pathPolicy.test.ts @@ -44,6 +44,14 @@ 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('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 cbef770c..1baa5bce 100644 --- a/test/unit/mcp/testPlanTools.test.ts +++ b/test/unit/mcp/testPlanTools.test.ts @@ -369,6 +369,22 @@ 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'); + + const result = server.call('provar_testplan_add-instance', { + project_path: projectDir, + test_case_path: path.join(projectDir, 'tests', 'MyTest.testcase'), + 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 From 9b5f32d360626c43d58a76261d73cb9dc8ed47df Mon Sep 17 00:00:00 2001 From: Michael Dailey Date: Wed, 6 May 2026 15:55:40 -0500 Subject: [PATCH 2/2] PDX-0: fix(mcp): address PR #146 review comments for root-path and test robustness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RCA: PR #146 Copilot review found three issues in the previous security fix: (1) filesystem root allowed-paths (/ or C:\) were broken — appending path.sep to a root key produces // or C:\ so startsWith always fails for children, meaning only the root itself was allowed instead of everything under it; (2) no regression test existed for the filesystem-root case; (3) the absolute-path rejection test relied on projectDir being absolute rather than constructing the absolute path explicitly. Fix: Handle root base keys separately in assertPathAllowed — use startsWith(rawBaseKey) directly since the root already ends with its own separator. Add a filesystem-root regression test using path.parse(tmp).root. Rewrite the absolute-path test to derive the escape path from path.parse(projectDir).root so it is always absolute regardless of how projectDir is constructed. Co-Authored-By: Claude Sonnet 4.6 --- src/mcp/security/pathPolicy.ts | 7 ++++++- test/unit/mcp/pathPolicy.test.ts | 8 ++++++++ test/unit/mcp/testPlanTools.test.ts | 5 ++++- 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/src/mcp/security/pathPolicy.ts b/src/mcp/security/pathPolicy.ts index 04d25f18..3797cbe5 100644 --- a/src/mcp/security/pathPolicy.ts +++ b/src/mcp/security/pathPolicy.ts @@ -84,7 +84,12 @@ export function assertPathAllowed(filePath: string, allowedPaths: string[]): voi // 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)); - const baseKey = !isRoot && rawBaseKey.endsWith(path.sep) ? rawBaseKey.slice(0, -1) : 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); }) ) { diff --git a/test/unit/mcp/pathPolicy.test.ts b/test/unit/mcp/pathPolicy.test.ts index 2c0ac3c3..35bbe6ff 100644 --- a/test/unit/mcp/pathPolicy.test.ts +++ b/test/unit/mcp/pathPolicy.test.ts @@ -52,6 +52,14 @@ describe('pathPolicy', () => { 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 cb5f7c9a..ceb09eff 100644 --- a/test/unit/mcp/testPlanTools.test.ts +++ b/test/unit/mcp/testPlanTools.test.ts @@ -373,9 +373,12 @@ describe('provar_testplan_add-instance', () => { 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: path.join(projectDir, 'tests', 'MyTest.testcase'), + test_case_path: absolutePath, plan_name: 'MyPlan', overwrite: false, dry_run: false,