fix(bedrock): tolerate a missing Connection header when signing#1
fix(bedrock): tolerate a missing Connection header when signing#1EffortlessSteven wants to merge 1 commit into
Conversation
|
Warning Review limit reached
More reviews will be available in 5 minutes and 45 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR fixes AWS SigV4 signing in Bedrock authentication by filtering out the ChangesConnection Header Filtering in Bedrock Auth
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Code Review
This pull request safely removes the "connection" header from the request headers before signing to prevent a potential KeyError when the header is missing, and adds comprehensive unit tests to verify this behavior. The reviewer suggested a cleaner and safer approach using headers.copy() and .pop("connection", None) to preserve the httpx.Headers type and correctly handle multi-value headers.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
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/lib/test_bedrock_auth.py`:
- Around line 29-45: The test test_strips_connection_header_from_signing only
checks lowercase "connection"; update it to also include a mixed-case header
(e.g., "Connection" or "ConNeCtIoN") when calling get_auth_headers so the
header-stripping behavior is verified case-insensitively, then assert (using the
existing signed = re.search(...) and assert "connection" not in
signed.group(1).split(";")) that the connection header is not present in the
SignedHeaders regardless of its case; keep using the same call-site and
Authorization parsing logic so the additional assertion uses the same signed
variable or a second equivalent extraction.
🪄 Autofix (Beta)
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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f59960c4-29d1-4b80-8927-55e039d0de81
📒 Files selected for processing (2)
src/anthropic/lib/bedrock/_auth.pytests/lib/test_bedrock_auth.py
📜 Review details
🧰 Additional context used
🪛 OpenGrep (1.22.0)
tests/lib/test_bedrock_auth.py
[WARNING] 9-9: Hardcoded AWS access key detected. Use environment variables or a secrets manager instead.
(coderabbit.secrets.aws-access-key)
🪛 Ruff (0.15.15)
tests/lib/test_bedrock_auth.py
[error] 10-10: Possible hardcoded password assigned to: "_SECRET_KEY"
(S105)
🔇 Additional comments (2)
src/anthropic/lib/bedrock/_auth.py (1)
59-61: LGTM!tests/lib/test_bedrock_auth.py (1)
14-27: LGTM!Also applies to: 46-64
0c89142 to
67a50df
Compare
get_auth_headers stripped the Connection header from the signed set via del headers["connection"], which raises KeyError on httpx.Headers when the header is absent (e.g. a custom http_client that omits or strips it). Filter the header out instead, matching the robust form already used in lib/aws/_auth.py. Adds unit tests for the bedrock signing helper, covering the missing-header regression and that a present Connection header stays out of SignedHeaders.
67a50df to
0ddb758
Compare
What this does
get_auth_headersstripsConnectionfrom the SigV4-signed set so a proxy that drops it cannot break signature verification. It useddel headers["connection"], which raisesKeyErroronhttpx.Headerswhen the header is absent (a customhttp_clientmay omit or strip it before signing).Fix: remove the header only if present instead of deleting by an assumed key, so the signed set never includes
connectionand a missingConnectionis a no-op, not aKeyError. Preserves thehttpx.Headerscontainer and case-insensitive match.Verification
Red on
main, green after.KeyError, patched signsConnectionSignedHeadersruff format,ruff checkpytest test_bedrock_auth + test_aws_authReview map
src/anthropic/lib/bedrock/_auth.py: copy headers, thenpop("connection", None)instead ofdel; signnew_headers.tests/lib/test_bedrock_auth.py: missing-Connection regression; mixed-caseConnectionexcluded fromSignedHeaders; sanity.