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.

Sign up for GitHub

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
Merged

breaking: disallow external navigation using goto #11207

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
6b7d066
breaking: disallow external navigation using `goto` by default
dummdidumm Dec 6, 2023
d3b6201
do it here instead
dummdidumm Dec 6, 2023
23ef6af
adjust test
dummdidumm Dec 6, 2023
cd73187
remove option, TODO tests
dummdidumm Dec 8, 2023
68e3799
tweak message
Rich-Harris Dec 8, 2023
11a498a
Update .changeset/silent-games-taste.md
Rich-Harris Dec 8, 2023
6f83f04
add a test, remove an obsolete test
Rich-Harris Dec 8, 2023
bf5a196
Merge branch 'goto-external' of github.com:sveltejs/kit into goto-ext…
Rich-Harris Dec 8, 2023
9d3a5c5
fix test
Rich-Harris Dec 8, 2023
83b6950
prettier
Rich-Harris Dec 8, 2023
581c8a7
fix
dummdidumm Dec 8, 2023
4ddf16d
Update packages/kit/src/exports/public.d.ts
Rich-Harris Dec 8, 2023
b8fe747
Update packages/kit/src/runtime/client/client.js
Rich-Harris Dec 8, 2023
830f0e2
fix test
Rich-Harris Dec 9, 2023
87dc49f
DRY out
Rich-Harris Dec 9, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/silent-games-taste.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@sveltejs/kit': major
---

breaking: disallow external navigation using `goto` by default
Rich-Harris marked this conversation as resolved.
Show resolved Hide resolved
2 changes: 1 addition & 1 deletion packages/kit/src/runtime/app/navigation.js
View file
Open in desktop
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
View file
Open in desktop
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 •
edited
Loading

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 •
edited
Loading

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) {
if (DEV) {
return Promise.reject(
`Cannot use \`goto\` with an external URL. Use \`window.location = "${href}"\` instead`
);
} else {
return Promise.reject();
}
}

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

Expand Down
7 changes: 4 additions & 3 deletions packages/kit/test/apps/basics/test/cross-platform/client.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,11 +143,11 @@ test.describe('Navigation lifecycle functions', () => {

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');
page.goto('https://google.de');
await page.waitForTimeout(500);
expect(page.url()).toBe(baseURL + '/navigation-lifecycle/before-navigate/prevent-navigation');
expect(await page.innerHTML('pre')).toBe('1 true goto');
});
Expand Down Expand Up @@ -216,7 +216,8 @@ 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.goto('https://google.de');
await page.waitForTimeout(500);
await app.goto('/navigation-lifecycle/before-navigate/prevent-navigation?x=1');

expect(await page.innerHTML('pre')).toBe('2 false goto');
Expand Down
Loading