diff --git a/classes/Kohana/Cookie.php b/classes/Kohana/Cookie.php index fe4cc490d..8ee3d239e 100644 --- a/classes/Kohana/Cookie.php +++ b/classes/Kohana/Cookie.php @@ -78,7 +78,7 @@ public static function get($key, $default = NULL) } // The cookie signature is invalid, delete it - Cookie::delete($key); + static::delete($key); } return $default; @@ -88,33 +88,38 @@ public static function get($key, $default = NULL) * Sets a signed cookie. Note that all cookie values must be strings and no * automatic serialization will be performed! * + * [!!] By default, Cookie::$expiration is 0 - if you skip/pass NULL for the optional + * lifetime argument your cookies will expire immediately unless you have separately + * configured Cookie::$expiration. + * + * * // Set the "theme" cookie * Cookie::set('theme', 'red'); * * @param string $name name of cookie * @param string $value value of cookie - * @param integer $expiration lifetime in seconds + * @param integer $lifetime lifetime in seconds * @return boolean * @uses Cookie::salt */ - public static function set($name, $value, $expiration = NULL) + public static function set($name, $value, $lifetime = NULL) { - if ($expiration === NULL) + if ($lifetime === NULL) { // Use the default expiration - $expiration = Cookie::$expiration; + $lifetime = Cookie::$expiration; } - if ($expiration !== 0) + if ($lifetime !== 0) { // The expiration is expected to be a UNIX timestamp - $expiration += time(); + $lifetime += static::_time(); } // Add the salt to the cookie value $value = Cookie::salt($name, $value).'~'.$value; - return setcookie($name, $value, $expiration, Cookie::$path, Cookie::$domain, Cookie::$secure, Cookie::$httponly); + return static::_setcookie($name, $value, $lifetime, Cookie::$path, Cookie::$domain, Cookie::$secure, Cookie::$httponly); } /** @@ -131,7 +136,7 @@ public static function delete($name) unset($_COOKIE[$name]); // Nullify the cookie and make it expire - return setcookie($name, NULL, -86400, Cookie::$path, Cookie::$domain, Cookie::$secure, Cookie::$httponly); + return static::_setcookie($name, NULL, -86400, Cookie::$path, Cookie::$domain, Cookie::$secure, Cookie::$httponly); } /** @@ -139,8 +144,10 @@ public static function delete($name) * * $salt = Cookie::salt('theme', 'red'); * - * @param string $name name of cookie - * @param string $value value of cookie + * @param string $name name of cookie + * @param string $value value of cookie + * + * @throws Kohana_Exception if Cookie::$salt is not configured * @return string */ public static function salt($name, $value) @@ -157,4 +164,35 @@ public static function salt($name, $value) return sha1($agent.$name.$value.Cookie::$salt); } + /** + * Proxy for the native setcookie function - to allow mocking in unit tests so that they do not fail when headers + * have been sent. + * + * @param string $name + * @param string $value + * @param integer $expire + * @param string $path + * @param string $domain + * @param boolean $secure + * @param boolean $httponly + * + * @return bool + * @see setcookie + */ + protected static function _setcookie($name, $value, $expire, $path, $domain, $secure, $httponly) + { + return setcookie($name, $value, $expire, $path, $domain, $secure, $httponly); + } + + /** + * Proxy for the native time function - to allow mocking of time-related logic in unit tests + * + * @return int + * @see time + */ + protected static function _time() + { + return time(); + } + } diff --git a/tests/kohana/CookieTest.php b/tests/kohana/CookieTest.php index 9c1fe400c..fd80c6dc1 100644 --- a/tests/kohana/CookieTest.php +++ b/tests/kohana/CookieTest.php @@ -11,13 +11,15 @@ * @category Tests * @author Kohana Team * @author Jeremy Bush - * @copyright (c) 2008-2012 Kohana Team + * @author Andrew Coulton + * @copyright (c) 2008-2014 Kohana Team * @license http://kohanaframework.org/license */ class Kohana_CookieTest extends Unittest_TestCase { + const UNIX_TIMESTAMP = 1411040141; + const COOKIE_EXPIRATION = 60; - protected $_default_salt = 'AdaoidadnA£ASDNadnaoiwdnawd'; /** * Sets up the environment */ @@ -26,152 +28,297 @@ public function setUp() // @codingStandardsIgnoreEnd { parent::setUp(); + Kohana_CookieTest_TestableCookie::$_mock_cookies_set = array(); - Cookie::$salt = $this->_default_salt; + $this->setEnvironment(array( + 'Cookie::$salt' => 'some-random-salt', + 'HTTP_USER_AGENT' => 'cli' + )); } /** - * Tears down the environment + * Tests that cookies are set with the global path, domain, etc options. + * + * @covers Cookie::set */ - // @codingStandardsIgnoreStart - public function tearDown() - // @codingStandardsIgnoreEnd + public function test_set_creates_cookie_with_configured_cookie_options() { - parent::tearDown(); + $this->setEnvironment(array( + 'Cookie::$path' => '/path', + 'Cookie::$domain' => 'my.domain', + 'Cookie::$secure' => TRUE, + 'Cookie::$httponly' => FALSE, + )); - Cookie::$salt = NULL; + Kohana_CookieTest_TestableCookie::set('cookie', 'value'); + + $this->assertSetCookieWith(array( + 'path' => '/path', + 'domain' => 'my.domain', + 'secure' => TRUE, + 'httponly' => FALSE + )); } /** - * Provides test data for test_set() + * Provider for test_set_calculates_expiry_from_lifetime * - * @return array + * @return array of $lifetime, $expect_expiry */ - public function provider_set() + public function provider_set_calculates_expiry_from_lifetime() { return array( - array('foo', 'bar', NULL, TRUE), - array('foo', 'bar', 10, TRUE), + array(NULL, self::COOKIE_EXPIRATION + self::UNIX_TIMESTAMP), + array(0, 0), + array(10, 10 + self::UNIX_TIMESTAMP), ); } /** - * Tests cookie::set() + * @param int $expiration + * @param int $expect_expiry * - * @test - * @dataProvider provider_set - * @covers cookie::set - * @param mixed $key key to use - * @param mixed $value value to set - * @param mixed $exp exp to set - * @param boolean $expected Output for cookie::set() - */ - public function test_set($key, $value, $exp, $expected) - { - if (headers_sent()) { - $this->markTestSkipped('Cannot test setting cookies as headers have already been sent'); - } + * @dataProvider provider_set_calculates_expiry_from_lifetime + * @covers Cookie::set + */ + public function test_set_calculates_expiry_from_lifetime($expiration, $expect_expiry) + { + $this->setEnvironment(array('Cookie::$expiration' => self::COOKIE_EXPIRATION)); + Kohana_CookieTest_TestableCookie::set('foo', 'bar', $expiration); + $this->assertSetCookieWith(array('expire' => $expect_expiry)); + } + + /** + * @covers Cookie::get + */ + public function test_get_returns_default_if_cookie_missing() + { + unset($_COOKIE['missing_cookie']); + $this->assertEquals('default', Cookie::get('missing_cookie', 'default')); + } - $this->assertSame($expected, cookie::set($key, $value, $exp)); + /** + * @covers Cookie::get + */ + public function test_get_returns_value_if_cookie_present_and_signed() + { + Kohana_CookieTest_TestableCookie::set('cookie', 'value'); + $cookie = Kohana_CookieTest_TestableCookie::$_mock_cookies_set[0]; + $_COOKIE[$cookie['name']] = $cookie['value']; + $this->assertEquals('value', Cookie::get('cookie', 'default')); } /** - * Provides test data for test_get() + * Provider for test_get_returns_default_without_deleting_if_cookie_unsigned * * @return array */ - public function provider_get() + public function provider_get_returns_default_without_deleting_if_cookie_unsigned() { - // setUp is called after the provider so we need to specify a - // salt here in order to use it in the provider - Cookie::$salt = $this->_default_salt; - return array( - array('foo', Cookie::salt('foo', 'bar').'~bar', 'bar'), - array('bar', Cookie::salt('foo', 'bar').'~bar', NULL), - array(NULL, Cookie::salt('foo', 'bar').'~bar', NULL), + array('unsalted'), + array('un~salted'), ); } /** - * Tests cookie::set() + * Verifies that unsigned cookies are not available to the kohana application, but are not affected for other + * consumers. + * + * @param string $unsigned_value * - * @test - * @dataProvider provider_get - * @covers cookie::get - * @param mixed $key key to use - * @param mixed $value value to set - * @param boolean $expected Output for cookie::get() + * @dataProvider provider_get_returns_default_without_deleting_if_cookie_unsigned + * @covers Cookie::get */ - public function test_get($key, $value, $expected) + public function test_get_returns_default_without_deleting_if_cookie_unsigned($unsigned_value) { - if (headers_sent()) { - $this->markTestSkipped('Cannot test setting cookies as headers have already been sent'); - } + $_COOKIE['cookie'] = $unsigned_value; + $this->assertEquals('default', Kohana_CookieTest_TestableCookie::get('cookie', 'default')); + $this->assertEquals($unsigned_value, $_COOKIE['cookie'], '$_COOKIE not affected'); + $this->assertEmpty(Kohana_CookieTest_TestableCookie::$_mock_cookies_set, 'No cookies set or changed'); + } - // Force $_COOKIE - if ($key !== NULL) - { - $_COOKIE[$key] = $value; - } + /** + * If a cookie looks like a signed cookie but the signature no longer matches, it should be deleted. + * + * @covers Cookie::get + */ + public function test_get_returns_default_and_deletes_tampered_signed_cookie() + { + $_COOKIE['cookie'] = Cookie::salt('cookie', 'value').'~tampered'; + $this->assertEquals('default', Kohana_CookieTest_TestableCookie::get('cookie', 'default')); + $this->assertDeletedCookie('cookie'); + } + + /** + * @covers Cookie::delete + */ + public function test_delete_removes_cookie_from_globals_and_expires_cookie() + { + $_COOKIE['cookie'] = Cookie::salt('cookie', 'value').'~tampered'; + $this->assertTrue(Kohana_CookieTest_TestableCookie::delete('cookie')); + $this->assertDeletedCookie('cookie'); + } + + /** + * @covers Cookie::delete + * @link http://dev.kohanaframework.org/issues/3501 + * @link http://dev.kohanaframework.org/issues/3020 + */ + public function test_delete_does_not_require_configured_salt() + { + Cookie::$salt = NULL; + $this->assertTrue(Kohana_CookieTest_TestableCookie::delete('cookie')); + $this->assertDeletedCookie('cookie'); + } + + /** + * @covers Cookie::salt + * @expectedException Kohana_Exception + */ + public function test_salt_throws_with_no_configured_salt() + { + Cookie::$salt = NULL; + Cookie::salt('key', 'value'); + } - $this->assertSame($expected, cookie::get($key)); + /** + * @covers Cookie::salt + */ + public function test_salt_creates_same_hash_for_same_values_and_state() + { + $name = 'cookie'; + $value = 'value'; + $this->assertEquals(Cookie::salt($name, $value), Cookie::salt($name, $value)); } /** - * Provides test data for test_delete() + * Provider for test_salt_creates_different_hash_for_different_data * * @return array */ - public function provider_delete() + public function provider_salt_creates_different_hash_for_different_data() { return array( - array('foo', TRUE), + array(array('name' => 'foo', 'value' => 'bar', 'salt' => 'our-salt', 'user-agent' => 'Chrome'), array('name' => 'changed')), + array(array('name' => 'foo', 'value' => 'bar', 'salt' => 'our-salt', 'user-agent' => 'Chrome'), array('value' => 'changed')), + array(array('name' => 'foo', 'value' => 'bar', 'salt' => 'our-salt', 'user-agent' => 'Chrome'), array('salt' => 'changed-salt')), + array(array('name' => 'foo', 'value' => 'bar', 'salt' => 'our-salt', 'user-agent' => 'Chrome'), array('user-agent' => 'Firefox')), + array(array('name' => 'foo', 'value' => 'bar', 'salt' => 'our-salt', 'user-agent' => 'Chrome'), array('user-agent' => NULL)), ); } /** - * Tests cookie::delete() + * @param array $first_args + * @param array $changed_args * - * @test - * @dataProvider provider_delete - * @covers cookie::delete - * @param mixed $key key to use - * @param boolean $expected Output for cookie::delete() + * @dataProvider provider_salt_creates_different_hash_for_different_data + * @covers Cookie::salt */ - public function test_delete($key, $expected) + public function test_salt_creates_different_hash_for_different_data($first_args, $changed_args) { - if (headers_sent()) { - $this->markTestSkipped('Cannot test setting cookies as headers have already been sent'); + $second_args = array_merge($first_args, $changed_args); + $hashes = array(); + foreach (array($first_args, $second_args) as $args) + { + Cookie::$salt = $args['salt']; + $this->set_or_remove_http_user_agent($args['user-agent']); + + $hashes[] = Cookie::salt($args['name'], $args['value']); } - $this->assertSame($expected, cookie::delete($key)); + $this->assertNotEquals($hashes[0], $hashes[1]); } /** - * Provides test data for test_salt() + * Verify that a cookie was deleted from the global $_COOKIE array, and that a setcookie call was made to remove it + * from the client. * - * @return array + * @param string $name */ - public function provider_salt() + // @codingStandardsIgnoreStart + protected function assertDeletedCookie($name) + // @codingStandardsIgnoreEnd { - return array( - array('foo', 'bar', 'b5773a6255d1deefc23f9f69bcc40fdc998e5802'), - ); + $this->assertArrayNotHasKey($name, $_COOKIE); + $this->assertSetCookieWith(array( + 'name' => $name, + 'value' => NULL, + 'expire' => -86400, + 'path' => Cookie::$path, + 'domain' => Cookie::$domain, + 'secure' => Cookie::$secure, + 'httponly' => Cookie::$httponly + )); } /** - * Tests cookie::salt() + * Verify that there was a single call to setcookie including the provided named arguments * - * @test - * @dataProvider provider_salt - * @covers cookie::salt - * @param mixed $key key to use - * @param mixed $value value to salt with - * @param boolean $expected Output for cookie::delete() + * @param array $expected + */ + // @codingStandardsIgnoreStart + protected function assertSetCookieWith($expected) + // @codingStandardsIgnoreEnd + { + $this->assertCount(1, Kohana_CookieTest_TestableCookie::$_mock_cookies_set); + $relevant_values = array_intersect_key(Kohana_CookieTest_TestableCookie::$_mock_cookies_set[0], $expected); + $this->assertEquals($expected, $relevant_values); + } + + /** + * Configure the $_SERVER[HTTP_USER_AGENT] environment variable for the test + * + * @param string $user_agent + */ + protected function set_or_remove_http_user_agent($user_agent) + { + if ($user_agent === NULL) + { + unset($_SERVER['HTTP_USER_AGENT']); + } + else + { + $_SERVER['HTTP_USER_AGENT'] = $user_agent; + } + } +} + +/** + * Class Kohana_CookieTest_TestableCookie wraps the cookie class to mock out the actual setcookie and time calls for + * unit testing. + */ +class Kohana_CookieTest_TestableCookie extends Cookie { + + /** + * @var array setcookie calls that were made */ - public function test_salt($key, $value, $expected) + public static $_mock_cookies_set = array(); + + /** + * {@inheritdoc} + */ + protected static function _setcookie($name, $value, $expire, $path, $domain, $secure, $httponly) { - $this->assertSame($expected, cookie::salt($key, $value)); + self::$_mock_cookies_set[] = array( + 'name' => $name, + 'value' => $value, + 'expire' => $expire, + 'path' => $path, + 'domain' => $domain, + 'secure' => $secure, + 'httponly' => $httponly + ); + + return TRUE; } + + /** + * @return int + */ + protected static function _time() + { + return Kohana_CookieTest::UNIX_TIMESTAMP; + } + }