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
13 changes: 13 additions & 0 deletions packages/kit/src/runtime/client/client.js
Original file line number Diff line number Diff line change
Expand Up @@ -1376,6 +1376,19 @@ export function create_client(app, target) {
},

goto: (href, opts = {}) => {
if (typeof href === 'string') {
href = new URL(href, get_base_uri(document));
Copy link
Member

@benmccann benmccann Dec 8, 2023

Choose a reason for hiding this comment

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

there are three places we do new URL(href, get_base_uri(document)) now. is that worthy of a helper?

Copy link
Member

Choose a reason for hiding this comment

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

I only see two — where's the third?

as an aside the if (typeof href === 'string') check is arguably unnecessary, since if href (which is a badly named argument) is already a URL then it'll just be cloned harmlessly. goto isn't a hot code path so it's probably more valuable to shrink the code than to avoid the extra work

Copy link
Member

Choose a reason for hiding this comment

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

one is new URL(url, get_base_uri(document)), but same thing. It also has that if (typeof href === 'string') check so we could either move it into the helper or remove it. I'm fine with removing it

Copy link
Member

@Rich-Harris Rich-Harris Dec 9, 2023

Choose a reason for hiding this comment

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

ah, right. yep, have consolidated all this logic in a replacement for get_base_uri called resolve_url. much nicer, good catch

}
if (href.origin !== origin) {
return Promise.reject(
new Error(
DEV
? `Cannot use \`goto\` with an external URL. Use \`window.location = "${href}"\` instead`
: 'Invalid URL'
Rich-Harris marked this conversation as resolved.
Show resolved Hide resolved
)
);
}

return goto(href, opts, 0);
},

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'
: '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