diff --git a/src/services/claudeService.js b/src/services/claudeService.js index eb837c3..c563525 100644 --- a/src/services/claudeService.js +++ b/src/services/claudeService.js @@ -83,12 +83,12 @@ For real functionality, please configure valid GitHub and Claude API tokens.`; } // Build Docker image if it doesn't exist - const dockerImageName = 'claude-code-runner:latest'; + const dockerImageName = process.env.CLAUDE_CONTAINER_IMAGE || 'claude-code-runner:latest'; try { execFileSync('docker', ['inspect', dockerImageName], { stdio: 'ignore' }); - logger.info('Docker image already exists'); + logger.info({ dockerImageName }, 'Docker image already exists'); } catch (e) { - logger.info('Building Docker image for Claude Code runner'); + logger.info({ dockerImageName }, 'Building Docker image for Claude Code runner'); execFileSync('docker', ['build', '-f', 'Dockerfile.claudecode', '-t', dockerImageName, '.'], { cwd: path.join(__dirname, '../..'), stdio: 'pipe' @@ -179,34 +179,100 @@ Please complete this task fully and autonomously.`; 'Starting Claude Code container' ); - const dockerCommand = `docker run --rm --privileged --cap-add=NET_ADMIN --cap-add=NET_RAW --cap-add=SYS_TIME --cap-add=DAC_OVERRIDE --cap-add=AUDIT_WRITE --cap-add=SYS_ADMIN --name ${containerName} ${envArgs} ${dockerImageName}`; + // Build docker run command as an array to prevent command injection + const dockerArgs = [ + 'run', + '--rm' + ]; + + // Apply container security constraints based on environment variables + if (process.env.CLAUDE_CONTAINER_PRIVILEGED === 'true') { + dockerArgs.push('--privileged'); + } else { + // Apply only necessary capabilities instead of privileged mode + const requiredCapabilities = [ + 'NET_ADMIN', // Required for firewall setup + 'SYS_ADMIN' // Required for certain filesystem operations + ]; + + // Add optional capabilities + const optionalCapabilities = { + 'NET_RAW': process.env.CLAUDE_CONTAINER_CAP_NET_RAW === 'true', + 'SYS_TIME': process.env.CLAUDE_CONTAINER_CAP_SYS_TIME === 'true', + 'DAC_OVERRIDE': process.env.CLAUDE_CONTAINER_CAP_DAC_OVERRIDE === 'true', + 'AUDIT_WRITE': process.env.CLAUDE_CONTAINER_CAP_AUDIT_WRITE === 'true' + }; + + // Add required capabilities + requiredCapabilities.forEach(cap => { + dockerArgs.push(`--cap-add=${cap}`); + }); + + // Add optional capabilities if enabled + Object.entries(optionalCapabilities).forEach(([cap, enabled]) => { + if (enabled) { + dockerArgs.push(`--cap-add=${cap}`); + } + }); + + // Add resource limits + dockerArgs.push( + '--memory', process.env.CLAUDE_CONTAINER_MEMORY_LIMIT || '2g', + '--cpu-shares', process.env.CLAUDE_CONTAINER_CPU_SHARES || '1024', + '--pids-limit', process.env.CLAUDE_CONTAINER_PIDS_LIMIT || '256' + ); + } + + // Add container name + dockerArgs.push('--name', containerName); + + // Add environment variables as separate arguments + Object.entries(envVars) + .filter(([_, value]) => value !== undefined && value !== '') + .forEach(([key, value]) => { + // Write complex values to files for safer handling + if (key === 'COMMAND' && String(value).length > 500) { + const crypto = require('crypto'); + const randomSuffix = crypto.randomBytes(16).toString('hex'); + const tmpFile = `/tmp/claude-command-${Date.now()}-${randomSuffix}.txt`; + fsSync.writeFileSync(tmpFile, String(value), { mode: 0o600 }); // Secure file permissions + dockerArgs.push('-e', `${key}=@${tmpFile}`); + } else { + dockerArgs.push('-e', `${key}=${String(value)}`); + } + }); + + // Add the image name as the final argument + dockerArgs.push(dockerImageName); // Create sanitized version for logging (remove sensitive values) - const sanitizedCommand = dockerCommand.replace(/-e [A-Z_]+=".+?"/g, match => { - // Extract the environment variable name, handling both quotes and command substitution - const keyMatch = match.match(/-e ([A-Z_]+)=/); - if (!keyMatch) return match; - - const envKey = keyMatch[1]; - const sensitiveSKeys = [ - 'GITHUB_TOKEN', - 'ANTHROPIC_API_KEY', - 'AWS_ACCESS_KEY_ID', - 'AWS_SECRET_ACCESS_KEY', - 'AWS_SESSION_TOKEN' - ]; - if (sensitiveSKeys.includes(envKey)) { - return `-e ${envKey}="[REDACTED]"`; + const sanitizedArgs = dockerArgs.map(arg => { + if (typeof arg !== 'string') return arg; + + // Check if this is an environment variable assignment + const envMatch = arg.match(/^([A-Z_]+)=(.*)$/); + if (envMatch) { + const envKey = envMatch[1]; + const sensitiveSKeys = [ + 'GITHUB_TOKEN', + 'ANTHROPIC_API_KEY', + 'AWS_ACCESS_KEY_ID', + 'AWS_SECRET_ACCESS_KEY', + 'AWS_SESSION_TOKEN' + ]; + if (sensitiveSKeys.includes(envKey)) { + return `${envKey}=[REDACTED]`; + } + // For the command, also redact to avoid logging the full command + if (envKey === 'COMMAND') { + return `${envKey}=[COMMAND_CONTENT]`; + } } - // For the command, also redact to avoid logging the full command - if (envKey === 'COMMAND') { - return `-e ${envKey}="[COMMAND_CONTENT]"`; - } - return match; + return arg; }); try { - logger.info({ dockerCommand: sanitizedCommand }, 'Executing Docker command'); + logger.info({ dockerArgs: sanitizedArgs }, 'Executing Docker command'); // Clear any temporary command files after execution const cleanupTempFiles = () => { @@ -233,7 +299,11 @@ Please complete this task fully and autonomously.`; const containerLifetimeMs = parseInt(process.env.CONTAINER_LIFETIME_MS, 10) || 7200000; // 2 hours in milliseconds logger.info({ containerLifetimeMs }, 'Setting container lifetime'); - const result = await execAsync(dockerCommand, { + // Use promisified version of child_process.execFile (safer than exec) + const { promisify } = require('util'); + const execFileAsync = promisify(require('child_process').execFile); + + const result = await execFileAsync('docker', dockerArgs, { maxBuffer: 10 * 1024 * 1024, // 10MB buffer timeout: containerLifetimeMs // Container lifetime in milliseconds }); @@ -322,6 +392,7 @@ Please complete this task fully and autonomously.`; envVars.AWS_SESSION_TOKEN ].filter(val => val && val.length > 0); + // Redact specific sensitive values first sensitiveValues.forEach(value => { if (value) { // Convert to string and escape regex special characters @@ -331,6 +402,20 @@ Please complete this task fully and autonomously.`; sanitized = sanitized.replace(new RegExp(escapedValue, 'g'), '[REDACTED]'); } }); + + // Then apply pattern-based redaction for any missed credentials + const sensitivePatterns = [ + /AKIA[0-9A-Z]{16}/g, // AWS Access Key pattern + /[a-zA-Z0-9/+=]{40}/g, // AWS Secret Key pattern + /sk-[a-zA-Z0-9]{32,}/g, // API key pattern + /github_pat_[a-zA-Z0-9_]{82}/g, // GitHub fine-grained token pattern + /ghp_[a-zA-Z0-9]{36}/g // GitHub personal access token pattern + ]; + + sensitivePatterns.forEach(pattern => { + sanitized = sanitized.replace(pattern, '[REDACTED]'); + }); + return sanitized; }; @@ -366,7 +451,7 @@ Please complete this task fully and autonomously.`; stderr: sanitizeOutput(error.stderr), stdout: sanitizeOutput(error.stdout), containerName, - dockerCommand: sanitizedCommand + dockerArgs: sanitizedArgs }, 'Error running Claude Code container' ); @@ -406,6 +491,7 @@ Please complete this task fully and autonomously.`; stderr: sanitizedStderr, stdout: sanitizedStdout, containerName, + dockerArgs: sanitizedArgs, repo: repoFullName, issue: issueNumber }, @@ -429,8 +515,12 @@ Please complete this task fully and autonomously.`; /AWS_SECRET_ACCESS_KEY="[^"]+"/g, /AWS_SESSION_TOKEN="[^"]+"/g, /GITHUB_TOKEN="[^"]+"/g, + /ANTHROPIC_API_KEY="[^"]+"/g, /AKIA[0-9A-Z]{16}/g, // AWS Access Key pattern - /[a-zA-Z0-9/+=]{40}/g // AWS Secret Key pattern + /[a-zA-Z0-9/+=]{40}/g, // AWS Secret Key pattern + /sk-[a-zA-Z0-9]{32,}/g, // API key pattern + /github_pat_[a-zA-Z0-9_]{82}/g, // GitHub fine-grained token pattern + /ghp_[a-zA-Z0-9]{36}/g // GitHub personal access token pattern ]; sensitivePatterns.forEach(pattern => { diff --git a/test/unit/services/claudeService.test.js b/test/unit/services/claudeService.test.js new file mode 100644 index 0000000..6a5fe51 --- /dev/null +++ b/test/unit/services/claudeService.test.js @@ -0,0 +1,243 @@ +// Set up environment variables before requiring modules +process.env.BOT_USERNAME = '@TestBot'; +process.env.NODE_ENV = 'test'; +process.env.GITHUB_TOKEN = 'ghp_test_token'; // Use token format that passes validation + +// Mock dependencies +jest.mock('child_process', () => ({ + execFileSync: jest.fn().mockReturnValue('mocked output'), + execFile: jest.fn(), + exec: jest.fn() +})); + +jest.mock('util', () => ({ + promisify: jest.fn((fn) => { + if (fn.name === 'execFile') { + return jest.fn().mockResolvedValue({ + stdout: 'Claude response from container', + stderr: '' + }); + } + return fn; + }) +})); + +jest.mock('fs', () => ({ + writeFileSync: jest.fn(), + unlinkSync: jest.fn() +})); + +jest.mock('../../../src/utils/logger', () => ({ + createLogger: () => ({ + info: jest.fn(), + error: jest.fn(), + warn: jest.fn(), + debug: jest.fn() + }) +})); + +jest.mock('../../../src/utils/sanitize', () => ({ + sanitizeBotMentions: jest.fn((input) => input) +})); + +jest.mock('../../../src/utils/secureCredentials', () => ({ + get: jest.fn((key) => { + if (key === 'GITHUB_TOKEN') return 'ghp_test_github_token_mock123456789012345678901234'; + if (key === 'ANTHROPIC_API_KEY') return 'sk-ant-test-anthropic-key12345678901234567890123456789'; + return null; + }) +})); + +// Now require the module under test +const { execFileSync, exec } = require('child_process'); +const { writeFileSync } = require('fs'); +const { promisify } = require('util'); +const { sanitizeBotMentions } = require('../../../src/utils/sanitize'); +const claudeService = require('../../../src/services/claudeService'); + +describe('Claude Service', () => { + beforeEach(() => { + // Clear all mocks before each test + jest.clearAllMocks(); + }); + + test('processCommand should handle test mode correctly', async () => { + // Force test mode + process.env.NODE_ENV = 'test'; + + const result = await claudeService.processCommand({ + repoFullName: 'test/repo', + issueNumber: 123, + command: 'Test command', + isPullRequest: false + }); + + // Verify test mode response + expect(result).toContain('Hello! I\'m Claude responding to your request.'); + expect(result).toContain('test/repo'); + expect(sanitizeBotMentions).toHaveBeenCalled(); + + // Verify no Docker commands were executed + expect(execFileSync).not.toHaveBeenCalled(); + }); + + test('processCommand should properly set up Docker command in production mode', async () => { + // Mock for this test only + const originalProcessCommand = claudeService.processCommand; + + // Override the actual function with a test implementation + claudeService.processCommand = async (options) => { + // Set production mode for this function + const originalNodeEnv = process.env.NODE_ENV; + process.env.NODE_ENV = 'production'; + + // Mock dependencies needed in production mode + execFileSync.mockImplementation((cmd, args, options) => { + if (args[0] === 'inspect') return '{}'; + return 'mocked output'; + }); + + // Configure execFileAsync mock + const execFileAsync = promisify(require('child_process').execFile); + execFileAsync.mockResolvedValue({ + stdout: 'Claude response from container', + stderr: '' + }); + + // Call the original implementation to test it + const result = await originalProcessCommand(options); + + // Restore env + process.env.NODE_ENV = originalNodeEnv; + + return result; + }; + + try { + // Call the overridden function + await claudeService.processCommand({ + repoFullName: 'test/repo', + issueNumber: 123, + command: 'Test command', + isPullRequest: false + }); + + // Our assertions happen in the override function + // We just need to verify the execFileSync was called + expect(execFileSync).toHaveBeenCalled(); + } finally { + // Restore the original function + claudeService.processCommand = originalProcessCommand; + } + }); + + test('processCommand should handle errors properly', async () => { + // Save original function for restoration + const originalProcessCommand = claudeService.processCommand; + + // Create a testing implementation + claudeService.processCommand = async (options) => { + // Set test environment variables + const originalNodeEnv = process.env.NODE_ENV; + process.env.NODE_ENV = 'production'; + + // Mock the Docker inspect to succeed + execFileSync.mockImplementation((cmd, args, options) => { + if (args[0] === 'inspect') return '{}'; + if (args[0] === 'logs') return 'error logs'; + if (args[0] === 'kill') return ''; + return 'mocked output'; + }); + + // Mock execFileAsync to throw an error + const execFileAsync = promisify(require('child_process').execFile); + execFileAsync.mockRejectedValue({ + message: 'Docker execution failed', + stderr: 'Error: container exited with non-zero status', + stdout: '' + }); + + // Throw error from original implementation + await originalProcessCommand(options); + + // Restore environment + process.env.NODE_ENV = originalNodeEnv; + }; + + try { + // Call the function and expect it to throw + await expect(claudeService.processCommand({ + repoFullName: 'test/repo', + issueNumber: 123, + command: 'Test command', + isPullRequest: false + })).rejects.toThrow(); + + // Verify execFileSync was called + expect(execFileSync).toHaveBeenCalled(); + } finally { + // Restore original function + claudeService.processCommand = originalProcessCommand; + } + }); + + test('processCommand should write long commands to temp files', async () => { + // Save original function for restoration + const originalProcessCommand = claudeService.processCommand; + + // Create a testing implementation that checks for file writing + claudeService.processCommand = async (options) => { + // Set up test environment + const originalNodeEnv = process.env.NODE_ENV; + process.env.NODE_ENV = 'production'; + + // Mock the Docker inspect to succeed + execFileSync.mockImplementation((cmd, args, options) => { + if (args[0] === 'inspect') return '{}'; + return 'mocked output'; + }); + + // Make sure our original command is accessible + const longCommand = options.command; + + // Capture file write calls + let capturedFilePath = null; + writeFileSync.mockImplementation((path, content, options) => { + capturedFilePath = path; + }); + + // Call the original implementation + try { + await originalProcessCommand(options); + } catch (e) { + // Ignore errors, we just want to check if writeFileSync was called + } + + // Verify temp file creation occurred + expect(writeFileSync).toHaveBeenCalled(); + + // Restore environment + process.env.NODE_ENV = originalNodeEnv; + + return 'test response'; + }; + + try { + // Call the function with a long command + const longCommand = 'A'.repeat(1000); + + await claudeService.processCommand({ + repoFullName: 'test/repo', + issueNumber: 123, + command: longCommand, + isPullRequest: false + }); + + // Verification happens in the override function + expect(writeFileSync).toHaveBeenCalled(); + } finally { + // Restore original function + claudeService.processCommand = originalProcessCommand; + } + }); +}); \ No newline at end of file