Skip to content

[Security][SecurityBundle] Add #[IsCsrfTokenValid] attribute #52961

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 9, 2023
Merged

[Security][SecurityBundle] Add #[IsCsrfTokenValid] attribute #52961

merged 1 commit into from
Dec 9, 2023

Conversation

yguedidi
Copy link
Contributor

@yguedidi yguedidi commented Dec 9, 2023 •
edited by wouterj
Loading

Q A
Branch? 7.1
Bug fix? no
New feature? yes
Deprecations? no
Issues Fix #52947
License MIT

#SymfonyHackday

@yguedidi yguedidi requested a review from chalasr as a code owner December 9, 2023 11:04
@carsonbot carsonbot added this to the 7.1 milestone Dec 9, 2023
@yguedidi yguedidi force-pushed the iscsrftokenvalid-attribute branch from c3cc2bb to 11d7d37 Compare December 9, 2023 11:08
@yguedidi yguedidi changed the title Add IsCsrfTokenValid attribute [Security] Add IsCsrfTokenValid attribute Dec 9, 2023
@yguedidi yguedidi force-pushed the iscsrftokenvalid-attribute branch 3 times, most recently from c48c718 to fec60e3 Compare December 9, 2023 11:44
@carsonbot carsonbot changed the title [Security] Add IsCsrfTokenValid attribute Add IsCsrfTokenValid attribute Dec 9, 2023
@carsonbot carsonbot changed the title Add IsCsrfTokenValid attribute [SecurityBundle] Add IsCsrfTokenValid attribute Dec 9, 2023
@carsonbot carsonbot changed the title [SecurityBundle] Add IsCsrfTokenValid attribute [Security][SecurityBundle] Add IsCsrfTokenValid attribute Dec 9, 2023
@OskarStark OskarStark changed the title [Security][SecurityBundle] Add IsCsrfTokenValid attribute [SecurityBundle] Add #[IsCsrfTokenValid] attribute Dec 9, 2023
@carsonbot carsonbot changed the title [SecurityBundle] Add #[IsCsrfTokenValid] attribute [Security][SecurityBundle] Add #[IsCsrfTokenValid] attribute Dec 9, 2023
@yguedidi yguedidi force-pushed the iscsrftokenvalid-attribute branch from fec60e3 to 54a7d6f Compare December 9, 2023 12:14
dsdeboer
dsdeboer approved these changes Dec 9, 2023
View reviewed changes
Copy link
Contributor

@dsdeboer dsdeboer left a comment

Choose a reason for hiding this comment

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

Nicely done 👏

@chalasr
Copy link
Member

chalasr commented Dec 9, 2023 •
edited
Loading

I know the name of the proposed attribute is inline with the #[IsGranted] one and I'm fundamentally fine with it, yet I'm wondering if some more imperative name wouldn't read better e.g. #[ValidateCsrfToken], #[ValidCsrfToken] or even #[RequireValidCsrfToken. Thoughts?

@OskarStark
Copy link
Contributor

OskarStark commented Dec 9, 2023 •
edited
Loading

I know the name of the proposed attribute is inline with the #[IsGranted] one and I'm fundamentally fine with it, yet I'm wondering if some more imperative name wouldn't read better e.g. #[ValidateCsrfToken], #[ValidCsrfToken] or even #[RequireValidCsrfToken. Thoughts?

#[HasValidCsrfToken] ?

@yguedidi
Copy link
Contributor Author

yguedidi commented Dec 9, 2023

@chalasr it's more inline with the AbstractController helper function ->isCsrfTokenValid(), like the #[IsGranted] is with ->isGranted()

@wouterj
Copy link
Member

wouterj commented Dec 9, 2023

@chalasr it's more inline with the AbstractController helper function ->isCsrfTokenValid(), like the #[IsGranted] is with ->isGranted()

Sounds valid to me 👍 (I honestly didn't know we had this helper method in the abstract controller)

@fabpot
Copy link
Member

fabpot commented Dec 9, 2023

@yguedidi Can you have a look at the tests, they seem broken by this PR.

@OskarStark
Copy link
Contributor

OskarStark commented Dec 9, 2023 •
edited
Loading

Let's go with the name then 👍🏻

Thanks for the explanation ℹ️

@yguedidi
Copy link
Contributor Author

yguedidi commented Dec 9, 2023

Can you have a look at the tests, they seem broken by this PR.

@fabpot Indeed low deps unit tests are broken by this PR!
I checked and I'm wondering what is the right fix.
Issue is that SecurityBundle will install security/http 6.4, so without the new listener.
Should I just upgrade the requirement to 7.1 or conditionally add the service if the class exists? or something else?

@fabpot
Copy link
Member

fabpot commented Dec 9, 2023

Can you have a look at the tests, they seem broken by this PR.

@fabpot Indeed low deps unit tests are broken by this PR! I checked and I'm wondering what is the right fix. Issue is that SecurityBundle will install security/http 6.4, so without the new listener. Should I just upgrade the requirement to 7.1 or conditionally add the service if the class exists? or something else?

You need to bump the minimum version in SecurityBundle.

@yguedidi yguedidi force-pushed the iscsrftokenvalid-attribute branch from 54a7d6f to a0e1d66 Compare December 9, 2023 16:40
@yguedidi
Copy link
Contributor Author

yguedidi commented Dec 9, 2023

@fabpot SecurityBundle tests now pass! other failures seems unrelated

@fabpot
Copy link
Member

fabpot commented Dec 9, 2023

Thank you @yguedidi.

@fabpot fabpot merged commit 5ae3df5 into symfony:7.1 Dec 9, 2023
@fabpot fabpot mentioned this pull request Dec 9, 2023
Closed
@yguedidi yguedidi deleted the iscsrftokenvalid-attribute branch December 9, 2023 17:25
@yguedidi yguedidi mentioned this pull request Mar 30, 2024
Merged
nicolas-grekas added a commit that referenced this pull request Apr 5, 2024
@nicolas-grekas
feature #54443 [Security] Add support for dynamic CSRF id with Expres…
695bd1a 
…sion in `#[IsCsrfTokenValid]` (yguedidi)

This PR was merged into the 7.1 branch.

Discussion
----------

[Security] Add support for dynamic CSRF id with Expression in `#[IsCsrfTokenValid]`

| 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:

```twig
{# 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:
```php
#[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

Commits
-------

8f99ca5 Add support for dynamic CSRF id in IsCsrfTokenValid
@fabpot fabpot mentioned this pull request May 2, 2024
Merged
@nicolas-grekas nicolas-grekas mentioned this pull request Jun 24, 2024
Merged
@nicolas-grekas nicolas-grekas mentioned this pull request Sep 11, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OskarStark OskarStark OskarStark left review comments

@chalasr chalasr chalasr approved these changes

@fabpot fabpot fabpot approved these changes

@wouterj wouterj wouterj approved these changes

@dsdeboer dsdeboer dsdeboer approved these changes

Assignees
No one assigned
Labels
Hack Day Security SecurityBundle Status: Reviewed
Projects
None yet
Milestone
7.1
Development

Successfully merging this pull request may close these issues.

What about an IsCsrfTokenValid attribute?
7 participants
@yguedidi @chalasr @OskarStark @wouterj @fabpot @dsdeboer @carsonbot