framework icon indicating copy to clipboard operation
framework copied to clipboard

feat(nuxt): `navigateTo` supports external redirects

Open TheAlexLichter opened this issue 3 years ago β€’ 13 comments

πŸ”— Linked issue

https://github.com/nuxt/framework/discussions/4997, resolves #5247

❓ Type of change

  • [ ] πŸ“– Documentation (updates to the documentation or readme)
  • [ ] 🐞 Bug fix (a non-breaking change that fixes an issue)
  • [x] πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • [ ] ✨ New feature (a non-breaking change that adds functionality)
  • [ ] ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

Right now, navigateTo supports only internal redirects (either via vue-router or by using the baseURL). This PR adds rudimentary support for external redirects and protocol checking:

  1. It checks if the link passed in is external (if it has a protocol based on ufo)
  2. If the protocol is script:, Nuxt will immediately throw an error on client- and server-side
  3. If allowExternal is not set to true, then a confirmation dialog will pop up on client-side and an error will be thrown on server-side
  4. If it is set to true it'll use h3 on the server-side or location.replace/location.href = X on the client-side

Also feel free to tell me if that's the right approach or if we want another function for this behavior πŸ€” Doc update will follow when the PR is more mature.

πŸ“ Checklist

  • [x] I have linked an issue or discussion.
  • [ ] I have updated the documentation accordingly.

TheAlexLichter avatar May 17 '22 10:05 TheAlexLichter

Deploy Preview for nuxt3-docs canceled.

Name Link
Latest commit d97e68a1df4f23affbdf1e1d9c71766919d5776e
Latest deploy log https://app.netlify.com/sites/nuxt3-docs/deploys/630647e659153b0007be89a0

netlify[bot] avatar May 17 '22 10:05 netlify[bot]

Should it only allow http/https based urls? Or would mailto and tel also need to work?

A check that does include mailto and tel.

try {
	url = new URL(string);
	//url route
} catch (_) {
	//normal route
}

Maybe it would be even better to have no check and have a additional entry in the to object so that the following would work:

navigateTo({url: "https://github.com"});

UnderKoen avatar May 17 '22 14:05 UnderKoen

Should it only allow http/https based urls? Or would mailto and tel also need to work?

A check that does include mailto and tel.

try {
	url = new URL(string);
	//url route
} catch (_) {
	//normal route
}

Great idea! Will incorporate that in the code ☺️

Maybe it would be even better to have no check and have a additional entry in the to object so that the following would work:

navigateTo({url: "https://github.com"});

That would be one option but would leave the "check" up to the user. IMO the DX is better when Nuxt is doing the check for the user instead of leaving it up to them.

TheAlexLichter avatar May 17 '22 15:05 TheAlexLichter

Thanks for this PR @manniL πŸ’š In general this is a really nice improvement that we support redirection to external URLs but I'm also little bit concerned about potential security risks implied by combination of this new feature and developer's mistake with a programmatic call. I suggest some options:

  • Explicit protocol validation to avoid things like script:
  • A new option externalURL: '' / external: true and abort when url is external with an error to use explicit option for external URL. (We might also use a simple alert/prompt fallback to concenst users about redireting when url is implicitly external)

pi0 avatar May 20 '22 09:05 pi0

Thanks for this PR @manniL πŸ’š In general this is a really nice improvement that we support redirection to external URLs but I'm also little bit concerned about potential security risks implied by combination of this new feature and developer's mistake with a programmatic call. I suggest some options:

* Explicit protocol validation to avoid things like `script:`

* A new option `externalURL: ''` / `external: true` and abort when `url` is external with an error to use explicit option for external URL. (We might also use a simple alert/prompt fallback to concenst users about redireting when `url` is implicitly external)

Thanks for the feedback! πŸ™ I agree regarding some security implications here. DX-wise, the protocol validation part would be more appealing IMO but from a security POV, a new option would make more sense.

What would be your favorite solution here?

TheAlexLichter avatar May 20 '22 10:05 TheAlexLichter

I would probably do both:

  • Protocol restriction in all cases
  • Abort SSR / Prompt Client side with url: 'https://...'
  • Introduce external: true or externalURL: string (Which one you think is better?)

pi0 avatar May 20 '22 11:05 pi0

* Protocol restriction in all cases

That means not executing any links that start with script:. Any other protocols we should consider?

* Abort SSR / Prompt Client side with `url: 'https://...'`

By throwing an error with the approriate message, right?

* Introduce `external: true` or `externalURL: string` (Which one you think is better?)

I'd probably go with external: true. This param won't have any influence when the URL is local and will only matter in case the path is actually an external one, right?

TheAlexLichter avatar May 20 '22 11:05 TheAlexLichter

That means not executing any links that start with script:. Any other protocols we should consider?

Considering the possibility of native app linking with custom protocols, yes the best we can exclude unsafe script: protocol.

By throwing an error with the appropriate message, right?

Would be an overkill to use good-old window.confirm as the default fallback? For server yes error is the best!

I'd probably go with external: true. This param won't have any influence when the URL is local and will only matter in case the path is actually an external one, right?

external: true seems good to me too! We can document to use external only if you are sure external URLs are safe. Or what about allowExternal?

pi0 avatar May 31 '22 22:05 pi0

I will add a little feedback as developer who faced with this problem. In general, reading the documentation, one gets the impression that the <nuxt-link>and navigateTo work identically. We can easily pass the external url in to without any additional: <nuxt-link to="https://github.com">. And I would expect the navigateTo to take exactly the same parameters and process them identically: navigateTo('https://github.com')

cawa-93 avatar Jun 02 '22 09:06 cawa-93

Updated the PR with the recommended changes πŸ˜‹

TheAlexLichter avatar Jun 17 '22 08:06 TheAlexLichter

Until this is merged β€”Β I've worked up a small composable function to allow explicit external redirects from route middleware: https://gist.github.com/justin-schroeder/f583797c702155f32c9078d1add2a594

justin-schroeder avatar Jul 12 '22 19:07 justin-schroeder

Hello! Are there any updates on this? It is quite critical for important workflows like OAuth 2.0 (being able to follow a server redirection to the login form, etc.), unless there is some other way to do it I missed.

serhez avatar Jul 21 '22 13:07 serhez

Found an interesting workaround for this one, prepending your URL with // enables you to perform an external redirect. For example, to redirect to http://google.com use navigateTo("//google.com"). This works provided your baseUrl is / as ufo will strip a slash from the start of your path.

Works because of parser differentials between the browser and vue-router/ufo.

Couldn't find a way to do this for other protocols.

OhB00 avatar Aug 09 '22 14:08 OhB00

@pi0 Please let me know what is left to do here besides the docs PR ☺️

TheAlexLichter avatar Aug 24 '22 12:08 TheAlexLichter

Thanks again for PR @manniL and sorry it took long. Finally, I've made few refactors to inline and simplify logic, avoid extra parse calls, improve messages and make the option simpler { external: true }. I hope this changes are fine to you.

pi0 avatar Aug 24 '22 15:08 pi0

(We also need to document new external key πŸ™ˆ ) => https://github.com/nuxt/nuxt.js/issues/14704

pi0 avatar Aug 24 '22 16:08 pi0