Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix CopySource unquoting of special + char #9992

Merged
merged 4 commits into from Jan 4, 2024
Merged

Conversation

bentsku
Copy link
Contributor

@bentsku bentsku commented Jan 4, 2024

Motivation

As reported with #9990, we had an issue when copying a file with a space in the name with Go. This issue was not present with Python however. When looking at the sample provided by the user, it appeared that Go in URL encoding space character with + when Python is using %20.
When calling unquote with Python, it won't replace an encoded URL + character with a space. We will need to manually replace it before unquote the URL to not accidentally replaced an encoded +.

Changes

Manually replace an encoded URL + when parsing the CopySource header.
Added 2 different AWS tests to verify exactly the fix:

  • one global with weird key encoding as we didn't have one
  • one very specific test for this use case using the HTTP client because botocore will automatically encode space as %20 so we couldn't reproduce exactly the issue. This test fails without the fix, confirming it. This specific test is xfail for the v2 legacy provider as moto does not handle this edge case (so not a regression from the new provider)

fixes #9990

@bentsku bentsku added aws:s3 Amazon Simple Storage Service semver: patch Non-breaking changes which can be included in patch releases labels Jan 4, 2024
@bentsku bentsku requested a review from steffyP January 4, 2024 13:55
@bentsku bentsku self-assigned this Jan 4, 2024
Copy link

github-actions bot commented Jan 4, 2024

S3 Image Test Results (AMD64 / ARM64)

  2 files    2 suites   3m 9s ⏱️
385 tests 335 ✅  50 💤 0 ❌
770 runs  670 ✅ 100 💤 0 ❌

Results for commit 155e7eb.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Jan 4, 2024

LocalStack Community integration with Pro

    2 files      2 suites   1h 15m 35s ⏱️
2 434 tests 2 208 ✅ 226 💤 0 ❌
2 435 runs  2 208 ✅ 227 💤 0 ❌

Results for commit 155e7eb.

♻️ This comment has been updated with latest results.

Copy link
Member

@steffyP steffyP left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for fixing, @bentsku!
LGTM! Nice to have additional tests 👍

tests/aws/services/s3/test_s3.py Outdated Show resolved Hide resolved
tests/aws/services/s3/test_s3.py Outdated Show resolved Hide resolved
@bentsku bentsku merged commit 0c01be0 into master Jan 4, 2024
32 checks passed
@bentsku bentsku deleted the fix-s3-copy-source-quote branch January 4, 2024 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
aws:s3 Amazon Simple Storage Service semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bug: Copying files with spaces in the name returns a 404 error
2 participants