Skip to content

Conversation

@tomlarkworthy
Copy link
Contributor

@tomlarkworthy tomlarkworthy commented Sep 17, 2023

Current S3 handler ends up stripping query/body/header parameters which are useful for various s3 API functions.

@tomlarkworthy tomlarkworthy changed the title Add support for query params on s3 requests to be passed through Add support for query params on s3 requests and body to be passed through Sep 23, 2023

const headers = ctx.request.headers || {};
const headers = {
...(ctx.request?.headers?.range && { range: ctx.request.headers.range }),
Copy link
Owner

@markusahlstrand markusahlstrand Sep 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm. it looks like this should have worked in the past as well as we're passing on all the headers? Am I missing something.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the current HEAD just passes on the range headers:-

headers.range = ctx.request.headers.range;

I changed it to pass on everything in this PR earlier. Then I realised that broke the AWS signing so I reverted back to jsut the range headers. Will need to fix at some point but I am not sure which headers break the signer yet. An aws4fetch-like lib suggests the host header is a problem jamesmbourne/aws4-axios#110

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the current HEAD initializes the headers to {} and then adds a range header only. We are indeed missing useful features like If-None-Match

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could add if-none-match and other headers but I am unsure of whether they are capitalised or not

}

const response = await aws.fetch(url, {
const response = await aws.fetch(url + (ctx.request.search || ''), {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this 1 main the main thing added, search params

const response = await aws.fetch(url + (ctx.request.search || ''), {
method: ctx.method || ctx.request.method,
headers,
...(ctx.request.body && { body: ctx.request.body }),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and body for PUT requests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants