Skip to content

[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

Merged

Conversation

Oviglo
Copy link
Contributor

@Oviglo Oviglo commented Mar 20, 2025

Q A
Branch? 7.3
Bug fix? no
New feature? yes
Deprecations? no
License MIT

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])]
public function delete(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(new Expression('"delete" ~ args["user"].getId()'), methods: ['DELETE'])]
public function delete(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,
    ]);
}

The isCsrfTokenValidfunction is ignored if request method is not settings in param.

What do you think about this ?

@Spomky
Copy link
Contributor

Spomky commented Mar 20, 2025

Hi @Oviglo,

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

@Oviglo Oviglo changed the title Add methods param in IsCsrfTokenValid attribute [Security][SecurityBundle] Add methods param in IsCsrfTokenValid attribute Mar 20, 2025
@Oviglo
Copy link
Contributor Author

Oviglo commented Mar 20, 2025

Hi @Oviglo,

I believe the best approach would be to separate the logic into two distinct methods: one for GET and another for DELETE. 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 commented Mar 20, 2025

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');
}

@nicolas-grekas
Copy link
Member

So, better close?

@Oviglo
Copy link
Contributor Author

Oviglo commented Mar 21, 2025

If you don’t find any utility, yes 😥

Merci à vous.

Copy link
Member

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

@carsonbot carsonbot changed the title [Security][SecurityBundle] Add methods param in IsCsrfTokenValid attribute Add methods param in IsCsrfTokenValid attribute Mar 22, 2025
@Spomky
Copy link
Contributor

Spomky commented Mar 22, 2025

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

@Oviglo
Copy link
Contributor Author

Oviglo commented Mar 22, 2025

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).

@carsonbot carsonbot changed the title Add methods param in IsCsrfTokenValid attribute [Security] Add methods param in IsCsrfTokenValid attribute Mar 22, 2025
@Oviglo Oviglo force-pushed the is-csrf-token-valid/add-method-param branch from adea370 to 28a9102 Compare March 23, 2025 09:12
@Oviglo Oviglo requested a review from nicolas-grekas March 23, 2025 09:21
Copy link
Contributor

@Spomky Spomky 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.

@nicolas-grekas nicolas-grekas force-pushed the is-csrf-token-valid/add-method-param branch from 0b4edd1 to 640e7a4 Compare March 24, 2025 09:17
@nicolas-grekas
Copy link
Member

Thank you @Oviglo.

@nicolas-grekas nicolas-grekas merged commit 1492e46 into symfony:7.3 Mar 24, 2025
3 of 7 checks passed
javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request Mar 26, 2025
…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
@Oviglo Oviglo deleted the is-csrf-token-valid/add-method-param branch March 27, 2025 07:54
@fabpot fabpot mentioned this pull request May 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants