Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[Security] AuthenticationUtils: fix returning coalesced value of LastUsername#47829
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
fabpot commentedOct 15, 2022
Can you tell us more about these "some sophisticated use cases"? |
sarbanha commentedOct 15, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I wish I could reveal part of the code but it breaches my client's NDA, I have two different form authenticators on one route and there are many constraints and business logic behind the scene. At some point I decided to rely on the "AuthenticationUtils" response when trying to retrieve the LastUsername value, after#42513 returning null values caused the code break at AuthenticationUtils.php. I found that the code tries to return blank string if the attribute doesn't exist in the session's attribute bag but it returns null value when the attribute was found with null value set. My PR suggests to convert probable null value to blank string before returning the LastUsername attribute. I hope that explains the issue better. |
xabbuh commentedOct 15, 2022
Do you write the value into the session yourself? Otherwise I fail to see how |
sarbanha commentedOct 15, 2022
I can check that, but IMAO the AuthenticationUtils is supposed to return a cleansed string value no matter what was found in session attribute bag, it should be either the last username in string or blank string, am I wrong? |
sarbanha commentedOct 15, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
checked that, yea, I collect a few more credential details and post them all to the app for authentication, so yep, I put it into session. |
wouterj commentedOct 17, 2022
Instead of writing Attributes starting with an underscore - like |
chalasr commentedOct 17, 2022
I'm going to close for the reason given by Wouter above. Thanks for proposing,@sarbanha |
sarbanha commentedOct 20, 2022 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Thank you all for your comments. I am going to ask to re-open this PR after closer review of my code and the communicated comments in here, One of my authenticators is OTP authenticator and doesn't have username field in the POST request, there are mobile number and OTP code plus csrf, when symfony starts the authentication process it internally adds I believe Symfony authentication mechanism looks for the username value to store in the This issue doesn't have any negative effect on the normal operation of web application since we don't usually enumerate the I believe there are two workarounds for this issue, first is that Core Authentication ignores to add |
chalasr commentedOct 20, 2022
Can you point out the code doing that? |
sarbanha commentedOct 24, 2022
I traced the issue, couldn't find it, it's weird! I will check the code deeper to figure out why this happens, if I found any clue, I will reply here |
In some sophisticated use cases, there might be conditions within which the Session object HAS LastUsername in the attribute bag but the value is null. I believe "AuthenticationUtils" is responsible to properly coalesce the retrieved value and turn it to a blank string if it's null. This PR responds to this issue.