Append trailing slash to URL if missing #14822

kharenis · 2021-04-19T14:48:06Z

Closes #5693
Index pages now automatically redirect to a path with a trailing"/". If the site was accessed through a proxy with a modified path, a missing trailing "/" would break the relative paths.

Closes qbittorrent#5693 Index pages now automatically redirect to a path with a trailing "/". If the site was accessed through a proxy with a modified path, a missing trailing "/" would break the relative paths.

glassez · 2021-04-22T10:33:35Z

Does it supersede #14787?

kharenis · 2021-04-22T10:37:38Z

Does it supersede #14787?

I believe so, I can't think of why a base path would be needed if this fix was to be added.

glassez · 2021-04-22T10:45:24Z

Could you explain in detail using some example URL, what exactly happens without and with this patch?

Chocobo1 · 2021-04-23T07:06:45Z

src/webui/www/private/index.html

@@ -2,6 +2,14 @@
 <html lang="${LANG}">

 <head>
+    <script>
+        let path = window.location.pathname;


you can use const:
const path = ...

Chocobo1 · 2021-04-23T07:06:57Z

src/webui/www/private/index.html

+    <script>
+        let path = window.location.pathname;
+        if (path) {
+            let lastChar = path.substr(path.length - 1);


substr is not recommended: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/String/substr#browser_compatibility
I would suggest: const lastChar = path.slice(-1);

Chocobo1 · 2021-04-23T07:11:41Z

src/webui/www/private/index.html

+        let path = window.location.pathname;
+        if (path) {
+            let lastChar = path.substr(path.length - 1);
+            if (lastChar != '/')


if (lastChar !== '/')

Chocobo1 · 2021-04-23T07:16:12Z

src/webui/www/private/index.html

+        if (path) {
+            let lastChar = path.substr(path.length - 1);
+            if (lastChar != '/')
+                window.location.replace(window.location.href + '/');


Would this redirection break other things? such as the magnet handler?

Chocobo1 · 2021-04-23T07:18:15Z

src/webui/www/public/index.html

@@ -2,6 +2,14 @@
 <html lang="${LANG}">

 <head>
+    <script>


Please move it to the next line of </noscript>.
The same for private/index.html.

FranciscoPombal · 2021-04-26T14:35:13Z

Don't have time to review this right now, but I believe we should give @Piccirello a chance to look at this.

Piccirello · 2021-04-26T15:11:06Z

Will review this tonight.

Piccirello

I think I'm lacking info right now as to how this solves the linked issue. Could you share some more info @kharenis and maybe an example or two?

My current inclination is that we should probably handle this redirect server-side, rather than client-side.

kharenis · 2021-04-27T14:55:34Z

@glassez @Chocobo1 @Piccirello
Hi guys, apologies for going AWOL, I've been having internet problems at home.

Given the following scenario:

qbt is being hosted on a server at http://10.10.10.10:8080
qbt is accessed through a reverse proxy that's been configured to listen on http://mywebsite.com/qbt
The reverse proxy replaces "/qbt" (or "/qbt/") in the path with "/" and forwards the requests to http://10.10.10.10:8080

Without the patch:
If a user attempts to reach qbt at http://mywebsite.com/qbt, the index page will load, however all of the resources with relative paths fail to load, as they are attempting to reach
[http://mywebsite.com/scripts/login.js, http://mywebsite.com/icons/qbittorrent-tray.svg] etc.

If a user attempts to reach qbt at http://mywebsite.com/qbt/, the relative paths are correctly resolved and everything loads as expected. [http://mywebsite.com/qbt/scripts/login.js, http://mywebsite.com/qbit/icons/qbittorrent-tray.svg] etc.

With the path:
If the user attempts to access http://mywebsite.com/qbt, they will be redirected to http://mywebsite.com/qbt/, and the index page & resources with relative paths all load as expected.

I'm not sure if it can be implemented server-side as qbt receives the path from the reverse proxy's perspective. - In the above scenario, the "/qbt" path from the user's perspective is seen as "/" to qbt.

glassez · 2021-04-27T15:36:56Z

I don't see any problems at server side from the example above.

glassez · 2021-04-27T15:59:25Z

The bottom line is how relative paths in HTML links are resolved. IIRC, they are resolved against the directory of file containing the link. Seems in case of http://mywebsite.com/qbt/ qbt/ is considered as that directory. But in case of http://mywebsite.com/qbt qbt is considered as filename in root directory so paths are resolved against root directory.
I wonder if it is possible to configure the proxy to redirect qbt to qbt/ itself? Or maybe configure it to replace /qbt/ only so http://mywebsite.com/qbt will not redirected?

Piccirello · 2021-04-27T17:15:41Z

Thanks for that explanation. We can definitely nix my earlier server-side suggestion.

It seems that without this fix, users can already employ a nasty regex in their proxy config, which will correctly handle the trailing slash. That's definitely not ideal. At the same time, this fix would now require (some) users to update their proxy config to properly handle double slashes. Is my understanding correct?

It might be helpful if you shared an example proxy config for nginx (or whatever reverse proxy you use) so that we can see what would be needed.

glassez · 2021-04-27T17:21:55Z

@Piccirello
Is it correct in HTTP world that both http://mywebsite.com/qbt and http://mywebsite.com/qbt/ points to the same page (if exclude reverse proxy from this scenario)? And how would the relative links on this page behave in this case?

FranciscoPombal · 2021-04-27T18:24:45Z

@kharenis @Piccirello

I just refreshed the wiki pages documenting reverse proxy setups with NGINX. I have never had this / problem. I use these setups on my machines pretty much as-is, and it "just works". Will they be impacted by this change?

Links to the relevant wiki pages:

https://github.com/qbittorrent/qBittorrent/wiki/NGINX-Reverse-Proxy-for-Web-UI
https://github.com/qbittorrent/qBittorrent/wiki/Linux-WebUI-HTTPS-with-Let's-Encrypt-certificates-and-NGINX-SSL-reverse-proxy

kharenis · 2021-04-28T13:29:09Z

@Piccirello
I use Haproxy for my reverse proxy and currently have it set to redirect if missing the trailing slash, and then rewrite the path for the forwarded request.
acl qbittorrent-redirect-acl var(txn.txnpath) -m end -i /qbittorrent
http-request set-var(txn.txnpath) path http-request redirect scheme https drop-query append-slash if qbittorrent-redirect-acl http-request set-path %[path,regsub(^/qbittorrent/,/)] if !qbittorrent-redirect-acl

@glassez

Is it correct in HTTP world that both http://mywebsite.com/qbt and http://mywebsite.com/qbt/ points to the same page (if exclude reverse proxy from this scenario)? And how would the relative links on this page behave in this case?

Technically no, whilst they're both a path to a resource, /qbt is treated as the file "qbt" in the "/" directory, and /qbt/ as a directory - with the convention that something representing the directory will be returned (typically an index page).
They're akin to:
http://10.10.10.10:8080/index.html and http://10.10.10.10:8080/

The relative paths get appended to the deepest directory in the path.

I guess what it really comes down to, is it qbt's responsibility to make sure it's being accessed using a directory path as it expects, or should the user have to make their proxy only allow access through a url with a trailing "/".

@FranciscoPombal

I just refreshed the wiki pages documenting reverse proxy setups with NGINX. I have never had this / problem. I use these setups on my machines pretty much as-is, and it "just works". Will they be impacted by this change?

Links to the relevant wiki pages:

https://github.com/qbittorrent/qBittorrent/wiki/NGINX-Reverse-Proxy-for-Web-UI
https://github.com/qbittorrent/qBittorrent/wiki/Linux-WebUI-HTTPS-with-Let's-Encrypt-certificates-and-NGINX-SSL-reverse-proxy

I don't use Nginx as my reverse proxy so haven't got experience with it, but are you able to access qbt without a trailing "/" in the url?
Also, from a quick search of the code, it looks like qbt doesn't make use of these headers;
X-Forwarded-Proto X-Forwarded-For X-Real-IP

I think the best way to handle it is a configurable base path in qbt so it always points things in the right direction without users having to faff around with proxy setups.

glassez · 2021-04-28T14:44:32Z

I guess what it really comes down to, is it qbt's responsibility to make sure it's being accessed using a directory path

For me this sounds the same as "it's application's responsibility to make sure it's being run using an executable file with some particular path". Unless you mean web UI rather than qBittorrent itself...

kharenis · 2021-04-28T20:54:44Z

I guess what it really comes down to, is it qbt's responsibility to make sure it's being accessed using a directory path

For me this sounds the same as "it's application's responsibility to make sure it's being run using an executable file with some particular path". Unless you mean web UI rather than qBittorrent itself...

Ah yep, I did indeed mean the web UI!

FranciscoPombal · 2021-04-29T13:10:18Z

@kharenis

@FranciscoPombal

I don't use Nginx as my reverse proxy so haven't got experience with it, but are you able to access qbt without a trailing "/" in the url?

Yes. What I notice is that a / is automatically appended. So, when I input mywebsite.com/qbt in the browser's address bar and hit return, the page loads successfully, and then the address bar will read mywebsite.com/qbt/ after the page is done loading. This means NGINX is redirecting me automatically.

I tried this with the reference setups documented in the wiki and a fresh browser cache/data every time, and this is reproducible. I opened up Firefox's network monitor with cache disabled, and indeed I found that I'm getting 301'd automatically:

request headers:

GET /qbt HTTP/2
Host: mywebsite.com
User-Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:88.0) Gecko/20100101 Firefox/88.0
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,*/*;q=0.8
Accept-Language: en-US,en;q=0.5
Accept-Encoding: gzip, deflate, br
DNT: 1
Connection: keep-alive
Upgrade-Insecure-Requests: 1

response headers:

HTTP/2 301 Moved Permanently
server: nginx
date: <snip>
content-type: text/html
content-length: 162
location: https://mywebsite.com/qbt/
strict-transport-security: max-age=31536000; includeSubDomains; preload
X-Firefox-Spdy: h2

and the subsequent requests/responses to the path with the trailing / work as normal. In fact, these results are consistent with what the NGINX docs say about such automatic redirections (https://nginx.org/en/docs/http/ngx_http_core_module.html#location):

If a location is defined by a prefix string that ends with the slash character, and requests are processed by one of proxy_pass, fastcgi_pass, uwsgi_pass, scgi_pass, memcached_pass, or grpc_pass, then the special processing is performed. In response to a request with URI equal to this string, but without the trailing slash, a permanent redirect with the code 301 will be returned to the requested URI with the slash appended. If this is not desired, an exact match of the URI and location could be defined like this:
location /user/ {
    proxy_pass http://user.example.com;
}

location = /user {
    proxy_pass http://login.example.com;
}

There is probably a way to accomplish this in other webservers as well, even if it is not the default.

Also, from a quick search of the code, it looks like qbt doesn't make use of these headers;
X-Forwarded-Proto X-Forwarded-For X-Real-IP

Yeah, they are there "for the future". It would be very nice to make use of at least X-Forwarded-For to not lock out reverse proxy users with no recourse (for whatever duration that the lock-out is configured for) after too many failed password attempts. This is because without looking at that header or something of the sort, from qBittorrent's perspective, all requests are made from the internal proxy IP address (typically 127.0.0.1), not the user's actual IP address. See also #11908.

I think the best way to handle it is a configurable base path in qbt so it always points things in the right direction without users having to faff around with proxy setups.

Since it currently "just works" with NGINX, wouldn't it be a better idea to simply find out what is needed to make it work with other webservers? Then it would be sufficient to add some text to qBittorrent's "javascript is disabled" error paragraph mentioning the necessary config change. After all, even if you introduce a configurable base path setting, it will still be broken out of the box for some users, and we have to inform them about what needs to be changed to fix the problem.

github-actions · 2021-08-10T00:49:42Z

This PR is stale because it has been 60 days with no activity. This PR will be automatically closed within 7 days if there is no further activity.

github-actions · 2021-08-18T00:47:49Z

This PR was closed because it has been stalled for some time with no activity.

Append Trailing Slash To Path If Missing

209f784

Closes qbittorrent#5693 Index pages now automatically redirect to a path with a trailing "/". If the site was accessed through a proxy with a modified path, a missing trailing "/" would break the relative paths.

kharenis force-pushed the trailing-slash branch from 642ce51 to 209f784 Compare April 19, 2021 15:09

Chocobo1 requested a review from a team April 22, 2021 04:29

glassez changed the title ~~Automatically Append / to url when missing~~ Append trailing slash to URL if missing Apr 22, 2021

glassez added the WebUI WebUI-related issues/changes label Apr 22, 2021

Chocobo1 reviewed Apr 23, 2021

View reviewed changes

FranciscoPombal requested a review from Piccirello April 26, 2021 14:35

Piccirello reviewed Apr 27, 2021

View reviewed changes

github-actions bot added the Stale label Aug 10, 2021

github-actions bot closed this Aug 18, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Append trailing slash to URL if missing #14822

Append trailing slash to URL if missing #14822

kharenis commented Apr 19, 2021

glassez commented Apr 22, 2021

kharenis commented Apr 22, 2021

glassez commented Apr 22, 2021

Chocobo1 Apr 23, 2021

Chocobo1 Apr 23, 2021

Chocobo1 Apr 23, 2021

Chocobo1 Apr 23, 2021

Chocobo1 Apr 23, 2021

FranciscoPombal commented Apr 26, 2021

Piccirello commented Apr 26, 2021

Piccirello left a comment

kharenis commented Apr 27, 2021

glassez commented Apr 27, 2021

glassez commented Apr 27, 2021

Piccirello commented Apr 27, 2021

glassez commented Apr 27, 2021

FranciscoPombal commented Apr 27, 2021

kharenis commented Apr 28, 2021 •

edited

Loading

glassez commented Apr 28, 2021 •

edited

Loading

kharenis commented Apr 28, 2021

FranciscoPombal commented Apr 29, 2021

github-actions bot commented Aug 10, 2021

github-actions bot commented Aug 18, 2021

Append trailing slash to URL if missing #14822

Append trailing slash to URL if missing #14822

Conversation

kharenis commented Apr 19, 2021

glassez commented Apr 22, 2021

kharenis commented Apr 22, 2021

glassez commented Apr 22, 2021

Chocobo1 Apr 23, 2021

Choose a reason for hiding this comment

Chocobo1 Apr 23, 2021

Choose a reason for hiding this comment

Chocobo1 Apr 23, 2021

Choose a reason for hiding this comment

Chocobo1 Apr 23, 2021

Choose a reason for hiding this comment

Chocobo1 Apr 23, 2021

Choose a reason for hiding this comment

FranciscoPombal commented Apr 26, 2021

Piccirello commented Apr 26, 2021

Piccirello left a comment

Choose a reason for hiding this comment

kharenis commented Apr 27, 2021

glassez commented Apr 27, 2021

glassez commented Apr 27, 2021

Piccirello commented Apr 27, 2021

glassez commented Apr 27, 2021

FranciscoPombal commented Apr 27, 2021

kharenis commented Apr 28, 2021 • edited Loading

glassez commented Apr 28, 2021 • edited Loading

kharenis commented Apr 28, 2021

FranciscoPombal commented Apr 29, 2021

github-actions bot commented Aug 10, 2021

github-actions bot commented Aug 18, 2021

kharenis commented Apr 28, 2021 •

edited

Loading

glassez commented Apr 28, 2021 •

edited

Loading