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 theNotBlank constraint instead.

xabbuh added this to the2.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

Copy link

szymach commentedJun 30, 2017

Very nice, but what aboutUniqueEntityValidator? If the$entity variable is anull, Doctrine will throw an exception when attempting toload metatada for that value.

Copy link

MemberAuthor

xabbuh commentedJul 1, 2017

Will theUniqueEntityValidator ever be triggered in that case given that theUniqueEntity constraint is added to a class but not a single property?

Simperfit approved these changes

Jul 1, 2017

View reviewed changes

Copy link

szymach commentedJul 1, 2017

@xabbuh Make a form with an entity asdata_class, set theUniqueEntityValidator constraint in the formconstraints option, thenempty_data tonull and it should fail.

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 theissue-23319 branch from010c61a to14d9d10Compare

July 2, 2017 07:02

do not validate empty values

fd7ad23

xabbuh force-pushed theissue-23319 branch from14d9d10 tofd7ad23Compare

July 2, 2017 07:03

fabpot changed the base branch frommaster to2.7

July 3, 2017 07:35

fabpot approved these changes

Jul 3, 2017

View reviewed changes

Copy link

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This change breaks FOSUserBundle'scurrent_password field used inChangePasswordFormType andProfileFormType allowing to send empty passwords instead of the user's current one.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Here to be specfic.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Contributor

symfonyamlJul 5, 2017•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

Contributor

craueJul 5, 2017•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

szymachJul 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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, anull can not be a password).

craue mentioned this pull request

Jul 3, 2017

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

Closed

This was referencedJul 4, 2017

Release v2.8.23#23367

Merged

Release v3.2.10#23368

Merged

Release v3.3.3#23369

Merged

This was referencedJul 5, 2017

Validates empty current password for ChangePasswordFormTypeFriendsOfSymfony/FOSUserBundle#2582

Merged

Validates empty current password for ProfileFormTypeFriendsOfSymfony/FOSUserBundle#2583

Merged

Copy link

Contributor

lstrojny commentedJul 14, 2017

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

Copy link

Contributor

beberlei commentedJul 14, 2017

The type is calledUserPassword, 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.

Copy link

Contributor

lstrojny commentedJul 14, 2017

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

xabbuh mentioned this pull request

Jul 14, 2017

[Security] validate empty passwords again#23507

Merged

Copy link

MemberAuthor

xabbuh commentedJul 14, 2017

see#23507

fabpot added a commit that referenced this pull request

Jul 17, 2017

security#23507[Security] validate empty passwords again (xabbuh)

559ccb2

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

symfony-splitter pushed a commit to symfony/security-core that referenced this pull request

Jul 17, 2017

security #23507 [Security] validate empty passwords again (xabbuh)

e81d640

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 examplesymfony/symfony#23341 (comment)). Thus I suggest to revert this part of #23341.Commits-------878198cefa [Security] validate empty passwords again

symfony-splitter pushed a commit to symfony/security that referenced this pull request

Jul 17, 2017

security #23507 [Security] validate empty passwords again (xabbuh)

78ae1f8

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 examplesymfony/symfony#23341 (comment)). Thus I suggest to revert this part of #23341.Commits-------878198cefa [Security] validate empty passwords again

Copy link

smcjones commentedNov 22, 2017•
edited
Loading

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.

Copy link

MemberAuthor

xabbuh commentedNov 22, 2017

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

Copy link

vaibhavpandeyvpz commentedJun 25, 2020

This linehttps://github.com/symfony/security-core/blob/master/Validator/Constraints/UserPasswordValidator.php#L43 takes out the freedom unlike other constraints where I would have added aNotBlank 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 theUserPassword is hard-coded against regular Symfony validation conventions.