Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

With routerBase being "/a/", landing on "/app" is incorrectly redirected to "/a/pp" #3555

Closed
eyedean opened this issue May 20, 2021 · 8 comments · Fixed by #3556
Closed

With routerBase being "/a/", landing on "/app" is incorrectly redirected to "/a/pp" #3555

eyedean opened this issue May 20, 2021 · 8 comments · Fixed by #3556

Comments

@eyedean
Copy link
Contributor

eyedean commented May 20, 2021

Version

3.5.1

Reproduction link

https://jsfiddle.net/eyedean/4qcd9kgy/21/

Steps to reproduce

It's hard to reproduce in the jsfiddle and such. That's what I mentioned in the TODO section of the link above.

Actual reproduction

  • Let's say I have a Vue App with HTML5 History Router and RouterBase being /a/.
  • If I land on www.mydomain.com/bpp/, VueRouter properly shows the App's 404 (catch on /*/ in routes) page.
  • However if I land on www.mydomain.com/app, then Vue Router redirects it to www.mydomain.com/a/pp which is not expected. (Assume /a/pp is an existing page, and not the intended target of /app)

What is expected?

show 404 on /app, just like /bpp.

What is actually happening?

redirects it to /a/pp, which can be a different existing page.


I tried to debug this on my own. Here is what I found.

When I set VueRouterBase to /a/ (with trailing slash, as advised in the documentation), router.options.base is properly /a/, but the history.base is going through normalizeBase and becomes /a. That's why /app gets sliced (via

if (base && path.toLowerCase().indexOf(base.toLowerCase()) === 0) {
path = path.slice(base.length)
}
) and becomes path: "/pp".

I also tried getting around this by setting routerbase to /a// (double trailing slash). It stopped the /app => /a/pp redirect, successfully, but then all my links were ugly! (e.g. <router-link to="/login"> would produce www.mydomain.com/a//login with double slash in between.)

@posva
Copy link
Member

posva commented May 20, 2021

The thing is if your base is /a/, it shouldn't be possible to access your application at /app/, only at /a/app.
Maybe a reasonable fix could improve this but, by design, this scenario is not meant to happen

@eyedean
Copy link
Contributor Author

eyedean commented May 20, 2021

@posva Thanks for your quick answer. I found a similar answer from you here as well.

Ideally, a higher-level router/reverse-proxy (e.g. NginX or CloudFront or Load Balancer) should catch this. But the inconsistent behavior on /bpp vs /app should be considered a bug to me.

eyedean added a commit to eyedean/vue-router that referenced this issue May 20, 2021
eyedean added a commit to eyedean/vue-router that referenced this issue May 20, 2021
eyedean added a commit to eyedean/vue-router that referenced this issue May 20, 2021
eyedean added a commit to eyedean/vue-router that referenced this issue May 20, 2021
@eyedean
Copy link
Contributor Author

eyedean commented May 20, 2021

@posva I created a PR for it at #3556. It's my first PR in vue-router and I hope I didn't miss anything. :)

PS. PR 3556 for Issue 3555! Nice 1:1 ratio of PRs to issues, I would say! :)

@posva
Copy link
Member

posva commented May 27, 2021

Thank you but I don't think that's the right fix because if your app can be served at /, then it shouldn't have a base. In other words, your app should only be reachable with the correct URL: /a/..., not with /app. If it is there is no point in using a base.

@eyedean
Copy link
Contributor Author

eyedean commented May 27, 2021

Thanks for your response @posva. I hear what you are saying, and as I mentioned before I totally agree that the actual fix for the situation should be in the LoadBalancer/Proxy who routes /app to this SPA app. My case here is against the "unexpected behavior" of vue-router.

Disambiguating 2 Problems

Let's imagine the case that the developer has set to forward /a* to the app (instead of /a/*) in the Nginx configs. (Let's call that "Problem A.")

Now, having the router of the app redirect /app to /a/pp (which actually might be an existing page of pp of the SPA) is what I am talking about here. My PR is to prohibit the router from doing /app => /a/pp redirect, which doesn't make any sense. (Let's call this "Problem B.")

Different base values inside vue-router

Note that the .base of Router object and .base of the history object are different in vue-router:
image
And this is the source of the issue that the comparison in the getLocation function of the history is made against /a.

How to fix B?

We know that Problem B wouldn't exist without Problem A -- That's probably why this issue hasn't been filed in years. So, do you think there can be a better fix for Problem B?

Thanks again!

@posva
Copy link
Member

posva commented May 28, 2021

And this is the source of the issue that the comparison in the getLocation function of the history is made against /a.

Then a proper fix would probably to use router.history.base which is the correct value while router.options is just the initial options object

eyedean added a commit to eyedean/vue-router that referenced this issue May 28, 2021
@eyedean
Copy link
Contributor Author

eyedean commented May 28, 2021

Wait, are we talking about the same PR? Who uses router.options.base? Did you get a chance to take a look at my PR?

When @yyx990803 first wrote this logic of getLocation() on July 23, 2016, he probably didn't fully consider the fact that base in history doesn't have the initial ending / (it would, if it was using router.option.base, but it doesn't) and so the logic of path.indexOf(base) === 0 would have a false positive for path=/app, base=/a.

I just improved his logic to be more accurate. I am pretty confident that if he, in 2016, could think of the case of path=/app and base=/a, he would consider covering that in his code.

Here is a screenshot of my PR. I am pretty sure it is not common in the open-source community and GitHub, to post a screenshot of the single line of code you are changing in PR, but I rather keep the conversation relevant. It's becoming personally offensive to me that I spend time providing details, and then I receive a random irrelevant response, which is not even related to my PR or helping with the problem we are talking about.

image

Here are questions for you:

  1. Do you think the if in Line90 of html5.js (left half of the above screenshot) is working as expected when its base is /a and getLocation('/app') is called? (Hint: The answer is obviously "NO", but I need to make sure it's obvious.)
  2. Do you have a better fix in mind for the problem in question 1?

PS. I am not blocked by this PR. I just wanted to give back to the Vue and VueRouter communities. And I personally prefer to contribute more to the projects that its moderators care about volunteer contributors like me, at least half as much I care about their project. So, if you think this ticket/PR should be closed, I will keep that in mind and walk away.

@posva
Copy link
Member

posva commented May 29, 2021

Wait, are we talking about the same PR? Who uses router.options.base? Did you get a chance to take a look at my PR?

I switched both values in my brain. Now it makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants