feat(nuxt): `navigateTo` supports external redirects #5022

TheAlexLichter · 2022-05-17T10:10:49Z

🔗 Linked issue

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

❓ Type of change

📖 Documentation (updates to the documentation or readme)
🐞 Bug fix (a non-breaking change that fixes an issue)
👌 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:

It checks if the link passed in is external (if it has a protocol based on ufo)
If the protocol is script:, Nuxt will immediately throw an error on client- and server-side
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
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

I have linked an issue or discussion.
I have updated the documentation accordingly.

netlify · 2022-05-17T10:10:55Z

✅ Deploy Preview for nuxt3-docs canceled.

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

UnderKoen · 2022-05-17T14:30:00Z

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"});

TheAlexLichter · 2022-05-17T15:01:56Z

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.

packages/nuxt/src/app/composables/router.ts

pi0 · 2022-05-20T09:48:48Z

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)

TheAlexLichter · 2022-05-20T10:00:52Z

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?

pi0 · 2022-05-20T11:04:13Z

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?)

TheAlexLichter · 2022-05-20T11:11:38Z

* 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?

pi0 · 2022-05-31T22:32:05Z

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?

cawa-93 · 2022-06-02T09:15:31Z

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

TheAlexLichter · 2022-06-17T08:51:23Z

Updated the PR with the recommended changes 😋

justin-schroeder · 2022-07-12T19:42:42Z

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

Co-authored-by: Koen van Staveren <[email protected]>

Co-authored-by: Daniel Roe <[email protected]>

serhez · 2022-07-21T13:12:04Z

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.

OhB00 · 2022-08-09T14:00:52Z

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.

TheAlexLichter · 2022-08-24T12:56:05Z

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

pi0 · 2022-08-24T15:41:56Z

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 · 2022-08-24T15:44:43Z

packages/nuxt/src/app/composables/router.ts

+    } else {
+      location.href = toPath
+    }
+    return Promise.resolve()


One last thing to ensure: In nuxt 2, we had this problem with external navigations that during navigation, rest of logic goes on. And solution was a custom made error because it was sync. I think we could do something like a never resolving promise that avoids this. (will make a reproduction and implement in later PR btw need more testing) => https://github.com/nuxt/framework/issues/6906

pi0 · 2022-08-24T16:05:32Z

(We also need to document new external key 🙈 ) => nuxt/nuxt#14704

bars83 · 2022-09-13T17:47:20Z

packages/nuxt/src/app/composables/router.ts

