Repository: localstack/localstack
Pull request: fix s3 key handling with trailing slash
<!-- Please refer to the contribution guidelines before raising a PR: https://github.com/localstack/localstack/blob/master/CONTRIBUTING.md -->

<!-- Why am I raising this PR? Add context such as related issues, PRs, or documentation. -->
## Motivation
This would fix #9837. We've had multiple iterations of this, we were still unquoting the key to compare with the `base_url` but there was no need for it, I think this was a remnant of pre-Werkzeug 3.0

<!-- What notable changes does this PR make? -->
## Changes
- update the logic to be more resilient in the parser (proper splitting of the path, multiple trailing slashes...) 
- add more test cases

<!-- The following sections are optional, but can be useful! 

## Testing

Description of how to test the changes

## TODO

What's left to do:

- [ ] ...
- [ ] ...

-->



Problem statement:
bug: S3 listObjects and folder names containing a space
### Is there an existing issue for this?

- [X] I have searched the existing issues

### Current Behavior

When folder names contain a space then `listObjects` returns names without `/` at the end. Without space it works fine.

### Expected Behavior

Folders containing space in the name are returned with `/` at the end.

### How are you starting LocalStack?

With a `docker run` command

### Steps To Reproduce

awslocal s3api put-object --bucket bucket-name --key "folder name/"

awslocal s3api list-objects --bucket bucket-name
```
{
    "Contents": [
        {
            "Key": "folder name",
            "LastModified": "2023-12-11T10:15:20.000Z",
            "ETag": "\"d41d8cd98f00b204e9800998ecf8427e\"",
            "Size": 0,
            "StorageClass": "STANDARD",
            "Owner": {
                "DisplayName": "webfile",
                "ID": "75aa57f09aa0c8caeab4f8c24e99d10f8e7faeebf76c078efc7c6caea54ba06a"
            }
        }
    ],
    "RequestCharged": null
}
```


### Environment

```markdown
- OS: Linux Mint 21.2
- LocalStack: localstack/localstack:3.0
```


### Anything else?

_No response_

