refactor(cli-proxy): decompose handleExec into buildExecEnv and runGhCommand

containers/cli-proxy/server.js

@@ -40,6 +40,8 @@ const PROTECTED_ENV_KEYS = Object.freeze({
   [Symbol.iterator]() { return _PROTECTED_ENV_KEYS[Symbol.iterator](); },
 });
 
+const UNSAFE_ENV_KEYS = new Set(['__proto__', 'constructor', 'prototype']);
+
 // --- Structured logging to /var/log/cli-proxy/access.jsonl ---
 
 const LOG_DIR = process.env.AWF_CLI_PROXY_LOG_DIR || '/var/log/cli-proxy';
@@ -201,68 +203,61 @@ function handleHealth(res) {
 }
 
 /**
- * Handle POST /exec
+ * Build the environment object for a subprocess by inheriting the server's environment
+ * and applying caller-supplied overrides, excluding any PROTECTED_ENV_KEYS.
  *
- * Expected request body (JSON):
- * {
- *   "args": ["pr", "list", "--repo", "owner/repo", "--json", "number,title"],
- *   "cwd": "/home/runner/work/repo/repo",   // optional
- *   "stdin": null,                           // optional, base64-encoded or null
- *   "env": { "GH_REPO": "owner/repo" }      // optional extra env vars
- * }
+ * Security-critical: ensures agents cannot override auth or TLS trust-store variables.
  *
- * Response body (JSON):
- * {
- *   "stdout": "...",
- *   "stderr": "...",
- *   "exitCode": 0
- * }
+ * @param {Record<string, string>|null|undefined} extraEnv - Optional caller-supplied env overrides
+ * @returns {NodeJS.ProcessEnv} The merged environment for the child process
  */
