breaking: disallow external navigation using goto
#11207
Conversation
🦋 Changeset detectedLatest commit: 87dc49f The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Should we add a test to check that |
|
I wonder if we should also add an |
|
One other question about this - should it be called |
|
We concluded that there's no value in having an |
Rich-Harris
changed the title
goto by defaultgoto
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
| @@ -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)); | |||
There was a problem hiding this comment.
there are three places we do new URL(href, get_base_uri(document)) now. is that worthy of a helper?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
ah, right. yep, have consolidated all this logic in a replacement for get_base_uri called resolve_url. much nicer, good catch
Co-authored-by: Ben McCann <322311+benmccann@users.noreply.github.com>
|
I don't have a strong opinion on whether this or the old API is better, but I'm fine with this change. I did notice though that the issue was created due to some worry about Anyway, that's my last comment, so feel free to merge whenever you're happy with it |
SvelteKit's goto(url) can't navigate to third party pages anymore (makes sense) sveltejs/kit#11207
SvelteKit's goto(url) can't navigate to third party pages anymore (makes sense) sveltejs/kit#11207
closes #8775
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm testand lint the project withpnpm lintandpnpm checkChangesets
pnpm changesetand following the prompts. Changesets that add features should beminorand those that fix bugs should bepatch. Please prefix changeset messages withfeat:,fix:, orchore:.