Fix ranged GET signing failure with Cloudflare R2#452
Fix ranged GET signing failure with Cloudflare R2#452w4nderlust wants to merge 1 commit intodurch:masterfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughReplaced per-variant header branching with logic that inserts Content-Length and Content-Type only when Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Brownian Motion (Brass)Recommendation: Proceed Summary: Fixes signing issue for ranged GET requests with Cloudflare R2. Highlights
Unknowns
Next actions
Reflection questions
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
s3/src/request/request_trait.rs (1)
703-719:⚠️ Potential issue | 🟠 Major
http_verb()is still too broad a proxy for signing body headers.This fixes the
GET/HEADcase, but Lines 712-719 still signContent-LengthandContent-Typefor zero-body commands likeCopyObject,AbortMultipartUpload,DeleteObject,DeleteBucket, andInitiateMultipartUploadbecause they arePUT/POST/DELETE.request_body()on Lines 231-254 treats those as empty, so the same signature-drift class can still happen on providers that normalize or ignore empty-requestContent-*headers. Please drive this off a dedicated “does this command serialize a body?” helper instead of the HTTP verb.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@s3/src/request/request_trait.rs` around lines 703 - 719, The code currently decides to sign Content-Length/Content-Type based on http_verb(), which still signs headers for zero-body commands (e.g., CopyObject, AbortMultipartUpload, DeleteObject, DeleteBucket, InitiateMultipartUpload); instead, check whether the command actually serializes a body (use an existing request_body() or add a helper like serializes_body()/has_request_body() on the Command) and only insert CONTENT_LENGTH/CONTENT_TYPE when that helper returns true; update the block that now matches on self.command().http_verb() to call self.command().request_body().is_some() (or the new helper) and keep special-case CopyObject header handling as-is.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@s3/src/request/request_trait.rs`:
- Around line 703-719: The code currently decides to sign
Content-Length/Content-Type based on http_verb(), which still signs headers for
zero-body commands (e.g., CopyObject, AbortMultipartUpload, DeleteObject,
DeleteBucket, InitiateMultipartUpload); instead, check whether the command
actually serializes a body (use an existing request_body() or add a helper like
serializes_body()/has_request_body() on the Command) and only insert
CONTENT_LENGTH/CONTENT_TYPE when that helper returns true; update the block that
now matches on self.command().http_verb() to call
self.command().request_body().is_some() (or the new helper) and keep
special-case CopyObject header handling as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 12ead844-6852-400e-923b-8eb7e89e5af0
📒 Files selected for processing (1)
s3/src/request/request_trait.rs
w4nderlust
force-pushed
the
fix/ranged-get-signing-r2
branch
from
3eb5fdf to
e32c578
Compare
Brownian Motion (Brass)Recommendation: Proceed Summary: Fixes a signing issue with ranged GET requests for Cloudflare R2. Highlights
Unknowns
Next actions
Reflection questions
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@s3/src/command.rs`:
- Around line 214-227: The headers() implementation in request_trait.rs still
checks HTTP verb via self.command().http_verb() to decide whether to insert
CONTENT_LENGTH and CONTENT_TYPE; change that logic to call
self.command().has_body() instead so headers are added for commands that
actually carry bodies (refer to the Command::has_body() method you added).
Replace the current match on HttpMethod with an if self.command().has_body() {
insert CONTENT_LENGTH and CONTENT_TYPE } branch, ensuring you use the same
header keys (CONTENT_LENGTH, CONTENT_TYPE) and value computation already
present; also add a short doctest/example to the public has_body() method’s doc
comment showing typical usage (e.g., calling Command::has_body() to gate header
insertion).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 936ed796-81ba-470d-9dc3-4b663a75cb5a
📒 Files selected for processing (2)
s3/src/command.rss3/src/request/request_trait.rs
✅ Files skipped from review due to trivial changes (1)
- s3/src/request/request_trait.rs
GetObjectRange (and other body-less commands like DeleteObject, AbortMultipartUpload, etc.) were getting content-length and content-type headers included in the signed request. For commands with no body these headers are empty/meaningless, but they still end up in the canonical request signature. Cloudflare R2 rejects the resulting signature (SignatureDoesNotMatch), while AWS S3 happens to tolerate it. Added a `has_body()` helper on Command that returns true only for commands that actually serialize request content (PutObject, UploadPart, CompleteMultipartUpload, etc.). The header insertion in request_trait.rs now checks `has_body()` instead of matching on the HTTP verb, which avoids signing empty headers for any current or future body-less command.
w4nderlust
force-pushed
the
fix/ranged-get-signing-r2
branch
from
e32c578 to
3ae1dd0
Compare
Brownian Motion (Brass)Recommendation: Proceed Summary: Fixes signing issue for body-less commands in Cloudflare R2 requests. Highlights
Unknowns
Next actions
Reflection questions
|
1 participant
Problem
get_object_range(ranged GET) requests fail withSignatureDoesNotMatchon Cloudflare R2.The
headers()method inrequest_trait.rshad a skip-list of commands that should not includecontent-lengthandcontent-typeheaders.GetObjectRangewas missing from this list, so it fell through to the catch-all_ =>arm that addscontent-length: 0andcontent-type: text/plain.These empty/meaningless headers get included in the AWS4-HMAC-SHA256 canonical request. AWS S3 happens to tolerate this, but Cloudflare R2 rejects the signature because the canonical request doesn't match what R2 computes server-side.
The same class of bug also affected other body-less commands routed through PUT/POST/DELETE (
DeleteObject,AbortMultipartUpload,InitiateMultipartUpload, etc.).Fix
Added a
has_body()helper onCommandthat returnstrueonly for commands that actually serialize request content (PutObject,UploadPart,CompleteMultipartUpload,CreateBucket,PutBucketLifecycle,PutBucketCors,PutObjectTagging). The header insertion now checkshas_body()instead of using a skip-list or matching on the HTTP verb. This prevents the same bug from happening if new body-less commands are added in the future.Testing
All existing tests pass. Verified the fix against a real Cloudflare R2 bucket: ranged GET downloads that were failing with
SignatureDoesNotMatchnow succeed.This change is
Summary by CodeRabbit
Refactor