[HttpFoundation] #[IsSignatureValid] attribute
#60395
Conversation
4730169 to
c72ef83
Compare
src/Symfony/Component/Security/Http/EventListener/IsSignatureValidAttributeListener.php
Show resolved
Hide resolved
|
This is looking great @santysisi! I'll give it a more thorough review after 7.3 is released. |
c72ef83 to
85320cf
Compare
|
We could also consider adding an extra argument to the attribute called |
85320cf to
c000b69
Compare
ready these 2 changes 🚀 |
c000b69 to
24fca48
Compare
24fca48 to
524b686
Compare
|
I believe the error is not relevant 🤔 |
src/Symfony/Component/Security/Http/Attribute/IsSignatureValid.php
Outdated
Show resolved
Hide resolved
src/Symfony/Component/Security/Http/Attribute/IsSignatureValid.php
Outdated
Show resolved
Hide resolved
5881d55 to
ce5fea7
Compare
|
Hi @nicolas-grekas 👋, Thanks so much for all your suggestions. I really appreciate them ❤️ Thanks again! ❤️ |
cb33c19 to
da9f672
Compare
| /** | ||
| * @param int|null $statusCode The HTTP status code to return if the signature is invalid. | ||
| * Ignored when 'throw' is true. If null, error code 404 is used. | ||
| * @param bool|null $throw If true, an exception is thrown on signature failure instead of returning a response. |
There was a problem hiding this comment.
This is confusing IMO. With throw: false, it still throws an HttpException, so it still goes through the global error handling to return the error page.
what changes is whether it throws a SignedUriException or an HttpException.
What I would suggest instead is to always throw an HttpException, with the SignedUriException attached as previous exception. This simplifies the feature while still allowing advanced use cases by inspecting the previous exception.
There was a problem hiding this comment.
Alternatively, always throw the SignedUriException and add the #[WithHttpStatus(404)] attribute on the SignedUriException, letting projects use the configuration in FrameworkBundle if they way to change the status code (assuming it is fine to have the same status code for all endpoints when signature verification fails)
Note that this option would allow having different status code for the various subclasses of SignedUriException to have 404 for a missing signature but 403 for an expired one for instance.
There was a problem hiding this comment.
Alternatively, always throw the SignedUriException and add the #[WithHttpStatus(404)] attribute on the SignedUriException, letting projects use the configuration in FrameworkBundle if they way to change the status code (assuming it is fine to have the same status code for all endpoints when signature verification fails)
I like this option.
| public function onKernelControllerArguments(ControllerArgumentsEvent $event): void | ||
| { | ||
| /** @var IsSignatureValid[] $attributes */ | ||
| if (!\is_array($attributes = $event->getAttributes()[IsSignatureValid::class] ?? null)) { |
There was a problem hiding this comment.
use $event->getAttributes(IsSignatureValid::class) instead, which always returns an array and already has the proper IsSignatureValid[] type thanks to the generic type.
| */ | ||
| public function __construct( | ||
| public ?int $statusCode = null, | ||
| public ?bool $throw = null, |
There was a problem hiding this comment.
there is no reason to make property nullable (as you make no difference between false and null)
| * Useful for global exception handling or listener-based workflows. | ||
| */ | ||
| public function __construct( | ||
| public ?int $statusCode = null, |
There was a problem hiding this comment.
instead of making the property nullable with a constant in the listener for the default status, I suggest making it non-nullable with 404 as default value (to be discussed whether it should be a 403 instead)
| /** | ||
| * @param int $validationFailedStatusCode The HTTP status code to return if the signature is invalid | ||
| * @param bool|null $throw If true, a {@see \Symfony\Component\HttpFoundation\Exception\SignedUriException} is thrown on signature failure instead of returning a response | ||
| * @param array|string $methods HTTP methods that require signature validation |
There was a problem hiding this comment.
| * @param array|string $methods HTTP methods that require signature validation | |
| * @param string[]|string $methods HTTP methods that require signature validation. An empty array means that no method filtering is done. |
| * @param array|string $methods HTTP methods that require signature validation | ||
| */ | ||
| public function __construct( | ||
| public int $validationFailedStatusCode = Response::HTTP_NOT_FOUND, |
There was a problem hiding this comment.
those public properties should be readonly IMO, instead of being mutable externally.
| public function __construct( | ||
| public int $validationFailedStatusCode = Response::HTTP_NOT_FOUND, | ||
| public ?bool $throw = null, | ||
| public array|string $methods = [], |
There was a problem hiding this comment.
I would suggest not using a promoted property here, so that the constructor argument can accept array|string but we normalize it to always store an array in the property.
da9f672 to
3811790
Compare
|
Hi @stof , thanks for your suggestion! |
| array|string $methods = [], | ||
| ) { | ||
| if (!method_exists(UriSigner::class, 'verify')) { | ||
| throw new \LogicException('The method UriSigner::verify is required. Please upgrade symfony/http-foundation to 7.3 version or ensure it is correctly installed.'); |
There was a problem hiding this comment.
| throw new \LogicException('The method UriSigner::verify is required. Please upgrade symfony/http-foundation to 7.3 version or ensure it is correctly installed.'); | |
| throw new \LogicException('The `IsSignatureValid` attribute requires symfony/http-foundation >= 7.3.'); |
There was a problem hiding this comment.
Hi 👋: , thanks for your suggestion ❤️
I've made the changes accordingly, let me know if there's anything else you'd like me to adjust.
3811790 to
05d2de6
Compare
✨ New Feature:
#[IsSignatureValid]AttributeThis 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, aSignedUriExceptionis thrown.✅ Usage Examples