[DoctrineBridge][Security][Validator] do not validate empty values #23341

xabbuh · 2017-06-30T18:38:18Z

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

Nearly all validators operating on scalar values (except for some special constraints) do ignore empty values. If you want to forbid them, you have to use the NotBlank constraint instead.

szymach · 2017-06-30T22:28:04Z

Very nice, but what about UniqueEntityValidator? If the $entity variable is a null, Doctrine will throw an exception when attempting to load metatada for that value.

xabbuh · 2017-07-01T09:15:44Z

Will the UniqueEntityValidator ever be triggered in that case given that the UniqueEntity constraint is added to a class but not a single property?

szymach · 2017-07-01T11:33:58Z

@xabbuh Make a form with an entity as data_class, set the UniqueEntityValidator constraint in the form constraints option, then empty_data to null and it should fail.

fabpot · 2017-07-03T07:38:59Z

Thank you @xabbuh.

…y values (xabbuh) This PR was merged into the 2.7 branch. Discussion ---------- [DoctrineBridge][Security][Validator] do not validate empty values | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #23319 | License | MIT | Doc PR | Nearly all validators operating on scalar values (except for some special constraints) do ignore empty values. If you want to forbid them, you have to use the `NotBlank` constraint instead. Commits ------- fd7ad23 do not validate empty values

craue · 2017-07-03T19:16:42Z

src/Symfony/Component/Security/Core/Validator/Constraints/UserPasswordValidator.php

@@ -39,6 +39,10 @@ public function validate($password, Constraint $constraint)
            throw new UnexpectedTypeException($constraint, __NAMESPACE__.'\UserPassword');
        }

+        if (null === $password || '' === $password) {
+            return;


This change breaks FOSUserBundle's current_password field used in ChangePasswordFormType and ProfileFormType allowing to send empty passwords instead of the user's current one.

Well they would receive an exception, so I am not sure that is a proper way of handling the case anyway.

I used to get the proper constraint message rendered with the form, no exception.

Probably because you ran it on PHP lower than 5.6, before function hash_equals was used. If that gets a null in comparison, it breaks.

Here to be specfic.

Huh, you are right, 2.8 is the first version that uses hash_equals. Not sure how this should be resolved though. Should not you use NotBlank validator anyway?

Well this change only standardizes the usage of the validator, since it already ignored empty strings. It is pretty obvious a null cannot be a password, so I would simply do a fix on the side of FOSUB

Now if using FOSUserBundle's ChangePasswordFormType, then logged in users can change their own password without typing the current password. I created a pull request:
FriendsOfSymfony/FOSUserBundle#2582

The PR really shouldn't have been merged as-is in older branches due to this BC break.

I still believe that this is an implementation error on FOS side, not a BC break. Empty values should be checked by a specific validator, not by one which by default should not handle them (as I have said before, a null can not be a password).

lstrojny · 2017-07-14T11:51:59Z

This is quite a dangerous change: when you validate a users current password and you do not specify a NotBlack constraint additionally to the UserPassword constraint, entering no password will pass validation. This change deserves at least a big fat warning in the upgrade guide.

beberlei · 2017-07-14T12:07:05Z

The type is called UserPassword, I would argue there is no such thing as an empty password, so how other fields work should not be relevant in this use case.

I don't agree about "at least a big fat warning", I would consider this a security problem and would prefer a revert at least on the UserPassword validator related lines.

lstrojny · 2017-07-14T12:31:32Z

@beberlei Indeed. The more I think about this the more I consider this a serious security problem.

xabbuh · 2017-07-14T14:04:45Z

see #23507

This PR was merged into the 2.7 branch. Discussion ---------- [Security] validate empty passwords again | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #23341 (comment) | License | MIT | Doc PR | It looks like this part of #23341 causes serious security issues for some users who rely on the validator to also compare the empty string with their user's password (see for example #23341 (comment)). Thus I suggest to revert this part of #23341. Commits ------- 878198c [Security] validate empty passwords again

This PR was merged into the 2.7 branch. Discussion ---------- [Security] validate empty passwords again | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | symfony/symfony#23341 (comment) | License | MIT | Doc PR | It looks like this part of #23341 causes serious security issues for some users who rely on the validator to also compare the empty string with their user's password (see for example symfony/symfony#23341 (comment)). Thus I suggest to revert this part of #23341. Commits ------- 878198cefa [Security] validate empty passwords again

smcjones · 2017-11-22T21:45:32Z

It would be nice to have the option to validate or not validate on empty data, leaving it to the developer. For example, nullable=true|false. Then there is no BC break but there is a new feature I could really use.

xabbuh · 2017-11-22T22:09:17Z

Feel free to open an issue and such a feature could be discussed.

vaibhavpandeyvpz · 2020-06-25T02:00:34Z

This line https://github.com/symfony/security-core/blob/master/Validator/Constraints/UserPasswordValidator.php#L43 takes out the freedom unlike other constraints where I would have added a NotBlank when required. It forces the current password to be not-empty and valid even when it's not needed. For example, I have a profile update form with other fields including 3 fields (current password, new password, confirm password) for password change. I only wish to require presence of value in current password when new password is not empty but the UserPassword is hard-coded against regular Symfony validation conventions.

xabbuh added this to the 2.7 milestone Jun 30, 2017

carsonbot added Status: Needs Review Security Validator Bug labels Jun 30, 2017

xabbuh mentioned this pull request Jun 30, 2017

Do not validate null values in UserPassword\UniqueEntity validators #23319

Closed

ogizanagi approved these changes Jun 30, 2017

View reviewed changes

Simperfit approved these changes Jul 1, 2017

View reviewed changes

xabbuh changed the title ~~[Security][Validator] do not validate empty values~~ [DoctrineBridge][Security][Validator] do not validate empty values Jul 2, 2017

xabbuh force-pushed the issue-23319 branch from 010c61a to 14d9d10 Compare July 2, 2017 07:02

do not validate empty values

fd7ad23

xabbuh force-pushed the issue-23319 branch from 14d9d10 to fd7ad23 Compare July 2, 2017 07:03

fabpot changed the base branch from master to 2.7 July 3, 2017 07:35

fabpot approved these changes Jul 3, 2017

View reviewed changes

fabpot merged commit fd7ad23 into symfony:2.7 Jul 3, 2017

xabbuh deleted the issue-23319 branch July 3, 2017 08:00

fabpot mentioned this pull request Jul 3, 2017

Release v2.7.30 #23361

Merged

craue reviewed Jul 3, 2017

View reviewed changes

craue mentioned this pull request Jul 3, 2017

the current user's password can be erroneously empty FriendsOfSymfony/FOSUserBundle#2580

Closed

This was referenced Jul 4, 2017

Release v2.8.23 #23367

Merged

Release v3.2.10 #23368

Merged

Release v3.3.3 #23369

Merged

This was referenced Jul 5, 2017

Validates empty current password for ChangePasswordFormType FriendsOfSymfony/FOSUserBundle#2582

Merged

Validates empty current password for ProfileFormType FriendsOfSymfony/FOSUserBundle#2583

Merged

xabbuh mentioned this pull request Jul 14, 2017

[Security] validate empty passwords again #23507

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DoctrineBridge][Security][Validator] do not validate empty values #23341

