fix: MCP security hardening (items 1-10)

.github/workflows/CI_Execution.yml

@@ -22,7 +22,7 @@ jobs:
   provardx-ci-execution:
     strategy:
       matrix:
-        os: ${{ fromJSON(inputs.OS && format('[{0}]', inputs.OS) || '["ubuntu-latest"]') }}
+        os: ${{ fromJSON(inputs.OS && format('[{0}]', inputs.OS) || '["ubuntu-latest", "macos-latest"]') }}
         nodeversion: [20]
     runs-on: ${{ matrix.os }}
     steps:
@@ -102,7 +102,6 @@ jobs:
       - name: MCP smoke test
         timeout-minutes: 5
         env:
-          NODE_ENV: test
           PROVAR_DEV_WHITELIST_KEYS: ${{ secrets.PROVAR_DEV_WHITELIST_KEYS }}
         run: node scripts/mcp-smoke.cjs
       - name: Check out Regression repo

scripts/mcp-smoke.cjs

@@ -30,7 +30,7 @@ const server = spawn('sf', ['provar', 'mcp', 'start', '--allowed-paths', TMP], {
   shell: true,
   env: {
     ...process.env,
-    PROVAR_DEV_WHITELIST_KEYS: process.env.PROVAR_DEV_WHITELIST_KEYS || 'true',
+    PROVAR_DEV_WHITELIST_KEYS: process.env.PROVAR_DEV_WHITELIST_KEYS || '',
   },
 });
 

src/mcp/licensing/algasClient.ts

@@ -127,14 +127,22 @@ export async function validateKeyWithAlgas(licenseKey: string): Promise<AlgasRes
 
     // 401 means the Cognito token was rotated mid-session — clear the module-level cache
     // and retry exactly once with a fresh token before surfacing an error.
