fix(provider): allowlist agent CLI environment variables

[yashrajp22]

An environment denylist forwarded unrecognized operator secrets to agent CLI subprocesses. Pass only explicit runtime, locale and local-login discovery variables; drop arbitrary tokens and configuration/loader overrides. API-key authentication remains excluded for these local-login CLI providers.

Validation: Unknown secret names, runtime compatibility and fake-child environment checks; combined provider suites.

This PR is stacked on #737; its diff contains this finding’s fix.

[rng1995]

[SkillSpector Review]

Hi @yashrajp22, thank you for turning the agent-CLI environment scrub into a fail-closed allowlist!

Value and readiness: The direction is right, and it fixes real leaks on main.

• What it fixes: The old prefix denylist forwarded GH_TOKEN, GEMINI_API_KEY, NPM_TOKEN, VAULT_TOKEN, DATABASE_URL, NODE_OPTIONS, PYTHONPATH and any future secret name to every CLI child process. With an exact-name allowlist, none of these reach claude, gemini or opencode.
• Tests: The new tests cover unknown names, lowercase names and loader hooks. The rewritten OpenCode fake binaries no longer depend on environment passthrough.
• What is still wrong: The allowlist also drops variables these CLIs need to reach their back ends. And Claude's availability probe still runs with the full environment. So some setups that work today would fail on every call while the provider still reports itself available.

It needs another revision.

Material findings

1. [Blocker] src/skillspector/providers/_agent_cli.py:81: _RUNTIME_ENV_NAMES has no proxy or CA-bundle variables.
   • What is missing: HTTPS_PROXY/https_proxy, HTTP_PROXY/http_proxy, ALL_PROXY, NO_PROXY/no_proxy, NODE_EXTRA_CA_CERTS, SSL_CERT_FILE and SSL_CERT_DIR.
   • What fails: claude, gemini and opencode all call their vendors' APIs over the network. Behind an egress proxy or a TLS-inspecting proxy, every call now fails and the scan degrades to static-only. On main, the same setup works because the denylist forwarded these variables.
   • Why forwarding is safe: They describe the network path, not a credential the child could misuse. A proxy URL's userinfo goes only to the proxy, and it already reached the child on main.
   • Expected fix: Add these names to the allowlist and assert them in test_allowlist_preserves_runtime_and_drops_unknown_secrets.
2. [Blocker] src/skillspector/providers/_agent_cli.py:734-749 (_claude_auth_check, unchanged in this diff): claude auth status runs with the full inherited environment, while inference now runs with the allowlist.
   • What fails: Take a Claude Code setup that works on main because of an environment variable, for example CLAUDE_CONFIG_DIR pointing at the directory that holds the login, or a proxy as in item 1. The probe reports loggedIn, but every claude -p call fails. The provider claims to be available while each batch errors.
   • Precedent: _opencode_auth_check already probes with the scrubbed environment for this reason, as its docstring says. #572 does the same for Copilot.
   • Expected fix: Pass env=_scrub_env() to the claude probe, so availability matches what inference will see. Add a test that the probe receives the scrubbed environment. To keep the custom-config-dir setup working, allowlist CLAUDE_CONFIG_DIR as a local-login location, as XDG_DATA_HOME already is for OpenCode.
3. [Non-blocking] Please run the opt-in live suite for claude and gemini under the new allowlist, on macOS and Linux, and post the result in the PR. The command is uv run pytest -m integration tests/integration/test_agent_cli_live.py.
   • Why: So far the validation uses only fake child processes. Claude Code on macOS reads its login from the Keychain, and I did not verify which of the dropped variables (for example USER or LOGNAME) that lookup needs.
4. [Non-blocking] tests/unit/test_agent_cli.py: add GH_TOKEN and GH_ENTERPRISE_TOKEN to the list of dropped secrets. GH_TOKEN is the leak still open in #572's review, and a one-line assertion pins it.

PIC tradeoffs: The allowlist removes every way of passing an API key or token to these CLIs through the environment. Examples are GEMINI_API_KEY, gateway setups using ANTHROPIC_AUTH_TOKEN and ANTHROPIC_BASE_URL, and CLAUDE_CODE_OAUTH_TOKEN for headless Claude Code.

That matches the README contract ("SkillSpector never reads or forwards API keys when these providers are active"), but it changes behaviour. On main those variables reached the child. So headless and CI users of claude_cli or gemini_cli who authenticate through the environment will lose LLM analysis. The PIC should confirm this, and it belongs in the release notes.

Verification and gaps:

