🌐 AI搜索 & 代理 主页
Skip to content

[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.

Sign up for GitHub

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

Jump to bottom
Merged
chalasr merged 1 commit into from
Jan 31, 2025
Merged

[Security][SecurityBundle] Show user account status errors #58300

chalasr merged 1 commit into from
Jan 31, 2025

Conversation

@core23
Copy link
Contributor

@core23 core23 commented Sep 18, 2024 •
edited by OskarStark
Loading

Q A
Branch? 7.3
Bug fix? no
New feature? yes
Deprecations? no
Issues Fix #50028
License MIT

According to the hideUserNotFoundExceptions flag, only UserNotFoundException 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.

@core23 core23 requested a review from chalasr as a code owner September 18, 2024 06:02
@carsonbot carsonbot added this to the 7.2 milestone Sep 18, 2024
@core23 core23 mentioned this pull request Sep 18, 2024
Closed
@core23 core23 force-pushed the fix-auth-exception-handling-72 branch 3 times, most recently from 80060ea to 71408c1 Compare September 18, 2024 06:12
@chalasr
Copy link
Member

chalasr commented Sep 18, 2024

From #56830 (review)

This should target 7.2 as this is a new feature.
@chalasr can you please have another look?
I don't like adding new options so if there's a way without, suggestions welcome :)

@nicolas-grekas unless we decide to revert a security fix we cannot avoid the option I think

chalasr
chalasr requested changes Sep 18, 2024
View reviewed changes
Copy link
Member

@chalasr chalasr left a 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/Component/Security/Http/Authentication/AuthenticatorManager.php Outdated
return true;
}

if ($this->showAccountStatusExceptions) {
Copy link
Member

@chalasr chalasr Sep 18, 2024

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

Copy link
Contributor Author

@core23 core23 Sep 20, 2024

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.

@wouterj
Copy link
Member

wouterj commented Sep 18, 2024 •
edited
Loading

I don't like adding new options so if there's a way without, suggestions welcome :)

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 hide_user_not_found option name might be a bit of a vague name, but it determines whether you want some default user enumeration prevention or not. If a business decides it wants partial user enumeration (by hiding only user not found exceptions), you can already achieve this by setting hide_user_not_found: false and implementing your own authentication failure handler that hides only UserNotFoundException.

I'm not convinced we need to provide an extra option to achieve this (especially because we consider the new feature less secure).

@core23
Copy link
Contributor Author

core23 commented Sep 18, 2024

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).

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: expose_security_errors: none|account|all?

@chalasr
Copy link
Member

chalasr commented Sep 18, 2024

Renaming/changing the existing option looks sensible to me as account status errors means that a user is actually found.

@carsonbot carsonbot changed the title Show user account status errors [Security][SecurityBundle] Show user account status errors Sep 19, 2024
@core23 core23 force-pushed the fix-auth-exception-handling-72 branch 2 times, most recently from c16c852 to b5b742d Compare September 20, 2024 19:17
@core23
Copy link
Contributor Author

core23 commented Sep 20, 2024

Changed the config key to hide_account_status_exceptions and applied the proposed changes.

@core23 core23 force-pushed the fix-auth-exception-handling-72 branch 2 times, most recently from 6792899 to 6347ad7 Compare September 20, 2024 19:27
@core23
Copy link
Contributor Author

core23 commented Oct 8, 2024

Anything left to do @chalasr ?

@chalasr
Copy link
Member

chalasr commented Oct 22, 2024

Sorry for the back and forths but I think there is confusion about my last comment:

Renaming/changing the existing option looks sensible to me as account status errors means that a user is actually found.

I agree with Wouter this is not worth a dedicated option, but I do think it's worth renaming the existing hide_user_not_found_exceptions option for clarity to make it take an enum as you proposed in #58300 (comment). It should still handle true/false and behave the same as today for BC until 8.0.

@core23
Copy link
Contributor Author

core23 commented Oct 24, 2024

So you're fine with deprecating the existing hide_user_not_found_exceptions option in favor of something like expose_security_errors with the states none, account , all?

And if you are using the new option, it should override the hide_user_not_found_exceptions setting?

@chalasr
Copy link
Member

chalasr commented Oct 25, 2024

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 all to keep the same default behavior as today.

@wouterj are you ok with the proposed plan?

