[Security][Http][SwitchUserListener] Ignore all non existent username protection errors

[fancyweb]

Q	A
Branch?	4.4
Bug fix?	yes
New feature?	no
Deprecations?	no
Tickets	#36174
License	MIT
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]

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

[nicolas-grekas]

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

[derrabus]

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]

Should we narrow to some specific error codes then?

[xabbuh]

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

see #36223 (comment)

[fancyweb]

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]

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

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

[wouterj]

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

symfony/src/Symfony/Component/Security/Http/Firewall/SwitchUserListener.php

Lines 156 to 167 in e0de6cc

	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 the EntityUserProvider 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 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.

[nicolas-grekas]

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

[fancyweb]

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

[nicolas-grekas]

Thank you @fancyweb.