-  if (process.client && isProcessingMiddleware()) {
+
+  const toPath = typeof to === 'string' ? to : ((to as RouteLocationPathRaw).path || '/')
+  const isExternal = hasProtocol(toPath, true)


why you check protocol for external? may be I want to redirect within current domain, i.e. respond with headers

HTTP/1.1 302 Found Location: /my-legacy-page

Would you please open an issue with a sandbox to track? 🙏🏼

atinux requested a review from danielroe May 17, 2022 10:18

danielroe reviewed May 17, 2022

View reviewed changes

packages/nuxt/src/app/composables/router.ts Outdated Show resolved Hide resolved

pi0 reviewed May 20, 2022

View reviewed changes

packages/nuxt/src/app/composables/router.ts Outdated Show resolved Hide resolved

packages/nuxt/src/app/composables/router.ts Outdated Show resolved Hide resolved

packages/nuxt/src/app/composables/router.ts Outdated Show resolved Hide resolved

TheAlexLichter force-pushed the feat-redirect-external branch from 601fd83 to 0299ab2 Compare June 17, 2022 08:07

TheAlexLichter marked this pull request as draft June 17, 2022 08:07

TheAlexLichter marked this pull request as ready for review June 17, 2022 08:49

TheAlexLichter requested review from pi0 and danielroe June 17, 2022 08:49

TheAlexLichter and others added 9 commits July 15, 2022 11:22

wip

a2bca32

wip

9bb7c33

feat: improve external url detection

37d150d

Co-authored-by: Koen van Staveren <[email protected]>

fix: handle external link when processing middleware

fb67669

Co-authored-by: Daniel Roe <[email protected]>

fix: return fallback '/' for getPath

c69ddd0

refactor: code style

ca0db61

refactor: use ufo for protocol check

4e3207f

refactor: use allowExternal and check for scripts

59a3549

fix: test

633e6e3

TheAlexLichter force-pushed the feat-redirect-external branch from b0fb76f to 633e6e3 Compare July 15, 2022 09:22

serhez mentioned this pull request Jan 19, 2023

The requested page still loads when making a redirection via route middleware nuxt/nuxt#14468

Closed

pi0 and others added 2 commits August 23, 2022 23:21

Merge branch 'main' into pr/manniL/5022

343c570

Merge branch 'main' into feat-redirect-external

f750239

pi0 added 4 commits August 24, 2022 17:11

fix: always return promise.

dea1f12

refactor: update error message and always throw error

89c6772

refactor: inline toPath computation and avoid extra url parsing

68b7f27

small change

aeeecdc

pi0 approved these changes Aug 24, 2022

View reviewed changes

update fixture

6ddf63b

pi0 reviewed Aug 24, 2022

View reviewed changes

t y p o

d97e68a

pi0 merged commit a4dfe23 into nuxt:main Aug 24, 2022

pi0 mentioned this pull request Jan 19, 2023

External redirection improvements nuxt/nuxt#14704

Closed

3 tasks

TheAlexLichter deleted the feat-redirect-external branch August 25, 2022 09:30

pi0 mentioned this pull request Aug 26, 2022

v3.0.0-rc.9 #6970

Merged

bars83 reviewed Sep 13, 2022

View reviewed changes

danielroe added the 3.x label Jan 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(nuxt): `navigateTo` supports external redirects #5022

feat(nuxt): `navigateTo` supports external redirects #5022

TheAlexLichter commented May 17, 2022 •

edited

Loading

netlify bot commented May 17, 2022 •

edited

Loading

UnderKoen commented May 17, 2022

TheAlexLichter commented May 17, 2022

pi0 commented May 20, 2022

TheAlexLichter commented May 20, 2022

pi0 commented May 20, 2022

TheAlexLichter commented May 20, 2022

pi0 commented May 31, 2022 •

edited

Loading

cawa-93 commented Jun 2, 2022 •

edited

Loading

TheAlexLichter commented Jun 17, 2022

justin-schroeder commented Jul 12, 2022

serhez commented Jul 21, 2022

OhB00 commented Aug 9, 2022 •

edited

Loading

TheAlexLichter commented Aug 24, 2022

pi0 commented Aug 24, 2022

pi0 Aug 24, 2022 •

edited

Loading

pi0 commented Aug 24, 2022 •

edited

Loading

bars83 Sep 13, 2022

pi0 Sep 13, 2022

feat(nuxt): navigateTo supports external redirects #5022

feat(nuxt): navigateTo supports external redirects #5022

Conversation

TheAlexLichter commented May 17, 2022 • edited Loading

🔗 Linked issue

❓ Type of change

📚 Description

📝 Checklist

netlify bot commented May 17, 2022 • edited Loading

✅ Deploy Preview for nuxt3-docs canceled.

UnderKoen commented May 17, 2022

TheAlexLichter commented May 17, 2022

pi0 commented May 20, 2022

TheAlexLichter commented May 20, 2022

pi0 commented May 20, 2022

TheAlexLichter commented May 20, 2022

pi0 commented May 31, 2022 • edited Loading

cawa-93 commented Jun 2, 2022 • edited Loading

TheAlexLichter commented Jun 17, 2022

justin-schroeder commented Jul 12, 2022

serhez commented Jul 21, 2022

OhB00 commented Aug 9, 2022 • edited Loading

TheAlexLichter commented Aug 24, 2022

pi0 commented Aug 24, 2022

pi0 Aug 24, 2022 • edited Loading

Choose a reason for hiding this comment

pi0 commented Aug 24, 2022 • edited Loading

bars83 Sep 13, 2022

Choose a reason for hiding this comment

pi0 Sep 13, 2022

Choose a reason for hiding this comment

feat(nuxt): `navigateTo` supports external redirects #5022

feat(nuxt): `navigateTo` supports external redirects #5022

TheAlexLichter commented May 17, 2022 •

edited

Loading

netlify bot commented May 17, 2022 •

edited

Loading

pi0 commented May 31, 2022 •

edited

Loading

cawa-93 commented Jun 2, 2022 •

edited

Loading

OhB00 commented Aug 9, 2022 •

edited

Loading

pi0 Aug 24, 2022 •

edited

Loading

pi0 commented Aug 24, 2022 •

edited

Loading