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

🐛 webui: Fix bug of cookie cannot set expiration #622

Merged
merged 1 commit into from
Nov 3, 2024

Conversation

Rirmach
Copy link

@Rirmach Rirmach commented Oct 30, 2024

Fix issue that expiration of cookie (SID) is not correctly set, resulting in logging in status cannot be remembered
Expiration using settings -> WebUI -> Session timeout

@c0re100 c0re100 merged commit 139035a into c0re100:v5_0_x Nov 3, 2024
20 checks passed
@database64128
Copy link

There are 3 problems with this patch.

  1. The API will hand out an expired cookie, if the session timeout is not positive, which is allowed!
  2. After the cookie expires, the web UI simply says it lost connectivity to the server, without redirecting to the login page. This is confusing and misleading.
  3. According to the developer's response WebUI login doesn't persist. cookie expiration is never set and defaults to "session". qbittorrent/qBittorrent#20993 (comment), this is the fault of specific browsers.

@c0re100 Please consider removing this patch. Thanks.

@Rirmach
Copy link
Author

Rirmach commented Dec 10, 2024

There are 3 problems with this patch.

  1. The API will hand out an expired cookie, if the session timeout is not positive, which is allowed!
  2. After the cookie expires, the web UI simply says it lost connectivity to the server, without redirecting to the login page. This is confusing and misleading.
  3. According to the developer's response WebUI login doesn't persist. cookie expiration is never set and defaults to "session". qbittorrent/qBittorrent#20993 (comment), this is the fault of specific browsers.

@c0re100 Please consider removing this patch. Thanks.

  1. For some session timeout settings that lead to invalid cookie expiration, additional handle logic could be added instead of just remove. Maybe cookie with expiration is quite an common practice in modern frontend technology stack and has the same meaning with session timeout (frontend vs backend).
  2. An incomplete implementation of default frontend webui. In fact, vuetorrent do redirect normally when cookie expired.
  3. I've tried newly installed legacy version of firefox (ver 116.0.2) and newer ms edge (Chromium) ver 131.0 and same issue confirmed. Maybe the developer could do some work of adaptation for modern browsers that widely used, instead of providing no feasible and effective solution, ignoring the user experience, just requiring users to try to find an unclear specific browser.

@database64128
Copy link

Maybe this patch works well for you, but it introduces major inconveniences for users like me who prefer to keep a tab of the web UI open in the browser for extended periods, often without closing the browser unless there's a system or browser update.

Before this patch, with default settings, I only needed to log in once on the initial launch of qBittorrent, and never had to do it again until maybe a system update. After this patch, the web UI breaks every hour, and my first attempt to fix this (by setting the timeout to -1) immediately revealed a problem with this patch, rendering the web UI unusable and forcing me to SSH into the system and send SIGTERM to qBittorrent.

It also took me about half an hour to find this PR, because I never expected it was a patch carried by qBee, and I was really surprised when I found out about this PR. Ultimately, it's up to @c0re100 to decide whether qBee should carry this patch. IMO this patch should have been at least tried in the upstream first, instead of just straight up landing in qBee without any proper review.

@Rirmach I think I understand where you are coming from, and your points make sense. But until we have proper fixes for the frontend and timeout issue, and have tried other things I mentioned above, we shouldn't carry this patch.

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

Successfully merging this pull request may close these issues.

3 participants