Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Conversation

Oviglo
Copy link
Contributor

@OvigloOviglo commentedMar 20, 2025
edited
Loading

QA
Branch?7.3
Bug fix?no
New feature?yes
Deprecations?no
LicenseMIT

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.

#[Route('/delete/{id}', name:'delete', methods: ['GET','DELETE'], requirements: ['id' => Requirement::UUID])]publicfunctiondelete(Request$request,User$user,UserManager$userManager):Response{if ($request->isMethod('DELETE') &&$this->isCsrfTokenValid('delete'.$user->getId(),$request->request->get('_token'))) {$userManager->remove($user);return$this->redirectToRoute('admin_user_index');    }return$this->render('/admin/user/delete.html.twig', ['entity' =>$user,    ]);}
#[Route('/delete/{id}', name:'delete', methods: ['GET','DELETE'], requirements: ['id' => Requirement::UUID])]#[IsCsrfTokenValid(newExpression('"delete" ~ args["user"].getId()'), methods: ['DELETE'])]publicfunctiondelete(Request$request,User$user,UserManager$userManager):Response{if ($request->isMethod('DELETE')) {$userManager->remove($user);return$this->redirectToRoute('admin_user_index');    }return$this->render('/admin/user/delete.html.twig', ['entity' =>$user,    ]);}

TheisCsrfTokenValidfunction is ignored if request method is not settings in param.

What do you think about this ?

@Spomky
Copy link
Contributor

Hi@Oviglo,

I believe the best approach would be to separate the logic into two distinct methods: one forGET and another forDELETE.
This would not only solve your issue but also improve the clarity and maintainability of your code.

@OvigloOviglo changed the titleAdd methods param in IsCsrfTokenValid attribute[Security][SecurityBundle] Add methods param in IsCsrfTokenValid attributeMar 20, 2025
@Oviglo
Copy link
ContributorAuthor

Hi@Oviglo,

I believe the best approach would be to separate the logic into two distinct methods: one forGET and another forDELETE. This would not only solve your issue but also improve the clarity and maintainability of your code.

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
Copy link
Contributor

Spomky commentedMar 20, 2025
edited
Loading

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');}
Oviglo reacted with thumbs up emoji

@nicolas-grekas
Copy link
Member

So, better close?

@Oviglo
Copy link
ContributorAuthor

If you don’t find any utility, yes 😥

Merci à vous.

Copy link
Member

@nicolas-grekasnicolas-grekas left a 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.

Oviglo reacted with thumbs up emoji
@carsonbotcarsonbot changed the title[Security][SecurityBundle] Add methods param in IsCsrfTokenValid attributeAdd methods param in IsCsrfTokenValid attributeMar 22, 2025
@Spomky
Copy link
Contributor

Instead of a list of methods, what about checking if it is a non-safe method?

@Oviglo
Copy link
ContributorAuthor

Yes you are right, a csrf token is only used for POST PUT DELETE and PATCH is’nt ?

@nicolas-grekas
Copy link
Member

CSRF tokens can also be used for GET requests, eg for logout links (that's a common practice, even if not recommended).

Oviglo reacted with thumbs up emoji

@carsonbotcarsonbot changed the titleAdd methods param in IsCsrfTokenValid attribute[Security] Add methods param in IsCsrfTokenValid attributeMar 22, 2025
@OvigloOvigloforce-pushed theis-csrf-token-valid/add-method-param branch fromadea370 to28a9102CompareMarch 23, 2025 09:12
Copy link
Contributor

@SpomkySpomky left a 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.

Oviglo reacted with heart emoji
@nicolas-grekasnicolas-grekasforce-pushed theis-csrf-token-valid/add-method-param branch from0b4edd1 to640e7a4CompareMarch 24, 2025 09:17
@nicolas-grekas
Copy link
Member

Thank you@Oviglo.

Oviglo reacted with hooray emoji

@nicolas-grekasnicolas-grekas merged commit1492e46 intosymfony:7.3Mar 24, 2025
3 of 7 checks passed
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull requestMar 26, 2025
…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
@OvigloOviglo deleted the is-csrf-token-valid/add-method-param branchMarch 27, 2025 07:54
@fabpotfabpot mentioned this pull requestMay 2, 2025
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@SpomkySpomkySpomky approved these changes

@chalasrchalasrAwaiting requested review from chalasrchalasr is a code owner

@nicolas-grekasnicolas-grekasAwaiting requested review from nicolas-grekas

Assignees
No one assigned
Projects
None yet
Milestone
7.3
Development

Successfully merging this pull request may close these issues.

5 participants
@Oviglo@Spomky@nicolas-grekas@OskarStark@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp