Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.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 ourterms of service andprivacy 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 ? |
Spomky commentedMar 20, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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])]publicfunctionget(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(newExpression('"delete" ~ args["user"].getId()'))]publicfunctiondelete(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 OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Security/Http/Attribute/IsCsrfTokenValid.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
src/Symfony/Component/Security/Http/EventListener/IsCsrfTokenValidAttributeListener.php OutdatedShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
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
to28a9102
CompareThere 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
to640e7a4
CompareThank you@Oviglo. |
1492e46
intosymfony:7.3Uh oh!
There was an error while loading.Please reload this page.
…tribute (Oviglo)This PR was squashed before being merged into the 7.3 branch.Discussion----------[Security] Add methods param doc for isCsrfTokenValid attributeAdd new params for isCsrfTokenValid attributePR:symfony/symfony#60007Issue:#20810Commits-------6d7c87f [Security] Add methods param doc for isCsrfTokenValid attribute
Uh oh!
There was an error while loading.Please reload this page.
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 ?