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

nicolas-grekas · 2017-12-06T09:32:37Z

Q	A
Branch?	3.4
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#25344
License	MIT
Doc PR	-

ThomHurks · 2017-12-06T18:12:35Z

src/Symfony/Component/HttpFoundation/Response.php

@@ -337,7 +337,7 @@ public function sendHeaders()
        }

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


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?

sroze · 2017-12-06T20:36:37Z

Nop, the Cookie now understands how to stringify itself differently based the raw or not raw. Therefore, we don't need PHP's `set*cookie`

…

On Wed, 6 Dec 2017 at 18:13, Thom Hurks ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/Symfony/Component/HttpFoundation/Response.php <#25348 (comment)>: > @@ -337,7 +337,7 @@ public function sendHeaders() } // headers - foreach ($this->headers->allPreserveCaseWithoutCookies() as $name => $values) { + foreach ($this->headers->allPreserveCase() as $name => $values) { 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? — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#25348 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAxHETp-qltXfQNc_xmHHXVlwKnmRftaks5s9tkdgaJpZM4Q3nK_> .

ThomHurks · 2017-12-06T20:42:55Z

@sroze Yes, I realized that after examining the code more closely and so I edited my comment. Still, cross-referencing the code with PHP's implementation may be a good thing to do to prevent regression in behaviour. I'm not sure how new the Cookie code is, and the code that is currently there is not actually used to generate the final cookie at the moment, so it may be incomplete/untested?

nicolas-grekas · 2017-12-06T21:13:35Z

@ThomHurks here is the native implem:
https://github.com/php/php-src/blob/master/ext/standard/head.c#L84

everything looks fine to me.

ThomHurks · 2017-12-07T09:02:52Z

@nicolas-grekas Not the case, there are some differences that I can spot:

Symfony does not check the value of value against the characters =,; \t\r\n\013\014 whereas PHP does. Symfony only checks it against name and PHP checks it agains name and value.
When a cookie needs to be deleted, Symfony sets the max-age to 31536001 and the expiry to now - max-age but PHP just passes in expiry = 1 and max-age = 0.
PHP checks if the expiry date has a year greater than 9999.
Symfony also urlencode's name, I don't see PHP do that.
Symfony uses urlencode for the name vs rawurlencode for the value, not sure why that difference is there.

sroze · 2017-12-07T09:07:10Z

I understand your concerns, and agree a bit. We will add integration testing to this PR very soon. Highlighting the important scenarios that you believe needs to be tested would be a very nice input from you! Could you do so?

…

On Thu, 7 Dec 2017 at 09:02, Thom Hurks ***@***.***> wrote: @nicolas-grekas <https://github.com/nicolas-grekas> Not the case, there are some differences that I can spot: - Symfony does not check the value of value against the characters =,; \t\r\n\013\014 whereas PHP does. Symfony only checks it against name and PHP checks it agains name and value. - When a cookie needs to be deleted, Symfony sets the max-age to 31536001 and the expiry to now - max-age but PHP just passes in expiry = 1 and max-age = 0. - PHP checks if the expiry date has a year greater than 9999. - Symfony also urlencode's name, I don't see PHP do that. - Symfony uses urlencode for the name vs rawurlencode for the value, not sure why that difference is there. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#25348 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAxHEcCjAYcpYVxNkRxVfnFdeovGgsknks5s96nCgaJpZM4Q3nK_> .

nicolas-grekas · 2017-12-07T19:51:45Z

I'd like to propose a plan:

@sroze would you like to submit a testing infra for this? Could be heavily inspired by AbstractSessionHandlerTest on 3.4
@ThomHurks would you like to submit a separate PR on 3.3 to align the behavior with native PHP?

sroze · 2017-12-07T20:13:46Z

I like this plan 👍

…

On Thu, 7 Dec 2017 at 19:51, Nicolas Grekas ***@***.***> wrote: I'd like to propose a plan: - @sroze <https://github.com/sroze> would you like to submit a testing infra for this? Could be heavily inspired by AbstractSessionHandlerTest on 3.4 - @ThomHurks <https://github.com/thomhurks> would you like to submit a separate PR on 3.3 to align the behavior with native PHP? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#25348 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAxHEbIHdt80LKCThXK49tqP3kTEwUoKks5s-EHXgaJpZM4Q3nK_> .

ThomHurks · 2017-12-07T22:02:57Z

@nicolas-grekas I will likely have time tomorrow to create a PR.

nicolas-grekas · 2017-12-22T21:33:12Z

@sroze @ThomHurks any progress?

