Skip to content

[HttpFoundation] Send cookies using header() to fix "SameSite" ones #25348

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Apr 22, 2018
Merged

[HttpFoundation] Send cookies using header() to fix "SameSite" ones #25348

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e350ea0
[HttpFoundation] Send cookies using header() to fix "SameSite" ones
nicolas-grekas Dec 6, 2017
73fec23
[HttpFoundation] Add functional tests for Response::sendHeaders()
cvilleger Apr 5, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions src/Symfony/Component/HttpFoundation/Cookie.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,12 +145,12 @@ public function __toString()
$str = ($this->isRaw() ? $this->getName() : urlencode($this->getName())).'=';

if ('' === (string) $this->getValue()) {
$str .= 'deleted; expires='.gmdate('D, d-M-Y H:i:s T', time() - 31536001).'; max-age=-31536001';
$str .= 'deleted; expires='.gmdate('D, d-M-Y H:i:s T', time() - 31536001).'; Max-Age=0';
} else {
$str .= $this->isRaw() ? $this->getValue() : rawurlencode($this->getValue());

if (0 !== $this->getExpiresTime()) {
$str .= '; expires='.gmdate('D, d-M-Y H:i:s T', $this->getExpiresTime()).'; max-age='.$this->getMaxAge();
$str .= '; expires='.gmdate('D, d-M-Y H:i:s T', $this->getExpiresTime()).'; Max-Age='.$this->getMaxAge();
}
}

Expand Down Expand Up @@ -224,7 +224,9 @@ public function getExpiresTime()
*/
public function getMaxAge()
{
return 0 !== $this->expire ? $this->expire - time() : 0;
$maxAge = $this->expire - time();

return 0 >= $maxAge ? 0 : $maxAge;
}

