refactor(cli-proxy): decompose handleExec into buildExecEnv and runGhCommand#5126
Conversation
✅ Coverage Check PassedOverall Coverage
📁 Per-file Coverage Changes (1 files)
Coverage comparison generated by |
There was a problem hiding this comment.
Pull request overview
This PR refactors the cli-proxy’s /exec handler (containers/cli-proxy/server.js) by extracting subprocess environment construction and gh execution into separate exported helpers, aiming to make the security-sensitive env filtering logic unit-testable and reduce handleExec complexity.
Changes:
- Extracts
buildExecEnv(extraEnv)to centralize env merging while filteringPROTECTED_ENV_KEYS. - Extracts
runGhCommand(args, childEnv, stdin)to wrapexecFile('gh', ...)execution and output capture. - Adds Jest coverage for both new helpers and updates exports; includes incidental
package-lock.jsonchanges.
Show a summary per file
| File | Description |
|---|---|
| containers/cli-proxy/server.js | Decomposes handleExec into buildExecEnv + runGhCommand and exports both helpers. |
| containers/cli-proxy/server.test.js | Adds unit tests for buildExecEnv and runGhCommand; updates imports accordingly. |
| containers/cli-proxy/package-lock.json | Removes libc constraints from several optional native binding entries (platform selection behavior may change). |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Files not reviewed (1)
- containers/cli-proxy/package-lock.json: Generated file
Comments suppressed due to low confidence (1)
containers/cli-proxy/server.js:217
- buildExecEnv builds childEnv as a normal object (Object.assign({}, process.env)). If extraEnv includes a JSON key like "proto", the assignment
childEnv[key] = valuecan throw (or mutate the prototype) because__proto__is a magic accessor on Object.prototype. Since extraEnv comes from untrusted JSON, this is a security/robustness bug and can be used to force 500s on /exec.
Use a null-prototype env object and explicitly skip dangerous keys (e.g. proto/constructor/prototype) before assignment.
// 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)) {
- Files reviewed: 2/3 changed files
- Comments generated: 4
| it('should silently skip non-string keys', () => { | ||
| // Object.entries coerces, but we guard typeof key === 'string' | ||
| const env = buildExecEnv({ MY_VAR: 'ok' }); | ||
| expect(env.MY_VAR).toBe('ok'); | ||
| }); |
| const childEnv = buildExecEnv(extraEnv); | ||
| const { stdout, stderr, exitCode } = await runGhCommand(args, childEnv, stdin); |
| process.env.PATH = ''; | ||
| try { | ||
| const result = await runGhCommand(['--version'], process.env, null); | ||
| // When the binary is not found, exitCode is the ENOENT error code (truthy, non-zero) |
| "node_modules/@unrs/resolver-binding-linux-arm64-gnu": { | ||
| "version": "1.11.1", | ||
| "resolved": "https://registry.npmjs.org/@unrs/resolver-binding-linux-arm64-gnu/-/resolver-binding-linux-arm64-gnu-1.11.1.tgz", | ||
| "integrity": "sha512-34gw7PjDGB9JgePJEmhEqBhWvCiiWCuXsL9hYphDF7crW7UgI05gyBAi6MF58uGcMOiOqSJ2ybEeCvHcq0BCmQ==", | ||
| "cpu": [ | ||
| "arm64" | ||
| ], | ||
| "dev": true, | ||
| "libc": [ | ||
| "glibc" | ||
| ], | ||
| "license": "MIT", |
|
@copilot address review feedback |
|
⏳ Copilot review left inline comments. @copilot To proceed:
|
Addressed in commit
Validation run:
|
|
🔌 Smoke Services — All services reachable! ✅ |
|
✅ Smoke Copilot BYOK completed. Copilot BYOK mode operational. 🔓 |
|
📡 Smoke OTel Tracing completed. All tracing scenarios validated. ✅ |
|
🔑 Smoke Copilot PAT PAT auth validated. All systems operational. ✅ |
|
✅ Smoke Copilot BYOK AOAI (api-key) completed. Copilot AOAI BYOK (api-key) mode operational. 🔓 |
|
✅ Build Test Suite completed successfully! |
|
✅ Smoke Copilot BYOK AOAI (Entra) completed. Copilot AOAI BYOK (Entra) mode operational. 🔓 |
|
❌ Contribution Check failed. Please review the logs for details. |
|
❌ Smoke Claude failed No user request was provided in this turn — only system reminders listing available skills, project context (CLAUDE.md), and safety policies were present. No GitHub action, code change, or analysis was requested, so no action was taken. |
|
✨ The prophecy is fulfilled... Smoke Codex has completed its mystical journey. The stars align. 🌟 |
|
Chroot tests passed! Smoke Chroot - All security and functionality tests succeeded. |
|
❌ Security Guard failed. Please review the logs for details. |
|
📰 VERDICT: Smoke Copilot has concluded. All systems operational. This is a developing story. 🎤 |
|
✅ Smoke Gemini completed. All facets verified. 💎 |
🧪 Smoke Test: Copilot BYOK (Direct)✅ All tests passed
Status: PASS | Running in direct BYOK mode (COPILOT_PROVIDER_API_KEY)
|
🧪 Smoke Test Results — PASS
PR: refactor(cli-proxy): decompose handleExec into buildExecEnv and runGhCommand Overall: PASS
|
🔬 Smoke Test Results — PAT Auth
Overall: PASS Auth mode: PAT (COPILOT_GITHUB_TOKEN)
|
🔍 Smoke Test: API Proxy OpenTelemetry Tracing
All 5 scenarios pass. OTEL tracing integration is fully functional on this PR's branch.
|
|
GitHub MCP Testing: ✅ Running in direct BYOK mode (AWF_AUTH_TYPE=github-oidc + AWF_AUTH_AZURE_* + COPILOT_PROVIDER_BASE_URL) via api-proxy → Azure OpenAI (Foundry, o4-mini-aw) authenticated via Microsoft Entra Overall: PASS
|
|
Merged PRs reviewed:
|
Chroot Smoke Test Results
Overall: ❌ Not all tests passed — Python and Node.js versions differ between host and chroot environments.
|
|
Overall: PASS
|
🏗️ Build Test Suite Results
Overall: 8/8 ecosystems passed — ✅ PASS
|
Smoke Test: GitHub Actions Services Connectivity
Overall: FAIL —
|
|
Gemini Smoke Test Results:
Overall status: PASS Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
network:
allowed:
- defaults
- "localhost"See Network Configuration for more information.
|
handleExecincontainers/cli-proxy/server.jswas a 107-line monolith mixing env construction, subprocess execution, and HTTP response — making the security-critical env-filtering logic untestable in isolation.Changes
buildExecEnv(extraEnv)— extracts the env-merge step; enforcesPROTECTED_ENV_KEYSfiltering without requiring an HTTP contextrunGhCommand(args, childEnv, stdin)— extracts theexecFilePromise wrapper; handles stdin, timeout/signal errors, and output capturehandleExec— reduced from 107 → ~35 lines; now a pure orchestrator delegating to the two sub-functionsNew tests (16 added, 52 total passing)
buildExecEnv: verifies all 6PROTECTED_ENV_KEYSare blocked, safe vars pass through, non-string values and null/undefinedextraEnvare handled gracefullyrunGhCommand: success path, non-zero exit on invalid subcommand, behavior when binary is missing