From 94d57b82ec0b9f3351909196eb8f61f5616202c9 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Fri, 24 Jul 2026 14:26:09 -0700 Subject: [PATCH 1/3] =?UTF-8?q?feat(groom):=20add=20key-broker.mjs=20?= =?UTF-8?q?=E2=80=94=20localhost=20header-injecting=20API-key=20proxy=20+?= =?UTF-8?q?=20unit=20tests=20(BE-4419)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .github/groom/README.md | 49 ++++- .github/groom/key-broker.mjs | 147 +++++++++++++ .github/groom/tests/test_key_broker.py | 282 +++++++++++++++++++++++++ 3 files changed, 477 insertions(+), 1 deletion(-) create mode 100644 .github/groom/key-broker.mjs create mode 100644 .github/groom/tests/test_key_broker.py diff --git a/.github/groom/README.md b/.github/groom/README.md index 22de4ee..486df5c 100644 --- a/.github/groom/README.md +++ b/.github/groom/README.md @@ -166,8 +166,55 @@ Single-signature probe (exit 0 = should file, 1 = suppressed): python3 .github/groom/ledger.py --repo owner/name --check "" ``` +## `key-broker.mjs` — the localhost API-key proxy (BE-4419) + +A tiny, dependency-free localhost HTTP proxy that **holds the real Anthropic API +key and injects it into forwarded requests**, so the groom agent steps (BE-4311) +can run the Claude CLI with only a **dummy** key and `ANTHROPIC_BASE_URL` pointed +at the broker — the real key never enters the agent step's environment. + +**Why not just put the key in the agent step's env?** The groom agent has an +unscoped `Read` tool and `Bash(cat:*)`, and everything on the runner is the same +user, so `/proc//environ` — and, crucially, `/proc//environ` +and `/proc//cmdline` — are all agent-readable. The broker's design +follows from that: + +1. **The real key arrives on stdin (first line), never via env or argv.** A key in + the process environment or command line would be recoverable straight out of + `/proc`. Stdin is not. The broker reads exactly the first line at startup and + exits non-zero if stdin closes without one. +2. **It never logs request/response headers or bodies** — the log file is + agent-readable too. At most it logs `method path -> status`. +3. **It listens on `127.0.0.1` only.** + +What it does per request: + +- `HEAD` / `GET` on `/` → answered locally with `200` (the CLI's connectivity + probe; never forwarded). +- Path starting with `/v1/` → forwarded to the upstream preserving method and + body (streamed both ways, so SSE works), **stripping** any inbound `x-api-key`, + `authorization`, and `host`, then **injecting** the real `x-api-key` and the + upstream `host`. Upstream status + response headers are copied back verbatim. +- Anything else → `404` locally. Upstream connection error → `502` with a static + body (no detail echoed). + +Env knobs (config only — **never** the key): + +| Env var | Default | Meaning | +|---|---|---| +| `GROOM_BROKER_PORT` | `8199` | port to listen on (`127.0.0.1:`) | +| `GROOM_BROKER_UPSTREAM` | `https://api.anthropic.com` | where `/v1/*` is forwarded (overridable so tests can point it at a local fake) | + +On listen it prints one readiness line (`groom-key-broker listening on +127.0.0.1:`); a consumer's wait-loop should key off the port being +connectable, not the line. + +> **groom.yml wiring lands in the sibling ticket (BE-4311)** — this file adds the +> broker script + its unit tests only; nothing in `groom.yml` calls it yet. + - **`tests/`** — `unittest` suite, run by - [`test-groom-scripts.yml`](../workflows/test-groom-scripts.yml). + [`test-groom-scripts.yml`](../workflows/test-groom-scripts.yml). The key-broker + tests boot the real script under `node` (skipped when `node` is absent). ```bash python3 -m unittest discover -s .github/groom/tests -p 'test_*.py' -v diff --git a/.github/groom/key-broker.mjs b/.github/groom/key-broker.mjs new file mode 100644 index 0000000..40ccf7d --- /dev/null +++ b/.github/groom/key-broker.mjs @@ -0,0 +1,147 @@ +#!/usr/bin/env node +// groom key-broker (BE-4419) — a tiny localhost HTTP proxy that HOLDS the real +// API key and injects it into forwarded requests, so the groom agent steps +// (BE-4311) can run the Claude CLI with only a DUMMY key and the CLI's base-URL +// override pointed at this broker (see .github/groom/README.md for the env +// wiring the caller sets). +// +// Why a broker at all: the groom agent has an unscoped Read tool and +// `Bash(cat:*)`, and everything on the runner is the same user — so +// `/proc//environ` and `/proc//cmdline` are +// agent-readable. Therefore the real key MUST arrive on **stdin** (never env, +// never argv), this process must never log request/response headers or bodies +// (the log file is agent-readable too — we log at most `method path -> status`), +// and it listens on 127.0.0.1 only. This file deliberately never names or reads +// the provider key's env var (a grep for the provider name over this file finds +// nothing) — the only env it reads is GROOM_BROKER_PORT / GROOM_BROKER_UPSTREAM. + +import http from "node:http"; +import https from "node:https"; +import readline from "node:readline"; + +const DEFAULT_PORT = 8199; +const DEFAULT_UPSTREAM = "https://api.anthropic.com"; + +function die(message) { + console.error(`groom-key-broker: ${message}`); + process.exit(1); +} + +// --- config (env only — NOT the key) ------------------------------------- +const rawPort = process.env.GROOM_BROKER_PORT || String(DEFAULT_PORT); +const PORT = Number.parseInt(rawPort, 10); +if (!Number.isInteger(PORT) || PORT < 1 || PORT > 65535) { + die(`invalid GROOM_BROKER_PORT: ${JSON.stringify(rawPort)}`); +} + +let upstream; +try { + upstream = new URL(process.env.GROOM_BROKER_UPSTREAM || DEFAULT_UPSTREAM); +} catch { + die(`invalid GROOM_BROKER_UPSTREAM: ${JSON.stringify(process.env.GROOM_BROKER_UPSTREAM)}`); +} +if (upstream.protocol !== "http:" && upstream.protocol !== "https:") { + die(`GROOM_BROKER_UPSTREAM must be http/https, got ${upstream.protocol}`); +} +const client = upstream.protocol === "http:" ? http : https; + +// --- the real key: first line of stdin, nothing else --------------------- +async function readKeyFromStdin() { + const rl = readline.createInterface({ input: process.stdin }); + for await (const line of rl) { + rl.close(); + return line; + } + return null; // stdin closed without a single line +} + +const firstLine = await readKeyFromStdin(); +// readline strips the trailing newline; also drop a lone CR from CRLF input. +const realKey = firstLine == null ? "" : firstLine.replace(/\r$/, ""); +// We no longer need stdin — release the pipe so nothing lingers holding the key. +process.stdin.destroy(); +if (!realKey) { + die("no API key on stdin (first line was empty or stdin closed before a line); refusing to start"); +} + +// --- request logging: method + path + status ONLY, never headers/body ---- +function logReq(method, path, status) { + process.stderr.write(`${method} ${path} -> ${status}\n`); +} + +// --- forward a /v1/* request to the upstream, injecting the real key ----- +function forward(req, res, method, path) { + const headers = { ...req.headers }; + // Strip any inbound credentials the CLI sent (the dummy x-api-key, and an + // authorization: Bearer if an auth-token env was set) and the inbound host. + delete headers["x-api-key"]; + delete headers["authorization"]; + delete headers["host"]; + headers["x-api-key"] = realKey; + headers["host"] = upstream.host; + + const upstreamReq = client.request( + { + protocol: upstream.protocol, + hostname: upstream.hostname, + port: upstream.port || undefined, + method, + path: req.url, // forward path+query verbatim (already validated /v1/*) + headers, + }, + (upstreamRes) => { + // Copy upstream status + response headers back verbatim; stream the body + // (SSE streaming works because we pipe rather than buffer). + res.writeHead(upstreamRes.statusCode || 502, upstreamRes.headers); + upstreamRes.pipe(res); + logReq(method, path, upstreamRes.statusCode || 502); + }, + ); + + upstreamReq.on("error", () => { + if (!res.headersSent) { + res.writeHead(502, { "content-type": "text/plain" }); + res.end("upstream unavailable"); + } else { + res.end(); + } + logReq(method, path, 502); + }); + + // Stream the request body up to the upstream (so large / streaming bodies work). + req.pipe(upstreamReq); +} + +const server = http.createServer((req, res) => { + const method = req.method || "GET"; + const rawUrl = req.url || "/"; + const path = rawUrl.split("?", 1)[0]; + + // The CLI's connectivity probe — answer locally, never forward. + if ((method === "HEAD" || method === "GET") && path === "/") { + res.writeHead(200); + res.end(); + logReq(method, path, 200); + return; + } + + if (path.startsWith("/v1/")) { + forward(req, res, method, path); + return; + } + + // Anything else is not part of the CLI's surface — refuse locally. + res.writeHead(404); + res.end(); + logReq(method, path, 404); +}); + +server.on("error", (err) => { + die(`failed to listen on 127.0.0.1:${PORT}: ${err.message}`); +}); + +server.listen(PORT, "127.0.0.1", () => { + // Exactly one readiness line (the wait-loop keys off the port being + // connectable; this line is for debugging). + console.log(`groom-key-broker listening on 127.0.0.1:${PORT}`); +}); diff --git a/.github/groom/tests/test_key_broker.py b/.github/groom/tests/test_key_broker.py new file mode 100644 index 0000000..870ecd3 --- /dev/null +++ b/.github/groom/tests/test_key_broker.py @@ -0,0 +1,282 @@ +#!/usr/bin/env python3 +"""Tests for the groom key-broker (BE-4419). + +`key-broker.mjs` is a localhost HTTP proxy that HOLDS the real API key and +injects it into forwarded requests, so the groom agent steps (BE-4311) can run +the Claude CLI with only a dummy key + a base-URL override pointed at the broker. +The properties under test are the ones the design leans on: + + * inbound credentials (a dummy `x-api-key`, an `authorization: Bearer`) are + stripped and the REAL key is injected before forwarding to the upstream; + * request bodies reach the upstream byte-for-byte (streaming, so SSE works); + * the connectivity probe (`HEAD /`) and non-`/v1/` paths are answered locally + and never forwarded; + * the real key is NOT visible in the broker process's env or argv + (`/proc//{environ,cmdline}` are agent-readable on the shared runner); + * an upstream connection error yields a local 502 and the broker stays alive. + +These are integration tests: they boot the real `key-broker.mjs` under `node` +against a local fake upstream. The class is skipped when `node` is absent. + +Run: python3 -m unittest discover -s .github/groom/tests -p 'test_*.py' -v +""" + +import http.client +import os +import shutil +import socket +import socketserver +import subprocess +import sys +import time +import unittest +from http.server import BaseHTTPRequestHandler, ThreadingHTTPServer +from threading import Thread + +# A distinctive sentinel so the /proc assertions can't false-negative against a +# real key that happens to live in this machine's environment. +REAL_KEY = "sk-broker-test-REALKEY-9f2c1a7b4e" + +NODE = shutil.which("node") +SCRIPT = os.path.abspath(os.path.join(os.path.dirname(__file__), "..", "key-broker.mjs")) + + +def _free_port(): + """Bind :0, read the assigned port, release it — good enough for a test.""" + s = socket.socket(socket.AF_INET, socket.SOCK_STREAM) + try: + s.bind(("127.0.0.1", 0)) + return s.getsockname()[1] + finally: + s.close() + + +def _wait_port(port, timeout=15): + deadline = time.time() + timeout + while time.time() < deadline: + try: + with socket.create_connection(("127.0.0.1", port), timeout=0.2): + return True + except OSError: + time.sleep(0.05) + return False + + +class _RecordingHandler(BaseHTTPRequestHandler): + """Fake upstream: records each request and returns a canned JSON response.""" + + def _handle(self): + length = int(self.headers.get("Content-Length") or 0) + body = self.rfile.read(length) if length else b"" + self.server.records.append({ + "method": self.command, + "path": self.path, + "headers": {k.lower(): v for k, v in self.headers.items()}, + "body": body, + }) + payload = self.server.resp_body + self.send_response(self.server.resp_status) + self.send_header("Content-Type", "application/json") + self.send_header("Content-Length", str(len(payload))) + self.end_headers() + self.wfile.write(payload) + + do_GET = _handle + do_POST = _handle + do_PUT = _handle + + def log_message(self, *args): # silence the default stderr access log + pass + + +class _Upstream(ThreadingHTTPServer): + def server_bind(self): + # Skip HTTPServer.server_bind's socket.getfqdn() reverse-DNS lookup, which + # can hang for tens of seconds on hosts with slow PTR resolution (seen on + # macOS). TCPServer.server_bind just binds; we set the name/port ourselves. + socketserver.TCPServer.server_bind(self) + host, port = self.server_address[:2] + self.server_name = host + self.server_port = port + + +def _start_upstream(): + server = _Upstream(("127.0.0.1", 0), _RecordingHandler) + server.records = [] + server.resp_status = 200 + server.resp_body = b'{"upstream":"pong","n":42}' + thread = Thread(target=server.serve_forever, daemon=True) + thread.start() + return server + + +def _start_broker(upstream_url, port): + env = {**os.environ, "GROOM_BROKER_UPSTREAM": upstream_url, "GROOM_BROKER_PORT": str(port)} + proc = subprocess.Popen( + [NODE, SCRIPT], + stdin=subprocess.PIPE, + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + env=env, + ) + # The real key arrives ONLY on stdin's first line — never env, never argv. + proc.stdin.write((REAL_KEY + "\n").encode()) + proc.stdin.flush() + return proc + + +def _stop_broker(proc): + proc.terminate() + try: + proc.wait(timeout=5) + except subprocess.TimeoutExpired: + proc.kill() + proc.wait() + if proc.stdin: + proc.stdin.close() + + +@unittest.skipUnless(NODE, "node is required to run the key-broker") +class KeyBrokerTest(unittest.TestCase): + def setUp(self): + self.upstream = _start_upstream() + # Cleanups run LIFO: shutdown() the serve loop first, then close the socket. + self.addCleanup(self.upstream.server_close) + self.addCleanup(self.upstream.shutdown) + upstream_url = "http://127.0.0.1:%d" % self.upstream.server_address[1] + + self.port = _free_port() + self.broker = _start_broker(upstream_url, self.port) + self.addCleanup(_stop_broker, self.broker) + if not _wait_port(self.port): + raise self.failureException("broker did not become connectable") + + def _conn(self, port=None): + return http.client.HTTPConnection("127.0.0.1", port or self.port, timeout=5) + + @property + def records(self): + return self.upstream.records + + # --- header injection + status/body round-trip ------------------------- + + def test_injects_real_key_and_strips_inbound_credentials(self): + self.upstream.resp_status = 202 # prove a non-200 status round-trips + conn = self._conn() + conn.request( + "POST", + "/v1/messages?beta=true", + body=b'{"model":"x"}', + headers={ + "x-api-key": "dummy", + "authorization": "Bearer nope", + "content-type": "application/json", + }, + ) + resp = conn.getresponse() + got_body = resp.read() + conn.close() + + self.assertEqual(resp.status, 202) + self.assertEqual(got_body, self.upstream.resp_body) + + self.assertEqual(len(self.records), 1) + rec = self.records[0] + self.assertEqual(rec["method"], "POST") + self.assertEqual(rec["path"], "/v1/messages?beta=true") + # The upstream saw the REAL key, not the dummy the CLI sent... + self.assertEqual(rec["headers"].get("x-api-key"), REAL_KEY) + self.assertNotEqual(rec["headers"].get("x-api-key"), "dummy") + # ...and the inbound authorization header was stripped entirely. + self.assertNotIn("authorization", rec["headers"]) + + def test_request_body_reaches_upstream_intact(self): + body = b'{"prompt":"unicode \xe2\x9c\x93 check","pad":"' + b"x" * 2048 + b'"}' + conn = self._conn() + conn.request( + "POST", + "/v1/messages", + body=body, + headers={"x-api-key": "dummy", "content-type": "application/json"}, + ) + resp = conn.getresponse() + resp.read() + conn.close() + + self.assertEqual(len(self.records), 1) + self.assertEqual(self.records[0]["body"], body) + + # --- local-only responses (never forwarded) --------------------------- + + def test_head_root_is_answered_locally(self): + conn = self._conn() + conn.request("HEAD", "/") + resp = conn.getresponse() + resp.read() + conn.close() + + self.assertEqual(resp.status, 200) + self.assertEqual(self.records, []) # the probe was NOT forwarded + + def test_non_v1_path_is_404_and_not_forwarded(self): + conn = self._conn() + conn.request("GET", "/anything-else") + resp = conn.getresponse() + resp.read() + conn.close() + + self.assertEqual(resp.status, 404) + self.assertEqual(self.records, []) + + # --- the key must not leak via the process table ---------------------- + + @unittest.skipUnless(sys.platform.startswith("linux"), "/proc is Linux-only") + def test_key_absent_from_process_env_and_argv(self): + pid = self.broker.pid + with open("/proc/%d/environ" % pid, "rb") as f: + environ = f.read() + with open("/proc/%d/cmdline" % pid, "rb") as f: + cmdline = f.read() + self.assertNotIn(REAL_KEY.encode(), environ) + self.assertNotIn(REAL_KEY.encode(), cmdline) + + # --- upstream failure is a local 502 and non-fatal -------------------- + + def test_upstream_down_returns_502_and_broker_survives(self): + dead_port = _free_port() # nothing is listening here + broker_port = _free_port() + proc = _start_broker("http://127.0.0.1:%d" % dead_port, broker_port) + self.addCleanup(_stop_broker, proc) + if not _wait_port(broker_port): + raise self.failureException("broker did not become connectable") + + for _ in range(2): # a second request proves the broker stayed alive + conn = self._conn(broker_port) + conn.request("POST", "/v1/messages", body=b"{}", + headers={"content-type": "application/json"}) + resp = conn.getresponse() + resp.read() + conn.close() + self.assertEqual(resp.status, 502) + + self.assertIsNone(proc.poll(), "broker should stay alive after an upstream error") + + +class BrokerStartupTest(unittest.TestCase): + """Startup guards that don't need a running upstream.""" + + @unittest.skipUnless(NODE, "node is required to run the key-broker") + def test_exits_nonzero_when_stdin_has_no_key(self): + # No key on stdin (immediate EOF) -> refuse to start, non-zero exit. + proc = subprocess.Popen( + [NODE, SCRIPT], + stdin=subprocess.DEVNULL, + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + env={**os.environ, "GROOM_BROKER_PORT": str(_free_port())}, + ) + self.assertNotEqual(proc.wait(timeout=10), 0) + + +if __name__ == "__main__": + unittest.main() From 1241e1f32eb1197b7fba5fbe974db31ecc0b8a2a Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Fri, 24 Jul 2026 14:58:05 -0700 Subject: [PATCH 2/3] fix(groom): harden key-broker stream teardown, timeouts, and config guards (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 --- .github/groom/README.md | 9 +- .github/groom/key-broker.mjs | 121 +++++++++++++++++++------ .github/groom/tests/test_key_broker.py | 68 ++++++++++++++ 3 files changed, 168 insertions(+), 30 deletions(-) diff --git a/.github/groom/README.md b/.github/groom/README.md index 486df5c..4ddff2b 100644 --- a/.github/groom/README.md +++ b/.github/groom/README.md @@ -203,11 +203,14 @@ Env knobs (config only — **never** the key): | Env var | Default | Meaning | |---|---|---| | `GROOM_BROKER_PORT` | `8199` | port to listen on (`127.0.0.1:`) | -| `GROOM_BROKER_UPSTREAM` | `https://api.anthropic.com` | where `/v1/*` is forwarded (overridable so tests can point it at a local fake) | +| `GROOM_BROKER_UPSTREAM` | `https://api.anthropic.com` | where `/v1/*` is forwarded (overridable so tests can point it at a local fake; `http://` is accepted **only** for loopback hosts, so a plaintext non-loopback upstream can't leak the injected key) | On listen it prints one readiness line (`groom-key-broker listening on -127.0.0.1:`); a consumer's wait-loop should key off the port being -connectable, not the line. +127.0.0.1:`); a consumer's wait-loop should key off **that line** (proof +the broker itself bound the port), not merely the port being connectable — a +foreign process already holding the port would pass a bare connect check while +the broker exits with `EADDRINUSE`, and the consumer would then stream prompts +and repo data (plus the dummy key) to an unrelated listener. > **groom.yml wiring lands in the sibling ticket (BE-4311)** — this file adds the > broker script + its unit tests only; nothing in `groom.yml` calls it yet. diff --git a/.github/groom/key-broker.mjs b/.github/groom/key-broker.mjs index 40ccf7d..4ca73cc 100644 --- a/.github/groom/key-broker.mjs +++ b/.github/groom/key-broker.mjs @@ -18,9 +18,17 @@ import http from "node:http"; import https from "node:https"; import readline from "node:readline"; +import { pipeline } from "node:stream"; const DEFAULT_PORT = 8199; const DEFAULT_UPSTREAM = "https://api.anthropic.com"; +// Socket-inactivity timeout for a forwarded upstream request: a stalled or dead +// upstream is torn down instead of pinning the forwarded request (and the real +// key it carries) plus its downstream socket open forever, which would let +// repeated stalls exhaust the broker's sockets. This is an INACTIVITY timeout — +// it does not fire while data (e.g. an SSE token stream) keeps flowing — so it +// is set generously to never clip a legitimately slow/long streaming reply. +const UPSTREAM_TIMEOUT_MS = 600_000; function die(message) { console.error(`groom-key-broker: ${message}`); @@ -29,11 +37,18 @@ function die(message) { // --- config (env only — NOT the key) ------------------------------------- const rawPort = process.env.GROOM_BROKER_PORT || String(DEFAULT_PORT); -const PORT = Number.parseInt(rawPort, 10); +// Reject the whole string, not just parseInt's leading digits — otherwise +// "8199x" (→ 8199) or "1e3" (→ 1) would be silently coerced to a valid port. +const PORT = /^\d+$/.test(rawPort) ? Number.parseInt(rawPort, 10) : NaN; if (!Number.isInteger(PORT) || PORT < 1 || PORT > 65535) { die(`invalid GROOM_BROKER_PORT: ${JSON.stringify(rawPort)}`); } +function isLoopbackHost(host) { + const h = host.replace(/^\[|\]$/g, ""); // strip IPv6 brackets, e.g. [::1] + return h === "localhost" || h === "::1" || /^127\./.test(h); +} + let upstream; try { upstream = new URL(process.env.GROOM_BROKER_UPSTREAM || DEFAULT_UPSTREAM); @@ -43,7 +58,17 @@ try { if (upstream.protocol !== "http:" && upstream.protocol !== "https:") { die(`GROOM_BROKER_UPSTREAM must be http/https, got ${upstream.protocol}`); } +// Cleartext http:// is allowed ONLY for loopback (the fake upstream used by the +// tests). Injecting the real key into a plaintext request to a remote host would +// transmit the secret in the clear — defeating the broker's whole purpose. +if (upstream.protocol === "http:" && !isLoopbackHost(upstream.hostname)) { + die(`GROOM_BROKER_UPSTREAM http:// is allowed only for loopback hosts (got ${upstream.hostname}); use https:// for remote upstreams`); +} const client = upstream.protocol === "http:" ? http : https; +// Prefix from the upstream URL (e.g. https://host/prefix → "/prefix"), stripped +// of a trailing slash; "" for a root upstream. Prepended to the forwarded path +// so a non-root upstream isn't silently dropped. +const upstreamPrefix = upstream.pathname.replace(/\/+$/, ""); // --- the real key: first line of stdin, nothing else --------------------- async function readKeyFromStdin() { @@ -56,8 +81,10 @@ async function readKeyFromStdin() { } const firstLine = await readKeyFromStdin(); -// readline strips the trailing newline; also drop a lone CR from CRLF input. -const realKey = firstLine == null ? "" : firstLine.replace(/\r$/, ""); +// readline strips the trailing newline; trim() drops a lone CR from CRLF input +// plus any leading/trailing whitespace common in copy-pasted secrets, which +// would otherwise be an illegal header character that throws on client.request. +const realKey = firstLine == null ? "" : firstLine.trim(); // We no longer need stdin — release the pipe so nothing lingers holding the key. process.stdin.destroy(); if (!realKey) { @@ -80,36 +107,74 @@ function forward(req, res, method, path) { headers["x-api-key"] = realKey; headers["host"] = upstream.host; - const upstreamReq = client.request( - { - protocol: upstream.protocol, - hostname: upstream.hostname, - port: upstream.port || undefined, - method, - path: req.url, // forward path+query verbatim (already validated /v1/*) - headers, - }, - (upstreamRes) => { - // Copy upstream status + response headers back verbatim; stream the body - // (SSE streaming works because we pipe rather than buffer). - res.writeHead(upstreamRes.statusCode || 502, upstreamRes.headers); - upstreamRes.pipe(res); - logReq(method, path, upstreamRes.statusCode || 502); - }, - ); - - upstreamReq.on("error", () => { + // `settled` guards the 502 path: once the upstream's headers are on their way + // back (or we've already emitted a 502), a later error must NOT try to write a + // second response. + let settled = false; + const fail502 = () => { + if (settled) return; + settled = true; if (!res.headersSent) { res.writeHead(502, { "content-type": "text/plain" }); res.end("upstream unavailable"); } else { - res.end(); + // Headers already flushed — destroy so the caller sees a broken + // connection, not a silently truncated "success". + res.destroy(); } logReq(method, path, 502); + }; + + let upstreamReq; + try { + upstreamReq = client.request({ + protocol: upstream.protocol, + hostname: upstream.hostname, + port: upstream.port || undefined, + method, + // Prepend any upstream path prefix; req.url is already validated /v1/*. + path: upstreamPrefix + req.url, + headers, + timeout: UPSTREAM_TIMEOUT_MS, + }); + } catch { + // client.request can throw synchronously (an illegal char in the request + // target or an injected key). Fail this one request; never crash the broker. + fail502(); + req.resume(); // drain the inbound body so its socket can close cleanly + return; + } + + upstreamReq.on("response", (upstreamRes) => { + // Past the 502 window: copy upstream status + headers back verbatim, then + // stream the body. pipeline tears down BOTH sides on any error/disconnect, + // so an upstream reset mid-stream destroys `res` (the caller sees a broken + // connection, not a truncated 200) and a client disconnect destroys the + // upstream response. + settled = true; + res.writeHead(upstreamRes.statusCode || 502, upstreamRes.headers); + logReq(method, path, upstreamRes.statusCode || 502); + pipeline(upstreamRes, res, () => {}); }); - // Stream the request body up to the upstream (so large / streaming bodies work). - req.pipe(upstreamReq); + // A stalled upstream trips the inactivity timeout; destroy() surfaces as an + // 'error' handled below (local 502 if nothing sent yet, else a torn stream). + upstreamReq.on("timeout", () => upstreamReq.destroy(new Error("upstream timeout"))); + // Connection-level errors (refused/reset/DNS) before any response → local 502. + upstreamReq.on("error", fail502); + + // Client disconnect (or normal completion) → tear down the outbound + // real-key-bearing request so it can't linger and leak its socket. + res.on("close", () => { + if (!upstreamReq.destroyed) upstreamReq.destroy(); + }); + + // Stream the inbound body up to the upstream (so large / streaming bodies + // work). pipeline aborts the upstream on a client abort rather than letting an + // unhandled 'error' on `req` crash the broker. + pipeline(req, upstreamReq, (err) => { + if (err && !upstreamReq.destroyed) upstreamReq.destroy(); + }); } const server = http.createServer((req, res) => { @@ -141,7 +206,9 @@ server.on("error", (err) => { }); server.listen(PORT, "127.0.0.1", () => { - // Exactly one readiness line (the wait-loop keys off the port being - // connectable; this line is for debugging). + // Exactly one readiness line. A consumer wait-loop should match THIS line + // (proof the broker itself bound the port) rather than a bare port-connect + // check, which a foreign process already holding the port would pass while the + // broker exits with EADDRINUSE. console.log(`groom-key-broker listening on 127.0.0.1:${PORT}`); }); diff --git a/.github/groom/tests/test_key_broker.py b/.github/groom/tests/test_key_broker.py index 870ecd3..4f94e2f 100644 --- a/.github/groom/tests/test_key_broker.py +++ b/.github/groom/tests/test_key_broker.py @@ -261,6 +261,51 @@ def test_upstream_down_returns_502_and_broker_survives(self): self.assertIsNone(proc.poll(), "broker should stay alive after an upstream error") + # --- a client abort mid-request must not crash the broker ------------- + + def test_client_abort_does_not_kill_broker(self): + # Send a Content-Length that promises more body than we deliver, then + # slam the socket shut — the classic "unhandled 'error' on req.pipe" + # crash. The broker must survive and still serve the next request. + raw = socket.create_connection(("127.0.0.1", self.port), timeout=5) + raw.sendall( + b"POST /v1/messages HTTP/1.1\r\n" + b"Host: 127.0.0.1\r\n" + b"Content-Type: application/json\r\n" + b"Content-Length: 4096\r\n\r\n" + b"{}" # far short of the promised 4096 bytes + ) + raw.close() # abort mid-body + time.sleep(0.2) + + self.assertIsNone(self.broker.poll(), "broker must survive a client abort") + conn = self._conn() # a fresh request proves it's still serving + conn.request("HEAD", "/") + resp = conn.getresponse() + resp.read() + conn.close() + self.assertEqual(resp.status, 200) + + # --- a non-root upstream keeps its path prefix ------------------------ + + def test_upstream_path_prefix_is_preserved(self): + prefixed = "http://127.0.0.1:%d/prefix" % self.upstream.server_address[1] + port = _free_port() + proc = _start_broker(prefixed, port) + self.addCleanup(_stop_broker, proc) + if not _wait_port(port): + raise self.failureException("broker did not become connectable") + + conn = self._conn(port) + conn.request("POST", "/v1/messages", body=b"{}", + headers={"content-type": "application/json"}) + resp = conn.getresponse() + resp.read() + conn.close() + + self.assertEqual(resp.status, 200) + self.assertEqual(self.records[-1]["path"], "/prefix/v1/messages") + class BrokerStartupTest(unittest.TestCase): """Startup guards that don't need a running upstream.""" @@ -277,6 +322,29 @@ def test_exits_nonzero_when_stdin_has_no_key(self): ) self.assertNotEqual(proc.wait(timeout=10), 0) + def _startup_exit_code(self, env_overrides): + env = {**os.environ, "GROOM_BROKER_PORT": str(_free_port())} + env.update(env_overrides) + proc = subprocess.Popen( + [NODE, SCRIPT], + stdin=subprocess.DEVNULL, + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + env=env, + ) + return proc.wait(timeout=10) + + @unittest.skipUnless(NODE, "node is required to run the key-broker") + def test_rejects_port_with_trailing_garbage(self): + # "8199x" must be rejected, not silently coerced to 8199. + self.assertNotEqual(self._startup_exit_code({"GROOM_BROKER_PORT": "8199x"}), 0) + + @unittest.skipUnless(NODE, "node is required to run the key-broker") + def test_rejects_plaintext_non_loopback_upstream(self): + # http:// to a non-loopback host would leak the key in cleartext. + code = self._startup_exit_code({"GROOM_BROKER_UPSTREAM": "http://example.com"}) + self.assertNotEqual(code, 0) + if __name__ == "__main__": unittest.main() From 55081e200666b19d8484821f3d7b227d5a7467bf Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Fri, 24 Jul 2026 15:32:26 -0700 Subject: [PATCH 3/3] test(groom): add regression test for stdin key trimming (BE-4419) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .github/groom/tests/test_key_broker.py | 28 ++++++++++++++++++++++++-- 1 file changed, 26 insertions(+), 2 deletions(-) diff --git a/.github/groom/tests/test_key_broker.py b/.github/groom/tests/test_key_broker.py index 4f94e2f..e691fef 100644 --- a/.github/groom/tests/test_key_broker.py +++ b/.github/groom/tests/test_key_broker.py @@ -110,7 +110,7 @@ def _start_upstream(): return server -def _start_broker(upstream_url, port): +def _start_broker(upstream_url, port, stdin_payload=None): env = {**os.environ, "GROOM_BROKER_UPSTREAM": upstream_url, "GROOM_BROKER_PORT": str(port)} proc = subprocess.Popen( [NODE, SCRIPT], @@ -120,7 +120,10 @@ def _start_broker(upstream_url, port): env=env, ) # The real key arrives ONLY on stdin's first line — never env, never argv. - proc.stdin.write((REAL_KEY + "\n").encode()) + # stdin_payload lets a test feed a raw (e.g. padded/CRLF) line; default is + # the clean key followed by the newline the broker's readline consumes. + payload = (REAL_KEY + "\n") if stdin_payload is None else stdin_payload + proc.stdin.write(payload.encode()) proc.stdin.flush() return proc @@ -190,6 +193,27 @@ def test_injects_real_key_and_strips_inbound_credentials(self): # ...and the inbound authorization header was stripped entirely. self.assertNotIn("authorization", rec["headers"]) + def test_stdin_key_is_trimmed(self): + # A padded/copy-pasted secret (leading+trailing spaces + CRLF) must reach + # the upstream TRIMMED: the raw value carries illegal header chars that + # would otherwise throw on client.request. Guards key-broker.mjs `.trim()`. + padded = " " + REAL_KEY + " \r\n" + port = _free_port() + upstream_url = "http://127.0.0.1:%d" % self.upstream.server_address[1] + proc = _start_broker(upstream_url, port, stdin_payload=padded) + self.addCleanup(_stop_broker, proc) + if not _wait_port(port): + raise self.failureException("padded-key broker did not become connectable") + + conn = self._conn(port) + conn.request("POST", "/v1/messages", body=b"{}", headers={"content-type": "application/json"}) + resp = conn.getresponse() + resp.read() + conn.close() + + # The trimmed key — not the padded input — reaches x-api-key upstream. + self.assertEqual(self.records[-1]["headers"].get("x-api-key"), REAL_KEY) + def test_request_body_reaches_upstream_intact(self): body = b'{"prompt":"unicode \xe2\x9c\x93 check","pad":"' + b"x" * 2048 + b'"}' conn = self._conn()