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

Add CSRF protection for login form #57

Merged
merged 1 commit into from
Apr 24, 2015

Conversation

xelaris
Copy link
Contributor

@xelaris xelaris commented Apr 23, 2015

No description provided.

@stof
Copy link
Member

stof commented Apr 24, 2015

👍

@javiereguiluz
Copy link
Member

@xelaris thanks for taking care of this important feature.

I have a proposal about this line:

<input type="hidden" name="_csrf_token" value="{{ csrf_token('authenticate') }}"/>

The current login form doesn't use form_start() and form_end() tags because when we developed the form, the best practice was to NOT use those tags. We changed that practice and now we DO recomment to use those tags.

Given that adding the CSRF hidden field manually is extremely rare (at least in my case), what do you think if we remove this field and rely instead on form_end() tag?

@stof
Copy link
Member

stof commented Apr 24, 2015

@javiereguiluz the login form is not rendered using the Symfony Form system (using it is overkill given you don't care about the form binding at all). so your suggestion does not work.
And this is precisely one of the valid use cases for the csrf_token function.

@javiereguiluz
Copy link
Member

@stof You are right! Let's merge this PR then.

@javiereguiluz
Copy link
Member

@xelaris thanks for the pull request!

@javiereguiluz javiereguiluz merged commit a95a5f9 into symfony:master Apr 24, 2015
javiereguiluz added a commit that referenced this pull request Apr 24, 2015
This PR was merged into the master branch.

Discussion
----------

Add CSRF protection for login form

Commits
-------

a95a5f9 Add CSRF protection for login form
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

Successfully merging this pull request may close these issues.

None yet

3 participants