+    // A new AbortController is created for the retry: the original controller's timer
+    // may have partially elapsed during the first request, leaving insufficient time.
     if (res.status === 401) {
       cachedToken = null;
       tokenExpiresAt = 0;
-      const refreshedToken = await getCognitoToken();
-      res = await fetch(`${TAG_BASE_URL}/${encodeURIComponent(licenseKey)}`, {
-        headers: { Authorization: `Bearer ${refreshedToken}` },
-        signal: controller.signal,
-      });
+      const retryController = new AbortController();
+      const retryTimeout = setTimeout(() => retryController.abort(), TIMEOUT_MS);
+      try {
+        const refreshedToken = await getCognitoToken();
+        res = await fetch(`${TAG_BASE_URL}/${encodeURIComponent(licenseKey)}`, {
+          headers: { Authorization: `Bearer ${refreshedToken}` },
+          signal: retryController.signal,
+        });
+      } finally {
+        clearTimeout(retryTimeout);
+      }
     }
 
     if (res.status === 404) {

src/mcp/licensing/ideDetection.ts

@@ -14,36 +14,13 @@
  * - licenseStatus — Activated | NotActivated | Expired | Invalid | QuotaReached
  * - licenseType — Fixed Seat | Floating | Trial | None
  * - lastOnlineAvailabilityCheckUtc — epoch ms of last ALGAS check by the IDE
- * - licenseKey — AES-128-ECB encrypted with key "provarautomation", Base64-encoded
- *
- * The licenseKey field is AES-128-ECB encrypted (LicenseSupport.KEY = "provarautomation").
- * We decrypt it to allow cross-referencing an explicitly supplied --license-key against
- * the IDE's already-validated license state without re-calling the licensing API.
  */
 
 import fs from 'node:fs';
 import path from 'node:path';
 import os from 'node:os';
-import { createDecipheriv } from 'node:crypto';
 import type { LicenseType } from './licenseCache.js';
 
-/** AES-128-ECB key used by Provar IDE to encrypt the licenseKey field. */
-const AES_KEY = Buffer.from('provarautomation'); // 16 bytes ASCII
-
-/**
- * Decrypt an AES-128-ECB + PKCS5 encrypted, Base64-encoded licenseKey field.
- * Returns the plaintext string, or null if decryption fails (wrong key / corrupt).
- */
-function decryptLicenseKeyField(encryptedBase64: string): string | null {
-  try {
-    const decipher = createDecipheriv('aes-128-ecb', AES_KEY, null);
-    const buf = Buffer.from(encryptedBase64, 'base64');
-    return Buffer.concat([decipher.update(buf), decipher.final()]).toString('utf-8');
-  } catch {
-    return null;
-  }
-}
-
 /** Mirrors License4J's DEFAULT_LICENSE_FOLDER_PATH + PROVAR_USER_HOME. */
 function provarLicensesDir(): string {
   const provarHome = process.env['PROVAR_HOME'] ?? path.join(os.homedir(), 'Provar');
@@ -142,53 +119,3 @@ export function findActivatedIdeLicense(): IdeLicenseState | null {
   return activated.sort((a, b) => b.lastOnlineCheckMs - a.lastOnlineCheckMs)[0];
 }
 
-/**
- * Search all IDE .properties files for one whose decrypted licenseKey matches
- * the supplied raw key string.
- *
- * Used to cross-reference an explicit --license-key against the IDE's already-
- * validated activation state without making a fresh API call.
- *
- * Returns the matching IdeLicenseState (which may have activated=false if the
- * IDE recorded the key but it isn't currently activated), or null when no file
- * decrypts to the given key.
- */
-export function findLicenseByDecryptedKey(rawKey: string): IdeLicenseState | null {
-  const dir = provarLicensesDir();
-  if (!fs.existsSync(dir)) return null;
-
-  let entries: fs.Dirent[];
-  try {
-    entries = fs.readdirSync(dir, { withFileTypes: true });
-  } catch {
-    return null;
-  }
-
-  for (const entry of entries) {
-    if (!entry.isFile() || !entry.name.endsWith('.properties')) continue;
-    try {
-      const content = fs.readFileSync(path.join(dir, entry.name), 'utf-8');
-      const props = parseProperties(content);
-
-      const encryptedKey = props.get('licenseKey');
-      if (!encryptedKey) continue;
-
-      const decrypted = decryptLicenseKeyField(encryptedKey);
-      if (decrypted !== rawKey) continue;
-
-      const licenseStatus = props.get('licenseStatus') ?? '';
-      const licenseType = parseLicenseType(props.get('licenseType') ?? '');
-      const lastCheck = parseInt(props.get('lastOnlineAvailabilityCheckUtc') ?? '0', 10);
-
-      return {
-        name: entry.name.slice(0, -'.properties'.length),
-        licenseType,
-        activated: licenseStatus === 'Activated',
-        lastOnlineCheckMs: isNaN(lastCheck) ? 0 : lastCheck,
-      };
-    } catch {
-      // Unreadable or corrupt file — skip
-    }
-  }
-  return null;
-}

src/mcp/licensing/licenseCache.ts

@@ -78,7 +78,12 @@ export function writeCacheEntry(entry: CacheEntry): void {
       }
     }
     cache[entry.keyHash] = entry;
-    fs.writeFileSync(file, JSON.stringify(cache, null, 2), 'utf-8');
+    fs.writeFileSync(file, JSON.stringify(cache, null, 2), { encoding: 'utf-8', mode: 0o600 });
+    try {
+      fs.chmodSync(file, 0o600);
+    } catch {
+      // Best-effort permission hardening; cache write itself already succeeded.
+    }
   } catch {
     // Cache write failure is non-fatal; validation result still returned to caller.
   }

src/mcp/security/pathPolicy.ts

@@ -5,6 +5,7 @@
  * For full license text, see LICENSE.md file in the repo root or https://opensource.org/licenses/BSD-3-Clause
  */
 
