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: append port to session cookie name #21619

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

dyseg
Copy link
Contributor

@dyseg dyseg commented Oct 15, 2024

Prevents overwriting the shared cookie between qBt sessions on the same host.
Separated from #21618. Original topic: #20873 (comment)

@glassez
Copy link
Member

glassez commented Oct 16, 2024

The issue of multiple qBittorrent instances under the same host was already fixed by implementing suggestion of allowing you to set custom cookie name.
Original Issue is #18329. Fixed by #18384.

@dyseg
Copy link
Contributor Author

dyseg commented Oct 16, 2024

In this case from my side this PR can be closed.

@Piccirello
Copy link
Member

It would be really nice to fix the multi-instance scenario for users without requiring them to manually configure a custom cookie name. I suspect most users are not aware of this footgun, and so having a comprehensive fix will also prevent these users from opening GitHub issues unnecessarily.

m_sessionCookieName = DEFAULT_SESSION_COOKIE_NAME;
m_sessionCookieName = DEFAULT_SESSION_COOKIE_NAME + QString::number(m_webUiPort);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An alternative would be to come up with a unique cookie name on startup.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, the current method looks better, since it still identifies the cookie name with a specific qBittorrent instance, so that some client with a smarter implementation than browsers (which is why the problem is) could even send it to a specific instance and not to everyone on the same host.

@glassez
Copy link
Member

glassez commented Oct 17, 2024

It would be really nice to fix the multi-instance scenario for users without requiring them to manually configure a custom cookie name. I suspect most users are not aware of this footgun, and so having a comprehensive fix will also prevent these users from opening GitHub issues unnecessarily.

It's reasonable.
But shouldn't it then completely replace the current solution with a manually set name? It hardly makes much sense to have both of them at the same time.

@Piccirello
Copy link
Member

It's reasonable. But shouldn't it then completely replace the current solution with a manually set name? It hardly makes much sense to have both of them at the same time.

Yes, I think having this would eliminate the need for setting a custom cookie name.

@Chocobo1

This comment was marked as resolved.

@xavier2k6 xavier2k6 added the WebUI WebUI-related issues/changes label Oct 17, 2024
@dyseg
Copy link
Contributor Author

dyseg commented Oct 17, 2024

Btw, chances are high, that other web applications name their session cookie also as SID. One example: gerbera/gerbera#2058 (comment)
And I'm sure that a quick github search would reveal more.
Even if the dynamic cookie name is not introduced, the default name could be changed to something which is unlikely to conflict with other webapps on the same host, path.

@Piccirello
Copy link
Member

Btw, chances are high, that other web applications name their session cookie also as SID. One example: gerbera/gerbera#2058 (comment) And I'm sure that a quick github search would reveal more. Even if the dynamic cookie name is not introduced, the default name could be changed to something which is unlikely to conflict with other webapps on the same host, path.

This is a good point. This PR switches the static portion of the name to QBT_SID, which should handle that.

@@ -179,7 +179,7 @@ WebApplication::WebApplication(IApplication *app, QObject *parent)
LogMsg(tr("Unacceptable session cookie name is specified: '%1'. Default one is used.")
.arg(m_sessionCookieName), Log::WARNING);
}
m_sessionCookieName = DEFAULT_SESSION_COOKIE_NAME;
m_sessionCookieName = DEFAULT_SESSION_COOKIE_NAME + QString::number(m_webUiPort);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
m_sessionCookieName = DEFAULT_SESSION_COOKIE_NAME + QString::number(m_webUiPort);
m_sessionCookieName = DEFAULT_SESSION_COOKIE_NAME + QString::number(Preferences::instance()->getWebUIPort());

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.

6 participants