Skip to content
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
94 changes: 94 additions & 0 deletions s3/src/command.rs
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,38 @@ impl<'a> Command<'a> {
}
}

/// Whether this command should include `Content-Length` and `Content-Type`
/// headers in the signed request.
///
/// Returns `true` for commands that serialize a request body, plus
/// `InitiateMultipartUpload`. The latter is a `POST` with an empty body
/// but is included because:
///
/// - Google Cloud Storage rejects the request with HTTP 411 if
/// `Content-Length` is omitted from a `POST`, even when the body is
/// empty.
/// - The `Content-Type` value carried by `InitiateMultipartUpload` is
/// not a description of the (empty) request body but the content type
/// to associate with the eventual multipart object on the server.
///
/// Body-less `GET`, `HEAD`, and `DELETE` commands return `false` so that
/// stray `Content-Length: 0` / `Content-Type: text/plain` headers do
/// not enter the AWS4-HMAC-SHA256 canonical request, which Cloudflare
/// R2 rejects as a signature mismatch (notably for ranged `GET`s).
pub fn has_body(&self) -> bool {
matches!(
self,
Command::PutObject { .. }
| Command::PutObjectTagging { .. }
| Command::UploadPart { .. }
| Command::InitiateMultipartUpload { .. }
| Command::CompleteMultipartUpload { .. }
| Command::CreateBucket { .. }
| Command::PutBucketLifecycle { .. }
| Command::PutBucketCors { .. }
)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}

pub fn content_length(&self) -> Result<usize, S3Error> {
let result = match &self {
Command::CopyObject { from: _ } => 0,
Expand Down Expand Up @@ -368,3 +400,65 @@ impl<'a> Command<'a> {
Ok(result)
}
}

#[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());
}
}
Comment on lines +421 to +503

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

36 changes: 18 additions & 18 deletions s3/src/request/request_trait.rs
Original file line number Diff line number Diff line change
Expand Up @@ -730,24 +730,24 @@ pub trait Request {

headers.insert(HOST, host_header.parse()?);

match self.command() {
Command::CopyObject { from } => {
headers.insert(HeaderName::from_static("x-amz-copy-source"), from.parse()?);
}
Command::ListObjects { .. } => {}
Command::ListObjectsV2 { .. } => {}
Command::HeadObject => {}
Command::GetObject => {}
Command::GetObjectTagging => {}
Command::GetBucketLocation => {}
Command::ListBuckets => {}
_ => {
headers.insert(
CONTENT_LENGTH,
self.command().content_length()?.to_string().parse()?,
);
headers.insert(CONTENT_TYPE, self.command().content_type().parse()?);
}
if let Command::CopyObject { from } = self.command() {
headers.insert(HeaderName::from_static("x-amz-copy-source"), from.parse()?);
}

// Include content-length and content-type only for commands that
// either carry a request body or are otherwise required by some
// providers to advertise these headers (see Command::has_body).
// Body-less GET/HEAD/DELETE/CopyObject must not sign these headers,
// otherwise Cloudflare R2 rejects the AWS4-HMAC-SHA256 signature
// because the empty content-length value corrupts the canonical
// request. InitiateMultipartUpload is included because GCS returns
// HTTP 411 on a POST without Content-Length, even with an empty body.
if self.command().has_body() {
headers.insert(
CONTENT_LENGTH,
self.command().content_length()?.to_string().parse()?,
);
headers.insert(CONTENT_TYPE, self.command().content_type().parse()?);
}
headers.insert(
HeaderName::from_static("x-amz-content-sha256"),
Expand Down
Loading