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: do not sign content-type in s3-request-presigner #1017

Merged

Conversation

trivikr
Copy link
Member

@trivikr trivikr commented Mar 19, 2020

Issue #, if available:
Fixes: #1000

Description of changes:
Do not sign content-type in s3-request-presigner

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@codecov-io
Copy link

Codecov Report

Merging #1017 into smithy-codegen will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@               Coverage Diff               @@
##           smithy-codegen    #1017   +/-   ##
===============================================
  Coverage           91.36%   91.36%           
===============================================
  Files                 144      144           
  Lines                2836     2837    +1     
  Branches              501      502    +1     
===============================================
+ Hits                 2591     2592    +1     
  Misses                245      245           
Impacted Files Coverage Δ
packages/s3-request-presigner/src/index.ts 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 22900d4...d26a90b. Read the comment docs.

@aws-sdk-js-automation
Copy link

AWS CodeBuild CI Report

  • CodeBuild project: sdk-staging-test
  • Commit ID: d26a90b
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@AllanZhengYP
Copy link
Contributor

In V2 it looks like the Content-Type not unsigned but moved from header to query string, I'm not able to find the rationale of making this change. However the lower cased content-type is not signable. I looked into other SDKs they don't set content-type to unsigned header either(e.g. Ruby SDK)

@trivikr
Copy link
Member Author

trivikr commented Mar 19, 2020

In V2 it looks like the Content-Type not unsigned but moved from header to query string

In v2, "Content-Type" is an unsignable header.
Code: https://github.com/aws/aws-sdk-js/blob/501b590f8d415df53655479381fb63033ea4ebc0/lib/signers/v4.js#L193

@AllanZhengYP
Copy link
Contributor

Looks like PHP is doing it because of the same issue we are facing: aws/aws-sdk-php#1521 (comment)

@trivikr trivikr merged commit e3c7dd2 into aws:smithy-codegen Mar 19, 2020
@trivikr trivikr deleted the s3-presigner-remove-content-type-header branch March 19, 2020 18:27
AllanZhengYP pushed a commit to AllanZhengYP/aws-sdk-js-v3 that referenced this pull request Mar 20, 2020
trivikr added a commit to trivikr/aws-sdk-js-v3 that referenced this pull request Mar 20, 2020
trivikr added a commit to trivikr/aws-sdk-js-v3 that referenced this pull request Mar 24, 2020
trivikr added a commit to trivikr/aws-sdk-js-v3 that referenced this pull request Mar 24, 2020
@lock
Copy link

lock bot commented Mar 27, 2020

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.

@lock lock bot locked as resolved and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants