fix: harden credential and runtime trust boundaries (rebase of #916) - #936
Draft
lidge-jun wants to merge 4 commits into
Draft
fix: harden credential and runtime trust boundaries (rebase of #916)#936lidge-jun wants to merge 4 commits into
lidge-jun wants to merge 4 commits into
Conversation
Review of this branch rejected three things. Two are docs; this is the behavior one. With no trusted launcher context, the branch deleted all three ambient Anthropic slots. That breaks a documented entry point: `bun src/cli/index.ts` is supported (structure/01_runtime.md:9) and has no launcher context, so a user who exported ANTHROPIC_API_KEY in their shell simply loses it. The two slot classes are not symmetric. ANTHROPIC_BASE_URL stays fail-closed — a dotenv-only destination combined with subscription auth is exactly how Claude's OAuth bearer and prompt leave for a host the repository chose, and losing a legitimate custom destination costs a flag rather than an account. Credentials are preserved: the destination is already pinned by the time they are read, so stripping them defends against a project file that could equally well have supplied the key it is being blamed for. The test that mandated the old behavior is replaced by two: an ambient key survives without context, and an ambient base URL is still replaced. That pair is the contract. Also corrected two structure docs the review flagged as stale: the ci.yml row still described the pre-#899 hosted-Windows selector, and the runtime decision log recorded the rejected fail-closed-for-everything behavior.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
The security review rejected my narrowing, correctly. I had preserved ambient credentials when no launcher context exists, reasoning that the destination is pinned before they are read so a dotenv key would only reach the local proxy. That reasoning does not survive the subscription path: CLAUDE_CODE_PROVIDER_MANAGED_BY_HOST is only set when opencodex owns an auth token — asserting it otherwise logs a subscriber out (#253) — so Claude Code's settings.env merge can still replace ANTHROPIC_BASE_URL after buildClaudeEnv returns. A preserved key travels to that host. The repository documents the destination residual for subscription mode; preserving credentials would have widened it into a credential leak. So all three slots fail closed again without provenance. Direct `bun src/cli/index.ts` loses ambient Anthropic values, which is a real cost to a documented entry point; the escape hatch is the published `ocx` bin, where genuine shell exports survive by proof. The gap that let the bad revision pass: the suite tested no-context credential handling and settings-hijack separately, never combined. It does now — a no-context ambient key must be absent after the merge that hijacks the destination. Reintroducing the narrowing fails 6 tests.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rebase of #916 (@Ingwannu) onto current
dev, with the three defects a branch review found.Draft on purpose. This touches authentication, credential handling, GitHub workflows, and the durable launcher —
MAINTAINERS.mdrequires explicit security review for exactly that surface. The review below is an audit, not that review, and an agent does not substitute for it. Do not merge on my say-so.The defects are real on current
devConfirmed by direct probe rather than by reading the PR description:
src/adapters/google.tsbuilds the request host fromprovider.locationwith no validation, then attaches an ADC bearer. A location ofattacker.example:443/capture#yields authorityattacker.example— I reproduced this againstdev.overrideBunPath()rereadsOPENCODEX_BUN_PATHafter Bun has loaded project dotenv, so everydurableBunRuntime()caller — service, shim, WinSW, tray — can persist a repository-chosen executable.ANTHROPIC_BASE_URLsurvived credential stripping and reached Claude, which can redirect an OAuth-bearing request.src/oauth/health.tsattached the admin token after only a forgeable/healthzidentity check.None of these were superseded by #917. That PR resolved the inbound management principal — who may call the star route. This is the outbound listener: whether the thing answering on the port deserves the token. Different boundary.
What the review changed
src/cli/claude.tsno-context fallback — narrowed. As submitted, a launch with no trusted launcher context deleted all three ambient Anthropic slots.bun src/cli/index.tsis a documented entry point (structure/01_runtime.md:9) and has no launcher context, so a user with a shell-exportedANTHROPIC_API_KEYsimply lost it.The two slot classes are not symmetric.
ANTHROPIC_BASE_URLstays fail-closed — a dotenv-only destination plus subscription auth is precisely how the OAuth bearer leaves for a repository-chosen host, and losing a legitimate custom destination costs a flag rather than an account. Credentials are preserved: the destination is already pinned by the time they are read, so stripping them defends against a project file that could equally well have supplied the key being blamed. The test that mandated the old behavior is replaced by two that pin the asymmetry.structure/06_docs-and-release.md— theci.ymlrow still described the pre-#899 hosted-Windows selector. Rewritten against the real job graph: four Linux shards plusgates, full macOS, Windows only at the shipping boundary, and the aggregatecijob asserting Windows actually succeeded there.structure/01_runtime.mddecision log — recorded the rejected fail-closed-for-everything behavior; now describes the split.Rebase
One conflict,
src/server/index.ts, resolved keeping both #917'smanagementPrincipal()dispatch and this branch's attestation imports.Evidence
bun run test: 7602 pass / 0 fail across 505 filesbun x tsc --noEmitexit 0,bun run privacy:scanpassedAuthorship on the original commits is preserved. @Ingwannu — the substance is yours; the three changes above are the review response, and I would rather you disagree with them here than have them land silently.
Security review round — my narrowing was wrong
The adversarial review returned FAIL on the one change I made to @Ingwannu's behavior, and it was right.
I had preserved ambient credentials when no launcher context exists, reasoning that the destination is pinned before they are read, so a dotenv-supplied key could only ever reach the local proxy. That reasoning does not survive the subscription path.
CLAUDE_CODE_PROVIDER_MANAGED_BY_HOSTis only set when opencodex owns an auth token — asserting it otherwise logs a subscriber out (#253) — so Claude Code'ssettings.envmerge can still replaceANTHROPIC_BASE_URLafterbuildClaudeEnvreturns. A preserved key travels to that host.This repository already documents the destination residual for subscription mode, with a test asserting it so it cannot drift into an assumed guarantee. Preserving credentials would have quietly widened that documented residual into a credential leak.
So all three slots fail closed again without provenance, exactly as @Ingwannu submitted it. Direct
bun src/cli/index.tsloses ambient Anthropic values — a real cost to a documented entry point, and the honest trade. The escape hatch is running through the publishedocxbin, where genuine shell exports survive by proof.The gap that let my bad revision pass: the suite tested no-context credential handling and settings-hijack separately, never combined. It does now — a no-context ambient key must be absent after the merge that hijacks the destination. Reintroducing the narrowing fails 6 tests.
The review also cleared the rest: the Vertex validator is a strict lowercase 1–63 byte DNS label rejecting dots, Unicode, percent-encoding, fragments, paths, uppercase and overlong values while accepting every legitimate location, enforced on both the adapter and the management write path; local attestation binds a fresh 256-bit challenge plus PID and port, fails closed on malformed proofs, and compares timing-safely; the Bun launcher stamps before Bun starts on every documented invocation including the Windows
.cmdshim; and the rebase keptmanagementPrincipal()with the correcthandleManagementAPIarity.Updated:
bun run test7602 pass / 0 fail across 505 files, typecheck exit 0, privacy scan passed.Still draft, still needs maintainer security review. Two rounds of adversarial audit are not that review.