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][Http][SwitchUserListener] Ignore all non existent username protection errors#36223

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

Conversation

@fancyweb
Copy link
Contributor

@fancywebfancyweb commentedMar 26, 2020
edited
Loading

QA
Branch?4.4
Bug fix?yes
New feature?no
Deprecations?no
Tickets#36174
LicenseMIT
Doc PR-

Since we generate the non existent username blindly, it can lead to Doctrine exceptions or any other exception.

We can catch all exceptions here but I guess it reduces the protection since the SQL query was not executed?

Alternative: we can only catch Doctrine DriverException (in addition to the existing AuthenticationException) and only silent the reported error codes?

@nicolas-grekas
Copy link
Member

nicolas-grekas commentedMar 26, 2020
edited
Loading

The driver/authenticator should be updated instead to me - it should throw an AuthenticationException

fancyweb reacted with thumbs up emoji

@nicolas-grekasnicolas-grekas added this to the4.4 milestoneMar 26, 2020
@fancywebfancyweb changed the base branch from4.4 to3.4March 26, 2020 17:20
@fancywebfancywebforce-pushed thesecurity-switch-user-listener-fix branch fromca680e1 to3843cdeCompareMarch 26, 2020 17:20
@fancywebfancyweb changed the title[Security][Http][SwitchUserListener] Ignore all non existent username protection errors[DoctrineBridge] Throw AuthenticationException if the user entity cannot be fetched at allMar 26, 2020
Copy link
Member

@nicolas-grekasnicolas-grekas left a comment
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

(once fabbot's report is fixed) <= false positive

xabbuh
xabbuh previously approved these changesMar 27, 2020
@derrabus
Copy link
Member

This does not feel right.DriverException could mean virtually anything. The database could be down, the table structure could be broken, … Those are hard errors that I would expect to result in an HTTP 500 error.

@fancyweb
Copy link
ContributorAuthor

fancyweb commentedMar 27, 2020
edited
Loading

Should we narrow to some specific error codes then?

@xabbuh
Copy link
Member

That's indeed a good point. What about moving thetrycatch to that particular statement inSwitchUserListener instead?

@fancyweb
Copy link
ContributorAuthor

That's indeed a good point. What about moving the try catch to that particular statement in SwitchUserListener instead?

That's what I did in the first place, until Nicolas' comment.

@nicolas-grekas
Copy link
Member

That's what I did in the first place, until Nicolas' comment.

I confirm: the issue is in the Doctrine bridge, not in theSwitchUserListener (which doesn't know anything bout databases.)

@wouterj
Copy link
Member

Hmm, I would prefer to change this innercatch() to catch all exceptions:

try {
$user =$this->provider->loadUserByUsername($username);
try {
$this->provider->loadUserByUsername($nonExistentUsername);
}catch (AuthenticationException$e) {
}
}catch (AuthenticationException$e) {
$this->provider->loadUserByUsername($currentUsername);
throw$e;
}

Putting this in theEntityUserProvider is too generic, as@derrabus pointed out. The doctrine error can be anything and we only want to ignore these errors for the fake username.

The firstloadUserByUsername call does only catch authentication exceptions. Thus any db error other than invalid username type is already catched by the first call. The second call thus only fail when the username is of the wrong type afaics.

xabbuh reacted with thumbs up emoji

@nicolas-grekas
Copy link
Member

The first loadUserByUsername call does only catch authentication exceptions. Thus any db error other than invalid username type is already catched by the first call. The second call thus only fail when the username is of the wrong type afaics.

OK, good argument! Let's revert to your initial proposal then@fancyweb, and sorry for the confusion :)

wouterj and xabbuh reacted with thumbs up emoji

@fancywebfancywebforce-pushed thesecurity-switch-user-listener-fix branch froma874f45 to42311d5CompareApril 1, 2020 09:17
@fancywebfancyweb requested a review fromsroze as acode ownerApril 1, 2020 09:17
@fancywebfancyweb changed the base branch from3.4 to4.4April 1, 2020 09:17
@fancyweb
Copy link
ContributorAuthor

I reverted to the initial change (on 4.4). Sorry for the massive ping 😅

xabbuh reacted with thumbs up emoji

@fancywebfancyweb changed the title[DoctrineBridge] Throw AuthenticationException if the user entity cannot be fetched at all[Security][Http][SwitchUserListener] Ignore all non existent username protection errorsApr 1, 2020
@nicolas-grekas
Copy link
Member

Thank you@fancyweb.

@nicolas-grekasnicolas-grekas merged commit15edfd3 intosymfony:4.4Apr 1, 2020
@fancywebfancyweb deleted the security-switch-user-listener-fix branchApril 1, 2020 09:29
This was referencedApr 28, 2020
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@nicolas-grekasnicolas-grekasnicolas-grekas approved these changes

@chalasrchalasrchalasr approved these changes

@xabbuhxabbuhxabbuh left review comments

@dunglasdunglasAwaiting requested review from dunglas

@lyrixxlyrixxAwaiting requested review from lyrixx

@srozesrozeAwaiting requested review from sroze

Assignees

No one assigned

Projects

None yet

Milestone

3.4

Development

Successfully merging this pull request may close these issues.

7 participants

@fancyweb@nicolas-grekas@derrabus@xabbuh@wouterj@chalasr@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp