symfony: UsernamePasswordFormAuthenticationListener cant handle missing field "_username"

Symfony version(s) affected: 3.4.12

Description
We are using a form_login in security.yml. If we submit an empty POST request to the check_path route, an BadRequestHttpException is thrown: The key “_username” must be a string, “NULL” given. This happend several times in our production system.

How to reproduce
Send an empty POST Request to the login-page configured in: security.firewalls.xxx.form_login.check_path

Possible Solution
Allow the username to be NULL, so it can be treated as an empty string. Symfony\Component\Security\Http\Firewall\UsernamePasswordFormAuthenticationListener.php method: attemptAuthentication() line 88: if ((!\is_string($username) && !\is_null($username)) || (\is_object($username) && !\method_exists($username, '__toString'))) {

About this issue

  • Original URL
  • State: closed
  • Created 6 years ago
  • Reactions: 2
  • Comments: 17 (10 by maintainers)

Most upvoted comments

We are running into the same issue, the current BadRequestHttpException makes sense in a development environment but not in a production environment. Any unauthenticated user on the internet can call the login url without POST parameters and spam the error logs with this exception, potentially flooding the server if error emails are triggered.

In my opinion it would make more sense if UsernamePasswordFormAuthenticationListener::requiresAuthentication() actually checks if the needed username and password are actually in the POST. It doesn’t make a lot of sense to check a login attempt without any credentials. Maybe a warning in the Symfony toolbar could trigger a developer to add the missing fields if they are not present in the login form.

edit: A workaround seems to be to ignore 400 errors in the logs but I’m not sure if it is the right solution:

monolog:
    handlers:
        main:
            type: fingers_crossed
            action_level: error
            handler: nested
            excluded_http_codes: [400, 404, 405]

The form_login listener expects form-data, if an user reaches this endpoint without the username and/or password parameters, telling him “Bad credentials” would be wrong. This case should not happen, it should be handled client side.

@darknmy you have to fix your form, it should send _username not username, as the error hints.

@bokonet As explained above, the client error is desired, not a bug. About the exception message when the username parameter is not set, please feel free to submit a PR against master if you think it can be improved.

@xabbuh Ok, now i understand why our solution is a problem in general. The Develooper needs the Information about misconfigured login-form-settings. But in my opinion an invalid client-request should never trigger an uncaught exception. What if the UsernamePasswordFormAuthenticationListener send an error message like “No Username submitted”? In this case the Developer got the hint of misconfigured login-form-setting as well.

@ostrolucky We are using swift_mailer in monolog-handlers to send Errors in prod environment with the “excluded_404s” parameter. see https://symfony.com/doc/3.4/logging/monolog_email.html If we change the action_level from ‘warning’ to ‘critical’ we will get 500 server errors only.

I think the same way as @iltar, a missing required request parameter deserves a client error. But looking a the PR where the request validation was introduced https://github.com/symfony/symfony/pull/25657#issuecomment-356933359, I may miss a good reason to allow omitting these. @xabbuh Is this what you had in mind? looks legit to you? If confirmed, the same issue applies to the json-login listener.

Isn’t that how it should work? I’m not too familiar with requests, but if there’s no username field given, it should tell you that it’s incorrect.