Respect redirects to external url destinations #4579
Conversation
redirect('https://www.google.com/') should go to http://www.google.com/
We should not prepend the request origin to the destination URL if they
are external.
|
|
Hi @cephalization, Welcome, and thank you for contributing to Remix! Before we consider your pull request, we ask that you sign our Contributor License Agreement (CLA). We require this only once. You may review the CLA and sign it by adding your name to contributors.yml. Once the CLA is signed, the If you have already signed the CLA and received this response in error, or if you have any questions, please contact us at hello@remix.run. Thanks! - The Remix team |
|
This is a very naïve PR but it reflects my desired behavior of remix redirect. I am sure there are changes to be made to make this scalable, particularly around the Just wanted to get this draft up to show that I am working on this. |
|
Thank you for signing the Contributor License Agreement. Let's get this merged! 🥳 |
|
Thanks @cephalization! For ease of ongoing dev during this transition of Remix onto RR 6.4, we are temporarily keeping a duplicate copy of the |
| @@ -817,7 +817,8 @@ export function resolvePath(to: To, fromPathname = "/"): Path { | |||
| } = typeof to === "string" ? parsePath(to) : to; | |||
|
|
|||
| let pathname = toPathname | |||
| ? toPathname.startsWith("/") | |||
| ? // we don't want to prepend the fromPathname on root or external toPathnames' | |||
| toPathname.startsWith("/") || toPathname.startsWith("http") | |||
There was a problem hiding this comment.
@brophdawg11 do we want to accept other protocols? (app:// itunes:// ..)
There was a problem hiding this comment.
Yeah definitely. I did this in RR the way it's checked in Remix today which should account for other protocols: https://github.com/remix-run/react-router/pull/9590/files#diff-c6f085a772081501e6db2af3eee90b15fe7cd7c965d2f220c2a3c5c7772bc0c4R2568
Awesome, thanks! Just ping me when/where I can help. |
|
Looks like this got automatically closed when we merged/deleted the other branch. I cherry-picked these commits into #4627 so @cephalization keeps the authorship of them! |
redirect('https://www.google.com/')should go tohttp://www.google.com/We should not prepend the request origin to the destination URL if they are external.
Closes: #4570
Testing Strategy:
New and existing integration tests pass. Still need to copy build into my remix app to test there.