PDX-0: fix(mcp): address PR #145 review comments for security hardening

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(

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}`);

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');

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