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

Presigner Host Injection accesses an invalid property #2725

Closed
cuuupid opened this issue Aug 29, 2021 · 3 comments
Closed

Presigner Host Injection accesses an invalid property #2725

cuuupid opened this issue Aug 29, 2021 · 3 comments
Assignees
Labels
bug This issue is a bug. closed-for-staleness response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days.

Comments

@cuuupid
Copy link

cuuupid commented Aug 29, 2021

Describe the bug

The bug exists in the s3-request-presigner package. Specifically, attempting to presign an existing request using presigner.presign(request) fails with a string error (cannot trim undefined).

This is caused by the SignatureV4 module attempting to access the host header, which does not exist. Issue #1988 addressed this and a PR was implemented to fill the host header automatically from the hostname property of the request. However, this fix introduces a bug as the property path is invalid.

While the fix retrieves the host header from the .hostname property of an HttpRequest object, this property path actually does not exist; the correct property path is .endpoint.hostname. Simply changing it to this or adding a || .endpoint.hostname would work here.

The line to fix:

requestToSign.headers.host = requestToSign.hostname;

A temporary workaround for others is to inject the .hostname property into the request before presigning. The above fix will then retrieve the .hostname property to populate the host header.

const req = new HttpRequest(endpoint, region)
req.hostname = req.endpoint.hostname
const url = await presigner.presign(req)

A similar bug occurs with the protocol which is sometimes undefined, resulting in presigned urls that begin with undefined//.

Your environment

Node.js 16.4.0

SDK version number

@aws-sdk/s3-request-presigner@3.28.0

Is the issue in the browser/Node.js/ReactNative?

Node.js

Details of the browser/Node.js/ReactNative version

v16.4.0

Steps to reproduce

const client = new S3(...) // with your credentials
const presigner = new S3RequestPresigner({...client.config, sha256: Hash.bind(null, "sha256")})

const objectUrl = '...' // some S3 object URL
const region = '...'
const endpoint = parseUrl(objectUrl) // @aws-sdk/url-parser
const req = new HttpRequest(endpoint, region)

const url = await presigner.presign(req)
console.log(formatUrl(urlReq)) // @aws-sdk/util-format-url

Observed behavior

Error:

Uncaught TypeError: Cannot read property 'trim' of undefined
    at Object.getCanonicalHeaders (...path to project.../node_modules/@aws-sdk/signature-v4/dist/cjs/getCanonicalHeaders.js:20:62)
    at SignatureV4.presign (...path to project.../node_modules/@aws-sdk/signature-v4/dist/cjs/SignatureV4.js:41:56)
    at processTicksAndRejections (node:internal/process/task_queues:96:5)

While the error message refers to Signaturev4, the issue is actually in s3-request-presigner as described above.

@cuuupid cuuupid added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Aug 29, 2021
@AllanZhengYP AllanZhengYP self-assigned this Sep 9, 2021
@AllanZhengYP AllanZhengYP removed the needs-triage This issue or PR still needs to be triaged. label Sep 9, 2021
@AllanZhengYP
Copy link
Contributor

Hi @pshah123

There's a line in question in your code snippet:

const req = new HttpRequest(endpoint, region) // Which HttpRequest?

Current HttpRequest does include the hostname:

export class HttpRequest implements HttpMessage, Endpoint {
public method: string;
public protocol: string;
public hostname: string;

@ajredniwja ajredniwja added the response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days. label Sep 10, 2021
@github-actions
Copy link

This issue has not received a response in 1 week. If you still think there is a problem, please leave a comment to avoid the issue from automatically closing.

@github-actions github-actions bot added closing-soon This issue will automatically close in 4 days unless further comments are made. closed-for-staleness and removed closing-soon This issue will automatically close in 4 days unless further comments are made. labels Sep 17, 2021
@github-actions
Copy link

github-actions bot commented Oct 6, 2021

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.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 6, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug This issue is a bug. closed-for-staleness response-requested Waiting on additional info and feedback. Will move to \"closing-soon\" in 7 days.
Projects
None yet
Development

No branches or pull requests

3 participants