Skip to content
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.

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

[SecurityBundle] Passwords are not encoded when algorithm set to "true" #34738

Conversation

@nieuwenhuisen
Copy link
Contributor

nieuwenhuisen commented Dec 1, 2019

Q A
Branch? 3.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #34725
License MIT
Doc PR -

If the algorithm is set to true, password will be encode as plain password.

security:
    encoders:
        App\User\User:
            algorithm: true

The reason for this is the not strict comparison of php switches.

switch ($config['algorithm']) {
            case 'plaintext':
}

true == 'plaintext' is true, so the first case is hit. My first solution was to cast the algorithm to a string, to prevent this. After some feedback I have catch this problem earlier and does not allow true as valid value to the algorithm option.

Ps. This is my first PR for Symfony, any feedback is welcome :-)!

@derrabus

This comment has been minimized.

Copy link
Contributor

derrabus commented Dec 1, 2019

Can we catch this earlier? imho, algorithm: true shouldn't pass the config validation.

@nieuwenhuisen

This comment has been minimized.

Copy link
Contributor Author

nieuwenhuisen commented Dec 2, 2019

Sounds reasonable. I will take a look at the config validation.

@nicolas-grekas nicolas-grekas added this to the 4.3 milestone Dec 2, 2019
@nicolas-grekas nicolas-grekas changed the base branch from master to 4.3 Dec 2, 2019
@nieuwenhuisen nieuwenhuisen force-pushed the nieuwenhuisen:fix_algorithm_true_converts_to_plain_password_encoder branch from c2926d9 to cdb0b49 Dec 2, 2019
@nieuwenhuisen

This comment has been minimized.

Copy link
Contributor Author

nieuwenhuisen commented Dec 2, 2019

I have reset my previous updates and change the configuration validation.
Now is true not allowed as algorithm config value.

@nieuwenhuisen nieuwenhuisen changed the title [Security] Passwords are not encoded when algorithm set to "true" [SecurityBundle] Passwords are not encoded when algorithm set to "true" Dec 2, 2019
@nieuwenhuisen nieuwenhuisen requested a review from nicolas-grekas Dec 3, 2019
@xabbuh
xabbuh approved these changes Dec 3, 2019
@chalasr
chalasr approved these changes Dec 3, 2019
@chalasr chalasr force-pushed the nieuwenhuisen:fix_algorithm_true_converts_to_plain_password_encoder branch from 851ffb9 to d00464f Dec 3, 2019
@chalasr chalasr modified the milestones: 4.3, 3.4 Dec 3, 2019
@chalasr chalasr force-pushed the nieuwenhuisen:fix_algorithm_true_converts_to_plain_password_encoder branch from d00464f to cb429cd Dec 3, 2019
@chalasr chalasr changed the base branch from 4.3 to 3.4 Dec 3, 2019
@chalasr chalasr force-pushed the nieuwenhuisen:fix_algorithm_true_converts_to_plain_password_encoder branch from 5d593d5 to 83a5517 Dec 3, 2019
@chalasr

This comment has been minimized.

Copy link
Member

chalasr commented Dec 3, 2019

Rebased on 3.4 since it applies there. Congratz for your first contrib!

@chalasr

This comment has been minimized.

Copy link
Member

chalasr commented Dec 3, 2019

Thank you @nieuwenhuisen.

chalasr added a commit that referenced this pull request Dec 3, 2019
…set to "true" (nieuwenhuisen)

This PR was merged into the 3.4 branch.

Discussion
----------

[SecurityBundle] Passwords are not encoded when algorithm set to "true"

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Tickets       | Fix #34725
| License       | MIT
| Doc PR        | -

If the algorithm is set to `true`, password will be encode as plain password.

```
security:
    encoders:
        App\User\User:
            algorithm: true
```

The reason for this is the not strict comparison of php switches.

```
switch ($config['algorithm']) {
            case 'plaintext':
}
```

`true == 'plaintext'` is `true`, so the first case is hit. My first solution was to cast the algorithm to a string, to prevent this. After some feedback I have catch this problem earlier and does not allow true as valid value to the algorithm option.

Ps. This is my first PR for Symfony, any feedback is welcome :-)!

Commits
-------

83a5517 [SecurityBundle] Passwords are not encoded when algorithm set to \"true\"
@chalasr chalasr merged commit 83a5517 into symfony:3.4 Dec 3, 2019
1 of 3 checks passed
1 of 3 checks passed
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details
@mhujer

This comment has been minimized.

Copy link
Contributor

mhujer commented Dec 3, 2019

@nieuwenhuisen Thanks for fixing it! 👍

@nieuwenhuisen nieuwenhuisen deleted the nieuwenhuisen:fix_algorithm_true_converts_to_plain_password_encoder branch Dec 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.