Samesite attribute for session cookie.

[campio97]

Added the option to set the samesite attribute for the session cookie. Default is "Strict"

[mmihalev]

I'm a little bit confused about the "None" option. I can't find it documented in the official php documentation - https://www.php.net/manual/en/function.setcookie.php
They seems to document only support for "Strict" and "Lax".

[campio97]

A future release of Chrome will only deliver cookies with cross-site requests if they are set with SameSite=None. For more detailshttps://www.chromestatus.com/feature/5088147346030592

[mmihalev]

A future release of Chrome will only deliver cookies with cross-site requests if they are set with SameSite=None. For more detailshttps://www.chromestatus.com/feature/5088147346030592

Yes , I'm familiar with this. However, I can't find anything about the "None" value. Nothing in the PHP RFC - https://wiki.php.net/rfc/same-site-cookie and nothing in the PHP Official Docs - https://www.php.net/manual/en/function.setcookie.php

That's why I'm confused :)

[sonnlv]

I tried this but my code not working.
here is my config



help me pls.

[campio97]

A future release of Chrome will only deliver cookies with cross-site requests if they are set with SameSite=None. For more detailshttps://www.chromestatus.com/feature/5088147346030592

Yes , I'm familiar with this. However, I can't find anything about the "None" value. Nothing in the PHP RFC - https://wiki.php.net/rfc/same-site-cookie and nothing in the PHP Official Docs - https://www.php.net/manual/en/function.setcookie.php

That's why I'm confused :)

Yes i know, i looked for this info too, but without result. Google said that will be required in April. So i think they will insert this info into the documentation

[campio97]

I tried this but my code not working.
here is my config



help me pls.

Can you post your Session and Cookie config please?

[sonnlv]

Can you post your Session and Cookie config please?
you mean

right?
please let me know how to fix this solution.

[campio97]

Google chrome say that if you want set the cookie SameSite = "None" you have to set the config "cookie_secure" = TRUE.

A future release of Chrome will only deliver cookies with cross-site requests if they are set with SameSite=None and Secure.

[sonnlv]

I did, but it not work even i upgraded php version to 7.3 to use new your commit.

[narfbg]

OK, a few things ...

1. The patch needs to follow our styleguide.
2. Why should 'Strict' be the default? I too want maximum security by default, but this is bound to trigger a wave of issue reports by people who haven't paid attention to this. And it would be especially confusing if (as it would be with the current patch) a PHP version change fundamentally alters this behavior, which leads me to the next point ...
3. There's a way to send cookies without setcookie() - use header(), making it possible to make the feature non-dependant on the PHP version, which we strive to do. This will also solve the problem with SameSite=None, which just won't work via setcookie() PHP's doesn't support it.

[campio97]

OK, a few things ...

1. The patch needs to follow our styleguide.
2. Why should 'Strict' be the default? I too want maximum security by default, but this is bound to trigger a wave of issue reports by people who haven't paid attention to this. And it would be especially confusing if (as it would be with the current patch) a PHP version change fundamentally alters this behavior, which leads me to the next point ...
3. There's a way to send cookies without setcookie() - use header(), making it possible to make the feature non-dependant on the PHP version, which we strive to do. This will also solve the problem with SameSite=None, which just won't work via setcookie() PHP's doesn't support it.

Thank you for your feedback.

1. Ok for the styleguide, i'll be more accurate.
2. So probably the best thing is to use a null value as default.
3. I know we can use the header() function, but this brings many complications. By default in the Session.php file there is this config ini_set('session.use_cookies', 1);, so the cookie is always created by the function session_start(). In php 7.3 has been added the option for samesite. Here the changelog of the set_cookie_params() function: "7.3.0 | An alternative signature supporting an options array has been added. This signature supports also setting of the SameSite cookie attribute."
   I think the best solution is make the patch compatible only with 7.3 version. If you agree I will develop it accordingly. if you prefer to make this feature compatible with previous versions, I am open to suggestions.

[ytetsuro]

@narfbg @campio97

There's a way to send cookies without setcookie() - use header(), making it possible to make the feature non-dependant on the PHP version, which we strive to do. This will also solve the problem with SameSite=None, which just won't work via setcookie() PHP's doesn't support it.

How about making this line the following code?
But that may not be a good solution.

sprintf('%s"; samesite="%s', $this->_config['cookie_path'], $this->_config['sess_cookie_samesite']);

see: https://github.com/nbs-system/snuffleupagus/issues/184

[dont-panic-42]

This code does not work for me - I get:

Message: Undefined index: sess_cookie_samesite
Filename: Session/Session.php
Line Number: 166

Line 166 is the new line in question:

'samesite' 	=> $this->_config['sess_cookie_samesite'],

Tracing the code, the $this->_config array is set up in the _configure() method beginning on line 254.

That is set up to populate $this->_config with various defaults if they were not specified in the $params array passed to the Session constructor. In my case $params is empty, and as _configure() has not been updated for this, then of course the new config item config_item('sess_cookie_samesite') never makes it into $this->_config.

Am I missing something? Shouldn't _configure() also be updated to set up that element of the _config array?

[PhoenixFnX]

This code does not work for me - I get:

Message: Undefined index: sess_cookie_samesite
Filename: Session/Session.php
Line Number: 166

Same issue for me. I also patched the __construct() method with this $param set just before the _configure($params) call :

		// Configuration ...
		isset($params['sess_cookie_samesite']) OR $params['sess_cookie_samesite'] = config_item('sess_cookie_samesite');
		$this->_configure($params);

Moreover, waiting for an official patch I recommend you to create a MY_Session.php file to override the __construct method (which I already did for another purpose)

[OskarsOzols]

This patch doesn't address issue of Safari iOS version 12.x bug https://bugs.webkit.org/show_bug.cgi?id=198181 when setting SameSite to 'None' actually is interpreted as 'Strict'. So for this specific iOS version SameSite attribute should not be applied.

[gxgpet]

Ok, so I think this should be addressed as soon as we can. Here go my thoughts on it...

1. Indeed, the patch needs to follow the style guide.
2. I would say that the issue should be, though, fixed in separated PRs per library/feature. Those should be: CI_Session, CI_Input, and CI_Security (yes, we have to address it for the CSRF feature as well).
3. I would advocate for the default value to be Lax, and not Strict. Exactly how the cookies worked "before", when the attribute value was missing.
4. While we could use the "PHP bug hack", a header() call would ensure that the same code will work for all PHP versions, thus reducing complexity. @campio97, why are you saying that ini_set('session.use_cookies', 1); is preventing us from using header()? I made a super-fast test on CI_Session by using the header() instead of setcookie() and it worked as expected; no duplicated Set-Cookie headers were sent. Am I missing something here?
5. The config value should be under cookie's settings section, and named something like cookie_samesite, and not sess_cookie_samesite. While this should specify a global cookie samesite policy for the entire CI app, CI_Input::set_cookie() should allow this value to be changed when required, by adding a method parameter there.

@OskarsOzols we can't implement a browser version-specific fix into CI's core... Upgrading the browser should fix it.

[narfbg]

1a26510