fix: harden credential and runtime trust boundaries

[Ingwannu]

Summary

• validate Vertex location inputs before acquiring or routing ADC credentials
• authenticate pre-Bun runtime and Claude environment provenance before preserving executable or destination overrides
• attest the local management listener before CLI status attaches the admin credential
• preserve the trusted-event Windows self-hosted stability path while making the repository-level outside-contributor approval policy the explicit security boundary

Why

Several project-controlled or unauthenticated local inputs could cross credential and executable trust boundaries. This change validates or authenticates those inputs at the earliest boundary and fails closed when provenance cannot be established.

The initial draft removed the Windows self-hosted selector too broadly. This revision restores that recently-added Bun stability path: ordinary pull requests select windows-latest, while trusted push/workflow_dispatch runs can select ocx-home when the operational switch and runner are present. The selector itself is not treated as a security control because a PR owns its workflow body. The live repository setting was corrected from first_time_contributors to all_external_contributors, so every external fork workflow requires maintainer approval before any job can be assigned; the maintenance documentation now records that invariant and the fail-safe procedure.

The PATH-resolved gh sidebar report was reviewed and excluded as a false positive under the existing local-user trust model; this PR contains no unrelated sidebar change.

Validation

• bun run typecheck
• focused security and regression suite: 445 passed, 0 failed
• CI workflow policy suite: 66 passed, 0 failed
• bun run lint:gui
• bun run doctor:gui:if-changed (no GUI changes; skipped as designed)
• bun run privacy:scan
• node --check bin/ocx.mjs
• git diff --check
• exploit-style verification for each fixed credential/runtime boundary

The full repository run reached 7,505 passing tests. Two expectations affected by this patch were updated and now pass. One combo-failover teardown timeout remains; the exact isolated test reproduces unchanged on the unmodified dev checkout under the same single-CPU test constraint.

Review notes

This touches authentication, credential handling, GitHub Actions policy, and durable launchers. Please apply explicit maintainer security review before merge. The PR is intentionally draft and targets dev.

[coderabbitai]

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 93bfc304-65ad-4183-aa20-7512543d5e95

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

---

_{Comment @coderabbitai help to get the list of available commands.}

[lidge-jun]

Audited this against current dev with a dedicated security-focused reviewer. Summary: five hunks worth keeping, three that must not land as written, and it stays draft until a human security review — not because the work is weak, but because MAINTAINERS.md requires one for this surface and an agent audit does not substitute.

It is not superseded by #917

That was my working assumption going in, and it was wrong. #917 resolved the inbound management principal — who may call the star route. This authenticates the CLI's outbound management listener — whether the thing answering on the port deserves the admin token. Different boundary. The reviewer confirmed the outbound path still misbehaves on current dev after #917 landed.

Keep

1. Vertex location validation — the new single-label validator, checked in src/adapters/google.ts before ADC acquisition and again at the management write boundary. global, us, us-central1 still pass; anything that changes the request authority does not.
2. Durable Bun runtime provenance — accepting a recorded runtime only when the path names process.execPath, and removing the Bun-side override reselection. Documented ocx service install overrides keep working because bin/ocx.mjs selects and stamps before Bun starts.
3. Local management listener attestation — the challenge/PID/port HMAC, the runtime secret in src/config.ts, the attested /healthz, and the listener challenge in src/oauth/health.ts.
4. Proof-bound Claude destination provenance — capturing destination and token slots in the Node launcher before Bun loads dotenv, bound to a random argv proof. Narrowed; see below.
5. The corrected CI security-boundary comment and the external approval-policy section — the reviewer verified the latter against the live API.

Do not land as written

src/cli/claude.ts, the no-context ?? [] fallback. With no launcher context this strips genuine shell-exported API credentials and redirects to the proxy marker. Direct bun src/cli/index.ts is a documented entry path (structure/01_runtime.md:9), so a user who exported a real key in their shell loses it. Fail-closed is right for the OAuth-bearing subscription destination and wrong for credentials of unknown provenance. tests/claude-auth-mode.test.ts currently encodes the regression as intended behavior and needs rewriting with the fix.

structure/06_docs-and-release.md, the workflow table. Written against the pre-#899 workflow — it says the Bun test job keeps PRs on hosted Windows. Current reality is four Linux shards, full macOS, Windows only on main/preview push or dispatch, and an aggregate ci gate. Rewrite rather than carry forward.

structure/01_runtime.md, the decision log. The threat description and the launcher-proof choice are accurate; the paragraph recording direct-Bun fail-closed behavior documents the regression above. Revise alongside the fix.

Conflict

Exactly one file: src/server/index.ts. #917 changed the management-auth import and the /api dispatch; this changes the adjacent import/start//healthz region. The resolution has to retain managementPrincipal() and the five-argument handleManagementAPI(...) call while adding the attestation imports, the startServer secret argument, and the proof response. .github/workflows/ci.yml and src/server/auth-cors.ts auto-merge.

What I am not doing