@wouterj
Copy link
Member

wouterj commented Oct 25, 2024 •
edited
Loading

Ok, let's go with an enum option with these conditions (maybe worth calling the middle one account_status to be super clear on what errors are exposed).

I guess the easiest BC way would be removing the default value from the current option adding adding the new option with an 'all' default value. Then, in the extension use the old option if it's defined (which means the user has configured it explicitly) + a deprecation notice. Otherwise - if the old option was not configured - use the new option.
When the old option is used, the extension can then transform true to 'all' and false to 'none'.

@chalasr
Copy link
Member

chalasr commented Oct 26, 2024

Sounds good 👍

@core23 core23 force-pushed the fix-auth-exception-handling-72 branch from 6347ad7 to ac57b42 Compare October 29, 2024 16:12
@core23 core23 force-pushed the fix-auth-exception-handling-72 branch from 5222ad3 to 6baf154 Compare December 15, 2024 14:18
@core23
Copy link
Contributor Author

core23 commented Dec 15, 2024

Thanks for the review @stof :)

Applied your suggestions and the build is passing

@core23 core23 requested review from chalasr and stof December 16, 2024 13:35
@core23 core23 force-pushed the fix-auth-exception-handling-72 branch from 6baf154 to f9b8d7e Compare January 5, 2025 11:10
@chalasr
Copy link
Member

chalasr commented Jan 5, 2025

@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?

@core23 core23 force-pushed the fix-auth-exception-handling-72 branch from f9b8d7e to 011efe1 Compare January 6, 2025 16:59
@core23
Copy link
Contributor Author

core23 commented Jan 6, 2025

Done :)

chalasr
chalasr reviewed Jan 6, 2025
View reviewed changes
@core23 core23 force-pushed the fix-auth-exception-handling-72 branch from 011efe1 to a565f64 Compare January 7, 2025 07:13
@core23
Copy link
Contributor Author

core23 commented Jan 30, 2025 •
edited
Loading

Any chance to get this merge soon-ish? @chalasr

chalasr
chalasr approved these changes Jan 31, 2025
View reviewed changes
Copy link
Member

@chalasr chalasr left a 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?

@core23 core23 force-pushed the fix-auth-exception-handling-72 branch from a565f64 to 7538e44 Compare January 31, 2025 13:00
@core23
Copy link
Contributor Author

core23 commented Jan 31, 2025

Done. The failing tests look unrelated to the changes

@core23 core23 requested a review from chalasr January 31, 2025 13:07
@chalasr chalasr force-pushed the fix-auth-exception-handling-72 branch from 7538e44 to 5a11de5 Compare January 31, 2025 13:26
@chalasr
Copy link
Member

chalasr commented Jan 31, 2025

Thank you @core23.

@chalasr chalasr merged commit a7a32f7 into symfony:7.3 Jan 31, 2025
8 of 11 checks passed
@chalasr chalasr mentioned this pull request Jan 31, 2025
Closed
@core23 core23 deleted the fix-auth-exception-handling-72 branch January 31, 2025 14:13
@fabpot fabpot mentioned this pull request May 2, 2025
Merged
@chalasr chalasr mentioned this pull request Jun 28, 2025
Merged
nicolas-grekas added a commit that referenced this pull request Jun 29, 2025
@nicolas-grekas
minor #60941 [SecurityBundle] Deprecate the `security.authentication.…
c50f247 
…hide_user_not_found` parameter (chalasr)

This PR was merged into the 7.4 branch.

Discussion
----------

[SecurityBundle] Deprecate the `security.authentication.hide_user_not_found` parameter

| Q             | A
| ------------- | ---
| Branch?       | 7.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | yes
| Issues        | -
| License       | MIT

We missed deprecating this unused parameter in #58300

Commits
-------

40a2d2f [SecurityBundle] Deprecate the `security.authentication.hide_user_not_found` parameter
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@stof stof Awaiting requested review from stof

@chalasr chalasr Awaiting requested review from chalasr chalasr is a code owner

Assignees

No one assigned

Labels

Feature Security SecurityBundle Status: Needs Review

Projects

None yet

Milestone

7.3

Development

Successfully merging this pull request may close these issues.

Locked account produces "Invalid credentials" message

7 participants

@core23 @chalasr @wouterj @stof @fabpot @OskarStark @carsonbot