-
Notifications
You must be signed in to change notification settings - Fork 47
EZP-25482: Fix exception with CSRF protection disabled #515
base: master
Are you sure you want to change the base?
Conversation
you need to add a test in https://github.com/ezsystems/PlatformUIBundle/blob/master/Tests/ApplicationConfig/Providers/SessionInfoTest.php |
9a2db3d
to
e39697f
Compare
@dpobel Done. |
e39697f
to
3362f0d
Compare
+1 |
|
@andrerom that's a very good remark, I need to check |
the actual login should work but did you check with an existing session ? |
@dpobel I'm not sure I understand. What and how do I need to test? It's not too obvious from JS code. |
:) |
Hm... This is what I get now when trying to login:
Is this expected? |
@@ -29,7 +29,7 @@ class SessionInfo implements Provider | |||
|
|||
public function __construct( | |||
SessionInterface $session, | |||
CsrfTokenManagerInterface $csrfTokenManager, | |||
CsrfTokenManagerInterface $csrfTokenManager = null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You cannot have a default value here. If you want to add null
as default value, you must move the argument at the end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Damn it! Slipped through my fingers!
But then it's a breaking change :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically yes, but it should be OK IMHO. And btw the service should be marked private
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically yes, but it should be OK IMHO.
Okay, fixed.
And btw the service should be marked
private
It already is? Or you meant Symfony DIC service? Any reason why?
The consensus reached after discussion with the platform team can be read on jira. This pull-request is not sufficient at the moment, as it only fixes an error, but doesn't make the wholee thing work. What we would like for the moment is that PlatformUI throws a clear and documented exception when the CsrfTokenManager is not set. We can then quietly consider an improvement to the JS rest client to allow it to work without a csrf token (not supported at the moment). |
Ok, I guess csrf should only be allowed to be disabled for basic auth mode, but we don't support that from ui atm so not applicable. Better error reporting, in this case especially on logging page is indeed missing piece here: https://jira.ez.no/browse/EZP-25344 |
When CSRF protection is disabled with
framework.csrf_protection.enabled
flag set to false, site crashes with an exception about missingsecurity.csrf.token_manager
service.Depends on: ezsystems/ezpublish-kernel#1589