fix(claude): bind proxy admission tokens to local destinations - #1360
Conversation
|
Important Review skippedNo new commits to review since the last review. ⚙️ 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:
📝 WalkthroughWalkthroughClaude proxy authentication now recognizes normalized loopback URLs, removes stale proxy markers, separates user credentials from admission secrets, and limits admission-token and host-assertion injection to eligible local proxy targets. ChangesClaude proxy authentication
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Claude
participant ProxyValidator
participant LocalProxy
participant ExternalGateway
Claude->>ProxyValidator: classify Anthropic base URL
ProxyValidator-->>Claude: local or external target
alt local proxy without user API key
Claude->>LocalProxy: inject admission token and host marker
else external gateway or user API key
Claude->>ExternalGateway: preserve user credential without host marker
end
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
|
✅ Deterministic PR hygiene checks passed. |
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request has been marked Ready for Review. |
|
Please rebase this PR onto the latest |
a2360b9 to
2a33dbc
Compare
![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/cli/claude.ts`:
- Around line 151-155: Update the authentication handling around the
proxy-marker injection and host-managed assertion to derive a
hostOwnsAuthentication flag requiring a local proxy target, no user API key, and
either PROXY_MARKER or a proxy admission secret. Use this flag when setting
CLAUDE_CODE_PROVIDER_MANAGED_BY_HOST instead of checking only for
ANTHROPIC_AUTH_TOKEN, and extend the external user-token coverage in
claude-auth-mode.test.ts to verify the host-managed assertion is absent.
- Around line 122-125: Update the stale URL replacement logging in the Claude
loopback handling to avoid logging raw existingBaseUrl; use parsed.origin or
omit the old destination while preserving the replacement message. Add a
regression test covering URL credentials and assert console.error does not
include them.
🪄 Autofix
❌ Autofix failed (check again to retry)
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: 3cf62c35-6b21-431e-a99b-5481002dcbd8
📒 Files selected for processing (2)
src/cli/claude.tstests/claude-auth-mode.test.ts
|
|
|
|
|
An unexpected error occurred while generating fixes: Not Found - https://docs.github.com/rest/git/refs#get-a-reference |
2a33dbc to
e6e1bff
Compare
Rate Limit Exceeded
|
Rate Limit Exceeded
|
Rate Limit Exceeded
|
Rate Limit Exceeded
|
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 `@src/cli/claude.ts`:
- Around line 122-125: Update the loopback URL replacement branch around
isClaudeLoopbackHostname so it compares the active proxy port with the URL’s
effective port, treating an empty parsed.port as HTTP port 80, consistent with
targetsLocalClaudeProxy. Replace stale default-port loopback URLs such as
http://localhost when the proxy uses another port, and add a regression case in
claude-auth-mode.test.ts covering http://localhost with port 10100.
🪄 Autofix
❌ Autofix failed (check again to retry)
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: aedef045-9efd-4dc6-872c-7482d139e823
📒 Files selected for processing (2)
src/cli/claude.tstests/claude-auth-mode.test.ts
|
An unexpected error occurred while generating fixes: Not Found - https://docs.github.com/rest/git/refs#get-a-reference |
e6e1bff to
c01de68
Compare
|
✅ Action performedReview finished.
|
|
|
|
Thanks @luvs01! Great work tightening the Claude credential boundary and covering the tricky loopback/default-port cases. Appreciate the thorough review follow-up. Merged. 🙏 |
2 participants
Summary
This keeps external gateway overrides supported without allowing OpenCodex-owned admission credentials or the proxy marker to follow them.
Verification
bun test tests/claude-auth-mode.test.ts tests/claude-cli.test.ts— 62 passed, 0 failed on Bun 1.3.14.bun run typecheck— passed.bun run privacy:scan— passed.git diff --check— passed.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