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

Cookies with invalid SameSite will be rejected #1053

Closed
MK-RD opened this issue Aug 12, 2021 · 32 comments
Closed

Cookies with invalid SameSite will be rejected #1053

MK-RD opened this issue Aug 12, 2021 · 32 comments
Labels
core Core functionalities, including the admin section enhancement New feature or request

Comments

@MK-RD
Copy link

MK-RD commented Aug 12, 2021

The "qtrans_admin_language" cookie will soon be rejected because it specifies either "None" or an invalid value for the "SameSite" attribute without using the "secure" attribute. For more information on the "SameSite" attribute, see https://developer.mozilla.org/docs/Web/HTTP/Headers/Set-Cookie/SameSite.

@herrvigg
Copy link
Collaborator

See also #976. Would the solution to add None when the Secure option is enabled?

@herrvigg herrvigg added the enhancement New feature or request label Aug 13, 2021
@herrvigg
Copy link
Collaborator

And set explicitly Lax for non-Secure connection?

@herrvigg herrvigg added the core Core functionalities, including the admin section label Aug 13, 2021
@MK-RD
Copy link
Author

MK-RD commented Aug 13, 2021

And set explicitly Lax for non-Secure connection?

Well, of course a secure cookie would be correct!
For anything else, I will not make a recommendation.

The warning appears because the SameSite policy for a cookie was not explicitly specified.
SameSite=None requires Secure!

@herrvigg
Copy link
Collaborator

This doesn't tell what SameSite to set.

@MK-RD
Copy link
Author

MK-RD commented Aug 13, 2021

In order to answer something like this, one would first have to determine the reasons why one does not want to use TLS encryption here.

@MK-RD
Copy link
Author

MK-RD commented Aug 13, 2021

If security is not required here and no server-side business logic is involved, then I would recommend alternative methods such as LocalStorage.

@herrvigg
Copy link
Collaborator

We don't need to answer these questions about TLS or Secure. The question here is the SameSite, with Secure or without Secure.

@herrvigg
Copy link
Collaborator

There's also qtrans_front_language which is the main cookie for the user navigation.

@herrvigg herrvigg changed the title Info: The "qtrans_admin_language" cookie will soon be rejected! Cookies with invalid SameSite will be rejected Aug 14, 2021
@herrvigg
Copy link
Collaborator

Better explanations here: https://web.dev/samesite-cookies-explained/

Imo we can set SameSite=Lax for both cookies (front and admin), regardless of the Secure flag. This should be the current behavior.
I don't really see the advantage of setting to SameSite=None with Secure.
Setting to Strict would be too restrictive, especially for the front cookie. We want to keep the user preferences when following links.

@MK-RD
Copy link
Author

MK-RD commented Aug 14, 2021

Even if the cookie does not store sensitive data, the cookie should only be transmitted from the client via a secure HTTPS connection if the application (HTTPS, FORCE_SSL_ADMIN) is configured in this way.

Another question is whether the cookie should be passed on for cross-domain requests.

In the case of an unsecured HTTP connection, the SameSite policy is useful to avoid misconfigurations.

In our production environments, data protection is very important and therefore these settings
adjusted by us accordingly - Usually only first-party cookies.

@herrvigg
Copy link
Collaborator

Again... why do you mention the secure HTTPS connection? There's an option in qTranslate-XT allowing to enable this. The Secure flag is set in that case. There should be no problem about this.

The main topic is here is SameSite which is different. I suggest we set Lax everywhere. Only if we really want SameSite=None we would require the Secure flag. I don't see any good reason for wanting this.

@herrvigg
Copy link
Collaborator

The question about the Secure has been fixed long ago: see #467. I opened a question regarding auto-detection and possible suppression of the force secure cookie option: #742 but that's a whole different topic. Not related to SameSite.

@MK-RD
Copy link
Author

MK-RD commented Aug 14, 2021

If the admin area is accessed via an HTTPS connection, only secure cookies should be used by default. In this case, the value for "SameSite" can remain on "None" by default.

If the admin area is called via an HTTP connection, the value for "SameSite" must be set by default in the future. In this case, the "Lax" value is a good compromise.

At installation time, the SameSite value cannot be determined for all use cases.

@herrvigg
Copy link
Collaborator

"SameSite" can remain on "None" by default.

Why would you want this for qTranslate? Think about the real use cases for admin and front cookies.

In our production environments, data protection is very important and therefore these settings adjusted by us accordingly - Usually only first-party cookies.

SameSite=None goes in contradiction with this. You'd rather want Strict in that case but this is not a good idea.

@MK-RD
Copy link
Author

MK-RD commented Aug 14, 2021

For the administration area I would of course always use "Strict" for "SameSite"!

@herrvigg
Copy link
Collaborator

of course

I don't see why this is so obvious. If you come to admin from a link you'd rather want the correct admin language to be set.

@MK-RD
Copy link
Author

MK-RD commented Aug 14, 2021

Why should the cookie be sent for cross-domain requests?

@MK-RD
Copy link
Author

MK-RD commented Aug 14, 2021

Anyway, that's my point of view.

@herrvigg
Copy link
Collaborator

I don't know the exact consequences of not having the admin cookie set when coming back from a link. So by default you'd rather want this. This is supported by Lax, unlike Strict.

The question is more why would you want to block it with Strict? First, you asked None which is the complete opposite. The need doesn't seem very clear. Please read this more carefully: https://web.dev/samesite-cookies-explained/

@MK-RD
Copy link
Author

MK-RD commented Aug 14, 2021

If the admin area is accessed via an HTTPS connection, only secure cookies should be used by default. In this case, the value for "SameSite" can remain on "None" by default.

