WebUI: Implement "Secure" flag for session cookie. Closes #11724

[FranciscoPombal]

Closes #11724.

Option is enabled by default for users using qBittorrent's built-in HTTPS capabilities. This flag will never be set if qBittorrent is using plain HTTP.

Users using HTTPS reverse proxies, like "qbt <-> (http) <-> proxy <-> (https) <-> user" should override the flag in the proxy in order to set it, if they wish to do so.

Testing:

• DONE Using HTTP: WebUI works unless secure cookie option is selected (which is expected, and hence the warning beside the option).
• DONE Using built-in HTTPS (with self-signed certs, real certs should not make a difference): works as expected; WebUI still works even if the secure cookie option is disabled (as previously).
• TODODONE: HTTPS with reverse proxy (like the drawing in https://github.com/qbittorrent/qBittorrent/wiki/Linux-WebUI-HTTPS-with-Let's-Encrypt-certificates-and-NGINX-SSL-reverse-proxy)

I installed nginx and used the following config:

# Default server configuration
#
server {
    listen 80 default_server;
    listen [::]:80 default_server;

    # SSL configuration
    #
    listen 443 ssl default_server;
    listen [::]:443 ssl default_server;
    # Self signed certs generated by the ssl-cert package
    # Don't use them in a production server!
    #
    include snippets/snakeoil.conf;

    root /var/www/html;

    # Add index.php to the list if you are using PHP
    index index.html index.htm index.nginx-debian.html;

    server_name _;

    location / {
        # First attempt to serve request as file, then
        # as directory, then fall back to displaying a 404.
        try_files $uri $uri/ =404;
    }

    location /qbit/ {
 
        proxy_pass           http://127.0.0.1:8080/;
 
        gzip_proxied         any;
        client_max_body_size 100M; # increased POST request size limit, for allowing adding a lot of torrents at once
 
        proxy_http_version   1.1;
 
        proxy_set_header     Host                              127.0.0.1:8080;
        proxy_set_header     X-Forwarded-Proto                 $scheme;
        proxy_set_header     X-Forwarded-Host                  $server_name:$server_port;
        proxy_set_header     X-Forwarded-For                   $remote_addr;
        proxy_set_header     X-Real-IP                         $remote_addr;
 
        proxy_hide_header    Referer;
        proxy_hide_header    Origin;
        proxy_set_header     Referer                           '';
        proxy_set_header     Origin                            '';
 
        # recent versions of qbittorrent already set headers like x-content-type-options, x-frame-options, and x-xss-protection
        add_header           X-Robots-Tag                      none;
        add_header           X-Permitted-Cross-Domain-Policies none;
        add_header           X-Download-Options                noopen;
 
        add_header           Strict-Transport-Security        'max-age=31536000; includeSubDomains; preload' always;
 
        add_header           Allow                             "GET, POST, HEAD" always;
        if ( $request_method !~ ^(GET|POST|HEAD)$ ) {
            return 405;
        }
    }
}

This config is close to a real "production" config, except it uses self-signed certs and does not do automatic HTTP->HTTPS redirection, for testing purposes. So the WebUI can be accessed either through either http://localhost/qbit/ or https://localhost/qbit/ (127.0.0.1 can also be used instead of localhost).

When the user logs in at https://localhost/qbit/ and then tries to access http://localhost/qbit/, they are unable to do so, and land back in the login page, which proves the cookie's secure flag is working as intended.

[Chocobo1]

What is the usefulness of using the secure attribute? I've read a little bit on the web and also this: https://en.wikipedia.org/wiki/Secure_cookie but IMO it isn't really useful to qbt particular case.

[Piccirello]

but IMO it isn't really useful to qbt particular case.

The secure flag makes it such that a cookie set in a secure context isn't accessible outside that context.
This helps protect against https downgrade attacks, which themselves can result in cookie theft and full account takeover. Downgrade attacks can only truly be prevented when a domain is using HSTS and is preloaded into the user's browser, which is quite rare.

I haven't reviewed this PR yet but will add my review.

[FranciscoPombal]

@Piccirello @Chocobo1 See update in OP, I have successfully completed the testing that I deemed necessary. Let me know if you think of anything else

[Chocobo1]

I seem to understand the topic a better now. Now I think whether we should always set the secure flag when https is in use in our http server instead of providing an UI option?

[FranciscoPombal]

@Chocobo1

I seem to understand the topic a better now. Now I think whether we should always set the secure flag when https is in use in our http server instead of providing an UI option?

I also thought that should be the case at first. However, the point of implementing this as an option is to allow more flexibility and ensure everything works properly in all cases, i.e., even if it's not qBittorrent itself providing the secure HTTPS communication channel with the browser:
#11724 (comment)

But maybe changing this:

if(m_isSecureCookieEnabled)
    cookie.setSecure(true);

to this:

if(m_isSecureCookieEnabled || m_isHttpsEnabled)
    cookie.setSecure(true);

and renaming the option to something like "Force secure cookie flag (useful for HTTPS reverse proxy configs)" would be the best of both worlds. This way, users using qBittorrent's built-in HTTPS directly automatically have the secure flag set out-of-the-box. HTTPS reverse proxy users still need to turn on the option. Thoughts?

[glassez]

Please fix commit message. You need to move "Closes" statement into message body.

[FranciscoPombal]

@glassez
Fixed

[Chocobo1]

HTTPS reverse proxy users still need to turn on the option.

I think we shouldn't take that case into consideration as HTTP is an end-to-end protocol and proxy breaks the end-to-end design.
Of course there will always be users wanting finer controls, so I'm thinking of cookie.setSecure(m_isSecureCookieEnabled && m_isHttpsEnabled);
And the option could be named "Set cookie secure flag when using HTTPS" with default ON.

[FranciscoPombal]

Of course there will always be users wanting finer controls, so I'm thinking of cookie.setSecure(m_isSecureCookieEnabled && m_isHttpsEnabled);

I will admit that doing it that way is more comfortable for users using a simple HTTPS setup. But the trade-off is that it completely cuts off the users using HTTPS reverse proxy (such as me). The way I proposed, everyone is given an option.

[Chocobo1]

But the trade-off is that it completely cuts off the users using HTTPS reverse proxy (such as me).

Do you mean qbt <-> (http) <-> proxy <-> (https) <-> user?
By using a proxy in the middle you can literally override any cookie flags, thus I would suggest you just do that, or did I misunderstood something?

[Piccirello]

By using a proxy in the middle you can literally override any cookie flags

I'm familiar with a reverse proxy's ability to modify arbitrary headers, but I'm not sure about arbitrary cookie flags. Nginx has proxy_cookie_domain and proxy_cookie_path for overwriting the Set-Cookie domain and path, respectively, but I don't see options for other cookie flags (secure, httpOnly, sameSite, etc). Am I overlooking the option?

We generally set http headers based on what will be of most value to most users. A lot of our users use a reverse proxy (myself included), but we usually don't tailor application behavior to them due to the proxy's ability to add/remove arbitrary headers. So if this is possible with a reverse proxy then I'd personally rather not add the option and instead just always set the secure flag when using https. But I'm not strictly opposed to the option either.

[Chocobo1]

but I don't see options for other cookie flags (secure, httpOnly, sameSite, etc). Am I overlooking the option?

FYI, this is google first answer: https://geekflare.com/httponly-secure-cookie-nginx/
However I couldn't verify it as I don't have a setup at hand.

[FranciscoPombal]

@Chocobo1

Do you mean qbt <-> (http) <-> proxy <-> (https) <-> user?

Exactly.

By using a proxy in the middle you can literally override any cookie flags, thus I would suggest you just do that, or did I misunderstood something?

I wasn't aware of that. I just knew that you could modify HTTP headers arbitrarily.

FYI, this is google first answer: https://geekflare.com/httponly-secure-cookie-nginx/
However I couldn't verify it as I don't have a setup at hand.

Thanks, I will test this. If it works, I will change the PR to the option you mentioned above,

cookie.setSecure(m_isSecureCookieEnabled && m_isHttpsEnabled);

and then I will update the relevant wiki pages, so that reverse proxy users know how they should change their setup.

[FranciscoPombal]

@Piccirello
Looks like adding a simple

 proxy_cookie_path / "/; Secure";

inside the location /qbit/ stanza does the trick.

[FranciscoPombal]

@Chocobo1 @Piccirello
Implemented suggested changes.

[glassez]

Shouldn't you disable "Enable cookie Secure flag" checkbox when HTTPS is disabled?

[FranciscoPombal]

@Chocobo1 @glassez
Implemented requested changes.

[Chocobo1]

Based on the current wording I would expect this option to be disabled/greyed out when qBittorrent's HTTPS option is disabled.

The latest update does that.

[Chocobo1]

@FranciscoPombal
Thank you!