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

Add support for cookies with "SameSite" option. #13766

Merged
merged 18 commits into from Oct 23, 2019
Merged

Add support for cookies with "SameSite" option. #13766

merged 18 commits into from Oct 23, 2019

Conversation

@ADmad
Copy link
Member

ADmad commented Oct 16, 2019

https://www.owasp.org/index.php/SameSite

I can add / update tests if the changes seem reasonable.

Todo:

  • Add / updates tests
  • Update Http\Client\Response.
@ADmad ADmad added the enhancement label Oct 16, 2019
@ADmad ADmad added this to the 4.0.0 milestone Oct 16, 2019
@ADmad ADmad force-pushed the 4.x-cookie-samesite branch from 2818bf5 to 5209bcf Oct 16, 2019
@ADmad ADmad force-pushed the 4.x-cookie-samesite branch from 5209bcf to 8b5c0d8 Oct 16, 2019
* (Keys must be lowercase).
* @return static
*/
public static function create(string $name, $value, array $options = [])

This comment has been minimized.

Copy link
@markstory

markstory Oct 18, 2019

Member

👍 This is a bit simpler than the constructor for basic usage.

src/Http/Cookie/Cookie.php Show resolved Hide resolved
}
if (PHP_VERSION_ID >= 70300) {
setcookie($name, $value, $cookie['options']);

This comment has been minimized.

Copy link
@markstory

markstory Oct 18, 2019

Member

Should we raise an exception if people try and use samesite cookies when they are on PHP<7.3?

This comment has been minimized.

Copy link
@ADmad

ADmad Oct 18, 2019

Author Member

I'll add an exception.

This comment has been minimized.

Copy link
@garas

garas Oct 18, 2019

Member

Could it generate header with Cookie::toHeaderValue() and send it using header() instead?

Exceptions might be missed if development and production uses different PHP version.

This comment has been minimized.

Copy link
@ADmad

ADmad Oct 18, 2019

Author Member

I guess that's an option too.

This comment has been minimized.

Copy link
@ADmad

ADmad Oct 19, 2019

Author Member

Directly switching to header() for PHP < 7.3 would making testing a pain and we would need different set of tests for different PHP versions.

Instead we can use the namespaced Cake\Http\setcookie() currently only used in tests in ResponseEmitter too which can internally either call PHP's setcookie() or header().

Scratch that, I have another idea.

This comment has been minimized.

Copy link
@garas

garas Oct 19, 2019

Member

I would use same header() always independently of PHP.

@ADmad

This comment has been minimized.

Copy link
Member Author

ADmad commented Oct 18, 2019

There are few other issues which I want to address:

  1. One would expect Cake\Http\Response::getCookie() and Cake\Http\Client\Response::getCookie() to return similar data but they don't. The former returns an array representation of cookie with keys name, value, path etc. while the latter just returns the cookie value.
  2. Cake\Http\Client\Response::getCookieData() returns cookie data array similar to Cake\Http\Response::getCookie() but there is a discrepancy in key names: expires vs expire, httponly vs httpOnly.
    Also for Http\Response the expire key is timestamp while for Http\Client\Response it's a formatted string.
    Is this comment This method is compatible with older client code that expects date strings instead of timestamps. for Http\Cllient\Reponse::convertCookieToArray() still relevant?

So my proposal is:

  1. Make getCookie() method of both response classes return Cookie instance instead of array. Using the object methods is lot more convenient than dealing using array keys which can easily have typos (especially considering the existing discrepancy in key names).
  2. Add a CookieInterface::toArray() method which returns array of form
    ['name' => '', 'value' => '', 'options' => ['path' => '', ...]]. The convertCookieToArray() methods of both response classes could then be dropped.

While changing return value of getCookie() is not BC and would cause inconvenience to those upgrading from 3.x, the API clean up will be worth it in the long run.

@ADmad ADmad force-pushed the 4.x-cookie-samesite branch from c233156 to 4743b95 Oct 18, 2019
setcookie() expects expires to be int.
@ADmad ADmad force-pushed the 4.x-cookie-samesite branch from 616c60c to 318ef33 Oct 18, 2019
@ADmad

This comment has been minimized.

Copy link
Member Author

ADmad commented Oct 18, 2019

Either I have made a silly mistake which I cannot spot or PHP is trolling me. Using the alternate setcookie() signature with 3rd argument as array for PHP 7.3+ throws error:

ArgumentCountError: Too few arguments to function Cake\Http\setcookie(), 3 passed in /home/runner/work/cakephp/cakephp/src/Http/ResponseEmitter.php on line 228 and at least 5 expected

@garas

This comment has been minimized.

@ADmad

This comment has been minimized.

Copy link
Member Author

ADmad commented Oct 18, 2019

Thanks, I got the hint when i saw Cake\Http\setcookie() as function name instead of setcookie() but it's too late in the day so I had decided to go troll hunting tomorrow 😛.

Also update option key name.
@ADmad ADmad force-pushed the 4.x-cookie-samesite branch from 4c0367b to 4b26aea Oct 19, 2019
ADmad added 3 commits Oct 19, 2019
@ADmad

This comment has been minimized.

Copy link
Member Author

ADmad commented Oct 19, 2019

I added a new static method Cookie::createFromHeaderString() to avoid the duplicate string parsing code in ResponseEmitter and Client/Response.

Only dealing with the SameSite for PHP < 7.3 and changing the return type of getCookie() methods of the two response classes remains.

@ADmad

This comment has been minimized.

Copy link
Member Author

ADmad commented Oct 19, 2019

I am a bit sure whether none should also be allowed as a valid value for SameSite. OWASP only mentions Lax and Strict as possible values but I have seen various articles which seem to indicate that chrome can have none too.

I am not going to add None as allowed value, it's not listed as a valid value in the RFC either.

Another question is should we default to Lax?

@ADmad ADmad force-pushed the 4.x-cookie-samesite branch from a55a6a3 to b45c664 Oct 19, 2019
@ADmad ADmad force-pushed the 4.x-cookie-samesite branch from bab2150 to 43542ae Oct 19, 2019
src/Http/Response.php Show resolved Hide resolved
return [
'name' => $this->name,
'value' => $this->getScalarValue(),
'options' => $this->getOptions(),

This comment has been minimized.

Copy link
@markstory

markstory Oct 19, 2019

Member

If these options were merged into the top level array, we would have fewer compatibility issues elsewhere.

This comment has been minimized.

Copy link
@ADmad

ADmad Oct 19, 2019

Author Member

I have changed back to flat array without the nesting for options key. But I really want to keep the expire to expires rename for consistency.

This comment has been minimized.

Copy link
@markstory

markstory Oct 20, 2019

Member

Would outputting both be an option?

This comment has been minimized.

Copy link
@ADmad

ADmad Oct 21, 2019

Author Member

If we output old keys too then even those staring fresh with 4.0 could inadvertently use the old keys and we wouldn't be able to do this clean up even in 5.0.

$key = $part;
$value = true;
}
header('Set-Cookie: ' . $cookie);

This comment has been minimized.

Copy link
@markstory

markstory Oct 19, 2019

Member

Have you checked that this will play nicely with sessions and setcookie()?

How will this work in integration tests?

This comment has been minimized.

Copy link
@garas

garas Oct 19, 2019

Member

You need to pass in false as the second argument to allow multiple Set-Cookie headers.

This comment has been minimized.

Copy link
@ADmad

ADmad Oct 19, 2019

Author Member

Have you checked that this will play nicely with sessions and setcookie()?

IDK, I just followed @garas's suggestion. If it doesn't work then users of PHP 7.2 just won't be able to use SameSite.

How will this work in integration tests?

There are no test failures :)

