-
-
Notifications
You must be signed in to change notification settings - Fork 2k
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
feat: implement skew protection #11987
Conversation
🦋 Changeset detectedLatest commit: d9bb954 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Woohoo! Thank you for implementing this 🥳 |
packages/adapter-vercel/index.js
Outdated
value: 'document' | ||
}, | ||
headers: { | ||
'Set-Cookie': `__vdpl=${process.env.VERCEL_DEPLOYMENT_ID}; Path=/${builder.config.kit.paths.base}; SameSite=Strict; Secure; HttpOnly` |
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.
previously you deleted the cookie for the version.json
route - is that not necessary after all? Not necessary because it's not a document request?
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.
correct — we only care about non-document requests for this file. AFAICT there's no way to set multiple cookies (you can't do set-cookie: [...]
, and if you add a header in multiple continue
routes the last one wins), so we had to get rid of it
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.
wait, no, hang on... this is a problem — the version.json
will be requested with the old deployment ID. gah
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.
(this seemed to work in my testing, because the cookie was still in my browser from the earlier iteration 🤦 )
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.
I've 'fixed' this by setting the second set-cookie
header on the response for the entry point, since we know it will happen before the client hydrates. Since adapters don't have access to the build manifest, this involves some hackery, but hopefully we can remove it in future once the Build Output API supports setting multiple set-cookie
headers in a single response.
Co-authored-by: Ben McCann <[email protected]>
closes #10947
This implements Vercel Skew Protection. Tested manually
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm test
and lint the project withpnpm lint
andpnpm check
Changesets
pnpm changeset
and following the prompts. Changesets that add features should beminor
and those that fix bugs should bepatch
. Please prefix changeset messages withfeat:
,fix:
, orchore:
.Edits