[Security] Ignore empty username or password login attempts

[llupa]

Q	A
Branch?	7.1
Bug fix?	no
New feature?	yes/no/improvement
Deprecations?	no
Issues	N/A
License	MIT

Context

A person going through Symfony docs for the first time wanted to create their own LoginFormType as a next step in their learning Symfony journey and noticed that you can submit empty username/password with form login.

They wanted to disallow this and tried to add validation. To validate a login form is not so straight forward as it either needs to be done with a custom authenticator (complex validation) or user provider if the data checks are simple.

After a simple discussion in Symfony slack with @wouterj I am opening this PR.

The approach to immediately ignore login attempts with empty username or empty password has already been introduced in Symfony 7.0 in this commit.

As always, I am happy to apply changes as requested!

[smnandre]

Could we leverage ParameterBag::getString() that already do the following checks ?

 if (!\is_scalar($value) && !$value instanceof \Stringable) {
 // throws

[stof]

@smnandre no, because this only allows getting a top-level key, while this code supports accessing a nested key.

[stof]

btw, the new logic added in this PR is not the type check (this was already present). It is the check for the empty string.

[smnandre]

@smnandre no, because this only allows getting a top-level key, while this code supports accessing a nested key.

Well this is a new discovery for me... :))

@stof WDYT: should this be mentionned in the documentation or is this is too "niche" usage ?

[wouterj]

Except from the one minor comment, this looks good to me!

Maybe this deserve a note in the UPGRADE-7.1.md file and the CHANGELOG file of the Security Http component? (or do we consider this too minor to mention?)

[llupa]

Except from the one minor comment, this looks good to me!

Maybe this deserve a note in the UPGRADE-7.1.md file and the CHANGELOG file of the Security Http component? (or do we consider this too minor to mention?)

I have to update some of the functional tests anyway. I can just add them 👍

[chalasr]

Maybe this deserve a note in the UPGRADE-7.1.md file and the CHANGELOG file of the Security Http component? (or do we consider this too minor to mention?)

Given the broken high-deps build, I wonder if this shouldn't even be done with a deprecation notice before making it throw in 8.0?

[wouterj]

Given the broken high-deps build, I wonder if this shouldn't even be done with a deprecation notice before making it throw in 8.0?

These are 3 tests submitting an empty login form to trigger a CSRF token error. This new condition now takes precedence, meaning it returns the wrong error.
I don't think that is something we have to worry about (in both situations, login errors), it rather reveals a bad test in our codebase. I can't think of a use-case that would result in success and will become a failure after this merge.

Besides, it's impossible to write a deprecation for this. We can only trigger the notice when someone actually logins in with an empty username/password. There is a very high likelihood that such deprecation will never be seen.

[llupa]

@wouterj Does your last comment mean that I can look into the CSRF test so it tests CSRF or wait first what is the consensus on this?

[wouterj]

@llupa let's fix the CSRF test (I hope it's just a matter of filling in a username and password).

I think we need consensus on whether we find this a hard BC break that deserves a smooth upgrade path, but the test need to be fixed whatever the conclusion (if we deprecate instead of throwing an error, we still need to fix the deprecation in that test),

[wouterj]

High deps tests are now green after your other PR is merged up.

Can you please solve the merge conflicts and rebase the PR's branch so the merge commit is removed from the PR diff?

[chalasr]

Worth a CHANGELOG entry to me

[llupa]

Worth a CHANGELOG entry to me

I added one and I think it is the correct place. If not, would you kindly me point where should it go?

[chalasr]

Thank you @llupa.

[J-Ben87]

Hi guys, I understand the will to prevent a useless authentication attempt when the username or password are empty, but now I'm getting a BadRequestHttpException which I can't use to customize the authentication error message.

Before, when the username or the password were empty, just like when they were invalid, I received an Invalid credentials error message which I could display to the user.
Now I'm getting an exception which is pretty irrelevant for the user attempting to log in.

Maybe an AuthenticationException would be a better exception? Otherwise what do you suggest?

[llupa]

This was done in line with JsonLoginAuthenticator.

I am not sure if sending an empty payload means that there was an error with the authentication process, when in fact this payload is bad.

[dgriffiths-lanentech]

I’m also seeing the same issue @J-Ben87 is experiencing.

I’ve recently created a new Symfony project on 7.1 and following the official documentation for security (which creates a very basic controller for logging in), it throws an exception when trying to log in using a space for the username and a space for the password.