/**
Expand Down
11 changes: 1 addition & 10 deletions src/Symfony/Component/HttpFoundation/Response.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,7 @@ public function sendHeaders()
}

// headers
foreach ($this->headers->allPreserveCaseWithoutCookies() as $name => $values) {
foreach ($this->headers->allPreserveCase() as $name => $values) {
Copy link

@ThomHurks ThomHurks Dec 6, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current cookie code distinguishes between urlencoding the cookie value and not doing that (raw), the setcookie() function also performs some filtering and escaping as I understand it. As far as I can see in the code, the allPreserveCase() function doesn't do that but just casts the Cookie object to string and as such isn't a 1-to-1 substitute for setcookie() and setrawcookie(), right?

Edit: Ah, I see in Cookie.php that it does distinguish between raw/non-raw and performs some checks. However a double-check to see what PHP does in setcookie() might still be useful so there's no regressions, right?

foreach ($values as $value) {
header($name.': '.$value, false, $this->statusCode);
}
Expand All @@ -336,15 +336,6 @@ public function sendHeaders()
// status
header(sprintf('HTTP/%s %s %s', $this->version, $this->statusCode, $this->statusText), true, $this->statusCode);

// cookies
foreach ($this->headers->getCookies() as $cookie) {
if ($cookie->isRaw()) {
setrawcookie($cookie->getName(), $cookie->getValue(), $cookie->getExpiresTime(), $cookie->getPath(), $cookie->getDomain(), $cookie->isSecure(), $cookie->isHttpOnly());
} else {
setcookie($cookie->getName(), $cookie->getValue(), $cookie->getExpiresTime(), $cookie->getPath(), $cookie->getDomain(), $cookie->isSecure(), $cookie->isHttpOnly());
}
}

return $this;
}

Expand Down
8 changes: 4 additions & 4 deletions src/Symfony/Component/HttpFoundation/Tests/CookieTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,13 +162,13 @@ public function testCookieIsCleared()
public function testToString()
{
$cookie = new Cookie('foo', 'bar', $expire = 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; max-age='.($expire - time()).'; path=/; domain=.myfoodomain.com; secure; httponly', (string) $cookie, '->__toString() returns string representation of the cookie');
$this->assertEquals('foo=bar; expires=Fri, 20-May-2011 15:25:52 GMT; Max-Age=0; path=/; domain=.myfoodomain.com; secure; httponly', (string) $cookie, '->__toString() returns string representation of the cookie');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please update assertEquals() calls to assertSame()? (as done at ClientTest).


$cookie = new Cookie('foo', 'bar with white spaces', strtotime('Fri, 20-May-2011 15:25:52 GMT'), '/', '.myfoodomain.com', true);
$this->assertEquals('foo=bar%20with%20white%20spaces; expires=Fri, 20-May-2011 15:25:52 GMT; max-age='.($expire - time()).'; path=/; domain=.myfoodomain.com; secure; httponly', (string) $cookie, '->__toString() encodes the value of the cookie according to RFC 3986 (white space = %20)');
$this->assertEquals('foo=bar%20with%20white%20spaces; expires=Fri, 20-May-2011 15:25:52 GMT; Max-Age=0; path=/; domain=.myfoodomain.com; secure; httponly', (string) $cookie, '->__toString() encodes the value of the cookie according to RFC 3986 (white space = %20)');

$cookie = new Cookie('foo', null, 1, '/admin/', '.myfoodomain.com');
$this->assertEquals('foo=deleted; expires='.gmdate('D, d-M-Y H:i:s T', $expire = time() - 31536001).'; max-age='.($expire - time()).'; path=/admin/; domain=.myfoodomain.com; httponly', (string) $cookie, '->__toString() returns string representation of a cleared cookie if value is NULL');
$this->assertEquals('foo=deleted; expires='.gmdate('D, d-M-Y H:i:s T', $expire = time() - 31536001).'; Max-Age=0; path=/admin/; domain=.myfoodomain.com; httponly', (string) $cookie, '->__toString() returns string representation of a cleared cookie if value is NULL');

$cookie = new Cookie('foo', 'bar', 0, '/', '');
$this->assertEquals('foo=bar; path=/; httponly', (string) $cookie);
Expand All @@ -194,7 +194,7 @@ public function testGetMaxAge()
$this->assertEquals($expire - time(), $cookie->getMaxAge());

$cookie = new Cookie('foo', 'bar', $expire = time() - 100);
$this->assertEquals($expire - time(), $cookie->getMaxAge());
$this->assertEquals(0, $cookie->getMaxAge());
}

public function testFromString()
Expand Down
39 changes: 39 additions & 0 deletions src/Symfony/Component/HttpFoundation/Tests/Fixtures/response-functional/common.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
<?php

use Symfony\Component\HttpFoundation\Response;

$parent = __DIR__;
while (!@file_exists($parent.'/vendor/autoload.php')) {
if (!@file_exists($parent)) {
// open_basedir restriction in effect
break;
}
if ($parent === dirname($parent)) {
echo "vendor/autoload.php not found\n";
exit(1);
}

$parent = dirname($parent);
}

require $parent.'/vendor/autoload.php';

error_reporting(-1);
ini_set('html_errors', 0);
ini_set('display_errors', 1);

header_remove('X-Powered-By');
header('Content-Type: text/plain; charset=utf-8');

register_shutdown_function(function () {
echo "\n";
session_write_close();
print_r(headers_list());
echo "shutdown\n";
});
ob_start();

$r = new Response();
$r->headers->set('Date', 'Sat, 12 Nov 1955 20:04:00 GMT');

return $r;
11 changes: 11 additions & 0 deletions ...mfony/Component/HttpFoundation/Tests/Fixtures/response-functional/cookie_max_age.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@

Warning: Expiry date cannot have a year greater than 9999 in %scookie_max_age.php on line 10

Array
(
[0] => Content-Type: text/plain; charset=utf-8
[1] => Cache-Control: no-cache, private
[2] => Date: Sat, 12 Nov 1955 20:04:00 GMT
[3] => Set-Cookie: foo=bar; expires=Sat, 01-Jan-10000 02:46:40 GMT; Max-Age=%d; path=/
)
shutdown
10 changes: 10 additions & 0 deletions src/Symfony/Component/HttpFoundation/Tests/Fixtures/response-functional/cookie_max_age.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
<?php

use Symfony\Component\HttpFoundation\Cookie;

$r = require __DIR__.'/common.inc';

$r->headers->setCookie(new Cookie('foo', 'bar', 253402310800, '', null, false, false));
$r->sendHeaders();

setcookie('foo2', 'bar', 253402310800, '/');
10 changes: 10 additions & 0 deletions ...Component/HttpFoundation/Tests/Fixtures/response-functional/cookie_raw_urlencode.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@

Array
(
[0] => Content-Type: text/plain; charset=utf-8
[1] => Cache-Control: no-cache, private
[2] => Date: Sat, 12 Nov 1955 20:04:00 GMT
[3] => Set-Cookie: ?*():@&+$/%#[]=?*():@&+$/%#[]; path=/
[4] => Set-Cookie: ?*():@&+$/%#[]=?*():@&+$/%#[]; path=/
)
shutdown
12 changes: 12 additions & 0 deletions ...fony/Component/HttpFoundation/Tests/Fixtures/response-functional/cookie_raw_urlencode.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<?php

use Symfony\Component\HttpFoundation\Cookie;

$r = require __DIR__.'/common.inc';

$str = '?*():@&+$/%#[]';

$r->headers->setCookie(new Cookie($str, $str, 0, '/', null, false, false, true));
$r->sendHeaders();

setrawcookie($str, $str, 0, '/', null, false, false);
9 changes: 9 additions & 0 deletions .../Component/HttpFoundation/Tests/Fixtures/response-functional/cookie_samesite_lax.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@

Array
(
[0] => Content-Type: text/plain; charset=utf-8
[1] => Cache-Control: no-cache, private
[2] => Date: Sat, 12 Nov 1955 20:04:00 GMT
[3] => Set-Cookie: CookieSamesiteLaxTest=LaxValue; path=/; httponly; samesite=lax
)
shutdown
8 changes: 8 additions & 0 deletions ...mfony/Component/HttpFoundation/Tests/Fixtures/response-functional/cookie_samesite_lax.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<?php

use Symfony\Component\HttpFoundation\Cookie;

$r = require __DIR__.'/common.inc';

$r->headers->setCookie(new Cookie('CookieSamesiteLaxTest', 'LaxValue', 0, '/', null, false, true, false, Cookie::SAMESITE_LAX));
$r->sendHeaders();
9 changes: 9 additions & 0 deletions ...mponent/HttpFoundation/Tests/Fixtures/response-functional/cookie_samesite_strict.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@

Array
(
[0] => Content-Type: text/plain; charset=utf-8
[1] => Cache-Control: no-cache, private
[2] => Date: Sat, 12 Nov 1955 20:04:00 GMT
[3] => Set-Cookie: CookieSamesiteStrictTest=StrictValue; path=/; httponly; samesite=strict
)
shutdown
8 changes: 8 additions & 0 deletions ...ny/Component/HttpFoundation/Tests/Fixtures/response-functional/cookie_samesite_strict.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<?php

use Symfony\Component\HttpFoundation\Cookie;

$r = require __DIR__.'/common.inc';

$r->headers->setCookie(new Cookie('CookieSamesiteStrictTest', 'StrictValue', 0, '/', null, false, true, false, Cookie::SAMESITE_STRICT));
$r->sendHeaders();
10 changes: 10 additions & 0 deletions ...ony/Component/HttpFoundation/Tests/Fixtures/response-functional/cookie_urlencode.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@

Array
(
[0] => Content-Type: text/plain; charset=utf-8
[1] => Cache-Control: no-cache, private
[2] => Date: Sat, 12 Nov 1955 20:04:00 GMT
[3] => Set-Cookie: %3F%2A%28%29%3A%40%26%2B%24%2F%25%23%5B%5D=%3F%2A%28%29%3A%40%26%2B%24%2F%25%23%5B%5D; path=/
[4] => Set-Cookie: ?*():@&+$/%#[]=%3F%2A%28%29%3A%40%26%2B%24%2F%25%23%5B%5D; path=/
)
shutdown
12 changes: 12 additions & 0 deletions src/Symfony/Component/HttpFoundation/Tests/Fixtures/response-functional/cookie_urlencode.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
<?php

use Symfony\Component\HttpFoundation\Cookie;

$r = require __DIR__.'/common.inc';

$str = '?*():@&+$/%#[]';

$r->headers->setCookie(new Cookie($str, $str, 0, '', null, false, false));
$r->sendHeaders();

setcookie($str, $str, 0, '/');
6 changes: 6 additions & 0 deletions .../Component/HttpFoundation/Tests/Fixtures/response-functional/invalid_cookie_name.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
The cookie name "Hello + world" contains invalid characters.
Array
(
[0] => Content-Type: text/plain; charset=utf-8
)
shutdown
11 changes: 11 additions & 0 deletions ...mfony/Component/HttpFoundation/Tests/Fixtures/response-functional/invalid_cookie_name.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
<?php

use Symfony\Component\HttpFoundation\Cookie;

$r = require __DIR__.'/common.inc';

try {
$r->headers->setCookie(new Cookie('Hello + world', 'hodor'));
} catch (\InvalidArgumentException $e) {
echo $e->getMessage();
}
58 changes: 58 additions & 0 deletions src/Symfony/Component/HttpFoundation/Tests/ResponseFunctionalTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,58 @@
<?php

/*
* This file is part of the Symfony package.
*
* (c) Fabien Potencier <fabien@symfony.com>
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Component\HttpFoundation\Tests;

use PHPUnit\Framework\TestCase;

/**
* @requires PHP 7.0
*/
class ResponseFunctionalTest extends TestCase
{
private static $server;

public static function setUpBeforeClass()
{
$spec = array(
1 => array('file', '/dev/null', 'w'),
2 => array('file', '/dev/null', 'w'),
);
if (!self::$server = @proc_open('exec php -S localhost:8054', $spec, $pipes, __DIR__.'/Fixtures/response-functional')) {
self::markTestSkipped('PHP server unable to start.');
}
sleep(1);
}

public static function tearDownAfterClass()
{
if (self::$server) {
proc_terminate(self::$server);
proc_close(self::$server);
}
}

/**
* @dataProvider provideCookie
*/
public function testCookie($fixture)
{
$result = file_get_contents(sprintf('http://localhost:8054/%s.php', $fixture));
$this->assertStringMatchesFormatFile(__DIR__.sprintf('/Fixtures/response-functional/%s.expected', $fixture), $result);
}

public function provideCookie()
{
foreach (glob(__DIR__.'/Fixtures/response-functional/*.php') as $file) {
yield array(pathinfo($file, PATHINFO_FILENAME));
}
}
}
4 changes: 2 additions & 2 deletions src/Symfony/Component/HttpFoundation/Tests/ResponseHeaderBagTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,7 @@ public function testToStringIncludesCookieHeaders()