You need to pass in false as the second argument to allow multiple Set-Cookie headers.

Fixed

This comment has been minimized.

Copy link
@ADmad

ADmad Oct 19, 2019

Author Member

Seems the SameSite restriction for session cookie can be handled this way
https://github.com/delight-im/PHP-Cookie/blob/b3ca3233dd1e1e91efb259b9b717a1c941d67ecc/src/Session.php#L146.

This comment has been minimized.

Copy link
@markstory

markstory Oct 20, 2019

Member

I'm not concerned about SameSite session cookies, more that using the set-cookie header can cause sessions to not work. This commit 7ebc261 has a removal in the ResponseTransformer that was a workaround for session cookies not being emitted.

This comment has been minimized.

Copy link
@ADmad

ADmad Oct 20, 2019

Author Member

Okay, I really don't want to go down that rabbit hole for now. I'll revert the use of header() and simply throw exception if samesite is set for php 7.2.

We can look into supporting samesite attribute for PHP 7.2 at a later date.

tests/TestCase/Http/Client/ResponseTest.php Outdated Show resolved Hide resolved
ADmad added 3 commits Oct 19, 2019
Doing so has caused problems with setting session cookie in the past.
Instead use a hack to set "SameSite" attribute for PHP 7.2.
if ($sameSite !== null) {
// Temporary hack for PHP 7.2 to set "SameSite" attribute
// https://stackoverflow.com/questions/39750906/php-setcookie-samesite-strict
$path .= '; samesite=' . $sameSite;

This comment has been minimized.

Copy link
@markstory

markstory Oct 21, 2019

Member

Neat solution.

@ADmad

This comment has been minimized.

Copy link
Member Author

ADmad commented Oct 21, 2019

This is ready for final review.

@markstory The only non BC change is that of key names in array returned by Http\Response::getCookie(). But I think the use cases for reading back cookies meant to be sent with response would be pretty rare. Usually those cookies are just set and emitted by server.

@ADmad

This comment has been minimized.

Copy link
Member Author

ADmad commented Oct 21, 2019

The Static Analysis job failure is unrelated and fixed on 4.x.

return [
'name' => $this->name,
'value' => $this->getScalarValue(),
] + $this->getOptions();

This comment has been minimized.

Copy link
@markstory
$this->assertSame('/dir/', $cookie['path'], 'session path.');
$this->assertTrue($cookie['secure'], 'cookie security flag missing');
$this->assertTrue($cookie['httpOnly'], 'cookie httpOnly flag missing');
$this->assertTrue($cookie['httponly'], 'cookie httpOnly flag missing');

This comment has been minimized.

Copy link
@markstory

markstory Oct 22, 2019

Member

These key renames should be added to the migration guide. I don't think rector is going to help us here as we can't easily discern valid replacements.

This comment has been minimized.

Copy link
@ADmad

ADmad Oct 22, 2019

Author Member

I'll make a PR for the migration guide.

ADmad added a commit to cakephp/docs that referenced this pull request Oct 22, 2019
markstory added a commit to cakephp/docs that referenced this pull request Oct 23, 2019
@markstory markstory merged commit 42f7c43 into 4.x Oct 23, 2019
11 of 13 checks passed
11 of 13 checks passed
PHP 7.2 & sqlite
Details
PHP 7.2 & mysql
Details
PHP 7.2 & postgres
Details
PHP 7.3 & sqlite
Details
PHP 7.3 & mysql
Details
PHP 7.3 & postgres
Details
Coding Standard
Details
Static Analysis Static Analysis
Details
coverage/coveralls Coverage decreased (-0.3%) to 92.957%
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
stickler-ci Pull request targets an ignored branch
Details
@markstory markstory deleted the 4.x-cookie-samesite branch Oct 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.