fix(vercel): use SameSite=Lax for skew protection cookie#4422
Conversation
|
@thribhuvan003 is attempting to deploy a commit to the Nitro Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe Vercel skew protection rule changes the ChangesVercel skew protection
Estimated code review effort: 1 (Trivial) | ~2 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/unit/vercel-skew.test.ts (1)
45-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding an edge case for
skewProtectionenabled butdeploymentIdmissing.The implementation guards with
nitro.options.vercel?.skewProtection && nitro.options.manifest?.deploymentId— whendeploymentIdis undefined the route is skipped. A test covering this case would prevent regressions if the guard condition is ever modified.♻️ Suggested additional test
it("does not emit __vdpl route when skew protection is off", () => { const config = generateBuildConfig(mockNitro({ skewProtection: false })); const skewRoute = config.routes?.find( (r: any) => r.headers?.["Set-Cookie"]?.includes("__vdpl=") ); expect(skewRoute).toBeUndefined(); }); + + it("does not emit __vdpl route when deploymentId is missing", () => { + const config = generateBuildConfig( + mockNitro({ skewProtection: true, deploymentId: undefined }) + ); + const skewRoute = config.routes?.find( + (r: any) => r.headers?.["Set-Cookie"]?.includes("__vdpl=") + ); + expect(skewRoute).toBeUndefined(); + }); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unit/vercel-skew.test.ts` around lines 45 - 51, Add a test alongside “does not emit __vdpl route when skew protection is off” covering skewProtection enabled with no deploymentId in the mock Nitro configuration, and assert that generateBuildConfig produces no route whose Set-Cookie header includes "__vdpl=". Use the existing mockNitro and route lookup pattern.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@test/unit/vercel-skew.test.ts`:
- Around line 45-51: Add a test alongside “does not emit __vdpl route when skew
protection is off” covering skewProtection enabled with no deploymentId in the
mock Nitro configuration, and assert that generateBuildConfig produces no route
whose Set-Cookie header includes "__vdpl=". Use the existing mockNitro and route
lookup pattern.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b5570675-3a3a-4e03-8602-91d39e2eac08
📒 Files selected for processing (2)
src/presets/vercel/utils.tstest/unit/vercel-skew.test.ts
Strict skipped the __vdpl pin on cross-site top-level navigations, so new HTML could load while /_nuxt assets stayed pinned to an old deploy.
60cb671 to
3b6ce93
Compare
pi0
left a comment
There was a problem hiding this comment.
LGTM, however this is diverging from sveltekit and vercel docs. keeping open little bit more. /cc @danielroe
|
i think Lax is the more correct option |
Co-authored-by: Pooya Parsa <pyapar@gmail.com>
SameSite=Lax for skew protection cookie
🔗 Linked issue
fixes #4414
❓ Type of change
📚 Description
skew pin cookie was SameSite=Strict, so after coming in from another site the html refreshed but /_nuxt assets still had the old __vdpl and 404'd. switched to Lax so the pin updates with the document. still Secure + HttpOnly.
unit test on the generated set-cookie — fails with Strict, passes with Lax.
📝 Checklist