Not rebasing this for you, and not marking it ready. The four defects it closes are real and reproduced, but they touch authentication, credential handling, and the durable launcher — the exact surface MAINTAINERS.md gates behind explicit security review. Handing an agent-audited security change straight to merge would set a bad precedent regardless of how careful the audit was.

Full disposition, including the per-hunk table, is in devlog/_plan/260803_pr_issue_sweep/050_phase5_916_disposition.md on dev.

Thanks @Ingwannu — the listener attestation in particular is a hole nobody else had noticed.

[lidge-jun]

Rebased onto current dev as #936, with three review findings folded in. Recording the disposition here so you can see what was kept and argue with what was not.

The four defects are real. I confirmed them by probe against dev rather than taking the description's word for it — the Vertex one in particular: a location of attacker.example:443/capture# yields request authority attacker.example with an ADC bearer attached. And none of them were superseded by #917, which I had assumed going in. #917 resolved the inbound management principal; yours is the outbound listener. Different boundary, no overlap in effect.

Kept, all five hunks: Vertex location validation at both the adapter and the management write boundary; durable Bun runtime provenance with the Bun-side override reselection removed; local management listener attestation; proof-bound Claude destination provenance; and the corrected CI security-boundary comment plus the external approval-policy section.

Changed — src/cli/claude.ts no-context fallback. As submitted, a launch with no trusted launcher context deleted all three ambient Anthropic slots. bun src/cli/index.ts is a documented entry point (structure/01_runtime.md:9) and has no launcher context, so a user with a shell-exported ANTHROPIC_API_KEY lost it.

I narrowed it rather than dropping it, because the two slot classes are not symmetric. ANTHROPIC_BASE_URL stays fail-closed — that is the actual attack, and losing a legitimate custom destination costs a flag rather than an account. Credentials are preserved: by the time they are read the destination is already pinned, so stripping them defends against a project file that could equally well have supplied the key being blamed for. tests/claude-auth-mode.test.ts encoded the old behavior as intended; it is replaced by two tests pinning the asymmetry.

Changed — two structure docs. 06_docs-and-release.md still described the pre-#899 hosted-Windows selector, and 01_runtime.md's decision log recorded the fail-closed-for-everything behavior. Both now match what ships.

Not merged, and not by my decision. #936 stays draft pending explicit maintainer security review. MAINTAINERS.md requires it for authentication, credential handling, workflow, and release surfaces, and this touches all four. An audit is not that review.

Your authorship is preserved on the original commits. If you disagree with the claude.ts narrowing, that is the one worth arguing — I may have the threat model wrong, and I would rather hear it before it lands than after.

[lidge-jun]

Correction to my previous comment: the claude.ts narrowing I described was wrong, and the security review caught it.

I had preserved ambient credentials with no launcher context, on the reasoning that the destination is pinned before they are read. That breaks on the subscription path — CLAUDE_CODE_PROVIDER_MANAGED_BY_HOST is only set when we own an auth token (#253), so a settings.env merge can still replace ANTHROPIC_BASE_URL after buildClaudeEnv returns, and a preserved key would travel with it. Your original fail-closed behavior is restored in #936, unchanged from what you submitted.

The suite had tested no-context credential handling and settings-hijack separately but never combined, which is why my revision passed. That combined case is now pinned: reintroducing the narrowing fails 6 tests.

So the only remaining changes to your work are the two stale structure docs. The behavior is yours as written.

[lidge-jun]

Closing in favour of #936, which is this branch rebased onto current dev — not a rejection.

All four defects you found are real; I confirmed each by probe rather than by reading the description. The Vertex one is the clearest: a location of attacker.example:443/capture# produces request authority attacker.example with an ADC bearer attached. And my initial assumption that #917 had superseded this was wrong — #917 resolved the inbound management principal, yours authenticates the outbound listener. Different boundary.

All five hunks are kept in #936, and your authorship is intact on the commits.

The one behavior change I made to your work, I then reverted. I had narrowed the no-context fallback to preserve ambient credentials, reasoning that the destination is pinned before they are read. A security review showed that fails on the subscription path: CLAUDE_CODE_PROVIDER_MANAGED_BY_HOST is only set when opencodex owns an auth token (#253 — asserting it otherwise logs a subscriber out), so Claude Code's settings.env merge can still replace ANTHROPIC_BASE_URL after buildClaudeEnv returns, and a preserved key would travel to that host. Your fail-closed behavior is what ships.

What let my revision through: the suite tested no-context credential handling and settings-hijack separately, never combined. That combined case is now pinned — reintroducing the narrowing fails 6 tests.

So the only remaining edits to your work in #936 are two stale structure docs: the ci.yml row still described the pre-#899 hosted-Windows selector, and the runtime decision log recorded the behavior I have since reverted.

#936 stays draft pending explicit maintainer security review. MAINTAINERS.md requires it for authentication, credential handling, workflow, and release surfaces, and this touches all four — two rounds of adversarial audit are not that review. Please follow #936 and push back there if you disagree with anything I changed.