fix(security): harden project settings trust boundary + MCP sanitization (#789)

* 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> * fix(security): remove unauthenticated file-based permission polling Remove the legacy file-based permission polling from useSwarmPermissionPoller that read from ~/.claude/teams/{name}/permissions/resolved/ — an unauthenticated directory where any local process could forge approval files to auto-approve tool uses for swarm teammates. The file polling was dead code: - The useSwarmPermissionPoller() hook was never mounted by any component - resolvePermission() (the file writer) was never imported outside its module - Permission responses are delivered exclusively via the mailbox system: Leader: sendPermissionResponseViaMailbox() → writeToMailbox() Worker: useInboxPoller → processMailboxPermissionResponse() Changes: - Remove file polling loop, processResponse(), and React hook imports from useSwarmPermissionPoller.ts (now a pure callback registry module) - Mark 7 file-based functions as @deprecated in permissionSync.ts - Add 4 regression tests verifying the removal No exported functions removed — only deprecated. All 5 consumer modules verified: they import only mailbox-based functions that remain unchanged. --------- Co-authored-by: auriti <auriti@users.noreply.github.com>
2026-04-21 12:28:03 +02:00
parent a6a3de5ac1
commit ae3b723f3b
8 changed files with 260 additions and 133 deletions
--- a/src/tests/security-hardening.test.ts
+++ 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')
+    }
+  })
+})
--- 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
--- 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) {
--- 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(),
--- 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',
--- 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',
--- 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
--- 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