fix(security): harden project settings trust boundary + MCP sanitization
- Sanitize MCP tool result text with recursivelySanitizeUnicode() to prevent Unicode injection via malicious MCP servers (tool definitions and prompts were already sanitized, but tool call results were not) - Read sandbox.enabled only from trusted settings sources (user, local, flag, policy) — exclude projectSettings to prevent malicious repos from silently disabling the sandbox via .claude/settings.json - Disable git hooks in plugin marketplace clone/pull/submodule operations with core.hooksPath=/dev/null to prevent code execution from cloned repos - Remove ANTHROPIC_FOUNDRY_API_KEY from SAFE_ENV_VARS to prevent credential injection from project-scoped settings without trust verification - Add ssrfGuardedLookup to WebFetch HTTP requests to block DNS rebinding attacks that could reach cloud metadata or internal services Security: closes trust boundary gap where project settings could override security-critical configuration. Follows the existing pattern established by hasAllowBypassPermissionsMode() which already excludes projectSettings. Co-authored-by: auriti <auriti@users.noreply.github.com>
This commit is contained in:
126
src/__tests__/security-hardening.test.ts
Normal file
126
src/__tests__/security-hardening.test.ts
Normal file
@@ -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')
|
||||||
|
})
|
||||||
|
})
|
||||||
@@ -2524,7 +2524,7 @@ export async function transformResultContent(
|
|||||||
return [
|
return [
|
||||||
{
|
{
|
||||||
type: 'text',
|
type: 'text',
|
||||||
text: resultContent.text,
|
text: recursivelySanitizeUnicode(resultContent.text) as string,
|
||||||
},
|
},
|
||||||
]
|
]
|
||||||
case 'audio': {
|
case 'audio': {
|
||||||
@@ -2569,7 +2569,9 @@ export async function transformResultContent(
|
|||||||
return [
|
return [
|
||||||
{
|
{
|
||||||
type: 'text',
|
type: 'text',
|
||||||
text: `${prefix}${resource.text}`,
|
text: recursivelySanitizeUnicode(
|
||||||
|
`${prefix}${resource.text}`,
|
||||||
|
) as string,
|
||||||
},
|
},
|
||||||
]
|
]
|
||||||
} else if ('blob' in resource) {
|
} else if ('blob' in resource) {
|
||||||
|
|||||||
@@ -15,6 +15,7 @@ import {
|
|||||||
} from '../../utils/mcpOutputStorage.js'
|
} from '../../utils/mcpOutputStorage.js'
|
||||||
import { getSettings_DEPRECATED } from '../../utils/settings/settings.js'
|
import { getSettings_DEPRECATED } from '../../utils/settings/settings.js'
|
||||||
import { asSystemPrompt } from '../../utils/systemPromptType.js'
|
import { asSystemPrompt } from '../../utils/systemPromptType.js'
|
||||||
|
import { ssrfGuardedLookup } from '../../utils/hooks/ssrfGuard.js'
|
||||||
import { isPreapprovedHost } from './preapproved.js'
|
import { isPreapprovedHost } from './preapproved.js'
|
||||||
import { makeSecondaryModelPrompt } from './prompt.js'
|
import { makeSecondaryModelPrompt } from './prompt.js'
|
||||||
|
|
||||||
@@ -281,6 +282,7 @@ export async function getWithPermittedRedirects(
|
|||||||
maxRedirects: 0,
|
maxRedirects: 0,
|
||||||
responseType: 'arraybuffer',
|
responseType: 'arraybuffer',
|
||||||
maxContentLength: MAX_HTTP_CONTENT_LENGTH,
|
maxContentLength: MAX_HTTP_CONTENT_LENGTH,
|
||||||
|
lookup: ssrfGuardedLookup,
|
||||||
headers: {
|
headers: {
|
||||||
Accept: 'text/markdown, text/html, */*',
|
Accept: 'text/markdown, text/html, */*',
|
||||||
'User-Agent': getWebFetchUserAgent(),
|
'User-Agent': getWebFetchUserAgent(),
|
||||||
|
|||||||
@@ -123,7 +123,6 @@ export const SAFE_ENV_VARS = new Set([
|
|||||||
'ANTHROPIC_DEFAULT_SONNET_MODEL_DESCRIPTION',
|
'ANTHROPIC_DEFAULT_SONNET_MODEL_DESCRIPTION',
|
||||||
'ANTHROPIC_DEFAULT_SONNET_MODEL_NAME',
|
'ANTHROPIC_DEFAULT_SONNET_MODEL_NAME',
|
||||||
'ANTHROPIC_DEFAULT_SONNET_MODEL_SUPPORTED_CAPABILITIES',
|
'ANTHROPIC_DEFAULT_SONNET_MODEL_SUPPORTED_CAPABILITIES',
|
||||||
'ANTHROPIC_FOUNDRY_API_KEY',
|
|
||||||
'ANTHROPIC_MODEL',
|
'ANTHROPIC_MODEL',
|
||||||
'ANTHROPIC_SMALL_FAST_MODEL_AWS_REGION',
|
'ANTHROPIC_SMALL_FAST_MODEL_AWS_REGION',
|
||||||
'ANTHROPIC_SMALL_FAST_MODEL',
|
'ANTHROPIC_SMALL_FAST_MODEL',
|
||||||
|
|||||||
@@ -532,6 +532,7 @@ export async function gitPull(
|
|||||||
): Promise<{ code: number; stderr: string }> {
|
): Promise<{ code: number; stderr: string }> {
|
||||||
logForDebugging(`git pull: cwd=${cwd} ref=${ref ?? 'default'}`)
|
logForDebugging(`git pull: cwd=${cwd} ref=${ref ?? 'default'}`)
|
||||||
const env = { ...process.env, ...GIT_NO_PROMPT_ENV }
|
const env = { ...process.env, ...GIT_NO_PROMPT_ENV }
|
||||||
|
const baseArgs = ['-c', 'core.hooksPath=/dev/null']
|
||||||
const credentialArgs = options?.disableCredentialHelper
|
const credentialArgs = options?.disableCredentialHelper
|
||||||
? ['-c', 'credential.helper=']
|
? ['-c', 'credential.helper=']
|
||||||
: []
|
: []
|
||||||
@@ -539,7 +540,7 @@ export async function gitPull(
|
|||||||
if (ref) {
|
if (ref) {
|
||||||
const fetchResult = await execFileNoThrowWithCwd(
|
const fetchResult = await execFileNoThrowWithCwd(
|
||||||
gitExe(),
|
gitExe(),
|
||||||
[...credentialArgs, 'fetch', 'origin', ref],
|
[...baseArgs, ...credentialArgs, 'fetch', 'origin', ref],
|
||||||
{ cwd, timeout: getPluginGitTimeoutMs(), stdin: 'ignore', env },
|
{ cwd, timeout: getPluginGitTimeoutMs(), stdin: 'ignore', env },
|
||||||
)
|
)
|
||||||
|
|
||||||
@@ -549,7 +550,7 @@ export async function gitPull(
|
|||||||
|
|
||||||
const checkoutResult = await execFileNoThrowWithCwd(
|
const checkoutResult = await execFileNoThrowWithCwd(
|
||||||
gitExe(),
|
gitExe(),
|
||||||
[...credentialArgs, 'checkout', ref],
|
[...baseArgs, ...credentialArgs, 'checkout', ref],
|
||||||
{ cwd, timeout: getPluginGitTimeoutMs(), stdin: 'ignore', env },
|
{ cwd, timeout: getPluginGitTimeoutMs(), stdin: 'ignore', env },
|
||||||
)
|
)
|
||||||
|
|
||||||
@@ -559,7 +560,7 @@ export async function gitPull(
|
|||||||
|
|
||||||
const pullResult = await execFileNoThrowWithCwd(
|
const pullResult = await execFileNoThrowWithCwd(
|
||||||
gitExe(),
|
gitExe(),
|
||||||
[...credentialArgs, 'pull', 'origin', ref],
|
[...baseArgs, ...credentialArgs, 'pull', 'origin', ref],
|
||||||
{ cwd, timeout: getPluginGitTimeoutMs(), stdin: 'ignore', env },
|
{ cwd, timeout: getPluginGitTimeoutMs(), stdin: 'ignore', env },
|
||||||
)
|
)
|
||||||
if (pullResult.code !== 0) {
|
if (pullResult.code !== 0) {
|
||||||
@@ -571,7 +572,7 @@ export async function gitPull(
|
|||||||
|
|
||||||
const result = await execFileNoThrowWithCwd(
|
const result = await execFileNoThrowWithCwd(
|
||||||
gitExe(),
|
gitExe(),
|
||||||
[...credentialArgs, 'pull', 'origin', 'HEAD'],
|
[...baseArgs, ...credentialArgs, 'pull', 'origin', 'HEAD'],
|
||||||
{ cwd, timeout: getPluginGitTimeoutMs(), stdin: 'ignore', env },
|
{ cwd, timeout: getPluginGitTimeoutMs(), stdin: 'ignore', env },
|
||||||
)
|
)
|
||||||
if (result.code !== 0) {
|
if (result.code !== 0) {
|
||||||
@@ -625,6 +626,8 @@ async function gitSubmoduleUpdate(
|
|||||||
[
|
[
|
||||||
'-c',
|
'-c',
|
||||||
'core.sshCommand=ssh -o BatchMode=yes -o StrictHostKeyChecking=yes',
|
'core.sshCommand=ssh -o BatchMode=yes -o StrictHostKeyChecking=yes',
|
||||||
|
'-c',
|
||||||
|
'core.hooksPath=/dev/null',
|
||||||
...credentialArgs,
|
...credentialArgs,
|
||||||
'submodule',
|
'submodule',
|
||||||
'update',
|
'update',
|
||||||
@@ -810,6 +813,8 @@ export async function gitClone(
|
|||||||
const args = [
|
const args = [
|
||||||
'-c',
|
'-c',
|
||||||
'core.sshCommand=ssh -o BatchMode=yes -o StrictHostKeyChecking=yes',
|
'core.sshCommand=ssh -o BatchMode=yes -o StrictHostKeyChecking=yes',
|
||||||
|
'-c',
|
||||||
|
'core.hooksPath=/dev/null',
|
||||||
'clone',
|
'clone',
|
||||||
'--depth',
|
'--depth',
|
||||||
'1',
|
'1',
|
||||||
|
|||||||
@@ -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 {
|
function getSandboxEnabledSetting(): boolean {
|
||||||
try {
|
try {
|
||||||
const settings = getSettings_DEPRECATED()
|
return !!(
|
||||||
return settings?.sandbox?.enabled ?? false
|
getSettingsForSource('userSettings')?.sandbox?.enabled ||
|
||||||
|
getSettingsForSource('localSettings')?.sandbox?.enabled ||
|
||||||
|
getSettingsForSource('flagSettings')?.sandbox?.enabled ||
|
||||||
|
getSettingsForSource('policySettings')?.sandbox?.enabled
|
||||||
|
)
|
||||||
} catch (error) {
|
} catch (error) {
|
||||||
logForDebugging(`Failed to get settings for sandbox check: ${error}`)
|
logForDebugging(`Failed to get settings for sandbox check: ${error}`)
|
||||||
return false
|
return false
|
||||||
|
|||||||
Reference in New Issue
Block a user