diff --git a/src/__tests__/security-hardening.test.ts b/src/__tests__/security-hardening.test.ts new file mode 100644 index 00000000..45c6e5d9 --- /dev/null +++ b/src/__tests__/security-hardening.test.ts @@ -0,0 +1,126 @@ +/** + * Security hardening regression tests. + * + * Covers: + * 1. MCP tool result Unicode sanitization + * 2. Sandbox settings source filtering (exclude projectSettings) + * 3. Plugin git clone/pull hooks disabled + * 4. ANTHROPIC_FOUNDRY_API_KEY removed from SAFE_ENV_VARS + * 5. WebFetch SSRF protection via ssrfGuardedLookup + */ + +import { describe, test, expect } from 'bun:test' +import { resolve } from 'path' + +const SRC = resolve(import.meta.dir, '..') +const file = (relative: string) => Bun.file(resolve(SRC, relative)) + +// --------------------------------------------------------------------------- +// Fix 1: MCP tool result Unicode sanitization +// --------------------------------------------------------------------------- +describe('MCP tool result sanitization', () => { + test('transformResultContent sanitizes text content', async () => { + const content = await file('services/mcp/client.ts').text() + // Tool definitions are already sanitized (line ~1798) + expect(content).toContain('recursivelySanitizeUnicode(result.tools)') + // Tool results must also be sanitized + expect(content).toMatch( + /case 'text':[\s\S]*?recursivelySanitizeUnicode\(resultContent\.text\)/, + ) + }) + + test('resource text content is also sanitized', async () => { + const content = await file('services/mcp/client.ts').text() + expect(content).toMatch( + /recursivelySanitizeUnicode\(\s*`\$\{prefix\}\$\{resource\.text\}`/, + ) + }) +}) + +// --------------------------------------------------------------------------- +// Fix 2: Sandbox settings source filtering +// --------------------------------------------------------------------------- +describe('Sandbox settings trust boundary', () => { + test('getSandboxEnabledSetting does not use getSettings_DEPRECATED', async () => { + const content = await file('utils/sandbox/sandbox-adapter.ts').text() + // Extract the getSandboxEnabledSetting function body + const fnMatch = content.match( + /function getSandboxEnabledSetting\(\)[^{]*\{([\s\S]*?)\n\}/, + ) + expect(fnMatch).not.toBeNull() + const fnBody = fnMatch![1] + // Must NOT use getSettings_DEPRECATED (reads all sources including project) + expect(fnBody).not.toContain('getSettings_DEPRECATED') + // Must use getSettingsForSource for individual trusted sources + expect(fnBody).toContain("getSettingsForSource('userSettings')") + expect(fnBody).toContain("getSettingsForSource('policySettings')") + // Must NOT read from projectSettings + expect(fnBody).not.toContain("'projectSettings'") + }) +}) + +// --------------------------------------------------------------------------- +// Fix 3: Plugin git hooks disabled +// --------------------------------------------------------------------------- +describe('Plugin git operations disable hooks', () => { + test('gitClone includes core.hooksPath=/dev/null', async () => { + const content = await file('utils/plugins/marketplaceManager.ts').text() + // The clone args must disable hooks + const cloneSection = content.slice( + content.indexOf('export async function gitClone('), + content.indexOf('export async function gitClone(') + 2000, + ) + expect(cloneSection).toContain("'core.hooksPath=/dev/null'") + }) + + test('gitPull includes core.hooksPath=/dev/null', async () => { + const content = await file('utils/plugins/marketplaceManager.ts').text() + const pullSection = content.slice( + content.indexOf('export async function gitPull('), + content.indexOf('export async function gitPull(') + 2000, + ) + expect(pullSection).toContain("'core.hooksPath=/dev/null'") + }) + + test('gitSubmoduleUpdate includes core.hooksPath=/dev/null', async () => { + const content = await file('utils/plugins/marketplaceManager.ts').text() + const subSection = content.slice( + content.indexOf('async function gitSubmoduleUpdate('), + content.indexOf('async function gitSubmoduleUpdate(') + 1000, + ) + expect(subSection).toContain("'core.hooksPath=/dev/null'") + }) +}) + +// --------------------------------------------------------------------------- +// Fix 4: ANTHROPIC_FOUNDRY_API_KEY not in SAFE_ENV_VARS +// --------------------------------------------------------------------------- +describe('SAFE_ENV_VARS excludes credentials', () => { + test('ANTHROPIC_FOUNDRY_API_KEY is not in SAFE_ENV_VARS', async () => { + const content = await file('utils/managedEnvConstants.ts').text() + // Extract the SAFE_ENV_VARS set definition + const safeStart = content.indexOf('export const SAFE_ENV_VARS') + const safeEnd = content.indexOf('])', safeStart) + const safeSection = content.slice(safeStart, safeEnd) + expect(safeSection).not.toContain('ANTHROPIC_FOUNDRY_API_KEY') + }) +}) + +// --------------------------------------------------------------------------- +// Fix 5: WebFetch SSRF protection +// --------------------------------------------------------------------------- +describe('WebFetch SSRF guard', () => { + test('getWithPermittedRedirects uses ssrfGuardedLookup', async () => { + const content = await file('tools/WebFetchTool/utils.ts').text() + expect(content).toContain( + "import { ssrfGuardedLookup } from '../../utils/hooks/ssrfGuard.js'", + ) + // The axios.get call in getWithPermittedRedirects must include lookup + const fnSection = content.slice( + content.indexOf('export async function getWithPermittedRedirects('), + content.indexOf('export async function getWithPermittedRedirects(') + + 1000, + ) + expect(fnSection).toContain('lookup: ssrfGuardedLookup') + }) +}) diff --git a/src/services/mcp/client.ts b/src/services/mcp/client.ts index 0f2bbf15..50db733b 100644 --- a/src/services/mcp/client.ts +++ b/src/services/mcp/client.ts @@ -2524,7 +2524,7 @@ export async function transformResultContent( return [ { type: 'text', - text: resultContent.text, + text: recursivelySanitizeUnicode(resultContent.text) as string, }, ] case 'audio': { @@ -2569,7 +2569,9 @@ export async function transformResultContent( return [ { type: 'text', - text: `${prefix}${resource.text}`, + text: recursivelySanitizeUnicode( + `${prefix}${resource.text}`, + ) as string, }, ] } else if ('blob' in resource) { diff --git a/src/tools/WebFetchTool/utils.ts b/src/tools/WebFetchTool/utils.ts index 5e18166a..eea52960 100644 --- a/src/tools/WebFetchTool/utils.ts +++ b/src/tools/WebFetchTool/utils.ts @@ -15,6 +15,7 @@ import { } from '../../utils/mcpOutputStorage.js' import { getSettings_DEPRECATED } from '../../utils/settings/settings.js' import { asSystemPrompt } from '../../utils/systemPromptType.js' +import { ssrfGuardedLookup } from '../../utils/hooks/ssrfGuard.js' import { isPreapprovedHost } from './preapproved.js' import { makeSecondaryModelPrompt } from './prompt.js' @@ -281,6 +282,7 @@ export async function getWithPermittedRedirects( maxRedirects: 0, responseType: 'arraybuffer', maxContentLength: MAX_HTTP_CONTENT_LENGTH, + lookup: ssrfGuardedLookup, headers: { Accept: 'text/markdown, text/html, */*', 'User-Agent': getWebFetchUserAgent(), diff --git a/src/utils/managedEnvConstants.ts b/src/utils/managedEnvConstants.ts index 86b2da29..86a5172c 100644 --- a/src/utils/managedEnvConstants.ts +++ b/src/utils/managedEnvConstants.ts @@ -123,7 +123,6 @@ export const SAFE_ENV_VARS = new Set([ 'ANTHROPIC_DEFAULT_SONNET_MODEL_DESCRIPTION', 'ANTHROPIC_DEFAULT_SONNET_MODEL_NAME', 'ANTHROPIC_DEFAULT_SONNET_MODEL_SUPPORTED_CAPABILITIES', - 'ANTHROPIC_FOUNDRY_API_KEY', 'ANTHROPIC_MODEL', 'ANTHROPIC_SMALL_FAST_MODEL_AWS_REGION', 'ANTHROPIC_SMALL_FAST_MODEL', diff --git a/src/utils/plugins/marketplaceManager.ts b/src/utils/plugins/marketplaceManager.ts index c7c84b61..62f5b1e5 100644 --- a/src/utils/plugins/marketplaceManager.ts +++ b/src/utils/plugins/marketplaceManager.ts @@ -532,6 +532,7 @@ export async function gitPull( ): Promise<{ code: number; stderr: string }> { logForDebugging(`git pull: cwd=${cwd} ref=${ref ?? 'default'}`) const env = { ...process.env, ...GIT_NO_PROMPT_ENV } + const baseArgs = ['-c', 'core.hooksPath=/dev/null'] const credentialArgs = options?.disableCredentialHelper ? ['-c', 'credential.helper='] : [] @@ -539,7 +540,7 @@ export async function gitPull( if (ref) { const fetchResult = await execFileNoThrowWithCwd( gitExe(), - [...credentialArgs, 'fetch', 'origin', ref], + [...baseArgs, ...credentialArgs, 'fetch', 'origin', ref], { cwd, timeout: getPluginGitTimeoutMs(), stdin: 'ignore', env }, ) @@ -549,7 +550,7 @@ export async function gitPull( const checkoutResult = await execFileNoThrowWithCwd( gitExe(), - [...credentialArgs, 'checkout', ref], + [...baseArgs, ...credentialArgs, 'checkout', ref], { cwd, timeout: getPluginGitTimeoutMs(), stdin: 'ignore', env }, ) @@ -559,7 +560,7 @@ export async function gitPull( const pullResult = await execFileNoThrowWithCwd( gitExe(), - [...credentialArgs, 'pull', 'origin', ref], + [...baseArgs, ...credentialArgs, 'pull', 'origin', ref], { cwd, timeout: getPluginGitTimeoutMs(), stdin: 'ignore', env }, ) if (pullResult.code !== 0) { @@ -571,7 +572,7 @@ export async function gitPull( const result = await execFileNoThrowWithCwd( gitExe(), - [...credentialArgs, 'pull', 'origin', 'HEAD'], + [...baseArgs, ...credentialArgs, 'pull', 'origin', 'HEAD'], { cwd, timeout: getPluginGitTimeoutMs(), stdin: 'ignore', env }, ) if (result.code !== 0) { @@ -625,6 +626,8 @@ async function gitSubmoduleUpdate( [ '-c', 'core.sshCommand=ssh -o BatchMode=yes -o StrictHostKeyChecking=yes', + '-c', + 'core.hooksPath=/dev/null', ...credentialArgs, 'submodule', 'update', @@ -810,6 +813,8 @@ export async function gitClone( const args = [ '-c', 'core.sshCommand=ssh -o BatchMode=yes -o StrictHostKeyChecking=yes', + '-c', + 'core.hooksPath=/dev/null', 'clone', '--depth', '1', diff --git a/src/utils/sandbox/sandbox-adapter.ts b/src/utils/sandbox/sandbox-adapter.ts index 170ecac7..37a0eadb 100644 --- a/src/utils/sandbox/sandbox-adapter.ts +++ b/src/utils/sandbox/sandbox-adapter.ts @@ -456,10 +456,19 @@ const checkDependencies = memoize((): SandboxDependencyCheck => { }) }) +/** + * Read sandbox.enabled only from trusted settings sources. + * projectSettings is intentionally excluded — a malicious repo could + * otherwise disable the sandbox via .claude/settings.json. + */ function getSandboxEnabledSetting(): boolean { try { - const settings = getSettings_DEPRECATED() - return settings?.sandbox?.enabled ?? false + return !!( + getSettingsForSource('userSettings')?.sandbox?.enabled || + getSettingsForSource('localSettings')?.sandbox?.enabled || + getSettingsForSource('flagSettings')?.sandbox?.enabled || + getSettingsForSource('policySettings')?.sandbox?.enabled + ) } catch (error) { logForDebugging(`Failed to get settings for sandbox check: ${error}`) return false