Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
Arman-Hosseini opened this issue Jul 11, 2019 · 8 comments
Open

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

Arman-Hosseini opened this issue Jul 11, 2019 · 8 comments

Comments

@Arman-Hosseini
Copy link

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.

@Arman-Hosseini
Copy link
Author

@Leannapelham
Please check this out.
Thanks.

@MolloKhan
Copy link
Contributor

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

@bocharsky-bw
Copy link
Member

bocharsky-bw commented Jul 13, 2019

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

@Arman-Hosseini
Copy link
Author

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

@weaverryan
Copy link
Member

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.

@bocharsky-bw
Copy link
Member

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?

@Arman-Hosseini
Copy link
Author

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

@bocharsky-bw
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants