8000 [HttpFoundation] Add `#[IsSignatureValid]` attribute by santysisi · Pull Request #60395 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[HttpFoundation] Add #[IsSignatureValid] attribute #60395

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
Sep 14, 2025
Merged

[HttpFoundation] Add #[IsSignatureValid] attribute #60395

merged 1 commit into from
Sep 14, 2025

Conversation

santysisi
Copy link
Contributor
@santysisi santysisi commented May 9, 2025 •
edited
Loading
Q A
Branch? 7.4
Bug fix? no
New feature? yes
Deprecations? no
Issues Feature #60189
License MIT

✨ New Feature: #[IsSignatureValid] Attribute

This attribute enables declarative signature validation on controller actions. It ensures that requests using specific HTTP methods are validated using UriSigner. If the signature is invalid, a SignedUriException is thrown.

✅ Usage Examples

#[IsSignatureValid] // Applies to all HTTP methods by default
public function someAction(): Response
#[IsSignatureValid(methods: ['POST', 'PUT'])] // Only validates POST and PUT requests
public function updateAction(): Response
#[IsSignatureValid(signer: 'my_custom_signer_service')] // Uses a custom UriSigner service
public function customSignedAction(): Response

@santysisi santysisi requested a review from chalasr as a code owner May 9, 2025 21:24
@carsonbot carsonbot added this to the 7.3 milestone May 9, 2025
@santysisi santysisi force-pushed the feature/verify-signature-attribute branch 5 times, most recently from 4730169 to c72ef83 Compare May 9, 2025 22:27
@santysisi
Copy link
Contributor Author
santysisi commented May 9, 2025 •
edited
Loading

Hi 👋
I'll update this version 7.3 to 7.4 once the new minor dev version is released.

Edit: I updated this version to 7.3 because I need this method from UriSigner.

@kbond
Copy link
Member
kbond commented May 12, 2025

This is looking great @santysisi! I'll give it a more thorough review after 7.3 is released.

@santysisi santysisi force-pushed the feature/verify-signature-attribute branch from c72ef83 to 85320cf Compare May 18, 2025 03:00
@santysisi
Copy link
Contributor Author

We could also consider adding an extra argument to the attribute called methods to restrict the validation to specific HTTP methods. This would align with the current behavior of the IsCsrfTokenValid attribute.

@fabpot fabpot modified the milestones: 7.3, 7.4 May 26, 2025
@santysisi santysisi force-pushed the feature/verify-signature-attribute branch from 85320cf to c000b69 Compare May 29, 2025 23:01
@santysisi
Copy link
Contributor Author

We could also consider adding an extra argument to the attribute called methods to restrict the validation to specific HTTP methods. This would align with the current behavior of the IsCsrfTokenValid attribute.

Hi 👋 I'll update this version 7.3 to 7.4 once the new minor dev version is released.

Edit: I updated this version to 7.3 because I need this method from UriSigner.

ready these 2 changes 🚀

kbond
kbond approved these changes May 30, 2025
View reviewed changes
Copy link
Member
@kbond kbond left a comment

Choose a reason for hiding this comment

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

This looks really great, nice work @santysisi!

@santysisi santysisi force-pushed the feature/verify-signature-attribute branch from c000b69 to 24fca48 Compare June 4, 2025 00:50
@santysisi santysisi mentioned this pull request Jun 4, 2025
Merged
@santysisi santysisi force-pushed the feature/verify-signature-attribute branch from 24fca48 to 524b686 Compare June 5, 2025 00:50
@santysisi
Copy link
Contributor Author

I believe the error is not relevant 🤔
I only rebased the branch to resolve some conflicts, so there shouldn't be any functional changes introduced.

nicolas-grekas
nicolas-grekas reviewed Jun 5, 2025
View reviewed changes
nicolas-grekas
nicolas-grekas reviewed Jun 5, 2025
View reviewed changes
@santysisi santysisi force-pushed the feature/verify-signature-attribute branch 3 times, most recently from a7d8c95 to 1d8202c Compare July 15, 2025 13:25
@kbond
Copy link
Member
kbond commented Jul 15, 2025

Awesome, let's wait for some others to take a look. As I said, I think tagged services is the way to go but maybe someone has a different idea.

@santysisi santysisi force-pushed the feature/verify-signature-attribute branch from 1d8202c to d2ff216 Compare July 17, 2025 22:47
@santysisi santysisi mentioned this pull request Jul 17, 2025
Closed
@alexander-schranz
Copy link
Contributor

I did not go over all comments, but for me it looks false this live inside Security namespace.

The UriSigner I can use currently without any SecurityBundle / Security services, so from my point of view the attribute should work also without any security related service and be more part of the FrameworkBundle like the UriSigner service itself.