Whilst I appreciate this is a bad payload, the code that the documentation tells new developers to implement, doesn’t handle the exception thrown gracefully. It doesn’t feel like the UX has been handled very well. Previously, it used to say “Invalid Credentials” but now throws an exception.

If this is the intended behaviour and won’t be adjusted, then the default implementation of the login controller doesn’t seem fit for purpose anymore, throwing an uncaught exception.

For someone who has followed the security documentation and has the default login controller code (I.e. just renders a template and has 2 addition lines of code, to fetch the username and error), what would you recommend we do? It seems the exception is thrown before the controller code is even hit. So do we need to extend the FormLoginAuthenticator? It doesn’t feel like the right solution

[wouterj]

The BadRequestHttpException in the current implementation treats an empty password equal to not having the password in the request at all. I think that is the most semantic and also equal to the behavior in other authenticators provided by the component.

The fix would be to have some client side validation before submitting the login form, e.g. adding the required attribute on the input element. I think it would be good to add this to the documentation (anyone up to do this?).

Also, is there a real world scenario where a user would have registered with the username being a single whitespace character? I feel like that is mostly an hypothetical scenario's that only occurs in local testing (and using anything other than a whitespace still works for these cases).

[llupa]

The fix would be to have some client side validation before submitting the login form, e.g. adding the required attribute on the input element. I think it would be good to add this to the documentation (anyone up to do this?).

I’ve recently created a new Symfony project on 7.1 and following the official documentation for security (which creates a very basic controller for logging in), it throws an exception when trying to log in using a space for the username and a space for the password.

@wouterj @dgriffiths-lanentech Do you have a quick reference where? I can definitely update the documentation.

[dgriffiths-lanentech]

@llupa Sure, here’s the documentation I followed which builds you the basic Login form.

Link: https://symfony.com/doc/current/security.html#form-login

Whilst I believe the client side fix will work, it still feels like there is still a gap in the server side logic. If JavaScript is turned off (and therefore client side validation turned off), the code would still result in this exception being thrown and stop us from gracefully handling the exception.

is there a real world scenario where a user would have registered with the username being a single whitespace character

No, there’s not. But in my opinion, it’s not far fetched to assume that someone may accidentally do this. With the current implementation, there seemingly isn’t a way to catch that exception and handle it gracefully, to improve the UX.

Documentation aside, I’d just be happy if someone with more knowledge, is able to share how we can gracefully handle this situation. The ability to catch this and handle this gracefully, seems to have been taken away (which is what I believe @J-Ben87 is alluding to).

[J-Ben87]

@dgriffiths-lanentech @wouterj Yes, I'm concerned that, in a "real world scenario", a user may submit an empty form accidentally (by pressing enter or clicking the submit button to fast), which would result in an ugly exception instead of just pointing him his mistake ("the username is empty", or at least "invalid credentials").

The fix would be to have some client side validation before submitting the login form

I'm not a huge fan of this: sometimes I'm asked by my customers to disable client side validation and rely only on server side validation. In this case we would still end up in that disappointing exception scenario 🙁

[Housik]

@llupa and @chalasr - are you crazy??? this edit should be rollbacked/edited as it totally wrong, because it is not following concept of Symfony Authentication architecture. Mainly, it throws HTTP exception type, instead of AUTHENTICATION exception type. There are more issues to discuss:

1. Object is called Authenticator - its role is NOT TO validate format of credentials, but AUTHENTICATE credentials - pls follows SINGLE OBJECT RESPONSIBILITY. If you need to validate format/value before authentication, there should be introduced new flow for login form validation outside this object (DTO object with Asserts or for example new validation subscriber type event).
2. In current state, if you decided to validate credentials, you should return BadCredentials exception with appropriate message/code - so it can be handled by current authentication flow (AuthenticationFailureHandlerInterface).

Usage of BadRequestHttpException in \Symfony\Component\Security\Http\Authenticator\JsonLoginAuthenticator makes sense, as it is used for APIs, where is no visual HTML output for consumer. But in LOGIN FORM, you can expect, that developer will need to transfrom exception to visual form error in HTML.

Now, every developer consuming this new code, has huge headache to catch BadRequestHttpException and display for user login form error in HTML template, because there is no simple mechanism to do it. Current mechanism is to leverage security.firewalls.[firewall].form_login.failure_handler, which can not be reached with your solution.

