Improve HTTP header errors (#3779)

This improves error messaging for HTTP header related errors.
----

_By submitting this pull request, I confirm that you can use, modify,
copy, and redistribute this contribution, under the terms of your
choice._

.changelog/1722544572.md

@@ -0,0 +1,12 @@
+---
+applies_to:
+- client
+authors:
+- Velfi
+references:
+- smithy-rs#3779
+breaking: false
+new_feature: false
+bug_fix: false
+---
+Improve error messaging when HTTP headers aren't valid UTF-8

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 <aws-sdk-rust@amazon.com>", "Zelda Hessler <zhessler@amazon.com>"]
 description = "Smithy runtime types."
 edition = "2021"

rust-runtime/aws-smithy-runtime-api/src/http/error.rs

@@ -16,48 +16,140 @@ 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<BoxError>,
+}
+
+#[derive(Debug)]
+enum Kind {
+    InvalidExtensions,
+    InvalidHeaderName,
+    InvalidHeaderValue,
+    InvalidStatusCode,
+    InvalidUri,
+    InvalidUriParts,
+    MissingAuthority,
+    MissingScheme,
+    NonUtf8Header(NonUtf8Header),
+}
+
+#[derive(Debug)]
+pub(super) struct NonUtf8Header {
+    error: Utf8Error,
+    value: Vec<u8>,
+    name: Option<String>,
+}
+
+impl NonUtf8Header {
+    #[cfg(any(feature = "http-1x", feature = "http-02x"))]
+    pub(super) fn new(name: String, value: Vec<u8>, error: Utf8Error) -> Self {
+        Self {
+            error,
+            value,
+            name: Some(name),
+        }
+    }
+
+    pub(super) fn new_missing_name(value: Vec<u8>, error: Utf8Error) -> Self {
+        Self {
+            error,
+            value,
+            name: None,
+        }
+    }
+}
 
 impl HttpError {
-    // TODO(httpRefactor): Add better error internals
-    pub(super) fn new<E: Into<Box<dyn Error + Send + Sync + 'static>>>(err: E) -> Self {
-        HttpError(err.into())
-    }
-
-    #[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_value(err: InvalidHeaderValue) -> Self {
-        Self(err.into())
-    }
-
-    pub(super) fn header_was_not_a_string(err: Utf8Error) -> Self {
-        Self(err.into())
+        Self {
+            kind: Kind::InvalidExtensions,
+            source: None,
+        }
     }
 
     pub(super) fn invalid_header_name(err: InvalidHeaderName) -> Self {
-        Self(err.into())
+        Self {
+            kind: Kind::InvalidHeaderName,
+            source: Some(Box::new(err)),
+        }
     }
 
