diff --git a/src/__tests__/security-hardening.test.ts b/src/__tests__/security-hardening.test.ts new file mode 100644 index 00000000..c32cb62a --- /dev/null +++ b/src/__tests__/security-hardening.test.ts @@ -0,0 +1,191 @@ +/** + * 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') + }) +}) + +// --------------------------------------------------------------------------- +// Fix 6: Swarm permission file polling removed (security hardening) +// --------------------------------------------------------------------------- +describe('Swarm permission file polling removed', () => { + test('useSwarmPermissionPoller hook no longer exists', async () => { + const content = await file( + 'hooks/useSwarmPermissionPoller.ts', + ).text() + // The file-based polling hook must not exist — it read from an + // unauthenticated resolved/ directory where any local process could + // forge approval files. + expect(content).not.toContain('function useSwarmPermissionPoller(') + // The file-based processResponse must not exist + expect(content).not.toContain('function processResponse(') + }) + + test('poller does not import from permissionSync', async () => { + const content = await file( + 'hooks/useSwarmPermissionPoller.ts', + ).text() + // Must not import anything from permissionSync — all file-based + // functions have been removed from this module's dependencies + expect(content).not.toContain('permissionSync') + }) + + test('file-based permission functions are marked deprecated', async () => { + const content = await file( + 'utils/swarm/permissionSync.ts', + ).text() + // All file-based functions must have @deprecated JSDoc + const deprecatedFns = [ + 'writePermissionRequest', + 'readPendingPermissions', + 'readResolvedPermission', + 'resolvePermission', + 'pollForResponse', + 'removeWorkerResponse', + ] + for (const fn of deprecatedFns) { + // Find the function and check that @deprecated appears before it + const fnIndex = content.indexOf(`export async function ${fn}(`) + if (fnIndex === -1) continue // submitPermissionRequest is a const, not async function + const preceding = content.slice(Math.max(0, fnIndex - 500), fnIndex) + expect(preceding).toContain('@deprecated') + } + }) + + test('mailbox-based functions are NOT deprecated', async () => { + const content = await file( + 'utils/swarm/permissionSync.ts', + ).text() + // These are the active path — must not be deprecated + const activeFns = [ + 'sendPermissionRequestViaMailbox', + 'sendPermissionResponseViaMailbox', + ] + for (const fn of activeFns) { + const fnIndex = content.indexOf(`export async function ${fn}(`) + expect(fnIndex).not.toBe(-1) + const preceding = content.slice(Math.max(0, fnIndex - 300), fnIndex) + expect(preceding).not.toContain('@deprecated') + } + }) +}) \ No newline at end of file diff --git a/src/hooks/useSwarmPermissionPoller.ts b/src/hooks/useSwarmPermissionPoller.ts index 0223cef1..710aa141 100644 --- a/src/hooks/useSwarmPermissionPoller.ts +++ b/src/hooks/useSwarmPermissionPoller.ts @@ -1,34 +1,23 @@ /** - * Swarm Permission Poller Hook + * Swarm Permission Callback Registry * - * This hook polls for permission responses from the team leader when running - * as a worker agent in a swarm. When a response is received, it calls the - * appropriate callback (onAllow/onReject) to continue execution. + * Manages callback registrations for permission requests and responses + * in agent swarms. Responses are delivered exclusively via the mailbox + * system (useInboxPoller → processMailboxPermissionResponse). * - * This hook should be used in conjunction with the worker-side integration - * in useCanUseTool.ts, which creates pending requests that this hook monitors. + * The legacy file-based polling (resolved/ directory) has been removed + * because it created an unauthenticated attack surface — any local process + * could forge approval files. The mailbox path is the sole active channel. */ -import { useCallback, useEffect, useRef } from 'react' -import { useInterval } from 'usehooks-ts' import { logForDebugging } from '../utils/debug.js' -import { errorMessage } from '../utils/errors.js' import { type PermissionUpdate, permissionUpdateSchema, } from '../utils/permissions/PermissionUpdateSchema.js' -import { - isSwarmWorker, - type PermissionResponse, - pollForResponse, - removeWorkerResponse, -} from '../utils/swarm/permissionSync.js' -import { getAgentName, getTeamName } from '../utils/teammate.js' - -const POLL_INTERVAL_MS = 500 /** - * Validate permissionUpdates from external sources (mailbox IPC, disk polling). + * Validate permissionUpdates from external sources (mailbox IPC). * Malformed entries from buggy/old teammate processes are filtered out rather * than propagated unchecked into callback.onAllow(). */ @@ -225,106 +214,9 @@ export function processSandboxPermissionResponse(params: { return true } -/** - * Process a permission response by invoking the registered callback - */ -function processResponse(response: PermissionResponse): boolean { - const callback = pendingCallbacks.get(response.requestId) - - if (!callback) { - logForDebugging( - `[SwarmPermissionPoller] No callback registered for request ${response.requestId}`, - ) - return false - } - - logForDebugging( - `[SwarmPermissionPoller] Processing response for request ${response.requestId}: ${response.decision}`, - ) - - // Remove from registry before invoking callback - pendingCallbacks.delete(response.requestId) - - if (response.decision === 'approved') { - const permissionUpdates = parsePermissionUpdates(response.permissionUpdates) - const updatedInput = response.updatedInput - callback.onAllow(updatedInput, permissionUpdates) - } else { - callback.onReject(response.feedback) - } - - return true -} - -/** - * Hook that polls for permission responses when running as a swarm worker. - * - * This hook: - * 1. Only activates when isSwarmWorker() returns true - * 2. Polls every 500ms for responses - * 3. When a response is found, invokes the registered callback - * 4. Cleans up the response file after processing - */ -export function useSwarmPermissionPoller(): void { - const isProcessingRef = useRef(false) - - const poll = useCallback(async () => { - // Don't poll if not a swarm worker - if (!isSwarmWorker()) { - return - } - - // Prevent concurrent polling - if (isProcessingRef.current) { - return - } - - // Don't poll if no callbacks are registered - if (pendingCallbacks.size === 0) { - return - } - - isProcessingRef.current = true - - try { - const agentName = getAgentName() - const teamName = getTeamName() - - if (!agentName || !teamName) { - return - } - - // Check each pending request for a response - for (const [requestId, _callback] of pendingCallbacks) { - const response = await pollForResponse(requestId, agentName, teamName) - - if (response) { - // Process the response - const processed = processResponse(response) - - if (processed) { - // Clean up the response from the worker's inbox - await removeWorkerResponse(requestId, agentName, teamName) - } - } - } - } catch (error) { - logForDebugging( - `[SwarmPermissionPoller] Error during poll: ${errorMessage(error)}`, - ) - } finally { - isProcessingRef.current = false - } - }, []) - - // Only poll if we're a swarm worker - const shouldPoll = isSwarmWorker() - useInterval(() => void poll(), shouldPoll ? POLL_INTERVAL_MS : null) - - // Initial poll on mount - useEffect(() => { - if (isSwarmWorker()) { - void poll() - } - }, [poll]) -} +// Legacy file-based polling (useSwarmPermissionPoller, processResponse) +// has been removed. Permission responses are now delivered exclusively +// via the mailbox system: +// Leader: sendPermissionResponseViaMailbox() → writeToMailbox() +// Worker: useInboxPoller → processMailboxPermissionResponse() +// See: fix(security) — remove unauthenticated file-based permission channel 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 diff --git a/src/utils/swarm/permissionSync.ts b/src/utils/swarm/permissionSync.ts index 001e7a26..1ebea56a 100644 --- a/src/utils/swarm/permissionSync.ts +++ b/src/utils/swarm/permissionSync.ts @@ -207,6 +207,10 @@ export function createPermissionRequest(params: { } /** + * @deprecated Use sendPermissionRequestViaMailbox() instead. This file-based + * approach writes to an unauthenticated directory where any local process can + * forge requests. Retained for backward compatibility but no longer called. + * * Write a permission request to the pending directory with file locking * Called by worker agents when they need permission approval from the leader * @@ -250,6 +254,10 @@ export async function writePermissionRequest( } /** + * @deprecated No longer called — permission requests are sent via mailbox. + * The pending directory is an unauthenticated channel. Retained for backward + * compatibility. + * * Read all pending permission requests for a team * Called by the team leader to see what requests need attention */ @@ -312,6 +320,11 @@ export async function readPendingPermissions( } /** + * @deprecated No longer called — permission responses are delivered via mailbox + * (processMailboxPermissionResponse). The resolved directory is an unauthenticated + * channel where any local process can forge approvals. Retained for backward + * compatibility. + * * Read a resolved permission request by ID * Called by workers to check if their request has been resolved * @@ -352,6 +365,10 @@ export async function readResolvedPermission( } /** + * @deprecated Use sendPermissionResponseViaMailbox() instead. This file-based + * approach writes to an unauthenticated directory where any local process can + * forge approvals. Retained for backward compatibility but no longer called. + * * Resolve a permission request * Called by the team leader (or worker in self-resolution cases) * @@ -536,6 +553,10 @@ export type PermissionResponse = { } /** + * @deprecated Use processMailboxPermissionResponse() via useInboxPoller instead. + * File-based polling reads from an unauthenticated directory where any local + * process can forge approval files. Retained for backward compatibility. + * * Poll for a permission response (worker-side convenience function) * Converts the resolved request into a simpler response format * @@ -564,6 +585,9 @@ export async function pollForResponse( } /** + * @deprecated File-based response cleanup is no longer needed — responses are + * delivered via mailbox. Retained for backward compatibility. + * * Remove a worker's response after processing * This is an alias for deleteResolvedPermission for backward compatibility */ @@ -601,6 +625,9 @@ export function isSwarmWorker(): boolean { } /** + * @deprecated File-based resolved permissions are no longer written. Responses + * are delivered via mailbox. Retained for backward compatibility. + * * Delete a resolved permission file * Called after a worker has processed the resolution */ @@ -635,8 +662,8 @@ export async function deleteResolvedPermission( } /** - * Submit a permission request (alias for writePermissionRequest) - * Provided for backward compatibility with worker integration code + * @deprecated Alias for writePermissionRequest, which is itself deprecated. + * Use sendPermissionRequestViaMailbox() instead. */ export const submitPermissionRequest = writePermissionRequest