Conversation
|
The patch is a workaround but does not fix the problem. The problem as I see it is that the session is not started even if there is already one. So, we probably need to call session_start() if the user already have a valid session. What do you think? |
|
Well, I'm not sure if we should do that kind of assumption automatically or not. Also I'm not sure how to do that, would you check the cookies and if there is a SESSID or whatever you start automatically? It's probably not too harmful, but still might be a problem for the login form, assuming you don't start the session before. Although you most likely want to check if there is a user in the session to know if you need to show the login form, so to check you'd start the session in most cases. Bottom line is.. I'm not sure :) But in any case I think that patch should be merged, it's not a workaround it's just proper error reporting instead of the dev having to figure out what the hell is happening (and believe me it wasn't obvious, I had to dive into the whole csrf generation stuff to finally figure out that it was using session_id() and realize my mistake). |
|
Just saw this, and I think it's closely related to my pull request: Besides, we cannot assume that we always have a session, e.g. when the user wants stateless requests. Forms should work even without a session resulting in reduced security ofc since the csrf will be the same for all users. |
|
I agree that we should not start the session automatically inside the form. I don't really see a use case where CSRF protection could be desired in a stateless environment. I merged this change into my master. |
|
I don't like the exception as nothing is wrong. The user just try to be as secure as possible by default. So, I think the form should still work even if there is no session and the CSRF protection is enabled. My take on the problem is here: It's in a topic branch, so tell me what you think about this before I merge it in my master branch. |
|
I don't like this solution. The dev believes he enables CSRF, but in reality it is disabled. I prefer transparency over magic here - if no session is available, enableCsrfProtection() must not be called. Of course, we have to document this. |
|
Same here, I really liked me exception better because you can't miss it. Otherwise it becomes silently insecure which is really evil. So far Sf2 is really secure by default with the output escaping, doctrine2 parameters etc, and it would be a shame to see this trend go away. |
|
It has nothing to do with security. If there is no session, there is no possible CSRF attack. Or am I missing something? |
|
Yes. Then why should the developer be allowed to call enableCsrfProtection()? |
|
Well actually, my original problem was that the token never matched because the session wasn't started until the point where I needed the token. So the token was generated without session_id(), but verified with a session_id() present. This wasn't a security issue, but it just prevented my code from working. This may be fixed now with the last session/request changes, I'm not sure. In this case then yes I guess it's not a security issue to disable it if there is no session, but you have to make sure that no session now means no session at all in this request. |
|
The security will always be reduced if a form can be submitted without a CSRF token. This even applies if we have no session. For example, someone restricts access to a form to a certain IP range/network. Without a CSRF token someone from outside the IP range might trick someone within the IP range to submit the form without knowing. However, I think this is a very specialized case. The main problem I see, is that the developer might be confused when a form doesn't validate because he has a session on the validation action (because for some reason he has written some data to it), but he has no session on the rendering page. In this case, his form will never validate, right? |
|
@schmittjoh: Yes. This confusion should be avoided by the exception thrown in Jordi's commit. If someone needs such a specialized CSRF token he can overwrite generateCsrfToken(). |
|
i also think it should be totally transparent, aka either the dev has disabled csrf entirely, has disabled it for a specific form or has disabled it specifically for this request. but implicitly disabling the logic in a method call based on if a session is enabled or not it is a bit too magic for me. also while recent changes aim to ensure that such race conditions (csrf token being generated before the session is started) do not happen, it seems to me like its making the code fragile and hard to debug in case these measures fail. |
|
We document it better :) |
|
Yes. The question is why his session is not started. |
|
Because he does not have any authentication, so there is no session. And by default, CSRF is enabled. |
|
Can we always start the session? |
|
probably not a good idea, since there could be heavy costs associated depending on the session storage used. |
|
Maybe it's better to have session on/off option in FrameworkBundle configuration. So the user could simply turn it on or off by configuration. In my opinion, it would be better if user will always explicitly specify that sessions is enabled/disabled in config. If we do so, we could simply provide better exceptions & checks in components, so they will tell user, that he must enable sessions before enabling CSRF or some authentication rules (in config). And by the way, if CSRF is enabled by default, maybe it's better way to enable sessions by default to, and give user explicit option to turn it off. It does make sence, because in most cases, developers works with non-stateless projects & needs sessions & CSRF protection for it. |
|
The configurable session with default "on" sounds good to me. Fabien? |
|
I opened this ticket http://trac.symfony-project.org/ticket/9278 |
|
Some of you do not want the session to be started automatically to generate the form. Ok, I understand. But Symfony2 is axed on the security. The CSRF Token is important for a form. It would bother me having to check if the session exists on each controller that instantiates a form, to prevent my users from getting an error 500. |
|
Why not use fabpot proposed solution? If there is no session, then CSRF is not needed for security. CSRF is only important when there is a session started, a session_id. No need for the developer to worry about it if he has not started any session. |
|
The problem I encountered was that I fetched my user object between the form definition and the form validation, and because of that, the CSRF token was initially not having the session_id, but when validating the form, the session was started so a CSRF token could be generated with session_id, and they didn't match anymore. I think this is way harder to debug and figure out what's wrong than to learn why you get an exception and then learn to enable the session (which could be enabled by default now) or disable the CSRF protection of all forms if you're not interested in it. |
|
Oh! right right right. So then, how do we make obvious on which actions we want session enabled. |
|
the patch has been merged. there is still an outstanding ticket, which i have commented. so we can close this one? |
|
I guess we can. |
This PR was merged into the 2.1-dev branch. Discussion ---------- Drop support for Symfony 4 Symfony 4 is dead and dropping support for it should make the maintenance of this package a bit easier. Commits ------- b360b35 Drop support for Symfony 4
…he monorepo (fabpot, dunglas, KorvinSzanto, xabbuh, aimeos, ahundiak, Danielss89, rougin, csunolgomez, Jérôme Parmentier, mtibben, Nyholm, ajgarlag, uphlewis, samnela, grachevko, nicolas-grekas, tinyroy, danizord, Daniel Degasperi, rbaarsma, Ekman, 4rthem, derrabus, mleczakm, iluuu1994, Tobion, chalasr, lemon-juice, franmomu, cidosx, erikn69, AurelienPillevesse) This PR was merged into the 6.4 branch. Discussion ---------- [PsrHttpMessageBridge] Import the bridge into the monorepo | Q | A | ------------- | --- | Branch? | 6.4 | Bug fix? | no | New feature? | yes | Deprecations? | no | Tickets | N/A | License | MIT | Doc PR | TODO⚠️ Don't squash! I propose to import the `symfony/psr-http-message-bridge` package into the Symfony monorepo for further maintenance. Commits ------- e40dd66 [PsrHttpMessageBridge] Patch return types and fix CS 266c09f [PsrHttpMessageBridge] Import the bridge into the monorepo 0c0323a Add 'src/Symfony/Bridge/PsrHttpMessage/' from commit '581ca6067eb62640de5ff08ee1ba6850a0ee472e' 581ca60 Prepare release 2.3.1 45d0349 Fix CS 6410dda bug #122 Don't rely on Request::getPayload() to populate the parsed body (nicolas-grekas) ef03b6d Don't rely on Request::getPayload() to populate the parsed body 3c62b81 minor #120 Prepare release 2.3.0 (derrabus) 96acbfd Prepare release 2.3.0 7eedd34 feature #119 Implement ValueResolverInterface (derrabus) 0b54b85 Implement ValueResolverInterface 6b2f5df feature #117 Leverage `Request::getPayload()` to populate the parsed body of PSR-7 requests (AurelienPillevesse) 3a8caad Leverage `Request::getPayload()` to populate the parsed body of PSR-7 requests 18c9e82 minor #118 Add native types where possible (derrabus) 4fd4323 Add native types where possible 28a732c minor #115 Prepare the 2.2.0 release (derrabus) 7944831 cs fix 99ddcaa Prepare the 2.2.0 release 8a5748d feature #113 Bump psr/http-message version (erikn69) ec83c1c Bump psr/http-message version 694016e feature #114 Drop support for Symfony 4 (derrabus) b360b35 Drop support for Symfony 4 998d8d2 minor #111 Adjustments for PHP CS Fixer 3 (derrabus) 5fa5f62 Adjustments for PHP CS Fixer 3 a125b93 minor #110 Add PHP 8.2 to CI (derrabus) 4592df2 Add PHP 8.2 to CI 4617ac3 bug #109 perf: ensure timely flush stream buffers (cidosx) 8c8a75b perf: ensure timely flush stream buffers d444f85 Update changelog 155a7ae bug #107 Ignore invalid HTTP headers when creating PSR7 objects (nicolas-grekas) 9a78a16 Ignore invalid HTTP headers when creating PSR7 objects bdb2871 minor #104 Add missing .gitattributes (franmomu) 808561a Add missing .gitattributes 316f5cb bug #103 Fix for wrong type passed to moveTo() (lemon-juice) 7f3b5c1 Fix for wrong type passed to moveTo() 22b37c8 minor #101 Release v2.1.2 (chalasr) c382d76 Release v2.1.2 c81476c feature #100 Allow Symfony 6 (chalasr) c7a0be3 Allow Symfony 6 df83a38 minor #98 Add PHP 8.1 to CI (derrabus) b2bd334 Add PHP 8.1 to CI 824711c minor #99 Add return types to fixtures (derrabus) f8f70fa Add return types to fixtures d558dcd minor #97 Inline $tmpDir (derrabus) d152649 Inline $tmpDir f12a9e6 minor #96 Run PHPUnit on GitHub Actions (derrabus) ab64c69 Run PHPUnit on GitHub Actions c901299 bug #95 Allow `psr/log` 2 and 3 (derrabus) 8e13ae4 Allow psr/log 2 and 3 26068fa Minor cleanups 87fabb9 Fix copyright year 3d9241f minor #92 remove link to sensio extra bundle which removed psr7 support (Tobion) 7078739 remove link to sensio extra bundle which removed psr7 support 81db2d4 feature #89 PSR HTTP message converters for controllers (derrabus) aa26e61 PSR HTTP message converters for controllers e62b239 minor #91 Fix CS (derrabus) 2bead22 Fix CS 488df9b minor #90 Fix CI failures with Xdebug 3 and test on PHP 7.4/8.0 as well (derrabus) a6697fd Fix CI failures with Xdebug 3 and test on PHP 7.4/8.0 as well c62f7d0 Update branch-alias 51a21cb Update changelog a20fff9 bug #87 Fix populating server params from URI in HttpFoundationFactory (nicolas-grekas) 4933e04 bug #86 Create cookies as raw in HttpFoundationFactory (nicolas-grekas) 66095a5 Fix populating server params from URI in HttpFoundationFactory 42cca49 Create cookies as raw in HttpFoundationFactory cffb3a8 bug #85 Fix BinaryFileResponse with range to psr response conversion (iluuu1994) 5d5932d Fix BinaryFileResponse with range to psr response conversion e44f249 bug #81 Don't normalize query string in PsrHttpFactory (nicolas-grekas) bc25829 Don't normalize query string in PsrHttpFactory df735ec bug #78 Fix populating default port and headers in HttpFoundationFactory (mleczakm) 4f30401 Fix populating default port and headers in HttpFoundationFactory 1309b64 bug #77 fix conversion for https requests (4rthem) e86de3f minor #79 Allow installation on php 8 (derrabus) 9243f93 Allow installation on php 8. d336c73 fix conversion for https requests 126903c Fix format of CHANGELOG.md ce709cd feature #75 Remove deprecated code (fabpot) dfc5238 Remove deprecated code 9d3e80d bug #72 Use adapter for UploadedFile objects (nicolas-grekas) a4f9f6d Use adapter for UploadedFile objects ec7892b Fix CHANGELOG, bump branch-alias 7ab4fe4 minor #70 Updated CHANGELOG (rbaarsma) 9ad4bcc Updated CHANGELOG c4c904a minor #71 Cleanup after bump to Symfony v4.4 (nicolas-grekas) e9a9557 Cleanup after bump to Symfony v4.4 3d10a6c feature #66 Add support for streamed Symfony request (Ekman) df26630 Add support for streamed Symfony request 5aa8ca9 bug #69 Allow Symfony 5.0 (rbaarsma) 1158149 Allow Symfony 5.0 81ae86d Merge branch '1.1' a33352a bug #64 Fixed createResponse (ddegasperi) 7a4b449 minor #65 Fix tests (ajgarlag) 19905b0 Fix tests 580de38 Fixed createResponse 9ab9d71 minor #63 Added links to documentation (Nyholm) 59b9406 Added links to documentation c1cb51c feature #50 Add support for streamed response (danizord) 4133c7a bug #48 Convert Request/Response multiple times (Nyholm) 8564bf7 Convert Request/Response multiple times 7cc1605 Add support for streamed response aebc14b feature #62 bump to PHP 7.1 (nicolas-grekas) 8e10923 bump to PHP 7.1 5e5e0c3 Revert "Undeprecate DiactorosFactory for 1.1" 921f866 Undeprecate DiactorosFactory for 1.1 8592ca3 bug #61 removed 'Set-Cookie' from header when it is already converted to a Symfony header cookie (tinyroy) dd1111e removed 'Set-Cookie' from header when it is already converted to a Symfony header cookie ba672d8 bump branch-alias 5f9a032 typo f2c48c5 fix tests 3a52e44 bug #59 Fix SameSite attribute conversion from PSR7 to HttpFoundation (ajgarlag) 5ee1f8f Fix SameSite attribute conversion from PSR7 to HttpFoundation f6d7d3a bug #58 [Bugfix] Typo header set-sookie (grachevko) 16eb6e1 minor #57 Excluded tests from classmap (samnela) 36a8065 Deprecate DiactorosFactory, use nyholm/psr7 for tests 5076934 bug #54 Fix #51 (compatability issue with zendframework/zend-diactoros ^2.0) (uphlewis) 757ea81 [Bugfix] Typo header set-sookie 25f9c3a Excluded tests from classmap 8ff61e5 Fix compatability issue with "zendframework/zend-diactoros": "^2.0." (#51) 53c15a6 updated CHANGELOG c821241 bumped version to 1.1 f26d01f minor #47 Updated changelog (Nyholm) c2282e3 Updated changelog eddd6c8 feature #43 Create PSR-7 messages using PSR-17 factories (ajgarlag) dd81b4b Create PSR-7 messages using PSR-17 factories f11f173 feature #45 Fixed broken build (Nyholm) 8780dd3 Fixed broken build c2b7579 bug #30 Fix the request target in PSR7 Request (mtibben) 94fcfa5 Fix the request target in PSR7 Request 64640ee minor #38 Run PHP 5.3 tests on Precise (Lctrs) 64c0cb0 Run PHP 5.3 tests on Precise b209840 minor #32 Allow Symfony 4 (dunglas) 97635f1 Allow Symfony 4 147a238 minor #31 test suite compatibility with PHPUnit 6 (xabbuh) f5c46f0 test suite compatibility with PHPUnit 6 66085f2 preparing 1.0 release 533d3e4 added a CHANGELOG for 1.0 14269f9 bug #28 Fix REQUEST_METHOD on symfony request (csunol) 98ab85a Fix REQUEST_METHOD on symfony request 29be4f8 updated LICENCE year d2db47c removed obsolete CHANGELOG file 1c30b17 bug #22 Fixes #16 Symfony Request created without any URI (rougin) a59c572 Fixes #16 Symfony Request created without any URI 7a5aa92 bug #23 Fixes #9 Bridge error when no file is selected (ahundiak, Danielss89) a1a631a Update assert error message e5d62e6 Fixes based on code-review 101b608 Handles null file in createrequest bridge. d16c63c bug #18 Allow multiple calls to Request::getContent() (aimeos) 9624b8b Allow multiple calls to Request::getContent() 9c747c4 Merge pull request #19 from xabbuh/travis-config a388c43 update Travis CI configuration ac5cd86 minor #14 Remove use of deprecated 'deep' parameter in tests (KorvinSzanto) 305c0fe Remove use of deprecated 'deep' parameter 3664dc0 minor #7 Test Diactoros Factory with PHP 5.4 (dunglas) bab1530 Test Diactoros Factory with PHP 5.4 d7660b8 Suggest psr/http-message-implementation dc7e308 removed the branch alias for now as we are pre 1.0 3f8977e feature #1 Initial support (dunglas) ca41146 Initial support 01b110b added the initial set of files
I ran into a problem because I was fetching my user object (hence doing a session_start()) between the form declaration and the form binding, so the session_id() wasn't included in the form token, but was included in the form it tried to match against it, not fun :)