I suggest to replace each occurence of \Symfony\Component\HttpKernel\Exception\BadRequestHttpException with \Symfony\Component\Security\Core\Exception\BadCredentialsException - which is used almost in every other Authenticator in this namespace

[llupa]

Hey @Housik no one is mad? What do you mean?

I was working today after work to generate the necessary HTML (no JS needed) to stop users from accidentally submitting a bad request.

I had a look at all Authenticators anyway and this is what I found:

• AccessTokenAuthenticator will throw a BadCredentialsException when empty $accessToken
• FormLoginAuthenticator will throw a BadRequestHttpException when empty $username or $password
• HttpBasicAuthenticator no credential check whatsoever
• JsonLoginAuthenticator will throw a BadRequestHttpException when empty $username or $password
• LoginLinkAuthenticator will throw an InvalidLoginLinkAuthenticationException when empty $username
• RememberMeAuthenticator will throw a LogicException when empty $rawCookie
• RemoteUserAuthenticator will throw a BadCredentialsException when empty REMOTE_USER
• X509Authenticator will throw a BadCredentialsException when empty $username

While I do not appreciate the inferred need for urgency in the previous comments, I can relate with the need for cohesion.

From my personal POV, I don't want my user provider to do any sort of unnecessary lazy loading or storage querying, and I find the failure on the request payload completely acceptable just as it is for JsonLogin.

For now I will continue my work on the make command to add HTML validation for attributes, supported for a decade and more, and wait for maintainer feedback 👍

[wouterj]

Please take into account the Symfony Code of Conduct while interacting within the Symfony community. We are discussing technical things in an objective matter. There is no place here for calling people crazy.

[J-Ben87]

Besides the tone, I couldn't agree more on the technical points raised by @Housik.

We could discuss for hours of which exception is the most relevant to be thrown, but since throwing an exception outside of the AuthenticationException scope can not be handled gently, IMO there shouldn't be any discussion on that specific point.

Unless the authentication process provides a way to handle such exceptions of course, but I'm pretty sure it would be much more work and complexity than simply accepting that it's not the authenticators responsibility to decide that a missing password is an invalid behavior.

[J-Ben87]

@llupa I think we all agree that:

I don't want my user provider to do any sort of unnecessary lazy loading or storage querying, and I find the failure on the request payload completely acceptable

Only we would rather have the authenticator throw an \Symfony\Component\Security\Core\Exception\AuthenticationException (\Symfony\Component\Security\Core\Exception\BadCredentialsException for instance) rather than a BadRequestHttpException so that it can be handled properly by the authentication mechanisms.

[Housik]

@wouterj - I am very sorry for tone, I had very difficult work day and as a cherry on the top, I spent half day to find a temporary, not nice fix, because of this code breaking commit.

@llupa This change completely breaks the concept of FORM LOGIN logic, as it is designed in the code and in the documentation - brings non advertised breaking change of how the whole login form authentication concept works and the commit should not have been approved with wrong exception tiype.

The BAD CREDENTIALS exception belongs to the Symfony\Component\Security\Core\Exception namespace - the exceptions are the CORE part of business logic for SECURITY/AUTHENTICATION module.... and the BAD REQUEST HTTP EXCEPTION belongs to the Symfony\Component\HttpKernel\Exception - which covers the outer HTTP COMMNUNICATION layer , not the AUTHENTICATION business logic... so first throw the BAD CREDENTIALS... and at the outer layer pack it in anything you want - for example BAD REQUEST EXCEPTION.

Now the code mix two different business logics toghether - as we say, we are mixing pears with apples.

FormLoginAuthentication is not designed for REST API or lazy loading (see the FORM in the name) - so it should not have one day to start returns BAD REQUEST response. What I suppose, it is designed primary for static login form -> you submit it -> in kind of any problem with credentials you get BAD CREDENTAILS exception which you can convert in AuthenticationFailureHandlerInterface for anything you want - primary it is used to return HTML response with form validation error or redirection to default_target_path on success.

I think the following approach is more suitable for what is requested and it fully respects documentation and logic of SECURITY module:

1. Change type of exception to BAD CREDENTIALS EXCEPTION
2. Process it in a custom AuthenticationFailureHandlerInterface class and if required, throw here any kind of HTTP based exception.

There is no any sort of unnecessary lazy loading or storage querying, just different exception type.

