Update presigner to only hoist headers in v2 presigned URLs #1808
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1808 +/- ##
=========================================
Coverage ? 95.75%
=========================================
Files ? 191
Lines ? 6668
Branches ? 1364
=========================================
Hits ? 6385
Misses ? 283
Partials ? 0
Continue to review full report at Codecov.
|
… pushes and only PRs
jeskew
force-pushed
the
fix/presign-v4
branch
from
32105ca to
ff90391
Compare
lib/signers/presign.js
Outdated
| var parsedUrl = AWS.util.urlParse(request.httpRequest.path); | ||
| var queryParams = parsedUrl.search | ||
| ? AWS.util.queryStringParse(parsedUrl.search.substr(1)) : {}; | ||
| queryParams['Expires'] = expires; |
There was a problem hiding this comment.
It doesn't look like queryParams is used after it's created.
|
|
||
| AWS.util.each(request.httpRequest.headers, function (key, value) { | ||
| if (key === expiresHeader) key = 'Expires'; | ||
| if (key.indexOf('x-amz-meta-') === 0) { |
There was a problem hiding this comment.
Do you know if there are any headers that we hoist to the query string today that don't also have to be sent as headers when using sigv4 presigned urls? Maybe that's just a feature of sigv2, but I'm wondering if we'll lose out on that feature with this change.
There was a problem hiding this comment.
There are, and they are all handled here. The current V4 presigner will always produce an invalid signature when any other headers are present.
There was a problem hiding this comment.
Ah ok. So we may want to add additional headers to that section you pointed to in the future, but as of now, this change won't cause any existing cases to break. This part looks good then!
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs and link to relevant comments in this thread. |
Resolves #1804. All integration tests were run locally and pass.