feat: add cookies.parse method - #16203
Conversation
🦋 Changeset detectedLatest commit: 7114700 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 |
…s (e.g. base64 padding), corrupting session tokens
This commit fixes the issue reported at packages/kit/src/runtime/server/cookie.js:164
## Bug
In `packages/kit/src/runtime/server/cookie.js`, the `parse(header)` method extracted the cookie name and value with:
```js
const [name, value] = head.split('=');
```
`String.prototype.split('=')` splits on **every** `=` and destructuring keeps only the first two array elements. For a value containing `=` characters this silently drops everything after the first `=`.
### Concrete trigger
Cookie values are very commonly base64-encoded and end with `=` padding (e.g. session tokens). For the input:
```
sessionid=YWJjZA==
```
`head.split('=')` yields `['sessionid', 'YWJjZA', '', '']`, so `value` becomes `'YWJjZA'` — the `==` padding is lost and the value is corrupted. Any consumer relying on `parse()` (e.g. reading back a `Set-Cookie` value) gets a broken token, which can break authentication/session handling.
### Secondary issue
The attribute loop used `pair.split('=', 2)`. The `limit` argument to `String.split` does **not** keep the remainder — `'a=b=c'.split('=', 2)` returns `['a', 'b']`, dropping `'c'`. This truncates attribute values that contain `=` (e.g. a `Path` containing `=`).
## Fix
Both name/value and attribute parsing now split on only the **first** `=` using `indexOf` + `slice`, which preserves all `=` characters in the remainder:
```js
const head_index = head.indexOf('=');
const name = head_index === -1 ? head : head.slice(0, head_index);
const value = head_index === -1 ? '' : head.slice(head_index + 1);
```
and for attributes:
```js
const index = pair.indexOf('=');
const key = (index === -1 ? pair : pair.slice(0, index)).trim().toLowerCase();
const value = index === -1 ? undefined : pair.slice(index + 1).trim();
```
The attribute fix preserves the previous behaviour where a value-less attribute (e.g. `Secure`) yields `undefined` (the `samesite`/value-checking logic already handles `undefined`).
Verified with a standalone reproduction: `sessionid=YWJjZA==` now correctly parses to `value: 'YWJjZA=='`.
Co-authored-by: Vercel <vercel[bot]@users.noreply.github.com>
Co-authored-by: Rich-Harris <hello@rich-harris.dev>
…nc` `GET` function, which is a syntax error.
This commit fixes the issue reported at packages/kit/src/exports/public.d.ts:304
## Bug
The JSDoc example for `cookies.parse` in `packages/kit/src/exports/public.d.ts` (line 306) contained:
```js
export function GET() {
const { cookies } = getRequestEvent();
const response = await fetch('...');
// ...
}
```
`await` is used inside `GET`, but `GET` is declared as a plain (non-`async`) function. Using `await` outside of an `async` function is a `SyntaxError` in JavaScript, so anyone copying this documented example verbatim would get code that fails to parse/run.
The same example also appears in the generated `packages/kit/types/index.d.ts` (line 279).
## Fix
Changed the declaration to `export async function GET()` in both the source `.d.ts` (primary edit) and the generated `types/index.d.ts`, making the example valid and runnable.
Co-authored-by: Vercel <vercel[bot]@users.noreply.github.com>
Co-authored-by: Rich-Harris <hello@rich-harris.dev>
|
Is there any good reason not to just copy verbatim the It also conveniently answers your "what to do" question -- it's so widely adopted that IMO its behavior is basically the standard now |
|
Honestly I didn't even realise |
| * | ||
| * @param header A valid `Set-Cookie` header | ||
| */ | ||
| parse: (header: string, options: import('cookie').ParseOptions) => import('cookie').SetCookie; |
There was a problem hiding this comment.
I think you can just type this whole function as import('cookie').parseSetCookie right? Then you don't have to worry about declaring all of the args/return type yourself
| * @param {string} header | ||
| * @param {import('cookie').ParseOptions} options |
There was a problem hiding this comment.
| * @param {string} header | |
| * @param {import('cookie').ParseOptions} options | |
| * @type {import('cookie').parseSetCookie} |
Install latestInstall the latest version of With spi @sveltejs/kit --commit 7114700f353c5f22782ec7301470d73d313f1f66With pnpm add https://pkg.svelte.dev/@sveltejs/kit/c/7114700f353c5f22782ec7301470d73d313f1f66Commit URL: https://pkg.svelte.dev/@sveltejs/kit/c/7114700f353c5f22782ec7301470d73d313f1f66 |
elliott-with-the-longest-name-on-github
left a comment
There was a problem hiding this comment.
This looks good to me. The only possible concern I could have is that parse could be confusing between "parse a set-cookie header" (the thing the server sends to the client) and "parse a cookie value" (the thing the client sends back to the server). If we ever wanted to add the latter this naming kind of makes that awkward. But I can't see why we would want to add that...
|
yeah the incoming |
This PR was opened by the [Changesets release](https://github.com/changesets/action) GitHub action. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to version-3, this PR will be updated.⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ `version-3` is currently in **pre mode** so this branch has prereleases rather than normal releases. If you want to exit prereleases, run `changeset pre exit` on `version-3`.⚠️ ⚠️ ⚠️ ⚠️ ⚠️ ⚠️ # Releases ## @sveltejs/adapter-node@6.0.0-next.2 ### Major Changes - breaking: add `kit.paths.origin` config option, remove `kit.prerender.origin` and the `adapter-node` `ORIGIN` environment variable ([#16161](#16161)) ### Patch Changes - Updated dependencies [[`3c434fb`](3c434fb), [`a9284e8`](a9284e8), [`3726a7a`](3726a7a), [`f9d2240`](f9d2240), [`a9284e8`](a9284e8), [`7c040ba`](7c040ba), [`223eaad`](223eaad), [`223eaad`](223eaad), [`3b907d4`](3b907d4), [`fd628a5`](fd628a5), [`223eaad`](223eaad), [`6c1d035`](6c1d035), [`178eac0`](178eac0), [`c6562a9`](c6562a9), [`8eca2ab`](8eca2ab), [`61cf188`](61cf188)]: - @sveltejs/kit@3.0.0-next.6 ## @sveltejs/kit@3.0.0-next.6 ### Major Changes - breaking: return no content for 204 responses ([#16200](#16200)) - breaking: form action responses now use the HTTP status code returned from `fail` ([#16200](#16200)) - breaking: nested server-only directories ([#15685](#15685)) - breaking: add `kit.paths.origin` config option, remove `kit.prerender.origin` and the `adapter-node` `ORIGIN` environment variable ([#16161](#16161)) - breaking: don't abort navigation when calling `invalidate(All)` during navigation ([#16188](#16188)) - breaking: allow `handleError` to influence status code ([#16162](#16162)) - breaking: forbid external redirects by default ([#16198](#16198)) ### Minor Changes - feat: use `type: 'module'` for service worker registrations ([#16169](#16169)) - feat: add `dirty()` property to form fields ([#16208](#16208)) - feat: add `cookies.parse` method ([#16203](#16203)) ### Patch Changes - fix: drain unconsumed request bodies so keep-alive connections don't hang ([#16170](#16170)) - fix: properly handle Date objects in form.fields.set ([#16168](#16168)) - fix: skip clean fields when programmatically validating forms ([#16208](#16208)) - breaking: experimental remote form `validate({ includeUntouched })` option is now `all` ([#16208](#16208)) - fix: return `undefined` from `fields.branch.issues()` when only `fields.branch.leaf` has issues ([#16187](#16187)) - feat: add field.touched() helper to remote form fields ([#14692](#14692)) ## @sveltejs/adapter-cloudflare@8.0.0-next.1 ### Patch Changes - fix: avoid overriding user's existing `_headers` rules ([#16183](#16183)) - Updated dependencies [[`3c434fb`](3c434fb), [`a9284e8`](a9284e8), [`3726a7a`](3726a7a), [`f9d2240`](f9d2240), [`a9284e8`](a9284e8), [`7c040ba`](7c040ba), [`223eaad`](223eaad), [`223eaad`](223eaad), [`3b907d4`](3b907d4), [`fd628a5`](fd628a5), [`223eaad`](223eaad), [`6c1d035`](6c1d035), [`178eac0`](178eac0), [`c6562a9`](c6562a9), [`8eca2ab`](8eca2ab), [`61cf188`](61cf188)]: - @sveltejs/kit@3.0.0-next.6 Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
closes #13680
closes #13681
closes #8564
Adds a
cookies.parsemethod for dealing with cookie headers from external sources:Design decisions that might warrant discussion:
SameSite=Nopeinstead of SameSite=None`, nothing will happen. Maybe it should throw instead? Or maybe it should just apply the value even if it's gibberish, to make it future-proof?Expires,Max-Age,Secure,HttpOnly,Partitioned,Priority,SameSite,DomainandPathPlease 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. Changesets that add features should beminorand those that fix bugs should bepatch. Please prefix changeset messages withfeat:,fix:, orchore:.Edits