8000 SameSite cookie attribute does not work; suggest to remove it · Issue #25344 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content
8000

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

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

Jump to bottom
Closed
ThomHurks opened this issue Dec 5, 2017 · 3 comments
Closed

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

ThomHurks opened this issue Dec 5, 2017 · 3 comments
Labels
Bug Status: Needs Review

Comments

@ThomHurks
Copy link
Q A
Bug report? yes
Feature request? no
BC Break report? no
RFC? no
Symfony version 3.4.1

In #19104 support was added for the SameSite attribute in cookies. However, a pretty big oversight is that this parameter is never passed to PHP's setcookie() in http-foundation/Response.php. You can't even if you'd want to, because PHP will only support SameSite in 7.3 see https://wiki.php.net/rfc/same-site-cookie

I'm not sure why SameSite was already added to Symfony if PHP doesn't support it yet, but this is quite misleading. I spent several hours debugging my Laravel app trying to figure out why SameSite wasn't sent, and eventually I found out that Symfony never passes it to PHP, because PHP doesn't accept the option yet.

Until PHP 7.3 is released and assuming the RFC I linked to is implemented, I suggest to remove the SameSite property from Symfony since right now it doesn't do anything and is plainly misleading.
This was referenced Dec 5, 2017
Closed
Closed
@curry684
Copy link
Contributor
curry684 commented Dec 5, 2017

Or just if (PHP_VERSION < 70300) { trigger_error('The SameSite option is not supported in PHP <7.3', E_USER_NOTICE); }

@ThomHurks
Copy link
Author

I'm not sure how Symfony typically handles cases like this, but if people enabled usage of SameSite thinking it will give them better protection against CSRF attacks, which is what SameSite is meant for, then it better be a serious error and not E_NOTICE.

@nicolas-grekas nicolas-grekas mentioned this issue Dec 6, 2017
Merged
@nicolas-grekas
Copy link
Member

See #25348, we have everything in core already to fix it now.

@staabm staabm mentioned this issue Jan 12, 2018
Closed
@lstrojny lstrojny mentioned this issue Mar 16, 2018
Closed
This was referenced Apr 5, 2018
Merged
Closed
Closed
fabpot added a commit that referenced this issue Apr 22, 2018
@fabpot
bug #25348 [HttpFoundation] Send cookies using header() to fix "SameS…
82a95df 
…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
@fabpot fabpot closed this as completed Apr 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Bug Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@fabpot @nicolas-grekas @curry684 @ThomHurks @carsonbot
0