Skip to content

Add support for query params on s3 requests and body to be passed through #134

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.

Sign up for GitHub

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

Open
tomlarkworthy wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Add support for query params on s3 requests and body to be passed through #134

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
9fdd032
Add support for query params on s3 rewuasts to be passed through
Sep 16, 2023
13b5f1e
Add body and header forwarding
Sep 22, 2023
6df7afd
Full headers is breaking AWS signing
Sep 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 5 additions & 6 deletions src/handlers/s3.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,15 +68,14 @@ export default function s3HandlerFactory({
? utils.resolveParams(`${resolvedEndpoint}/{file}`, ctx.params)
: resolvedEndpoint; // Bucket operations

const headers: Record<string, string> = {};
const headers = {
...(ctx.request?.headers?.range && { range: ctx.request.headers.range }),
Copy link
Owner

@markusahlstrand markusahlstrand Sep 23, 2023 •
edited
Loading

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

@tomlarkworthy tomlarkworthy Sep 23, 2023

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:-

cloudworker-proxy/src/handlers/s3.ts

Line 74 in 10c4ffc

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

@tomlarkworthy tomlarkworthy Sep 23, 2023

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

@tomlarkworthy tomlarkworthy Sep 24, 2023

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

};

if (ctx.request.headers.range) {
headers.range = ctx.request.headers.range;
}

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

@tomlarkworthy tomlarkworthy Sep 23, 2023

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

method: ctx.method || ctx.request.method,
headers,
...(ctx.request.body && { body: ctx.request.body }),
Copy link
Contributor Author

@tomlarkworthy tomlarkworthy Sep 23, 2023

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

});

ctx.status = response.status;
Expand Down
74 changes: 74 additions & 0 deletions test/handlers/s3.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,4 +81,78 @@ describe('s3', () => {
await s3(ctx);
expect(ctx.status).to.equal(200);
});

it('Range headers are forwarded', async () => {
fetchMock.mock(`http://localhost:9000/myBucket`, (req) => {
return {
status: 200,
};
});
const s3 = s3Factory({
endpoint: 'http://localhost:9000',
forcePathStyle: true,
bucket: 'myBucket',
accessKeyId: 'DERP',
secretAccessKey: 'DERP',
enableBucketOperations: true,
});

const ctx = helpers.getCtx();
ctx.params = {};
ctx.request.headers['range'] = 'blah';
await s3(ctx);
expect(ctx.status).to.equal(200);
expect(fetchMock.lastOptions().headers['range']).to.equal('blah');
});

it('Request body is forwarded', async () => {
fetchMock.mock(`http://localhost:9000/myBucket`, (req) => {
return {
status: 200,
};
});
const s3 = s3Factory({
endpoint: 'http://localhost:9000',
forcePathStyle: true,
bucket: 'myBucket',
accessKeyId: 'DERP',
secretAccessKey: 'DERP',
enableBucketOperations: true,
});

const ctx = helpers.getCtx();
ctx.request.method = 'PUT';
ctx.request.body = 'hello world';
ctx.params = {};
ctx.request.headers['If-None-Match'] = 'blah';
await s3(ctx);
expect(ctx.status).to.equal(200);
// aws4fetch converts body to readable stream so it is not so easy to figure out the
// actual value
expect(await fetchMock.lastOptions().body).is.not.undefined;
});



it('List bucket forwards query params', async () => {
fetchMock.mock(`http://localhost:9000/myBucket?prefix=foo&list-type=2`, {
status: 200,
});

const s3 = s3Factory({
endpoint: 'http://localhost:9000',
forcePathStyle: true,
bucket: 'myBucket',
accessKeyId: 'DERP',
secretAccessKey: 'DERP',
enableBucketOperations: true,
});

const ctx = helpers.getCtx();
ctx.request.search = '?prefix=foo&list-type=2';
ctx.params = {};
await s3(ctx);

expect(ctx.status).to.equal(200);
});
});
7 changes: 2 additions & 5 deletions test/helpers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ class Context {
request: {
method: string;
path: string;
query: {};
search?: string;
hostname: string;
host: string;
protocol: string;
Expand All @@ -23,7 +23,7 @@ class Context {
host: 'example.com',
hostname: 'example.com',
protocol: 'http',
query: {},
search: '',
headers: {},
};
this.event = {};
Expand All @@ -34,9 +34,6 @@ class Context {
};
this.body = undefined;
this.status = 404;

// Shortcuts directly on the context
this.query = this.request.query;
}

set(key: string, value: string) {
Expand Down