Skip to content
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 support for dynamic CSRF id with Expression in #[IsCsrfTokenValid] #54443

Conversation

yguedidi
Copy link
Contributor

@yguedidi yguedidi commented Mar 30, 2024

Q A
Branch? 7.1
Bug fix? no
New feature? yes
Deprecations? no
Issues continuation of #52961 from Hackday
License MIT

Use case is for example on a list page with delete action per item, and you want a CSRF token per item, so in the template you have something like the following:

{# in a loop over multiple posts #}
<form action="{{ path('post_delete', {post: post.id}) }}" method="POST">
    <input type="hidden" name="_token" value="{{ csrf_token('delete-post-' ~ post.id) }}">
    ...
</form>

The new feature will allow:

#[IsCsrfTokenValid(new Expression('"delete-post-" ~ args["post"].id'))]
public function delete(Request $request, Post $post): Response
{
    // ... delete the post
}

Maybe this need more tests but need help identify which test cases are useful.
Hope this can pass before the feature freeze

@94noni
Copy link
Contributor

94noni commented Mar 30, 2024

will this works with for example an object in the controller action ?

Expression (delete item « post.slug »)
__invoke(Post $post)

@yguedidi yguedidi force-pushed the add-support-for-dynamic-csrf-id-in-iscsrftokenvalid branch from e40a7e1 to a9643cb Compare March 30, 2024 20:35
@yguedidi
Copy link
Contributor Author

will this works with for example an object in the controller action ?

@94noni yes it should as it used the ExpressionLanguage component, like in IsGranted you can set subject="post.author"

@yguedidi yguedidi requested a review from smnandre March 30, 2024 20:38
@smnandre
Copy link
Contributor

I'm curious what is the problem this scenario help to solve ? You still have to write manually this id on the template right ?

@yguedidi
Copy link
Contributor Author

I'm curious what is the problem this scenario help to solve ? You still have to write manually this id on the template right ?

@smnandre it's for example on a list page with delete action per item, and you want a CSRF token per item, so in the template you have something like the following:

{# in a loop over multiple posts #}
<form action="{{ path('post_delete', {post: post.id}) }}" method="POST">
    <input type="hidden" name="_token" value="{{ csrf_token('delete-post-' ~ post.id) }}">
    ...
</form>

@yguedidi yguedidi force-pushed the add-support-for-dynamic-csrf-id-in-iscsrftokenvalid branch from a9643cb to 6858285 Compare March 30, 2024 22:08
@yguedidi yguedidi requested a review from smnandre March 30, 2024 22:09
Copy link
Member

@stof stof left a comment

Choose a reason for hiding this comment

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

The code snippet in the description is also wrong. Accessing a key in an array will be args["post"], not args.post. In ExpressionLanguage, the . operator is a strict equivalent of the -> operator in PHP. It does not have the Twig magic.

@yguedidi
Copy link
Contributor Author

The code snippet in the description is also wrong. Accessing a key in an array will be args["post"], not args.post. In ExpressionLanguage, the . operator is a strict equivalent of the -> operator in PHP. It does not have the Twig magic.

good catch! was written from memory without checking, thank you, description updated

@yguedidi yguedidi requested a review from stof April 2, 2024 21:57
@carsonbot carsonbot changed the title Add support for dynamic CSRF id with Expression in #[IsCsrfTokenValid] [Security] Add support for dynamic CSRF id with Expression in #[IsCsrfTokenValid] Apr 3, 2024
@yguedidi yguedidi force-pushed the add-support-for-dynamic-csrf-id-in-iscsrftokenvalid branch 2 times, most recently from 4dc452b to 7db4866 Compare April 4, 2024 03:18
@nicolas-grekas nicolas-grekas added the ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" label Apr 4, 2024
@yguedidi yguedidi force-pushed the add-support-for-dynamic-csrf-id-in-iscsrftokenvalid branch from 7db4866 to c09c734 Compare April 4, 2024 17:19
@nicolas-grekas nicolas-grekas force-pushed the add-support-for-dynamic-csrf-id-in-iscsrftokenvalid branch from c09c734 to 8f99ca5 Compare April 5, 2024 09:34
@nicolas-grekas
Copy link
Member

Thank you @yguedidi.

@nicolas-grekas nicolas-grekas merged commit 695bd1a into symfony:7.1 Apr 5, 2024
7 of 10 checks passed
@yguedidi yguedidi deleted the add-support-for-dynamic-csrf-id-in-iscsrftokenvalid branch April 5, 2024 12:46
@fabpot fabpot mentioned this pull request May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature Security ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" Status: Reviewed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants