Repository: rusoto/rusoto (language: rust)

Issue:
S3 list_objects's marker field gets %-encoded keys wrong
I have a bucket with keys that contain both `%2F` and `/`. `list_objects` returns them as strings like `a%2Fb/c`. Passing those strings as keys to `get_object` yields `NoSuchKey` errors because they get converted to `a/b/c` somewhere along the line, removing the distinction between `%2F` and `/`.

There's a silly workaround where I %-encode the % sign as well, requesting the key `a%252Fb/c` works, but I think it's clear that passing keys from `list_objects` to `get_object` should work without further mangling in user code.

Here's a demonstration of it going wrong and the workaround:

```
$ aws s3 ls --recursive s3://rusoto-test-nf0cfhnpuwxl0y8y
2019-04-27 07:28:17          4 a%2Fb/c

$ RUST_LOG=debug ./target/debug/foo eu-west-1 rusoto-test-nf0cfhnpuwxl0y8y a%2Fb/c 2>&1 | grep -P '
 final_uri: https://s3-eu-west-1.amazonaws.com/rusoto-test-nf0cfhnpuwxl0y8y/a/b/c
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Service(NoSuchKey("The specified key does not exist."))', src/libcore/result.rs:999:5

$ RUST_LOG=debug ./target/debug/foo eu-west-1 rusoto-test-nf0cfhnpuwxl0y8y a%252Fb/c 2>&1 | grep -P 'final_uri|panic'
 final_uri: https://s3-eu-west-1.amazonaws.com/rusoto-test-nf0cfhnpuwxl0y8y/a%252Fb/c
```

with example program:
```rust
#[derive(Debug, structopt::StructOpt)]
struct Opt {
    region: rusoto_core::Region,
    bucket: String,
    key: String,
}

fn main() {
    let opt: Opt = structopt::StructOpt::from_args();
    env_logger::init();
    let s3_client = rusoto_s3::S3Client::new(opt.region);
    rusoto_s3::S3::get_object(&s3_client, rusoto_s3::GetObjectRequest{
        bucket: opt.bucket,
        key: opt.key,
        .. Default::default()
    }).sync().unwrap();
}
```


Discussion / hints:
This also applies when using keys in the `marker` field of S3's `list_object` request, probably because that also gets serialized into the URL as a GET param.
#1372 fixes `get_object` (yay! thanks!), but `list_objects` is still wrong
Planning on taking a look at this.
Placing some some initial findings/notes on this for future reference:
1. https://github.com/rusoto/rusoto/blob/master/rusoto/core/src/signature.rs#L679 is where the decode happens for the query parameters of the S3 uri. 
2. Removing the call to `decode_uri` does result in the correct result(marker/prefix) in the output, however it does cause this test: https://github.com/rusoto/rusoto/blob/master/rusoto/core/src/signature.rs#L829 to fail. 

Moving forward I'm going to look into what exactly that test is doing and whether it is failing correctly. My guess is that it probably is so there might need to be another way of parsing the query parameters. Might be some assumption being made on the encoding of the request or decoding of the response. I'll take a look at other SDKs to see how this specific functionality is supported. 

3. Boto3 request: `url=https://test-bucket-encoding-1569890941.s3.amazonaws.com/?marker=a%252Fb%2Fc%2Ftest_file&encoding-type=url`
Rusoto request: ` final_uri: https://s3.amazonaws.com/test-bucket-encoding-1569949841?marker=a%2Fb%2Fc%2Ftest_file`
---------
4. After experimentation with boto3 and reading over https://docs.aws.amazon.com/AmazonS3/latest/API/sigv4-query-string-auth.html#query-string-auth-v4-signin (specifically the UriEncode function). I feel that the decoding prior to encoding should be removed. One example, when adding `arg1%7BXXarg2%7B+%2BXX+%2B` as a prefix/marker in boto3 the end result was `arg1%257BXXarg2%257B%2B%252BXX%2B%252B` in the URL. I'm going to put up a PR with that decoding removed and with that encoding outliers test updated.

Fix patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index c3c986ac092..c66b013777c 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -4,6 +4,9 @@
 
 (Please put changes here)
 
+- Don't decode query string parameters before encoding it. Results in fixing the prefix and marker
+params for s3 `list_objects` methods
+
 ## [0.41.0] - 2019-10-07
 
 - Add `HttpClient::from_builder`
diff --git a/rusoto/core/src/signature.rs b/rusoto/core/src/signature.rs
index 8d1620481f4..25ee9e3d058 100644
--- a/rusoto/core/src/signature.rs
+++ b/rusoto/core/src/signature.rs
@@ -5,6 +5,9 @@
 //!
 //! If needed, the request will be re-issued to a temporary redirect endpoint.  This can happen with
 //! newly created S3 buckets not in us-standard/us-east-1.
+//!
+//! Please note that this module does not expect URIs to already be encoded.
+//!
 
 use std::borrow::Cow;
 use std::collections::btree_map::Entry;
@@ -18,10 +21,10 @@ use bytes::Bytes;
 use hex;
 use hmac::{Hmac, Mac};
 use md5;
+use percent_encoding::{percent_decode, utf8_percent_encode, AsciiSet, NON_ALPHANUMERIC};
 use sha2::{Digest, Sha256};
 use time::now_utc;
 use time::Tm;
-use percent_encoding::{percent_decode, utf8_percent_encode, AsciiSet, NON_ALPHANUMERIC};
 
 use crate::credential::AwsCredentials;
 use crate::param::{Params, ServiceParams};
@@ -677,7 +680,7 @@ pub fn encode_uri_path(uri: &str) -> String {
 
 #[inline]
 fn encode_uri_strict(uri: &str) -> String {
-    utf8_percent_encode(&decode_uri(uri), &STRICT_ENCODE_SET).collect::<String>()
+    utf8_percent_encode(uri, &STRICT_ENCODE_SET).collect::<String>()
 }
 
 #[inline]
@@ -837,7 +840,7 @@ mod tests {
         request.add_param("arg2%7B+%2B", "+%2B");
         assert_eq!(
             super::build_canonical_query_string(&request.params),
-            "arg1%7B=arg1%7B&arg2%7B%20%2B=%20%2B"
+            "arg1%257B=arg1%257B&arg2%257B%20%252B=%20%252B"
         );
         assert_eq!(
             super::canonical_uri(&request.path, &Region::default()),


Test patch:
diff --git a/integration_tests/tests/s3.rs b/integration_tests/tests/s3.rs
index f1465fb2334..22bac838ab7 100644
--- a/integration_tests/tests/s3.rs
+++ b/integration_tests/tests/s3.rs
@@ -120,7 +120,6 @@ impl Drop for TestS3Client {
     }
 }
 
-// NOTE: Do we want to use logging calls instead of printlns??
 // inititializes logging
 fn init_logging() {
     let _ = env_logger::try_init();
@@ -456,6 +455,81 @@ fn test_multipart_stream_uploads() {
     )
 }
 
+#[test]
+fn test_list_objects_encoding() {
+    init_logging();
+
+    let bucket_name = format!("test-bucket-{}-{}", "encoding".to_owned(), get_time().sec);
+    let test_client = TestS3Client::new(bucket_name.clone());
+    test_client.create_test_bucket(bucket_name.clone());
+
+    let filename = "a%2Fb/c/test_file".to_owned();
+    let prefix = "a%2Fb/c".to_owned();
+    test_client.put_test_object(filename.clone());
+
+    let list_obj_req_v1 = ListObjectsRequest {
+        bucket: bucket_name.clone(),
+        marker: Some(prefix.clone()),
+        prefix: Some(prefix.clone()),
+        ..Default::default()
+    };
+
+    let resp_v1 = test_client
+        .s3
+        .list_objects(list_obj_req_v1)
+        .sync()
+        .expect("failed to list objects v1");
+
+    assert!(&resp_v1.contents.is_some());
+    let contents_v1 = resp_v1.contents.clone().unwrap();
+    assert_eq!(contents_v1.len(), 1);
+
+    let object = &contents_v1[0];
+    assert!(&object.key.is_some());
+
+    let key = object.key.clone().unwrap();
+    assert_eq!(key, filename);
+
+    // wrap up v1 list obj test with getting the obj with the key returned
+    let get_obj_req = GetObjectRequest {
+        bucket: bucket_name.clone(),
+        key: key.clone(),
+        ..Default::default()
+    };
+    assert!(test_client.s3.get_object(get_obj_req).sync().is_ok());
+
+    let list_obj_req_v2 = ListObjectsV2Request {
+        bucket: bucket_name.clone(),
+        prefix: Some(prefix.clone()),
+        ..Default::default()
+    };
+    let resp_v2 = &test_client
+        .s3
+        .list_objects_v2(list_obj_req_v2)
+        .sync()
+        .expect("failed to list objects v2");
+
+    assert!(&resp_v2.contents.is_some());
+    let contents_v2 = resp_v2.contents.clone().unwrap();
+    assert_eq!(contents_v2.len(), 1);
+
+    let object = &contents_v2[0];
+    assert!(&object.key.is_some());
+
+    let key = object.key.clone().unwrap();
+    assert_eq!(key, filename);
+
+    // wrap up v2 list obj test with getting the obj with the key returned
+    let get_obj_req = GetObjectRequest {
+        bucket: bucket_name.clone(),
+        key: key.clone(),
+        ..Default::default()
+    };
+    assert!(test_client.s3.get_object(get_obj_req).sync().is_ok());
+
+    test_delete_object(&test_client.s3, &bucket_name, &key);
+}
+
 fn test_multipart_upload(
     client: &S3Client,
     region: &Region,