From e981fab4477afc50179522627f0f01009c8cddad Mon Sep 17 00:00:00 2001 From: Josh Cunningham Date: Tue, 19 Nov 2019 14:34:05 -0800 Subject: [PATCH] Add SameSite handling to CookieStore --- src/Auth0.php | 5 +- src/Store/CookieStore.php | 128 ++++++++++++++-------- tests/Store/CookieStoreTest.php | 182 ++++++++++++++++++++++++-------- 3 files changed, 226 insertions(+), 89 deletions(-) diff --git a/src/Auth0.php b/src/Auth0.php index 72b62d2b..53a431ca 100644 --- a/src/Auth0.php +++ b/src/Auth0.php @@ -302,7 +302,10 @@ public function __construct(array $config) $transientStore = $config['transient_store'] ?? null; if (! $transientStore instanceof StoreInterface) { - $transientStore = new CookieStore(); + $transientStore = new CookieStore([ + 'legacy_samesite_none' => $config['legacy_samesite_none_cookie'] ?? null, + 'samesite' => 'form_post' === $this->responseMode ? 'None' : 'Lax', + ]); } $this->transientHandler = new TransientStoreHandler( $transientStore ); diff --git a/src/Store/CookieStore.php b/src/Store/CookieStore.php index 9bd0a0da..85fdc3d8 100644 --- a/src/Store/CookieStore.php +++ b/src/Store/CookieStore.php @@ -13,36 +13,44 @@ class CookieStore implements StoreInterface const BASE_NAME = 'auth0_'; /** - * Cookie base name, configurable on instantiation. + * Cookie base name. + * Use config key 'base_name' to set this during instantiation. * * @var string */ protected $baseName; /** - * Cookie expiration, configurable on instantiation. + * Cookie expiration length. + * This will be added to current time or $this->now to set cookie expiration time. + * Use config key 'expiration' to set this during instantiation. * * @var integer */ protected $expiration; /** - * SameSite attribute for all cookies. + * SameSite attribute for all cookies set with class instance. + * Must be one of None, Strict, Lax (default is no SameSite attribute). + * Use config key 'samesite' to set this during instantiation. * - * @var integer + * @var null|string */ protected $sameSite; /** * Time to use as "now" in expiration calculations. - * Useful for testing. + * Used primarily for testing. + * Use config key 'now' to set this during instantiation. * * @var null|integer */ protected $now; /** - * Support legacy browsers for SameSite=None + * Support legacy browsers for SameSite=None. + * This will set/get/delete a fallback cookie with no SameSite attribute if $this->sameSite is None. + * Use config key 'legacy_samesite_none' to set this during instantiation. * * @var boolean */ @@ -51,20 +59,19 @@ class CookieStore implements StoreInterface /** * CookieStore constructor. * - * @param array $options Set cookie options. + * @param array $options Cookie options. See class properties above for keys and types allowed. */ public function __construct(array $options = []) { $this->baseName = $options['base_name'] ?? self::BASE_NAME; $this->expiration = $options['expiration'] ?? 600; - $sameSite = 'Lax'; - if (isset($options['samesite']) && is_string($options['samesite'])) { + if (! empty($options['samesite']) && is_string($options['samesite'])) { $sameSite = ucfirst($options['samesite']); - } - if (in_array($sameSite, ['None', 'Strict', 'Lax'])) { - $this->sameSite = $sameSite; + if (in_array($sameSite, ['None', 'Strict', 'Lax'])) { + $this->sameSite = $sameSite; + } } $this->now = $options['now'] ?? null; @@ -84,7 +91,17 @@ public function set($key, $value) { $key_name = $this->getCookieName($key); $_COOKIE[$key_name] = $value; - $this->setCookie($key_name, $value); + + if ($this->sameSite) { + $this->setCookieHeader($key_name, $value, $this->getExpTimecode()); + } else { + $this->setCookie($key_name, $value, $this->getExpTimecode()); + } + + if ($this->legacySameSiteNone && 'None' === $this->sameSite) { + $_COOKIE['_'.$key_name] = $value; + $this->setCookie('_'.$key_name, $value, $this->getExpTimecode()); + } } /** @@ -99,7 +116,14 @@ public function set($key, $value) public function get($key, $default = null) { $key_name = $this->getCookieName($key); - return $_COOKIE[$key_name] ?? $default; + $value = $default; + + // If we're handling legacy browsers, check for fallback value first. + if ($this->legacySameSiteNone) { + $value = $_COOKIE['_'.$key_name] ?? $value; + } + + return $_COOKIE[$key_name] ?? $value; } /** @@ -113,38 +137,43 @@ public function delete($key) { $key_name = $this->getCookieName($key); unset($_COOKIE[$key_name]); - $this->setCookie( $key_name ); + $this->setCookie( $key_name, '', 0 ); + + if ($this->legacySameSiteNone) { + unset($_COOKIE['_'.$key_name]); + $this->setCookie( '_'.$key_name, '', 0 ); + } } /** - * @param $name - * @param $value - * @param $handleSameSite + * Build the header to use when setting SameSite cookies. + * Core setcookie() function does not handle SameSite before PHP 7.3. + * + * @param string $name Cookie name. + * @param string $value Cookie value. + * @param integer $expire Cookie expiration timecode. * * @return string */ - public function getSetCookieHeader(string $name, string $value, $handleSameSite = true) : string + public function getSameSiteCookieHeader(string $name, string $value, int $expire) : string { $date = new \Datetime(); - $date->setTimestamp($this->getExpTimecode()); - $date->setTimezone(new \DateTimeZone('GMT')); + $date->setTimestamp($expire) + ->setTimezone(new \DateTimeZone('GMT')); - $header = sprintf( - '%s=%s; path=/; expires=%s; HttpOnly', + return sprintf( + 'Set-Cookie: %s=%s; path=/; expires=%s; HttpOnly; SameSite=%s%s', $name, $value, - $date->format(\DateTime::COOKIE) + $date->format($date::COOKIE), + $this->sameSite, + 'None' === $this->sameSite ? '; Secure' : '' ); - - if ($handleSameSite) { - $header .= '; SameSite=' . $this->sameSite; - $header .= 'None' === $this->sameSite ? '; Secure' : ''; - } - - return $header; } /** + * Get cookie expiration timecode to use. + * * @return integer */ private function getExpTimecode() : int @@ -153,26 +182,35 @@ private function getExpTimecode() : int } /** - * Set or delete a cookie. + * Wrapper around PHP core setcookie() function to assist with testing. * - * @param string $name Cookie name to set. - * @param string $value Cookie value. + * @param string $name Complete cookie name to set. + * @param string $value Value of the cookie to set. + * @param integer $expire Expiration time in Unix timecode format. * * @return boolean + * + * @codeCoverageIgnore */ - protected function setCookie(string $name, string $value = '') : bool + protected function setCookie(string $name, string $value, int $expire) : bool { - if (! $this->sameSite) { - return setcookie($name, $value, $this->getExpTimecode(), '/', '', false, true); - } - - header('Set-Cookie: '.$this->getSetCookieHeader($name, $value), false); - - if ($this->legacySameSiteNone && 'None' === $this->sameSite) { - header('Set-Cookie: _'.$this->getSetCookieHeader($name, $value, false), false); - } + return setcookie($name, $value, $expire, '/', '', false, true); + } - return true; + /** + * Wrapper around PHP core header() function to assist with testing. + * + * @param string $name Complete cookie name to set. + * @param string $value Value of the cookie to set. + * @param integer $expire Expiration time in Unix timecode format. + * + * @return void + * + * @codeCoverageIgnore + */ + protected function setCookieHeader(string $name, string $value, int $expire) : void + { + header($this->getSameSiteCookieHeader($name, $value, $expire), false); } /** diff --git a/tests/Store/CookieStoreTest.php b/tests/Store/CookieStoreTest.php index 556f777f..c490fd8b 100644 --- a/tests/Store/CookieStoreTest.php +++ b/tests/Store/CookieStoreTest.php @@ -12,35 +12,40 @@ class CookieStoreTest extends TestCase { - /** - * Mock CookieStore instance to skip setcookie() call. - * - * @var CookieStore - */ - public static $mockStore; + private static $mockSpyCookie; - /** - * Mock spy for calls to setCookie. - * - * @var AnyInvokedCount - */ - public static $mockSpy; + private static $mockSpyHeader; /** - * Run before all tests in this suite. + * Run after each test in this suite. */ - public function setUp() + public function tearDown() { - self::$mockStore = $this->getMockBuilder(CookieStore::class)->setMethods(['setCookie'])->getMock(); - self::$mockStore->expects(self::$mockSpy = $this->any())->method('setCookie')->willReturn(true); + $_COOKIE = []; + self::$mockSpyCookie = null; + self::$mockSpyHeader = null; } /** - * Run after each test in this suite. + * @param array $args + * + * @return \PHPUnit\Framework\MockObject\MockObject|CookieStore */ - public function tearDown() + public function getMock(array $args = []) { - $_COOKIE = []; + $mockStore = $this->getMockBuilder(CookieStore::class) + ->setConstructorArgs([$args]) + ->setMethods(['setCookie','setCookieHeader']) + ->getMock(); + + $mockStore->expects(self::$mockSpyCookie = $this->any()) + ->method('setCookie') + ->willReturn(true); + + $mockStore->expects(self::$mockSpyHeader = $this->any()) + ->method('setCookieHeader'); + + return $mockStore; } public function testGetCookieName() @@ -53,75 +58,166 @@ public function testCustomBaseName() { $store = new CookieStore(['base_name' => 'custom_base']); $this->assertEquals('custom_base_test_key', $store->getCookieName('test_key')); + + $store = new CookieStore(['base_name' => 'custom_base_']); + $this->assertEquals('custom_base__test_key', $store->getCookieName('test_key')); + } + + public function testSetNoSameSite() + { + $mockStore = $this->getMock(['now' => 1, 'expiration' => 1]); + $mockStore->set('test_set_key', '__test_set_value__'); + + $this->assertEquals('__test_set_value__', $_COOKIE['auth0__test_set_key']); + $this->assertArrayNotHasKey('_auth0__test_set_key', $_COOKIE); + + $this->assertCount(0, (array) self::$mockSpyHeader->getInvocations()); + $this->assertCount(1, (array) self::$mockSpyCookie->getInvocations()); + + $setCookieParams = self::$mockSpyCookie->getInvocations()[0]->getParameters(); + + $this->assertEquals('auth0__test_set_key', $setCookieParams[0]); + $this->assertEquals('__test_set_value__', $setCookieParams[1]); + $this->assertEquals(2, $setCookieParams[2]); + } + + public function testSetSameSiteNone() + { + $mockStore = $this->getMock(['now' => 10, 'expiration' => 10, 'samesite' => 'None']); + $mockStore->set('test_set_key', '__test_set_value__'); + + $this->assertEquals('__test_set_value__', $_COOKIE['auth0__test_set_key']); + $this->assertEquals('__test_set_value__', $_COOKIE['_auth0__test_set_key']); + + $this->assertCount(1, (array) self::$mockSpyHeader->getInvocations()); + + $setHeaderParams = self::$mockSpyHeader->getInvocations()[0]->getParameters(); + + $this->assertEquals('auth0__test_set_key', $setHeaderParams[0]); + $this->assertEquals('__test_set_value__', $setHeaderParams[1]); + $this->assertEquals(20, $setHeaderParams[2]); + + $this->assertCount(1, (array) self::$mockSpyCookie->getInvocations()); + + $setCookieParams = self::$mockSpyCookie->getInvocations()[0]->getParameters(); + + $this->assertEquals('_auth0__test_set_key', $setCookieParams[0]); + $this->assertEquals('__test_set_value__', $setCookieParams[1]); + $this->assertEquals(20, $setCookieParams[2]); } - public function testSet() + public function testSetSameSiteNoneNoLegacy() { - self::$mockStore->set('test_set_key', '__test_set_value__'); + $mockStore = $this->getMock(['legacy_samesite_none' => false, 'samesite' => 'None']); + $mockStore->set('test_set_key', '__test_set_value__'); $this->assertEquals('__test_set_value__', $_COOKIE['auth0__test_set_key']); - $this->assertCount(1, (array) self::$mockSpy->getInvocations()); + $this->assertArrayNotHasKey('_auth0__test_set_key', $_COOKIE); + $this->assertCount(0, (array) self::$mockSpyCookie->getInvocations()); + $this->assertCount(1, (array) self::$mockSpyHeader->getInvocations()); - $setCookieParams = self::$mockSpy->getInvocations()[0]->getParameters(); + $setCookieParams = self::$mockSpyHeader->getInvocations()[0]->getParameters(); $this->assertEquals('auth0__test_set_key', $setCookieParams[0]); $this->assertEquals('__test_set_value__', $setCookieParams[1]); + $this->assertGreaterThanOrEqual(time() + 600, $setCookieParams[2]); } public function testGet() { - $store = new CookieStore(); - $_COOKIE['auth0__test_get_key'] = '__test_get_value__'; + $store = new CookieStore(); + + $_COOKIE['auth0__test_get_key'] = '__test_get_value__'; + $_COOKIE['_auth0__test_get_key'] = '__test_get_legacy_value__'; $this->assertEquals('__test_get_value__', $store->get('test_get_key')); $this->assertEquals('__test_default_value__', $store->get('test_empty_key', '__test_default_value__')); + + unset($_COOKIE['auth0__test_get_key']); + $this->assertEquals('__test_get_legacy_value__', $store->get('test_get_key')); + } + + public function testGetNoLegacy() + { + $store = new CookieStore(['legacy_samesite_none' => false]); + + $_COOKIE['auth0__test_get_key'] = '__test_get_value__'; + $_COOKIE['_auth0__test_get_key'] = '__test_get_legacy_value__'; + + $this->assertEquals('__test_get_value__', $store->get('test_get_key')); + $this->assertEquals('__test_default_value__', $store->get('test_empty_key', '__test_default_value__')); + + unset($_COOKIE['auth0__test_get_key']); + $this->assertNull($store->get('test_get_key')); } public function testDelete() { - $_COOKIE['auth0__test_delete_key'] = '__test_delete_value__'; + $_COOKIE['auth0__test_delete_key'] = '__test_delete_value__'; + $_COOKIE['_auth0__test_delete_key'] = '__test_delete_value__'; - self::$mockStore->delete('test_delete_key'); + $mockStore = $this->getMock(); + $mockStore->delete('test_delete_key'); - $this->assertNull(self::$mockStore->get('test_delete_key')); + $this->assertNull($mockStore->get('test_delete_key')); $this->assertArrayNotHasKey('auth0__test_delete_key', $_COOKIE); - $this->assertCount(1, (array) self::$mockSpy->getInvocations()); + $this->assertArrayNotHasKey('_auth0__test_delete_key', $_COOKIE); + + $this->assertCount(0, (array) self::$mockSpyHeader->getInvocations()); + $this->assertCount(2, (array) self::$mockSpyCookie->getInvocations()); - $setCookieParams = self::$mockSpy->getInvocations()[0]->getParameters(); + $setCookieParams = self::$mockSpyCookie->getInvocations()[0]->getParameters(); $this->assertEquals('auth0__test_delete_key', $setCookieParams[0]); $this->assertEquals('', $setCookieParams[1]); + $this->assertEquals(0, $setCookieParams[2]); + + $setCookieParams = self::$mockSpyCookie->getInvocations()[1]->getParameters(); + + $this->assertEquals('_auth0__test_delete_key', $setCookieParams[0]); + $this->assertEquals('', $setCookieParams[1]); + $this->assertEquals(0, $setCookieParams[2]); } - public function testGetSetCookieHeaderDefault() + public function testDeleteNoLegacy() { - $store = new CookieStore(['now' => 303943620, 'expiration' => 0]); + $_COOKIE['auth0__test_delete_key'] = '__test_delete_value__'; + $_COOKIE['_auth0__test_delete_key'] = '__test_delete_value__'; - $header = $store->getSetCookieHeader('__test_name_1__', '__test_value_1__'); - $this->assertEquals( - '__test_name_1__=__test_value_1__; path=/; expires=Sunday, 19-Aug-1979 20:47:00 GMT; HttpOnly; SameSite=Lax', - $header - ); + $mockStore = $this->getMock(['legacy_samesite_none' => false]); + $mockStore->delete('test_delete_key'); + + $this->assertNull($mockStore->get('test_delete_key')); + $this->assertArrayNotHasKey('auth0__test_delete_key', $_COOKIE); + $this->assertArrayHasKey('_auth0__test_delete_key', $_COOKIE); + + $this->assertCount(1, (array) self::$mockSpyCookie->getInvocations()); + + $setCookieParams = self::$mockSpyCookie->getInvocations()[0]->getParameters(); + + $this->assertEquals('auth0__test_delete_key', $setCookieParams[0]); + $this->assertEquals('', $setCookieParams[1]); + $this->assertEquals(0, $setCookieParams[2]); } public function testGetSetCookieHeaderStrict() { - $store = new CookieStore(['now' => 303943620, 'expiration' => 0, 'samesite' => 'strict']); + $store = new CookieStore(['now' => 303943620, 'expiration' => 0, 'samesite' => 'lax']); - $header = $store->getSetCookieHeader('__test_name_1__', '__test_value_1__'); + $header = $store->getSameSiteCookieHeader('__test_name_1__', '__test_value_1__', 303943620); $this->assertEquals( - '__test_name_1__=__test_value_1__; path=/; expires=Sunday, 19-Aug-1979 20:47:00 GMT; HttpOnly; SameSite=Strict', + 'Set-Cookie: __test_name_1__=__test_value_1__; path=/; expires=Sunday, 19-Aug-1979 20:47:00 GMT; HttpOnly; SameSite=Lax', $header ); } public function testGetSetCookieHeaderNone() { - $store = new CookieStore(['now' => 303943620, 'expiration' => 0, 'samesite' => 'none']); + $store = new CookieStore(['now' => 303943620, 'expiration' => 0, 'samesite' => 'none']); - $header = $store->getSetCookieHeader('__test_name_1__', '__test_value_1__'); + $header = $store->getSameSiteCookieHeader('__test_name_1__', '__test_value_1__', 303943620); $this->assertEquals( - '__test_name_1__=__test_value_1__; path=/; expires=Sunday, 19-Aug-1979 20:47:00 GMT; HttpOnly; SameSite=None; Secure', + 'Set-Cookie: __test_name_1__=__test_value_1__; path=/; expires=Sunday, 19-Aug-1979 20:47:00 GMT; HttpOnly; SameSite=None; Secure', $header ); }