[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 !