breaking: disallow external navigation using goto (#11207)

* breaking: disallow external navigation using `goto` by default closes #8775 * do it here instead * adjust test * remove option, TODO tests * tweak message * Update .changeset/silent-games-taste.md * add a test, remove an obsolete test * fix test * prettier * fix * Update packages/kit/src/exports/public.d.ts Co-authored-by: Ben McCann <[email protected]> * Update packages/kit/src/runtime/client/client.js Co-authored-by: Ben McCann <[email protected]> * fix test * DRY out --------- Co-authored-by: Rich Harris <[email protected]> Co-authored-by: Rich Harris <[email protected]> Co-authored-by: Ben McCann <[email protected]>
sveltejs · Dec 9, 2023 · dab9817 · dab9817
1 parent 8e7cfef
commit dab9817
Show file tree

Hide file tree

Showing 8 changed files with 66 additions and 30 deletions.
diff --git a/.changeset/silent-games-taste.md b/.changeset/silent-games-taste.md
@@ -0,0 +1,5 @@
+---
+'@sveltejs/kit': major
+---
+
+breaking: disallow external navigation with `goto`
diff --git a/packages/kit/src/exports/public.d.ts b/packages/kit/src/exports/public.d.ts
@@ -846,7 +846,7 @@ export interface Navigation {
 	/**
 	 * The type of navigation:
 	 * - `form`: The user submitted a `<form>`
-	 * - `leave`: The user is leaving the app by closing the tab or using the back/forward buttons to go to a different document
+	 * - `leave`: The app is being left either because the tab is being closed or a navigation to a different document is occurring
 	 * - `link`: Navigation was triggered by a link click
 	 * - `goto`: Navigation was triggered by a `goto(...)` call or a redirect
 	 * - `popstate`: Navigation was triggered by back/forward navigation

diff --git a/packages/kit/src/runtime/app/navigation.js b/packages/kit/src/runtime/app/navigation.js
@@ -23,7 +23,7 @@ export const disableScrollHandling = /* @__PURE__ */ client_method('disable_scro
  * @param {boolean} [opts.replaceState] If `true`, will replace the current `history` entry rather than creating a new one with `pushState`
  * @param {boolean} [opts.noScroll] If `true`, the browser will maintain its scroll position rather than scrolling to the top of the page after navigation
  * @param {boolean} [opts.keepFocus] If `true`, the currently focused element will retain focus after navigation. Otherwise, focus will be reset to the body
- * @param {boolean} [invalidateAll] If `true`, all `load` functions of the page will be rerun. See https://kit.svelte.dev/docs/load#rerunning-load-functions for more info on invalidation.
+ * @param {boolean} [opts.invalidateAll] If `true`, all `load` functions of the page will be rerun. See https://kit.svelte.dev/docs/load#rerunning-load-functions for more info on invalidation.
  * @param {any} [opts.state] The state of the new/updated history entry
  * @returns {Promise<void>}
  */

diff --git a/packages/kit/src/runtime/client/client.js b/packages/kit/src/runtime/client/client.js
@@ -18,7 +18,7 @@ import { parse } from './parse.js';
 import * as storage from './session-storage.js';
 import {
 	find_anchor,
-	get_base_uri,
+	resolve_url,
 	get_link_info,
 	get_router_options,
 	is_external_url,
@@ -235,12 +235,8 @@ export function create_client(app, target) {
 		redirect_count,
 		nav_token
 	) {
-		if (typeof url === 'string') {
-			url = new URL(url, get_base_uri(document));
-		}
-
 		return navigate({
-			url,
+			url: resolve_url(url),
 			scroll: noScroll ? scroll_state() : null,
 			keepfocus: keepFocus,
 			redirect_count,
@@ -1375,8 +1371,20 @@ export function create_client(app, target) {
 			}
 		},
 
-		goto: (href, opts = {}) => {
-			return goto(href, opts, 0);
+		goto: (url, opts = {}) => {
+			url = resolve_url(url);
+
+			if (url.origin !== origin) {
+				return Promise.reject(
+					new Error(
+						DEV
+							? `Cannot use \`goto\` with an external URL. Use \`window.location = "${url}"\` instead`
+							: 'goto: invalid URL'
+					)
+				);
+			}
+
+			return goto(url, opts, 0);
 		},
 
 		invalidate: (resource) => {
@@ -1396,7 +1404,7 @@ export function create_client(app, target) {
 		},
 
 		preload_data: async (href) => {
-			const url = new URL(href, get_base_uri(document));
+			const url = resolve_url(href);
 			const intent = get_navigation_intent(url, false);
 
 			if (!intent) {

diff --git a/packages/kit/src/runtime/client/utils.js b/packages/kit/src/runtime/client/utils.js
@@ -8,16 +8,18 @@ import { PRELOAD_PRIORITIES } from './constants.js';
 
 export const origin = BROWSER ? location.origin : '';
 
-/** @param {HTMLDocument} doc */
-export function get_base_uri(doc) {
-	let baseURI = doc.baseURI;
+/** @param {string | URL} url */
+export function resolve_url(url) {
+	if (url instanceof URL) return url;
+
+	let baseURI = document.baseURI;
 
 	if (!baseURI) {
-		const baseTags = doc.getElementsByTagName('base');
-		baseURI = baseTags.length ? baseTags[0].href : doc.URL;
+		const baseTags = document.getElementsByTagName('base');
+		baseURI = baseTags.length ? baseTags[0].href : document.URL;
 	}
 
-	return baseURI;
+	return new URL(url, baseURI);
 }
 
 export function scroll_state() {

diff --git a/packages/kit/test/apps/basics/src/routes/goto/+page.svelte b/packages/kit/test/apps/basics/src/routes/goto/+page.svelte
@@ -0,0 +1,17 @@
+<script>
+	import { goto } from '$app/navigation';
+
+	let message = '...';
+</script>
+
+<button
+	on:click={async () => {
+		try {
+			await goto('https://example.com');
+		} catch (e) {
+			message = e.message;
+		}
+	}}>goto</button
+>
+
+<p>{message}</p>
diff --git a/packages/kit/test/apps/basics/test/client.test.js b/packages/kit/test/apps/basics/test/client.test.js
@@ -841,3 +841,15 @@ test.describe('Assets', () => {
 		).toBe(true);
 	});
 });
+
+test.describe('goto', () => {
+	test('goto fails with external URL', async ({ page }) => {
+		await page.goto('/goto');
+		await page.click('button');
+
+		const message = process.env.DEV
+			? 'Cannot use `goto` with an external URL. Use `window.location = "https://example.com/"` instead'
+			: 'goto: invalid URL';
+		await expect(page.locator('p')).toHaveText(message);
+	});
+});
diff --git a/packages/kit/test/apps/basics/test/cross-platform/client.test.js b/packages/kit/test/apps/basics/test/cross-platform/client.test.js
@@ -141,17 +141,6 @@ test.describe('Navigation lifecycle functions', () => {
 		expect(await page.innerHTML('pre')).toBe('1 false goto');
 	});
 
-	test('beforeNavigate prevents external navigation triggered by goto', async ({
-		page,
-		app,
-		baseURL
-	}) => {
-		await page.goto('/navigation-lifecycle/before-navigate/prevent-navigation');
-		await app.goto('https://google.de');
-		expect(page.url()).toBe(baseURL + '/navigation-lifecycle/before-navigate/prevent-navigation');
-		expect(await page.innerHTML('pre')).toBe('1 true goto');
-	});
-
 	test('beforeNavigate prevents navigation triggered by back button', async ({
 		page,
 		app,
@@ -215,8 +204,11 @@ test.describe('Navigation lifecycle functions', () => {
 	}) => {
 		await page.goto('/navigation-lifecycle/before-navigate/prevent-navigation');
 		await page.click('h1'); // The browsers block attempts to prevent navigation on a frame that's never had a user gesture.
-
-		await app.goto('https://google.de');
+		page.on('dialog', async (dialog) => {
+			await dialog.dismiss();
+		});
+		await page.close({ runBeforeUnload: true });
+		await page.waitForTimeout(100);
 		await app.goto('/navigation-lifecycle/before-navigate/prevent-navigation?x=1');
 
 		expect(await page.innerHTML('pre')).toBe('2 false goto');