From 1a054ce93836b8fe807ad15de2b099614f535195 Mon Sep 17 00:00:00 2001 From: Nugine Date: Fri, 9 Oct 2026 00:41:32 +0800 Subject: [PATCH 1/4] refactor(s3s): move the POST Object form parsing into a handwritten module PostObject::deserialize_http and serialize_http were emitted as function bodies into crates/s3s/src/ops/generated/post_object.rs. Move both verbatim into crates/s3s/src/ops/post_object.rs and emit thin delegates instead, so the POST-specific parsing and response serialization live in hand-written code. The struct, the Operation impl and the call glue stay generated, and the wrapper methods keep their public signatures. No behavior change. Assisted-by: DeepSeek Harness:deepseek-flash --- codegen/src/v2/post_object.rs | 74 +--------------- crates/s3s/src/ops/generated/post_object.rs | 72 +--------------- crates/s3s/src/ops/mod.rs | 1 + crates/s3s/src/ops/post_object.rs | 95 +++++++++++++++++++++ 4 files changed, 102 insertions(+), 140 deletions(-) create mode 100644 crates/s3s/src/ops/post_object.rs diff --git a/codegen/src/v2/post_object.rs b/codegen/src/v2/post_object.rs index aa70e414a..e8b3ae121 100644 --- a/codegen/src/v2/post_object.rs +++ b/codegen/src/v2/post_object.rs @@ -201,32 +201,13 @@ pub(crate) fn codegen_fork_op(rust_types: &RustTypes) { "use crate::dto::*;", "use crate::error::*;", "use crate::http;", - "use crate::ops::{CallContext, PutObject};", + "use crate::ops::CallContext;", "", ]); g(["pub struct PostObject;", "", "impl PostObject {"]); g([ " pub fn deserialize_http(req: &mut http::Request) -> S3Result {", - " let Some(m) = req.s3ext.multipart.take() else {", - " return Err(invalid_request!(\"missing multipart form\"));", - " };", - "", - " // Parse POST-specific fields before consuming the multipart form", - " let success_action_redirect: Option = match http::parse_field_value(&m, \"success_action_redirect\")? {", - " Some(v) => Some(v),", - " None => http::parse_field_value(&m, \"redirect\")?,", - " };", - " let success_action_status: Option = http::parse_field_value(&m, \"success_action_status\")?;", - "", - " // Get the validated POST policy from request extensions", - " let policy = req.s3ext.post_policy.take();", - "", - " let put_input = PutObject::deserialize_http_multipart(req, m)?;", - " let mut post_input = put_object_input_into_post_object_input(put_input);", - " post_input.success_action_redirect = success_action_redirect;", - " post_input.success_action_status = success_action_status;", - " post_input.policy = policy;", - " Ok(post_input)", + " crate::ops::post_object::deserialize_http(req)", " }", "", " pub fn serialize_http(", @@ -236,56 +217,7 @@ pub(crate) fn codegen_fork_op(rust_types: &RustTypes) { " success_action_status: Option,", " output: &PostObjectOutput,", " ) -> S3Result {", - " let etag_str = output.e_tag.as_ref().map(ETag::value).unwrap_or_default();", - "", - " // Handle success_action_redirect: return 303 See Other with Location header", - " if let Some(redirect_url) = success_action_redirect {", - " // Defense-in-depth: Reject URLs with control characters that could enable header injection", - " if redirect_url.chars().any(char::is_control) {", - " return Err(s3_error!(InvalidArgument, \"success_action_redirect contains invalid control characters\"));", - " }", - "", - " // Parse the URL to validate and manipulate it properly", - " let mut url = url::Url::parse(redirect_url).map_err(|e| s3_error!(e, InvalidArgument, \"Invalid redirect URL\"))?;", - "", - " // Add query parameters (bucket, key, etag) to the URL", - " url.query_pairs_mut()", - " .append_pair(\"bucket\", bucket)", - " .append_pair(\"key\", key)", - " .append_pair(\"etag\", etag_str);", - "", - " let mut res = http::Response::with_status(http::StatusCode::SEE_OTHER);", - " res.headers.insert(", - " hyper::header::LOCATION,", - " url.as_str().parse().map_err(|e| s3_error!(e, InternalError))?", - " );", - " return Ok(res);", - " }", - "", - " // Handle success_action_status", - " match success_action_status {", - " Some(200) => {", - " // 200 OK with empty body", - " Ok(http::Response::with_status(http::StatusCode::OK))", - " }", - " Some(201) => {", - " // 201 Created with XML body using PostResponse DTO", - " let location = format!(\"/{bucket}/{key}\");", - " let post_response = crate::dto::PostResponse {", - " location: &location,", - " bucket,", - " key,", - " etag: etag_str,", - " };", - " let mut res = http::Response::with_status(http::StatusCode::CREATED);", - " http::set_xml_body(&mut res, &post_response)?;", - " Ok(res)", - " }", - " _ => {", - " // 204 No Content (default, also for unrecognized values)", - " Ok(http::Response::with_status(http::StatusCode::NO_CONTENT))", - " }", - " }", + " crate::ops::post_object::serialize_http(bucket, key, success_action_redirect, success_action_status, output)", " }", "}", "", diff --git a/crates/s3s/src/ops/generated/post_object.rs b/crates/s3s/src/ops/generated/post_object.rs index 39c4e35c1..15acc30b7 100644 --- a/crates/s3s/src/ops/generated/post_object.rs +++ b/crates/s3s/src/ops/generated/post_object.rs @@ -6,32 +6,13 @@ use crate::dto::*; use crate::error::*; use crate::http; -use crate::ops::{CallContext, PutObject}; +use crate::ops::CallContext; pub struct PostObject; impl PostObject { pub fn deserialize_http(req: &mut http::Request) -> S3Result { - let Some(m) = req.s3ext.multipart.take() else { - return Err(invalid_request!("missing multipart form")); - }; - - // Parse POST-specific fields before consuming the multipart form - let success_action_redirect: Option = match http::parse_field_value(&m, "success_action_redirect")? { - Some(v) => Some(v), - None => http::parse_field_value(&m, "redirect")?, - }; - let success_action_status: Option = http::parse_field_value(&m, "success_action_status")?; - - // Get the validated POST policy from request extensions - let policy = req.s3ext.post_policy.take(); - - let put_input = PutObject::deserialize_http_multipart(req, m)?; - let mut post_input = put_object_input_into_post_object_input(put_input); - post_input.success_action_redirect = success_action_redirect; - post_input.success_action_status = success_action_status; - post_input.policy = policy; - Ok(post_input) + crate::ops::post_object::deserialize_http(req) } pub fn serialize_http( @@ -41,54 +22,7 @@ impl PostObject { success_action_status: Option, output: &PostObjectOutput, ) -> S3Result { - let etag_str = output.e_tag.as_ref().map(ETag::value).unwrap_or_default(); - - // Handle success_action_redirect: return 303 See Other with Location header - if let Some(redirect_url) = success_action_redirect { - // Defense-in-depth: Reject URLs with control characters that could enable header injection - if redirect_url.chars().any(char::is_control) { - return Err(s3_error!(InvalidArgument, "success_action_redirect contains invalid control characters")); - } - - // Parse the URL to validate and manipulate it properly - let mut url = url::Url::parse(redirect_url).map_err(|e| s3_error!(e, InvalidArgument, "Invalid redirect URL"))?; - - // Add query parameters (bucket, key, etag) to the URL - url.query_pairs_mut() - .append_pair("bucket", bucket) - .append_pair("key", key) - .append_pair("etag", etag_str); - - let mut res = http::Response::with_status(http::StatusCode::SEE_OTHER); - res.headers - .insert(hyper::header::LOCATION, url.as_str().parse().map_err(|e| s3_error!(e, InternalError))?); - return Ok(res); - } - - // Handle success_action_status - match success_action_status { - Some(200) => { - // 200 OK with empty body - Ok(http::Response::with_status(http::StatusCode::OK)) - } - Some(201) => { - // 201 Created with XML body using PostResponse DTO - let location = format!("/{bucket}/{key}"); - let post_response = crate::dto::PostResponse { - location: &location, - bucket, - key, - etag: etag_str, - }; - let mut res = http::Response::with_status(http::StatusCode::CREATED); - http::set_xml_body(&mut res, &post_response)?; - Ok(res) - } - _ => { - // 204 No Content (default, also for unrecognized values) - Ok(http::Response::with_status(http::StatusCode::NO_CONTENT)) - } - } + crate::ops::post_object::serialize_http(bucket, key, success_action_redirect, success_action_status, output) } } diff --git a/crates/s3s/src/ops/mod.rs b/crates/s3s/src/ops/mod.rs index e1621d923..23a9119fe 100644 --- a/crates/s3s/src/ops/mod.rs +++ b/crates/s3s/src/ops/mod.rs @@ -17,6 +17,7 @@ use self::signature::{CredentialsExt, SignatureContext}; mod get_object; mod multipart; mod payload_length; +mod post_object; #[cfg(test)] mod tests; diff --git a/crates/s3s/src/ops/post_object.rs b/crates/s3s/src/ops/post_object.rs new file mode 100644 index 000000000..bb549a5c7 --- /dev/null +++ b/crates/s3s/src/ops/post_object.rs @@ -0,0 +1,95 @@ +// SPDX-License-Identifier: Apache-2.0 +// SPDX-FileCopyrightText: 2023-2026 The s3s Authors + +//! The POST-specific parsing and serialization of the `PostObject` operation. +//! +//! The form fields, the policy and the `success_action_*` response semantics +//! live here; the generated operation module delegates to these functions. + +use crate::dto::ETag; +use crate::dto::PostObjectInput; +use crate::dto::PostObjectOutput; +use crate::dto::put_object_input_into_post_object_input; +use crate::error::*; +use crate::http; +use crate::ops::PutObject; + +pub(crate) fn deserialize_http(req: &mut http::Request) -> S3Result { + let Some(m) = req.s3ext.multipart.take() else { + return Err(invalid_request!("missing multipart form")); + }; + + // Parse POST-specific fields before consuming the multipart form + let success_action_redirect: Option = match http::parse_field_value(&m, "success_action_redirect")? { + Some(v) => Some(v), + None => http::parse_field_value(&m, "redirect")?, + }; + let success_action_status: Option = http::parse_field_value(&m, "success_action_status")?; + + // Get the validated POST policy from request extensions + let policy = req.s3ext.post_policy.take(); + + let put_input = PutObject::deserialize_http_multipart(req, m)?; + let mut post_input = put_object_input_into_post_object_input(put_input); + post_input.success_action_redirect = success_action_redirect; + post_input.success_action_status = success_action_status; + post_input.policy = policy; + Ok(post_input) +} + +pub(crate) fn serialize_http( + bucket: &str, + key: &str, + success_action_redirect: Option<&str>, + success_action_status: Option, + output: &PostObjectOutput, +) -> S3Result { + let etag_str = output.e_tag.as_ref().map(ETag::value).unwrap_or_default(); + + // Handle success_action_redirect: return 303 See Other with Location header + if let Some(redirect_url) = success_action_redirect { + // Defense-in-depth: Reject URLs with control characters that could enable header injection + if redirect_url.chars().any(char::is_control) { + return Err(s3_error!(InvalidArgument, "success_action_redirect contains invalid control characters")); + } + + // Parse the URL to validate and manipulate it properly + let mut url = url::Url::parse(redirect_url).map_err(|e| s3_error!(e, InvalidArgument, "Invalid redirect URL"))?; + + // Add query parameters (bucket, key, etag) to the URL + url.query_pairs_mut() + .append_pair("bucket", bucket) + .append_pair("key", key) + .append_pair("etag", etag_str); + + let mut res = http::Response::with_status(http::StatusCode::SEE_OTHER); + res.headers + .insert(hyper::header::LOCATION, url.as_str().parse().map_err(|e| s3_error!(e, InternalError))?); + return Ok(res); + } + + // Handle success_action_status + match success_action_status { + Some(200) => { + // 200 OK with empty body + Ok(http::Response::with_status(http::StatusCode::OK)) + } + Some(201) => { + // 201 Created with XML body using PostResponse DTO + let location = format!("/{bucket}/{key}"); + let post_response = crate::dto::PostResponse { + location: &location, + bucket, + key, + etag: etag_str, + }; + let mut res = http::Response::with_status(http::StatusCode::CREATED); + http::set_xml_body(&mut res, &post_response)?; + Ok(res) + } + _ => { + // 204 No Content (default, also for unrecognized values) + Ok(http::Response::with_status(http::StatusCode::NO_CONTENT)) + } + } +} From f472c1d7247fd9fe6e809c0dcc63af393fcb1325 Mon Sep 17 00:00:00 2001 From: Nugine Date: Fri, 9 Oct 2026 00:44:20 +0800 Subject: [PATCH 2/4] test(s3s): cover the POST Object redirect and status branches directly The wire suite covers the client-visible shapes, but it cannot reach the control-character rejection or the URL parse failure of the redirect, nor pin the status mapping in isolation. Call serialize_http directly for the default, 200, 201 and unrecognized statuses, for a redirect with a control character and for an unparsable redirect. Assisted-by: DeepSeek Harness:deepseek-flash --- crates/s3s/src/ops/post_object.rs | 73 +++++++++++++++++++++++++++++++ 1 file changed, 73 insertions(+) diff --git a/crates/s3s/src/ops/post_object.rs b/crates/s3s/src/ops/post_object.rs index bb549a5c7..1e8daf011 100644 --- a/crates/s3s/src/ops/post_object.rs +++ b/crates/s3s/src/ops/post_object.rs @@ -93,3 +93,76 @@ pub(crate) fn serialize_http( } } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::error::S3ErrorCode; + use hyper::StatusCode; + + fn output_with_etag(value: &str) -> PostObjectOutput { + PostObjectOutput { + e_tag: Some(ETag::Strong(value.to_owned())), + ..Default::default() + } + } + + fn assert_no_body(res: &http::Response) { + let bytes = res.body.bytes(); + assert!(bytes.is_none_or(|bytes| bytes.is_empty()), "the response must not carry a body"); + } + + #[test] + fn the_default_status_is_204() { + let res = serialize_http("bucket", "key", None, None, &PostObjectOutput::default()).expect("no status"); + assert_eq!(res.status, StatusCode::NO_CONTENT); + assert_no_body(&res); + } + + #[test] + fn status_200_is_ok_without_a_body() { + let res = serialize_http("bucket", "key", None, Some(200), &output_with_etag("\"abc\"")).expect("200"); + assert_eq!(res.status, StatusCode::OK); + assert_no_body(&res); + } + + #[test] + fn status_201_writes_the_post_response_xml() { + let res = serialize_http("bucket", "key", None, Some(201), &output_with_etag("\"abc\"")).expect("201"); + assert_eq!(res.status, StatusCode::CREATED); + let body = res.body.bytes().expect("the XML body is buffered"); + let body = std::str::from_utf8(&body).expect("the body is UTF-8"); + assert!(body.contains("/bucket/key"), "{body}"); + assert!(body.contains("bucket"), "{body}"); + assert!(body.contains("key"), "{body}"); + assert!(body.contains(""abc""), "{body}"); + } + + #[test] + fn an_unrecognized_status_is_204() { + let res = serialize_http("bucket", "key", None, Some(418), &PostObjectOutput::default()).expect("418"); + assert_eq!(res.status, StatusCode::NO_CONTENT); + assert_no_body(&res); + } + + #[test] + fn a_control_character_in_the_redirect_is_rejected() { + // The WHATWG URL parser strips tabs, so this sample only fails because of the + // explicit control-character check. + let redirect = "https://example.com/a\tb"; + assert!(url::Url::parse(redirect).is_ok(), "the URL parser accepts the sample"); + + let err = serialize_http("bucket", "key", Some(redirect), None, &PostObjectOutput::default()) + .err() + .expect("a control character must be rejected"); + assert_eq!(*err.code(), S3ErrorCode::InvalidArgument); + } + + #[test] + fn an_unparsable_redirect_is_an_invalid_argument() { + let err = serialize_http("bucket", "key", Some("not a url"), None, &PostObjectOutput::default()) + .err() + .expect("a relative URL must be rejected"); + assert_eq!(*err.code(), S3ErrorCode::InvalidArgument); + } +} From bd7e3806c5e098b60901b480e1bf363cbf12b2ea Mon Sep 17 00:00:00 2001 From: Nugine Date: Fri, 9 Oct 2026 02:05:38 +0800 Subject: [PATCH 3/4] test(s3s): cover the POST Object form errors and the XML error path The patch coverage of the moved module left the missing multipart form, the legacy redirect fallback, a malformed success_action_status and the XML body error path unexercised. Cover each directly; the moved functions stay byte for byte unchanged. Assisted-by: DeepSeek Harness:deepseek-flash --- crates/s3s/src/ops/post_object.rs | 70 +++++++++++++++++++++++++++++++ 1 file changed, 70 insertions(+) diff --git a/crates/s3s/src/ops/post_object.rs b/crates/s3s/src/ops/post_object.rs index 1e8daf011..c2c45d470 100644 --- a/crates/s3s/src/ops/post_object.rs +++ b/crates/s3s/src/ops/post_object.rs @@ -165,4 +165,74 @@ mod tests { .expect("a relative URL must be rejected"); assert_eq!(*err.code(), S3ErrorCode::InvalidArgument); } + + /// Builds a prepared POST request whose form carries one extra field. + /// + /// The policy conditions cover the extra field, so prepare accepts the form. + async fn prepared_post_request(extra_field: (&str, &str)) -> crate::http::Request { + use crate::auth::SecretKey; + use crate::ops::tests::common::post_policy_test_helpers as post; + use std::sync::Arc; + + let (name, value) = extra_field; + let s3: Arc = Arc::new(post::TestS3NoOp); + let config = post::create_test_config(1024 * 1024); + let auth = post::create_test_auth(); + let ccx = post::create_test_context(&s3, &config, &auth); + let secret_key: SecretKey = "wJalrXUtnFEMI/K7MDENG+bPxRfiCYEXAMPLEKEY".into(); + + let field_ref = ["$", name].concat(); + let policy_json = format!( + r#"{{"expiration":"2030-01-01T00:00:00.000Z","conditions":[["eq","{}","{}"],{}]}}"#, + field_ref, + value, + post::BASE_CONDITIONS, + ); + let mut req = + post::build_post_object_request_with(policy_json.as_str(), "content", &secret_key, false, &[extra_field], &[]); + crate::ops::prepare(&mut req, &ccx).await.expect("the signed form prepares"); + req + } + + #[test] + fn deserialize_http_without_a_multipart_form_is_invalid_request() { + let mut req = crate::http::Request::from( + hyper::Request::builder() + .method(hyper::Method::POST) + .uri("http://localhost/bucket") + .body(crate::http::Body::empty()) + .expect("a valid request"), + ); + + let err = deserialize_http(&mut req).expect_err("a request without a multipart form"); + assert_eq!(*err.code(), S3ErrorCode::InvalidRequest); + assert_eq!(err.message(), Some("missing multipart form")); + } + + #[test] + fn status_201_rejects_a_control_character_in_the_response_xml() { + let err = serialize_http("bucket", "key", None, Some(201), &output_with_etag("\u{1}")) + .err() + .expect("a control character must not reach the XML body"); + assert_eq!(*err.code(), S3ErrorCode::InternalError); + } + + #[tokio::test] + async fn deserialize_http_falls_back_to_the_legacy_redirect_field() { + let redirect = "https://example.com/callback"; + let mut req = prepared_post_request(("redirect", redirect)).await; + + let input = deserialize_http(&mut req).expect("the form parses"); + assert_eq!(input.success_action_redirect.as_deref(), Some(redirect)); + assert_eq!(input.success_action_status, None); + } + + #[tokio::test] + async fn deserialize_http_rejects_a_malformed_success_action_status() { + let mut req = prepared_post_request(("success_action_status", "abc")).await; + + let err = deserialize_http(&mut req).expect_err("a non-numeric status is rejected"); + assert_eq!(*err.code(), S3ErrorCode::InvalidArgument); + assert_eq!(err.message(), Some(r#"invalid field value: success_action_status: "abc""#)); + } } From 9624377a02d0592dcf63c403bdb59683670bed49 Mon Sep 17 00:00:00 2001 From: Nugine Date: Fri, 9 Oct 2026 02:09:47 +0800 Subject: [PATCH 4/4] test(s3s): pin that the redirect fields are validated while serializing parse_field_value is infallible for String, and a non-UTF-8 field is rejected by the multipart parser before the operation sees it, so the two redirect lookups have no reachable error path. Call them with a text value and show the rejection happens when the response is serialized instead. Assisted-by: DeepSeek Harness:deepseek-flash --- crates/s3s/src/ops/post_object.rs | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/crates/s3s/src/ops/post_object.rs b/crates/s3s/src/ops/post_object.rs index c2c45d470..6f8992233 100644 --- a/crates/s3s/src/ops/post_object.rs +++ b/crates/s3s/src/ops/post_object.rs @@ -235,4 +235,23 @@ mod tests { assert_eq!(*err.code(), S3ErrorCode::InvalidArgument); assert_eq!(err.message(), Some(r#"invalid field value: success_action_status: "abc""#)); } + + #[tokio::test] + async fn redirect_fields_are_validated_while_serializing_not_while_parsing() { + // parse_field_value is infallible for String: every text value parses, so the ? + // on the two redirect lookups has no reachable error path. The value is validated + // when the response is serialized instead. + let mut req = prepared_post_request(("success_action_redirect", "not a url")).await; + let input = deserialize_http(&mut req).expect("a text value parses"); + assert_eq!(input.success_action_redirect.as_deref(), Some("not a url")); + + let mut req = prepared_post_request(("redirect", "also not a url")).await; + let input = deserialize_http(&mut req).expect("a text value parses through the fallback"); + assert_eq!(input.success_action_redirect.as_deref(), Some("also not a url")); + + let err = serialize_http("bucket", "key", Some("not a url"), None, &PostObjectOutput::default()) + .err() + .expect("the redirect is rejected when the response is serialized"); + assert_eq!(*err.code(), S3ErrorCode::InvalidArgument); + } }