feat(groom): add key-broker.mjs — localhost header-injecting API-key proxy + unit tests (BE-4419)#72
feat(groom): add key-broker.mjs — localhost header-injecting API-key proxy + unit tests (BE-4419)#72mattmillerai wants to merge 3 commits into
Conversation
…proxy + unit tests (BE-4419)
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds a localhost Node.js API-key broker that reads credentials from stdin, forwards authenticated ChangesKey Broker
Sequence Diagram(s)sequenceDiagram
participant GroomCLI
participant key-broker.mjs
participant FakeUpstream
GroomCLI->>key-broker.mjs: Send /v1/* request
key-broker.mjs->>key-broker.mjs: Remove inbound auth headers
key-broker.mjs->>FakeUpstream: Forward request with stdin API key
FakeUpstream-->>key-broker.mjs: Return response or stream
key-broker.mjs-->>GroomCLI: Forward status, headers, and body
Possibly related PRs
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Cursor Review — Consolidated panel
Triggered by @mattmillerai.
Found 10 finding(s).
| Severity | Count |
|---|---|
| 🟠 High | 2 |
| 🟡 Medium | 3 |
| 🟢 Low | 5 |
Panel: 8/8 reviewers contributed findings.
…uards (BE-4419) Address the cursor-review panel findings on the localhost key-broker proxy: - Stream teardown: forward both directions via stream.pipeline so an upstream reset mid-response or a client abort mid-upload tears down BOTH sides instead of crashing the broker on an unhandled 'error'. A client disconnect now aborts the outbound real-key request (res 'close' -> upstreamReq.destroy()) so it can't linger and leak its socket. - Post-headers upstream failure now res.destroy()s (caller sees a broken connection) instead of res.end() presenting a truncated body as success. - Add a generous socket-inactivity timeout so a stalled upstream is reclaimed rather than pinning the forwarded request + socket forever. - Wrap client.request in try/catch (it can throw synchronously on an illegal request target / injected key) and trim() the key (strips CR + stray whitespace) so a copy-pasted secret can't throw an illegal-header error. - Restrict cleartext http:// upstreams to loopback hosts so a misconfigured remote http upstream can't transmit the injected key in the clear. - Prepend the upstream URL's path prefix so a non-root upstream isn't dropped. - Reject GROOM_BROKER_PORT strings with trailing garbage (8199x, 1e3). - README: recommend keying the readiness wait-loop off the readiness LINE (proof the broker bound the port), not a bare port-connect check. Tests: add coverage for client-abort survival, path-prefix forwarding, strict port rejection, and plaintext-non-loopback upstream rejection. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/groom/tests/test_key_broker.py:
- Around line 163-191: Add a regression test alongside
test_injects_real_key_and_strips_inbound_credentials that starts the broker with
a stdin key containing leading/trailing spaces and CRLF, then sends a request
and verifies the upstream x-api-key equals REAL_KEY rather than the untrimmed
input. Reuse the existing broker startup, cleanup, port-waiting, connection, and
record-capture helpers.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 98d5dc6a-f283-473b-b263-47bef8d265cf
📒 Files selected for processing (3)
.github/groom/README.md.github/groom/key-broker.mjs.github/groom/tests/test_key_broker.py
Feed the broker a padded/CRLF key on stdin and assert the trimmed value — not the raw input — reaches x-api-key upstream, locking down the key-broker.mjs .trim() hardening. Parameterizes _start_broker with an optional raw stdin payload. Addresses CodeRabbit review on PR #72. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
ELI-5
The groom agent that cleans up code runs on a shared runner where it can read any file and any process's environment. If we handed it the real Anthropic API key, it could just read the key straight out of
/proc. So instead we run a tiny local doorman (key-broker.mjs): it holds the real key, the agent gets a fake one, and the agent points the Claude CLI at the doorman (ANTHROPIC_BASE_URL=http://127.0.0.1:<port>). Every real request passes through the doorman, which swaps the fake key for the real one before sending it upstream. The key only ever arrives on the doorman's stdin — never in its environment or command line — so it can't be recovered from/proc. This PR adds the doorman script and its tests only; wiring it intogroom.ymlis a sibling ticket (BE-4311).What this adds
.github/groom/key-broker.mjs— a dependency-free (node:http/node:https, node ≥18, ESM) localhost proxy that reads the real key from stdin's first line, listens on127.0.0.1:<port>, answers the CLI'sHEAD/GET /connectivity probe locally with200, forwards/v1/*to the upstream (streaming both directions so SSE works) after stripping inboundx-api-key/authorization/hostand injecting the realx-api-key+ upstreamhost, returns404for anything else, and502(static body) on an upstream connection error. Config viaGROOM_BROKER_PORT(default8199) andGROOM_BROKER_UPSTREAM(defaulthttps://api.anthropic.com, overridable for tests). It never logs headers/bodies — at mostmethod path -> status..github/groom/tests/test_key_broker.py— aunittestintegration suite that boots the real script undernodeagainst a local fake upstream (skipped whennodeis absent). Discovery picks it up automatically via the existingtest-groom-scripts.yml.key-broker.mjssection in.github/groom/README.md(what it's for, the stdin-not-env rule and why, the env knobs, and the note thatgroom.ymlwiring lands in BE-4311).Tests
Covers every case the ticket enumerates, plus a startup guard:
x-api-key/authorization, and round-trips a non-200 upstream status and body;HEAD /→200, not forwarded;GET /anything-else→404, not forwarded;/proc/<pid>/{environ,cmdline}do not contain the real key (Linux-only, skipped elsewhere — so it exercises in CI onubuntu-latest, skips on the macOS dev box);502and the broker stays alive for a subsequent request;python3 -m unittest discover -s .github/groom/tests -p 'test_*.py' -v→Ran 66 tests ... OK (skipped=1)locally (the 1 skip is the Linux/procassertion on macOS).Acceptance criteria
ubuntu-latestshipsnode, so thenode-gated class runs there and the/procassertion is live).grep -c ANTHROPIC .github/groom/key-broker.mjs→0(the key is never read from env/argv; the only env reads areGROOM_BROKER_PORT/GROOM_BROKER_UPSTREAM). The comments were deliberately worded to avoid the provider token so this stays literally 0..github/workflows/groom.yml(nor any workflow file —test-groom-scripts.ymlalready triggers on.github/groom/**andubuntu-latest's preinstalled node is sufficient, so nosetup-nodestep was needed).Judgment calls / notes
key-broker.mjsfrom an earlier interrupted attempt was on the branch; I reviewed it adversarially, rebased the work onto the currentorigin/main, and kept the sound implementation. The one substantive fix: the residue's header comment falsely claimedgrep -c ANTHROPIC"is 0" while its own comments contained the token (actual count was 3) — reworded so the count is genuinely 0.req/res/upstreamReserrorhandlers) beyond the ticket's enumerated branches: a CLI that aborts mid-stream would otherwise throw an unhandled streamerrorand crash this long-lived proxy. These no-op/teardown handlers don't change the specified happy path.server_bindto skipHTTPServer'ssocket.getfqdn()reverse-DNS lookup, which hung ~35s per test on the dev box (and is a general flake risk); the suite now runs in ~3s.404/502/startup-diepaths are a proxy's correct protocol behavior, not a user-facing capability denial (no "X not supported" dead-end, no product surface to probe), so the falsification discipline doesn't apply here.