halaei · 2018-01-12T14:27:41Z

Regarding spectre attack and suggested mitigations should this be considered as a critical security fix?

sroze · 2018-01-12T14:48:15Z

I don't think it has anything to do with the attacks you've mentioned. I did not get time to get ahead with this so if anybody would like until I get back to it, you're welcome 😉

halaei · 2018-01-12T15:08:12Z

@sroze I quote the part from https://www.chromium.org/Home/chromium-security/ssca that seems to imply this is actually relevant:

Where possible, prevent cookies from entering the renderer process' memory by using the SameSite and HTTPOnly cookie attributes, and by avoiding reading from document.cookie.

xabbuh · 2018-01-29T15:24:03Z

moving to the 3.4 milestone as the last bugfix release for 3.3 was published today

nicolas-grekas · 2018-04-20T20:00:48Z

Back on here, this PR now contains a second commit provided by @cvilleger, thank to him for working on it.

The Response + Cookie classes are now functionnally tested, and their behavior is a bit more aligned to PHP native one:

Symfony does not check the value of value against the characters =,; \t\r\n\013\014 whereas PHP does. Symfony only checks it against name and PHP checks it agains name and value.

It does for the name, but doesn't care for the value. HTTP allows anything so no reason to restrict like PHP.

When a cookie needs to be deleted, Symfony sets the max-age to 31536001 and the expiry to now - max-age but PHP just passes in expiry = 1 and max-age = 0.

Aligned

PHP checks if the expiry date has a year greater than 9999.

I don't see any reason, kept as is (and tested).

Symfony also urlencode's name, I don't see PHP do that.

PHP should encode. Symfony does, that's OK.

Symfony uses urlencode for the name vs rawurlencode for the value, not sure why that difference is there.

Unrelated to the PHP vs Symfony delta, kept as is.

Status: needs review

phansys · 2018-04-21T05:42:37Z

src/Symfony/Component/HttpFoundation/Tests/CookieTest.php

@@ -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');


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

fabpot · 2018-04-22T06:01:26Z

Thank you @nicolas-grekas.

…ite" ones (nicolas-grekas, cvilleger) This PR was merged into the 3.4 branch. Discussion ---------- [HttpFoundation] Send cookies using header() to fix "SameSite" ones | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #25344 | License | MIT | Doc PR | - Commits ------- 73fec23 [HttpFoundation] Add functional tests for Response::sendHeaders() e350ea0 [HttpFoundation] Send cookies using header() to fix "SameSite" ones

reatang · 2018-05-02T03:25:05Z

Added the RFC document：rfc6265
The Set-Cookie Header：
https://tools.ietf.org/html/rfc6265#section-5.2.2

Seldaek · 2018-07-02T07:32:14Z

@nicolas-grekas you said:

Symfony also urlencode's name, I don't see PHP do that.
PHP should encode. Symfony does, that's OK.

This might be correct from a spec point of view, but FYI upgrading our app broke cookies with names "foo:bar:baz" as the : gets encoded and CloudFront's cookie whitelist does not recognize the encoded foo%3Abar as being the same cookie as foo:bar so the cookie gets dropped before reaching our servers. We could work around it but I wonder if this should be considered a regression or not.

Seldaek · 2018-07-02T08:52:51Z

Also a related issue, clearCookie does not allow clearing cookies without urlencoding, so a cookie set without urlencoding can not be cleared.

nicolas-grekas · 2018-07-03T12:16:29Z

@Seldaek I think we can fix these. WOuld you mind opening an issue?

@Seldaek

…grekas) This PR was merged into the 3.4 branch. Discussion ---------- [HttpFoundation] don't encode cookie name for BC | Q | A | ------------- | --- | Branch? | 3.4 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | - | License | MIT | Doc PR | - As reported by @Seldaek in #25348 (comment) Commits ------- d28949b [HttpFoundation] don't encode cookie name for BC

nicolas-grekas added this to the 3.3 milestone Dec 6, 2017

carsonbot added Status: Needs Review HttpFoundation Bug labels Dec 6, 2017

nicolas-grekas mentioned this pull request Dec 6, 2017

SameSite cookie attribute does not work; suggest to remove it #25344

Closed

fernandobandeira mentioned this pull request Dec 6, 2017

SameSite cookie flag not working laravel/framework#21872

Closed

ThomHurks reviewed Dec 6, 2017

View reviewed changes

nicolas-grekas added Status: Needs Work and removed Status: Needs Review labels Dec 9, 2017

halaei mentioned this pull request Jan 12, 2018

implement same site cookie php/php-src#2613