This is the way, how we have to think about the programming - to respect business logic layers, framework modularity and do not try to mix everything into the one object....

[wouterj]

I can see the point of the current exception type not ending up in the authentication failure handler, which conflicts with the default way of displaying errors when using the form login authenticator and isn't easy to handle differently. I'm open to accept a PR changing the exception type to bad credentials if anyone is up to submitting a PR for this.

I do however completely disagree with all the talking about this check not being the responsibility of the authenticator. Checking credentials is part of authentication. The Symfony authenticator delegates most of this to listeners, but it's still the responsibility of an authenticator to turn an incoming request into a security passport that leads to a valid security token. An empty username and password is never valid and centralizing this validation in the authenticator seems like a very good solution to me.

Also, let me again say that all anyone in the Symfony community is doing is trying to improve the life of others. We might have a disagreement in what does and what doesn't count as an improvement, but I ask strongly to discuss this in a kind and objective way. There is no "you're thinking wrong, I'm thinking right" in programming. Big thanks to @llupa for pushing this improvement in the first place and thanks in advance to the next person for continuing it.

[llupa]

@wouterj I had the branch ready and can push in 5 minutes. Just to be sure, this is specifically for empty username and password? When username, password or csrf_token are not string / stringable the BadRequests remain, yes?

[Housik]

I do however completely disagree with all the talking about this check not being the responsibility of the authenticator.

Thank you guys for understanding the issue with exception. Now please let me try to explain my argument regarding validation:

My position: I agree, that into Authenticator the credentials has to enter validated for same basic rules (non-empty string, etc). BUT - it will be more convenient to put the validation code in a separate object to follow the programming rules and keep framework modular.

Why? Let me please explain:

• Imagine, that there are another authenticators and in every authenticator there is a need of credentials validation.
• Imagine more - there are another authenticators using exactly the same structure of credential = user id + password.
• Now, look into the authenticator namespace and you will find another Autheticators using the same structure of credentials - I see FormLoginAuthenticator, JsonLoginAuthenticator, HttpBasicAuthenticator, partly RemoteUserAuthenticator, partly X509Authenticator, partly LoginLinkAuthenticator
• with all those authenticators we can suppose, that they will be also glad to consume valid credentials
• so they need the validation -> so the validation logic should be shared -> so the validation code should not be repeated.

Now, please let me mention "Don't repeat yourself" (DRY)

"Don't repeat yourself" (DRY) is a principle of software development aimed at reducing repetition of information which is likely to change, replacing it with abstractions that are less likely to change, or using data normalization which avoids redundancy in the first place.

Mainly because of this principle, if there are two or more authenticators using the same validation logic, it would be optimal to introduce some kind of new layer/abstraction for validation, like the new CredentialsValidatorInterface object injected via contructor do the authenticator. It offers better framework modularity.

We can then go even further, introduce new configuration option for FormLoginAuthenticator like security.firewalls.[firewall].form_login.credentials_validator, so you can define your custom validator (otherwise will be used the default one).

If we will go even further... the main difference between authenticators (except authentication itself) is, how the credentials are extracted. So we can introduce CredentialsExtractorInterface being injected into the Autheticator object. This CredentialsExtractorInterface is the place to inject CredentialValidatorInterface.

In Authenticator - there will be only one method -> authenticate(Request $request) with custom logic (eg. call $credentials = $this->credentialExtractor->extract($request), etc., all other logic will be in separate objects injected to the Auhenticator via constructor. In CredentialsExtractorInterface will contains call to $this->credentialValidator->validate($credentials).

With this code structure, we simplified Authenticator, followed the Single Object Responsibility and DRY rules and kept framework modular.

Side notes:

• Validation of credentials on the client side (in HTML or Javascript) should be optional and the framework code should not rely on it.
• FormLoginAuthenticator component should introduce some new code, that will leverage Symfony Form Validation semantic, so Form is submitted -> Form is validated with Asserts in CredentialsDTO -> valid CredentialsDTO enters the authentication process.... so there is no need of credentials validation code in authenticator itself
• The same logic apply to JsonLoginAuthenticator - leverage Symfony\Component\HttpKernel\Attribute\MapRequestPayload to convert request to DTO containing requested Asserts and let framework automatically respond with 400 - BAD CREDENTIALS in case of validation error (as you can see in feature introduction here).

For this two authenticators with this kind of solution there is is no need of credentials validation at all in Authenticator object.