-    pub(super) fn invalid_uri(err: InvalidUri) -> Self {
-        Self(err.into())
+    pub(super) fn invalid_header_value(err: InvalidHeaderValue) -> Self {
+        Self {
+            kind: Kind::InvalidHeaderValue,
+            source: Some(Box::new(err)),
+        }
     }
 
     pub(super) fn invalid_status_code() -> Self {
-        Self("invalid HTTP status code".into())
+        Self {
+            kind: Kind::InvalidStatusCode,
+            source: None,
+        }
+    }
+
+    pub(super) fn invalid_uri(err: InvalidUri) -> Self {
+        Self {
+            kind: Kind::InvalidUri,
+            source: Some(Box::new(err)),
+        }
+    }
+
+    pub(super) fn invalid_uri_parts(err: http_02x::Error) -> Self {
+        Self {
+            kind: Kind::InvalidUriParts,
+            source: Some(Box::new(err)),
+        }
+    }
+
+    pub(super) fn missing_authority() -> Self {
+        Self {
+            kind: Kind::MissingAuthority,
+            source: None,
+        }
+    }
+
+    pub(super) fn missing_scheme() -> Self {
+        Self {
+            kind: Kind::MissingScheme,
+            source: None,
+        }
+    }
+
+    pub(super) fn non_utf8_header(non_utf8_header: NonUtf8Header) -> Self {
+        Self {
+            kind: Kind::NonUtf8Header(non_utf8_header),
+            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"),
+            NonUtf8Header(hv) => {
+                // In some cases, we won't know the key so we default to "<unknown>".
+                let key = hv.name.as_deref().unwrap_or("<unknown>");
+                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}")
+            },
+        }
     }
 }
 
 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 _)
     }
 }

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,12 +181,12 @@ impl TryFrom<http_02x::HeaderMap> for Headers {
     type Error = HttpError;
 
     fn try_from(value: http_02x::HeaderMap) -> Result<Self, Self::Error> {
-        if let Some(e) = value
-            .values()
-            .filter_map(|value| std::str::from_utf8(value.as_bytes()).err())
-            .next()
-        {
-            Err(HttpError::header_was_not_a_string(e))
+        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<HeaderValue> = Default::default();
             string_safe_headers.extend(
@@ -206,12 +206,12 @@ impl TryFrom<http_1x::HeaderMap> for Headers {
     type Error = HttpError;
 
     fn try_from(value: http_1x::HeaderMap) -> Result<Self, Self::Error> {
-        if let Some(e) = value
-            .values()
-            .filter_map(|value| std::str::from_utf8(value.as_bytes()).err())
-            .next()
-        {
-            Err(HttpError::header_was_not_a_string(e))
+        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<HeaderValue> = Default::default();
             string_safe_headers.extend(value.into_iter().map(|(k, v)| {
@@ -285,13 +285,23 @@ mod sealed {
         fn into_maybe_static(self) -> Result<MaybeStatic, HttpError> {
             Ok(Cow::Owned(
                 std::str::from_utf8(self.as_bytes())
-                    .map_err(HttpError::header_was_not_a_string)?
+                    .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(HttpError::header_was_not_a_string)
+            std::str::from_utf8(self.as_bytes()).map_err(|err| {
+                HttpError::non_utf8_header(NonUtf8Header::new_missing_name(
+                    self.as_bytes().to_vec(),
+                    err,
+                ))
+            })
         }
     }
 
@@ -315,7 +325,6 @@ mod sealed {
 
 mod header_value {
     use super::*;
-    use std::str::Utf8Error;
 
     /// HeaderValue type
     ///
@@ -334,16 +343,26 @@ mod header_value {
 
     impl HeaderValue {
         #[allow(dead_code)]
-        pub(crate) fn from_http02x(value: http_02x::HeaderValue) -> Result<Self, Utf8Error> {
-            let _ = std::str::from_utf8(value.as_bytes())?;
+        pub(crate) fn from_http02x(value: http_02x::HeaderValue) -> Result<Self, HttpError> {
+            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),
             })
         }
 
         #[allow(dead_code)]
-        pub(crate) fn from_http1x(value: http_1x::HeaderValue) -> Result<Self, Utf8Error> {
-            let _ = std::str::from_utf8(value.as_bytes())?;
+        pub(crate) fn from_http1x(value: http_1x::HeaderValue) -> Result<Self, HttpError> {
+            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),
             })
@@ -426,7 +445,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 +464,7 @@ fn header_value(value: MaybeStatic, panic_safe: bool) -> Result<HeaderValue, Htt
             http_02x::HeaderValue::try_from(s).map_err(HttpError::invalid_header_value)?
         }
     };
-    HeaderValue::from_http02x(header).map_err(HttpError::new)
+    HeaderValue::from_http02x(header)
 }
 
 #[cfg(test)]

rust-runtime/aws-smithy-runtime-api/src/http/request.rs

@@ -80,16 +80,14 @@ impl Uri {
         let endpoint = endpoint.into_parts();
         let authority = endpoint
             .authority
-            .ok_or_else(|| HttpError::new("endpoint must contain authority"))?;
-        let scheme = endpoint
-            .scheme
-            .ok_or_else(|| HttpError::new("endpoint must have scheme"))?;
+            .ok_or_else(HttpError::missing_authority)?;
+        let scheme = endpoint.scheme.ok_or_else(HttpError::missing_scheme)?;
         let new_uri = http_02x::Uri::builder()
             .authority(authority)
             .scheme(scheme)
             .path_and_query(merge_paths(endpoint.path_and_query, &self.parsed).as_ref())
             .build()
-            .map_err(HttpError::new)?;
+            .map_err(HttpError::invalid_uri_parts)?;
         self.as_string = new_uri.to_string();
         self.parsed = ParsedUri::H0(new_uri);
         Ok(())