From 89bc398c334bd9c21366cefc232613c553dd871c Mon Sep 17 00:00:00 2001 From: Justin Martin Date: Fri, 1 Nov 2024 12:07:07 -0700 Subject: [PATCH 1/5] Improve clarity of Transfer-Encoding errors. Don't error on identity TE --- spansy/src/http/span.rs | 122 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 112 insertions(+), 10 deletions(-) diff --git a/spansy/src/http/span.rs b/spansy/src/http/span.rs index a93b40e..eeff909 100644 --- a/spansy/src/http/span.rs +++ b/spansy/src/http/span.rs @@ -222,12 +222,23 @@ fn request_body_len(request: &Request) -> Result { // the Transfer-Encoding overrides the Content-Length if request .headers_with_name("Transfer-Encoding") - .next() - .is_some() + .any(|h| { + std::str::from_utf8(h.value.0.as_bytes()) + .unwrap_or("") + .split(',') + .any(|v| v.trim() != "identity") + }) { - Err(ParseError( - "Transfer-Encoding not supported yet".to_string(), - )) + let bad_values = request + .headers_with_name("Transfer-Encoding") + .map(|h| std::str::from_utf8(h.value.0.as_bytes()).unwrap_or("")) + .collect::>() + .join(", "); + + Err(ParseError(format!( + "Transfer-Encoding other than identity not supported yet: {:?}", + bad_values + ))) } else if let Some(h) = request.headers_with_name("Content-Length").next() { // If a valid Content-Length header field is present without Transfer-Encoding, its decimal value // defines the expected message body length in octets. @@ -258,12 +269,23 @@ fn response_body_len(response: &Response) -> Result { if response .headers_with_name("Transfer-Encoding") - .next() - .is_some() + .any(|h| { + std::str::from_utf8(h.value.0.as_bytes()) + .unwrap_or("") + .split(',') + .any(|v| v.trim() != "identity") + }) { - Err(ParseError( - "Transfer-Encoding not supported yet".to_string(), - )) + let bad_values = response + .headers_with_name("Transfer-Encoding") + .map(|h| std::str::from_utf8(h.value.0.as_bytes()).unwrap_or("")) + .collect::>() + .join(", "); + + Err(ParseError(format!( + "Transfer-Encoding other than identity not supported yet: {:?}", + bad_values + ))) } else if let Some(h) = response.headers_with_name("Content-Length").next() { // If a valid Content-Length header field is present without Transfer-Encoding, its decimal value // defines the expected message body length in octets. @@ -363,6 +385,46 @@ mod tests { Content-Length: 14\r\n\r\n\ {\"foo\": \"bar\"}"; + const TEST_REQUEST_TRANSFER_ENCODING_IDENTITY: &[u8] = b"\ + POST / HTTP/1.1\r\n\ + Transfer-Encoding: identity\r\n\ + Content-Length: 12\r\n\r\n\ + Hello World!"; + + const TEST_RESPONSE_TRANSFER_ENCODING_IDENTITY: &[u8] = b"\ + HTTP/1.1 200 OK\r\n\ + Transfer-Encoding: identity\r\n\ + Content-Length: 12\r\n\r\n\ + Hello World!"; + + const TEST_REQUEST_TRANSFER_ENCODING_CHUNKED: &[u8] = b"\ + POST / HTTP/1.1\r\n\ + Transfer-Encoding: chunked\r\n\r\n\ + a\r\n\ + Hello World!\r\n\ + 0\r\n\r\n"; + + const TEST_RESPONSE_TRANSFER_ENCODING_CHUNKED: &[u8] = b"\ + HTTP/1.1 200 OK\r\n\ + Transfer-Encoding: chunked\r\n\r\n\ + a\r\n\ + Hello World!\r\n\ + 0\r\n\r\n"; + + const TEST_REQUEST_TRANSFER_ENCODING_MULTIPLE: &[u8] = b"\ + POST / HTTP/1.1\r\n\ + Transfer-Encoding: chunked, identity\r\n\r\n\ + a\r\n\ + Hello World!\r\n\ + 0\r\n\r\n"; + + const TEST_RESPONSE_TRANSFER_ENCODING_MULTIPLE: &[u8] = b"\ + HTTP/1.1 200 OK\r\n\ + Transfer-Encoding: chunked, identity\r\n\r\n\ + a\r\n\ + Hello World!\r\n\ + 0\r\n\r\n"; + #[test] fn test_parse_request() { let req = parse_request(TEST_REQUEST).unwrap(); @@ -496,4 +558,44 @@ mod tests { assert_eq!(value.span(), "{\"foo\": \"bar\"}"); } + + #[test] + fn test_parse_request_transfer_encoding_identity() { + let req = parse_request(TEST_REQUEST_TRANSFER_ENCODING_IDENTITY).unwrap(); + assert_eq!(req.body.unwrap().span(), b"Hello World!".as_slice()); + } + + #[test] + fn test_parse_response_transfer_encoding_identity() { + let res = parse_response(TEST_RESPONSE_TRANSFER_ENCODING_IDENTITY).unwrap(); + assert_eq!(res.body.unwrap().span(), b"Hello World!".as_slice()); + } + + #[test] + fn test_parse_request_transfer_encoding_chunked() { + let err = parse_request(TEST_REQUEST_TRANSFER_ENCODING_CHUNKED).unwrap_err(); + assert!(matches!(err, ParseError(_))); + assert!(err.to_string().contains("Transfer-Encoding other than identity not supported yet")); + } + + #[test] + fn test_parse_response_transfer_encoding_chunked() { + let err = parse_response(TEST_RESPONSE_TRANSFER_ENCODING_CHUNKED).unwrap_err(); + assert!(matches!(err, ParseError(_))); + assert!(err.to_string().contains("Transfer-Encoding other than identity not supported yet")); + } + + #[test] + fn test_parse_request_transfer_encoding_multiple() { + let err = parse_request(TEST_REQUEST_TRANSFER_ENCODING_MULTIPLE).unwrap_err(); + assert!(matches!(err, ParseError(_))); + assert!(err.to_string().contains("Transfer-Encoding other than identity not supported yet")); + } + + #[test] + fn test_parse_response_transfer_encoding_multiple() { + let err = parse_response(TEST_RESPONSE_TRANSFER_ENCODING_MULTIPLE).unwrap_err(); + assert!(matches!(err, ParseError(_))); + assert!(err.to_string().contains("Transfer-Encoding other than identity not supported yet")); + } } From a1692a3732af72767bae9afe1180321420557a0a Mon Sep 17 00:00:00 2001 From: Justin Martin Date: Fri, 1 Nov 2024 12:12:11 -0700 Subject: [PATCH 2/5] Add changelog entry --- spansy/CHANGELOG.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/spansy/CHANGELOG.md b/spansy/CHANGELOG.md index ef88a6b..6c5e52c 100644 --- a/spansy/CHANGELOG.md +++ b/spansy/CHANGELOG.md @@ -5,3 +5,6 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] + +### Fixed +- Improve handling of Transfer-Encoding errors From 60472fcd1dc54a50211f6c4b97bdbaf207eb94c3 Mon Sep 17 00:00:00 2001 From: Justin Martin Date: Mon, 28 Apr 2025 13:47:56 -0700 Subject: [PATCH 3/5] Check only first header. Use non-empty string to indicate invalid utf-8. Interpolate bad values directly into error message --- spansy/src/http/span.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/spansy/src/http/span.rs b/spansy/src/http/span.rs index eeff909..00f92ba 100644 --- a/spansy/src/http/span.rs +++ b/spansy/src/http/span.rs @@ -222,22 +222,23 @@ fn request_body_len(request: &Request) -> Result { // the Transfer-Encoding overrides the Content-Length if request .headers_with_name("Transfer-Encoding") - .any(|h| { + .next() + .map(|h| { std::str::from_utf8(h.value.0.as_bytes()) .unwrap_or("") .split(',') .any(|v| v.trim() != "identity") }) + .unwrap_or(false) { let bad_values = request .headers_with_name("Transfer-Encoding") - .map(|h| std::str::from_utf8(h.value.0.as_bytes()).unwrap_or("")) + .map(|h| std::str::from_utf8(h.value.0.as_bytes()).unwrap_or("{invalid utf-8}")) .collect::>() .join(", "); Err(ParseError(format!( - "Transfer-Encoding other than identity not supported yet: {:?}", - bad_values + "Transfer-Encoding other than identity not supported yet: {bad_values}" ))) } else if let Some(h) = request.headers_with_name("Content-Length").next() { // If a valid Content-Length header field is present without Transfer-Encoding, its decimal value From 2d807711968e3fc4981d06ae3bb14497234ff6bb Mon Sep 17 00:00:00 2001 From: Justin Martin Date: Mon, 28 Apr 2025 13:57:13 -0700 Subject: [PATCH 4/5] Same for response --- spansy/src/http/span.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/spansy/src/http/span.rs b/spansy/src/http/span.rs index 00f92ba..eec4102 100644 --- a/spansy/src/http/span.rs +++ b/spansy/src/http/span.rs @@ -270,22 +270,23 @@ fn response_body_len(response: &Response) -> Result { if response .headers_with_name("Transfer-Encoding") - .any(|h| { + .next() + .map(|h| { std::str::from_utf8(h.value.0.as_bytes()) .unwrap_or("") .split(',') .any(|v| v.trim() != "identity") }) + .unwrap_or(false) { let bad_values = response .headers_with_name("Transfer-Encoding") - .map(|h| std::str::from_utf8(h.value.0.as_bytes()).unwrap_or("")) + .map(|h| std::str::from_utf8(h.value.0.as_bytes()).unwrap_or("{invalid utf-8}")) .collect::>() .join(", "); Err(ParseError(format!( - "Transfer-Encoding other than identity not supported yet: {:?}", - bad_values + "Transfer-Encoding other than identity not supported yet: {bad_values}" ))) } else if let Some(h) = response.headers_with_name("Content-Length").next() { // If a valid Content-Length header field is present without Transfer-Encoding, its decimal value From 112891e133eacee2b912b985ef71aea68ea84db4 Mon Sep 17 00:00:00 2001 From: Justin Martin Date: Mon, 28 Apr 2025 13:58:14 -0700 Subject: [PATCH 5/5] rustfmt --- spansy/src/http/span.rs | 47 ++++++++++++++++++++++++++--------------- 1 file changed, 30 insertions(+), 17 deletions(-) diff --git a/spansy/src/http/span.rs b/spansy/src/http/span.rs index eec4102..becbb20 100644 --- a/spansy/src/http/span.rs +++ b/spansy/src/http/span.rs @@ -218,8 +218,8 @@ fn request_body_len(request: &Request) -> Result { // The presence of a message body in a request is signaled by a Content-Length // or Transfer-Encoding header field. - // If a message is received with both a Transfer-Encoding and a Content-Length header field, - // the Transfer-Encoding overrides the Content-Length + // If a message is received with both a Transfer-Encoding and a Content-Length + // header field, the Transfer-Encoding overrides the Content-Length if request .headers_with_name("Transfer-Encoding") .next() @@ -241,22 +241,25 @@ fn request_body_len(request: &Request) -> Result { "Transfer-Encoding other than identity not supported yet: {bad_values}" ))) } else if let Some(h) = request.headers_with_name("Content-Length").next() { - // If a valid Content-Length header field is present without Transfer-Encoding, its decimal value - // defines the expected message body length in octets. + // If a valid Content-Length header field is present without Transfer-Encoding, + // its decimal value defines the expected message body length in octets. std::str::from_utf8(h.value.0.as_bytes())? .parse::() .map_err(|err| ParseError(format!("failed to parse Content-Length value: {err}"))) } else { - // If this is a request message and none of the above are true, then the message body length is zero + // If this is a request message and none of the above are true, then the message + // body length is zero Ok(0) } } /// Calculates the length of the response body according to RFC 9112, section 6. fn response_body_len(response: &Response) -> Result { - // Any response to a HEAD request and any response with a 1xx (Informational), 204 (No Content), or 304 (Not Modified) - // status code is always terminated by the first empty line after the header fields, regardless of the header fields - // present in the message, and thus cannot contain a message body or trailer section. + // Any response to a HEAD request and any response with a 1xx (Informational), + // 204 (No Content), or 304 (Not Modified) status code is always terminated + // by the first empty line after the header fields, regardless of the header + // fields present in the message, and thus cannot contain a message body or + // trailer section. match response .status .code @@ -289,16 +292,18 @@ fn response_body_len(response: &Response) -> Result { "Transfer-Encoding other than identity not supported yet: {bad_values}" ))) } else if let Some(h) = response.headers_with_name("Content-Length").next() { - // If a valid Content-Length header field is present without Transfer-Encoding, its decimal value - // defines the expected message body length in octets. + // If a valid Content-Length header field is present without Transfer-Encoding, + // its decimal value defines the expected message body length in octets. std::str::from_utf8(h.value.0.as_bytes())? .parse::() .map_err(|err| ParseError(format!("failed to parse Content-Length value: {err}"))) } else { - // If this is a response message and none of the above are true, then there is no way to - // determine the length of the message body except by reading it until the connection is closed. + // If this is a response message and none of the above are true, then there is + // no way to determine the length of the message body except by reading + // it until the connection is closed. - // We currently consider this an error because we have no outer context information. + // We currently consider this an error because we have no outer context + // information. Err(ParseError( "A response with a body must contain either a Content-Length or Transfer-Encoding header".to_string(), )) @@ -577,27 +582,35 @@ mod tests { fn test_parse_request_transfer_encoding_chunked() { let err = parse_request(TEST_REQUEST_TRANSFER_ENCODING_CHUNKED).unwrap_err(); assert!(matches!(err, ParseError(_))); - assert!(err.to_string().contains("Transfer-Encoding other than identity not supported yet")); + assert!(err + .to_string() + .contains("Transfer-Encoding other than identity not supported yet")); } #[test] fn test_parse_response_transfer_encoding_chunked() { let err = parse_response(TEST_RESPONSE_TRANSFER_ENCODING_CHUNKED).unwrap_err(); assert!(matches!(err, ParseError(_))); - assert!(err.to_string().contains("Transfer-Encoding other than identity not supported yet")); + assert!(err + .to_string() + .contains("Transfer-Encoding other than identity not supported yet")); } #[test] fn test_parse_request_transfer_encoding_multiple() { let err = parse_request(TEST_REQUEST_TRANSFER_ENCODING_MULTIPLE).unwrap_err(); assert!(matches!(err, ParseError(_))); - assert!(err.to_string().contains("Transfer-Encoding other than identity not supported yet")); + assert!(err + .to_string() + .contains("Transfer-Encoding other than identity not supported yet")); } #[test] fn test_parse_response_transfer_encoding_multiple() { let err = parse_response(TEST_RESPONSE_TRANSFER_ENCODING_MULTIPLE).unwrap_err(); assert!(matches!(err, ParseError(_))); - assert!(err.to_string().contains("Transfer-Encoding other than identity not supported yet")); + assert!(err + .to_string() + .contains("Transfer-Encoding other than identity not supported yet")); } }