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.

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

Allow setting multiple cookies in Netlify adapter #3092

Merged
merged 4 commits into from
Apr 12, 2022
Merged

Allow setting multiple cookies in Netlify adapter #3092

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e000795
Allow setting multiple cookies in Netlify adapter
matthewp Apr 12, 2022
911386d
Adds a changeset
matthewp Apr 12, 2022
fb588a1
Set the response status code
matthewp Apr 12, 2022
15cd9fc
Add a comment on why this is needed
matthewp Apr 12, 2022
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
5 changes: 5 additions & 0 deletions .changeset/sharp-moose-perform.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@astrojs/netlify': patch
---

Fixes setting multiple cookies with the Netlify adapter
28 changes: 24 additions & 4 deletions packages/integrations/netlify/src/netlify-functions.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,14 +33,34 @@ export const createExports = (manifest: SSRManifest, args: Args) => {
};
}

const response = await app.render(request);
const response: Response = await app.render(request);
const responseBody = await response.text();

return {
statusCode: 200,
headers: Object.fromEntries(response.headers.entries()),
const responseHeaders = Object.fromEntries(response.headers.entries());
const fnResponse: any = {
statusCode: response.status,
headers: responseHeaders,
body: responseBody,
};

// Special-case set-cookie which has to be set an different way :/
Copy link
Member

Choose a reason for hiding this comment

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

so strange. mind expanding a bit on why exactly this is needed, for future readers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah sure, the tldr here is that the Fetch API concatenates headers and comma-separates them when you try to get them with headers.get('foo'). Set-Cookie is the only header that doesn't work comma-separated and must be set with multiple Set-Cookie headers. Fetch does not have an API for that, because there's no need for it in the browser.

Luckily node-fetch has a special API for this use-case and Netlify will always use that polyfill so its safe to use. Other adapters will have to do it different ways, since there is no standard.

For reference (with various host providers weighing in): whatwg/fetch#973

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'll add a comment with a link to that issue.

if (response.headers.has('set-cookie') && 'raw' in response.headers) {
// Node fetch allows you to get the raw headers, which includes multiples of the same type.
// This is needed because Set-Cookie *must* be called for each cookie, and can't be
// concatenated together.
type HeadersWithRaw = Headers & {
raw: () => Record<string, string[]>;
};

const rawPacked = (response.headers as HeadersWithRaw).raw();
if('set-cookie' in rawPacked) {
fnResponse.multiValueHeaders = {
'set-cookie': rawPacked['set-cookie']
}
}
}

return fnResponse;
};

return { handler };
Expand Down
50 changes: 50 additions & 0 deletions packages/integrations/netlify/test/cookies.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
import { expect } from 'chai';
import { load as cheerioLoad } from 'cheerio';
import { loadFixture } from '../../../astro/test/test-utils.js';
import netlifyAdapter from '../dist/index.js';
import { fileURLToPath } from 'url';

describe('Cookies', () => {
/** @type {import('../../../astro/test/test-utils').Fixture} */
let fixture;

before(async () => {
fixture = await loadFixture({
root: new URL('./fixtures/cookies/', import.meta.url).toString(),
experimental: {
ssr: true,
},
adapter: netlifyAdapter({
dist: new URL('./fixtures/cookies/dist/', import.meta.url),
}),
site: `http://example.com`,
vite: {
resolve: {
alias: {
'@astrojs/netlify/netlify-functions.js': fileURLToPath(
new URL('../dist/netlify-functions.js', import.meta.url)
),
},
},
},
});
await fixture.build();
});

it('Can set multiple', async () => {
const entryURL = new URL('./fixtures/cookies/dist/functions/entry.mjs', import.meta.url);
const { handler } = await import(entryURL);
const resp = await handler({
httpMethod: 'POST',
headers: {},
rawUrl: 'http://example.com/login',
body: '{}',
isBase64Encoded: false
});
expect(resp.statusCode).to.equal(301);
expect(resp.headers.location).to.equal('/');
expect(resp.multiValueHeaders).to.be.deep.equal({
'set-cookie': [ 'foo=foo; HttpOnly', 'bar=bar; HttpOnly' ]
});
});
});
6 changes: 6 additions & 0 deletions packages/integrations/netlify/test/fixtures/cookies/src/pages/index.astro
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
<html>
<head><title>Testing</title></head>
<body>
<h1>Testing</h1>
</body>
</html>
12 changes: 12 additions & 0 deletions packages/integrations/netlify/test/fixtures/cookies/src/pages/login.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@

export function post() {
const headers = new Headers();
headers.append('Set-Cookie', `foo=foo; HttpOnly`);
headers.append('Set-Cookie', `bar=bar; HttpOnly`);
headers.append('Location', '/');

return new Response('', {
status: 301,
headers,
});
}