Redo url encoding (#368)

* Redo URL encoding to cover all cases

* Fix test that missed a couple of encodings

* Should have been escaping !

aws/rust-runtime/aws-hyper/src/test_connection.rs

@@ -118,7 +118,7 @@ impl<B: Into<hyper::Body>> tower::Service<http::Request<SdkBody>> for TestConnec
             self.requests
                 .lock()
                 .unwrap()
-                .push(ValidateRequest { actual, expected });
+                .push(ValidateRequest { expected, actual });
             std::future::ready(Ok(resp.map(|body| SdkBody::from(body.into()))))
         } else {
             std::future::ready(Err("No more data".into()))

aws/sdk-codegen/src/main/kotlin/software/amazon/smithy/rustsdk/FluentClientGenerator.kt

@@ -107,7 +107,7 @@ class FluentClientGenerator(protocolConfig: ProtocolConfig) {
 
                 pub fn from_conf_conn(conf: crate::Config, conn: #{aws_hyper}::conn::Standard) -> Self {
                     let client = #{aws_hyper}::Client::new(conn);
-                    Self { handle: std::sync::Arc::new(Handle { conf, client })}
+                    Self { handle: std::sync::Arc::new(Handle { client, conf })}
                 }
 
                 pub fn conf(&self) -> &crate::Config {

codegen-test/model/rest-json-extras.smithy

@@ -15,7 +15,7 @@ apply QueryPrecedence @httpRequestTests([
         method: "POST",
         uri: "/Precedence",
         body: "",
-        queryParams: ["bar=%26%F0%9F%90%B1", "hello%20there=how's%20your%20encoding?", "a%20%26%20b%20%26%20c=better%20encode%20%3D%20this"],
+        queryParams: ["bar=%26%F0%9F%90%B1", "hello%20there=how%27s%20your%20encoding%3F", "a%20%26%20b%20%26%20c=better%20encode%20%3D%20this"],
         params: {
             foo: "&🐱",
             baz: {

codegen/src/test/kotlin/software/amazon/smithy/rust/codegen/generators/http/RequestBindingGeneratorTest.kt

@@ -153,7 +153,7 @@ class RequestBindingGeneratorTest {
             assert_eq!(o.as_str(), "/somebucket%2Fok/1970-04-28T03%3A58%3A45Z");
             o.clear();
             inp.uri_query(&mut o);
-            assert_eq!(o.as_str(), "?paramName=svq!!%25%26&hello=0&hello=1&hello=2&hello=44")
+            assert_eq!(o.as_str(), "?paramName=svq%21%21%25%26&hello=0&hello=1&hello=2&hello=44")
             """
         )
     }

rust-runtime/smithy-http/Cargo.toml

@@ -12,6 +12,7 @@ http-body = "0.4.0"
 http = "0.2.3"
 thiserror = "1"
 pin-project = "1"
+percent-encoding = "2.1.0"
 
 # We are using hyper for our streaming body implementation, but this is an internal detail.
 hyper = "0.14.5"

rust-runtime/smithy-http/src/label.rs

@@ -3,24 +3,36 @@
  * SPDX-License-Identifier: Apache-2.0.
  */
 
-/// Formatting values as Smithy
-/// [httpLabel](https://awslabs.github.io/smithy/1.0/spec/core/http-traits.html#httplabel-trait)
+//! 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 percent_encoding::AsciiSet;
 use smithy_types::Instant;
 use std::fmt::Debug;
 
+const GREEDY: &AsciiSet = &BASE_SET.remove(b'/');
+
 pub fn fmt_default<T: Debug>(t: T) -> String {
     format!("{:?}", t)
 }
 
 pub fn fmt_string<T: AsRef<str>>(t: T, greedy: bool) -> String {
-    let s = t.as_ref().replace(":", "%3A");
-    if greedy {
-        s
-    } else {
-        s.replace("/", "%2F")
-    }
+    let uri_set = if greedy { GREEDY } else { BASE_SET };
+    percent_encoding::utf8_percent_encode(t.as_ref(), &uri_set).to_string()
 }
 
 pub fn fmt_timestamp(t: &Instant, format: smithy_types::instant::Format) -> String {
     crate::query::fmt_string(t.fmt(format))
 }
+
+#[cfg(test)]
+mod test {
+    use crate::label::fmt_string;
+
+    #[test]
+    fn greedy_params() {
+        assert_eq!(fmt_string("a/b", false), "a%2Fb");
+        assert_eq!(fmt_string("a/b", true), "a/b");
+    }
+}

rust-runtime/smithy-http/src/lib.rs

@@ -17,3 +17,4 @@ pub mod query;
 pub mod response;
 pub mod result;
 pub mod retry;
+mod urlencode;

rust-runtime/smithy-http/src/query.rs

@@ -3,80 +3,25 @@
  * SPDX-License-Identifier: Apache-2.0.
  */
 
+use crate::urlencode::BASE_SET;
+use percent_encoding::utf8_percent_encode;
 /// Formatting values into the query string as specified in
 /// [httpQuery](https://awslabs.github.io/smithy/1.0/spec/core/http-traits.html#httpquery-trait)
 use smithy_types::Instant;
 use std::fmt::Debug;
 
-const HEX_CHARS: &[u8; 16] = b"0123456789ABCDEF";
-
 pub fn fmt_default<T: Debug>(t: T) -> String {
     format!("{:?}", t)
 }
 
 pub fn fmt_string<T: AsRef<str>>(t: T) -> String {
-    let bytes = t.as_ref();
-    let final_capacity = bytes
-        .chars()
-        .map(|c| {
-            if is_valid_query(c) {
-                1
-            } else {
-                c.len_utf8() * 3
-            }
-        })
-        .sum();
-    let mut out = String::with_capacity(final_capacity);
-    for char in bytes.chars() {
-        url_encode(char, &mut out);
-    }
-    debug_assert_eq!(out.capacity(), final_capacity);
-    out
+    utf8_percent_encode(t.as_ref(), BASE_SET).to_string()
 }
 
 pub fn fmt_timestamp(t: &Instant, format: smithy_types::instant::Format) -> String {
     fmt_string(t.fmt(format))
 }
 
-fn is_valid_query(c: char) -> bool {
-    // Although & / = are allowed in the query string, we want to escape them
-    let explicit_invalid = |c: char| !matches!(c, '&' | '=');
-    let unreserved = |c: char| c.is_alphanumeric() || matches!(c, '-' | '.' | '_' | '~');
-
-    // RFC-3986 §3.3 allows sub-delims (defined in section2.2) to be in the path component.
-    // This includes both colon ':' and comma ',' characters.
-    // Smithy protocol tests percent encode these expected values though whereas `encodeUrlPath()`
-    // does not and follows the RFC. Fixing the tests was discussed but would adversely affect
-    // other SDK's and we were asked to work around it.
-    // Replace any left over sub-delims with the percent encoded value so that tests can proceed
-    // https://tools.ietf.org/html/rfc3986#section-3.3
-
-    let sub_delims = |c: char| match c {
-        '!' | '$' | '\'' | '(' | ')' | '*' | '+' | /*',' |*/ ';' => true,
-        // TODO: should &/= be url encoded?
-        '&' | '=' => false,
-        _ => false,
-    };
-    let p_char = |c: char| unreserved(c) || sub_delims(c) || /* c == ':' || */ c == '@';
-    explicit_invalid(c) && (p_char(c) || c == '/' || c == '?')
-}
-
-fn url_encode(c: char, buff: &mut String) {
-    if is_valid_query(c) {
-        buff.push(c)
-    } else {
-        let mut inner_buff = [0; 4];
-        let u8_slice = c.encode_utf8(&mut inner_buff).as_bytes();
-        for c in u8_slice {
-            let upper = (c & 0xf0) >> 4;
-            let lower = c & 0x0f;
-            buff.push('%');
-            buff.push(HEX_CHARS[upper as usize] as char);
-            buff.push(HEX_CHARS[lower as usize] as char);
-        }
-    }
-}
-
 /// Simple abstraction to enable appending params to a string as query params
 ///
 /// ```rust
@@ -113,22 +58,16 @@ impl<'a> Writer<'a> {
 
 #[cfg(test)]
 mod test {
-    use crate::query::{fmt_string, is_valid_query};
-
-    #[test]
-    fn test_valid_query_chars() {
-        assert_eq!(is_valid_query(' '), false);
-        assert_eq!(is_valid_query('a'), true);
-        assert_eq!(is_valid_query('/'), true);
-        assert_eq!(is_valid_query('%'), false);
-    }
+    use crate::query::fmt_string;
 
     #[test]
-    fn test_url_encode() {
+    fn url_encode() {
         assert_eq!(fmt_string("y̆").as_str(), "y%CC%86");
         assert_eq!(fmt_string(" ").as_str(), "%20");
-        assert_eq!(fmt_string("foo/baz%20").as_str(), "foo/baz%2520");
+        assert_eq!(fmt_string("foo/baz%20").as_str(), "foo%2Fbaz%2520");
         assert_eq!(fmt_string("&=").as_str(), "%26%3D");
         assert_eq!(fmt_string("🐱").as_str(), "%F0%9F%90%B1");
+        // `:` needs to be encoded, but only for AWS services
+        assert_eq!(fmt_string("a:b"), "a%3Ab")
     }
 }

rust-runtime/smithy-http/src/urlencode.rs

@@ -0,0 +1,34 @@
+/*
+ * Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
+ * SPDX-License-Identifier: Apache-2.0.
+ */
+
+use percent_encoding::{AsciiSet, CONTROLS};
+
+/// base set of characters that must be URL encoded
+pub const BASE_SET: &AsciiSet = &CONTROLS
+    .add(b' ')
+    .add(b'/')
+    // RFC-3986 §3.3 allows sub-delims (defined in section2.2) to be in the path component.
+    // This includes both colon ':' and comma ',' characters.
+    // Smithy protocol tests & AWS services percent encode these expected values. Signing
+    // will fail if these values are not percent encoded
+    .add(b':')
+    .add(b',')
+    .add(b'?')
+    .add(b'#')
+    .add(b'[')
+    .add(b')')
+    .add(b')')
+    .add(b'@')
+    .add(b'!')
+    .add(b'$')
+    .add(b'&')
+    .add(b'\'')
+    .add(b'(')
+    .add(b')')
+    .add(b'*')
+    .add(b'+')
+    .add(b';')
+    .add(b'=')
+    .add(b'%');