WebUI: normalize url in post-login redirect

[Piccirello]

This is an attempt to fix one of the issues described in #10405. Several users indicate that they must manually add a trailing slash after logging in. This PR ensures that the URL the user is redirected to always contains a trailing slash.

[glassez]

This is an attempt to fix one of the issues described in #10405.

Are you able to reproduce the issue?

[Piccirello]

This is an attempt to fix one of the issues described in #10405.

Are you able to reproduce the issue?

No, I'm still not able to reproduce it. However I see little downside to the change (it's actually a bit cleaner than the current approach) and it appears that it would fix the issue reported in #10405 (comment), #10405 (comment), and #10405 (comment)

[glassez]

it's actually a bit cleaner than the current approach

In what way?

BTW, how about the address like http://localhost:8088/index.html?

it appears that it would fix the issue reported in #10405 (comment), #10405 (comment), and #10405 (comment)

Is there any confirmation of this?

[glassez]

As food for thought, I could suggest the following cases:

1. incorrect browser behavior regarding the cache, so it reloads the page from the cache instead of the server.
2. incorrect browser behavior regarding the processing of cookies returned by "auth" API, so that it reloads the page without sending SID cookie.

[Piccirello]

it's actually a bit cleaner than the current approach

In what way?

We currently have two lines that are meant to do the same thing - location.replace and location.reload. Now we just have one.

BTW, how about the address like http://localhost:8088/index.html?

Hmm, this is an edge case. Will add logic for this.

it appears that it would fix the issue reported in #10405 (comment), #10405 (comment), and #10405 (comment)

Is there any confirmation of this?

You have the same information that I have - just read the linked issue.

We try this, see if it helps, if not we can always roll it back. Otherwise we just do nothing.

[Piccirello]

I've simplified the logic so it relies entirely on the URL class to normalize the value. new URL("https://localhost:8080").toString() evaluates to https://localhost:8080/ which achieves the desired intent.

[glassez]

You have the same information that I have - just read the linked issue.

We try this, see if it helps, if not we can always roll it back. Otherwise we just do nothing.

At least I tried to analyze it and make some assumptions. And you? What do you think is the problem?

BTW, if I didn't misunderstand it, in some cases the problem was described as reversed, i.e. removing the trailing slash solved it.

[Piccirello]

You have the same information that I have - just read the linked issue.
We try this, see if it helps, if not we can always roll it back. Otherwise we just do nothing.

At least I tried to analyze it and make some assumptions. And you? What do you think is the problem?

I answered exactly this in this PR's description. Hence this PR. We can sit around for 6 years guessing what the issue is, or we can put up a potential fix and let users try it. And either way, I actually think the logic in this PR is cleaner than the existing logic, regardless of whether it ends up solving the issue.

1. incorrect browser behavior regarding the cache, so it reloads the page from the cache instead of the server.

qBittorrent already sends no-store for the login page, which tells the browser not to cache it.

2. incorrect browser behavior regarding the processing of cookies returned by "auth" API, so that it reloads the page without sending SID cookie.

This seems quite unlikely, otherwise this would be impacting far more sites than qBittorrent.

BTW, if I didn't misunderstand it, in some cases the problem was described as reversed, i.e. removing the trailing slash solved it.

The linked issue literally includes "Removing the trailing slash then loads the full UI" in the title, which is why that's the issue I'm attempting to solve. It's possible there's another issue, but I haven't looked into that one.

[glassez]

BTW, if I didn't misunderstand it, in some cases the problem was described as reversed, i.e. removing the trailing slash solved it.

The linked issue literally includes "Removing the trailing slash then loads the full UI"

To me, this sounds like removing the trailing slash allows you to load the UI (i.e. solves the problem).

In fact, it seems to me that adding a slash, as well as removing it (if it was originally present when the user opened the login page), only causes the browser to correctly request the page from the server, sending previously received cookies, etc.

3. incorrect browser behavior regarding the processing of cookies returned by "auth" API, so that it reloads the page without sending SID cookie.

This seems quite unlikely, otherwise this would be impacting far more sites than qBittorrent.

Doesn't #10405 (comment) indicate such an issue?

[glassez]

And either way, I actually think the logic in this PR is cleaner than the existing logic, regardless of whether it ends up solving the issue.

Maybe... but they both look like dirty workarounds for me.

We can sit around for 6 years guessing what the issue is, or we can put up a potential fix and let users try it.

OK, I won't bother you.

[Chocobo1]

No, I'm still not able to reproduce it.

This piece of code is revised almost every major release and I'm not going to approve it just because you think it is better. I'll need root cause analysis and reproduction.

Hence this PR. We can sit around for 6 years guessing what the issue is, or we can put up a potential fix and let users try it.

No. And you completely disregarded all previous rationales that leads to today weird code. Search for previous PR and issues related to this login code. And make sure regressions do not creep back.

---

From #10405 (comment) and #10405 (comment), I reckon this is due to two instances of webui that shared the same cookie. One webui logs in and affected the other webui. This is largely due to user misunderstanding of how cookies work: https://stackoverflow.com/questions/1612177/are-http-cookies-port-specific
I suppose #23228 could remedy this issue and it should let those affected user try it before attempting another workaround.

Or maybe the browser/reverse proxy messed up and cached the cookie instead of refreshing it. Anyway, the root cause won't be clear unless we have reproduction at hand.

[Piccirello]

No. And you completely disregarded all previous rationales that leads to today weird code. Search for previous PR and issues related to this login code. And make sure regressions do not creep back.

That's a lot of assumptions. I read through #21832 and #20442. It seems to me that directly setting the page's href is a more direct approach than trying to tamper with browser history (location.replace) or trying to reload (location.reload). It also doesn't seem that this approach would cause a regression with any of the previous issues that those PRs solved.

I'll need root cause analysis and reproduction.

No one has been able to reliably reproduce the issue after 6 years. Did you read the thread? You seem to be in the camp of "do nothing and hope it helps". I'm in the camp of wanting to try something and see if it fixes it. if it doesn't fix it then there's no downside and the code actually got cleaner.

[Chocobo1]

I'm in the camp of wanting to try something and see if it fixes it.

No, you are shooting in the dark and hope it didn't break anything. You have no understanding of why it didn't work in the first place and no testament that your code will resolve #10405 also no guarantee that the regressions won't come back. This is not the way of engineering.

if it doesn't fix it then there's no downside and the code actually got cleaner.

Until another user reports that their weird browser/client don't work anymore.

[Piccirello]

I maintain my code, so if something else breaks, I'll fix it. Can you identify any issues with my code? The current code is a mess - it has two calls to accomplish the same thing, and window.location.reload() doesn't even take a boolean arg. The new code is more precise, even if it doesn't solve #10405.

[Piccirello]

@qbittorrent/bug-handlers it would be great to move this PR forward. This replaces the legacy redirection logic, which requires two commands that do the same thing (location.replace and location.reload), with one command that properly normalizes the URL. This should not be a controversial change.