$bag->clearCookie('foo');

$this->assertSetCookieHeader('foo=deleted; expires='.gmdate('D, d-M-Y H:i:s T', time() - 31536001).'; max-age=-31536001; path=/; httponly', $bag);
$this->assertSetCookieHeader('foo=deleted; expires='.gmdate('D, d-M-Y H:i:s T', time() - 31536001).'; Max-Age=0; path=/; httponly', $bag);
}

public function testClearCookieSecureNotHttpOnly()
Expand All @@ -125,7 +125,7 @@ public function testClearCookieSecureNotHttpOnly()

$bag->clearCookie('foo', '/', null, true, false);

$this->assertSetCookieHeader('foo=deleted; expires='.gmdate('D, d-M-Y H:i:s T', time() - 31536001).'; max-age=-31536001; path=/; secure', $bag);
$this->assertSetCookieHeader('foo=deleted; expires='.gmdate('D, d-M-Y H:i:s T', time() - 31536001).'; Max-Age=0; path=/; secure', $bag);
}

public function testReplace()
Expand Down
17 changes: 6 additions & 11 deletions src/Symfony/Component/HttpKernel/Tests/ClientTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,22 +60,17 @@ public function testFilterResponseConvertsCookies()
$m = $r->getMethod('filterResponse');
$m->setAccessible(true);