Hints from issue discussion:
Welcome to LocalStack! Thanks for reporting your first issue and our team will be working towards fixing the issue for you or reach out for more background information. We recommend joining our [Slack Community](https://localstack.cloud/contact/) for real-time help and drop a message to LocalStack Pro Support if you are a Pro user! If you are willing to contribute towards fixing this issue, please have a look at our [contributing guidelines](https://github.com/localstack/.github/blob/main/CONTRIBUTING.md) and our [contributing guide](https://docs.localstack.cloud/contributing/).

Commit message: update comment

Patch to review:
diff --git a/localstack/aws/protocol/parser.py b/localstack/aws/protocol/parser.py
index c0237d81193c1..eda8b144932cf 100644
--- a/localstack/aws/protocol/parser.py
+++ b/localstack/aws/protocol/parser.py
@@ -69,7 +69,6 @@
 from abc import ABC
 from email.utils import parsedate_to_datetime
 from typing import IO, Any, Dict, List, Mapping, Optional, Tuple, Union
-from urllib.parse import unquote
 from xml.etree import ElementTree as ETree
 
 import cbor2
@@ -1063,17 +1062,21 @@ def _parse_shape(
         Special handling of parsing the shape for s3 object-names (=key):
         trailing '/' are valid and need to be preserved, however, the url-matcher removes it from the key
         we check the request.url to verify the name.
-        We decode the key for the comparison with `base_url` in case of special characters.
+        Trailing slashes are stripped from the path, so we need special logic to compare the parsed Key parameter
+        against the path and add the missing slashes
         """
         if (
             shape is not None
             and uri_params is not None
             and shape.serialization.get("location") == "uri"
             and shape.serialization.get("name") == "Key"
-            and request.base_url.endswith(f"{unquote(uri_params['Key'])}/")
+            and (
+                (trailing_slashes := request.path.partition(uri_params["Key"])[2])
+                and all(char == "/" for char in trailing_slashes)
+            )
         ):
             uri_params = dict(uri_params)
-            uri_params["Key"] = uri_params["Key"] + "/"
+            uri_params["Key"] = uri_params["Key"] + trailing_slashes
         return super()._parse_shape(request, shape, node, uri_params)
 
     @_text_content
diff --git a/tests/aws/services/s3/test_s3.py b/tests/aws/services/s3/test_s3.py
index 141ff857d4547..f44790c2540e8 100644
--- a/tests/aws/services/s3/test_s3.py
+++ b/tests/aws/services/s3/test_s3.py
@@ -517,7 +517,19 @@ def test_upload_file_multipart(self, s3_bucket, tmpdir, snapshot, aws_client):
         condition=is_v2_provider,
         paths=["$..ServerSideEncryption"],
     )
-    @pytest.mark.parametrize("key", ["file%2Fname", "test@key/", "test%123", "test%percent"])
+    @pytest.mark.parametrize(
+        "key",
+        [
+            "file%2Fname",
+            "test@key/",
+            "test%123",
+            "test%percent",
+            "test key/",
+            "test key//",
+            "test%123/",
+            "a/%F0%9F%98%80/",
+        ],
+    )
     def test_put_get_object_special_character(self, s3_bucket, aws_client, snapshot, key):
         snapshot.add_transformer(snapshot.transform.s3_api())
         resp = aws_client.s3.put_object(Bucket=s3_bucket, Key=key, Body=b"test")
diff --git a/tests/aws/services/s3/test_s3.snapshot.json b/tests/aws/services/s3/test_s3.snapshot.json
index e5babbfe95186..580965299b741 100644
--- a/tests/aws/services/s3/test_s3.snapshot.json
+++ b/tests/aws/services/s3/test_s3.snapshot.json
@@ -6985,7 +6985,7 @@
     }
   },
   "tests/aws/services/s3/test_s3.py::TestS3::test_put_get_object_special_character[file%2Fname]": {
-    "recorded-date": "13-09-2023, 22:47:29",
+    "recorded-date": "12-12-2023, 13:46:25",
     "recorded-content": {
       "put-object-special-char": {
         "ETag": "\"098f6bcd4621d373cade4e832627b4f6\"",
@@ -7039,7 +7039,7 @@
     }
   },
   "tests/aws/services/s3/test_s3.py::TestS3::test_put_get_object_special_character[test@key/]": {
-    "recorded-date": "13-09-2023, 22:47:32",
+    "recorded-date": "12-12-2023, 13:46:27",
     "recorded-content": {
       "put-object-special-char": {
         "ETag": "\"098f6bcd4621d373cade4e832627b4f6\"",
@@ -7093,7 +7093,7 @@
     }
   },
   "tests/aws/services/s3/test_s3.py::TestS3::test_put_get_object_special_character[test%123]": {
-    "recorded-date": "13-09-2023, 22:47:34",
+    "recorded-date": "12-12-2023, 13:46:30",
     "recorded-content": {
       "put-object-special-char": {
         "ETag": "\"098f6bcd4621d373cade4e832627b4f6\"",
@@ -7147,7 +7147,7 @@
     }
   },
   "tests/aws/services/s3/test_s3.py::TestS3::test_put_get_object_special_character[test%percent]": {
-    "recorded-date": "13-09-2023, 22:47:36",
+    "recorded-date": "12-12-2023, 13:46:33",
     "recorded-content": {
       "put-object-special-char": {
         "ETag": "\"098f6bcd4621d373cade4e832627b4f6\"",
@@ -7200,6 +7200,222 @@
       }
     }
   },
+  "tests/aws/services/s3/test_s3.py::TestS3::test_put_get_object_special_character[test key/]": {
+    "recorded-date": "12-12-2023, 13:46:36",
+    "recorded-content": {
+      "put-object-special-char": {
+        "ETag": "\"098f6bcd4621d373cade4e832627b4f6\"",
+        "ServerSideEncryption": "AES256",
+        "ResponseMetadata": {
+          "HTTPHeaders": {},
+          "HTTPStatusCode": 200
+        }
+      },
+      "list-object-special-char": {
+        "Contents": [
+          {
+            "ETag": "\"098f6bcd4621d373cade4e832627b4f6\"",
+            "Key": "test key/",
+            "LastModified": "datetime",
+            "Size": 4,
+            "StorageClass": "STANDARD"
+          }
+        ],
+        "EncodingType": "url",
+        "IsTruncated": false,
+        "KeyCount": 1,
+        "MaxKeys": 1000,
+        "Name": "<bucket-name:1>",
+        "Prefix": "",
+        "ResponseMetadata": {
+          "HTTPHeaders": {},
+          "HTTPStatusCode": 200
+        }
+      },
+      "get-object-special-char": {
+        "AcceptRanges": "bytes",
+        "Body": "test",
+        "ContentLength": 4,
+        "ContentType": "binary/octet-stream",
+        "ETag": "\"098f6bcd4621d373cade4e832627b4f6\"",
+        "LastModified": "datetime",
+        "Metadata": {},
+        "ServerSideEncryption": "AES256",
+        "ResponseMetadata": {
+          "HTTPHeaders": {},
+          "HTTPStatusCode": 200
+        }
+      },
+      "del-object-special-char": {
+        "ResponseMetadata": {
+          "HTTPHeaders": {},
+          "HTTPStatusCode": 204
+        }
+      }
+    }
+  },
+  "tests/aws/services/s3/test_s3.py::TestS3::test_put_get_object_special_character[test key//]": {
+    "recorded-date": "12-12-2023, 13:46:39",
+    "recorded-content": {
+      "put-object-special-char": {
+        "ETag": "\"098f6bcd4621d373cade4e832627b4f6\"",
+        "ServerSideEncryption": "AES256",
+        "ResponseMetadata": {
+          "HTTPHeaders": {},
+          "HTTPStatusCode": 200
+        }
+      },
+      "list-object-special-char": {
+        "Contents": [
+          {
+            "ETag": "\"098f6bcd4621d373cade4e832627b4f6\"",
+            "Key": "test key//",
+            "LastModified": "datetime",
+            "Size": 4,
+            "StorageClass": "STANDARD"
+          }
+        ],
+        "EncodingType": "url",
+        "IsTruncated": false,
+        "KeyCount": 1,
+        "MaxKeys": 1000,
+        "Name": "<bucket-name:1>",
+        "Prefix": "",
+        "ResponseMetadata": {
+          "HTTPHeaders": {},
+          "HTTPStatusCode": 200
+        }
+      },
+      "get-object-special-char": {
+        "AcceptRanges": "bytes",
+        "Body": "test",
+        "ContentLength": 4,
+        "ContentType": "binary/octet-stream",
+        "ETag": "\"098f6bcd4621d373cade4e832627b4f6\"",
+        "LastModified": "datetime",
+        "Metadata": {},
+        "ServerSideEncryption": "AES256",
+        "ResponseMetadata": {
+          "HTTPHeaders": {},
+          "HTTPStatusCode": 200
+        }
+      },
+      "del-object-special-char": {
+        "ResponseMetadata": {
+          "HTTPHeaders": {},
+          "HTTPStatusCode": 204
+        }
+      }
+    }
+  },
+  "tests/aws/services/s3/test_s3.py::TestS3::test_put_get_object_special_character[test%123/]": {
+    "recorded-date": "12-12-2023, 13:46:41",
+    "recorded-content": {
+      "put-object-special-char": {
+        "ETag": "\"098f6bcd4621d373cade4e832627b4f6\"",
+        "ServerSideEncryption": "AES256",
+        "ResponseMetadata": {
+          "HTTPHeaders": {},
+          "HTTPStatusCode": 200
+        }
+      },
+      "list-object-special-char": {
+        "Contents": [
+          {
+            "ETag": "\"098f6bcd4621d373cade4e832627b4f6\"",
+            "Key": "test%123/",
+            "LastModified": "datetime",
+            "Size": 4,
+            "StorageClass": "STANDARD"
+          }
+        ],
+        "EncodingType": "url",
+        "IsTruncated": false,
+        "KeyCount": 1,
+        "MaxKeys": 1000,
+        "Name": "<bucket-name:1>",
+        "Prefix": "",
+        "ResponseMetadata": {
+          "HTTPHeaders": {},
+          "HTTPStatusCode": 200
+        }
+      },
+      "get-object-special-char": {
+        "AcceptRanges": "bytes",
+        "Body": "test",
+        "ContentLength": 4,
+        "ContentType": "binary/octet-stream",
+        "ETag": "\"098f6bcd4621d373cade4e832627b4f6\"",
+        "LastModified": "datetime",
+        "Metadata": {},
+        "ServerSideEncryption": "AES256",
+        "ResponseMetadata": {
+          "HTTPHeaders": {},
+          "HTTPStatusCode": 200
+        }
+      },
+      "del-object-special-char": {
+        "ResponseMetadata": {
+          "HTTPHeaders": {},
+          "HTTPStatusCode": 204
+        }
+      }
+    }
+  },
+  "tests/aws/services/s3/test_s3.py::TestS3::test_put_get_object_special_character[a/%F0%9F%98%80/]": {
+    "recorded-date": "12-12-2023, 13:46:44",
+    "recorded-content": {
+      "put-object-special-char": {
+        "ETag": "\"098f6bcd4621d373cade4e832627b4f6\"",
+        "ServerSideEncryption": "AES256",
+        "ResponseMetadata": {
+          "HTTPHeaders": {},
+          "HTTPStatusCode": 200
+        }
+      },
+      "list-object-special-char": {
+        "Contents": [
+          {
+            "ETag": "\"098f6bcd4621d373cade4e832627b4f6\"",
+            "Key": "a/%F0%9F%98%80/",
+            "LastModified": "datetime",
+            "Size": 4,
+            "StorageClass": "STANDARD"
+          }
+        ],
+        "EncodingType": "url",
+        "IsTruncated": false,
+        "KeyCount": 1,
+        "MaxKeys": 1000,
+        "Name": "<bucket-name:1>",
+        "Prefix": "",
+        "ResponseMetadata": {
+          "HTTPHeaders": {},
+          "HTTPStatusCode": 200
+        }
+      },
+      "get-object-special-char": {
+        "AcceptRanges": "bytes",
+        "Body": "test",
+        "ContentLength": 4,
+        "ContentType": "binary/octet-stream",
+        "ETag": "\"098f6bcd4621d373cade4e832627b4f6\"",
+        "LastModified": "datetime",
+        "Metadata": {},
+        "ServerSideEncryption": "AES256",
+        "ResponseMetadata": {
+          "HTTPHeaders": {},
+          "HTTPStatusCode": 200
+        }
+      },
+      "del-object-special-char": {
+        "ResponseMetadata": {
+          "HTTPHeaders": {},
+          "HTTPStatusCode": 204
+        }
+      }
+    }
+  },
   "tests/aws/services/s3/test_s3.py::TestS3::test_s3_get_object_headers": {
     "recorded-date": "03-08-2023, 04:25:53",
     "recorded-content": {


Code review comments:
diff_hunk: @@ -1063,17 +1062,21 @@ def _parse_shape(
         Special handling of parsing the shape for s3 object-names (=key):
         trailing '/' are valid and need to be preserved, however, the url-matcher removes it from the key
         we check the request.url to verify the name.
-        We decode the key for the comparison with `base_url` in case of special characters.
+        Trailing slashes are stripped from the path, so we need special logic to compare the parsed Key parameter
+        against the path and add the missing slashes
path: localstack/aws/protocol/parser.py
text: @user1:
nit: This comment is a bit confusing now, because it describes the removal of the trailing slashes twice.

@author:
Oops, yes, will rewrite everything!