Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve handling of Transfer-Encoding errors #46

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

Improve handling of Transfer-Encoding errors #46

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
dd5fb13
Improve clarity of Transfer-Encoding errors. Don't error on identity TE
TheFrozenFire Nov 1, 2024
b78d6ba
Add changelog entry
TheFrozenFire Nov 1, 2024
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
3 changes: 3 additions & 0 deletions spansy/CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
122 changes: 112 additions & 10 deletions spansy/src/http/span.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,12 +222,23 @@ fn request_body_len(request: &Request) -> Result<usize, ParseError> {
// the Transfer-Encoding overrides the Content-Length
if request
.headers_with_name("Transfer-Encoding")
.next()
.is_some()
.any(|h| {
Copy link
Member

Choose a reason for hiding this comment

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

It would be sufficient to just handle the first header with this key, ie

if let Some(header) = request.headers_with_name("Transfer-Encoding").first()

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(""))
Copy link
Member

Choose a reason for hiding this comment

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

unwrap_or("") will be misleading if the header value is not value utf8. Better to return a parse error indicating that.

.collect::<Vec<_>>()
.join(", ");

Err(ParseError(format!(
"Transfer-Encoding other than identity not supported yet: {:?}",
bad_values
Comment on lines +239 to +240
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"Transfer-Encoding other than identity not supported yet: {:?}",
bad_values
"Transfer-Encoding other than identity not supported yet: {bad_values}"

Copy link
Member

Choose a reason for hiding this comment

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

bad_values is String which implements Display, so no need to format using Debug

)))
} 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.
Expand Down Expand Up @@ -258,12 +269,23 @@ fn response_body_len(response: &Response) -> Result<usize, ParseError> {

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::<Vec<_>>()
.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.
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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"));
}
}
Loading