$expected = array(
'foo=bar; expires=Sun, 15-Feb-2009 20:00:00 GMT; max-age='.(strtotime('Sun, 15-Feb-2009 20:00:00 GMT') - time()).'; path=/foo; domain=http://example.com; secure; httponly',
'foo1=bar1; expires=Sun, 15-Feb-2009 20:00:00 GMT; max-age='.(strtotime('Sun, 15-Feb-2009 20:00:00 GMT') - time()).'; path=/foo; domain=http://example.com; secure; httponly',
);

$response = new Response();
$response->headers->setCookie(new Cookie('foo', 'bar', \DateTime::createFromFormat('j-M-Y H:i:s T', '15-Feb-2009 20:00:00 GMT')->format('U'), '/foo', 'http://example.com', true, true));
$response->headers->setCookie($cookie1 = new Cookie('foo', 'bar', \DateTime::createFromFormat('j-M-Y H:i:s T', '15-Feb-2009 20:00:00 GMT')->format('U'), '/foo', 'http://example.com', true, true));
$domResponse = $m->invoke($client, $response);
$this->assertEquals($expected[0], $domResponse->getHeader('Set-Cookie'));
$this->assertSame((string) $cookie1, $domResponse->getHeader('Set-Cookie'));

$response = new Response();
$response->headers->setCookie(new Cookie('foo', 'bar', \DateTime::createFromFormat('j-M-Y H:i:s T', '15-Feb-2009 20:00:00 GMT')->format('U'), '/foo', 'http://example.com', true, true));
$response->headers->setCookie(new Cookie('foo1', 'bar1', \DateTime::createFromFormat('j-M-Y H:i:s T', '15-Feb-2009 20:00:00 GMT')->format('U'), '/foo', 'http://example.com', true, true));
$response->headers->setCookie($cookie1 = new Cookie('foo', 'bar', \DateTime::createFromFormat('j-M-Y H:i:s T', '15-Feb-2009 20:00:00 GMT')->format('U'), '/foo', 'http://example.com', true, true));
$response->headers->setCookie($cookie2 = new Cookie('foo1', 'bar1', \DateTime::createFromFormat('j-M-Y H:i:s T', '15-Feb-2009 20:00:00 GMT')->format('U'), '/foo', 'http://example.com', true, true));
$domResponse = $m->invoke($client, $response);
$this->assertEquals($expected[0], $domResponse->getHeader('Set-Cookie'));
$this->assertEquals($expected, $domResponse->getHeader('Set-Cookie', false));
$this->assertSame((string) $cookie1, $domResponse->getHeader('Set-Cookie'));
$this->assertSame(array((string) $cookie1, (string) $cookie2), $domResponse->getHeader('Set-Cookie', false));
}

public function testFilterResponseSupportsStreamedResponses()
Expand Down