external fetch calls: ensure serialized cookie values are url-encoded #7736
Conversation
TLDR
before:
```
browser
|
(request with url-encoded cookie) (cookie=foo%2Bbar)
|
v
sveltekit node app
|
(external fetch request made with url-DEcoded cookie) (cookie=foo+bar)
|
v
backend service
```
after:
```
browser
|
(request with url-encoded cookie) (cookie=foo%2Bbar)
|
v
sveltekit node app
|
(external fetch request made with url-ENcoded cookie) (cookie=foo%2Bbar)
|
v
backend service
```
My kit app was making external `fetch` requests with cookies that were different
from the the cookies that were part of the original request to the app.
My request coming from the browser had a session cookie that was
url-encoded. My sveltekit server code would then use the kit-`fetch` to
make a request to a backend api server that uses that same session token
in the cookie. But when the request to the api left the kit app, the
cookie was not url-encoded anymore. This caused problems in my api
server because it would parse the
cookies by doing a url-decode on the values. Given my session token
could contain characters that could further url-decode, it would get
garbled when it was url-decoded by the api, because it was not
url-encoded anymore.
From my superficial web readings, there seems to be no hard standard
on the encoding of cookies. But the industry trend seems to be to
url-encode cookie values.
Given that:
a) The industry trend seems to be to url-encode cookie values.
b) This cookie.js code already url-decodes cookies on the way in.
c) "Forwarded" input cookies are different on the way out than on the
way in
I assumed the attached fix is reasonable, that cookies should just
always be url-encoded.
🦋 Changeset detectedLatest commit: 519a24f 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 |
ximus
changed the title
|
if this change makes sense, then I assume there needs to be a changeset as this is a change in behavior. |
There was a problem hiding this comment.
I think the change makes sense - so yes, please add a changeset 👍 We need to move the encoding step into the previous for loops though. Theoretically, someone could have invoked cookies.set with a different encoding option, which we need to use (line 177 something like .. = (cookie.options.encode || encodeURIComponent)(cookie.value);)
|
lgtm. stackoverflow suggests this is the right solution: https://stackoverflow.com/questions/1969232/what-are-allowed-characters-in-cookies |
|
Thanks @dummdidumm, refactored to honour any Also created two top level Added a changeset. kept as separate commits for ease of PR review but you can squash on merge or I can squash myself, let me know. |
TLDR
before: external fetch cookie was sent plain
after: external fetch cookie is url-encoded
Description
My kit app was making external
fetchrequests with cookies that were different from the the cookies that were part of the original request to the app.My request coming from the browser had a session cookie that was url-encoded. My sveltekit server code would then use the kit-
fetchto make a request to a backend api server that uses that same session token in the cookie. But when the request to the api left the kit app, the cookie was not url-encoded anymore. This caused problems in my api server because it would parse thecookies by doing a url-decode on the values. Given my session token could contain characters that could further url-decode, it would get garbled when it was url-decoded by the api, because it was not url-encoded anymore.
From my superficial web readings, there seems to be no hard standard on the encoding of cookies. But the industry trend seems to be to url-encode cookie values.
Conclusion
Given that:
I assumed the attached fix is reasonable, that cookies should just always be url-encoded.
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm testand lint the project withpnpm lintandpnpm checkChangesets
pnpm changesetand following the prompts. All changesets should bepatchuntil SvelteKit 1.0