• What I reviewed: this PR's own commit, 2b00db0, on top of #737's 16435ad. GitHub shows the diff against yashraj/disable-unsafe-codex-provider.
• No CI yet: ci.yml runs only for pull requests into main, so CI has never run on this PR. After #737 merges, retarget this PR to main and push or update the branch. Retargeting alone does not trigger pull_request CI.
• Callers of _scrub_env():
  • run_agent_cli for claude, gemini and opencode. codex and agy fail before anything is spawned.
  • _opencode_auth_check and _prepare_opencode_env. OpenCode keeps its login store through XDG_DATA_HOME and isolates everything else, so only item 1 affects it.
• Interplay with #572 (copilot_cli): there is no textual conflict, but there are two semantic effects.
  • Helpful: _prepare_copilot_env builds on _scrub_env(). After this PR, GH_TOKEN never reaches Copilot, whatever #572 re-injects. That settles #572's open blocker, so land this PR before #572.
  • Breaking: #572's fake Copilot binary reads FAKE_COPILOT_VERSION and ATTACK_MARKERS from the child environment (tests/provider/test_copilot_cli.py:1102, 1105). Under this allowlist, its 9.9.99 version test would see 1.0.91, then crash on the missing marker variable and fail its match="1.0.91" assertion. Whichever PR lands second must bake those values into the fake script, as this PR did for OpenCode. Copilot also needs the proxy variables from item 1.
• Conflicts: git merge-tree is clean against main and against #736, #744, #737 and #572.
• Tests: I did not run them, per policy.

---

Decision: Changes Requested (reviewed head 2b00db0bc2958ce8988945f0cf8750eec1b351cb)

[yashrajp22]

Thanks for the additional notes. I added explicit checks for GH_TOKEN and GH_ENTERPRISE_TOKEN, and carried over the disabled-Codex documentation update from #737. All 272 targeted tests pass, with 10 platform or optional-provider skips; I have not verified signed-in Claude and Gemini on macOS and Linux, so that live compatibility check remains open. This PR is still stacked on #737 and will need main-targeted CI after the parent lands.

[rng1995]

[SkillSpector Review]

Hi @yashrajp22, thank you for the quick fixes and for pinning each one with a regression test!

Value and readiness: Both blockers are fixed.

• Network settings: the allowlist now keeps the proxy and CA variables, in upper and lower case. claude, gemini and opencode can still reach their back ends behind an egress or TLS-inspecting proxy.
• Probe consistency: the Claude availability probe now runs with the same scrubbed environment as inference. CLAUDE_CONFIG_DIR is kept, so custom login directories still work.
• Secrets: API keys, GH_TOKEN, cloud credentials, loader hooks and unknown variable names still never reach a CLI child process.

I found no required code change, so the PR is ready for final maintainer and PIC review. Two things remain before merge:

• It can land only after #737. Then it must be retargeted to main and get a CI run; CI has never run on this PR.
• The signed-in live check on macOS and Linux is still open (item 3 below). I recommend the maintainer gets that result before merging.

Previous findings:

1. [Blocker] The allowlist had no proxy or CA-bundle variables: Resolved.
   • src/skillspector/providers/_agent_cli.py:96-102 adds HTTP_PROXY, HTTPS_PROXY, ALL_PROXY, NO_PROXY, NODE_EXTRA_CA_CERTS, SSL_CERT_FILE and SSL_CERT_DIR.
   • _scrub_env (:134) compares key.upper(), so https_proxy and no_proxy pass as well.
   • test_allowlist_preserves_runtime_and_drops_unknown_secrets (tests/unit/test_agent_cli.py:283) asserts both spellings of each proxy name.
2. [Blocker] claude auth status ran with the full inherited environment: Resolved.
   • _claude_auth_check passes env=_scrub_env() (:757). Claude has no prepare_env, so the probe and inference now get the same variables.
   • CLAUDE_CONFIG_DIR is allowlisted (:95).
   • test_claude_auth_probe_uses_the_inference_environment (tests/unit/test_agent_cli.py:333-351) asserts that the probe gets the login and proxy settings but not GH_TOKEN. The test fails on the old code, which passed no env.
3. [Non-blocking] Run the live suite under the allowlist on macOS and Linux: Still open. You said that signed-in claude and gemini were not verified.
4. [Non-blocking] Assert that GH_TOKEN and GH_ENTERPRISE_TOKEN are dropped: Resolved (tests/unit/test_agent_cli.py:311-312).

Material findings

1. [Non-blocking] tests/unit/test_agent_cli.py:283-331: the new allowlist test fails on Windows.
   • Why: Windows os.environ stores names in upper case. https_proxy and HTTPS_PROXY collapse into one entry, and SystemRoot and AppData come back as SYSTEMROOT and APPDATA. So assert _scrub_env() == runtime (:330) and the round-trip check on the next line fail.
   • Impact: CI runs only on Ubuntu, so it stays green. But this file already has a Windows-only test (:834), so people do run it on Windows.
   • Fix: compare upper-cased keys on Windows, or keep the mixed-case entries for POSIX only.
