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

Jump to bottom

fix: allow cookies to be set in rewritten responses #11280

Merged
merged 3 commits into from
Jun 20, 2024
Merged

fix: allow cookies to be set in rewritten responses #11280

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
e47e3f5
fix: allow cookies to be set in rewritten responses
ascorbic Jun 18, 2024
8f9fb35
Merge cookies
ascorbic Jun 18, 2024
a5412a7
Add support for endpoints and more tests
ascorbic Jun 20, 2024
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/big-eyes-share.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'astro': patch
---

Fixes a bug that prevented cookies from being set when using experimental rewrites
3 changes: 3 additions & 0 deletions packages/astro/src/core/app/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,9 @@ export class App {
for (const setCookieHeaderValue of App.getSetCookieFromResponse(response)) {
response.headers.append('set-cookie', setCookieHeaderValue);
}
} else {
// It may have been set already in a rewrite
response.headers.delete('set-cookie');
}

Reflect.set(response, responseSentSymbol, true);
Expand Down
6 changes: 6 additions & 0 deletions packages/astro/src/core/render-context.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,12 @@ export class RenderContext {
streaming,
this.routeData
);

if (result.cookies) {
for (const cookie of AstroCookies.consume(result.cookies)) {
response.headers.append('Set-Cookie', cookie);
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should append the headers here. They are already appended further up in the response, here:

astro/packages/astro/src/core/app/index.ts

Lines 346 to 350 in cb4d078

if (addCookieHeader) {
for (const setCookieHeaderValue of App.getSetCookieFromResponse(response)) {
response.headers.append('set-cookie', setCookieHeaderValue);
}
}

What I think is happening in this bug is that the Response has changed because of rewriting, so the AstroCookies object is not attached to it anymore. So what I think you can do is call attachCookiesToResponse here and then when the cookies are consumed further up the chain they will be appended to the response there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like they actually need to be merged, so I took a different approach

}
}
} catch (e) {
// If there is an error in the page's frontmatter or instantiation of the RenderTemplate fails midway,
// we signal to the rest of the internals that we can ignore the results of existing renders and avoid kicking off more of them.
Expand Down
7 changes: 7 additions & 0 deletions packages/astro/test/astro-cookies.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,13 @@ describe('Astro.cookies', () => {
assert.equal(response.headers.has('set-cookie'), true);
}
});

it('can set cookies in a rewritten request', async () => {
const response = await fixture.fetch('/from');
assert.equal(response.status, 200);

assert.equal(response.headers.get('set-cookie'), 'my_cookie=value');
});
});

describe('Production', () => {
Expand Down
4 changes: 4 additions & 0 deletions packages/astro/test/fixtures/astro-cookies/src/pages/from.astro
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
console.log('cookie value: ', Astro.cookies.get('my_cookie')?.value);
Copy link
Member

Choose a reason for hiding this comment

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

Should we add another test where in the first page we set a lorem value, then in the rewritten page we set a ipsum value, and assert that the cookie has ipsum as value?

return Astro.rewrite('/rewrite-target');
---
16 changes: 16 additions & 0 deletions packages/astro/test/fixtures/astro-cookies/src/pages/rewrite-target.astro
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
---
Astro.cookies.set('my_cookie', 'value')
---

<html lang="en">
<head>
<meta charset="utf-8" />
<link rel="icon" type="image/svg+xml" href="/favicon.svg" />
<meta name="viewport" content="width=device-width" />
<meta name="generator" content={Astro.generator} />
<title>Page 2</title>
</head>
<body>
<h1>Page 2</h1>
</body>
</html>
Loading