knpuniversity/symfony

[ep6-security] [login-form-authenticator]

Arman-Hosseini opened this issue · 8 comments

Hello guys.
I have reviewed this chapter and I noticed a problem.

In this section, the CSRF token is not checked and this is a serious problem.

src/AppBundle/Security/LoginFormAuthenticator.php

public function getCredentials(Request $request)
{
    $isLoginSubmit = $request->getPathInfo() == '/login' && $request->isMethod('POST');
    if (!$isLoginSubmit) {
        // skip authentication
        return;
    }
    $form = $this->formFactory->create(LoginForm::class);
    $form->handleRequest($request);
    $data = $form->getData();
    return $data;
}

And I considered the following solution for it:

    if ( $form->isSubmitted() && $form->isValid() )
    {
        $data = $form->getData();
        return $data;
    }
    throw new CustomUserMessageAuthenticationException("The CSRF token is invalid!");

I hope that this issue will be useful.

@Leannapelham
Please check this out.
Thanks.

Hey @Arman-Hosseini

Thanks for highlighting that problem. Your solution may work but here is a better way to check if the CRSF token is valid
https://symfonycasts.com/screencast/symfony-security/csrf-token

Actually, CSRF is not too important for login form I think, as you still need to know the valid credentials, i.e. username and password. I don't see a big problem that CSRF may harm here, do you @Arman-Hosseini ? I mean, if someone will send a SCRF request to the login endpoint - it will just fail because the intruder does not know valid credentials. Or, if the intruder does know the credentials - well, then the CSRF is the smallest problem for you because they can log in directly without any CSRF :)

Yes, but not having it will lead to easier cracking of the login form.
@bocharsky-bw

Yea, even on the login form, you should have csrf. It looks like we show that for Symfony 4, but not 3. Probably best to add a note, since 3.4 is still supported.

Also, fun fact, samesite cookies kill the need for csrf. They are possible in Symfony 4, but I think will be on by default for Symfony 5. We’ll likely show those for our sf5 tutorial.

Agree, sounds good to add a note about it with a link that refers to the page Diego mentioned where we're talking about CSRF in Sf4. @Arman-Hosseini could you create an initial a PR for this note?

Yes, I will be making a request soon.
With thanks for your attention.
@bocharsky-bw
@weaverryan

OK, great! Thank you for helping with this @Arman-Hosseini !