Fix ranged GET signing failure with Cloudflare R2#452
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaced per-variant header branching with logic that inserts ChangesRequest header construction
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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
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
|
|
Thanks for the R2 signing fix — I verified locally that this branch fixes the targeted R2 provider failure: cd s3
cargo test streaming_big_r2_put_head_get_delete_object -- --ignored --nocaptureOn this PR ( However, I also found a deterministic GCS regression in the same local environment. The targeted GCS streaming multipart test passes repeatedly on current cd s3
cargo test streaming_big_gc_put_head_get_delete_object -- --ignored --nocaptureObserved results:
The failure is: The likely cause is the new Command::PutObject { .. }
| Command::PutObjectTagging { .. }
| Command::UploadPart { .. }
| Command::CompleteMultipartUpload { .. }
| Command::CreateBucket { .. }
| Command::PutBucketLifecycle { .. }
| Command::PutBucketCors { .. }But So I agree with the direction — avoiding empty signed body headers fixes R2 — but this PR needs one more adjustment before merge:
Once that is fixed, this should be a strong compatibility improvement. |
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.
GCS rejects POST requests without a Content-Length header (HTTP 411), even when the body is empty. The previous has_body() definition omitted Content-Length for InitiateMultipartUpload, regressing streaming multipart uploads against GCS. Add InitiateMultipartUpload to has_body() so Content-Length: 0 is sent on the initiate-multipart POST. Document why the helper covers a body-less command. Add provider-agnostic unit tests for has_body() covering ranged GET, body-less DELETE/CopyObject, InitiateMultipartUpload, and body-bearing commands.
w4nderlust
force-pushed
the
fix/ranged-get-signing-r2
branch
from
3ae1dd0 to
52881b0
Compare
Brownian Motion (Brass)Recommendation: Proceed Summary: Fixes signature mismatch for ranged GET requests on Cloudflare R2. Highlights
Unknowns
Next actions
Reflection questions
|
|
Thanks for the careful message. You're right, the I've pushed a fix and rebased the branch onto current Change (
I left the rest of the body-less Also worth noting that Provider-agnostic regression coverage added in
Verification on my end: I don't have a GCS bucket set up locally to re-run |
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 `@s3/src/command.rs`:
- Around line 404-464: Add tests that exercise Request::headers() to ensure
body-related headers match the Command classification: create requests for
Command::GetObjectRange (and Command::GetObject/HeadObject/ListBuckets) and
assert that headers do not contain Content-Length/Content-Type; create requests
for Command::InitiateMultipartUpload to assert Content-Length: 0 is present; and
create requests for Command::DeleteObject/AbortMultipartUpload/CopyObject to
assert no body headers. Use the existing Command variants (e.g.,
Command::GetObjectRange, Command::InitiateMultipartUpload,
Command::DeleteObject) and the Request::headers() method to validate the actual
header set so future changes to has_body() won’t drift from the signed headers.
- Around line 236-247: The has_body() matcher is missing the
Command::DeleteObjects variant so multi-object delete requests are treated as
body-less; update the Command::has_body() function to include
Command::DeleteObjects { .. } alongside the other variants (e.g., PutObject,
UploadPart, InitiateMultipartUpload, CompleteMultipartUpload, CreateBucket,
PutBucketLifecycle, PutBucketCors) so DeleteObjects returns true and the
request/request_trait logic will include Content-Length/Content-Type and body
signing.
🪄 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: b3c2ed91-1d3f-49c8-bbc0-e43e3d8b3627
📒 Files selected for processing (2)
s3/src/command.rss3/src/request/request_trait.rs
| #[cfg(test)] | ||
| mod tests { | ||
| use super::*; | ||
|
|
||
| /// Body-less `GET`s (notably ranged `GET`) must not advertise body | ||
| /// headers, otherwise Cloudflare R2 rejects the AWS4-HMAC-SHA256 | ||
| /// signature for ranged downloads. | ||
| #[test] | ||
| fn ranged_get_does_not_have_body() { | ||
| let cmd = Command::GetObjectRange { | ||
| start: 0, | ||
| end: Some(1023), | ||
| }; | ||
| assert!(!cmd.has_body()); | ||
| assert!(!Command::GetObject.has_body()); | ||
| assert!(!Command::HeadObject.has_body()); | ||
| assert!(!Command::ListBuckets.has_body()); | ||
| } | ||
|
|
||
| /// `DELETE` and `CopyObject` carry no request body. | ||
| #[test] | ||
| fn delete_and_copy_do_not_have_body() { | ||
| assert!(!Command::DeleteObject.has_body()); | ||
| assert!(!Command::AbortMultipartUpload { upload_id: "u" }.has_body()); | ||
| assert!(!Command::CopyObject { from: "x" }.has_body()); | ||
| } | ||
|
|
||
| /// `InitiateMultipartUpload` is body-less but must still be reported as | ||
| /// having a body so that `Content-Length: 0` is sent. GCS returns HTTP | ||
| /// 411 on `POST` requests without `Content-Length`, even when the body | ||
| /// is empty. | ||
| #[test] | ||
| fn initiate_multipart_upload_has_body_for_gcs_compat() { | ||
| let cmd = Command::InitiateMultipartUpload { | ||
| content_type: "application/octet-stream", | ||
| }; | ||
| assert!(cmd.has_body()); | ||
| assert_eq!(cmd.http_verb(), HttpMethod::Post); | ||
| assert_eq!(cmd.content_length().unwrap(), 0); | ||
| } | ||
|
|
||
| /// Body-bearing commands report `has_body() == true` so signing | ||
| /// includes accurate `Content-Length` / `Content-Type`. | ||
| #[test] | ||
| fn body_bearing_commands_have_body() { | ||
| let put = Command::PutObject { | ||
| content: b"hello", | ||
| content_type: "text/plain", | ||
| custom_headers: None, | ||
| multipart: None, | ||
| }; | ||
| assert!(put.has_body()); | ||
|
|
||
| let upload = Command::UploadPart { | ||
| part_number: 1, | ||
| content: b"data", | ||
| upload_id: "u", | ||
| }; | ||
| assert!(upload.has_body()); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Please cover the header integration, not just has_body().
These tests prove the enum classification, but the regression surface is Request::headers() in s3/src/request/request_trait.rs. A header-level test for ranged GET vs InitiateMultipartUpload/DeleteObjects would catch future drifts between has_body() and the signed header set.
🤖 Prompt for 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.
In `@s3/src/command.rs` around lines 404 - 464, Add tests that exercise
Request::headers() to ensure body-related headers match the Command
classification: create requests for Command::GetObjectRange (and
Command::GetObject/HeadObject/ListBuckets) and assert that headers do not
contain Content-Length/Content-Type; create requests for
Command::InitiateMultipartUpload to assert Content-Length: 0 is present; and
create requests for Command::DeleteObject/AbortMultipartUpload/CopyObject to
assert no body headers. Use the existing Command variants (e.g.,
Command::GetObjectRange, Command::InitiateMultipartUpload,
Command::DeleteObject) and the Request::headers() method to validate the actual
header set so future changes to has_body() won’t drift from the signed headers.
DeleteObjects is a POST with an XML body listing the keys to delete (Content-Type: application/xml, content_length > 0). It was missing from has_body(), so multi-object delete requests would have been signed without Content-Length/Content-Type, causing failures and signature mismatches on strict providers. Add a unit test pinning the body-bearing classification, content type, and non-zero content length.
Brownian Motion (Brass)Recommendation: Proceed Summary: PR effectively fixes signing issues for body-less commands in S3 requests. Highlights
Unknowns
Next actions
Reflection questions
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
s3/src/command.rs (1)
236-249: ⚡ Quick winAdd a
# Examplesdoctest tohas_body().The method's narrative doc comment is thorough, but a short compilable example is still missing. The coding guideline requires documentation examples for all public APIs.
📝 Suggested doctest addition
+ /// # Examples + /// + /// ``` + /// # use rust_s3::command::Command; + /// assert!(Command::PutObject { content: b"hi", content_type: "text/plain", custom_headers: None, multipart: None }.has_body()); + /// assert!(!Command::GetObject.has_body()); + /// assert!(!Command::GetObjectRange { start: 0, end: Some(1023) }.has_body()); + /// ``` pub fn has_body(&self) -> bool {As per coding guidelines: "All public APIs should have documentation examples."
🤖 Prompt for 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. In `@s3/src/command.rs` around lines 236 - 249, The public method Command::has_body is missing a doctest example; add a short, compilable Rust doc example under the function's doc comment showing usage and assertions—for example construct a Command::PutObject with sample fields and assert has_body() is true, and assert that Command::GetObject and Command::GetObjectRange { start: 0, end: Some(1023) } return false; ensure the example uses fully qualified path if necessary (e.g., use rust_s3::command::Command;) and is fenced as a doctest so it compiles during cargo test.
🤖 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.
Nitpick comments:
In `@s3/src/command.rs`:
- Around line 236-249: The public method Command::has_body is missing a doctest
example; add a short, compilable Rust doc example under the function's doc
comment showing usage and assertions—for example construct a Command::PutObject
with sample fields and assert has_body() is true, and assert that
Command::GetObject and Command::GetObjectRange { start: 0, end: Some(1023) }
return false; ensure the example uses fully qualified path if necessary (e.g.,
use rust_s3::command::Command;) and is fenced as a doctest so it compiles during
cargo test.
Brownian Motion (Brass)Recommendation: Proceed Summary: Fixes signature mismatch for ranged GET requests on Cloudflare R2. Highlights
Unknowns
Next actions
Reflection questions
|
2 participants
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
Behavior
Tests