From fe0b125dd7e31fd1555a8017e97ef7576357f9a7 Mon Sep 17 00:00:00 2001 From: Russell Cohen Date: Thu, 9 Dec 2021 11:57:05 -0500 Subject: [PATCH] Fix label & query URI encoding (#953) * Fix label & query URI encoding https://github.com/awslabs/aws-sdk-rust/issues/331 demonstrated that we were failing to properly encode characters for URI path components and query components in several situation. This: - Fixes the specific bugs - Adds proptests (run locally with 16K cases) to verify that this is the complete set. - Adds an S3-specific protocol test that targets this issue * Make the test a bit stronger * Update changelog --- CHANGELOG.next.toml | 12 ++++++++++++ aws/sdk/aws-models/s3-tests.smithy | 16 ++++++++++++++++ .../proptest-regressions/label.txt | 10 ++++++++++ .../proptest-regressions/query.txt | 9 +++++++++ rust-runtime/aws-smithy-http/src/label.rs | 16 +++++++++++++--- rust-runtime/aws-smithy-http/src/query.rs | 9 +++++++++ rust-runtime/aws-smithy-http/src/urlencode.rs | 7 ++++++- 7 files changed, 75 insertions(+), 4 deletions(-) create mode 100644 rust-runtime/aws-smithy-http/proptest-regressions/label.txt create mode 100644 rust-runtime/aws-smithy-http/proptest-regressions/query.txt diff --git a/CHANGELOG.next.toml b/CHANGELOG.next.toml index 61bc314a6..94c12e757 100644 --- a/CHANGELOG.next.toml +++ b/CHANGELOG.next.toml @@ -129,3 +129,15 @@ message = "The features `aws-hyper/rustls` and `aws-hyper/native-tls` have been meta = { "breaking" = true, "tada" = false, "bug" = false } references = ["smithy-rs#947"] author = "rcoh" + +[[aws-sdk-rust]] +message = "Fixed a bug where certain characters caused a panic during URI encoding." +meta = { "breaking" = false, "tada" = false, "bug" = true } +references = ["smithy-rs#953", "aws-sdk-rust#331"] +author = "rcoh" + +[[smithy-rs]] +message = "Fixed a bug where certain characters caused a panic during URI encoding." +meta = { "breaking" = false, "tada" = false, "bug" = true } +references = ["smithy-rs#953", "aws-sdk-rust#331"] +author = "rcoh" diff --git a/aws/sdk/aws-models/s3-tests.smithy b/aws/sdk/aws-models/s3-tests.smithy index 60368d060..effcb8698 100644 --- a/aws/sdk/aws-models/s3-tests.smithy +++ b/aws/sdk/aws-models/s3-tests.smithy @@ -24,6 +24,7 @@ apply NotFound @httpResponseTests([ } ]) + apply GetBucketLocation @httpResponseTests([ { id: "GetBucketLocation", @@ -183,3 +184,18 @@ apply PutObject @httpRequestTests([ } } ]) + +apply HeadObject @httpRequestTests([ + { + id: "HeadObjectUriEncoding", + documentation: "https://github.com/awslabs/aws-sdk-rust/issues/331", + + method: "HEAD", + protocol: "aws.protocols#restXml", + uri: "/test-bucket/%3C%3E%20%60%3F%F0%9F%90%B1", + params: { + Bucket: "test-bucket", + Key: "<> `?🐱", + } + } +]) diff --git a/rust-runtime/aws-smithy-http/proptest-regressions/label.txt b/rust-runtime/aws-smithy-http/proptest-regressions/label.txt new file mode 100644 index 000000000..355d10946 --- /dev/null +++ b/rust-runtime/aws-smithy-http/proptest-regressions/label.txt @@ -0,0 +1,10 @@ +# Seeds for failure cases proptest has generated in the past. It is +# automatically read and these particular cases re-run before any +# novel cases are generated. +# +# It is recommended to check this file in to source control so that +# everyone who runs the test benefits from these saved cases. +cc dfac816a3fc3fff8523f1a1707da0065b72fc3c0d70fce001627a8e2e7ee5e0e # shrinks to s = ">" +cc 22bce3cd581f5f5a55e6ba18b1fb027481a496f6b35fee6dc4ef84659b99ddca # shrinks to s = "`" +cc be619cccfee48e3bf642cf0f82e98e00dceccbe10963fbaf3a622a68a55a3227 # shrinks to s = "?\"" +cc 3e0b2e6f64642d7c58e5d2fe9223f75238a874bd8c3812dcb3ecc721d9aa0243 # shrinks to s = " " diff --git a/rust-runtime/aws-smithy-http/proptest-regressions/query.txt b/rust-runtime/aws-smithy-http/proptest-regressions/query.txt new file mode 100644 index 000000000..a33785243 --- /dev/null +++ b/rust-runtime/aws-smithy-http/proptest-regressions/query.txt @@ -0,0 +1,9 @@ +# Seeds for failure cases proptest has generated in the past. It is +# automatically read and these particular cases re-run before any +# novel cases are generated. +# +# It is recommended to check this file in to source control so that +# everyone who runs the test benefits from these saved cases. +cc b8ff8401495a7e4b4604f4438d8fc6b0ba63a58ddf58273ddcb3bb511e5cf91a # shrinks to s = "<" +cc 59ee40f6a097f80254a91d0ee7d6cde97a353f7ccdf83eddd1d437781019431f # shrinks to s = "\"" +cc 65e6e5f9082c6cbebf599af889721d30d8ee2388f2f7be372520aa86526c8379 # shrinks to s = ">" diff --git a/rust-runtime/aws-smithy-http/src/label.rs b/rust-runtime/aws-smithy-http/src/label.rs index 5b445f0a7..6d1065a6b 100644 --- a/rust-runtime/aws-smithy-http/src/label.rs +++ b/rust-runtime/aws-smithy-http/src/label.rs @@ -6,15 +6,15 @@ //! Formatting values as Smithy //! [httpLabel](https://awslabs.github.io/smithy/1.0/spec/core/http-traits.html#httplabel-trait) -use crate::urlencode::BASE_SET; +use crate::urlencode::LABEL_SET; use aws_smithy_types::date_time::{DateTimeFormatError, Format}; use aws_smithy_types::DateTime; use percent_encoding::AsciiSet; -const GREEDY: &AsciiSet = &BASE_SET.remove(b'/'); +const GREEDY: &AsciiSet = &LABEL_SET.remove(b'/'); pub fn fmt_string>(t: T, greedy: bool) -> String { - let uri_set = if greedy { GREEDY } else { BASE_SET }; + let uri_set = if greedy { GREEDY } else { LABEL_SET }; percent_encoding::utf8_percent_encode(t.as_ref(), uri_set).to_string() } @@ -25,10 +25,20 @@ pub fn fmt_timestamp(t: &DateTime, format: Format) -> Result Writer<'a> { #[cfg(test)] mod test { use crate::query::{fmt_string, Writer}; + use http::Uri; + use proptest::proptest; #[test] fn url_encode() { @@ -79,4 +81,11 @@ mod test { writer.push_kv("b", "c"); assert_eq!(out, "?a&b=c"); } + + proptest! { + #[test] + fn test_encode_request(s: String) { + let _: Uri = format!("http://host.example.com/?{}", fmt_string(&s)).parse().expect("all strings should be encoded properly"); + } + } } diff --git a/rust-runtime/aws-smithy-http/src/urlencode.rs b/rust-runtime/aws-smithy-http/src/urlencode.rs index 870f190ed..e41f8da1f 100644 --- a/rust-runtime/aws-smithy-http/src/urlencode.rs +++ b/rust-runtime/aws-smithy-http/src/urlencode.rs @@ -30,7 +30,12 @@ pub const BASE_SET: &AsciiSet = &CONTROLS .add(b'+') .add(b';') .add(b'=') - .add(b'%'); + .add(b'%') + .add(b'<') + .add(b'>') + .add(b'"'); + +pub const LABEL_SET: &AsciiSet = &BASE_SET.add(b'`').add(b'?').add(b' '); #[cfg(test)] mod test { -- GitLab