Skip to content

[Security] : Aligning CSRF tokenId with other code sample #19808

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: 5.4
Choose a base branch
from
Open

[Security] : Aligning CSRF tokenId with other code sample #19808

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d80b3c4
[Security]: Aligning CSRF `tokenId` with other code sample
ThomasLandauer Apr 20, 2024
12e045d
Update custom_authenticator.rst
ThomasLandauer Apr 20, 2024
92a8fe8
Update security.rst
ThomasLandauer Apr 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions security.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -960,8 +960,7 @@ First, you need to enable CSRF on the form login:

Then, use the ``csrf_token()`` function in the Twig template to generate a CSRF
token and store it as a hidden field of the form. By default, the HTML field
must be called ``_csrf_token`` and the string used to generate the value must
be ``authenticate``:
is called ``_csrf_token`` and takes an arbitrary string as argument ``tokenId``:
Copy link
Member

@wouterj wouterj Apr 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This section is talking about the build-in form login authenticator. You must define it as authenticate to make it work with this authenticator.

I think we can improve the wording on https://symfony.com/doc/current/security/csrf.html#csrf-protection-in-login-forms, but the change in this document must be reverted. A suggested rewording for the linked section:

- See :ref:`form_login-csrf` for a login form that is protected from CSRF
+ When using the ``form_login`` authenticator, see :ref:`form_login-csrf` to protected from
  attacks. You can also configure the
  :ref:`CSRF protection for the logout action <reference-security-logout-csrf>`.

+ When implementing a custom authenticator, use the ``CsrfTokenBadge`` on the
+ :doc:`security passport </security/custom_authenticator>`.

Copy link
Contributor Author

@ThomasLandauer ThomasLandauer Apr 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I changed it, please take a look now.

For your suggested change, I'll come back to it after we found a solution for the below conversation.


.. code-block:: html+twig

Expand All @@ -971,7 +970,7 @@ be ``authenticate``:
<form action="{{ path('app_login') }}" method="post">
{# ... the login fields #}

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

<button type="submit">login</button>
</form>
Expand Down
4 changes: 2 additions & 2 deletions security/custom_authenticator.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -349,9 +349,9 @@ would initialize the passport like this::
{
public function authenticate(Request $request): Passport
{
$password = $request->request->get('password');
$username = $request->request->get('username');
$csrfToken = $request->request->get('csrf_token');
$password = $request->request->get('password');
$csrfToken = $request->request->get('_csrf_token');
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should also revert this change. The underscore prefix is only something used in the build-in authenticator (it's a convention in Symfony to prefix things with an underscore to avoid conflicts with application names).

When implementing a custom authenticator, you can name the field whatever you like and it's better to not use the underscore prefix as this counters the anti-conflict purpose of the prefix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, but now the docs are inconsistent (i.e. the HTML shown on one page doesn't work with the PHP code shown on the other page).

Solution? => Show the right HTML code here too!

To make this possible, the list at https://symfony.com/doc/5.x/security/custom_authenticator.html#passport-badges needs to be changed to sub-headings. Then the PHP code block we're talking about can be moved upwards under the (new) "CsrfTokenBadge" heading (=where it belongs anyway). Then I can add this HTML, resulting in a complete copy-pastable sample:

<input type="hidden" name="csrf_token" value="{{ csrf_token('login') }}">
<input type="text" name="username">
<input type="password" name="password">

What do you think?


// ... validate no parameter is empty

Expand Down
Loading