Repository: boto/botocore
Pull request #2799 (2022-10-27 23:10:02)

Fix for S3 pre-signed URLs with bucket names that contain dots
Addresses https://github.com/boto/botocore/issues/2798.

Version 1.28.0 erroneously introduced a change in behavior for S3 pre-signed URLs with bucket names that aren't valid host labels as defined in [RFC 1123](https://datatracker.ietf.org/doc/html/rfc1123#section-2). After the change, the resulting URLs no longer used the regional endpoint. 

This PR fixes the specific issue reported in https://github.com/boto/botocore/issues/2798 and adds test coverage for it. In addition, it reverts to pre-1.28.0 logic for whether to use a regional or global endpoint for certain S3 requests where either option is valid.

codecov-commenter: # [Codecov](https://codecov.io/gh/boto/botocore/pull/2799?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=boto) Report
Base: **93.57**% // Head: **93.57**% // Increases project coverage by **`+0.00%`** :tada:
> Coverage data is based on head [(`24e949c`)](https://codecov.io/gh/boto/botocore/pull/2799?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=boto) compared to base [(`7d5a59f`)](https://codecov.io/gh/boto/botocore/commit/7d5a59f4165011e5fa8bfac419fdbc836858873e?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=boto).
> Patch coverage: 100.00% of modified lines in pull request are covered.

> :exclamation: Current head 24e949c differs from pull request most recent head a7dff45. Consider uploading reports for the commit a7dff45 to get more accurate results

<details><summary>Additional details and impacted files</summary>


```diff
@@           Coverage Diff            @@
##           develop    #2799   +/-   ##
========================================
  Coverage    93.57%   93.57%           
========================================
  Files           63       63           
  Lines        13302    13303    +1     
========================================
+ Hits         12447    12448    +1     
  Misses         855      855           
```


| [Impacted Files](https://codecov.io/gh/boto/botocore/pull/2799?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=boto) | Coverage Δ | |
|---|---|---|
| [botocore/handlers.py](https://codecov.io/gh/boto/botocore/pull/2799/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=boto#diff-Ym90b2NvcmUvaGFuZGxlcnMucHk=) | `96.42% <100.00%> (+<0.01%)` | :arrow_up: |

Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=boto). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=boto)

</details>

[:umbrella: View full report at Codecov](https://codecov.io/gh/boto/botocore/pull/2799?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=boto).   
:loudspeaker: Do you have feedback about the report comment? [Let us know in this issue](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=boto).


Patch:
diff --git a/botocore/handlers.py b/botocore/handlers.py
index 8bfc501e9..30a3d3358 100644
--- a/botocore/handlers.py
+++ b/botocore/handlers.py
@@ -46,6 +46,7 @@ from botocore.docs.utils import (
     AutoPopulatedParam,
     HideParamFromOperations,
 )
+from botocore.endpoint_provider import VALID_HOST_LABEL_RE
 from botocore.exceptions import (
     AliasConflictParameterError,
     ParamValidationError,
@@ -1109,10 +1110,23 @@ def customize_endpoint_resolver_builtins(
     elif bucket_is_arn:
         builtins[EndpointResolverBuiltins.AWS_S3_FORCE_PATH_STYLE] = False
 
+    # Bucket names that are invalid host labels require path-style addressing.
+    # If path-style addressing was specifically requested, the default builtin
+    # value is already set.
+    path_style_required = (
+        bucket_name is not None and not VALID_HOST_LABEL_RE.match(bucket_name)
+    )
+    path_style_requested = builtins[
+        EndpointResolverBuiltins.AWS_S3_FORCE_PATH_STYLE
+    ]
+
+    # Path-style addressing is incompatible with the global endpoint for
+    # presigned URLs. If the bucket name is an ARN, the ARN's region should be
+    # used in the endpoint.
     if (
-        context.get('is_presign_request')
-        and context.get('use_global_endpoint')
-        and not builtins[EndpointResolverBuiltins.AWS_S3_FORCE_PATH_STYLE]
+        context.get('use_global_endpoint')
+        and not path_style_required
+        and not path_style_requested
         and not bucket_is_arn
     ):
         builtins[EndpointResolverBuiltins.AWS_REGION] = 'aws-global'


Test patch:
diff --git a/tests/functional/test_s3.py b/tests/functional/test_s3.py
index 84868b2cc..66f77e7d0 100644
--- a/tests/functional/test_s3.py
+++ b/tests/functional/test_s3.py
@@ -3395,6 +3395,16 @@ def _addressing_for_presigned_url_test_cases():
         signature_version="s3v4",
         expected_url=("https://bucket.s3.us-east-1.amazonaws.com/key"),
     )
+    # Bucket names that contain dots or are otherwise not virtual host style
+    # compatible should always resolve to a regional endpoint.
+    # https://github.com/boto/botocore/issues/2798
+    yield dict(
+        region="us-west-1",
+        bucket="foo.bar.biz",
+        key="key",
+        signature_version="s3",
+        expected_url="https://s3.us-west-1.amazonaws.com/foo.bar.biz/key",
+    )
 
 
 @pytest.mark.parametrize(