Closed

xabbuh modified the milestones: 3.3, 3.4 Jan 29, 2018

nicolas-grekas mentioned this pull request Mar 16, 2018

[HttpFoundation] Support samesite cookies in response #26478

Closed

nicolas-grekas changed the base branch from 3.3 to 3.4 April 4, 2018 14:52

nicolas-grekas force-pushed the same-site-cookie branch from e04c84a to b2785b0 Compare April 4, 2018 14:52

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

e350ea0

nicolas-grekas force-pushed the same-site-cookie branch from 30cac90 to 6386d39 Compare April 6, 2018 11:09

nicolas-grekas force-pushed the same-site-cookie branch from 6386d39 to 12aa1cd Compare April 6, 2018 11:13

nicolas-grekas force-pushed the same-site-cookie branch 2 times, most recently from ef09ba8 to 65a6ec1 Compare April 20, 2018 18:36

[HttpFoundation] Add functional tests for Response::sendHeaders()

73fec23

nicolas-grekas force-pushed the same-site-cookie branch from 65a6ec1 to 73fec23 Compare April 20, 2018 18:41

carsonbot added Status: Needs Review Status: Needs Work and removed Status: Needs Work Status: Needs Review labels Apr 20, 2018

phansys suggested changes Apr 21, 2018

View reviewed changes

carsonbot removed the Status: Needs Work label Apr 22, 2018

fabpot approved these changes Apr 22, 2018

View reviewed changes

carsonbot added the Status: Reviewed label Apr 22, 2018

fabpot merged commit 73fec23 into symfony:3.4 Apr 22, 2018

nicolas-grekas deleted the same-site-cookie branch April 26, 2018 21:09

This was referenced Apr 30, 2018

Release v3.4.9 #27102

Merged

Release v4.0.9 #27103

Merged

nicolas-grekas mentioned this pull request Jul 7, 2018

[HttpFoundation] don't encode cookie name for BC #27885

Merged

rpkamp mentioned this pull request Aug 6, 2018

[Security] Ability to set SameSite attribute for session and remember me cookie #27631

Closed

summercms mentioned this pull request Oct 2, 2020

Use browser 2020 recommendations for SameSite cookie rules octobercms/october#5293

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

nicolas-grekas commented Dec 6, 2017 •

edited

Loading

ThomHurks Dec 6, 2017 •

edited

Loading

sroze commented Dec 6, 2017 via email

ThomHurks commented Dec 6, 2017

nicolas-grekas commented Dec 6, 2017

ThomHurks commented Dec 7, 2017

sroze commented Dec 7, 2017 via email

nicolas-grekas commented Dec 7, 2017

sroze commented Dec 7, 2017 via email

ThomHurks commented Dec 7, 2017

nicolas-grekas commented Dec 22, 2017

halaei commented Jan 12, 2018

sroze commented Jan 12, 2018

halaei commented Jan 12, 2018

xabbuh commented Jan 29, 2018

nicolas-grekas commented Apr 20, 2018 •

edited

Loading

phansys Apr 21, 2018

fabpot commented Apr 22, 2018

reatang commented May 2, 2018

Seldaek commented Jul 2, 2018

Seldaek commented Jul 2, 2018

nicolas-grekas commented Jul 3, 2018

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

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

Conversation

nicolas-grekas commented Dec 6, 2017 • edited Loading

ThomHurks Dec 6, 2017 • edited Loading

Choose a reason for hiding this comment

sroze commented Dec 6, 2017 via email

ThomHurks commented Dec 6, 2017

nicolas-grekas commented Dec 6, 2017

ThomHurks commented Dec 7, 2017

sroze commented Dec 7, 2017 via email

nicolas-grekas commented Dec 7, 2017

sroze commented Dec 7, 2017 via email

ThomHurks commented Dec 7, 2017

nicolas-grekas commented Dec 22, 2017

halaei commented Jan 12, 2018

sroze commented Jan 12, 2018

halaei commented Jan 12, 2018

xabbuh commented Jan 29, 2018

nicolas-grekas commented Apr 20, 2018 • edited Loading

phansys Apr 21, 2018

Choose a reason for hiding this comment

fabpot commented Apr 22, 2018

reatang commented May 2, 2018

Seldaek commented Jul 2, 2018

Seldaek commented Jul 2, 2018

nicolas-grekas commented Jul 3, 2018

nicolas-grekas commented Dec 6, 2017 •

edited

Loading

ThomHurks Dec 6, 2017 •

edited

Loading

nicolas-grekas commented Apr 20, 2018 •

edited

Loading