Unverified Commit 8d09dade authored by david-perez's avatar david-perez Committed by GitHub
Browse files

Fix query string decoding (#2201) 

`serde_urlencoded` can't decode into `Vec<(&str, &str)` query string
slices that need percent decoding.
parent bcea15ad

CHANGELOG.next.toml

+7 −1
Original line number Diff line number Diff line
@@ -10,3 +10,9 @@ 
# references = ["smithy-rs#920"]

# meta = { "breaking" = false, "tada" = false, "bug" = false, "target" = "client | server | all"}

# author = "rcoh"



[[smithy-rs]]

message = "Fix severe bug where a router fails to deserialize percent-encoded query strings, reporting no operation match when there could be one. If your Smithy model uses an operation with a request URI spec containing [query string literals](https://smithy.io/2.0/spec/http-bindings.html#query-string-literals), you are affected. This fix was released in `aws-smithy-http-server` v0.53.1."

references = ["smithy-rs#2201"]

meta = { "breaking" = false, "tada" = false, "bug" = true, "target" = "server"}

author = "david-perez"

rust-runtime/aws-smithy-http-server/src/routing/request_spec.rs

+19 −3
Original line number Diff line number Diff line
@@ -181,13 +181,18 @@ impl RequestSpec { 


        match req.uri().query() {

            Some(query) => {

                // We can't use `HashMap<&str, &str>` because a query string key can appear more

                // We can't use `HashMap<Cow<str>, Cow<str>>` because a query string key can appear more

                // than once e.g. `/?foo=bar&foo=baz`. We _could_ use a multiset e.g. the `hashbag`

                // crate.

                let res = serde_urlencoded::from_str::<Vec<(&str, &str)>>(query);

                // We must deserialize into `Cow<str>`s because `serde_urlencoded` might need to

                // return an owned allocated `String` if it has to percent-decode a slice of the query string.

                let res = serde_urlencoded::from_str::<Vec<(Cow<str>, Cow<str>)>>(query);



                match res {

                    Err(_) => Match::No,

                    Err(error) => {

                        tracing::debug!(query, %error, "failed to deserialize query string");

                        Match::No

                    }

                    Ok(query_map) => {

                        for query_segment in self.uri_spec.path_and_query.query_segments.0.iter() {

                            match query_segment {
@@ -367,6 +372,17 @@ mod tests { 
        );

    }



    #[test]

    fn encoded_query_string() {

        let request_spec =

            RequestSpec::from_parts(Method::DELETE, Vec::new(), vec![QuerySegment::Key("foo".to_owned())]);



        assert_eq!(

            Match::Yes,

            request_spec.matches(&req(&Method::DELETE, "/?foo=hello%20world", None))

        );

    }



    fn ab_spec() -> RequestSpec {

        RequestSpec::from_parts(

            Method::GET,