If the admin area is called via an HTTP connection, the value for "SameSite" must be set by default in the future. In this case, the "Lax" value is a good compromise.

At installation time, the SameSite value cannot be determined for all use cases.

I don't think I need to go to school here!

@herrvigg
Copy link
Collaborator

Me neither. You just wrote None here... and now Strict... It's quite clear the real need is not clear.

I will set Lax by default everywhere unless someone finds a good reason for doing differently.

herrvigg added a commit that referenced this issue Aug 14, 2021
Set SameSite explicitly to avoid future browser rejection.
The options API requires PHP 7.3+.
herrvigg added a commit that referenced this issue Aug 14, 2021
Set SameSite explicitly to avoid future browser rejection.
The options API requires PHP 7.3.0+.
@MK-RD
Copy link
Author

MK-RD commented Aug 28, 2021

Me neither. You just wrote None here... and now Strict... It's quite clear the real need is not clear.

I will set Lax by default everywhere unless someone finds a good reason for doing differently.

The only thing that is now "by default" is the new potential for misconfigurations (HTTPS, SameSite), of course, hard-coded.

function qtranxf_setcookie_language($lang, $cookie_name, $cookie_path) {
    global $q_config;

    // SameSite only available with options API from PHP 7.3.0
    if (version_compare( PHP_VERSION, '7.3.0') >= 0) {
        setcookie( $cookie_name, $lang, [
            'expires'  => strtotime('+1year'),
            'path'     => $cookie_path,
            'secure'   => $q_config['use_secure_cookie'],
            'httponly' => true,
            'samesite' => 'Lax'
        ] );
    } else {
        // only meant for server-side, set 'httponly' flag
        setcookie($cookie_name, $lang, strtotime('+1year'), $cookie_path, null, $q_config['use_secure_cookie'], true);
    }
}

It's a real shame that nobody is involved here!

@herrvigg
Copy link
Collaborator

new potential for misconfigurations (HTTPS, SameSite), of course, hard-coded.
It's a real shame that nobody is involved here!

Please change this tone, these are not constructive comments.
Again, you criticize without explaining your real need and where there could be a problem. "Potential misconfiguration" is speculation only. The comments you gave are self-contradictory. I will not add additional custom options just for the sake of being not "hard-coded". This adds additional complexity to maintain, it must be justified. Let's stick to real use cases!

@herrvigg
Copy link
Collaborator

The current default behavior for SameSite before this patch is Lax. If this were a real problem we would have known it already. The current patch is just making this choice explicit to avoid future potential rejections, not based on the value of SameSite, but from the implicit choice. We don't even know when the browsers will enforce this check.

@MK-RD
Copy link
Author

MK-RD commented Aug 28, 2021

Here's a more flexible approach.

if (defined('QTX_COOKIE_SAMESITE')) {
    $coockie_samesite = QTX_COOKIE_SAMESITE;
}

@herrvigg
Copy link
Collaborator

Added a constant, but without check. We want to set the policy explicitly from now.

You should explicitly communicate the intended SameSite policy for your cookie (rather than relying on browsers to apply SameSite=Lax automatically). This will also improve the experience across browsers as not all of them default to Lax yet.

In fact, all the modern browsers have raised their default SameSite policy from None (legacy) to Lax long ago (e.g. Chrome 80 early 2020). This works regardless of HTTPS. The prehistoric ones don't handle anything at all, but in all logic receiving SameSite will not change anything for those not supporting it. It's up to the users to upgrade their client for better security.

@herrvigg
Copy link
Collaborator

Regarding the very potential future cases:

  • some admins may want to raise even more to Strict. But that's very unlikely for this kind of cookie.
  • some admins may want to go down to None (combined with HTTPS). But this is less secure, to enable any cross-site request (for which purpose?).

Almost 2 years have passed already without anyone needing any of this.

@MK-RD
Copy link
Author

MK-RD commented Aug 28, 2021

Regarding the very potential future cases:

* some admins may want to raise even more to `Strict`. But that's very unlikely for this kind of cookie.

* some admins may want to go down to `None` (combined with HTTPS). But this is less secure, to enable any cross-site request (for which purpose?).

Almost 2 years have passed already without anyone needing any of this.

Of course you can see it that way. Especially for backend cookies. Frontend cookies have a different attention due to privacy issues. Regardless of whether this is relevant from a security point of view. Technical requirements etc.

I would really like to end this topic now.

@herrvigg
Copy link
Collaborator

I would really like to end this topic now.

But you don't.

Frontend cookies have a different attention due to privacy issues. Regardless of whether this is relevant from a security point of view. Technical requirements etc.

My explanations are valid both for the admin and frontend.

Since the beginning you have been mentioning HTTPS which is entirely customizable by the admin so there can't be any problem about it. Eventually my plan is even to remove this option but HTTPS is not the topic here.

The topic is Samesite. What are these "technical requirements"? You still don't explain CLEARLY what you need between None and Strict which are two complete opposites. In this thread you have been asking first for one, then for the other. In practice, we have been with Lax for two years. Has this been a real problem in your setup? It doesn't seem so.

@MK-RD
Copy link
Author

MK-RD commented Aug 28, 2021

Specifications on customer side (agencies) regardless of our opinion!

I will adjust the plugin itself after each update.

So don't worry about me!

The SameSite property has several values for a reason.

@herrvigg
Copy link
Collaborator

I will adjust the plugin itself after each update.
The SameSite property has several values for a reason.

Have you been changing them until now?

@herrvigg
Copy link
Collaborator

Released in 3.11.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Core functionalities, including the admin section enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants