fix: MCP security hardening (items 1-10)#109
Merged
Merged
Conversation
1. pathPolicy: resolve symlinks via realpathSync to prevent containment bypass
- A symlink inside an allowed dir pointing outside could bypass the check
- Falls back to lexical resolution for non-existent (output) paths
- Resolves allowedPaths through realpathSync too
2. automationTools: assertPathAllowed on properties_path in config.load
- properties_path was passed directly to sf CLI with no path policy check
- Thread ServerConfig through registerAutomationConfigLoad and registerAllAutomationTools
- PathPolicyError surfaced as structured isError response
3. mcp-smoke.cjs: remove || 'true' license bypass fallback
- Changed to || '' so smoke test fails if PROVAR_DEV_WHITELIST_KEYS is unset
- Prevents silent license bypass in forks/local runs without the secret
4. algasClient: fix 401 retry reusing expired AbortController
- Original controller timer may have nearly elapsed before 401 response arrives
- Retry now uses a fresh AbortController with a full TIMEOUT_MS budget
Tests: add symlink containment test to pathPolicy.test.ts
add path policy enforcement tests to automationTools.test.ts
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
5. CI_Execution.yml: remove NODE_ENV=test from MCP smoke step
- NODE_ENV=test bypassed license validation entirely via the test shortcut
- PROVAR_DEV_WHITELIST_KEYS secret now exercises the whitelist code path
6. antTools: assertPathAllowed for all path inputs in provar.ant.generate
- provar_home, project_path, results_path, license_path, smtp_path,
project_cache_path now validated before being embedded in ANT XML
- Prevents LLM from embedding out-of-bounds paths in generated build.xml
7. antTools: add single-quote escape to escapeXmlAttr
- ' was missing from the escape set, creating an XML injection vector
- Added .replace(/'/g, ''')
8. licenseCache: write cache file with mode 0o600
- Default umask (644) allowed any local user to read/modify the cache
- With 0o600 only the owning user can read or write it
9. ideDetection: remove findLicenseByDecryptedKey and AES decryption code
- Function is dead code (never called by licenseValidator)
- Removing it eliminates the hardcoded AES-128-ECB key from the public package
- Remove createDecipheriv import, AES_KEY constant, decryptLicenseKeyField
- Remove corresponding tests from licenseValidator.test.ts
10. CI_Execution.yml: restore macos-latest to default CI matrix
- Only ubuntu-latest was being tested by default after the matrix fix
- os.homedir() and path behaviour differ on macOS; primary user OS
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Security and correctness hardening for the Provar MCP V1 server, primarily focused on preventing filesystem path escape (including symlink traversal), removing credential-adjacent dead code, and tightening licensing/CI behavior.
Changes:
- Hardened path containment checks (now resolves symlinks via
fs.realpathSync) and threadedServerConfig.allowedPathsinto automation/ANT tooling for enforcement. - Removed IDE license-key decryption logic (and associated tests) to avoid hardcoded key exposure.
- Improved operational safety: safer 401 retry timeout handling, tighter license cache file permissions intent, and CI/smoke-test license bypass removal.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| test/unit/mcp/pathPolicy.test.ts | Adds symlink containment tests for path policy. |
| test/unit/mcp/licensing/licenseValidator.test.ts | Removes tests for deleted IDE decrypt-based license lookup. |
| test/unit/mcp/automationTools.test.ts | Updates tool registration to pass ServerConfig and adds path policy enforcement tests. |
| src/mcp/tools/automationTools.ts | Enforces path policy for properties_path and threads ServerConfig through registration. |
| src/mcp/tools/antTools.ts | Enforces path policy across ANT path inputs; improves XML attribute escaping. |
| src/mcp/server.ts | Passes config into automation tool registration. |
| src/mcp/security/pathPolicy.ts | Switches to realpath-based containment to prevent symlink traversal. |
| src/mcp/licensing/licenseCache.ts | Writes cache file with restrictive mode (owner-only intent). |
| src/mcp/licensing/ideDetection.ts | Removes AES decrypt logic with hardcoded key and the related lookup function. |
| src/mcp/licensing/algasClient.ts | Uses a fresh AbortController for the 401 retry fetch. |
| scripts/mcp-smoke.cjs | Removes whitelist fallback that could bypass license validation when secret is unset. |
| .github/workflows/CI_Execution.yml | Restores macOS CI matrix and removes NODE_ENV=test from smoke step. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
3 tasks
mrdailey99
added a commit
that referenced
this pull request
Apr 7, 2026
PR #109 added assertPathAllowed checks on input path fields (provar_home, project_path, results_path) but existing test fixtures used '..' segments that trigger PATH_TRAVERSAL before the allowedPaths.length guard. - antTools.test.ts: changed config to allowedPaths:[] and updated minimalInput() to use tmpDir-based paths (68 passing, was 26+38 fail) - automationTools.test.ts: fixed traversal test to use string concat instead of path.join (which normalises away '..' segments) - pathPolicy.test.ts: extracted typed local var to fix TS compile error that caused mocha to print help instead of running tests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Security and correctness fixes for Provar MCP V1, identified during pre-merge review of #107.
Changes
Path containment (symlink hardening)
pathPolicy.ts: Replacepath.resolve()withfs.realpathSync()to prevent symlink traversal attacks; both the input path and allowed paths are resolved through the real filesystemautomationTools.ts: ThreadServerConfigthroughregisterAllAutomationTools→registerAutomationConfigLoadand addassertPathAllowedcheck forproperties_pathantTools.ts: AddassertPathAllowedfor all 6 path inputs (provar_home,project_path,results_path,license_path,smtp_path,project_cache_path)server.ts: PassconfigtoregisterAllAutomationToolsXML injection
antTools.ts: Add'→'escape inescapeXmlAttrDead code / credential exposure
ideDetection.ts: Remove AES-128-ECB decrypt function with hardcoded key'provarautomation'(unused dead code, key was committed to public npm package)License bypass removal
scripts/mcp-smoke.cjs: Remove|| 'true'fallback onPROVAR_DEV_WHITELIST_KEYS(was bypassing license validation when secret not set).github/workflows/CI_Execution.yml: RemoveNODE_ENV: testfrom smoke test step (was bypassing license validation in CI)HTTP retry safety
algasClient.ts: Create freshAbortControllerfor the 401-retry fetch instead of reusing the elapsed oneFile permissions
licenseCache.ts: Write cache file with0o600mode (owner read/write only)CI coverage
.github/workflows/CI_Execution.yml: Restoremacos-latestto default CI matrixTests updated
pathPolicy.test.ts: Added symlink containment tests (skipped on Windows, runs on Linux/macOS CI)automationTools.test.ts: Updated forServerConfigparam + path policy enforcement testslicenseValidator.test.ts: RemovedfindLicenseByDecryptedKeytests (function removed)Deferred (items 11-16)
Hardcoded version string, duplicate
SfNotFoundError,shell:truein smoke test,/fail/iregex in qualityHubTools, license freshness check, and yarn devDependency — addressed after this PR is tested.Test plan
PROVAR_DEV_WHITELIST_KEYSsecret set🤖 Generated with Claude Code