-async function handleExec(req, res) {
-  const startTime = Date.now();
-  let body;
-  try {
-    const raw = await readBody(req, res);
-    // null means readBody already sent a 413 error response
-    if (raw === null) return;
-    body = JSON.parse(raw.toString('utf8'));
-  } catch {
-    accessLog({ event: 'exec_error', error: 'Invalid JSON body' });
-    return sendError(res, 400, 'Invalid JSON body');
-  }
-
-  const { args, cwd, stdin, env: extraEnv } = body;
-
-  // Validate args
-  const validation = validateArgs(args);
-  if (!validation.valid) {
-    accessLog({ event: 'exec_denied', args, error: validation.error });
-    return sendError(res, 403, validation.error);
-  }
-
-  accessLog({ event: 'exec_start', args, cwd: cwd || null });
-
-  // Build environment for the subprocess
+function buildExecEnv(extraEnv) {
   // Inherit server environment (includes GH_HOST, NODE_EXTRA_CA_CERTS, GH_REPO, etc.)
   const childEnv = Object.assign({}, process.env);
   if (extraEnv && typeof extraEnv === 'object') {
     // Only allow safe string env overrides; never allow overriding keys in PROTECTED_ENV_KEYS.
     for (const [key, value] of Object.entries(extraEnv)) {
-      if (typeof key === 'string' && typeof value === 'string' && !PROTECTED_ENV_KEYS.has(key)) {
+      if (
+        typeof key === 'string'
+        && typeof value === 'string'
+        && !PROTECTED_ENV_KEYS.has(key)
+        && !UNSAFE_ENV_KEYS.has(key)
+      ) {
         childEnv[key] = value;
       }
     }
   }
+  return childEnv;
+}
 
-  // Execute gh directly (no shell — prevents injection attacks)
-  // Always use the server's own cwd — the agent sends its container workspace
-  // path which doesn't exist inside the cli-proxy container.
-  let stdout = '';
-  let stderr = '';
-  let exitCode = 0;
+/**
+ * Execute `gh` with the given args, environment, and optional base64-encoded stdin.
+ *
+ * Runs gh directly via execFile (no shell — prevents injection attacks).
+ * Always uses the server's own cwd — the agent sends its container workspace
+ * path which doesn't exist inside the cli-proxy container.
+ *
+ * @param {string[]} args - Arguments passed to gh (excluding 'gh' itself)
+ * @param {NodeJS.ProcessEnv} childEnv - Environment for the child process
+ * @param {string|null|undefined} stdin - Optional base64-encoded stdin data
+ * @returns {Promise<{stdout: string, stderr: string, exitCode: number}>}
+ */
+async function runGhCommand(args, childEnv, stdin) {
+  const normalizeExitCode = code => {
+    if (typeof code === 'number' && Number.isFinite(code)) {
+      return code;
+    }
+    if (typeof code === 'string') {
+      const parsedCode = Number.parseInt(code, 10);
+      if (!Number.isNaN(parsedCode)) {
+        return parsedCode;
+      }
+    }
+    return 1;
+  };
 
   try {
-    const result = await new Promise((resolve, reject) => {
+    return await new Promise((resolve, reject) => {
       const child = execFile('gh', args, {
         cwd: process.cwd(),
         env: childEnv,
@@ -278,7 +273,7 @@ async function handleExec(req, res) {
         resolve({
           stdout: childStdout || '',
           stderr: childStderr || '',
-          exitCode: err ? (err.code || 1) : 0,
+          exitCode: err ? normalizeExitCode(err.code) : 0,
         });
       });
 
@@ -295,17 +290,58 @@ async function handleExec(req, res) {
         child.stdin.end();
       }
     });
-
-    stdout = result.stdout;
-    stderr = result.stderr;
-    exitCode = result.exitCode;
   } catch (err) {
     // Only expose a safe message, not a full stack trace
     const errMsg = err instanceof Error ? err.message : 'Command execution failed';
-    stderr = errMsg;
-    exitCode = 1;
+    return { stdout: '', stderr: errMsg, exitCode: 1 };
+  }
+}
+
+/**
+ * Handle POST /exec
+ *
+ * Expected request body (JSON):
+ * {
+ *   "args": ["pr", "list", "--repo", "owner/repo", "--json", "number,title"],
+ *   "cwd": "/home/runner/work/repo/repo",   // optional
+ *   "stdin": null,                           // optional, base64-encoded or null
+ *   "env": { "GH_REPO": "owner/repo" }      // optional extra env vars
+ * }
+ *
+ * Response body (JSON):
+ * {
+ *   "stdout": "...",
+ *   "stderr": "...",
+ *   "exitCode": 0
+ * }
+ */
+async function handleExec(req, res) {
+  const startTime = Date.now();
+  let body;
+  try {
+    const raw = await readBody(req, res);
+    // null means readBody already sent a 413 error response
+    if (raw === null) return;
+    body = JSON.parse(raw.toString('utf8'));
+  } catch {
+    accessLog({ event: 'exec_error', error: 'Invalid JSON body' });
+    return sendError(res, 400, 'Invalid JSON body');
   }
 
+  const { args, cwd, stdin, env: extraEnv } = body;
+
+  // Validate args
+  const validation = validateArgs(args);
+  if (!validation.valid) {
+    accessLog({ event: 'exec_denied', args, error: validation.error });
+    return sendError(res, 403, validation.error);
+  }
+
+  accessLog({ event: 'exec_start', args, cwd: cwd || null });
+
+  const childEnv = buildExecEnv(extraEnv);
+  const { stdout, stderr, exitCode } = await runGhCommand(args, childEnv, stdin);
+
   const responseBody = JSON.stringify({ stdout, stderr, exitCode });
 
   const durationMs = Date.now() - startTime;
@@ -377,4 +413,4 @@ if (require.main === module) {
   });
 }
 
-module.exports = { validateArgs, ALWAYS_DENIED_SUBCOMMANDS, PROTECTED_ENV_KEYS };
+module.exports = { validateArgs, ALWAYS_DENIED_SUBCOMMANDS, PROTECTED_ENV_KEYS, buildExecEnv, runGhCommand };

containers/cli-proxy/server.test.js

@@ -6,7 +6,7 @@
  * The server only enforces meta-command denial (auth, config, extension).
  */
 
-const { validateArgs, ALWAYS_DENIED_SUBCOMMANDS, PROTECTED_ENV_KEYS } = require('./server');
+const { validateArgs, ALWAYS_DENIED_SUBCOMMANDS, PROTECTED_ENV_KEYS, buildExecEnv, runGhCommand } = require('./server');
 
 describe('PROTECTED_ENV_KEYS', () => {
   it('should protect GH_HOST from agent override', () => {
@@ -186,3 +186,114 @@ describe('validateArgs', () => {
     });
   });
 });
+
+describe('buildExecEnv', () => {
+  const originalEnv = process.env;
+
+  beforeEach(() => {
+    process.env = { ...originalEnv, GH_HOST: 'github.com', GH_TOKEN: 'secret-token', GITHUB_TOKEN: 'secret-github-token', NODE_EXTRA_CA_CERTS: '/certs/ca.crt', SSL_CERT_FILE: '/certs/ssl.crt', GIT_SSL_CAINFO: '/certs/git.crt', EXISTING_VAR: 'existing' };
+  });
+
+  afterEach(() => {
+    process.env = originalEnv;
+  });
+
+  it('should inherit server environment variables', () => {
+    const env = buildExecEnv(null);
+    expect(env.EXISTING_VAR).toBe('existing');
+  });
+
+  it('should apply safe extra env vars', () => {
+    const env = buildExecEnv({ MY_VAR: 'my-value' });
+    expect(env.MY_VAR).toBe('my-value');
+  });
+
+  it('should not allow overriding GH_HOST', () => {
+    const env = buildExecEnv({ GH_HOST: 'evil.com' });
+    expect(env.GH_HOST).toBe('github.com');
+  });
+
+  it('should not allow overriding GH_TOKEN', () => {
+    const env = buildExecEnv({ GH_TOKEN: 'stolen' });
+    expect(env.GH_TOKEN).toBe('secret-token');
+  });
+
+  it('should not allow overriding GITHUB_TOKEN', () => {
+    const env = buildExecEnv({ GITHUB_TOKEN: 'stolen' });
+    expect(env.GITHUB_TOKEN).toBe('secret-github-token');
+  });
+
+  it('should not allow overriding NODE_EXTRA_CA_CERTS', () => {
+    const env = buildExecEnv({ NODE_EXTRA_CA_CERTS: '/evil/ca.crt' });
+    expect(env.NODE_EXTRA_CA_CERTS).toBe('/certs/ca.crt');
+  });
+
+  it('should not allow overriding SSL_CERT_FILE', () => {
+    const env = buildExecEnv({ SSL_CERT_FILE: '/evil/ssl.crt' });
+    expect(env.SSL_CERT_FILE).toBe('/certs/ssl.crt');
+  });
+
+  it('should not allow overriding GIT_SSL_CAINFO', () => {
+    const env = buildExecEnv({ GIT_SSL_CAINFO: '/evil/git.crt' });
+    expect(env.GIT_SSL_CAINFO).toBe('/certs/git.crt');
+  });
+
+  it('should ignore dangerous prototype keys from JSON payloads', () => {
+    const extraEnv = JSON.parse('{"__proto__":"polluted","constructor":"polluted","prototype":"polluted","MY_VAR":"ok"}');
+    const env = buildExecEnv(extraEnv);
+    expect(env.MY_VAR).toBe('ok');
+    expect(env.__proto__).toBe(Object.prototype);
+    expect(env.constructor).toBe(Object);
+    expect(env.prototype).toBeUndefined();
+    expect({}.polluted).toBeUndefined();
+  });
+
+  it('should silently skip non-string values', () => {
+    const env = buildExecEnv({ MY_VAR: 42 });
+    expect(env.MY_VAR).toBeUndefined();
+  });
+
+  it('should handle null extraEnv gracefully', () => {
+    const env = buildExecEnv(null);
+    expect(env.EXISTING_VAR).toBe('existing');
+  });
+
+  it('should handle undefined extraEnv gracefully', () => {
+    const env = buildExecEnv(undefined);
+    expect(env.EXISTING_VAR).toBe('existing');
+  });
+
+  it('should handle non-object extraEnv gracefully', () => {
+    const env = buildExecEnv('not-an-object');
+    expect(env.EXISTING_VAR).toBe('existing');
+  });
+});
+
+describe('runGhCommand', () => {
+  it('should return stdout, stderr, and exitCode on success', async () => {
+    const result = await runGhCommand(['--version'], process.env, null);
+    expect(result).toHaveProperty('stdout');
+    expect(result).toHaveProperty('stderr');
+    expect(result).toHaveProperty('exitCode');
+    expect(typeof result.stdout).toBe('string');
+    expect(typeof result.exitCode).toBe('number');
+  });
+
+  it('should return non-zero exitCode for invalid gh subcommand', async () => {
+    const result = await runGhCommand(['__nonexistent_subcommand__'], process.env, null);
+    expect(result.exitCode).not.toBe(0);
+  });
+
+  it('should return non-zero exitCode when gh binary is not found', async () => {
+    // Temporarily remove gh from PATH
+    const savedPath = process.env.PATH;
+    process.env.PATH = '';
+    try {
+      const result = await runGhCommand(['--version'], process.env, null);
+      expect(typeof result.exitCode).toBe('number');
+      expect(result.exitCode).toBe(1);
+    } finally {
+      process.env.PATH = savedPath;
+    }
+  });
+});