-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Security][SecurityBundle] Show user account status errors #58300
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
Conversation
80060ea
to
71408c1
Compare
From #56830 (review)
@nicolas-grekas unless we decide to revert a security fix we cannot avoid the option I think |
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.
We need an entry in both SecurityBundle and Security\Http's CHANGELOG files
src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php
Outdated
Show resolved
Hide resolved
return true; | ||
} | ||
|
||
if ($this->showAccountStatusExceptions) { |
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.
I would expect CustomAuthenticationMessageExceptions
to show up even if showAccountStatusExceptions
is false, as it's the case today with hideUserNotFoundExceptions
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.
Not sure how to deal with the CustomAuthenticationMessageExceptions
as this is no account specific exception (extends AccountStatusException
), but a general authentication exception (extends AuthenticationException
).
The new key is named hide_account_status_exceptions
, so this might be confusing.
src/Symfony/Bundle/SecurityBundle/Resources/config/security_authenticator.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Http/Authentication/AuthenticatorManager.php
Outdated
Show resolved
Hide resolved
Maybe an unpopular opinion: I'm not sure if I would implement this feature in Symfony at all. This is part of code that was released with the user enumeration CVE. With the new option set to true, you'll reintroduce the possibility of user enumeration based on error messages as you now get "Invalid credentials" (user does not exists) or "This account is locked" (account status error). The I'm not convinced we need to provide an extra option to achieve this (especially because we consider the new feature less secure). |
I agree, in theory you could use this to enumerate all users that have an account status error, but user does not exist errors are still silenced with solution. But you may also enumerate users by the time it takes to generate the response at all, if you want to use all possible ways ;) The current solution looks broken to me. You either expose ALL user information or no user feedback at all. There is no way to inform the user, that it is currently not possible to login with the user. The current solution resulted in several service desk tickets that I need to check every time. What about creating a new enum config key with three different states like: |
Renaming/changing the existing option looks sensible to me as account status errors means that a user is actually found. |
c16c852
to
b5b742d
Compare
Changed the config key to |
6792899
to
6347ad7
Compare
Anything left to do @chalasr ? |
Sorry for the back and forths but I think there is confusion about my last comment:
I agree with Wouter this is not worth a dedicated option, but I do think it's worth renaming the existing |
So you're fine with deprecating the existing And if you are using the new |
Yes. The new option should win over the to-be-deprecated one when it's explicitly set (to not change the behavior of apps explicitly setting the old option to false). The new option should default to @wouterj are you ok with the proposed plan? |
Ok, let's go with an enum option with these conditions (maybe worth calling the middle one I guess the easiest BC way would be removing the default value from the current option adding adding the new option with an |
Sounds good 👍 |
6347ad7
to
ac57b42
Compare
src/Symfony/Bundle/SecurityBundle/DependencyInjection/MainConfiguration.php
Outdated
Show resolved
Hide resolved
src/Symfony/Bundle/SecurityBundle/DependencyInjection/MainConfiguration.php
Outdated
Show resolved
Hide resolved
src/Symfony/Bundle/SecurityBundle/DependencyInjection/MainConfiguration.php
Outdated
Show resolved
Hide resolved
src/Symfony/Bundle/SecurityBundle/DependencyInjection/SecurityExtension.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Http/Authentication/AuthenticatorManager.php
Outdated
Show resolved
Hide resolved
src/Symfony/Bundle/SecurityBundle/DependencyInjection/MainConfiguration.php
Show resolved
Hide resolved
src/Symfony/Bundle/SecurityBundle/DependencyInjection/MainConfiguration.php
Show resolved
Hide resolved
5222ad3
to
6baf154
Compare
Thanks for the review @stof :) Applied your suggestions and the build is passing |
6baf154
to
f9b8d7e
Compare
@core23 Can you please update the UPGRADE-7.3 file about the deprecated option? And add a line about that deprecation in the already updated CHANGELOG file also? |
f9b8d7e
to
011efe1
Compare
Done :) |
011efe1
to
a565f64
Compare
Any chance to get this merge soon-ish? @chalasr |
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.
Can you please rebase?
a565f64
to
7538e44
Compare
Done. The failing tests look unrelated to the changes |
7538e44
to
5a11de5
Compare
Thank you @core23. |
According to the
hideUserNotFoundExceptions
flag, onlyUserNotFoundException
should be hidden, but the implementation was also silencing account specific errors.To fix the issue in a BC way, a new
hide_account_status_exceptions
config key was introduced to show account exceptions.This key should be changed to false with an upcoming version (7.3 or 7.4?), so that the existing
hide_user_not_found
config key works as intended with the upcoming symfony 8 release.