From b9acf1790f1676cdd1f04224b18856d6ddf28a6a Mon Sep 17 00:00:00 2001 From: Zelda Hessler Date: Thu, 1 Aug 2024 15:40:47 -0500 Subject: [PATCH 1/4] improve HTTP header errors --- .changelog/1722544572.md | 12 +++ .../aws-smithy-runtime-api/Cargo.toml | 2 +- .../aws-smithy-runtime-api/src/http/error.rs | 101 ++++++++++++++---- .../src/http/headers.rs | 34 +++--- .../src/http/request.rs | 8 +- 5 files changed, 116 insertions(+), 41 deletions(-) create mode 100644 .changelog/1722544572.md diff --git a/.changelog/1722544572.md b/.changelog/1722544572.md new file mode 100644 index 0000000000..23860f6af2 --- /dev/null +++ b/.changelog/1722544572.md @@ -0,0 +1,12 @@ +--- +applies_to: +- client +authors: +- Velfi +references: +- smithy-rs +breaking: false +new_feature: false +bug_fix: false +--- +Improve error messaging when HTTP headers aren't valid UTF-8 diff --git a/rust-runtime/aws-smithy-runtime-api/Cargo.toml b/rust-runtime/aws-smithy-runtime-api/Cargo.toml index 2b2761d800..45f8df686d 100644 --- a/rust-runtime/aws-smithy-runtime-api/Cargo.toml +++ b/rust-runtime/aws-smithy-runtime-api/Cargo.toml @@ -1,6 +1,6 @@ [package] name = "aws-smithy-runtime-api" -version = "1.7.1" +version = "1.7.2" authors = ["AWS Rust SDK Team ", "Zelda Hessler "] description = "Smithy runtime types." edition = "2021" diff --git a/rust-runtime/aws-smithy-runtime-api/src/http/error.rs b/rust-runtime/aws-smithy-runtime-api/src/http/error.rs index 97c946d7d6..7d9eba9283 100644 --- a/rust-runtime/aws-smithy-runtime-api/src/http/error.rs +++ b/rust-runtime/aws-smithy-runtime-api/src/http/error.rs @@ -16,48 +16,113 @@ use std::str::Utf8Error; /// An error occurred constructing an Http Request. /// /// This is normally due to configuration issues, internal SDK bugs, or other user error. -pub struct HttpError(BoxError); +pub struct HttpError { + kind: Kind, + source: Option, +} + +#[derive(Debug)] +enum Kind { + InvalidExtensions, + InvalidHeaderName, + InvalidHeaderValue, + InvalidStatusCode, + InvalidUri, + InvalidUriParts, + MissingAuthority, + MissingScheme, + NotUtf8(Vec), +} impl HttpError { - // TODO(httpRefactor): Add better error internals - pub(super) fn new>>(err: E) -> Self { - HttpError(err.into()) + pub(super) fn invalid_extensions() -> Self { + Self { + kind: Kind::InvalidExtensions, + source: None, + } } - #[allow(dead_code)] - pub(super) fn invalid_extensions() -> Self { - Self("Extensions were provided during initialization. This prevents the request format from being converted.".into()) + pub(super) fn invalid_header_name(err: InvalidHeaderName) -> Self { + Self { + kind: Kind::InvalidHeaderName, + source: Some(Box::new(err)), + } } pub(super) fn invalid_header_value(err: InvalidHeaderValue) -> Self { - Self(err.into()) + Self { + kind: Kind::InvalidHeaderValue, + source: Some(Box::new(err)), + } + } + + pub(super) fn invalid_status_code() -> Self { + Self { + kind: Kind::InvalidStatusCode, + source: None, + } } - pub(super) fn header_was_not_a_string(err: Utf8Error) -> Self { - Self(err.into()) + pub(super) fn invalid_uri(err: InvalidUri) -> Self { + Self { + kind: Kind::InvalidUri, + source: Some(Box::new(err)), + } } - pub(super) fn invalid_header_name(err: InvalidHeaderName) -> Self { - Self(err.into()) + pub(super) fn invalid_uri_parts(err: http_02x::Error) -> Self { + Self { + kind: Kind::InvalidUriParts, + source: Some(Box::new(err)), + } } - pub(super) fn invalid_uri(err: InvalidUri) -> Self { - Self(err.into()) + pub(super) fn missing_authority() -> Self { + Self { + kind: Kind::MissingAuthority, + source: None, + } } - pub(super) fn invalid_status_code() -> Self { - Self("invalid HTTP status code".into()) + pub(super) fn missing_scheme() -> Self { + Self { + kind: Kind::MissingScheme, + source: None, + } + } + + pub(super) fn not_utf8(header_value_bytes: &[u8]) -> Self { + Self { + kind: Kind::NotUtf8(header_value_bytes.to_owned()), + source: None, + } } } impl Display for HttpError { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - write!(f, "an error occurred creating an HTTP Request") + use Kind::*; + match &self.kind { + InvalidExtensions => write!(f, "Extensions were provided during initialization. This prevents the request format from being converted."), + InvalidHeaderName => write!(f, "invalid header name"), + InvalidHeaderValue => write!(f, "invalid header value"), + InvalidStatusCode => write!(f, "invalid HTTP status code"), + InvalidUri => write!(f, "endpoint is not a valid URI"), + InvalidUriParts => write!(f, "endpoint parts are not valid"), + MissingAuthority => write!(f, "endpoint must contain authority"), + MissingScheme => write!(f, "endpoint must contain scheme"), + NotUtf8(hv) => { + let utf8_err: Utf8Error = std::str::from_utf8(hv).expect_err("we've already proven the value is not UTF-8"); + let hv = String::from_utf8_lossy(hv); + let problem_char = utf8_err.valid_up_to(); + write!(f, "header value `{hv}` contains non-UTF8 octet at index {problem_char}") + }, + } } } impl Error for HttpError { fn source(&self) -> Option<&(dyn Error + 'static)> { - Some(self.0.as_ref()) + self.source.as_ref().map(|err| err.as_ref() as _) } } diff --git a/rust-runtime/aws-smithy-runtime-api/src/http/headers.rs b/rust-runtime/aws-smithy-runtime-api/src/http/headers.rs index 79ab2668b1..84979d3280 100644 --- a/rust-runtime/aws-smithy-runtime-api/src/http/headers.rs +++ b/rust-runtime/aws-smithy-runtime-api/src/http/headers.rs @@ -181,12 +181,11 @@ impl TryFrom for Headers { type Error = HttpError; fn try_from(value: http_02x::HeaderMap) -> Result { - if let Some(e) = value + if let Some(not_utf8) = value .values() - .filter_map(|value| std::str::from_utf8(value.as_bytes()).err()) - .next() + .find(|value| std::str::from_utf8(value.as_bytes()).is_err()) { - Err(HttpError::header_was_not_a_string(e)) + Err(HttpError::not_utf8(not_utf8.as_bytes())) } else { let mut string_safe_headers: http_02x::HeaderMap = Default::default(); string_safe_headers.extend( @@ -206,12 +205,11 @@ impl TryFrom for Headers { type Error = HttpError; fn try_from(value: http_1x::HeaderMap) -> Result { - if let Some(e) = value + if let Some(not_utf8) = value .values() - .filter_map(|value| std::str::from_utf8(value.as_bytes()).err()) - .next() + .find(|value| std::str::from_utf8(value.as_bytes()).is_err()) { - Err(HttpError::header_was_not_a_string(e)) + Err(HttpError::not_utf8(not_utf8.as_bytes())) } else { let mut string_safe_headers: http_02x::HeaderMap = Default::default(); string_safe_headers.extend(value.into_iter().map(|(k, v)| { @@ -285,13 +283,14 @@ mod sealed { fn into_maybe_static(self) -> Result { Ok(Cow::Owned( std::str::from_utf8(self.as_bytes()) - .map_err(HttpError::header_was_not_a_string)? + .map_err(|_err| HttpError::not_utf8(self.as_bytes()))? .to_string(), )) } fn as_str(&self) -> Result<&str, HttpError> { - std::str::from_utf8(self.as_bytes()).map_err(HttpError::header_was_not_a_string) + std::str::from_utf8(self.as_bytes()) + .map_err(|_err| HttpError::not_utf8(self.as_bytes())) } } @@ -315,7 +314,6 @@ mod sealed { mod header_value { use super::*; - use std::str::Utf8Error; /// HeaderValue type /// @@ -334,16 +332,18 @@ mod header_value { impl HeaderValue { #[allow(dead_code)] - pub(crate) fn from_http02x(value: http_02x::HeaderValue) -> Result { - let _ = std::str::from_utf8(value.as_bytes())?; + pub(crate) fn from_http02x(value: http_02x::HeaderValue) -> Result { + let _ = std::str::from_utf8(value.as_bytes()) + .map_err(|_err| HttpError::not_utf8(value.as_bytes()))?; Ok(Self { _private: Inner::H0(value), }) } #[allow(dead_code)] - pub(crate) fn from_http1x(value: http_1x::HeaderValue) -> Result { - let _ = std::str::from_utf8(value.as_bytes())?; + pub(crate) fn from_http1x(value: http_1x::HeaderValue) -> Result { + let _ = std::str::from_utf8(value.as_bytes()) + .map_err(|_err| HttpError::not_utf8(value.as_bytes()))?; Ok(Self { _private: Inner::H1(value), }) @@ -426,7 +426,7 @@ fn header_name( Cow::Borrowed(s) if panic_safe => { http_02x::HeaderName::try_from(s).map_err(HttpError::invalid_header_name) } - Cow::Borrowed(staticc) => Ok(http_02x::HeaderName::from_static(staticc)), + Cow::Borrowed(static_s) => Ok(http_02x::HeaderName::from_static(static_s)), Cow::Owned(s) => { http_02x::HeaderName::try_from(s).map_err(HttpError::invalid_header_name) } @@ -445,7 +445,7 @@ fn header_value(value: MaybeStatic, panic_safe: bool) -> Result Date: Thu, 1 Aug 2024 15:42:28 -0500 Subject: [PATCH 2/4] update CHANGELOG.next.toml --- .changelog/1722544572.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changelog/1722544572.md b/.changelog/1722544572.md index 23860f6af2..a3d00cb34f 100644 --- a/.changelog/1722544572.md +++ b/.changelog/1722544572.md @@ -4,7 +4,7 @@ applies_to: authors: - Velfi references: -- smithy-rs +- smithy-rs#3779 breaking: false new_feature: false bug_fix: false From 2d44023a91a97ff857dc2cbfd5787fed9d814d5e Mon Sep 17 00:00:00 2001 From: Zelda Hessler Date: Wed, 7 Aug 2024 16:22:23 -0500 Subject: [PATCH 3/4] make non-utf8 header error even more helpful --- .../aws-smithy-runtime-api/src/http/error.rs | 42 +++++++++++--- .../src/http/headers.rs | 55 +++++++++++++------ 2 files changed, 71 insertions(+), 26 deletions(-) diff --git a/rust-runtime/aws-smithy-runtime-api/src/http/error.rs b/rust-runtime/aws-smithy-runtime-api/src/http/error.rs index 7d9eba9283..a16cfc1a0b 100644 --- a/rust-runtime/aws-smithy-runtime-api/src/http/error.rs +++ b/rust-runtime/aws-smithy-runtime-api/src/http/error.rs @@ -31,7 +31,32 @@ enum Kind { InvalidUriParts, MissingAuthority, MissingScheme, - NotUtf8(Vec), + NonUtf8Header(NonUtf8Header), +} + +#[derive(Debug)] +pub(super) struct NonUtf8Header { + error: Utf8Error, + value: Vec, + name: Option, +} + +impl NonUtf8Header { + pub(super) fn new(name: String, value: Vec, error: Utf8Error) -> Self { + Self { + error, + value, + name: Some(name), + } + } + + pub(super) fn new_missing_name(value: Vec, error: Utf8Error) -> Self { + Self { + error, + value, + name: None, + } + } } impl HttpError { @@ -91,9 +116,9 @@ impl HttpError { } } - pub(super) fn not_utf8(header_value_bytes: &[u8]) -> Self { + pub(super) fn non_utf8_header(non_utf8_header: NonUtf8Header) -> Self { Self { - kind: Kind::NotUtf8(header_value_bytes.to_owned()), + kind: Kind::NonUtf8Header(non_utf8_header), source: None, } } @@ -111,11 +136,12 @@ impl Display for HttpError { InvalidUriParts => write!(f, "endpoint parts are not valid"), MissingAuthority => write!(f, "endpoint must contain authority"), MissingScheme => write!(f, "endpoint must contain scheme"), - NotUtf8(hv) => { - let utf8_err: Utf8Error = std::str::from_utf8(hv).expect_err("we've already proven the value is not UTF-8"); - let hv = String::from_utf8_lossy(hv); - let problem_char = utf8_err.valid_up_to(); - write!(f, "header value `{hv}` contains non-UTF8 octet at index {problem_char}") + NonUtf8Header(hv) => { + // In some cases, we won't know the key so we default to "". + let key = hv.name.as_deref().unwrap_or(""); + let value = String::from_utf8_lossy(&hv.value); + let index = hv.error.valid_up_to(); + write!(f, "header `{key}={value}` contains non-UTF8 octet at index {index}") }, } } diff --git a/rust-runtime/aws-smithy-runtime-api/src/http/headers.rs b/rust-runtime/aws-smithy-runtime-api/src/http/headers.rs index 84979d3280..98e571b8c2 100644 --- a/rust-runtime/aws-smithy-runtime-api/src/http/headers.rs +++ b/rust-runtime/aws-smithy-runtime-api/src/http/headers.rs @@ -5,7 +5,7 @@ //! Types for HTTP headers -use crate::http::error::HttpError; +use crate::http::error::{HttpError, NonUtf8Header}; use std::borrow::Cow; use std::fmt::Debug; use std::str::FromStr; @@ -181,11 +181,12 @@ impl TryFrom for Headers { type Error = HttpError; fn try_from(value: http_02x::HeaderMap) -> Result { - if let Some(not_utf8) = value - .values() - .find(|value| std::str::from_utf8(value.as_bytes()).is_err()) - { - Err(HttpError::not_utf8(not_utf8.as_bytes())) + if let Some(utf8_error) = value.iter().find_map(|(k, v)| { + std::str::from_utf8(v.as_bytes()) + .err() + .map(|err| NonUtf8Header::new(k.as_str().to_owned(), v.as_bytes().to_vec(), err)) + }) { + Err(HttpError::non_utf8_header(utf8_error)) } else { let mut string_safe_headers: http_02x::HeaderMap = Default::default(); string_safe_headers.extend( @@ -205,11 +206,12 @@ impl TryFrom for Headers { type Error = HttpError; fn try_from(value: http_1x::HeaderMap) -> Result { - if let Some(not_utf8) = value - .values() - .find(|value| std::str::from_utf8(value.as_bytes()).is_err()) - { - Err(HttpError::not_utf8(not_utf8.as_bytes())) + if let Some(utf8_error) = value.iter().find_map(|(k, v)| { + std::str::from_utf8(v.as_bytes()) + .err() + .map(|err| NonUtf8Header::new(k.as_str().to_owned(), v.as_bytes().to_vec(), err)) + }) { + Err(HttpError::non_utf8_header(utf8_error)) } else { let mut string_safe_headers: http_02x::HeaderMap = Default::default(); string_safe_headers.extend(value.into_iter().map(|(k, v)| { @@ -283,14 +285,23 @@ mod sealed { fn into_maybe_static(self) -> Result { Ok(Cow::Owned( std::str::from_utf8(self.as_bytes()) - .map_err(|_err| HttpError::not_utf8(self.as_bytes()))? + .map_err(|err| { + HttpError::non_utf8_header(NonUtf8Header::new_missing_name( + self.as_bytes().to_vec(), + err, + )) + })? .to_string(), )) } fn as_str(&self) -> Result<&str, HttpError> { - std::str::from_utf8(self.as_bytes()) - .map_err(|_err| HttpError::not_utf8(self.as_bytes())) + std::str::from_utf8(self.as_bytes()).map_err(|err| { + HttpError::non_utf8_header(NonUtf8Header::new_missing_name( + self.as_bytes().to_vec(), + err, + )) + }) } } @@ -333,8 +344,12 @@ mod header_value { impl HeaderValue { #[allow(dead_code)] pub(crate) fn from_http02x(value: http_02x::HeaderValue) -> Result { - let _ = std::str::from_utf8(value.as_bytes()) - .map_err(|_err| HttpError::not_utf8(value.as_bytes()))?; + let _ = std::str::from_utf8(value.as_bytes()).map_err(|err| { + HttpError::non_utf8_header(NonUtf8Header::new_missing_name( + value.as_bytes().to_vec(), + err, + )) + })?; Ok(Self { _private: Inner::H0(value), }) @@ -342,8 +357,12 @@ mod header_value { #[allow(dead_code)] pub(crate) fn from_http1x(value: http_1x::HeaderValue) -> Result { - let _ = std::str::from_utf8(value.as_bytes()) - .map_err(|_err| HttpError::not_utf8(value.as_bytes()))?; + let _ = std::str::from_utf8(value.as_bytes()).map_err(|err| { + HttpError::non_utf8_header(NonUtf8Header::new_missing_name( + value.as_bytes().to_vec(), + err, + )) + })?; Ok(Self { _private: Inner::H1(value), }) From 1c7111b88b66b8b58791f3009c11ce941f6b25a2 Mon Sep 17 00:00:00 2001 From: Zelda Hessler Date: Wed, 7 Aug 2024 16:38:34 -0500 Subject: [PATCH 4/4] add missing feature gate --- rust-runtime/aws-smithy-runtime-api/src/http/error.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/rust-runtime/aws-smithy-runtime-api/src/http/error.rs b/rust-runtime/aws-smithy-runtime-api/src/http/error.rs index a16cfc1a0b..68b2c0cf6c 100644 --- a/rust-runtime/aws-smithy-runtime-api/src/http/error.rs +++ b/rust-runtime/aws-smithy-runtime-api/src/http/error.rs @@ -42,6 +42,7 @@ pub(super) struct NonUtf8Header { } impl NonUtf8Header { + #[cfg(any(feature = "http-1x", feature = "http-02x"))] pub(super) fn new(name: String, value: Vec, error: Utf8Error) -> Self { Self { error,