Skip to content

Commit 827140d

Browse files
CopilotCopilot
authored
refactor(cli-proxy): decompose handleExec into buildExecEnv and runGhCommand (#5126)
* Initial plan * refactor: decompose handleExec into buildExecEnv and runGhCommand sub-functions * fix(cli-proxy): normalize exec exitCode and harden env key filtering --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
1 parent b8fc6d4 commit 827140d

2 files changed

Lines changed: 203 additions & 56 deletions

File tree

‎containers/cli-proxy/server.js‎

Lines changed: 91 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,8 @@ const PROTECTED_ENV_KEYS = Object.freeze({
4040
[Symbol.iterator]() { return _PROTECTED_ENV_KEYS[Symbol.iterator](); },
4141
});
4242

43+
const UNSAFE_ENV_KEYS = new Set(['__proto__', 'constructor', 'prototype']);
44+
4345
// --- Structured logging to /var/log/cli-proxy/access.jsonl ---
4446

4547
const LOG_DIR = process.env.AWF_CLI_PROXY_LOG_DIR || '/var/log/cli-proxy';
@@ -201,68 +203,61 @@ function handleHealth(res) {
201203
}
202204

203205
/**
204-
* Handle POST /exec
206+
* Build the environment object for a subprocess by inheriting the server's environment
207+
* and applying caller-supplied overrides, excluding any PROTECTED_ENV_KEYS.
205208
*
206-
* Expected request body (JSON):
207-
* {
208-
* "args": ["pr", "list", "--repo", "owner/repo", "--json", "number,title"],
209-
* "cwd": "/home/runner/work/repo/repo", // optional
210-
* "stdin": null, // optional, base64-encoded or null
211-
* "env": { "GH_REPO": "owner/repo" } // optional extra env vars
212-
* }
209+
* Security-critical: ensures agents cannot override auth or TLS trust-store variables.
213210
*
214-
* Response body (JSON):
215-
* {
216-
* "stdout": "...",
217-
* "stderr": "...",
218-
* "exitCode": 0
219-
* }
211+
* @param {Record<string, string>|null|undefined} extraEnv - Optional caller-supplied env overrides
212+
* @returns {NodeJS.ProcessEnv} The merged environment for the child process
220213
*/
221-
async function handleExec(req, res) {
222-
const startTime = Date.now();
223-
let body;
224-
try {
225-
const raw = await readBody(req, res);
226-
// null means readBody already sent a 413 error response
227-
if (raw === null) return;
228-
body = JSON.parse(raw.toString('utf8'));
229-
} catch {
230-
accessLog({ event: 'exec_error', error: 'Invalid JSON body' });
231-
return sendError(res, 400, 'Invalid JSON body');
232-
}
233-
234-
const { args, cwd, stdin, env: extraEnv } = body;
235-
236-
// Validate args
237-
const validation = validateArgs(args);
238-
if (!validation.valid) {
239-
accessLog({ event: 'exec_denied', args, error: validation.error });
240-
return sendError(res, 403, validation.error);
241-
}
242-
243-
accessLog({ event: 'exec_start', args, cwd: cwd || null });
244-
245-
// Build environment for the subprocess
214+
function buildExecEnv(extraEnv) {
246215
// Inherit server environment (includes GH_HOST, NODE_EXTRA_CA_CERTS, GH_REPO, etc.)
247216
const childEnv = Object.assign({}, process.env);
248217
if (extraEnv && typeof extraEnv === 'object') {
249218
// Only allow safe string env overrides; never allow overriding keys in PROTECTED_ENV_KEYS.
250219
for (const [key, value] of Object.entries(extraEnv)) {
251-
if (typeof key === 'string' && typeof value === 'string' && !PROTECTED_ENV_KEYS.has(key)) {
220+
if (
221+
typeof key === 'string'
222+
&& typeof value === 'string'
223+
&& !PROTECTED_ENV_KEYS.has(key)
224+
&& !UNSAFE_ENV_KEYS.has(key)
225+
) {
252226
childEnv[key] = value;
253227
}
254228
}
255229
}
230+
return childEnv;
231+
}
256232

257-
// Execute gh directly (no shell — prevents injection attacks)
258-
// Always use the server's own cwd — the agent sends its container workspace
259-
// path which doesn't exist inside the cli-proxy container.
260-
let stdout = '';
261-
let stderr = '';
262-
let exitCode = 0;
233+
/**
234+
* Execute `gh` with the given args, environment, and optional base64-encoded stdin.
235+
*
236+
* Runs gh directly via execFile (no shell — prevents injection attacks).
237+
* Always uses the server's own cwd — the agent sends its container workspace
238+
* path which doesn't exist inside the cli-proxy container.
239+
*
240+
* @param {string[]} args - Arguments passed to gh (excluding 'gh' itself)
241+
* @param {NodeJS.ProcessEnv} childEnv - Environment for the child process
242+
* @param {string|null|undefined} stdin - Optional base64-encoded stdin data
243+
* @returns {Promise<{stdout: string, stderr: string, exitCode: number}>}
244+
*/
245+
async function runGhCommand(args, childEnv, stdin) {
246+
const normalizeExitCode = code => {
247+
if (typeof code === 'number' && Number.isFinite(code)) {
248+
return code;
249+
}
250+
if (typeof code === 'string') {
251+
const parsedCode = Number.parseInt(code, 10);
252+
if (!Number.isNaN(parsedCode)) {
253+
return parsedCode;
254+
}
255+
}
256+
return 1;
257+
};
263258

264259
try {
265-
const result = await new Promise((resolve, reject) => {
260+
return await new Promise((resolve, reject) => {
266261
const child = execFile('gh', args, {
267262
cwd: process.cwd(),
268263
env: childEnv,
@@ -278,7 +273,7 @@ async function handleExec(req, res) {
278273
resolve({
279274
stdout: childStdout || '',
280275
stderr: childStderr || '',
281-
exitCode: err ? (err.code || 1) : 0,
276+
exitCode: err ? normalizeExitCode(err.code) : 0,
282277
});
283278
});
284279

@@ -295,17 +290,58 @@ async function handleExec(req, res) {
295290
child.stdin.end();
296291
}
297292
});
298-
299-
stdout = result.stdout;
300-
stderr = result.stderr;
301-
exitCode = result.exitCode;
302293
} catch (err) {
303294
// Only expose a safe message, not a full stack trace
304295
const errMsg = err instanceof Error ? err.message : 'Command execution failed';
305-
stderr = errMsg;
306-
exitCode = 1;
296+
return { stdout: '', stderr: errMsg, exitCode: 1 };
297+
}
298+
}
299+
300+
/**
301+
* Handle POST /exec
302+
*
303+
* Expected request body (JSON):
304+
* {
305+
* "args": ["pr", "list", "--repo", "owner/repo", "--json", "number,title"],
306+
* "cwd": "/home/runner/work/repo/repo", // optional
307+
* "stdin": null, // optional, base64-encoded or null
308+
* "env": { "GH_REPO": "owner/repo" } // optional extra env vars
309+
* }
310+
*
311+
* Response body (JSON):
312+
* {
313+
* "stdout": "...",
314+
* "stderr": "...",
315+
* "exitCode": 0
316+
* }
317+
*/
318+
async function handleExec(req, res) {
319+
const startTime = Date.now();
320+
let body;
321+
try {
322+
const raw = await readBody(req, res);
323+
// null means readBody already sent a 413 error response
324+
if (raw === null) return;
325+
body = JSON.parse(raw.toString('utf8'));
326+
} catch {
327+
accessLog({ event: 'exec_error', error: 'Invalid JSON body' });
328+
return sendError(res, 400, 'Invalid JSON body');
307329
}
308330

331+
const { args, cwd, stdin, env: extraEnv } = body;
332+
333+
// Validate args
334+
const validation = validateArgs(args);
335+
if (!validation.valid) {
336+
accessLog({ event: 'exec_denied', args, error: validation.error });
337+
return sendError(res, 403, validation.error);
338+
}
339+
340+
accessLog({ event: 'exec_start', args, cwd: cwd || null });
341+
342+
const childEnv = buildExecEnv(extraEnv);
343+
const { stdout, stderr, exitCode } = await runGhCommand(args, childEnv, stdin);
344+
309345
const responseBody = JSON.stringify({ stdout, stderr, exitCode });
310346

311347
const durationMs = Date.now() - startTime;
@@ -377,4 +413,4 @@ if (require.main === module) {
377413
});
378414
}
379415

380-
module.exports = { validateArgs, ALWAYS_DENIED_SUBCOMMANDS, PROTECTED_ENV_KEYS };
416+
module.exports = { validateArgs, ALWAYS_DENIED_SUBCOMMANDS, PROTECTED_ENV_KEYS, buildExecEnv, runGhCommand };

‎containers/cli-proxy/server.test.js‎

Lines changed: 112 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@
66
* The server only enforces meta-command denial (auth, config, extension).
77
*/
88

9-
const { validateArgs, ALWAYS_DENIED_SUBCOMMANDS, PROTECTED_ENV_KEYS } = require('./server');
9+
const { validateArgs, ALWAYS_DENIED_SUBCOMMANDS, PROTECTED_ENV_KEYS, buildExecEnv, runGhCommand } = require('./server');
1010

1111
describe('PROTECTED_ENV_KEYS', () => {
1212
it('should protect GH_HOST from agent override', () => {
@@ -186,3 +186,114 @@ describe('validateArgs', () => {
186186
});
187187
});
188188
});
189+
190+
describe('buildExecEnv', () => {
191+
const originalEnv = process.env;
192+
193+
beforeEach(() => {
194+
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' };
195+
});
196+
197+
afterEach(() => {
198+
process.env = originalEnv;
199+
});
200+
201+
it('should inherit server environment variables', () => {
202+
const env = buildExecEnv(null);
203+
expect(env.EXISTING_VAR).toBe('existing');
204+
});
205+
206+
it('should apply safe extra env vars', () => {
207+
const env = buildExecEnv({ MY_VAR: 'my-value' });
208+
expect(env.MY_VAR).toBe('my-value');
209+
});
210+
211+
it('should not allow overriding GH_HOST', () => {
212+
const env = buildExecEnv({ GH_HOST: 'evil.com' });
213+
expect(env.GH_HOST).toBe('github.com');
214+
});
215+
216+
it('should not allow overriding GH_TOKEN', () => {
217+
const env = buildExecEnv({ GH_TOKEN: 'stolen' });
218+
expect(env.GH_TOKEN).toBe('secret-token');
219+
});
220+
221+
it('should not allow overriding GITHUB_TOKEN', () => {
222+
const env = buildExecEnv({ GITHUB_TOKEN: 'stolen' });
223+
expect(env.GITHUB_TOKEN).toBe('secret-github-token');
224+
});
225+
226+
it('should not allow overriding NODE_EXTRA_CA_CERTS', () => {
227+
const env = buildExecEnv({ NODE_EXTRA_CA_CERTS: '/evil/ca.crt' });
228+
expect(env.NODE_EXTRA_CA_CERTS).toBe('/certs/ca.crt');
229+
});
230+
231+
it('should not allow overriding SSL_CERT_FILE', () => {
232+
const env = buildExecEnv({ SSL_CERT_FILE: '/evil/ssl.crt' });
233+
expect(env.SSL_CERT_FILE).toBe('/certs/ssl.crt');
234+
});
235+
236+
it('should not allow overriding GIT_SSL_CAINFO', () => {
237+
const env = buildExecEnv({ GIT_SSL_CAINFO: '/evil/git.crt' });
238+
expect(env.GIT_SSL_CAINFO).toBe('/certs/git.crt');
239+
});
240+
241+
it('should ignore dangerous prototype keys from JSON payloads', () => {
242+
const extraEnv = JSON.parse('{"__proto__":"polluted","constructor":"polluted","prototype":"polluted","MY_VAR":"ok"}');
243+
const env = buildExecEnv(extraEnv);
244+
expect(env.MY_VAR).toBe('ok');
245+
expect(env.__proto__).toBe(Object.prototype);
246+
expect(env.constructor).toBe(Object);
247+
expect(env.prototype).toBeUndefined();
248+
expect({}.polluted).toBeUndefined();
249+
});
250+
251+
it('should silently skip non-string values', () => {
252+
const env = buildExecEnv({ MY_VAR: 42 });
253+
expect(env.MY_VAR).toBeUndefined();
254+
});
255+
256+
it('should handle null extraEnv gracefully', () => {
257+
const env = buildExecEnv(null);
258+
expect(env.EXISTING_VAR).toBe('existing');
259+
});
260+
261+
it('should handle undefined extraEnv gracefully', () => {
262+
const env = buildExecEnv(undefined);
263+
expect(env.EXISTING_VAR).toBe('existing');
264+
});
265+
266+
it('should handle non-object extraEnv gracefully', () => {
267+
const env = buildExecEnv('not-an-object');
268+
expect(env.EXISTING_VAR).toBe('existing');
269+
});
270+
});
271+
272+
describe('runGhCommand', () => {
273+
it('should return stdout, stderr, and exitCode on success', async () => {
274+
const result = await runGhCommand(['--version'], process.env, null);
275+
expect(result).toHaveProperty('stdout');
276+
expect(result).toHaveProperty('stderr');
277+
expect(result).toHaveProperty('exitCode');
278+
expect(typeof result.stdout).toBe('string');
279+
expect(typeof result.exitCode).toBe('number');
280+
});
281+
282+
it('should return non-zero exitCode for invalid gh subcommand', async () => {
283+
const result = await runGhCommand(['__nonexistent_subcommand__'], process.env, null);
284+
expect(result.exitCode).not.toBe(0);
285+
});
286+
287+
it('should return non-zero exitCode when gh binary is not found', async () => {
288+
// Temporarily remove gh from PATH
289+
const savedPath = process.env.PATH;
290+
process.env.PATH = '';
291+
try {
292+
const result = await runGhCommand(['--version'], process.env, null);
293+
expect(typeof result.exitCode).toBe('number');
294+
expect(result.exitCode).toBe(1);
295+
} finally {
296+
process.env.PATH = savedPath;
297+
}
298+
});
299+
});

0 commit comments

Comments
 (0)