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

breaking: disallow external navigation using goto #11207

Merged
merged 15 commits into from
Dec 9, 2023
5 changes: 5 additions & 0 deletions .changeset/silent-games-taste.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@sveltejs/kit': major
---

breaking: disallow external navigation with `goto`
2 changes: 1 addition & 1 deletion packages/kit/src/exports/public.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion packages/kit/src/runtime/app/navigation.js
Original file line number Diff line number Diff line change
Expand Up @@ -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>}
*/
Expand Down
26 changes: 17 additions & 9 deletions packages/kit/src/runtime/client/client.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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) => {
Expand All @@ -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) {
Expand Down
14 changes: 8 additions & 6 deletions packages/kit/src/runtime/client/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -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() {
Expand Down
17 changes: 17 additions & 0 deletions packages/kit/test/apps/basics/src/routes/goto/+page.svelte
Original file line number Diff line number Diff line change
@@ -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>
12 changes: 12 additions & 0 deletions packages/kit/test/apps/basics/test/client.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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);
});
});
18 changes: 5 additions & 13 deletions packages/kit/test/apps/basics/test/cross-platform/client.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -141,17 +141,6 @@
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,
Expand Down Expand Up @@ -215,8 +204,11 @@
}) => {
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');
Expand Down Expand Up @@ -678,7 +670,7 @@
const { port } = await start_server((_req, res) => {
res.end(html_ok);
});

Check warning on line 673 in packages/kit/test/apps/basics/test/cross-platform/client.test.js

View workflow job for this annotation

GitHub Actions / Cross-browser-test (18, ubuntu-latest, firefox, dev)

flaky test: responds to <form method="GET"> submission without reload

retries: 2
await page.goto(`/routing/slashes?port=${port}`);
await page.locator(`a[href="http://localhost:${port}/with-slash/"]`).click();
expect(await page.content()).toBe(html_ok);
Expand Down
Loading