-
-
Notifications
You must be signed in to change notification settings - Fork 9.6k
[Security] Add methods param in IsCsrfTokenValid attribute #60007
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
[Security] Add methods param in IsCsrfTokenValid attribute #60007
Conversation
Hi @Oviglo, I believe the best approach would be to separate the logic into two distinct methods: one for |
Yes, but it will make my controller heavier. Another solution is to create an empty form for using native csrf validation. Is it an official recommendation to use only one method per action ? |
I’m not sure what you mean by "heavier" in this context. There is no official recommendation to use only one method per action, it is more about the single-responsibility principle. Here’s an example that should work as expected: #[Route('/delete/{id}', name: 'get', methods: ['GET'], requirements: ['id' => Requirement::UUID])]
public function get(User $user): Response
{
return $this->render('/admin/user/delete.html.twig', ['entity' => $user]);
}
#[Route('/delete/{id}', name: 'delete', methods: ['DELETE'], requirements: ['id' => Requirement::UUID])]
#[IsCsrfTokenValid(new Expression('"delete" ~ args["user"].getId()'))]
public function delete(User $user, UserManager $userManager): Response
{
$userManager->remove($user);
return $this->redirectToRoute('admin_user_index');
} |
So, better close? |
If you don’t find any utility, yes 😥 Merci à vous. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Work for me despite the proposed alternative.
src/Symfony/Component/Security/Http/Attribute/IsCsrfTokenValid.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Http/Attribute/IsCsrfTokenValid.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Http/EventListener/IsCsrfTokenValidAttributeListener.php
Outdated
Show resolved
Hide resolved
Instead of a list of methods, what about checking if it is a non-safe method? |
Yes you are right, a csrf token is only used for POST PUT DELETE and PATCH is’nt ? |
CSRF tokens can also be used for GET requests, eg for logout links (that's a common practice, even if not recommended). |
adea370
to
28a9102
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Thank you.
0b4edd1
to
640e7a4
Compare
Thank you @Oviglo. |
…tribute (Oviglo) This PR was squashed before being merged into the 7.3 branch. Discussion ---------- [Security] Add methods param doc for isCsrfTokenValid attribute Add new params for isCsrfTokenValid attribute PR: symfony/symfony#60007 Issue: #20810 Commits ------- 6d7c87f [Security] Add methods param doc for isCsrfTokenValid attribute
I use a controller action to show a confirmation message for delete entity, i think it could be usefull to add 'methods' param in
#[IsCsrfTokenValid]
attribute.The
isCsrfTokenValid
function is ignored if request method is not settings in param.What do you think about this ?