fix(cli): bind local diagnostic reads to runtime - #1448
Conversation
📝 WalkthroughWalkthroughThe PR replaces reusable management credentials and attestation exchanges with short-lived, process-bound HMAC capabilities. It adds direct local HTTP transport and updates management authentication, diagnostics, OAuth health checks, documentation, and tests. ChangesLocal management read capabilities
Estimated code review effort: 5 (Critical) | ~100 minutes Sequence Diagram(s)sequenceDiagram
participant DoctorOrOAuthHealth
participant LocalManagementReadClient
participant RuntimePortRecord
participant DirectLocalHttp
participant ManagementAuth
participant ManagementAPI
DoctorOrOAuthHealth->>LocalManagementReadClient: Request an allowed local GET
LocalManagementReadClient->>RuntimePortRecord: Read runtime PID, port, and secret
LocalManagementReadClient->>LocalManagementReadClient: Create a short-lived bound capability
LocalManagementReadClient->>DirectLocalHttp: Send the local HTTP request
DirectLocalHttp->>ManagementAuth: Deliver capability and attestation headers
ManagementAuth->>ManagementAPI: Authorize the exact read once
ManagementAPI-->>DoctorOrOAuthHealth: Return memory or account health data
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
the
intake: hygiene-blocked
|
⏳ DRAFT
What to do
Review readiness checklist
2/4 boxes ticked. This pull request was already a draft. Its draft status will be preserved after every issue above is resolved. |
|
@codex review |
|
✅ Action performedReview finished.
|
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/server/local-management-read-client.ts`:
- Around line 69-81: Ensure the capability-bearing request in the local
management client uses a direct connection that bypasses HTTP/HTTPS proxies for
the exact loopback host, rather than relying on default fetch behavior. Apply
the same proxy-bypass protection to shared loopback probes used by
proxyLiveness. Add a test covering configured proxy environment variables and
verifying loopback requests do not route through the proxy.
In `@structure/05_gui-and-management-api.md`:
- Around line 29-31: Fix the sentence at the boundary after “port” by removing
the stray “it” and connecting the clause so it clearly states that each
capability includes a short expiry in the HMAC and is consumed once by the
server. Preserve the surrounding management authentication guarantees and
wording.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 14642474-7241-472e-bf88-42bfa4d6827b
📒 Files selected for processing (10)
src/cli/doctor.tssrc/lib/local-management-capability.tssrc/oauth/health.tssrc/server/local-management-read-client.tssrc/server/management-auth.tsstructure/05_gui-and-management-api.mdtests/doctor.test.tstests/local-management-capability.test.tstests/oauth-health.test.tstests/server-management-auth.test.ts
|
Going to put a hard stop on this since i am working on CLI right now. We will see if its still viable after I am done working on it. |
c517aaa to
37055d5
Compare
|
@codex review |
|
✅ Action performedReview finished.
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37055d5c7b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const parsedHostname = url.hostname.startsWith("[") && url.hostname.endsWith("]") | ||
| ? url.hostname.slice(1, -1) | ||
| : url.hostname; | ||
| const hostname = parsedHostname.toLowerCase() === "localhost" ? "127.0.0.1" : parsedHostname; |
There was a problem hiding this comment.
When hostname is configured as localhost on a system where Bun resolves it to ::1, the service binds only on IPv6, but this conversion forces every liveness, readiness, status, and capability connection to 127.0.0.1, which returns ECONNREFUSED. As a result, CLI commands can report a running proxy as stopped or diagnostics as unavailable, and lifecycle commands may fail to locate it. Resolve localhost normally or attempt both loopback families while still using the direct, proxy-bypassing socket transport.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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 `@src/server/direct-local-http.ts`:
- Around line 232-298: Add a default timeout to the socket created in the direct
local HTTP request Promise, using the socket timeout mechanism to fail stalled
connections even when no signal is supplied. Ensure the timeout invokes finish
with an appropriate error and preserves the existing cleanup and rejection
behavior; keep caller-provided abort handling unchanged.
- Around line 150-190: Update parseResponse to call parseResponseHead for the
status and headers instead of duplicating boundary, status-line, and header
parsing. Reuse the returned head metadata and preserve the existing
bodyless-status and content-length handling; remove only the redundant
head-parsing logic, while ensuring chunked bodies continue through the
established framing/decoding path owned by the shared parser.
In `@structure/05_gui-and-management-api.md`:
- Around line 26-37: Update the authentication table in
structure/05_gui-and-management-api.md to document the runtime-secret-derived
local-read HMAC capability as an additional, scoped management admission
mechanism. State that it authorizes only GET requests to
/api/codex-auth/accounts and /api/system/memory, or explicitly qualify the table
as covering only reusable credential classes while adding this capability
separately; do not broaden it to other /api/* routes.
In `@tests/local-management-direct-transport.test.ts`:
- Around line 109-123: Update the proxy server callback in createServer to
record any capability header received, then directly assert after the request
flow that the recorded capability-header list is empty. Preserve the existing
proxyPaths assertion and response behavior, ensuring the test explicitly guards
against credential exposure rather than relying only on path-count inference.
- Around line 33-39: Extend the directLocalHttpFetch tests with a mid-flight
abort against a server that accepts the connection and never responds, asserting
the rejection preserves AbortError; keep the existing pre-flight case. Add a
focused checkProxyHealth test that aborts during the pending request and asserts
the result is "timed out" rather than "unreachable", using the existing test
helpers and cleanup patterns.
- Around line 41-65: Add negative framing tests alongside the existing
content-length and chunked cases, using the createTcpServer harness and
directLocalHttpFetch to send malformed responses and assert rejection. Cover
representative fail-closed paths in directLocalHttpFetch, including invalid
status or headers, invalid or oversized content length, invalid chunk framing,
and truncated bodies, while preserving cleanup of sockets and the server.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: e7990b88-b1c8-4893-91d8-a32ea1c36ff7
📒 Files selected for processing (7)
src/cli/status.tssrc/oauth/health.tssrc/server/direct-local-http.tssrc/server/local-management-read-client.tssrc/server/proxy-liveness.tsstructure/05_gui-and-management-api.mdtests/local-management-direct-transport.test.ts
| function parseResponse(bytes: Buffer): Response { | ||
| const boundary = headerBoundary(bytes); | ||
| if (boundary < 0) throw new Error("direct local HTTP response has no header boundary"); | ||
| const lines = bytes.subarray(0, boundary).toString("latin1").split("\r\n"); | ||
| const statusLine = lines.shift() ?? ""; | ||
| const match = /^HTTP\/1\.[01] ([0-9]{3})(?: (.*))?$/.exec(statusLine); | ||
| if (!match) throw new Error("direct local HTTP response has an invalid status line"); | ||
| const status = Number(match[1]); | ||
| if (status < 200 || status > 599) throw new Error("direct local HTTP response has an unsupported status"); | ||
|
|
||
| const headers = new Headers(); | ||
| for (const line of lines) { | ||
| const colon = line.indexOf(":"); | ||
| if (colon <= 0) throw new Error("direct local HTTP response has an invalid header"); | ||
| headers.append(line.slice(0, colon).trim(), line.slice(colon + 1).trim()); | ||
| } | ||
|
|
||
| let body = bytes.subarray(boundary + 4); | ||
| if (/\bchunked\b/i.test(headers.get("transfer-encoding") ?? "")) { | ||
| body = decodeChunkedBody(body); | ||
| headers.delete("transfer-encoding"); | ||
| headers.delete("content-length"); | ||
| } else { | ||
| const rawLength = headers.get("content-length"); | ||
| if (rawLength !== null) { | ||
| if (!/^[0-9]+$/.test(rawLength)) throw new Error("direct local HTTP response has an invalid content length"); | ||
| const length = Number(rawLength); | ||
| if (!Number.isSafeInteger(length) || body.byteLength < length) { | ||
| throw new Error("direct local HTTP response body is truncated"); | ||
| } | ||
| body = body.subarray(0, length); | ||
| } | ||
| } | ||
|
|
||
| const bodyless = status === 204 || status === 205 || status === 304; | ||
| return new Response(bodyless ? null : new Uint8Array(body), { | ||
| status, | ||
| statusText: match[2] ?? "", | ||
| headers, | ||
| }); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Remove the duplicated response parsing. parseResponse re-implements parseResponseHead and re-validates chunk framing.
This file parses the same wire bytes with two independent implementations:
- Lines 153-165 duplicate lines 61-72 of
parseResponseHeadexactly.advanceResponseFramingcallsparseResponseHeadat line 86, butparseResponseinlines a second copy instead of calling it. decodeChunkedBody(lines 16-45) re-validates chunk sizes and terminators thatadvanceResponseFramingalready validated at lines 107-137.
Failure mode: the two copies must stay in sync. If a later change hardens one copy, for example rejecting whitespace before the header colon per RFC 9112 §5.1 (both copies currently accept it through .trim()), the framer and the final parse will disagree about the same response bytes. For a transport that carries capability-bound reads, a framer/parser disagreement is a parsing-differential risk, not only a style problem.
Call parseResponseHead from parseResponse so one implementation owns head parsing.
♻️ Proposed fix to remove the duplicated head parsing
function parseResponse(bytes: Buffer): Response {
const boundary = headerBoundary(bytes);
if (boundary < 0) throw new Error("direct local HTTP response has no header boundary");
- const lines = bytes.subarray(0, boundary).toString("latin1").split("\r\n");
- const statusLine = lines.shift() ?? "";
- const match = /^HTTP\/1\.[01] ([0-9]{3})(?: (.*))?$/.exec(statusLine);
- if (!match) throw new Error("direct local HTTP response has an invalid status line");
- const status = Number(match[1]);
- if (status < 200 || status > 599) throw new Error("direct local HTTP response has an unsupported status");
-
- const headers = new Headers();
- for (const line of lines) {
- const colon = line.indexOf(":");
- if (colon <= 0) throw new Error("direct local HTTP response has an invalid header");
- headers.append(line.slice(0, colon).trim(), line.slice(colon + 1).trim());
- }
+ const { status, statusText, headers } = parseResponseHead(bytes, boundary);
let body = bytes.subarray(boundary + 4);
@@
const bodyless = status === 204 || status === 205 || status === 304;
return new Response(bodyless ? null : new Uint8Array(body), {
status,
- statusText: match[2] ?? "",
+ statusText,
headers,
});
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function parseResponse(bytes: Buffer): Response { | |
| const boundary = headerBoundary(bytes); | |
| if (boundary < 0) throw new Error("direct local HTTP response has no header boundary"); | |
| const lines = bytes.subarray(0, boundary).toString("latin1").split("\r\n"); | |
| const statusLine = lines.shift() ?? ""; | |
| const match = /^HTTP\/1\.[01] ([0-9]{3})(?: (.*))?$/.exec(statusLine); | |
| if (!match) throw new Error("direct local HTTP response has an invalid status line"); | |
| const status = Number(match[1]); | |
| if (status < 200 || status > 599) throw new Error("direct local HTTP response has an unsupported status"); | |
| const headers = new Headers(); | |
| for (const line of lines) { | |
| const colon = line.indexOf(":"); | |
| if (colon <= 0) throw new Error("direct local HTTP response has an invalid header"); | |
| headers.append(line.slice(0, colon).trim(), line.slice(colon + 1).trim()); | |
| } | |
| let body = bytes.subarray(boundary + 4); | |
| if (/\bchunked\b/i.test(headers.get("transfer-encoding") ?? "")) { | |
| body = decodeChunkedBody(body); | |
| headers.delete("transfer-encoding"); | |
| headers.delete("content-length"); | |
| } else { | |
| const rawLength = headers.get("content-length"); | |
| if (rawLength !== null) { | |
| if (!/^[0-9]+$/.test(rawLength)) throw new Error("direct local HTTP response has an invalid content length"); | |
| const length = Number(rawLength); | |
| if (!Number.isSafeInteger(length) || body.byteLength < length) { | |
| throw new Error("direct local HTTP response body is truncated"); | |
| } | |
| body = body.subarray(0, length); | |
| } | |
| } | |
| const bodyless = status === 204 || status === 205 || status === 304; | |
| return new Response(bodyless ? null : new Uint8Array(body), { | |
| status, | |
| statusText: match[2] ?? "", | |
| headers, | |
| }); | |
| } | |
| function parseResponse(bytes: Buffer): Response { | |
| const boundary = headerBoundary(bytes); | |
| if (boundary < 0) throw new Error("direct local HTTP response has no header boundary"); | |
| const { status, statusText, headers } = parseResponseHead(bytes, boundary); | |
| let body = bytes.subarray(boundary + 4); | |
| if (/\bchunked\b/i.test(headers.get("transfer-encoding") ?? "")) { | |
| body = decodeChunkedBody(body); | |
| headers.delete("transfer-encoding"); | |
| headers.delete("content-length"); | |
| } else { | |
| const rawLength = headers.get("content-length"); | |
| if (rawLength !== null) { | |
| if (!/^[0-9]+$/.test(rawLength)) throw new Error("direct local HTTP response has an invalid content length"); | |
| const length = Number(rawLength); | |
| if (!Number.isSafeInteger(length) || body.byteLength < length) { | |
| throw new Error("direct local HTTP response body is truncated"); | |
| } | |
| body = body.subarray(0, length); | |
| } | |
| } | |
| const bodyless = status === 204 || status === 205 || status === 304; | |
| return new Response(bodyless ? null : new Uint8Array(body), { | |
| status, | |
| statusText, | |
| headers, | |
| }); | |
| } |
🧰 Tools
🪛 OpenGrep (1.26.0)
[ERROR] 155-155: Dynamic command passed to child_process.exec/execSync. Use child_process.execFile or spawn with an argument array instead.
(coderabbit.command-injection.exec-js)
🤖 Prompt for 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.
In `@src/server/direct-local-http.ts` around lines 150 - 190, Update parseResponse
to call parseResponseHead for the status and headers instead of duplicating
boundary, status-line, and header parsing. Reuse the returned head metadata and
preserve the existing bodyless-status and content-length handling; remove only
the redundant head-parsing logic, while ensuring chunked bodies continue through
the established framing/decoding path owned by the shared parser.
| return await new Promise<Response>((resolve, reject) => { | ||
| let socket: Socket | undefined; | ||
| let settled = false; | ||
| let receivedBytes = 0; | ||
| let responseBytes = Buffer.allocUnsafe(4 * 1024); | ||
| let framing: ResponseFraming = { kind: "head", searchFrom: 0 }; | ||
| const cleanup = () => signal?.removeEventListener("abort", onAbort); | ||
| const finish = (error?: Error) => { | ||
| if (settled) return; | ||
| settled = true; | ||
| cleanup(); | ||
| try { socket?.destroy(); } catch { /* ignore */ } | ||
| if (error) { | ||
| reject(error); | ||
| return; | ||
| } | ||
| try { | ||
| resolve(parseResponse(responseBytes.subarray(0, receivedBytes))); | ||
| } catch (parseError) { | ||
| reject(parseError instanceof Error ? parseError : new Error(String(parseError))); | ||
| } | ||
| }; | ||
| const onAbort = () => { | ||
| const error = signal ? abortReason(signal) : new Error("direct local HTTP request aborted"); | ||
| try { socket?.destroy(error); } catch { /* ignore */ } | ||
| finish(error); | ||
| }; | ||
|
|
||
| socket = net.createConnection({ host: hostname, port }); | ||
| signal?.addEventListener("abort", onAbort, { once: true }); | ||
| if (signal?.aborted) { | ||
| onAbort(); | ||
| return; | ||
| } | ||
| socket.on("connect", () => { | ||
| try { socket?.write(requestBytes); } catch (error) { | ||
| finish(error instanceof Error ? error : new Error(String(error))); | ||
| } | ||
| }); | ||
| socket.on("data", chunk => { | ||
| if (settled) return; | ||
| const bytes = Buffer.from(chunk); | ||
| receivedBytes += bytes.byteLength; | ||
| if (receivedBytes > DIRECT_LOCAL_HTTP_MAX_BYTES) { | ||
| finish(new Error("direct local HTTP response exceeds the byte cap")); | ||
| return; | ||
| } | ||
| if (receivedBytes > responseBytes.byteLength) { | ||
| let capacity = responseBytes.byteLength; | ||
| while (capacity < receivedBytes) capacity = Math.min(DIRECT_LOCAL_HTTP_MAX_BYTES, capacity * 2); | ||
| const grown = Buffer.allocUnsafe(capacity); | ||
| responseBytes.copy(grown); | ||
| responseBytes = grown; | ||
| } | ||
| bytes.copy(responseBytes, receivedBytes - bytes.byteLength); | ||
| try { | ||
| framing = advanceResponseFraming(responseBytes.subarray(0, receivedBytes), framing); | ||
| if (framing.kind === "complete") finish(); | ||
| } catch (error) { | ||
| finish(error instanceof Error ? error : new Error(String(error))); | ||
| } | ||
| }); | ||
| socket.once("end", () => finish()); | ||
| socket.once("error", error => finish(error)); | ||
| socket.once("close", () => finish()); | ||
| }); | ||
| }) as typeof fetch; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add a socket timeout. Without a caller-supplied signal, this promise can hang forever.
signal is optional at line 207. If a caller omits it, nothing bounds the request:
net.createConnectionat line 260 applies no connect timeout.- No
socket.setTimeoutis set, so an idle peer that accepts the connection and never writes leaves the promise pending. finishruns only fromdata,end,error,close, oronAbort. None of these fire for a silently stalled peer.
Every caller in this cohort currently passes a signal (src/cli/status.ts line 119, src/server/proxy-liveness.ts lines 116 and 314), so this is not exploitable today. The function is exported and typed as typeof fetch, so the next caller can omit the signal and hang a CLI command with no output. Set a default socket timeout so the transport fails closed on its own.
🛡️ Proposed fix to bound the request without a signal
+const DIRECT_LOCAL_HTTP_DEFAULT_TIMEOUT_MS = 10_000;
+
function abortReason(signal: AbortSignal): Error { socket = net.createConnection({ host: hostname, port });
+ socket.setTimeout(DIRECT_LOCAL_HTTP_DEFAULT_TIMEOUT_MS, () => {
+ const error = new Error("direct local HTTP request timed out");
+ error.name = "TimeoutError";
+ finish(error);
+ });
signal?.addEventListener("abort", onAbort, { once: true });The buffer growth at lines 279-285 and the copy offset at line 286 are correct: line 275 bounds receivedBytes by the cap before the loop runs, so the doubling always terminates, and every read is bounded by subarray(0, receivedBytes), so the allocUnsafe tail is never exposed.
🤖 Prompt for 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.
In `@src/server/direct-local-http.ts` around lines 232 - 298, Add a default
timeout to the socket created in the direct local HTTP request Promise, using
the socket timeout mechanism to fail stalled connections even when no signal is
supplied. Ensure the timeout invokes finish with an appropriate error and
preserves the existing cleanup and rejection behavior; keep caller-provided
abort handling unchanged.
Source: Path instructions
| CLI health collection follows the same boundary without transporting the reusable management | ||
| credential. `ocx status` and `ocx doctor` derive process-scoped HMAC capabilities from the protected | ||
| `runtime-port.json` secret for exactly two read-only GETs: `/api/codex-auth/accounts` and | ||
| `/api/system/memory`. Each capability is bound to its method, path, nonce, proxy PID, and port. A | ||
| short expiry is part of the HMAC, and the server consumes each capability once. A capability cannot | ||
| authorize another management route or survive process replacement. These probes connect directly | ||
| to the selected listener instead of delegating local identity to an environment HTTP proxy. Their | ||
| output distinguishes | ||
| a missing proxy, rejected local capability, and an unexpected management response so a reachable | ||
| `401` cannot be reported as "proxy not running." Legacy or configured-port-only listeners still | ||
| satisfy ordinary liveness, but their detailed CLI health remains unavailable until restarted with | ||
| an attested runtime record and capability-aware server. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the local-read capability in the authentication table.
Line 26 introduces an additional accepted management admission mechanism. Lines 15-21 still state that only three mutually exclusive credential classes exist, and the management row lists only reusable admin-token sources.
Add a scoped local-read capability entry, or qualify the table as covering only reusable credential classes. State that the runtime-secret-derived HMAC capability authorizes only the two exact GET paths. This prevents future code from rejecting the valid capability or expanding its scope to all /api/* routes.
🤖 Prompt for 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.
In `@structure/05_gui-and-management-api.md` around lines 26 - 37, Update the
authentication table in structure/05_gui-and-management-api.md to document the
runtime-secret-derived local-read HMAC capability as an additional, scoped
management admission mechanism. State that it authorizes only GET requests to
/api/codex-auth/accounts and /api/system/memory, or explicitly qualify the table
as covering only reusable credential classes while adding this capability
separately; do not broaden it to other /api/* routes.
| test("preserves an AbortError for an already-cancelled request", async () => { | ||
| const controller = new AbortController(); | ||
| controller.abort(); | ||
| await expect(directLocalHttpFetch("http://127.0.0.1:9/healthz", { | ||
| signal: controller.signal, | ||
| })).rejects.toMatchObject({ name: "AbortError" }); | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add a mid-flight abort test. This test covers only the pre-flight abort path.
Aborting before the call exercises line 213 of src/server/direct-local-http.ts, which throws before any socket is created. Port 9 is never contacted.
The untested path is the one this PR hardened. When the abort arrives after the socket connects, onAbort at lines 254-258 calls socket.destroy(error), and the socket error handler at line 295 also calls finish(error). Whichever fires first wins the settled guard, so the rejected error can carry name === "Error" instead of "AbortError". That race is exactly why src/cli/status.ts lines 134-136 now also check controller.signal.aborted. Neither the race nor the new classification has a test.
Add two cases: abort against a server that accepts the connection and never replies, and a checkProxyHealth case asserting the result is "timed out" rather than "unreachable".
💚 Proposed mid-flight abort test
+ test("rejects a mid-flight request after the socket connects", async () => {
+ const sockets = new Set<Socket>();
+ const server = createTcpServer(socket => {
+ sockets.add(socket);
+ socket.once("close", () => sockets.delete(socket));
+ // Accept, then never reply, to force a mid-flight abort.
+ });
+ let port = 0;
+ try {
+ port = await listen(server);
+ const controller = new AbortController();
+ const pending = directLocalHttpFetch(`http://127.0.0.1:${port}/healthz`, {
+ signal: controller.signal,
+ });
+ await Bun.sleep(50);
+ controller.abort();
+ await expect(pending).rejects.toThrow();
+ expect(controller.signal.aborted).toBe(true);
+ } finally {
+ for (const socket of sockets) socket.destroy();
+ if (port !== 0) await close(server);
+ }
+ });As per path instructions for tests/**: "A behavior change in src/ should come with a focused regression test near the existing tests for that subsystem."
🤖 Prompt for 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.
In `@tests/local-management-direct-transport.test.ts` around lines 33 - 39, Extend
the directLocalHttpFetch tests with a mid-flight abort against a server that
accepts the connection and never responds, asserting the rejection preserves
AbortError; keep the existing pre-flight case. Add a focused checkProxyHealth
test that aborts during the pending request and asserts the result is "timed
out" rather than "unreachable", using the existing test helpers and cleanup
patterns.
Source: Path instructions
| test.each([ | ||
| ["content-length", (body: string) => `Content-Length: ${Buffer.byteLength(body)}\r\n\r\n${body}`], | ||
| ["chunked", (body: string) => `Transfer-Encoding: chunked\r\n\r\n${Buffer.byteLength(body).toString(16)}\r\n${body}\r\n0\r\n\r\n`], | ||
| ])("finishes a %s response without waiting for a keep-alive socket to close", async (_name, frame) => { | ||
| const sockets = new Set<Socket>(); | ||
| const body = JSON.stringify({ ok: true }); | ||
| const server = createTcpServer(socket => { | ||
| sockets.add(socket); | ||
| socket.once("close", () => sockets.delete(socket)); | ||
| socket.once("data", () => { | ||
| socket.write(`HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nConnection: keep-alive\r\n${frame(body)}`); | ||
| }); | ||
| }); | ||
| let port = 0; | ||
| try { | ||
| port = await listen(server); | ||
| const response = await directLocalHttpFetch(`http://127.0.0.1:${port}/healthz`, { | ||
| signal: AbortSignal.timeout(500), | ||
| }); | ||
| expect(await response.json()).toEqual({ ok: true }); | ||
| } finally { | ||
| for (const socket of sockets) socket.destroy(); | ||
| if (port !== 0) await close(server); | ||
| } | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Add negative framing cases. No test asserts that the transport fails closed on malformed responses.
This test proves the happy path for both framing modes, which is the highest-value assertion in the file. The failure direction has no coverage.
src/server/direct-local-http.ts throws at more than twenty sites, including the invalid status line (line 64), the unsupported status (line 66), the invalid header (line 70), the header byte cap (line 83), the invalid content length (line 95), the response byte cap (line 99), the invalid chunk size (line 119), the invalid chunk terminator (line 134), and the truncated body (line 178). None of these are exercised.
These are the security-relevant paths. This transport carries capability-bound reads, so the guarantee that matters is that a hostile process holding the port cannot feed a fabricated or truncated response that parses as valid. The createTcpServer harness in this test already supplies everything needed to assert it.
💚 Proposed negative framing cases
+ test.each([
+ ["an invalid status line", "NOT-HTTP 200 OK\r\nContent-Length: 0\r\n\r\n"],
+ ["an invalid header", "HTTP/1.1 200 OK\r\n: novalue\r\nContent-Length: 0\r\n\r\n"],
+ ["an invalid content length", "HTTP/1.1 200 OK\r\nContent-Length: abc\r\n\r\n"],
+ ["a truncated content-length body", "HTTP/1.1 200 OK\r\nContent-Length: 64\r\n\r\nshort"],
+ ["an invalid chunk size", "HTTP/1.1 200 OK\r\nTransfer-Encoding: chunked\r\n\r\nzz\r\nx\r\n"],
+ ])("rejects %s", async (_name, frame) => {
+ const sockets = new Set<Socket>();
+ const server = createTcpServer(socket => {
+ sockets.add(socket);
+ socket.once("close", () => sockets.delete(socket));
+ socket.once("data", () => {
+ socket.write(frame);
+ socket.end();
+ });
+ });
+ let port = 0;
+ try {
+ port = await listen(server);
+ await expect(directLocalHttpFetch(`http://127.0.0.1:${port}/healthz`, {
+ signal: AbortSignal.timeout(500),
+ })).rejects.toThrow(/direct local HTTP response/);
+ } finally {
+ for (const socket of sockets) socket.destroy();
+ if (port !== 0) await close(server);
+ }
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| test.each([ | |
| ["content-length", (body: string) => `Content-Length: ${Buffer.byteLength(body)}\r\n\r\n${body}`], | |
| ["chunked", (body: string) => `Transfer-Encoding: chunked\r\n\r\n${Buffer.byteLength(body).toString(16)}\r\n${body}\r\n0\r\n\r\n`], | |
| ])("finishes a %s response without waiting for a keep-alive socket to close", async (_name, frame) => { | |
| const sockets = new Set<Socket>(); | |
| const body = JSON.stringify({ ok: true }); | |
| const server = createTcpServer(socket => { | |
| sockets.add(socket); | |
| socket.once("close", () => sockets.delete(socket)); | |
| socket.once("data", () => { | |
| socket.write(`HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nConnection: keep-alive\r\n${frame(body)}`); | |
| }); | |
| }); | |
| let port = 0; | |
| try { | |
| port = await listen(server); | |
| const response = await directLocalHttpFetch(`http://127.0.0.1:${port}/healthz`, { | |
| signal: AbortSignal.timeout(500), | |
| }); | |
| expect(await response.json()).toEqual({ ok: true }); | |
| } finally { | |
| for (const socket of sockets) socket.destroy(); | |
| if (port !== 0) await close(server); | |
| } | |
| }); | |
| test.each([ | |
| ["content-length", (body: string) => `Content-Length: ${Buffer.byteLength(body)}\r\n\r\n${body}`], | |
| ["chunked", (body: string) => `Transfer-Encoding: chunked\r\n\r\n${Buffer.byteLength(body).toString(16)}\r\n${body}\r\n0\r\n\r\n`], | |
| ])("finishes a %s response without waiting for a keep-alive socket to close", async (_name, frame) => { | |
| const sockets = new Set<Socket>(); | |
| const body = JSON.stringify({ ok: true }); | |
| const server = createTcpServer(socket => { | |
| sockets.add(socket); | |
| socket.once("close", () => sockets.delete(socket)); | |
| socket.once("data", () => { | |
| socket.write(`HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nConnection: keep-alive\r\n${frame(body)}`); | |
| }); | |
| }); | |
| let port = 0; | |
| try { | |
| port = await listen(server); | |
| const response = await directLocalHttpFetch(`http://127.0.0.1:${port}/healthz`, { | |
| signal: AbortSignal.timeout(500), | |
| }); | |
| expect(await response.json()).toEqual({ ok: true }); | |
| } finally { | |
| for (const socket of sockets) socket.destroy(); | |
| if (port !== 0) await close(server); | |
| } | |
| }); | |
| test.each([ | |
| ["an invalid status line", "NOT-HTTP 200 OK\r\nContent-Length: 0\r\n\r\n"], | |
| ["an invalid header", "HTTP/1.1 200 OK\r\n: novalue\r\nContent-Length: 0\r\n\r\n"], | |
| ["an invalid content length", "HTTP/1.1 200 OK\r\nContent-Length: abc\r\n\r\n"], | |
| ["a truncated content-length body", "HTTP/1.1 200 OK\r\nContent-Length: 64\r\n\r\nshort"], | |
| ["an invalid chunk size", "HTTP/1.1 200 OK\r\nTransfer-Encoding: chunked\r\n\r\nzz\r\nx\r\n"], | |
| ])("rejects %s", async (_name, frame) => { | |
| const sockets = new Set<Socket>(); | |
| const server = createTcpServer(socket => { | |
| sockets.add(socket); | |
| socket.once("close", () => sockets.delete(socket)); | |
| socket.once("data", () => { | |
| socket.write(frame); | |
| socket.end(); | |
| }); | |
| }); | |
| let port = 0; | |
| try { | |
| port = await listen(server); | |
| await expect(directLocalHttpFetch(`http://127.0.0.1:${port}/healthz`, { | |
| signal: AbortSignal.timeout(500), | |
| })).rejects.toThrow(/direct local HTTP response/); | |
| } finally { | |
| for (const socket of sockets) socket.destroy(); | |
| if (port !== 0) await close(server); | |
| } | |
| }); |
🤖 Prompt for 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.
In `@tests/local-management-direct-transport.test.ts` around lines 41 - 65, Add
negative framing tests alongside the existing content-length and chunked cases,
using the createTcpServer harness and directLocalHttpFetch to send malformed
responses and assert rejection. Cover representative fail-closed paths in
directLocalHttpFetch, including invalid status or headers, invalid or oversized
content length, invalid chunk framing, and truncated bodies, while preserving
cleanup of sockets and the server.
Source: Path instructions
| const proxy = createServer((request, response) => { | ||
| const rawPath = request.url ?? "/"; | ||
| proxyPaths.push(rawPath); | ||
| const pathname = new URL(rawPath, "http://127.0.0.1").pathname; | ||
| if (pathname === "/__proxy-control") { | ||
| response.writeHead(200, { "content-type": "application/json" }); | ||
| response.end(JSON.stringify({ via: "proxy" })); | ||
| return; | ||
| } | ||
| // Return valid-looking data so the assertion detects routing, not parsing. | ||
| reply(rawPath, (status, body) => { | ||
| response.writeHead(status, { "content-type": "application/json" }); | ||
| response.end(JSON.stringify(body)); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Assert directly that the capability header never reached the proxy.
The PR's central security claim is that this transport prevents credential exposure through a forwarding proxy. This test proves it only by inference: line 195 asserts proxyPaths has length 1, so a capability read that went through the proxy would be caught as a second recorded path.
The proxy handler at lines 109-123 never inspects request headers, so nothing states the claim directly. If a later change makes the proxy handler forward or record differently, the length assertion could still pass while a token leaked.
Record the capability header on the proxy side and assert the list is empty. That converts the inference into the direct assertion.
💚 Proposed direct assertion
const proxyPaths: string[] = [];
+ const proxyCapabilities: string[] = [];
let targetPort = 0; const proxy = createServer((request, response) => {
const rawPath = request.url ?? "/";
proxyPaths.push(rawPath);
+ const leaked = request.headers["x-opencodex-local-capability"];
+ if (typeof leaked === "string") proxyCapabilities.push(leaked);
const pathname = new URL(rawPath, "http://127.0.0.1").pathname; expect(proxyPaths).toHaveLength(1);
expect(proxyPaths[0]).toEndWith("/__proxy-control");
+ // The capability token must never reach a forwarding proxy.
+ expect(proxyCapabilities).toEqual([]);
expect(targetPaths).toEqual(["/healthz", "/readyz", "/api/system/memory"]);As per path instructions for src/**: "tokens and OAuth material must never be logged or serialized into responses." This test is the guard for that property.
Also applies to: 195-199
🤖 Prompt for 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.
In `@tests/local-management-direct-transport.test.ts` around lines 109 - 123,
Update the proxy server callback in createServer to record any capability header
received, then directly assert after the request flow that the recorded
capability-header list is empty. Preserve the existing proxyPaths assertion and
response behavior, ensuring the test explicitly guards against credential
exposure rather than relying only on path-count inference.
Source: Path instructions
2 participants
Summary
ocx doctorand CLI account-health reads with short-lived, single-use local capabilitiesThe previous flow authenticated a
/healthzresponse and then sent a reusable management credential on a separate HTTP request. A transparent forwarding proxy could relay the health challenge to the genuine listener and observe that credential on the diagnostic request. The new flow never reads or transmits the reusable admin credential; it sends only an endpoint-scoped capability that expires within 10 seconds and is consumed once. These local probes connect directly to the selected listener, so an environment proxy cannot observe the capability or fabricate the diagnostic response.Legitimate local diagnostics remain available for the exact protected runtime record. Unattested, stale, or legacy runtime targets now return an honest unavailable result before any request is made.
Verification
bun x --package typescript@7.0.2 tsc --noEmit— passedbun run privacy:scan— passedAuthorization, and zerox-opencodex-api-keyheadersgit diff HEAD^ --check— passedbun test --isolate— 10,615 passed, 11 skipped, 176 failed, 13 errors across 674 files; changed management-auth and diagnostic paths passed, while unrelated existing Windows symlink/ACL/EBUSY cleanup, 5-second timeout, subprocess PATH, and fixture-state failures kept the full suite non-greenChecklist
Review readiness checklist
This PR stays in draft until every box below is ticked. Tick all four boxes once the requirements are met:
All CI tests are green on my local testing.
I pushed my PR to the latest dev commit.
I resolved all correct Codex and CodeRabbit findings.
My PR is ready for review.
Summary by CodeRabbit
New Features
Bug Fixes
ocx doctornow advises restarting the proxy when detailed diagnostics are unavailable.