S3: fix unquoting of S3 object keys in DeleteObjects

[bentsku]

As reported in localstack/localstack#9428, moto is decoding the content of the DeleteObjects body, but those keys are not going through the URL encoding as they're in the XML, and should be used as is.

I've removed the get_safe_path call from DeleteObjects and added a test validating the behaviour, tested against AWS.

This devolved into a bit of a rabbit hole, as it seems fixing this still did not delete the keys, because they were saved with an emoji in the name where it should have been using the raw url encoded format like S3.

object_key_emoji = "a/%F0%9F%98%80"
client.put_object(Bucket=bucket_name, Key=object_key_emoji, Body="emoji percent encoding")

Result of ListObjects:

{
    "Contents": [
        {
            "ETag": "\"ec53baa61c0c0b736a567bdef59250f3\"",
            "Key": "a/😀",
        }
     ]
}

I've changed the get_safe_path util to make use of Werkzeug RAW_URI, as this contains the URI as received by the server. We can then use those proper values to construct the object key, and this circumvents all issue related to the key encoding. This is a rather big change, but if the tests are green, I believe this would be safe to do.

[bblommers]

This devolved into a bit of a rabbit hole

Yup.. that sounds familiar! It took me ages to get it working in the first place, but I wasn't really happy with the outcome. If you can mange to simplify the solution, that would be very welcome.

For context, this was the original PR with some rationale: #6602

This is now complicated by the introduction of MotoProxy (#6848), which does not have the concept of a RAW_URI.

[bentsku]

Thanks for the pointer towards MotoProxy, I had totally missed that. I will validate the failing x-copy-source* tests with URL encoding against AWS to see what the behaviour is and what I've missed, and then will work to make it work for MotoProxy. Those Werkzeug upgrades were no joke 😓

It seems all tests that are failing, for Server and Proxy, are the following:

• FAILED tests/test_s3/test_s3_multipart.py::test_multipart_upload_with_copy_key[the-unicode-\U0001f4a9-key]
• FAILED tests/test_s3/test_s3_multipart.py::test_multipart_upload_with_copy_key[key-with?question-mark]
• FAILED tests/test_s3/test_s3_multipart.py::test_multipart_upload_with_copy_key[key-with%2Fembedded%2Furl%2Fencoding]

It might be working for MotoProxy already then?

[bblommers]

Just to make things interesting, I've added the moto-core label, which runs the test against older werkzeug versions.

I'm assuming that's not a problem, because RAW_URI will be exposed for older werkzeug versions as well - but let's see.

Edit: OK, that looks good. The tests do fail, but not because of werkzeug.

[bentsku]

Yes, I actually had a wrong assumption for x-amz-copy-source, AWS required the value to be url-encoded too, but this does not go through the whole werkzeug decoding, so a simple unquote will do like it CopyObject. Hope this will make everything green! 🤞

[bentsku]

Seems like the failure might have been a flake as everything else is green? Looks this might work! 😄

[codecov]

Codecov Report

Merging #6933 (a4f8700) into master (531fdc7) will decrease coverage by 0.01%.
Report is 4 commits behind head on master.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #6933      +/-   ##
==========================================
- Coverage   95.83%   95.83%   -0.01%     
==========================================
  Files         830      830              
  Lines       81371    81328      -43     
==========================================
- Hits        77983    77940      -43     
  Misses       3388     3388              

Flag	Coverage Δ
servertests	36.66% <88.88%> (-0.03%)	⬇️
unittests	95.77% <100.00%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
moto/core/responses.py	96.05% <100.00%> (+0.01%)	⬆️
moto/core/utils.py	95.65% <100.00%> (-0.70%)	⬇️
moto/s3/responses.py	95.78% <100.00%> (+0.19%)	⬆️

... and 1 file with indirect coverage changes

[bblommers]

Yes, this LGTM. Awesome - thank you for fixing this @bentsku!

[github-actions]

This is now part of moto >= 4.2.7.dev18