edge-runtime icon indicating copy to clipboard operation
edge-runtime copied to clipboard

Allow setting cookies with special characters

Open abhi12299 opened this issue 1 year ago • 6 comments

This is a fix for https://github.com/vercel/next.js/issues/70523

What?

Access the ResponseCookie object using the NextResponse class in a nextjs middleware like so:

const response = NextResponse.next()
const data = {
  value: "bar 50%"
}

response.cookies.set({
  name: 'foo',
  value: JSON.stringify(data)
})

You'll notice that the cookie being set crashes the application. The reason for this is explained below:

  • ResponseCookie's constructor calls the parseSetCookie method: https://github.com/vercel/edge-runtime/blob/8312ccd3e507aa8683b3705f2ca2d9862183982b/packages/cookies/src/response-cookies.ts#L31-L33
  • parseSetCookie function calls the parseCookie function: https://github.com/vercel/edge-runtime/blob/8312ccd3e507aa8683b3705f2ca2d9862183982b/packages/cookies/src/serialize.ts#L54-L59
  • parseCookie calls the decodeURIComponent function on the cookie value: https://github.com/vercel/edge-runtime/blob/8312ccd3e507aa8683b3705f2ca2d9862183982b/packages/cookies/src/serialize.ts#L42-L44
  • parseSetCookie function again calls the decodeURIComponent function on line 75: https://github.com/vercel/edge-runtime/blob/8312ccd3e507aa8683b3705f2ca2d9862183982b/packages/cookies/src/serialize.ts#L73-L76

This double invocation of decodeURIComponent throws an error and crashes the application if the cookie contains special characters.

abhi12299 avatar Sep 29 '24 11:09 abhi12299

⚠️ No Changeset found

Latest commit: 6dcc952c1f9fed22d8b61c5cd29eac91e5a81c39

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

changeset-bot[bot] avatar Sep 29 '24 11:09 changeset-bot[bot]

@abhi12299 is attempting to deploy a commit to the Vercel Team on Vercel.

A member of the Team first needs to authorize it.

vercel[bot] avatar Sep 29 '24 11:09 vercel[bot]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
edge-runtime ⬜️ Skipped (Inspect) Oct 7, 2024 9:33am

vercel[bot] avatar Oct 07 '24 09:10 vercel[bot]

It's important for this particular framework that it adheres to the published RFC 6265 which requires that cookie values are URL encoded. We'd prefer in these cases to have the application throw an error during parsing if the cookie values can't be parsed rather than allowing un-encoded values from the cookie parser itself.

If the issue is stemming from double-decoding I'd rather solve that specifically rather than have it silently fail decoding.

wyattjoh avatar Oct 08 '24 15:10 wyattjoh

as per my limited understanding of this codebase, i would assume that the headers being set for set-cookie would still encode the cookie value. see here: https://github.com/vercel/edge-runtime/blob/37333f530635771e42c5cc81ca64534c432f2c29/packages/cookies/src/serialize.ts#L21

i may be wrong, but the outbound headers still encode the cookie value - this issue stems from double decoding. Am i missing something obvious here?

abhi12299 avatar Oct 08 '24 16:10 abhi12299

What is the purpose of calling decodeURIComponent when defining the new cookie object? The function parseCookie is already returning a new map with all values decoded. It seems redundant to me, or I'm missing something?

Sathosk avatar Oct 25 '24 14:10 Sathosk