From dab98172c57f8302c650b1abd21f639605371a35 Mon Sep 17 00:00:00 2001 From: Simon H <5968653+dummdidumm@users.noreply.github.com> Date: Sat, 9 Dec 2023 02:20:57 +0100 Subject: [PATCH] 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 <322311+benmccann@users.noreply.github.com> * Update packages/kit/src/runtime/client/client.js Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com> * fix test * DRY out --------- Co-authored-by: Rich Harris Co-authored-by: Rich Harris Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com> --- .changeset/silent-games-taste.md | 5 ++++ packages/kit/src/exports/public.d.ts | 2 +- packages/kit/src/runtime/app/navigation.js | 2 +- packages/kit/src/runtime/client/client.js | 26 ++++++++++++------- packages/kit/src/runtime/client/utils.js | 14 +++++----- .../apps/basics/src/routes/goto/+page.svelte | 17 ++++++++++++ .../kit/test/apps/basics/test/client.test.js | 12 +++++++++ .../basics/test/cross-platform/client.test.js | 18 ++++--------- 8 files changed, 66 insertions(+), 30 deletions(-) create mode 100644 .changeset/silent-games-taste.md create mode 100644 packages/kit/test/apps/basics/src/routes/goto/+page.svelte diff --git a/.changeset/silent-games-taste.md b/.changeset/silent-games-taste.md new file mode 100644 index 000000000000..fcd66101a965 --- /dev/null +++ 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 index 4e5e4bce53aa..0b16948b1d73 100644 --- 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 `
` - * - `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 index 527a85fa14d7..d8b942c5e358 100644 --- 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} */ diff --git a/packages/kit/src/runtime/client/client.js b/packages/kit/src/runtime/client/client.js index 08465d6b5342..d5e66430424d 100644 --- 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 index d330ff8c0c64..b9cbc8c62c89 100644 --- 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 new file mode 100644 index 000000000000..a3920c936119 --- /dev/null +++ b/packages/kit/test/apps/basics/src/routes/goto/+page.svelte @@ -0,0 +1,17 @@ + + + + +

{message}

diff --git a/packages/kit/test/apps/basics/test/client.test.js b/packages/kit/test/apps/basics/test/client.test.js index aac4812f51c1..26c7df9d2be3 100644 --- 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 index 598c6e4c356e..7392c2465c41 100644 --- 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');