[DoctrineBridge][Security][Validator] do not validate empty values #23341

xabbuh commented Jun 30, 2017

szymach commented Jun 30, 2017

xabbuh commented Jul 1, 2017

szymach commented Jul 1, 2017

fabpot commented Jul 3, 2017

craue Jul 3, 2017

szymach Jul 3, 2017

craue Jul 3, 2017

szymach Jul 3, 2017

szymach Jul 3, 2017

szymach Jul 3, 2017

szymach Jul 3, 2017

symfonyaml Jul 5, 2017 •

edited

craue Jul 5, 2017 •

edited

szymach Jul 5, 2017

lstrojny commented Jul 14, 2017

beberlei commented Jul 14, 2017

lstrojny commented Jul 14, 2017

xabbuh commented Jul 14, 2017

smcjones commented Nov 22, 2017 •

edited

xabbuh commented Nov 22, 2017

vaibhavpandeyvpz commented Jun 25, 2020

[DoctrineBridge][Security][Validator] do not validate empty values #23341

[DoctrineBridge][Security][Validator] do not validate empty values #23341

Conversation

xabbuh commented Jun 30, 2017

szymach commented Jun 30, 2017

xabbuh commented Jul 1, 2017

szymach commented Jul 1, 2017

fabpot commented Jul 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

symfonyaml Jul 5, 2017 • edited

Choose a reason for hiding this comment

craue Jul 5, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lstrojny commented Jul 14, 2017

beberlei commented Jul 14, 2017

lstrojny commented Jul 14, 2017

xabbuh commented Jul 14, 2017

smcjones commented Nov 22, 2017 • edited

xabbuh commented Nov 22, 2017

vaibhavpandeyvpz commented Jun 25, 2020

symfonyaml Jul 5, 2017 •

edited

craue Jul 5, 2017 •

edited

smcjones commented Nov 22, 2017 •

edited