Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Closed

Conversation

@sarbanha
Copy link
Contributor

QA
Branch?6.1
Bug fix?yes
New feature?no
Deprecations?no
TicketsFix #...
LicenseMIT
Doc PRsymfony/symfony-docs#...

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.

@fabpot
Copy link
Member

Can you tell us more about these "some sophisticated use cases"?

@sarbanha
Copy link
ContributorAuthor

sarbanha commentedOct 15, 2022
edited
Loading

Can you tell us more about these "some sophisticated use cases"?

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
Copy link
Member

Do you write the value into the session yourself? Otherwise I fail to see hownull values could end up there.

@sarbanha
Copy link
ContributorAuthor

Do you write the value into the session yourself? Otherwise I fail to see hownull values could end up there.

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
Copy link
ContributorAuthor

sarbanha commentedOct 15, 2022
edited
Loading

Do you write the value into the session yourself? Otherwise I fail to see hownull values could end up there.

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
Copy link
Member

Instead of writingnull to the session, I would remove the attribute from the session instead. Or, if I'm fully honest, I would advise to not write custom values to this attribute at all.

Attributes starting with an underscore - like_security.last_username - are meant for internal usage in Symfony. They are not meant to be written to outside the Symfony core, and are likely to cause more issues like this one.

chalasr and jvasseur reacted with thumbs up emoji

@chalasr
Copy link
Member

I'm going to close for the reason given by Wouter above. Thanks for proposing,@sarbanha

@sarbanha
Copy link
ContributorAuthor

sarbanha commentedOct 20, 2022
edited
Loading

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,
I have to correct my previous comment about storing attributes in_security.xxx attribute bag. I don't store any extra value in the security session.

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_security.last_username=null

I believe Symfony authentication mechanism looks for the username value to store in the_security.last_username .

This issue doesn't have any negative effect on the normal operation of web application since we don't usually enumerate the_security.xxxx values manually. But, it breaks the Symfony Profiler while retrieving list the session attributes.

I believe there are two workarounds for this issue, first is that Core Authentication ignores to add_security.last_username to the attribute bag of the session, second is to update the AuthenticationUtils to return coalesced value of LastUsername as this PR suggests.

@chalasr

@chalasr
Copy link
Member

when symfony starts the authentication process it internally adds _security.last_username=null

Can you point out the code doing that?

@sarbanha
Copy link
ContributorAuthor

when symfony starts the authentication process it internally adds _security.last_username=null

Can you point out the code doing that?

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

wouterj reacted with thumbs up emoji

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@wouterjwouterjAwaiting requested review from wouterjwouterj is a code owner

@chalasrchalasrAwaiting requested review from chalasrchalasr is a code owner

Assignees

No one assigned

Projects

None yet

Milestone

6.1

Development

Successfully merging this pull request may close these issues.

6 participants

@sarbanha@fabpot@xabbuh@wouterj@chalasr@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp