security: Stop automatically adding URLs from server-side load fetch calls to dependencies (#9945)

elliott-with-the-longest-name-on-github · Rich Harris · web-flow · commit 535905cdb02e · 2023-05-17T14:58:14.000-04:00
* feat: Add `dangerZone` config

* breaking: Don't implicitly track deps in server-side fetch

* changeset

* bein dumb

* fix: Server load invalidation

* fix: Write config for server

* fix: test

* unsurprisingly i am dumb

* docs: Clarify difference between server `fetch` and universal `fetch`

* tests

* rename to trackServerFetches

---------

Co-authored-by: Rich Harris &lt;git@rich-harris.dev&gt;
diff --git a/.changeset/shaggy-moons-sort.md b/.changeset/shaggy-moons-sort.md
@@ -0,0 +1,5 @@
+---
+'@sveltejs/kit': minor
+---
+
+security: Stop implicitly tracking URLs as dependencies in server-side `load`s
diff --git a/documentation/docs/20-core-concepts/20-load.md b/documentation/docs/20-core-concepts/20-load.md
@@ -537,7 +537,7 @@ Dependency tracking does not apply _after_ the `load` function has returned —
 
 ### Manual invalidation
 
-You can also re-run `load` functions that apply to the current page using [`invalidate(url)`](modules#$app-navigation-invalidate), which re-runs all `load` functions that depend on `url`, and [`invalidateAll()`](modules#$app-navigation-invalidateall), which re-runs every `load` function.
+You can also re-run `load` functions that apply to the current page using [`invalidate(url)`](modules#$app-navigation-invalidate), which re-runs all `load` functions that depend on `url`, and [`invalidateAll()`](modules#$app-navigation-invalidateall), which re-runs every `load` function. Server load functions will never automatically depend on a fetched `url` to avoid leaking secrets to the client.
 
 A `load` function depends on `url` if it calls `fetch(url)` or `depends(url)`. Note that `url` can be a custom identifier that starts with `[a-z]:`:
 
@@ -585,7 +585,7 @@ To summarize, a `load` function will re-run in the following situations:
 - It references a property of `params` whose value has changed
 - It references a property of `url` (such as `url.pathname` or `url.search`) whose value has changed. Properties in `request.url` are _not_ tracked
 - It calls `await parent()` and a parent `load` function re-ran
-- It declared a dependency on a specific URL via [`fetch`](#making-fetch-requests) or [`depends`](types#public-types-loadevent), and that URL was marked invalid with [`invalidate(url)`](modules#$app-navigation-invalidate)
+- It declared a dependency on a specific URL via [`fetch`](#making-fetch-requests) (universal load only) or [`depends`](types#public-types-loadevent), and that URL was marked invalid with [`invalidate(url)`](modules#$app-navigation-invalidate)
 - All active `load` functions were forcibly re-run with [`invalidateAll()`](modules#$app-navigation-invalidateall)
 
 `params` and `url` can change in response to a `<a href="..">` link click, a [`<form>` interaction](form-actions#get-vs-post), a [`goto`](modules#$app-navigation-goto) invocation, or a [`redirect`](modules#sveltejs-kit-redirect).
diff --git a/packages/kit/src/core/config/index.spec.js b/packages/kit/src/core/config/index.spec.js
@@ -69,6 +69,9 @@ const get_defaults = (prefix = '') => ({
 		csrf: {
 			checkOrigin: true
 		},
+		dangerZone: {
+			trackServerFetches: false
+		},
 		embedded: false,
 		env: {
 			dir: process.cwd(),
diff --git a/packages/kit/src/core/config/options.js b/packages/kit/src/core/config/options.js
@@ -111,6 +111,11 @@ const options = object(
 				checkOrigin: boolean(true)
 			}),
 
+			dangerZone: object({
+				// TODO 2.0: Remove this
+				trackServerFetches: boolean(false)
+			}),
+
 			embedded: boolean(false),
 
 			env: object({
diff --git a/packages/kit/src/core/sync/write_server.js b/packages/kit/src/core/sync/write_server.js
@@ -34,6 +34,7 @@ export const options = {
 	app_template_contains_nonce: ${template.includes('%sveltekit.nonce%')},
 	csp: ${s(config.kit.csp)},
 	csrf_check_origin: ${s(config.kit.csrf.checkOrigin)},
+	track_server_fetches: ${s(config.kit.dangerZone.trackServerFetches)},
 	embedded: ${config.kit.embedded},
 	env_public_prefix: '${config.kit.env.publicPrefix}',
 	hooks: null, // added lazily, via \`get_hooks\`
diff --git a/packages/kit/src/runtime/server/data/index.js b/packages/kit/src/runtime/server/data/index.js
@@ -76,7 +76,8 @@ export async function render_data(
 								}
 							}
 							return data;
-						}
+						},
+						track_server_fetches: options.track_server_fetches
 					});
 				} catch (e) {
 					aborted = true;
diff --git a/packages/kit/src/runtime/server/page/index.js b/packages/kit/src/runtime/server/page/index.js
@@ -150,7 +150,8 @@ export async function render_page(event, page, options, manifest, state, resolve
 								if (parent) Object.assign(data, await parent.data);
 							}
 							return data;
-						}
+						},
+						track_server_fetches: options.track_server_fetches
 					});
 				} catch (e) {
 					load_error = /** @type {Error} */ (e);
diff --git a/packages/kit/src/runtime/server/page/load_data.js b/packages/kit/src/runtime/server/page/load_data.js
@@ -10,10 +10,18 @@ import { validate_depends } from '../../shared.js';
  *   state: import('types').SSRState;
  *   node: import('types').SSRNode | undefined;
  *   parent: () => Promise<Record<string, any>>;
+ *   track_server_fetches: boolean;
  * }} opts
  * @returns {Promise<import('types').ServerDataNode | null>}
  */
-export async function load_server_data({ event, state, node, parent }) {
+export async function load_server_data({
+	event,
+	state,
+	node,
+	parent,
+	// TODO 2.0: Remove this
+	track_server_fetches
+}) {
 	if (!node?.server) return null;
 
 	let done = false;
@@ -51,7 +59,10 @@ export async function load_server_data({ event, state, node, parent }) {
 				);
 			}
 
-			uses.dependencies.add(url.href);
+			// TODO 2.0: Remove this
+			if (track_server_fetches) {
+				uses.dependencies.add(url.href);
+			}
 
 			return event.fetch(info, init);
 		},
diff --git a/packages/kit/src/runtime/server/page/respond_with_error.js b/packages/kit/src/runtime/server/page/respond_with_error.js
@@ -44,7 +44,8 @@ export async function respond_with_error({
 				event,
 				state,
 				node: default_layout,
-				parent: async () => ({})
+				parent: async () => ({}),
+				track_server_fetches: options.track_server_fetches
 			});
 
 			const server_data = await server_data_promise;
diff --git a/packages/kit/test/apps/basics/test/client.test.js b/packages/kit/test/apps/basics/test/client.test.js
@@ -456,14 +456,15 @@ test.describe('Invalidation', () => {
 		expect(shared).not.toBe(next_shared);
 	});
 
-	test('fetch in server load can be invalidated', async ({ page, app, request }) => {
+	test('fetch in server load cannot be invalidated', async ({ page, app, request }) => {
+		// TODO 2.0: Can remove this test after `dangerZone.trackServerFetches` and associated code is removed
 		await request.get('/load/invalidation/server-fetch/count.json?reset');
 		await page.goto('/load/invalidation/server-fetch');
 		const selector = '[data-testid="count"]';
 
 		expect(await page.textContent(selector)).toBe('1');
 		await app.invalidate('/load/invalidation/server-fetch/count.json');
-		expect(await page.textContent(selector)).toBe('2');
+		expect(await page.textContent(selector)).toBe('1');
 	});
 
 	test('+layout.js is re-run when shared dep is invalidated', async ({ page }) => {
diff --git a/packages/kit/test/apps/options/source/pages/server-fetch-invalidate/+page.server.js b/packages/kit/test/apps/options/source/pages/server-fetch-invalidate/+page.server.js
@@ -0,0 +1,6 @@
+// TODO 2.0: Delete
+/** @type {import('./$types').PageServerLoad} */
+export async function load({ fetch }) {
+	const res = await fetch('/path-base/server-fetch-invalidate/count.json');
+	return res.json();
+}
diff --git a/packages/kit/test/apps/options/source/pages/server-fetch-invalidate/+page.svelte b/packages/kit/test/apps/options/source/pages/server-fetch-invalidate/+page.svelte
@@ -0,0 +1,6 @@
+<script>
+	// TODO 2.0: Delete
+	export let data;
+</script>
+
+<p data-testid="count">{data.count}</p>
diff --git a/packages/kit/test/apps/options/source/pages/server-fetch-invalidate/count.json/+server.js b/packages/kit/test/apps/options/source/pages/server-fetch-invalidate/count.json/+server.js
@@ -0,0 +1,10 @@
+// TODO 2.0: Delete
+import { json } from '@sveltejs/kit';
+
+let count = 0;
+
+/** @type {import('./$types').RequestHandler} */
+export function GET({ url }) {
+	if (url.searchParams.has('reset')) count = 0;
+	return json({ count: count++ });
+}
diff --git a/packages/kit/test/apps/options/svelte.config.js b/packages/kit/test/apps/options/svelte.config.js
@@ -9,6 +9,9 @@ const config = {
 				'require-trusted-types-for': ['script']
 			}
 		},
+		dangerZone: {
+			trackServerFetches: true
+		},
 		files: {
 			assets: 'public',
 			lib: 'source/components',
diff --git a/packages/kit/test/apps/options/test/test.js b/packages/kit/test/apps/options/test/test.js
@@ -297,3 +297,22 @@ test.describe('Routing', () => {
 		await expect(page.locator('h2')).toHaveText('target: 0');
 	});
 });
+
+test.describe('load', () => {
+	// TODO 2.0: Remove this test
+	test('fetch in server load can be invalidated when `dangerZone.trackServerFetches` is set', async ({
+		page,
+		app,
+		request,
+		javaScriptEnabled
+	}) => {
+		test.skip(!javaScriptEnabled, 'JavaScript is disabled');
+		await request.get('/path-base/server-fetch-invalidate/count.json?reset');
+		await page.goto('/path-base/server-fetch-invalidate');
+		const selector = '[data-testid="count"]';
+
+		expect(await page.textContent(selector)).toBe('1');
+		await app.invalidate('/path-base/server-fetch-invalidate/count.json');
+		expect(await page.textContent(selector)).toBe('2');
+	});
+});
diff --git a/packages/kit/test/prerendering/basics/test/tests.spec.js b/packages/kit/test/prerendering/basics/test/tests.spec.js
@@ -170,9 +170,7 @@ test('fetches data from local endpoint', () => {
 			{
 				type: 'data',
 				data: [{ message: 1 }, 'hello'],
-				uses: {
-					dependencies: ['http://example.com/origin/message.json']
-				}
+				uses: {}
 			}
 		]
 	});
diff --git a/packages/kit/types/index.d.ts b/packages/kit/types/index.d.ts
@@ -343,6 +343,16 @@ export interface KitConfig {
 		 */
 		checkOrigin?: boolean;
 	};
+	/**
+	 * Here be dragons. Enable at your peril.
+	 */
+	dangerZone?: {
+		/**
+		 * Automatically add server-side `fetch`ed URLs to the `dependencies` map of `load` functions. This will expose secrets
+		 * to the client if your URL contains them.
+		 */
+		trackServerFetches?: boolean;
+	};
 	/**
 	 * Whether or not the app is embedded inside a larger app. If `true`, SvelteKit will add its event listeners related to navigation etc on the parent of `%sveltekit.body%` instead of `window`, and will pass `params` from the server rather than inferring them from `location.pathname`.
 	 * @default false
diff --git a/packages/kit/types/internal.d.ts b/packages/kit/types/internal.d.ts
@@ -333,6 +333,7 @@ export interface SSROptions {
 	app_template_contains_nonce: boolean;
 	csp: ValidatedConfig['kit']['csp'];
 	csrf_check_origin: boolean;
+	track_server_fetches: boolean;
 	embedded: boolean;
 	env_public_prefix: string;
 	hooks: ServerHooks;

-Original file line number
+Diff line change
@@ @@ -0,0 +1,5 @@ @@
 +---
 +'@sveltejs/kit': minor
 +---
++
 +security: Stop implicitly tracking URLs as dependencies in server-side `load`s
Original file line number	Diff line number	Diff line change
`@@ -76,7 +76,8 @@ export async function render_data(`
`76`	`76`	`}`
`77`	`77`	`}`
`78`	`78`	`return data;`
`79`		`- }`
	`79`	`+ },`
	`80`	`+ track_server_fetches: options.track_server_fetches`
`80`	`81`	`});`
`81`	`82`	`} catch (e) {`
`82`	`83`	`aborted = true;`