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

Support configuring WebUI URL base path #21471

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Piccirello
Copy link
Member

@Piccirello Piccirello commented Oct 2, 2024

This PR adds support for configuring the URL base path used to access the WebUI. This makes it easier to use qBittorrent behind a reverse proxy with a custom path (e.g. https://example.com/qbit/).

The WebUI previously relied heavily on relative paths. For example, api requests were made to api/v2/... and scripts were loaded from scripts/lib/... (note the lack of a leading /). Now, the WebUI exclusively makes use of absolute paths, defaulting to a base path of /. These means that, by default, these resources will be loaded from /api/v2/... and /scripts/lib..., respectively. An empty base path can no longer be set - if attempting to do so, it will use /.

Closes #5693.

Screenshot 2024-10-02 at 11 16 00

@Piccirello Piccirello added the WebUI WebUI-related issues/changes label Oct 2, 2024
@Piccirello Piccirello added this to the 5.1 milestone Oct 2, 2024
@Piccirello Piccirello force-pushed the webui-base-url branch 3 times, most recently from bf342fb to b07bc4f Compare October 4, 2024 14:55
@Piccirello Piccirello marked this pull request as ready for review October 8, 2024 22:25
@Piccirello Piccirello requested a review from a team October 8, 2024 22:25
@kevinpeno
Copy link

kevinpeno commented Oct 13, 2024

From what I can tell from the code, using your current examples, setting "URL base path" to qbit/ will result in the same relative path issue. Example instance results in qbit/css/style.css?v=${CACHEID}css/style.css?v=${CACHEID} or worse, if I set to qbit you'd end up with qbitcss/style.css?v=${CACHEID}css/style.css?v=${CACHEID}

You probably want to add some sanity checks if you really want to push this through, but unless a proxy doesn't support rewrites this might be unnecessary (see my comment here).

If you do push forward, you may want to use the base html tag as you only need set this one time in the header rather than at each point a url is needed, which will likely prevent accidental misses should new includes be added at a later time. base should work for scripting too, however sometimes scripting ignores it because it forces url generation rather than relying on the browser to build it, so keep that in mind when testing.

@Piccirello
Copy link
Member Author

From what I can tell from the code, using your current examples, setting "URL base path" to qbit/ will result in the same relative path issue. Example instance results in qbit/css/style.css?v=${CACHEID}css/style.css?v=${CACHEID} or worse, if I set to qbit you'd end up with qbitcss/style.css?v=${CACHEID}css/style.css?v=${CACHEID}

You probably want to add some sanity checks if you really want to push this through, but unless a proxy doesn't support rewrites this might be unnecessary (see my comment here).

If you do push forward, you may want to use the base html tag as you only need set this one time in the header rather than at each point a url is needed, which will likely prevent accidental misses should new includes be added at a later time. base should work for scripting too, however sometimes scripting ignores it because it forces url generation rather than relying on the browser to build it, so keep that in mind when testing.

Thanks for the feedback. There's already a sanity check to ensure the path ends with a trailing slash. Good call on the leading slash - I'll add a check for that.

The reason I'm not using the base tag is because API requests made from the client don't respect it. It does seem like a good safeguard for other resources, though I'd like to avoid a potentially confusing scenario where some resources prepend the base path automatically while others don't.

@Piccirello Piccirello force-pushed the webui-base-url branch 3 times, most recently from 3a65cad to 0c1f2a8 Compare October 15, 2024 07:07
@Piccirello Piccirello force-pushed the webui-base-url branch 2 times, most recently from 71cea6b to 3e6db0d Compare November 4, 2024 20:32
@Piccirello
Copy link
Member Author

@qbittorrent/bug-handlers Can someone please review this? It has been open for over a month. There are merge conflicts but the general approach will remain the same.

@Chocobo1
Copy link
Member

Chocobo1 commented Nov 20, 2024

@qbittorrent/bug-handlers Can someone please review this? It has been open for over a month. There are merge conflicts but the general approach will remain the same.

There is one concern from me but I'm not sure it is fully related. I haven't evaluated thoroughly.
AFAIK the web app is heavily based on one policy: Same-origin policy. The security measures, the storage on browsers, all most everything was designed with the assumption that policy will hold.
Now this PR adds an option to change that and all the related functionality now have to share origin with other "paths". For example, qbt at example.com/qbt and another app at example.com/xyz and they will have the same origin. Here is something that might happen: https://stackoverflow.com/questions/57697269/find-a-way-to-have-different-local-storage-object-for-same-origin-using-differe
Note that I do see many users requesting it but I'm not sure whether it is doable.

Also, another related question: is nginx able to inject base url in the requests on its own (without any help from qbt side)? Is it problematic to do so?

@Piccirello
Copy link
Member Author

The security measures, the storage on browsers, all most everything was designed with the assumption that policy will hold. Now this PR adds an option to change that and all the related functionality now have to share origin with other "paths". For example, qbt at example.com/qbt and another app at example.com/xyz and they will have the same origin. Here is something that might happen: https://stackoverflow.com/questions/57697269/find-a-way-to-have-different-local-storage-object-for-same-origin-using-differe Note that I do see many users requesting it but I'm not sure whether it is doable.

I think it's a valid concern, however users seem to really want this feature. I think the best option would be to allow users to configure this but also make it clear (maybe in the wiki) that this approach is suboptimal and that using a unique (sub)domain is preferable.

Also, another related question: is nginx able to inject base url in the requests on its own (without any help from qbt side)? Is it problematic to do so?

I'm not certain. In #5693 it seems that some users are using nginx's rewrite directive to achieve this. It's not clear to me if other webservers support this too.

The cache now persists more than just translations.
This provides the underlying support for having a configurable base path. Future commit(s) will expose this functionality to users.
This applies to all scripts, css, images, and WebAPI requests.
@Chocobo1
Copy link
Member

Chocobo1 commented Dec 8, 2024

Also, another related question: is nginx able to inject base url in the requests on its own (without any help from qbt side)? Is it problematic to do so?

I'm not certain. In #5693 it seems that some users are using nginx's rewrite directive to achieve this. It's not clear to me if other webservers support this too.

From my experience with reverse proxies, I would prefer that if the reverse proxy is able to support the feature by itself then qbt shouldn't bother with it. This is based on the assumption that the reverse proxy won't have to resort to quirky workarounds. The opening post in #5693 provided an example of nginx config and it looks fine to me. Note that our wiki page for nginx also mentioned such usage.
And thus, I haven't seen the necessity of providing such config knob in qbt.

@glassez
Copy link
Member

glassez commented Dec 8, 2024

I would prefer that if the reverse proxy is able to support the feature by itself then qbt shouldn't bother with it

👍

@Piccirello
Copy link
Member Author

If this qBittorrent feature's goal is to support setting a custom base path, it seems to me that qBittorrent should be responsible for setting that base path. We shouldn't rely on a feature that may be available in a subset of reverse proxies.

@glassez
Copy link
Member

glassez commented Dec 9, 2024

If this qBittorrent feature's goal is to support setting a custom base path, it seems to me that qBittorrent should be responsible for setting that base path.

"Setting a custom base path" is clearly not a goal in itself. Its purpose is only to circumvent the problem of access via reverse proxy (using some kind of configurations).

(I don't remember getting an answer to my previously asked question.) Can't the problem be solved by using relative paths?

@Piccirello
Copy link
Member Author

"Setting a custom base path" is clearly not a goal in itself. Its purpose is only to circumvent the problem of access via reverse proxy (using some kind of configurations).

The goal of this change is to be able to host qBittorrent at some path other than the root (/).

(I don't remember getting an answer to my previously asked question.) Can't the problem be solved by using relative paths?

It can't be solved solely with relative paths, otherwise there would be no need for this PR. I also personally believe absolute paths are prefereable to relative paths when it's possible to use absolute paths.

@glassez
Copy link
Member

glassez commented Dec 17, 2024

It can't be solved solely with relative paths

Do you have anything substantial to back up these words? Some specific case among those found in the qBittorrent WebUI sources, in which it is impossible to use a relative path.

@Piccirello
Copy link
Member Author

It can't be solved solely with relative paths

Do you have anything substantial to back up these words? Some specific case among those found in the qBittorrent WebUI sources, in which it is impossible to use a relative path.

The issue this fixes has an entire thread discussing why this doesn't work. If this could be solved solely with relatively paths, there would be no need for this PR.

What's your reservation with switching to absolute paths?

@glassez
Copy link
Member

glassez commented Dec 18, 2024

The issue this fixes has an entire thread discussing why this doesn't work. If this could be solved solely with relatively paths, there would be no need for this PR.

So, can't you summarize them and provide a brief description of the essence of the problem?

What's your reservation with switching to absolute paths?

For me it looks like a workaround rather than a solution.
Although I don't mind it if it's impossible to have a more flexible/scalable solution.

I really don't understand why it can't work, provided that all resources correctly refer to each other by their relative paths.

@CodeAbuser
Copy link

So, can't you summarize them and provide a brief description of the essence of the problem?

There is a perfectly readable summary in the first comment of the GitHub issue that this PR resolves. I'll paste it here for people that can't find the link...

It would be really useful if there is an option to specify the base URL path for use in reverse proxies. This way one can circumvent having to type the port number into the URL.

For instance, when the qBittorent web UI is running on port 8080, and the base url is set to /qbt. Then the complete URL would become 127.0.0.1:8080/qbt. One can then use, per example, nginx to reverse proxy this to a more friendly url like so:

server {
  listen  80;
  server_name jt.REDACTED.net localhost 192.168.1.6;

  location /qbt {
    proxy_pass http://127.0.0.1:8080;
    proxy_set_header Host $host;
        proxy_set_header X-Real-IP $remote_addr;
        proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
  }
}

Thus allowing one to go to jt.REDACTED.net/qbt and land on the qBittorrent page.

@glassez
Copy link
Member

glassez commented Dec 19, 2024

So, can't you summarize them and provide a brief description of the essence of the problem?

There is a perfectly readable summary in the first comment of the GitHub issue that this PR resolves. I'll paste it here for people that can't find the link...

@CodeAbuser
You obviously didn't understand what I was asking.

@Chocobo1
Copy link
Member

Chocobo1 commented Dec 22, 2024

I really don't understand why it can't work, provided that all resources correctly refer to each other by their relative paths.

I suspect the problem would be solely at web api paths instead of html/js.

But again, if proxies are able to handle it, I don't see why qbt should care about it. Furthermore, I don't see why qbt server would need different base path when there were no proxy involved. (AFAIK a proxy is required in order to run different qbt instances on different base paths)

@glassez
Copy link
Member

glassez commented Dec 22, 2024

I really don't understand why it can't work, provided that all resources correctly refer to each other by their relative paths.

I suspect the problem would be solely at web api paths instead of html/js.

This does not change the essence of the question. If all of them are correctly referenced in the WebAPI code by their relative paths what could be the problem?

P.S. I've just run nginx using config provided in qBittorrent Wiki and I found no problems (at least by testing it quickly).

@Piccirello
Copy link
Member Author

This does not change the essence of the question. If all of them are correctly referenced in the WebAPI code by their relative paths what could be the problem?

P.S. I've just run nginx using config provided in qBittorrent Wiki and I found no problems (at least by testing it quickly).

According to this comment, the Nginx config in the wiki is incomplete.

Another relevant comment: #5693 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WebUI WebUI-related issues/changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Configuration for WebUI base url for use in reverse proxy
5 participants