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 identity 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
|
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request is already Ready for Review. |
|
@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".
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
Wibias
left a comment
Wibias
left a comment
There was a problem hiding this comment.
Re-review: changes are still required.
The current P1 in the direct loopback transport is blocking: forcing localhost to 127.0.0.1 breaks installations where the listener resolved/bound to ::1, so liveness, readiness, status, and capability reads can falsely report a healthy runtime as unavailable. Keep the proxy-bypass transport, but preserve IPv6 localhost resolution or try both loopback families.
The current parser-differential and transport-hardening threads should also be addressed, especially consolidating response-head parsing and directly testing that capability headers never reach a forwarding proxy. The branch is now also not mergeable against current dev; resolve that before re-review.
37055d5 to
6af1bf4
Compare
6af1bf4 to
14133eb
Compare
Wibias
left a comment
Wibias
left a comment
There was a problem hiding this comment.
Re-review on exact head f3f53f7cb93c61682c899ec761d1147ff98a2c72, including a fresh security pass over the local-management capability, direct transport, management auth, OAuth health, liveness/readiness, and the final fixture corrections.
I do not find a remaining code/security blocker on this head. The previous P1 IPv6 issue is fixed by preserving localhost and using autoSelectFamily; the capability-bearing path now bypasses HTTP(S) proxy environment handling through node:net; response-head parsing is shared; direct requests are bounded by timeout/byte/framing checks; and the proxy-leak, abort, malformed-framing, and IPv4/IPv6 regressions are covered.
The local-read capability itself is narrowly scoped and fails closed: HMAC domain + nonce + exact GET + one of two allowlisted paths + PID + listener port + short expiry, constant-time verification, no query inheritance, and server-side single-use replay tracking. requireManagementAuth and managementPrincipal only preserve the already-admitted same Request via the WeakSet. OAuth health no longer sends the reusable admin credential and only mints a capability after re-reading a runtime record whose PID/port match the live runtime target. I do not see an authZ escalation or reusable-credential exfiltration path here.
One design nuance is that the capability authenticates the request to the server rather than cryptographically authenticating the response back to the CLI. A foreign listener could fabricate diagnostic data, but it receives only a short-lived endpoint/PID/port-bound one-time capability, not a reusable management secret, and cannot redirect that capability to a different genuine listener. I do not consider that a blocker for this change's threat model.
The earlier doctor-oauth fixture failures are also addressed on this head by giving those test fixtures the runtime PID/source/attestation evidence the new production contract requires.
I am still requesting changes for integration/readiness only. Current dev is 4a299fa09352e9f3d5dd119f0d892f92d3d0bab1; this head is 3 commits ahead and 2 behind with merge base 70d2e1758c180188a729dd63812703b76cfeeba6. Exact-head Cross-platform CI (31566691608) and React Doctor (31566691632) are both action_required and each has 0 jobs, so GitHub CI has not executed this head. The PR also remains draft with the full-CI/readiness checklist unchecked.
Please rebase onto current dev, obtain executed exact-head CI, and request re-review. No additional feature/security change is requested from me at this point.
Wibias
added
the
maintainer-sponsored
github-actions
Bot
added
review-ready
and removed
intake: hygiene-blocked
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
|
Tip For best results, initiate chat on the files or code changes.
The PR has the 🧠 Learnings usedYou are interacting with an AI system. |
|
Tip For best results, initiate chat on the files or code changes.
🧠 Learnings usedYou are interacting with an AI system. |
Wibias
left a comment
Wibias
left a comment
There was a problem hiding this comment.
Security-only deep revalidation on exact head f3f53f7cb93c61682c899ec761d1147ff98a2c72.
I re-ran the security reasoning across the full changed boundary: capability construction/verification, management admission, replay handling, direct transport, response framing, CLI doctor/status consumers, OAuth health reads, IPv4/IPv6 resolution, proxy bypass, stale runtime/PID behavior, and cross-route/cross-capability confusion.
I found no remaining reportable P0-P3 security finding on this head.
- The local-read HMAC is domain-separated and bound to exact
GET, one of two allowlisted paths, nonce, runtime PID, listener port, and short expiry. Verification rejects malformed/future-overlong capabilities and uses constant-time comparison. - Server admission additionally requires a queryless allowlisted path and the exact current PID, consumes a valid capability once, and keeps replay state bounded. The
WeakSet<Request>only preserves the same admitted Request acrossrequireManagementAuth->managementPrincipal; it does not make the capability reusable on another request. - The local-read principal cannot authorize arbitrary
/api/*routes and cannot cross into the separate restart-capability contract. - Production capability reads no longer transport the reusable admin token. They use the direct
node:nettransport, so HTTP(S)/ALL proxy environment variables cannot observe the capability or fabricate the response through a forwarding proxy. - The direct transport is bounded by byte caps and socket/request timeouts, shares response-head parsing, rejects malformed/truncated framing, strips proxy-specific headers, and preserves localhost IPv4/IPv6 resolution with
autoSelectFamily. - OAuth health and doctor memory reads fail closed on unattested/runtime-mismatched targets; the memory response is also checked against the attested target PID.
- I also checked deliberately non-loopback configured bindings: the capability remains only a short-lived, single-use authorization for the selected listener and one exact read. It does not become a reusable management credential or widen route authorization; confidentiality of an explicitly non-loopback plain-HTTP deployment remains the pre-existing deployment boundary.
So the current blocker remains integration/readiness rather than security: the branch still needs rebasing onto current dev and an executed exact-head CI signal.
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/cli/doctor.ts`:
- Around line 615-623: Update fetchServiceMemory so its timeout contract is
explicit: either omit timeoutMs from the accepted LocalManagementReadDeps type
for this function, or preserve a caller-provided timeout while defaulting to
SERVICE_MEMORY_TIMEOUT_MS. Ensure the implementation and function signature
consistently reflect the chosen behavior.
In `@structure/05_gui-and-management-api.md`:
- Around line 26-31: Update the documentation paragraph describing local-read
HMAC capabilities to remove `ocx status` from the claim, since its
`src/cli/status.ts` flow uses `findLiveProxy()` and `/healthz` via
`directLocalHttpFetch`; limit the described capability-based collection to `ocx
doctor` and OAuth health unless status is changed to use
`fetchBoundLocalManagementRead`.
🪄 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: b1d43859-2295-4ae7-abff-91b280a77fee
📒 Files selected for processing (16)
src/cli/doctor.tssrc/cli/status.tssrc/lib/local-management-capability.tssrc/oauth/health.tssrc/server/direct-local-http.tssrc/server/local-management-read-client.tssrc/server/management-auth.tssrc/server/proxy-liveness.tsstructure/05_gui-and-management-api.mdtests/cli-status-json.test.tstests/doctor-oauth.test.tstests/doctor.test.tstests/local-management-capability.test.tstests/local-management-direct-transport.test.tstests/oauth-health.test.tstests/server-management-auth.test.ts
| export async function fetchServiceMemory( | ||
| host: string, | ||
| port: number, | ||
| token: string | null, | ||
| fetchImpl: typeof fetch = fetch, | ||
| target: LiveProxy, | ||
| deps: LocalManagementReadDeps = {}, | ||
| ): Promise<ServiceMemoryReport> { | ||
| try { | ||
| const res = await fetchImpl(`http://${host}:${port}/api/system/memory`, { | ||
| headers: token ? { "x-opencodex-api-key": token } : {}, | ||
| signal: AbortSignal.timeout(SERVICE_MEMORY_TIMEOUT_MS), | ||
| const read = await fetchBoundLocalManagementRead(target, LOCAL_MANAGEMENT_READ_PATHS.systemMemory, { | ||
| ...deps, | ||
| timeoutMs: SERVICE_MEMORY_TIMEOUT_MS, | ||
| }); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
fetchServiceMemory advertises a timeoutMs option that it discards.
The parameter type is LocalManagementReadDeps, which declares timeoutMs?: number. At lines 620-623 the spread ...deps is applied first, then timeoutMs: SERVICE_MEMORY_TIMEOUT_MS overwrites it. A caller that passes timeoutMs therefore gets silent no-op behavior, and the type signature does not communicate that. Make the contract explicit in one of two ways.
♻️ Option 1 (preferred): remove the knob from the accepted type
export async function fetchServiceMemory(
target: LiveProxy,
- deps: LocalManagementReadDeps = {},
+ deps: Omit<LocalManagementReadDeps, "timeoutMs"> = {},
): Promise<ServiceMemoryReport> {♻️ Option 2: honor a caller-supplied timeout with the doctor default
const read = await fetchBoundLocalManagementRead(target, LOCAL_MANAGEMENT_READ_PATHS.systemMemory, {
...deps,
- timeoutMs: SERVICE_MEMORY_TIMEOUT_MS,
+ timeoutMs: deps.timeoutMs ?? SERVICE_MEMORY_TIMEOUT_MS,
});📝 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.
| export async function fetchServiceMemory( | |
| host: string, | |
| port: number, | |
| token: string | null, | |
| fetchImpl: typeof fetch = fetch, | |
| target: LiveProxy, | |
| deps: LocalManagementReadDeps = {}, | |
| ): Promise<ServiceMemoryReport> { | |
| try { | |
| const res = await fetchImpl(`http://${host}:${port}/api/system/memory`, { | |
| headers: token ? { "x-opencodex-api-key": token } : {}, | |
| signal: AbortSignal.timeout(SERVICE_MEMORY_TIMEOUT_MS), | |
| const read = await fetchBoundLocalManagementRead(target, LOCAL_MANAGEMENT_READ_PATHS.systemMemory, { | |
| ...deps, | |
| timeoutMs: SERVICE_MEMORY_TIMEOUT_MS, | |
| }); | |
| export async function fetchServiceMemory( | |
| target: LiveProxy, | |
| deps: Omit<LocalManagementReadDeps, "timeoutMs"> = {}, | |
| ): Promise<ServiceMemoryReport> { | |
| try { | |
| const read = await fetchBoundLocalManagementRead(target, LOCAL_MANAGEMENT_READ_PATHS.systemMemory, { | |
| ...deps, | |
| timeoutMs: SERVICE_MEMORY_TIMEOUT_MS, | |
| }); |
🤖 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/cli/doctor.ts` around lines 615 - 623, Update fetchServiceMemory so its
timeout contract is explicit: either omit timeoutMs from the accepted
LocalManagementReadDeps type for this function, or preserve a caller-provided
timeout while defaulting to SERVICE_MEMORY_TIMEOUT_MS. Ensure the implementation
and function signature consistently reflect the chosen behavior.
| CLI health collection follows the same boundary without transporting the reusable management | ||
| credential. Its local-read HMAC capability is an additional single-use, route-scoped admission | ||
| mechanism, not a reusable credential class. `ocx status` and `ocx doctor` derive these 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 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Determine whether ocx status derives local-read capabilities.
set -euo pipefail
fd -t f 'status.ts' -p src/cli -x cat -n {}
# Find every caller of the bound local-management read.
rg -nP -C3 '\bfetchBoundLocalManagementRead\s*\(' --type=ts srcRepository: lidge-jun/opencodex
Length of output: 15936
Remove ocx status from the capability claim.
src/cli/status.ts uses findLiveProxy() and /healthz probes through directLocalHttpFetch; it does not call fetchBoundLocalManagementRead. Limit this statement to ocx doctor and OAuth health, or document the capability path if ocx status must use it.
🤖 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 - 31, Update the
documentation paragraph describing local-read HMAC capabilities to remove `ocx
status` from the claim, since its `src/cli/status.ts` flow uses
`findLiveProxy()` and `/healthz` via `directLocalHttpFetch`; limit the described
capability-based collection to `ocx doctor` and OAuth health unless status is
changed to use `fetchBoundLocalManagementRead`.
|
Thank you @luvs01 for the work on this PR and for iterating through the security review feedback. This is useful because local diagnostics no longer need to send a reusable management credential after a separate health probe. |
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— passedtsc --noEmitandgit diff --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.