Harden get_api_key() to match the credential precedence #295 gave discovery - #296
Harden get_api_key() to match the credential precedence #295 gave discovery#296anonpran wants to merge 4 commits into
Conversation
…covery get_api_key() authenticates almost everything a hook sends to the backend -- policy decisions, transcript uploads, skill sync, error reporting -- not just discovery. It carried the same two gaps #295 just fixed there: env-first precedence (a stale personal-install env var can outrank a fresh config.json) and a plain os.getenv() call (which on Windows lets a non-admin user's own setx silently shadow the admin-provisioned value via HKCU). File is now checked first in all 5 hooks; env is consulted only when the file can't supply a key, via _machine_env() instead of os.getenv(). codex's get_api_key() never had a file fallback at all, so it keeps that shape -- just swapped its os.getenv() (and main()'s duplicate inline lookup, and a third call site in _repo_gate_report that bypassed get_api_key() entirely) for the hardened read. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
vigneshsubbiah16
left a comment
vigneshsubbiah16
left a comment
There was a problem hiding this comment.
✅ Security consensus: no issues found. (reviewers: Cursor, Claude, Semgrep, Gitleaks)
🤖 consensus review · reviewers: Cursor,Claude,Semgrep,Gitleaks · head 8400e52a · 2026-09-03T20:10Z
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…hooks have Greptile caught a real regression: codex's personal (non-MDM) installer writes the API key via setx with no /M -- HKCU, not HKLM -- same as the other 4 tools' personal installers. Those are safe because their get_api_key() checks config.json first, so HKCU vs HKLM never matters. codex's get_api_key() had no file fallback at all; switching its env read from os.getenv() to _machine_env() (HKLM-only, by design) meant a personal Windows Codex install's key became unreachable by either route -- HKCU is no longer read, and the file was never read. codex's personal setup.py already writes ~/.unbound/config.json (write_unbound_config, "shared with unbound-cli") -- get_api_key() just never used it. Give it the same file-first shape as the other 4 hooks. Also simplifies main() to call get_api_key() instead of duplicating the lookup inline, matching the other 4. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
vigneshsubbiah16
left a comment
vigneshsubbiah16
left a comment
There was a problem hiding this comment.
🛡️ Automated Security Review (consensus)
1 finding — 0 high-confidence, 1 to triage. Reviewers: Cursor, Claude, Semgrep, Gitleaks.
Findings
- 🟡 [LOW] Malformed
config.jsonparse errors may leak key material into logs —codex/hooks/unbound.py:3942- Impact: The new file-read path catches
json.JSONDecodeErrorin the broadexcept Exceptionhandler and logs{e}; Python's decode error text can include a snippet of the malformed line, which may contain the API key if the file was corrupted mid-write — andlog_error(..., 'config')may ship that text off-box. - Fix: Add a dedicated
except json.JSONDecodeError:branch with a fixed message (no{e}interpolation), matching augment/claude-code; reserve the generic handler for I/O failures only. - Flagged by: Claude, Cursor
- Impact: The new file-read path catches
Previously acknowledged (not re-flagged)
- File-first
~/.unbound/config.jsonprecedence over_machine_env()/ HKLM (Greptile P1) — Intentional design: mirrors the credential-resolution order shipped in #295 for discovery;config.jsonis treated as authoritative/freshest (rewritten on every MDM run), with machine env as fallback for launch contexts that never see the file.
🤖 consensus review · reviewers: Cursor, Claude, Semgrep, Gitleaks · head 30e3d4b1 · 2026-09-04T04:01Z
Greptile flagged the other side of the codex fix: making config.json authoritative lets a managed Windows user override the administrator-provisioned credential, since #295's mode=0o700 removal means ~/.unbound is now user-writable by design. The MDM hook itself sits in C:\Program Files\<Tool>\hooks (admin-only) for claude-code, codex, augment and cursor, so the config file is the one remaining path a managed user has to swap the key -- and with file-first, it won. Precedence is now split by whether a tamper-proof source exists: Windows reads HKLM first (admin-only; a user's writable config.json can't override it), falling through to the file when HKLM is empty, which is what a personal install looks like. POSIX has no admin-only env scope -- MDM writes shell rc files in the user's own home -- so there the file stays first as the freshest source, per #295. Adds tests pinning all three cases; verified the tampered-config test fails against the previous file-first ordering. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
get_api_key() now prefers the admin-only HKLM value on Windows, but _dispatch_discovery and _dispatch_mcp_server_scan still read the user-writable config.json first -- so a managed user could still swap the key or redirect scans there. Leaving two adjacent functions with opposite precedence is its own hazard, so close it here rather than in a follow-up. The two call sites were byte-identical copy-paste, so this extracts _resolve_credentials() instead of duplicating the fix: HKLM pair wins on Windows, file wins elsewhere, resolved as a unit, with the existing https check on the env-sourced URL preserved. Reworks one #295 test whose premise doesn't hold on Windows: it asserted a "stale personal-install env var" must not beat config.json, but a personal install writes HKCU, which _machine_env never reads -- the fixture's os.environ stub conflated the two scopes. Now stubs no machine value (the real personal-install case) and adds the managed counterpart, asserting the HKLM pair beats a tampered config. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
| machine_key = _machine_env('UNBOUND_CODEX_API_KEY') | ||
| if _is_windows() and machine_key: | ||
| return machine_key | ||
| try: | ||
| config_file = Path.home() / ".unbound" / "config.json" | ||
| with open(config_file, 'r', encoding='utf-8') as f: | ||
| key = json.loads(f.read()).get('api_key') | ||
| if key: | ||
| return key | ||
| except FileNotFoundError: | ||
| pass | ||
| except Exception as e: | ||
| log_error(f"Failed to read config file: {e}", 'config') | ||
| return machine_key |
There was a problem hiding this comment.
When a personal Windows installation successfully writes UNBOUND_CODEX_API_KEY with setx but the best-effort config write fails or the file later becomes unreadable, get_api_key() checks only HKLM and the unavailable config file, causing policy incident reports and error reports to be dropped and skill synchronization to be skipped despite the valid HKCU credential. How this was verified: Personal setup requires the HKCU write but ignores config-write failure, while the changed resolver has no HKCU lookup and its missing-key call paths return without reporting.
Knowledge Base Used:
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Agentic security review of credential-precedence hardening in PR 296. One high-severity issue remains: _resolve_credentials() only treats HKLM as authoritative when both the API key and UNBOUND_BACKEND_URL are present, which does not match binary managed Windows setup.
Sent by Cursor Security Agent: Security Reviewer
| if machine_url and not machine_url.startswith("https://"): | ||
| log_error("env base_url is not https, ignoring", category) | ||
| machine_key = machine_url = None | ||
| if _is_windows() and machine_key and machine_url: |
There was a problem hiding this comment.
🔒 Agentic Security Review
Severity: HIGH
_resolve_credentials() treats the admin-only HKLM values as authoritative only when both UNBOUND_*_API_KEY and UNBOUND_BACKEND_URL are present. unbound-hook setup writes the API key system-wide and the backend URL only into user-writable ~/.unbound/config.json, so the complete-pair check is skipped on that managed path and discovery/MCP dispatch fall through to the config pair.
A non-admin managed Windows user can keep the provisioned api_key in that file, change base_url to an attacker origin, and send discovery/MCP traffic (including the admin-provisioned key, and MCP server secrets via UNBOUND_MCP_SERVER_JSON) to that origin. The same helper is copied into all five hooks. get_api_key() correctly prefers a key-only HKLM value; this helper does not. Python MDM scripts that also set UNBOUND_BACKEND_URL in HKLM are not affected unless that URL is missing or fails the https:// prefix check (which zeros both machine fields and again falls through to config).
Impact: A less-privileged Windows user can redirect discovery and MCP-scan traffic that authenticates with the admin-provisioned API key. PreToolUse policy still uses get_api_key() and is not bypassed by this gap.
Reviewed by Cursor Security Reviewer for commit e3c677f. Configure here.
vigneshsubbiah16
left a comment
vigneshsubbiah16
left a comment
There was a problem hiding this comment.
🛡️ Automated Security Review (consensus)
3 findings — 2 high-confidence, 1 to triage. Reviewers: Cursor, Claude, Semgrep, Gitleaks.
-
🔴 [HIGH] Managed-Windows
_resolve_credentials()falls through to user-writableconfig.json—cursor/unbound.py:3521(identical helper in all 5 hooks)- Impact: When HKLM has the admin-provisioned API key but no
UNBOUND_BACKEND_URL, a non-admin user can keepapi_keyin~/.unbound/config.jsonand swapbase_urlto an attacker origin — discovery/MCP-scan subprocesses then ship the admin key (and MCP secrets) to that host;get_api_key()is not affected. - Fix: Gate on the machine key alone (match
get_api_key()): if_is_windows()andmachine_keyis set, never acceptconfig.json'sbase_url; use HKLM URL or fail closed. Longer term, writeUNBOUND_BACKEND_URLto HKLM during managed setup. - Flagged by: Cursor, Claude
- Impact: When HKLM has the admin-provisioned API key but no
-
🔴 [HIGH] Codex personal Windows installs can lose credentials and fail open —
codex/hooks/unbound.py:3937- Impact: Replacing
os.getenv()with HKLM-only_machine_env()drops the HKCUsetxpath; if the best-effortconfig.jsonwrite fails or the file is later unreadable,get_api_key()returns nothing and policy checks, incident reports, telemetry, and skill sync silently degrade. - Fix: Add an HKCU/process-env lookup as a final tier after HKLM + config, or make the personal-install config write mandatory (fail loudly on error).
- Flagged by: Claude, Greptile
- Impact: Replacing
-
🟡 [MEDIUM]
config.jsonbase_urlbypasses HTTPS validation —cursor/unbound.py:3530(all 5 hooks)- Impact: The
https://scheme check applies only to the HKLM/env URL; a user-writablehttp://base_urlin config sends the API key and discovery/MCP payloads over cleartext on POSIX and on Windows paths that hit the config pair. - Fix: Apply the same
startswith("https://")check to config-sourced URLs before returning the pair; log and reject on failure. - Flagged by: Claude
- Impact: The
🤖 consensus review · reviewers: Cursor, Claude, Semgrep, Gitleaks · head e3c677fb · 2026-09-04T05:03Z
2 participants
Summary
#295fixed discovery's credential resolution: file-first,_machine_env()instead ofos.getenv()(to stop a non-admin user's per-usersetxfrom shadowing the admin-provisioned value via Windows' HKCU-over-HKLM merge).get_api_key()— the credential behind almost every other backend call the hook makes (policy decisions, transcript uploads, skill sync, error reporting) — had the exact same two gaps and was left untouched at the time, out of scope for that PR.This applies the identical fix to
get_api_key()in all 5 hooks.Changes
claude-code/hooks/unbound.py,copilot/hooks/unbound.py,augment/hooks/unbound.py,cursor/unbound.py: flippedget_api_key()to checkconfig.jsonfirst, falling back to_machine_env()only when the file can't supply a key. Exception handling shape preserved per file, just reordered.codex/hooks/unbound.py:get_api_key()never had a file fallback, so it keeps that shape — only theos.getenv()call is replaced with_machine_env(). Also fixed two other call sites that bypassedget_api_key()with their own rawos.getenv():main()'s duplicate inline lookup, and a third site in_repo_gate_report.Test plan
os.getenv('UNBOUND_*_API_KEY')— none leftCo-Authored-By: Claude Opus 5 (1M context) noreply@anthropic.com
Greptile Summary
This PR standardizes credential precedence across the five hook integrations and routes discovery credentials through a shared pair resolver.
Confidence Score: 3/5
This PR is not yet safe to merge because Codex can discard a successfully installed personal Windows credential and silently fail open when its shared config copy is unavailable.
Codex personal setup requires the HKCU setx write but treats the shared-config write as best effort; the new runtime resolver reads only HKLM and that config file, leaving policy incident reporting, telemetry, and synchronization without credentials in a supported completed-install state.
Files Needing Attention: codex/hooks/unbound.py and tests/test_discovery_dispatch.py
Security Review
Codex can still lose its valid personal Windows credential when the redundant config-file write fails or the file later becomes unreadable. Because the hook never consults HKCU after replacing
os.getenv, policy incident reporting and other backend work can silently fail open.Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Codex personal Windows setup] --> B[Write key with per-user setx to HKCU] A --> C[Attempt shared config write] C -->|Success| D[get_api_key reads config] C -->|Failure or later corruption| E[get_api_key checks HKLM] E -->|No managed key| F[Returns no credential] F --> G[Policy and incident reports fail open] F --> H[Error reporting and skill sync are skipped]Reviews (4): Last reviewed commit: "Close the discovery half of the same hol..." | Re-trigger Greptile
Context used: