-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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 unconditional key removal from request.headers
#2948
base: develop
Are you sure you want to change the base?
Conversation
See boto/botocore#2948 for the upstream bug fix. This only occurs for toekn-based auth, and therefore wasn't caught in tests or in local testing.
See boto/botocore#2948 for the upstream bug fix. This only occurs for toekn-based auth, and therefore wasn't caught in tests or in local testing.
See boto/botocore#2948 for the upstream bug fix. This only occurs for toekn-based auth, and therefore wasn't caught in tests or in local testing.
…19056) (#19111) See boto/botocore#2948 for the upstream bug fix. This only occurs for toekn-based auth, and therefore wasn't caught in tests or in local testing. Co-authored-by: Joshua Cannon <[email protected]>
…19056) (#19110) See boto/botocore#2948 for the upstream bug fix. This only occurs for toekn-based auth, and therefore wasn't caught in tests or in local testing. Co-authored-by: Joshua Cannon <[email protected]>
headers['x-amz-security-token'] = self.credentials.token | ||
if 'X-Amz-Security-Token' in headers: | ||
del headers['X-Amz-Security-Token'] | ||
headers['X-Amz-Security-Token'] = self.credentials.token |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also fixed the casing to be consistent with the rest of the file
Codecov ReportPatch coverage:
❗ 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 #2948 +/- ##
===========================================
- Coverage 93.29% 93.29% -0.01%
===========================================
Files 64 64
Lines 13588 13593 +5
===========================================
+ Hits 12677 12681 +4
- Misses 911 912 +1
☔ View full report in Codecov by Sentry. |
This appears correct at first glance. Can you share how you discovered this problem? I am surprised that this code has existed for 9 years. Having some details on how to reproduce it will help find any related issues and create test coverage. |
In the linked issue (pantsbuild/pants#19056) it is mentioned this was only an issue when using token-based auth. See if @thejcannon can fill in more details. |
Admittedly, my usage was using a plain dict instead of the botocore types because we're already doing a bit of monkeypatching. I wanted to reduce how much from botocore we were using. When I hit this behavior I figured the N other places something similar is being done (delete before replacement) meant this code was just either forgotten about or came later. IIUC the behavior is likely possible? It seems like the other locations are to avoid the multiple-values-for-one-header behavior. After this issue, I found out that botocore is using the email message header class from stdlib and just switched to that. So our specific use case is OK. I suspect there is a still a bug with multiple headers though (likely only through the programmatic API). |
This code mirrors
botocore/botocore/auth.py
Line 191 in 97ba590
botocore/botocore/auth.py
Line 449 in 97ba590