-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Add note that the bug appears only with trailingSlash: ignore #12130
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
Conversation
✅ Deploy Preview for astro-docs-2 ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
}, { | ||
path: '/', // Critical: ensures cookie works with trailingSlash: "ignore" | ||
httpOnly: true, | ||
sameSite: 'lax', | ||
maxAge: 60 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
httpOnly, sameSite, maxAge follow best practice, it could remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No problem with showing best practices, but we do want to make sure the emphasis here is on supporting how to deal with changed behaviour (since this is the upgrade guide); it's not meant to be a recipe teaching from scratch.
Will ask others to share if they find this helpful or potentially cluttering/distracting!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for handling this to address the issue, @jp-knj ! 🙌
I will ask others to comment on the extent of the changes here, as I've described below. My instinct is to simply correct the code example in the two places, as I don't feel any of the extra commentary is necessary. I would appreciate the input from others on the team!
Co-authored-by: Sarah Rainsberger <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, let's close an issue! 🎉 Thank you again @jp-knj !
Description (required)
This PR fixes a bug in the v5 upgrade guide's cookie-forwarding example for form actions. The example code was missing a path attribute when setting cookies, which causes actions to fail silently when using trailingSlash: "ignore" (the default with build.format: "directory").
The Problem:
Without an explicit path, cookies default to the request's directory path per RFC 6265. When a POST request to
/route
sets a cookie without a path, it's scoped to/route
. The subsequent redirect to /route/ (after PR: withastro/astro#13997) can't access this cookie, making it appear as if the action never executed.Related issues & labels (optional)
context.originPathname
includes the trailing slash when it shouldn't astro#14177 (comment)