Restrict external navigation with goto by default

[efstajas]

Describe the problem

Currently, you can pass an absolute external path (e.g. "https://wikipedia.com") to the goto function, and it will happily navigate away to outside the SvelteKit app.

Generally, this makes sense of course, but it caught me off-guard. Having used Nuxt before, where the router.navigateTo function forbids external navigation unless you explicitly set external to true in its options, I implemented a mechanism that allows our sign-in page to push the user back to a route-guarded page using a backTo URL parameter that would get appended to the sign-in page URL. Once the user signed in, the page would check whether backTo is set to a path, and if yes pass its value to goto.

Recently, it was found that it was in fact possible to set this parameter to an absolute external URL, and it would happily push the user to that external page. Consequently, even javascript: URLs were possible, resulting in an XSS vulnerability.

I understand that passing any user-editable string to goto was probably foolish on my part, but I didn't even consider the fact that external navigation would be possible at the time. Luckily, we caught this before going live on production, but I feel like this could be a common pitfall (especially given at least one other popular framework protects against it), with potentially severe consequences.

Describe the proposed solution

Add a new paremeter external to goto's options object, which is set to false by default. If false, the function should ensure that the navigation would not push the user outside the current application, and throw an error if it would. If true, the current behavior should apply.

See Nuxt.js' navigateTo function. https://nuxt.com/docs/api/utils/navigate-to

Alternatives considered

Add the external parameter, but set it to true by default, so developers can opt-in to blocking external URLs when passing user-editable strings to goto while avoiding a breaking change.

Importance

would make my life easier

Additional Information

No response

[dummdidumm]

Would be interesting to know how other frameworks (Nextjs, Remix, SolidStart) handle this

[Rich-Harris]

We could forbid javascript: URLs and it would fix the vulnerability without (really) being a breaking change

[efstajas]

Would be interesting to know how other frameworks (Nextjs, Remix, SolidStart) handle this

Unless someone jumps in with deeper knowledge on this before, I'm happy to do some research on this early this week.

We could forbid javascript: URLs and it would fix the vulnerability without (really) being a breaking change

👍 That would definitely be a good step to take, I guess there's zero good reasons why you would ever want to goto a javascript: URL.

However with this specific vulnerability we accidentally created, even just the ability to create a link that forwards the user to any external site after logging in is also quite critical in itself, of course. The user would first click on a trustworthy link on the correct domain, and then potentially get forwarded to a phishing scam without realizing the domain had changed.

Maybe blocking external links could be opt-in per call to goto alternatively, so that there's at least an easy way to secure navigations when passing user-editable strings as the path. It turns out that securing that isn't super trivial - you need to guard against strings including protocol:, but also double-slashes at the beginning (//wikipedia.com), plus possibly more patterns I'm not aware of. But of course making it opt-in wouldn't be "secure by default"...

[h0wl]

Regarding Nextjs from what I saw in redirects (https://nextjs.org/docs/api-reference/next.config.js/redirects) logic, when external redirects are handled they check if the URLs are matching http or https, everything else is dropped - https://github.com/vercel/next.js/blob/13b32ba98179aa94ac2e402f272e5c6a3356d310/packages/next/src/lib/load-custom-routes.ts#L559 handleRoutes: https://github.com/vercel/next.js/blob/13b32ba98179aa94ac2e402f272e5c6a3356d310/packages/next/src/lib/load-custom-routes.ts#L177-L178 and processRoutes https://github.com/vercel/next.js/blob/13b32ba98179aa94ac2e402f272e5c6a3356d310/packages/next/src/lib/load-custom-routes.ts#L477.

However they also have a helper function called res.redirect - https://nextjs.org/docs/api-routes/response-helpers. This one appears to have no checks at all - https://github.com/vercel/next.js/blob/13b32ba98179aa94ac2e402f272e5c6a3356d310/packages/next/src/server/api-utils/index.ts#L54

[Rich-Harris]

It turns out that securing that isn't super trivial

You don't need to do anything fancy to tell if a URL is external or not, this is sufficient:

function is_external(url) {
  return new URL(url, location).origin !== location.origin;
}

You can certainly make the case that goto should only accept internal URLs — the API is, in retrospect, a bit weird insofar as none of the options (keepFocus, state etc) apply, and it returns a promise that never resolves. I'd probably be in favour of changing it. The question is whether we consider changing the behaviour now to be a bugfix (in which case we can include it in a patch release) or a breaking change that needs to wait for a major.

@h0wl the Next.js API to compare against is router.push, not redirects. The docs don't explicitly say that external paths don't work (I haven't tried it to check), but they do include this note:

You don't need to use router.push for external URLs. window.location is better suited for those cases.

[efstajas]

You don't need to do anything fancy to tell if a URL is external or not, this is sufficient:

duh! that's so much better – changing our check to this for now :)

the API is, in retrospect, a bit weird insofar as none of the options (keepFocus, state etc) apply

That's another great point, yeah. I suppose then instead of adding that external param, just not supporting external navigations at all with goto and referring people to window.location for such cases in the docs might be an even better & potentially less confusing option, given all those arguments that are only relevant for internal navigation.

[dummdidumm]

We decided we just straight up disallow external navigation from goto - just use window.location then. Also, it would be nice to adjust the types to reflect that (only strings starting with a . or / are allowed)

[benmccann]

Also, it would be nice to adjust the types to reflect that (only strings starting with a . or / are allowed)

URLs starting with . are uncommon. It'd seem more common to do goto('about'). Could we do something like Exclude<string, 'http:// | https://'>?

[Rich-Harris]

Good point. AFAICT there's no way to express that in TypeScript though

[benmccann]

I'd probably just accept any string in that case

[elliott-with-the-longest-name-on-github]

@Rich-Harris It's actually quite possible in TypeScript, but IMO this will get too complicated for the use case and start to become burdensome. This kind of thing is better as a runtime check, as we'd probably also want to verify that URL instances also aren't external.

[Rich-Harris]

Merged #11207, so I'll close this