@santysisi santysisi force-pushed the feature/verify-signature-attribute branch from d2ff216 to 35cbbc0 Compare July 18, 2025 22:42
@santysisi
Copy link
Contributor Author

I did not go over all comments, but for me it looks false this live inside Security namespace.

The UriSigner I can use currently without any SecurityBundle / Security services, so from my point of view the attribute should work also without any security related service and be more part of the FrameworkBundle like the UriSigner service itself.

Hi 👋 First of all, thank you very much for your comment 😊

You're right. When I started working on this feature, I saw this new attribute as something similar to IsGranted or IsCsrfTokenValid, which is why I initially placed it in the symfony/security-http component and the symfony/security-bundle bundle.

However, you're right that UriSigner resides in the symfony/http-foundation component and can be used without the symfony/security-bundle or any other security components. Therefore, it would make more sense for this attribute to reside in HttpFoundation and be part of the FrameworkBundle instead of the SecurityBundle.

Thanks again for the comment!

What do you think, @kbond?

@kbond
Copy link
Member
kbond commented Jul 21, 2025

Yeah, this is a good point. I wonder if http-kernel would be the better spot? This component has other controller-related attributes/listeners.

@alexander-schranz
Copy link
Contributor

@kbond HttpKernel sounds good for me if I look at the attributes there: https://github.com/symfony/symfony/tree/7.4/src/Symfony/Component/HttpKernel/Attribute

@santysisi santysisi force-pushed the feature/verify-signature-attribute branch from 35cbbc0 to 0c3ec83 Compare July 21, 2025 22:06
@santysisi
Copy link
Contributor Author
santysisi commented Jul 21, 2025 •
edited
Loading

Moved the attribute to the HttpKernel component 😄

@santysisi santysisi changed the title [HttpFoundation] Add #[IsSignatureValid] attribute [HttpKernel] Add #[IsSignatureValid] attribute Jul 21, 2025
@santysisi santysisi force-pushed the feature/verify-signature-attribute branch from 0c3ec83 to 5b0b74e Compare July 21, 2025 22:43
@santysisi santysisi force-pushed the feature/verify-signature-attribute branch from 5b0b74e to c304021 Compare August 16, 2025 23:30
stof
stof reviewed Sep 10, 2025
View reviewed changes
src/Symfony/Component/HttpKernel/Attribute/IsSignatureValid.php
#[\Attribute(\Attribute::IS_REPEATABLE | \Attribute::TARGET_CLASS | \Attribute::TARGET_METHOD | \Attribute::TARGET_FUNCTION)]
final class IsSignatureValid
{
public readonly array $methods;
Copy link
Member

Choose a reason for hiding this comment

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

Please add the phpdoc @var string[] to properly document the public API

@carsonbot carsonbot changed the title [HttpKernel] Add #[IsSignatureValid] attribute [HttpFoundation] Add #[IsSignatureValid] attribute Sep 14, 2025
@fabpot fabpot force-pushed the feature/verify-signature-attribute branch from c304021 to b3a992d Compare September 14, 2025 08:10
@fabpot
Copy link
Member
fabpot commented Sep 14, 2025

Thank you @santysisi.

@fabpot fabpot merged commit 9fd4503 into symfony:7.4 Sep 14, 2025
5 of 12 checks passed
@fabpot fabpot mentioned this pull request Sep 14, 2025
Closed
@santysisi santysisi mentioned this pull request Sep 15, 2025
Closed
@xabbuh xabbuh mentioned this pull request Sep 15, 2025
Merged
nicolas-grekas added a commit that referenced this pull request Sep 15, 2025
@nicolas-grekas
minor #61756 [HttpKernel] drop the ability to configure a signer with…
280fd47 
… the `#[IsSignatureValid]` attribute (xabbuh)

This PR was merged into the 7.4 branch.

Discussion
----------

[HttpKernel] drop the ability to configure a signer with the `#[IsSignatureValid]` attribute

| Q             | A
| ------------- | ---
| Branch?       | 7.4
| Bug fix?      | no
| New feature?  | no
| Deprecations? | no
| Issues        |
| License       | MIT

I would like to challenge the decision made in #60395 to support custom URI signers per usage of the new `#[IsSignatureValid]` attribute. I doubt that use cases for it are so wide-spread that we need to add support for it here.

Commits
-------

7fafe69 drop the ability to configure a signer with the IsSignatureValid attribute
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kbond kbond kbond approved these changes

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@stof stof stof left review comments

@fabpot fabpot fabpot approved these changes

@chalasr chalasr Awaiting requested review from chalasr

+1 more reviewer

@94noni 94noni 94noni left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Feature HttpFoundation Status: Reviewed

Projects

None yet

Milestone

7.4

Development

Successfully merging this pull request may close these issues.

10 participants

@santysisi @kbond @ruudk @alexander-schranz @fabpot @nicolas-grekas @stof @94noni @chalasr @carsonbot
0