2. [Non-blocking] src/skillspector/providers/_agent_cli.py:81: consider allowlisting USER, LOGNAME and USERNAME.
   • Why: they name the account and are not credentials. Claude Code keeps its macOS login in the Keychain, and I could not verify whether that lookup reads the account name from the environment.
   • Suggestion: adding them is the cheapest hedge for item 3. If the live run passes without them, leave the list as it is.

PIC tradeoffs: Unchanged from last round. CLI authentication through environment variables no longer works. This covers CLAUDE_CODE_OAUTH_TOKEN for headless Claude Code, ANTHROPIC_AUTH_TOKEN with ANTHROPIC_BASE_URL for gateways, and GEMINI_API_KEY. On main these reached the child process.

• Claude users who rely on them should now see "not authenticated" at preflight. That is clearer than before, because the probe now agrees with inference.
• Gemini has no authentication probe, so its calls fail one by one.

Either way, the scan is static-only. For key-based authentication, the hosted providers remain the path: anthropic, and gemini through ADC since #496. This matches the README promise that CLI providers never receive API keys. It needs the PIC's confirmation and a release note.

Verification and gaps:

• What I reviewed: this PR's own change against #737, git diff ce44b61..d41ab95. That is 2b00db0 plus a8a816d (the fixes) and 8b9a82c (formatting only).

  • 349b692 is a cherry-pick of #737's ce44b61: same content, different context lines.
  • d41ab95 merges ce44b61 with an empty combined diff.
  • All commits are by the author and carry Signed-off-by.

• Same environment for probes and scans: every process that _agent_cli.py starts now gets the scrubbed environment:

  • the claude probe (:752-758);
  • the opencode version and login probes (:800-823, built from _prepare_opencode_env(_scrub_env()));
  • the opencode policy preflight (:583-591), which reuses the inference environment;
  • inference itself (:1164-1193).

  The gemini, codex and agy checks start no process.

• Secrets: API keys, GH_TOKEN/GH_ENTERPRISE_TOKEN, AWS_*, GOOGLE_*, AZURE_*, NODE_OPTIONS, PYTHONPATH, SSLKEYLOGFILE and GIT_SSL_KEY are all dropped. Credentials embedded in a proxy URL still reach the child, as they do on main, because the child needs them to get out through the proxy.

• CA variables: REQUESTS_CA_BUNDLE and CURL_CA_BUNDLE are not needed. claude, gemini and opencode are Node or Bun programs, and those read NODE_EXTRA_CA_CERTS.

• Home and config directories:

  • claude uses HOME/USERPROFILE and CLAUDE_CONFIG_DIR.
  • gemini uses HOME.
  • opencode keeps XDG_DATA_HOME for its login store. _prepare_opencode_env deliberately redirects its other XDG directories.

• Windows: PATH, PATHEXT, USERPROFILE, APPDATA, LOCALAPPDATA, SYSTEMROOT, WINDIR, TEMP and TMP are kept. As far as I can trace, that is enough for npm .cmd shims and Node networking. I could not test it on Windows.

• #572: it still reads FAKE_COPILOT_VERSION and ATTACK_MARKERS from the child environment (tests/provider/test_copilot_cli.py:927-930, 1102-1105, 1197). Its tests will break under this allowlist until those values are baked into the fake binary. Its _prepare_copilot_env starts from _scrub_env() and re-adds only COPILOT_GITHUB_TOKEN and COPILOT_HOME, so this PR still closes #572's GH_TOKEN leak. Land this PR before #572.

• #496: no interaction. The new gemini provider is an in-process HTTP client and never calls _scrub_env.

• Conflicts:

  • #737 (the base): clean.
  • main: this branch inherits #737's three conflicts with #496 (.env.example, README.md, cli.py). Merging #737's resolved branch into this one clears them. Merge rather than rebase, because this branch carries both ce44b61 and its cherry-pick.
  • #572: both PRs edit the provider lists (.env.example, README.md, docs/DEVELOPMENT.md, cli.py, providers/__init__.py), so the second to land rebases.

• Lint: ruff check and ruff format --check pass on the three touched files.

• CI: none. ci.yml runs only for pull requests into main, so CI will first run after the retarget and a push or branch update.

• Tests were not run locally, per policy.

---

Decision: Approved (reviewed head d41ab95703b948d60a0f35703436fd6ec32d86df)

[yashrajp22]

Thanks for catching the Windows test issue! I normalized the expected environment names on Windows and kept the exact checks that unknown credentials are excluded. I also retained USER, LOGNAME, and USERNAME and merged the updated parent branch; 325 tests passed and 10 skipped, including the fake CLI checks. Real signed-in Claude and Gemini checks on macOS and Linux still need a safe authenticated environment, so I haven’t claimed those are verified.