From c2bc707a4de91f9ca609a170176eb30e3e45cb10 Mon Sep 17 00:00:00 2001 From: Andrew Coulton Date: Sun, 14 Apr 2013 00:07:46 +0100 Subject: [PATCH 1/4] fixed detection of secure cookies received over https BrowserKit's cookie handling only recognises a secure cookie if the cookie option is set and the cookie was set over an https request. The client was not passing the url into the cookiejar update code, causing Cookie::isSecure() to always return false for every cookie. Fixes symfony/symfony#7666 --- src/Symfony/Component/BrowserKit/Client.php | 2 +- src/Symfony/Component/BrowserKit/Tests/ClientTest.php | 9 +++++++++ 2 files changed, 10 insertions(+), 1 deletion(-) diff --git a/src/Symfony/Component/BrowserKit/Client.php b/src/Symfony/Component/BrowserKit/Client.php index c57b9e37a1..8664f31ab0 100644 --- a/src/Symfony/Component/BrowserKit/Client.php +++ b/src/Symfony/Component/BrowserKit/Client.php @@ -266,7 +266,7 @@ abstract class Client $response = $this->filterResponse($this->response); - $this->cookieJar->updateFromResponse($response); + $this->cookieJar->updateFromResponse($response, $uri); $this->redirect = $response->getHeader('Location'); diff --git a/src/Symfony/Component/BrowserKit/Tests/ClientTest.php b/src/Symfony/Component/BrowserKit/Tests/ClientTest.php index e03b73cf0b..49b0c04129 100644 --- a/src/Symfony/Component/BrowserKit/Tests/ClientTest.php +++ b/src/Symfony/Component/BrowserKit/Tests/ClientTest.php @@ -205,6 +205,15 @@ class ClientTest extends \PHPUnit_Framework_TestCase $this->assertEquals(array('foo' => 'bar'), $client->getCookieJar()->allValues('http://www.example.com/foo/foobar'), '->request() updates the CookieJar'); } + public function testRequestSecureCookies() + { + $client = new TestClient(); + $client->setNextResponse(new Response('foo', 200, array('Set-Cookie' => 'foo=bar; path=/; secure'))); + $client->request('GET', 'https://www.example.com/foo/foobar'); + + $this->assertTrue($client->getCookieJar()->get('foo','/','www.example.com')->isSecure()); + } + public function testClick() { if (!class_exists('Symfony\Component\DomCrawler\Crawler')) { From 495d0e366e0d9feea6c4873e567ea919093ff179 Mon Sep 17 00:00:00 2001 From: Fabien Potencier Date: Sat, 20 Apr 2013 19:50:53 +0200 Subject: [PATCH 2/4] [HttpFoundation] fixed empty domain= in Cookie::__toString() --- src/Symfony/Component/HttpFoundation/Cookie.php | 2 +- src/Symfony/Component/HttpFoundation/Tests/CookieTest.php | 5 +++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/Symfony/Component/HttpFoundation/Cookie.php b/src/Symfony/Component/HttpFoundation/Cookie.php index fe3a4cff42..43bf87dc23 100644 --- a/src/Symfony/Component/HttpFoundation/Cookie.php +++ b/src/Symfony/Component/HttpFoundation/Cookie.php @@ -95,7 +95,7 @@ class Cookie $str .= '; path='.$this->path; } - if (null !== $this->getDomain()) { + if ($this->getDomain()) { $str .= '; domain='.$this->getDomain(); } diff --git a/src/Symfony/Component/HttpFoundation/Tests/CookieTest.php b/src/Symfony/Component/HttpFoundation/Tests/CookieTest.php index 6a9948d62a..bfedd27fe1 100644 --- a/src/Symfony/Component/HttpFoundation/Tests/CookieTest.php +++ b/src/Symfony/Component/HttpFoundation/Tests/CookieTest.php @@ -117,11 +117,12 @@ class CookieTest extends \PHPUnit_Framework_TestCase public function testToString() { $cookie = new Cookie('foo', 'bar', strtotime('Fri, 20-May-2011 15:25:52 GMT'), '/', '.myfoodomain.com', true); - $this->assertEquals('foo=bar; expires=Fri, 20-May-2011 15:25:52 GMT; domain=.myfoodomain.com; secure; httponly', $cookie->__toString(), '->__toString() returns string representation of the cookie'); $cookie = new Cookie('foo', null, 1, '/admin/', '.myfoodomain.com'); - $this->assertEquals('foo=deleted; expires=' . gmdate("D, d-M-Y H:i:s T", time()-31536001) . '; path=/admin/; domain=.myfoodomain.com; httponly', $cookie->__toString(), '->__toString() returns string representation of a cleared cookie if value is NULL'); + + $cookie = new Cookie('foo', 'bar', 0, '/', ''); + $this->assertEquals('foo=bar; httponly', $cookie->__toString()); } } From c884151e1656d382507c4a682b605d24d14b4266 Mon Sep 17 00:00:00 2001 From: Fabien Potencier Date: Sat, 20 Apr 2013 20:04:45 +0200 Subject: [PATCH 3/4] [BrowserKit] removed dead code --- src/Symfony/Component/BrowserKit/Cookie.php | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/Symfony/Component/BrowserKit/Cookie.php b/src/Symfony/Component/BrowserKit/Cookie.php index 6e1cdbf3c2..9feeb06dc9 100644 --- a/src/Symfony/Component/BrowserKit/Cookie.php +++ b/src/Symfony/Component/BrowserKit/Cookie.php @@ -145,10 +145,9 @@ class Cookie if ((false === $urlParts = parse_url($url)) || !isset($urlParts['host']) || !isset($urlParts['path'])) { throw new \InvalidArgumentException(sprintf('The URL "%s" is not valid.', $url)); } - $parts = array_merge($urlParts, $parts); - $values['domain'] = $parts['host']; - $values['path'] = substr($parts['path'], 0, strrpos($parts['path'], '/')); + $values['domain'] = $urlParts['host']; + $values['path'] = substr($urlParts['path'], 0, strrpos($urlParts['path'], '/')); } foreach ($parts as $part) { From 36d057b69efe67d84cbf73a3bafce283c1fc4c3b Mon Sep 17 00:00:00 2001 From: Fabien Potencier Date: Sat, 20 Apr 2013 20:04:59 +0200 Subject: [PATCH 4/4] [HttpFoundation][BrowserKit] fixed path when converting a cookie to a string An empty path has a different meaning than a /; it means that the path is the one from the current URI. --- src/Symfony/Component/BrowserKit/Cookie.php | 2 +- .../Component/BrowserKit/Tests/ClientTest.php | 2 +- .../Component/BrowserKit/Tests/CookieTest.php | 24 +++++++++---------- .../Component/HttpFoundation/Cookie.php | 2 +- .../HttpFoundation/Tests/CookieTest.php | 4 ++-- .../Tests/ResponseHeaderBagTest.php | 6 ++--- 6 files changed, 20 insertions(+), 20 deletions(-) diff --git a/src/Symfony/Component/BrowserKit/Cookie.php b/src/Symfony/Component/BrowserKit/Cookie.php index 9feeb06dc9..48a5cfd5bd 100644 --- a/src/Symfony/Component/BrowserKit/Cookie.php +++ b/src/Symfony/Component/BrowserKit/Cookie.php @@ -95,7 +95,7 @@ class Cookie $cookie .= '; domain='.$this->domain; } - if ('/' !== $this->path) { + if ($this->path) { $cookie .= '; path='.$this->path; } diff --git a/src/Symfony/Component/BrowserKit/Tests/ClientTest.php b/src/Symfony/Component/BrowserKit/Tests/ClientTest.php index 49b0c04129..9235fd97b8 100644 --- a/src/Symfony/Component/BrowserKit/Tests/ClientTest.php +++ b/src/Symfony/Component/BrowserKit/Tests/ClientTest.php @@ -211,7 +211,7 @@ class ClientTest extends \PHPUnit_Framework_TestCase $client->setNextResponse(new Response('foo', 200, array('Set-Cookie' => 'foo=bar; path=/; secure'))); $client->request('GET', 'https://www.example.com/foo/foobar'); - $this->assertTrue($client->getCookieJar()->get('foo','/','www.example.com')->isSecure()); + $this->assertTrue($client->getCookieJar()->get('foo', '/', 'www.example.com')->isSecure()); } public function testClick() diff --git a/src/Symfony/Component/BrowserKit/Tests/CookieTest.php b/src/Symfony/Component/BrowserKit/Tests/CookieTest.php index be53eea417..606b2e2e91 100644 --- a/src/Symfony/Component/BrowserKit/Tests/CookieTest.php +++ b/src/Symfony/Component/BrowserKit/Tests/CookieTest.php @@ -26,14 +26,14 @@ class CookieTest extends \PHPUnit_Framework_TestCase public function getTestsForToFromString() { return array( - array('foo=bar'), + array('foo=bar; path=/'), array('foo=bar; path=/foo'), - array('foo=bar; domain=google.com'), - array('foo=bar; domain=example.com; secure', 'https://example.com/'), - array('foo=bar; httponly'), + array('foo=bar; domain=google.com; path=/'), + array('foo=bar; domain=example.com; path=/; secure', 'https://example.com/'), + array('foo=bar; path=/; httponly'), array('foo=bar; domain=google.com; path=/foo; secure; httponly', 'https://google.com/'), - array('foo=bar=baz'), - array('foo=bar%3Dbaz'), + array('foo=bar=baz; path=/'), + array('foo=bar%3Dbaz; path=/'), ); } @@ -67,17 +67,17 @@ class CookieTest extends \PHPUnit_Framework_TestCase public function testFromStringWithCapitalization() { - $this->assertEquals('Foo=Bar', (string) Cookie::fromString('Foo=Bar')); - $this->assertEquals('foo=bar; expires=Fri, 31 Dec 2010 23:59:59 GMT', (string) Cookie::fromString('foo=bar; Expires=Fri, 31 Dec 2010 23:59:59 GMT')); - $this->assertEquals('foo=bar; domain=www.example.org; httponly', (string) Cookie::fromString('foo=bar; DOMAIN=www.example.org; HttpOnly')); + $this->assertEquals('Foo=Bar; path=/', (string) Cookie::fromString('Foo=Bar')); + $this->assertEquals('foo=bar; expires=Fri, 31 Dec 2010 23:59:59 GMT; path=/', (string) Cookie::fromString('foo=bar; Expires=Fri, 31 Dec 2010 23:59:59 GMT')); + $this->assertEquals('foo=bar; domain=www.example.org; path=/; httponly', (string) Cookie::fromString('foo=bar; DOMAIN=www.example.org; HttpOnly')); } public function testFromStringWithUrl() { - $this->assertEquals('foo=bar; domain=www.example.com', (string) Cookie::FromString('foo=bar', 'http://www.example.com/')); + $this->assertEquals('foo=bar; domain=www.example.com; path=/', (string) Cookie::FromString('foo=bar', 'http://www.example.com/')); $this->assertEquals('foo=bar; domain=www.example.com; path=/foo', (string) Cookie::FromString('foo=bar', 'http://www.example.com/foo/bar')); - $this->assertEquals('foo=bar; domain=www.example.com', (string) Cookie::FromString('foo=bar; path=/', 'http://www.example.com/foo/bar')); - $this->assertEquals('foo=bar; domain=www.myotherexample.com', (string) Cookie::FromString('foo=bar; domain=www.myotherexample.com', 'http://www.example.com/')); + $this->assertEquals('foo=bar; domain=www.example.com; path=/', (string) Cookie::FromString('foo=bar; path=/', 'http://www.example.com/foo/bar')); + $this->assertEquals('foo=bar; domain=www.myotherexample.com; path=/', (string) Cookie::FromString('foo=bar; domain=www.myotherexample.com', 'http://www.example.com/')); } public function testFromStringThrowsAnExceptionIfCookieIsNotValid() diff --git a/src/Symfony/Component/HttpFoundation/Cookie.php b/src/Symfony/Component/HttpFoundation/Cookie.php index 43bf87dc23..f8fbe67a6e 100644 --- a/src/Symfony/Component/HttpFoundation/Cookie.php +++ b/src/Symfony/Component/HttpFoundation/Cookie.php @@ -91,7 +91,7 @@ class Cookie } } - if ('/' !== $this->path) { + if ($this->path) { $str .= '; path='.$this->path; } diff --git a/src/Symfony/Component/HttpFoundation/Tests/CookieTest.php b/src/Symfony/Component/HttpFoundation/Tests/CookieTest.php index bfedd27fe1..3249ba3251 100644 --- a/src/Symfony/Component/HttpFoundation/Tests/CookieTest.php +++ b/src/Symfony/Component/HttpFoundation/Tests/CookieTest.php @@ -117,12 +117,12 @@ class CookieTest extends \PHPUnit_Framework_TestCase public function testToString() { $cookie = new Cookie('foo', 'bar', strtotime('Fri, 20-May-2011 15:25:52 GMT'), '/', '.myfoodomain.com', true); - $this->assertEquals('foo=bar; expires=Fri, 20-May-2011 15:25:52 GMT; domain=.myfoodomain.com; secure; httponly', $cookie->__toString(), '->__toString() returns string representation of the cookie'); + $this->assertEquals('foo=bar; expires=Fri, 20-May-2011 15:25:52 GMT; path=/; domain=.myfoodomain.com; secure; httponly', $cookie->__toString(), '->__toString() returns string representation of the cookie'); $cookie = new Cookie('foo', null, 1, '/admin/', '.myfoodomain.com'); $this->assertEquals('foo=deleted; expires=' . gmdate("D, d-M-Y H:i:s T", time()-31536001) . '; path=/admin/; domain=.myfoodomain.com; httponly', $cookie->__toString(), '->__toString() returns string representation of a cleared cookie if value is NULL'); $cookie = new Cookie('foo', 'bar', 0, '/', ''); - $this->assertEquals('foo=bar; httponly', $cookie->__toString()); + $this->assertEquals('foo=bar; path=/; httponly', $cookie->__toString()); } } diff --git a/src/Symfony/Component/HttpFoundation/Tests/ResponseHeaderBagTest.php b/src/Symfony/Component/HttpFoundation/Tests/ResponseHeaderBagTest.php index fc3d908cb1..89b3e872df 100644 --- a/src/Symfony/Component/HttpFoundation/Tests/ResponseHeaderBagTest.php +++ b/src/Symfony/Component/HttpFoundation/Tests/ResponseHeaderBagTest.php @@ -69,11 +69,11 @@ class ResponseHeaderBagTest extends \PHPUnit_Framework_TestCase $bag = new ResponseHeaderBag(array()); $bag->setCookie(new Cookie('foo', 'bar')); - $this->assertContains("Set-Cookie: foo=bar; httponly", explode("\r\n", $bag->__toString())); + $this->assertContains("Set-Cookie: foo=bar; path=/; httponly", explode("\r\n", $bag->__toString())); $bag->clearCookie('foo'); - $this->assertContains("Set-Cookie: foo=deleted; expires=".gmdate("D, d-M-Y H:i:s T", time() - 31536001)."; httponly", explode("\r\n", $bag->__toString())); + $this->assertContains("Set-Cookie: foo=deleted; expires=".gmdate("D, d-M-Y H:i:s T", time() - 31536001)."; path=/; httponly", explode("\r\n", $bag->__toString())); } public function testReplace() @@ -113,7 +113,7 @@ class ResponseHeaderBagTest extends \PHPUnit_Framework_TestCase $this->assertContains("Set-Cookie: foo=bar; path=/path/foo; domain=foo.bar; httponly", $headers); $this->assertContains("Set-Cookie: foo=bar; path=/path/foo; domain=foo.bar; httponly", $headers); $this->assertContains("Set-Cookie: foo=bar; path=/path/bar; domain=bar.foo; httponly", $headers); - $this->assertContains("Set-Cookie: foo=bar; httponly", $headers); + $this->assertContains("Set-Cookie: foo=bar; path=/; httponly", $headers); $cookies = $bag->getCookies(ResponseHeaderBag::COOKIES_ARRAY); $this->assertTrue(isset($cookies['foo.bar']['/path/foo']['foo']));