Fix generate_presigned_url for S3 operations where the modeled request URI contains a query component

[jonemo]

Addresses #2962

Fixes a bug where, for certain S3 operations and when using the default signer, the URL used in the signature string was invalid. This resulted in invalid presigned URLs.

Context

Botocore uses a JSON file that defines the S3 service model for constructing request URLs for S3 API requests. For example, the GetObjectAcl operation is modeled with request URI /{Bucket}/{Key+}?acl. Since the introduction of virtual-hosted-style addressing, botocore has dropped the leading /{Bucket} from the service model's request URIs. Separate S3-specific logic then re-introduces the bucket name back into the URL later, either in virtual-hosted or path style addressing depending on configuration and bucket name. However, irrespective of addressing style, the bucket name must be included in the URL path used for Signature Version 2 signatures.

Botocore version 1.28.0 contains a change for how these special rules for constructing S3 service URLs from the service model are implemented. This change failed to account for modeled request URIs that contain a query component. For example, PutBucketCors has the modeled request URI /{Bucket}?cors. As reported in #2962, the "?cors" string was incorrectly included in the signed string twice.

Solution

In the remove_bucket_from_url_paths_from_model event handler, the query is removed from the request URI before it is copied into the authPath field.

(Note that the remove_bucket_from_url_paths_from_model event handler is applied for every request in the s3 client during the before-parameter-build. However, the event handler is idempotent and only transforms the model once during the first request for each operation.)

Testing

I decided against creating an integration test for this case because using presigned URLs for any of the affected S3 operations requires non-trivial custom code on top of what botocore provides. For example, the repo code in #2962 constructs a XML request body and calculates a checksum header. Such an integration test would provide more test coverage for custom test code than for botocore logic itself.

The newly introduced unit test for the remove_bucket_from_url_paths_from_model event handler should be sufficient to avoid regressions on the bug.

The repro code provided in #2962 works with MODIFY_FUNC = False after this change.

[codecov-commenter]

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.07 🎉

Comparison is base (bc89f15) 93.29% compared to head (9f884b6) 93.36%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #2971      +/-   ##
===========================================
+ Coverage    93.29%   93.36%   +0.07%     
===========================================
  Files           64       65       +1     
  Lines        13592    13841     +249     
===========================================
+ Hits         12680    12923     +243     
- Misses         912      918       +6     

Impacted Files	Coverage Δ
botocore/handlers.py	95.22% <100.00%> (+<0.01%)	⬆️

... and 8 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.