Unverified Commit a94a5e08 authored by John DiSanti's avatar John DiSanti Committed by GitHub
Browse files

Fix panics in `Headers::try_insert` and `Headers::try_append` (#3157) 

The `try_insert` and `try_append` methods on `Headers` would panic when
passing a non-ASCII value for the key, or an invalid UTF-8 value for the
value. This was due to them using the http crate's `from_static` method,
which panics on these invalid values. This PR makes them always use
`try_from` instead.

----

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

rust-runtime/aws-smithy-runtime-api/Cargo.toml

+1 −0
Original line number Diff line number Diff line
@@ -26,6 +26,7 @@ tracing = "0.1" 
zeroize = { version = "1", optional = true }



[dev-dependencies]

proptest = "1"

tokio = { version = "1.25", features = ["rt", "macros"] }



[package.metadata.docs.rs]

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

+119 −23
Original line number Diff line number Diff line
@@ -92,28 +92,31 @@ impl Headers { 
    /// This will *replace* any existing value for this key. Returns the previous associated value if any.

    ///

    /// # Panics

    /// If the key or value are not valid ascii, this function will panic.

    /// If the key is not valid ASCII, or if the value is not valid UTF-8, this function will panic.

    pub fn insert(

        &mut self,

        key: impl AsHeaderComponent,

        value: impl AsHeaderComponent,

    ) -> Option<String> {

        self.try_insert(key, value)

            .expect("HeaderName or HeaderValue was invalid")

        let key = header_name(key, false).unwrap();

        let value = header_value(value.into_maybe_static().unwrap(), false).unwrap();

        self.headers

            .insert(key, value)

            .map(|old_value| old_value.into())

    }



    /// Insert a value into the headers structure.

    ///

    /// This will *replace* any existing value for this key. Returns the previous associated value if any.

    ///

    /// If the key or value are not valid ascii, an error is returned

    /// If the key is not valid ASCII, or if the value is not valid UTF-8, this function will return an error.

    pub fn try_insert(

        &mut self,

        key: impl AsHeaderComponent,

        value: impl AsHeaderComponent,

    ) -> Result<Option<String>, HttpError> {

        let key = header_name(key)?;

        let value = header_value(value.into_maybe_static()?)?;

        let key = header_name(key, true)?;

        let value = header_value(value.into_maybe_static()?, true)?;

        Ok(self

            .headers

            .insert(key, value)
@@ -122,14 +125,24 @@ impl Headers { 


    /// Appends a value to a given key

    ///

    /// If the key or value are NOT valid ascii, an error is returned

    /// # Panics

    /// If the key is not valid ASCII, or if the value is not valid UTF-8, this function will panic.

    pub fn append(&mut self, key: impl AsHeaderComponent, value: impl AsHeaderComponent) -> bool {

        let key = header_name(key.into_maybe_static().unwrap(), false).unwrap();

        let value = header_value(value.into_maybe_static().unwrap(), false).unwrap();

        self.headers.append(key, value)

    }



    /// Appends a value to a given key

    ///

    /// If the key is not valid ASCII, or if the value is not valid UTF-8, this function will return an error.

    pub fn try_append(

        &mut self,

        key: impl AsHeaderComponent,

        value: impl AsHeaderComponent,

    ) -> Result<bool, HttpError> {

        let key = header_name(key.into_maybe_static()?)?;

        let value = header_value(value.into_maybe_static()?)?;

        let key = header_name(key.into_maybe_static()?, true)?;

        let value = header_value(value.into_maybe_static()?, true)?;

        Ok(self.headers.append(key, value))

    }
@@ -141,15 +154,6 @@ impl Headers { 
            .remove(key.as_ref())

            .map(|h| h.as_str().to_string())

    }



    /// Appends a value to a given key

    ///

    /// # Panics

    /// If the key or value are NOT valid ascii, this function will panic

    pub fn append(&mut self, key: impl AsHeaderComponent, value: impl AsHeaderComponent) -> bool {

        self.try_append(key, value)

            .expect("HeaderName or HeaderValue was invalid")

    }

}



impl TryFrom<HeaderMap> for Headers {
@@ -326,13 +330,19 @@ pub use header_value::HeaderValue; 


type MaybeStatic = Cow<'static, str>;



fn header_name(name: impl AsHeaderComponent) -> Result<http0::HeaderName, HttpError> {

fn header_name(

    name: impl AsHeaderComponent,

    panic_safe: bool,

) -> Result<http0::HeaderName, HttpError> {

    name.repr_as_http02x_header_name().or_else(|name| {

        name.into_maybe_static().and_then(|cow| {

            if cow.chars().any(|c| c.is_uppercase()) {

                return Err(HttpError::new("Header names must be all lower case"));

        name.into_maybe_static().and_then(|mut cow| {

            if cow.chars().any(|c| c.is_ascii_uppercase()) {

                cow = Cow::Owned(cow.to_ascii_uppercase());

            }

            match cow {

                Cow::Borrowed(s) if panic_safe => {

                    http0::HeaderName::try_from(s).map_err(HttpError::invalid_header_name)

                }

                Cow::Borrowed(staticc) => Ok(http0::HeaderName::from_static(staticc)),

                Cow::Owned(s) => {

                    http0::HeaderName::try_from(s).map_err(HttpError::invalid_header_name)
@@ -342,8 +352,11 @@ fn header_name(name: impl AsHeaderComponent) -> Result<http0::HeaderName, HttpEr 
    })

}



fn header_value(value: MaybeStatic) -> Result<HeaderValue, HttpError> {

fn header_value(value: MaybeStatic, panic_safe: bool) -> Result<HeaderValue, HttpError> {

    let header = match value {

        Cow::Borrowed(b) if panic_safe => {

            http0::HeaderValue::try_from(b).map_err(HttpError::invalid_header_value)?

        }

        Cow::Borrowed(b) => http0::HeaderValue::from_static(b),

        Cow::Owned(s) => {

            http0::HeaderValue::try_from(s).map_err(HttpError::invalid_header_value)?
@@ -364,4 +377,87 @@ mod tests { 
            .parse::<HeaderValue>()

            .expect_err("cannot contain control characters");

    }



    #[test]

    fn no_panic_insert_upper_case_header_name() {

        let mut headers = Headers::new();

        headers.insert("I-Have-Upper-Case", "foo");

    }

    #[test]

    fn no_panic_append_upper_case_header_name() {

        let mut headers = Headers::new();

        headers.append("I-Have-Upper-Case", "foo");

    }



    #[test]

    #[should_panic]

    fn panic_insert_invalid_ascii_key() {

        let mut headers = Headers::new();

        headers.insert("💩", "foo");

    }

    #[test]

    #[should_panic]

    fn panic_insert_invalid_header_value() {

        let mut headers = Headers::new();

        headers.insert("foo", "💩");

    }

    #[test]

    #[should_panic]

    fn panic_append_invalid_ascii_key() {

        let mut headers = Headers::new();

        headers.append("💩", "foo");

    }

    #[test]

    #[should_panic]

    fn panic_append_invalid_header_value() {

        let mut headers = Headers::new();

        headers.append("foo", "💩");

    }



    #[test]

    fn no_panic_try_insert_invalid_ascii_key() {

        let mut headers = Headers::new();

        assert!(headers.try_insert("💩", "foo").is_err());

    }

    #[test]

    fn no_panic_try_insert_invalid_header_value() {

        let mut headers = Headers::new();

        assert!(headers

            .try_insert(

                "foo",

                // Valid header value with invalid UTF-8

                http0::HeaderValue::from_bytes(&[0xC0, 0x80]).unwrap()

            )

            .is_err());

    }

    #[test]

    fn no_panic_try_append_invalid_ascii_key() {

        let mut headers = Headers::new();

        assert!(headers.try_append("💩", "foo").is_err());

    }

    #[test]

    fn no_panic_try_append_invalid_header_value() {

        let mut headers = Headers::new();

        assert!(headers

            .try_insert(

                "foo",

                // Valid header value with invalid UTF-8

                http0::HeaderValue::from_bytes(&[0xC0, 0x80]).unwrap()

            )

            .is_err());

    }



    proptest::proptest! {

        #[test]

        fn insert_header_prop_test(input in ".*") {

            let mut headers = Headers::new();

            let _ = headers.try_insert(input.clone(), input);

        }



        #[test]

        fn append_header_prop_test(input in ".*") {

            let mut headers = Headers::new();

            let _ = headers.try_append(input.clone(), input);

        }

    }

}