fix(oauth): attest credential-free live provider reloads - #1541
Conversation
github-actions
Bot
added
the
intake: hygiene-blocked
|
⏳ DRAFT
What to do
Review readiness checklist
0/4 boxes ticked. Automatic draft conversion failed (token cannot change draft status). Please convert this pull request to a draft manually. The required |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (19)
📝 WalkthroughWalkthroughOAuth and API-key login now save credentials to disk, then attempt an attested, process-bound reload in a running proxy. The proxy validates the request, rereads the provider, updates live state, refreshes related state, and reports failures to the CLI. ChangesLocal provider reload
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant LoginCLI
participant ProxyHealth
participant ManagementAuth
participant ProviderRoute
participant LiveConfig
LoginCLI->>ProxyHealth: Attest PID, port, nonce, and capability version
ProxyHealth-->>LoginCLI: Return runtime proof
LoginCLI->>ManagementAuth: Send bodyless provider reload capability
ManagementAuth->>ProviderRoute: Admit single-use request
ProviderRoute->>LiveConfig: Adopt validated persisted provider
LiveConfig-->>ProviderRoute: Refresh live state
ProviderRoute-->>LoginCLI: Return reload outcome
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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: 4be2c6dced
ℹ️ 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".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/oauth-login-cli-live-update.test.ts`:
- Around line 61-75: Update the test handler around notifyRunningProxy to
increment a dedicated counter when url.pathname is "/healthz". After awaiting
notifyRunningProxy, assert that the health-probe counter is greater than zero,
then retain the existing receivedPaths empty assertion to verify the management
POST remains blocked.
🪄 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: 899560d5-1817-4349-82b6-ceb4ad0bca10
📒 Files selected for processing (2)
src/oauth/login-cli.tstests/oauth-login-cli-live-update.test.ts
4be2c6d to
a11d672
Compare
luvs01
changed the title
|
Exact-head handoff for |
a11d672 to
de921ff
Compare
|
Exact-head update for de921ff on current dev c125b61: the rebase is conflict-free, the exact post-rebase capability and reload contracts pass on Bun 1.3.14 and Bun 1.4, TypeScript checks and privacy scans pass on both runtimes, diff-check is clean, and all review threads remain resolved. The full local suite was not duplicated because GitHub CI owns that workload. The remaining unsponsored_surface result is the expected authentication-surface gate; please review this exact head for maintainer-sponsored and approve exact-head GitHub Actions when appropriate. |
Wibias
left a comment
Wibias
left a comment
There was a problem hiding this comment.
Deep security + normal review completed on de921ff8213e5e69797b0eaa9861cc8e6f1e733f.
I found no actionable correctness or security finding in the current diff.
Security paths reviewed end to end:
- CLI -> protected runtime record -> fresh
/healthzHMAC attestation -> bodyless reload request - domain-separated HMAC capability binding to provider name, method, exact path, nonce, PID, port, and 10 s expiry
- one-shot replay protection and exact body/path/query checks in management auth
- listener replacement/runtime-record recheck between proof and POST
- direct TCP transport, including proxy-header stripping, no environment proxy, bounded response framing, and no redirect path
- management principal propagation into the exact reload route
- disk snapshot authority, same-buffer SHA-256, post-DNS exact-byte recheck under the config mutation lock, and named-row-only adoption
- provider destination validation, namespace collision checks, live-baseline preservation, and cache/state invalidation
- credential exposure: provider/API/OAuth values and the reusable management token no longer cross this reload socket
I also checked the previous P1 listener-impersonation finding. The current design removes that credential-bearing attack path rather than merely adding another listener identity heuristic.
Residual assumptions I did not treat as new findings: compromise by another process running with the same protected config/runtime-state access, and the repository's existing provider-DNS rebinding/config-time resolution semantics.
I am not approving yet for process reasons only. This touches shared OAuth/config/auth/server code, and the repository guidance requires the full test suite before approval. The PR explicitly leaves the repository-wide suite to GitHub CI, while the current Cross-platform CI and React Doctor runs are still action_required. Once the required full CI has actually run green on this exact head, I have no code/security blocker.
The reload result was discarded and login printed unconditional success. A CLI talking to a proxy that predates attested reload would persist the credential and report success while the running process kept routing with the old one, with nothing telling the operator a restart was needed. Return the outcome from notifyRunningProxy and distinguish the two null cases (nothing listening, or a provider outside the live-reload allowlist) from a real failure, warn on failure after the success line, and document the restart requirement.
Review: LAND (one Medium fixed on top)Threat-model reviewed against the current tree with an independent reviewer, then verified on a Linux CI host. The contributor commit is preserved; my fix is a separate commit on top. What holds upThe capability is minted over The 10-second window uses The config-digest recheck is a real bounded TOCTOU fix, not a decorative one: the digest is computed over the same buffer that is subsequently parsed and adopted under the mutation lock, so there is no re-read gap. A non-cooperating filesystem editor can still write immediately after the final read, but no in-process lock can prevent that and the codebase already acknowledges the limit. The consumed-capability map is bounded at 256 entries with expired entries pruned on valid admission, so it is not a memory-growth vector. One thing the description overstatesPID and port binding sounds stronger than it is. Both are observable or guessable; neither adds entropy. The security root is possession of the per-process attestation secret, and the nonce supplies uniqueness rather than secrecy. PID/port are useful defense-in-depth against stale runtime records and endpoint replacement, not against a leaked secret. Worth stating plainly so nobody later relies on them as authentication. The Medium I fixed
Now the outcome is returned and surfaced, with the two harmless cases (nothing listening, provider outside the live-reload allowlist) kept distinct from a real failure so they do not produce noise: The restart requirement is now documented in A note on my own fix, because it is a good trap. My first attempt wrote VerificationLinux CI host, Bun 1.3.14, at the pushed head
The regression above was confirmed red before the fix (1 fail) and green after. |
…reload call #1541 (fb4f2fe) added a seventh 'await convergeCodexCatalog()' to provider-routes.ts in the new attested credential-free provider-reload route, but the inventory contract still expected 6, leaving dev CI red for every open PR. Update the expected count and test title to match. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ovider-reload test(codex-convergence): update route inventory for #1541's attested reload call
3 participants
Summary
POST /api/providers/reloadoperationconfig.jsonsnapshot without sending provider credentials or a reusable management tokenTest plan
/healthzcapability contract test passed on Bun 1.3.14 and Bun 1.4.0-canary.1tsc --noEmitpassed on Bun 1.3.14 and Bun 1.4.0-canary.1bun run privacy:scanpassed on both runtimes after rebasing to currentdevgit diff --checkpassedFocused dual-runtime validation covers the capability contract, replay and TTL bounds, direct bodyless POST framing, attestation failure, runtime-record replacement, named-row-only live adoption, disk-change rejection during destination validation, OAuth/key-login live convergence, unchanged disk bytes, and config-baseline preservation. The repository-wide suite is left to GitHub CI rather than duplicated locally.
Checklist
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
Documentation