+import fs from 'node:fs';
 import path from 'node:path';
 
 export class PathPolicyError extends Error {
@@ -24,15 +25,42 @@ export class PathPolicyError extends Error {
  * - PATH_NOT_ALLOWED — resolved path is outside all allowed roots
  *
  * When allowedPaths is empty, all paths are permitted (unrestricted mode).
+ *
+ * Symlinks are resolved via fs.realpathSync so that a symlink inside an allowed
+ * directory pointing to a location outside it cannot bypass containment.
+ * If the path does not yet exist (e.g. an output file to be created), the parent
+ * directory is resolved instead and the basename re-attached.
  */
 export function assertPathAllowed(filePath: string, allowedPaths: string[]): void {
   // Check the original path for `..` segments before any normalization resolves them away
   const rawSegments = filePath.split(/[/\\]+/).filter((s) => s.length > 0);
   if (rawSegments.some((s) => s === '..')) {
     throw new PathPolicyError('PATH_TRAVERSAL', `Path traversal detected: ${filePath}`);
   }
-  const resolved = path.resolve(filePath);
-  const resolvedAllowed = allowedPaths.map((p) => path.resolve(path.normalize(p)));
+
+  // Resolve symlinks so a symlink inside an allowed dir that points outside cannot bypass
+  // the containment check. Fall back to lexical resolution when the path doesn't exist yet.
+  let resolved: string;
+  try {
+    resolved = fs.realpathSync(filePath);
+  } catch {
+    // Path doesn't exist — resolve the parent (which should exist) to catch symlinks there
+    const parent = path.dirname(path.resolve(filePath));
+    try {
+      resolved = path.join(fs.realpathSync(parent), path.basename(filePath));
+    } catch {
+      resolved = path.resolve(filePath);
+    }
+  }
+
+  const resolvedAllowed = allowedPaths.map((p) => {
+    try {
+      return fs.realpathSync(p);
+    } catch {
+      return path.resolve(path.normalize(p));
+    }
+  });
+
   if (
     resolvedAllowed.length > 0 &&
     !resolvedAllowed.some((base) => resolved === base || resolved.startsWith(base + path.sep))

src/mcp/server.ts

@@ -63,7 +63,7 @@ export function createProvarMcpServer(config: ServerConfig): McpServer {
   registerProjectValidateFromPath(server, config);
   registerAllPropertiesTools(server, config);
   registerAllQualityHubTools(server);
-  registerAllAutomationTools(server);
+  registerAllAutomationTools(server, config);
   registerAllDefectTools(server);
   registerAllAntTools(server, config);
   registerAllRcaTools(server);

src/mcp/tools/antTools.ts

@@ -251,6 +251,15 @@ export function registerAntGenerate(server: McpServer, config: ServerConfig): vo
       });
 
       try {
+        // Validate all path inputs before writing anything — these get embedded in the
+        // generated ANT build.xml and would be accessed by ANT at execution time.
+        assertPathAllowed(input.provar_home, config.allowedPaths);
+        assertPathAllowed(input.project_path, config.allowedPaths);
+        assertPathAllowed(input.results_path, config.allowedPaths);
+        if (input.license_path) assertPathAllowed(input.license_path, config.allowedPaths);
+        if (input.smtp_path) assertPathAllowed(input.smtp_path, config.allowedPaths);
+        if (input.project_cache_path) assertPathAllowed(input.project_cache_path, config.allowedPaths);
+
         const xmlContent = buildAntXml(input);
         const filePath = input.output_path ? path.resolve(input.output_path) : undefined;
         let written = false;
@@ -559,6 +568,7 @@ function escapeXmlAttr(value: string): string {
   return value
     .replace(/&/g, '&')
     .replace(/"/g, '"')
+    .replace(/'/g, ''')
     .replace(/</g, '&lt;')
     .replace(/>/g, '&gt;');
 }

src/mcp/tools/automationTools.ts

@@ -13,6 +13,8 @@ import { z } from 'zod';
 import { McpServer } from '@modelcontextprotocol/sdk/server/mcp.js';
 import { makeError, makeRequestId } from '../schemas/common.js';
 import { log } from '../logging/logger.js';
+import type { ServerConfig } from '../server.js';
+import { assertPathAllowed, PathPolicyError } from '../security/pathPolicy.js';
 import { sfSpawnHelper } from './sfSpawn.js';
 
 // ── SF CLI discovery ──────────────────────────────────────────────────────────
@@ -135,7 +137,7 @@ function handleSpawnError(err: unknown, requestId: string, toolName: string): {
 
 // ── Tool: provar.automation.config.load ──────────────────────────────────────
 
-export function registerAutomationConfigLoad(server: McpServer): void {
+export function registerAutomationConfigLoad(server: McpServer, config: ServerConfig): void {
   server.tool(
     'provar.automation.config.load',
     [
@@ -153,6 +155,7 @@ export function registerAutomationConfigLoad(server: McpServer): void {
       log('info', 'provar.automation.config.load', { requestId, properties_path });
 
       try {
+        assertPathAllowed(properties_path, config.allowedPaths);
         const result = runSfCommand(['provar', 'automation', 'config', 'load', '--properties-file', properties_path], sf_path);
         const response = { requestId, exitCode: result.exitCode, stdout: result.stdout, stderr: result.stderr, properties_path };
 
@@ -162,6 +165,9 @@ export function registerAutomationConfigLoad(server: McpServer): void {
 
         return { content: [{ type: 'text' as const, text: JSON.stringify(response) }], structuredContent: response };
       } catch (err) {
+        if (err instanceof PathPolicyError) {
+          return { isError: true as const, content: [{ type: 'text' as const, text: JSON.stringify(makeError(err.code, err.message, requestId, false)) }] };
+        }
         return handleSpawnError(err, requestId, 'provar.automation.config.load');
       }
     }
@@ -458,9 +464,9 @@ export function registerAutomationSetup(server: McpServer): void {
 
 // ── Bulk registration ─────────────────────────────────────────────────────────
 
-export function registerAllAutomationTools(server: McpServer): void {
+export function registerAllAutomationTools(server: McpServer, config: ServerConfig): void {
   registerAutomationSetup(server);
-  registerAutomationConfigLoad(server);
+  registerAutomationConfigLoad(server, config);
   registerAutomationTestRun(server);
   registerAutomationCompile(server);
   registerAutomationMetadataDownload(server);

test/unit/mcp/automationTools.test.ts

@@ -66,7 +66,8 @@ describe('automationTools', () => {
 
     const { registerAllAutomationTools, setSfPathCacheForTesting } = await import('../../../src/mcp/tools/automationTools.js');
     setSfPathCacheForTesting('sf'); // bypass probe spawn; tests control spawnStub directly
-    registerAllAutomationTools(server as unknown as McpServer);
+    // allowedPaths: [] means unrestricted — existing tests use arbitrary paths
+    registerAllAutomationTools(server as unknown as McpServer, { allowedPaths: [] });
   });
 
   afterEach(() => {
@@ -461,6 +462,45 @@ describe('automationTools', () => {
       const [cmd] = spawnStub.firstCall.args as [string, string[]];
       assert.equal(cmd, '/custom/bin/sf');
     });
+
+    describe('path policy enforcement', () => {
+      let restrictedServer: MockMcpServer;
+      const allowedDir = os.tmpdir();
+
+      beforeEach(async () => {
+        restrictedServer = new MockMcpServer();
+        const { registerAutomationConfigLoad, setSfPathCacheForTesting } = await import('../../../src/mcp/tools/automationTools.js');
+        setSfPathCacheForTesting('sf');
+        registerAutomationConfigLoad(restrictedServer as unknown as McpServer, { allowedPaths: [allowedDir] });
+      });
+
+      it('rejects properties_path outside allowed paths', () => {
+        const result = restrictedServer.call('provar.automation.config.load', {
+          properties_path: '/etc/passwd',
+        });
+        assert.ok(isError(result));
+        assert.equal(parseBody(result).error_code, 'PATH_NOT_ALLOWED');
+        assert.ok(!spawnStub.called, 'sf should not be spawned for a rejected path');
+      });
+
+      it('rejects properties_path with .. traversal', () => {
+        const result = restrictedServer.call('provar.automation.config.load', {
+          properties_path: path.join(allowedDir, '..', 'etc', 'passwd'),
+        });
+        assert.ok(isError(result));
+        assert.equal(parseBody(result).error_code, 'PATH_TRAVERSAL');
+        assert.ok(!spawnStub.called, 'sf should not be spawned for a path traversal');
+      });
+
+      it('allows properties_path within allowed paths', () => {
+        spawnStub.returns(makeSpawnResult('', '', 0));
+        const allowed = path.join(allowedDir, 'provardx-properties.json');
+        const result = restrictedServer.call('provar.automation.config.load', {
+          properties_path: allowed,
+        });
+        assert.ok(!isError(result));
+      });
+    });
   });
 
   // ── sf_path threading ─────────────────────────────────────────────────────