Skip to content
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

add new cookies API #6593

Merged
merged 8 commits into from
Sep 6, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/grumpy-jobs-poke.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@sveltejs/kit': patch
---

[breaking] add API for interacting with cookies
6 changes: 3 additions & 3 deletions documentation/docs/03-routing.md
Original file line number Diff line number Diff line change
Expand Up @@ -141,7 +141,7 @@ export {};
import { error } from '@sveltejs/kit';

/** @type {import('./$types').Action} */
export async function POST({ request, setHeaders, url }) {
export async function POST({ cookies, request, url }) {
const values = await request.formData();

const username = /** @type {string} */ (values.get('username'));
Expand All @@ -167,8 +167,8 @@ export async function POST({ request, setHeaders, url }) {
};
}

setHeaders({
'set-cookie': createSessionCookie(user.id)
cookies.set('sessionid', createSessionCookie(user.id), {
httpOnly: true
});

return {
Expand Down
23 changes: 3 additions & 20 deletions documentation/docs/05-load.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ export function load(event) {

### Input properties

The argument to a `load` function is a `LoadEvent` (or, for server-only `load` functions, a `ServerLoadEvent` which inherits `clientAddress`, `locals`, `platform` and `request` from `RequestEvent`). All events have the following properties:
The argument to a `load` function is a `LoadEvent` (or, for server-only `load` functions, a `ServerLoadEvent` which inherits `clientAddress`, `cookies`, `locals`, `platform` and `request` from `RequestEvent`). All events have the following properties:

#### data

Expand Down Expand Up @@ -221,6 +221,7 @@ export async function load({ parent, fetch }) {
If you need to set headers for the response, you can do so using the `setHeaders` method. This is useful if you want the page to be cached, for example:

```js
// @errors: 2322
/// file: src/routes/blog/+page.js
/** @type {import('./$types').PageLoad} */
export async function load({ fetch, setHeaders }) {
Expand All @@ -240,25 +241,7 @@ export async function load({ fetch, setHeaders }) {

Setting the same header multiple times (even in separate `load` functions) is an error — you can only set a given header once.

The exception is `set-cookie`, which can be set multiple times and can be passed an array of strings:

```js
/// file: src/routes/+layout.server.js
/** @type {import('./$types').LayoutLoad} */
export async function load({ setHeaders }) {
setHeaders({
'set-cookie': 'a=1; HttpOnly'
});

setHeaders({
'set-cookie': 'b=2; HttpOnly'
});

setHeaders({
'set-cookie': ['c=3; HttpOnly', 'd=4; HttpOnly']
});
}
```
You cannot add a `set-cookie` header with `setHeaders` — use the [`cookies`](/docs/types#sveltejs-kit-cookies) API in a server-only `load` function instead.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes me wonder if we should have a section "inputs exclusive to isomorphic load" and "inputs exlusive to server-only load" in here (can be done in a separate PR)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably yeah, since we haven't actually introduced RequestEvent by this stage


### Output

Expand Down
4 changes: 2 additions & 2 deletions documentation/docs/06-hooks.md
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ declare namespace App {
}
}

const getUserInformation: (cookie: string | null) => Promise<User>;
const getUserInformation: (cookie: string | undefined) => Promise<User>;

// declare global {
// const getUserInformation: (cookie: string) => Promise<User>;
Expand All @@ -50,7 +50,7 @@ const getUserInformation: (cookie: string | null) => Promise<User>;
// ---cut---
/** @type {import('@sveltejs/kit').Handle} */
export async function handle({ event, resolve }) {
event.locals.user = await getUserInformation(event.request.headers.get('cookie'));
event.locals.user = await getUserInformation(event.cookies.get('sessionid'));

const response = await resolve(event);
response.headers.set('x-custom-header', 'potato');
Expand Down
76 changes: 76 additions & 0 deletions packages/kit/src/runtime/server/cookie.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,76 @@
import * as cookie from 'cookie';

/**
* @param {Request} request
* @param {URL} url
*/
export function get_cookies(request, url) {
const initial_cookies = cookie.parse(request.headers.get('cookie') ?? '');

/** @type {Array<{ name: string, value: string, options: import('cookie').CookieSerializeOptions }>} */
const new_cookies = [];

/** @type {import('types').Cookies} */
const cookies = {
get(name, opts) {
const decode = opts?.decode || decodeURIComponent;

let i = new_cookies.length;
while (i--) {
const cookie = new_cookies[i];

if (
cookie.name === name &&
domain_matches(url.hostname, cookie.options.domain) &&
path_matches(url.pathname, cookie.options.path)
) {
return cookie.value;
}
}

return name in initial_cookies ? decode(initial_cookies[name]) : undefined;
},
set(name, value, options = {}) {
new_cookies.push({
name,
value,
options: {
httpOnly: true,
secure: true,
...options
}
});
},
delete(name) {
new_cookies.push({ name, value: '', options: { expires: new Date(0) } });
}
};

return { cookies, new_cookies };
}

/**
* @param {string} hostname
* @param {string} [constraint]
*/
export function domain_matches(hostname, constraint) {
if (!constraint) return true;

const normalized = constraint[0] === '.' ? constraint.slice(1) : constraint;

if (hostname === normalized) return true;
return hostname.endsWith('.' + normalized);
}

/**
* @param {string} path
* @param {string} [constraint]
*/
export function path_matches(path, constraint) {
if (!constraint) return true;

const normalized = constraint.endsWith('/') ? constraint.slice(0, -1) : constraint;

if (path === normalized) return true;
return path.startsWith(normalized + '/');
}
35 changes: 21 additions & 14 deletions packages/kit/src/runtime/server/index.js
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import * as cookie from 'cookie';
import { render_endpoint } from './endpoint.js';
import { render_page } from './page/index.js';
import { render_response } from './page/render.js';
Expand All @@ -8,6 +9,7 @@ import { decode_params, disable_search, normalize_path } from '../../utils/url.j
import { exec } from '../../utils/routing.js';
import { render_data } from './data/index.js';
import { DATA_SUFFIX } from '../../constants.js';
import { get_cookies } from './cookie.js';

/* global __SVELTEKIT_ADAPTER_NAME__ */

Expand Down Expand Up @@ -116,16 +118,16 @@ export async function respond(request, options, state) {
}
}

/** @type {import('types').ResponseHeaders} */
/** @type {Record<string, string>} */
const headers = {};

/** @type {string[]} */
const cookies = [];
const { cookies, new_cookies } = get_cookies(request, url);

if (state.prerendering) disable_search(url);

/** @type {import('types').RequestEvent} */
const event = {
cookies,
getClientAddress:
state.getClientAddress ||
(() => {
Expand All @@ -144,15 +146,9 @@ export async function respond(request, options, state) {
const value = new_headers[key];

if (lower === 'set-cookie') {
const new_cookies = /** @type {string[]} */ (Array.isArray(value) ? value : [value]);

for (const cookie of new_cookies) {
if (cookies.includes(cookie)) {
throw new Error(`"${key}" header already has cookie with same value`);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this check is no longer done in the new version - any drawbacks omitting it and we should add it back, or is it fine like this?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's harmless (if pointless) to duplicate cookies, the later one wins. To recreate this check we'd have to serialize eagerly, but it would only catch this fairly weird edge case — would probably make more sense to prevent different cookies, but I didn't want to get into questions about what to do with domain or path don't match, so I figured I'd just rely on people not being daft

}

cookies.push(cookie);
}
throw new Error(
`Use \`event.cookie.set(name, value, options)\` instead of \`event.setHeaders\` to set cookies`

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should't this be event.cookies.set?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep. fixed

);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of throwing here, couldn't we invoke something like cookies.set(cookie.parse(value)) here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could, but no-one wants to write this...

import * as cookie from 'cookie';

export const actions = {
  default: async ({ setHeaders }) => {
    // ...
    setHeaders({
      'set-cookie': cookie.serialize('answer', 42, {
        httpOnly: true
      })
    });
   }
};

...when they could write this:

export const actions = {
  default: async ({ cookies }) => {
    // ...
    cookies.set('answer', 42);
   }
};

I for one would be grateful for an error message that said 'you should delete a bunch of code, and you can probably remove the cookie dependency'.

It also feels very weird if you use setHeaders followed by cookies.get — it doesn't feel like it should work, and yet it does. Always better to have one way to do things.

} else if (lower in headers) {
throw new Error(`"${key}" header is already set`);
} else {
Expand Down Expand Up @@ -275,8 +271,11 @@ export async function respond(request, options, state) {
}
}

for (const cookie of cookies) {
response.headers.append('set-cookie', cookie);
for (const new_cookie of new_cookies) {
response.headers.append(
'set-cookie',
cookie.serialize(new_cookie.name, new_cookie.value, new_cookie.options)
);
}

// respond with 304 if etag matches
Expand Down Expand Up @@ -338,6 +337,14 @@ export async function respond(request, options, state) {
} catch (e) {
const error = coalesce_to_error(e);
return handle_fatal_error(event, options, error);
} finally {
event.cookies.set = () => {
throw new Error('Cannot use `cookies.set(...)` after the response has been generated');
};

event.setHeaders = () => {
throw new Error('Cannot use `setHeaders(...)` after the response has been generated');
};
}
}

Expand Down
25 changes: 0 additions & 25 deletions packages/kit/src/runtime/server/page/cookie.js

This file was deleted.

2 changes: 1 addition & 1 deletion packages/kit/src/runtime/server/page/fetch.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import * as cookie from 'cookie';
import * as set_cookie_parser from 'set-cookie-parser';
import { respond } from '../index.js';
import { domain_matches, path_matches } from './cookie.js';
import { domain_matches, path_matches } from '../cookie.js';

/**
* @param {{
Expand Down
4 changes: 1 addition & 3 deletions packages/kit/test/apps/basics/src/hooks.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import fs from 'fs';
import cookie from 'cookie';
import { sequence } from '@sveltejs/kit/hooks';

/** @type {import('@sveltejs/kit').HandleError} */
Expand All @@ -23,8 +22,7 @@ export const handle = sequence(
return resolve(event);
},
({ event, resolve }) => {
const cookies = cookie.parse(event.request.headers.get('cookie') || '');
event.locals.name = cookies.name;
event.locals.name = event.cookies.get('name');
return resolve(event);
},
async ({ event, resolve }) => {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
export function load({ setHeaders }) {
setHeaders({
'set-cookie': 'cookie1=value1'
/** @type {import('./$types').LayoutServerLoad} */
export function load({ cookies }) {
cookies.set('cookie1', 'value1', {
secure: false // safari
});
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
export function load({ setHeaders }) {
setHeaders({
'set-cookie': 'cookie2=value2'
/** @type {import('./$types').PageServerLoad} */
export function load({ cookies }) {
cookies.set('cookie2', 'value2', {
secure: false // safari
});
}
Original file line number Diff line number Diff line change
@@ -1,11 +1,8 @@
import { json } from '@sveltejs/kit';

/** @type {import('./$types').RequestHandler} */
export function GET({ request }) {
const cookie = request.headers.get('cookie');

const match = /answer=([^;]+)/.exec(cookie);
const answer = +match?.[1];
export function GET({ cookies }) {
const answer = +cookies.get('answer');

return json(
{ answer },
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
import { redirect } from '@sveltejs/kit';

export function load({ setHeaders }) {
setHeaders({ 'set-cookie': 'shadow-redirect=happy' });
/** @type {import('./$types').PageServerLoad} */
export function load({ cookies }) {
cookies.set('shadow-redirect', 'happy', {
secure: false // safari
});
throw redirect(302, '/shadowed/redirected');
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
import { redirect } from '@sveltejs/kit';

export function POST({ setHeaders }) {
setHeaders({ 'set-cookie': 'shadow-redirect=happy' });
/** @type {import('./$types').PageServerLoad} */
export function POST({ cookies }) {
cookies.set('shadow-redirect', 'happy', {
secure: false // safari
});
throw redirect(302, '/shadowed/redirected');
}
Loading