Skip to content

[FrameworkBundle][Serializer] Add an ArgumentResolver to deserialize & validate user input #45628

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

Closed
wants to merge 13 commits into from

Conversation

GaryPEGEOT
Copy link
Contributor

@GaryPEGEOT GaryPEGEOT commented Mar 3, 2022

Q A
Branch? 6.1
Bug fix? no
New feature? yes
Deprecations? no
Tickets N/A
License MIT
Doc PR TODO

Assuming you wish to validate some DTO:

<?php

use Symfony\Component\Validator\Constraints as Assert;

class DummyDto
{
    #[Assert\NotBlank()]
    public ?string $randomText = null;

    #[Assert\IsTrue()]
    public bool $itMustBeTrue = true;
}

If you're not using API Platform, you will probably have to inject serializer & validator in your controller, check if input can be normalized & is valid:

// Before
public function __invoke(Request $request): Response
{
        $input = $this->serializer->deserialize($request->getContent(), DummyDto::class, 'json');
        $errors = $this->validator->validate($input);

        if ($errors->count()) {
            // Some logic to return error to user
        }

While this not the end of the world (:crossed_fingers:), It can be cumbersome and doesn't necessarily bring value.

Using Symfony\Component\Serializer\Annotation\Input provide an opt'in way to get a DTO deserialized and validated directly in the controller:

// After

use Symfony\Component\Serializer\Annotation\Input;

public function __invoke(#[Input()] DummyDto $input): Response
{
        // do your logic

In case the value is not valid, user will receive a HTTP 422 response. Once again, it's completely opt'in (For those of you who don't like magic 🦄 )

Copy link
Member

@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

I like this feature. I have some small comments I need help to understand.

@mathsunn
Copy link

mathsunn commented Mar 3, 2022

Nice feature ! Thanks

@derrabus
Copy link
Member

derrabus commented Mar 3, 2022

I like the basic idea of this feature, thank you for the PR. I however believe that it should be implemented differently.

First of all, the marker interface. In a world with native attributes, we should not use marker interfaces anymore. Also, it does not really make sense to add a marker (interface or attribute) to a DTO class just because in some controller I might want to deserialize a request payload into it. If we assume that such a DTO class could be a Doctrine entity at the same time, the current implementation would create unpleasant ambiguites with good old param converters or its replacement (see #43854).

I strongly advice to implement this as an attribute for controller parameters. This would also solve the configuration issues mentioned.

Secondly, I'd like to move as little logic as possible into the framework bundle. I believe this argument resolver should live inside the serializer component and validation should be made optional.

@carsonbot carsonbot changed the title [FrameworkBundle] Add an ArgumentResolver to deserialize & validate user input [FrameworkBundle][Serializer] Add an ArgumentResolver to deserialize & validate user input Mar 3, 2022
public function resolve(Request $request, ArgumentMetadata $argument): iterable
{
try {
$input = $this->serializer->deserialize($request->getContent(), $argument->getType(), $request->attributes->get('_format', 'json'));
Copy link
Member

Choose a reason for hiding this comment

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

Are we sure the request payload is JSON? Should we check the content type? Should we allow the controller to configure the expected format?

try {
$input = $this->serializer->deserialize($request->getContent(), $argument->getType(), $request->attributes->get('_format', 'json'));
} catch (ExceptionInterface $exception) {
throw new UnparsableInputException($exception->getMessage(), 0, $exception);
Copy link
Member

Choose a reason for hiding this comment

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

Why not UnprocessableEntityHttpException?

];
$reason .= "{$violation->getPropertyPath()}: {$violation->getMessage()} ";
}
$response = new Response($this->serializer->serialize($data, $format), Response::HTTP_UNPROCESSABLE_ENTITY);
Copy link
Member

Choose a reason for hiding this comment

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

The serializer is capable of rendering a constraint violation list. Why are we inventing a new output format here?

Copy link
Member

@lyrixx lyrixx left a comment

Choose a reason for hiding this comment

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

I totally agree with @derrabus 👍🏼

Comment on lines 42 to 48
foreach ($throwable->getViolations() as $violation) {
$data['errors'][] = [
'propertyPath' => $violation->getPropertyPath(),
'message' => $violation->getMessage(),
'code' => $violation->getCode(),
];
$reason .= "{$violation->getPropertyPath()}: {$violation->getMessage()} ";
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks!

public function resolve(Request $request, ArgumentMetadata $argument): iterable
{
try {
$input = $this->serializer->deserialize($request->getContent(), $argument->getType(), $request->attributes->get('_format', 'json'));
Copy link
Member

Choose a reason for hiding this comment

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

Can you also enable the support for type property?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure what you mean by that 😄

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice! thanks, I'll add that

}

$response = new Response($this->serializer->serialize($throwable->getViolations(), $format), Response::HTTP_UNPROCESSABLE_ENTITY);
$this->logger->info('Invalid input rejected: "{reason}"', ['reason' => (string) $throwable->getViolations()]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not super sure what to log here, as violations might contain sensitive information?

Copy link
Member

Choose a reason for hiding this comment

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

IMHO, the reasons are not really useful in the log

@GaryPEGEOT
Copy link
Contributor Author

GaryPEGEOT commented Mar 5, 2022

@derrabus I switched to Attribute, way nicer indeed, thanks. Not sure about the optional validation though, for two reasons:

  1. It imply to create custom normalization for PartialDenormalizationException instead of relying on ConstraintViolationListNormalizer
  2. Not sure if it's a good practice to "not validate" user data.

And either way, If a dev really don't care about validation, adding no assertion should result in little overhead.

$throwable = $event->getThrowable();
$format = $event->getRequest()->attributes->get('_format', 'json');

if (!$throwable instanceof ValidationFailedException) {
Copy link
Member

Choose a reason for hiding this comment

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

This change is potentially dangerous. Existing application might already use this exception and your listener will add unexpected sematics to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed! Validation is now optional as well

@derrabus
Copy link
Member

derrabus commented Mar 6, 2022

Not sure if it's a good practice to "not validate" user data.

No, but Symfony's validator is certainly not the only tool for that. If we add this functionality to the serializer component (which can even be used standalone), the validator should not be a mandatory dependency for using it.

And either way, If a dev really don't care about validation, adding no assertion should result in little overhead.

Sure, but installing the validator although it's not used actually is unnecessary overhead.

Copy link
Member

@yceruto yceruto left a comment

Choose a reason for hiding this comment

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

I like the idea too, but I think the implementation of the error response should be improved and adapted to use the current ErrorRenderer mechanism.

$context = array_merge($attribute->serializationContext, [
DenormalizerInterface::COLLECT_DENORMALIZATION_ERRORS => true,
]);
$format = $attribute->format ?? $request->attributes->get('_format', 'json');
Copy link
Member

Choose a reason for hiding this comment

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

Use $request->getPreferredFormat('json') instead (it will take into account the acceptable content type when provided)

Copy link
Member

@derrabus derrabus Mar 7, 2022

Choose a reason for hiding this comment

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

Neither is correct, imho. getPreferredFormat() as well as the _format attribute hint at the format of the response. What we need here is the format of the request payload. This can be inferred from the Content-Type header of the request.

That being said, the developer should be able to limit the set of possible formats (and content types?) via the attribute. Maybe we should even default to JSON-only, so nobody enables CSV deserialization by accident. 😓

Copy link
Member

Choose a reason for hiding this comment

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

You're right, then we should use $request->getContentType() instead.

[Off topic] Btw, the name of this method is not exactly what you'd expect here, it returns json, xml, etc. even if that's what we want here it seems like the expected result (according to the method name) is application/json, etc. maybe this method should be renamed to getContentTypeFormat() or something like this, but that's another topic :)

@@ -8,6 +8,7 @@ CHANGELOG
* Load PHP configuration files by default in the `MicroKernelTrait`
* Add `cache:pool:invalidate-tags` command
* Add `xliff` support in addition to `xlf` for `XliffFileDumper`
* Add an ArgumentResolver to deserialize & validate user input
Copy link
Member

Choose a reason for hiding this comment

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

Can you add the specific class(es) added?

This changelog belongs to the Serializer component now, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have to move that 😅


$errors = new ConstraintViolationList();

foreach ($e->getErrors() as $exception) {
Copy link
Member

@yceruto yceruto Mar 7, 2022

Choose a reason for hiding this comment

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

I don't think it's the argument resolver's responsibility to normalize these errors, but the Serializer normalizers instead.

In order not to depend on the Validator component (as I've pointed out in another comment) you can add a new PartialDenormalizationProblemNormalizer just to handle this kind of exception.

$input = $this->serializer->deserialize(data: $request->getContent(), type: $argument->getType(), format: $format, context: $context);
} catch (PartialDenormalizationException $e) {
if (null === $this->validator) {
throw new UnprocessableEntityHttpException(message: $e->getMessage(), previous: $e);
Copy link
Member

@yceruto yceruto Mar 7, 2022

Choose a reason for hiding this comment

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

This will show an empty message which is not useful. However, we can still normalize the PartialDenormalizationException to show all errors. I mean, it shouldn't depend exclusively on the Validator ConstraintViolationList as it's optional.

*
* @author Gary PEGEOT <garypegeot@gmail.com>
*/
class InputValidationFailedExceptionListener
Copy link
Member

@yceruto yceruto Mar 7, 2022

Choose a reason for hiding this comment

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

I don't think this listener is really necessary to return the proper response (content + status code). Instead, you can go through the ErrorRenderer system by directly adding a custom Serializer normalizer. The Framework already knows how to handle exceptions according to the request format (see SearializerErrorRenderer).

$format = $attribute->format ?? $request->attributes->get('_format', 'json');

try {
$input = $this->serializer->deserialize(data: $request->getContent(), type: $argument->getType(), format: $format, context: $context);
Copy link
Member

Choose a reason for hiding this comment

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

Please use ordered arguments.

@jvasseur
Copy link
Contributor

jvasseur commented Mar 7, 2022

Would it make sense to split this into 2 different features: one that deserialize input and one that validate controller arguments (with 2 separate attributes)?

This would allow to use the deserialize attribute with custom validation logic or the validation one with other argument resolver.

@derrabus
Copy link
Member

derrabus commented Mar 7, 2022

Would it make sense to split this into 2 different features: one that deserialize input and one that validate controller arguments (with 2 separate attributes)?

This would allow us to validate any arguments, not only those that have been deserialized from the request payload. 🤔

@GaryPEGEOT
Copy link
Contributor Author

Would it make sense to split this into 2 different features: one that deserialize input and one that validate controller arguments (with 2 separate attributes)?

This would allow us to validate any arguments, not only those that have been deserialized from the request payload.

Can it be multiple ArgumentResolver for one specific argument ? or does the first ArgumentResolver to supports an argument takes over?

@jvasseur
Copy link
Contributor

jvasseur commented Mar 7, 2022

The first argument resolver take over, the validation would have to be implemented in another way (probably with an event listener).

@GaryPEGEOT
Copy link
Contributor Author

@lyrixx @dunglas would make sense to create an AbstractProblemNormalizer to regroup ConstraintViolationListNormalizer and the new PartialDenormalizationProblemNormalizer?

@GaryPEGEOT GaryPEGEOT force-pushed the feat/user-input-resolver branch from 9e90c03 to 5976a4b Compare March 9, 2022 17:32
@lyrixx
Copy link
Member

lyrixx commented Mar 10, 2022

I think yes 👍🏼

]);
$format = $attribute->format ?? $request->getContentType() ?? 'json';

yield $this->serializer->deserialize($request->getContent(), $argument->getType(), $format, $context);
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be great to resolve input not only from request content but from query/request parameters

Something like this:

if ($attribute->isFromQueryString()) {
    $input = $this->denormalizer->denormalize($request->query->all(), $argument->getType());
} else {
    if ($request->getContentType() === 'form') {
        $input = $this->denormalizer->denormalize($request->request->all(), $argument->getType());
    } else {
        $input = $this->serializer->deserialize($request->getContent(), $type, $format);
    }
}

@dunglas
Copy link
Member

dunglas commented Mar 18, 2022

I agree too.

Copy link
Member

@dunglas dunglas left a comment

Choose a reason for hiding this comment

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

To be honest, I would recommend using API Platform instead of adding such code to Symfony.
This is a complex and security sensitive topic (see https://cheatsheetseries.owasp.org/cheatsheets/REST_Security_Cheat_Sheet.html) and by going down the rabbit hole we'll likely end up reimplementing most of API Platform's logic.

$context = array_merge($attribute->context, [
DenormalizerInterface::COLLECT_DENORMALIZATION_ERRORS => true,
]);
$format = $attribute->format ?? $request->getContentType() ?? 'json';
Copy link
Member

Choose a reason for hiding this comment

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

For security reasons, I suggest to throw an exception if the format isn't provided in the Content-Type header and if the excepted format (the format explicitly passed as parameter by the user) doesn't match the value of Content-Type.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could we just use $request->toArray()? Or it may be too restrictive in terms of format?

@fabpot fabpot modified the milestones: 6.1, 6.2 May 20, 2022
$response = new Response($this->serializer->serialize($throwable->getViolations(), $format), Response::HTTP_UNPROCESSABLE_ENTITY);
$this->logger->info('Invalid input rejected: "{reason}"', ['reason' => (string) $throwable->getViolations()]);

$event->setResponse($response);
Copy link
Contributor

Choose a reason for hiding this comment

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

What about stopping propagation as this listener is only here to create the HTTP response?

Copy link
Member

Choose a reason for hiding this comment

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

public function __invoke(ExceptionEvent $event): void
{
$throwable = $event->getThrowable();
$format = $event->getRequest()->attributes->get('_format', 'json');
Copy link
Contributor

Choose a reason for hiding this comment

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

Better use getPreferredFormat that handles the content negotiation part:

Suggested change
$format = $event->getRequest()->attributes->get('_format', 'json');
$format = $event->getPreferredFormat('json');

$context = array_merge($attribute->context, [
DenormalizerInterface::COLLECT_DENORMALIZATION_ERRORS => true,
]);
$format = $attribute->format ?? $request->getContentType() ?? 'json';
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we just use $request->toArray()? Or it may be too restrictive in terms of format?

{
$attribute = $this->getAttribute($argument);
$context = array_merge($attribute->context, [
DenormalizerInterface::COLLECT_DENORMALIZATION_ERRORS => true,
Copy link
Contributor

Choose a reason for hiding this comment

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

What about adding AbstractObjectNormalizer::ALLOW_EXTRA_ATTRIBUTES => false?

@Nyholm
Copy link
Member

Nyholm commented Jul 27, 2022

To be honest, I would recommend using API Platform instead of adding such code to Symfony.

I've been using this feature for a few months now. I must say that I really love it. It makes is simple to validate API input and have a clear contract with your clients.

I undertand part of API Platform is also solving this issue. But I dont think that would stop us from adding this to Symfony.

$response = new Response($this->serializer->serialize($throwable->getViolations(), $format), Response::HTTP_UNPROCESSABLE_ENTITY);
$this->logger->info('Invalid input rejected: "{reason}"', ['reason' => (string) $throwable->getViolations()]);

$event->setResponse($response);
Copy link
Member

Choose a reason for hiding this comment

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

}

$response = new Response($this->serializer->serialize($throwable->getViolations(), $format), Response::HTTP_UNPROCESSABLE_ENTITY);
$this->logger->info('Invalid input rejected: "{reason}"', ['reason' => (string) $throwable->getViolations()]);
Copy link
Member

Choose a reason for hiding this comment

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

IMHO, the reasons are not really useful in the log

$throwable = $event->getThrowable();
$format = $event->getRequest()->attributes->get('_format', 'json');

if (!$throwable instanceof InputValidationFailedException) {
Copy link
Member

Choose a reason for hiding this comment

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

Who throw this exception?

@faizanakram99
Copy link
Contributor

faizanakram99 commented Nov 3, 2022

Had a similar idea a couple of years ago 😃 #36093 , i am glad we are finally getting an implementation for it (in core)

@nicolas-grekas nicolas-grekas modified the milestones: 6.2, 6.3 Nov 5, 2022
@aszenz
Copy link

aszenz commented Dec 30, 2022

We use a similar RequestBody attribute to resolve request contents, but recently i found a flaw in it, it doesn't validate that the correct arguments are passed as per php doc block.

<?php
class Preference
{
    public const CATEGORY_1 = 'foo';
    public const CATEGORY_2 = 'bar';

    /**
     * TODO - These types are just for annotation, request body resolver won't validate these when constructing the object from request contents.
     *
     * @param non-empty-string           $option
     * @param self::CATEGORY_* $category
     */
    public function __construct(
        public string $category,
        public string $option,
        public mixed $value = null,
    ) {
    }
}

Does this pr aim to attempt validating against php doc blocks ?

@derrabus
Copy link
Member

Does this pr aim to attempt validating against php doc blocks ?

I don't think we should do that.

@nicolas-grekas
Copy link
Member

Shall we close in favor of #49138?

@GaryPEGEOT GaryPEGEOT closed this Feb 23, 2023
nicolas-grekas added a commit that referenced this pull request Apr 14, 2023
…and `#[MapQueryString]` to map Request input to typed objects (Koc)

This PR was merged into the 6.3 branch.

Discussion
----------

[HttpKernel] Create Attributes `#[MapRequestPayload]` and `#[MapQueryString]` to map Request input to typed objects

| Q             | A
| ------------- | ---
| Branch?       | 6.3
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Tickets       | #36037, #36093, #45628, #47425,  #49002, #49134
| License       | MIT
| Doc PR        | TBD

Yet another variation of how we can map raw Request data to typed objects with validation. We can even build OpenApi Specification based on this DTO classes using [NelmioApiDocBundle](https://github.com/nelmio/NelmioApiDocBundle).

## Usage Example 🔧
### `#[MapRequestPayload]`

```php
class PostProductReviewPayload
{
    public function __construct(
        #[Assert\NotBlank]
        #[Assert\Length(min: 10, max: 500)]
        public readonly string $comment,
        #[Assert\GreaterThanOrEqual(1)]
        #[Assert\LessThanOrEqual(5)]
        public readonly int $rating,
    ) {
    }
}

class PostJsonApiController
{
    public function __invoke(
        #[MapRequestPayload] PostProductReviewPayload $payload,
    ): Response {
        // $payload is validated and fully typed representation of the request body $request->getContent()
        //  or $request->request->all()
    }
}
```

### `#[MapQueryString]`

```php
class GetOrdersQuery
{
    public function __construct(
        #[Assert\Valid]
        public readonly ?GetOrdersFilter $filter,
        #[Assert\LessThanOrEqual(500)]
        public readonly int $limit = 25,
        #[Assert\LessThanOrEqual(10_000)]
        public readonly int $offset = 0,
    ) {
    }
}

class GetOrdersFilter
{
    public function __construct(
        #[Assert\Choice(['placed', 'shipped', 'delivered'])]
        public readonly ?string $status,
        public readonly ?float $total,
    ) {
    }
}

class GetApiController
{
    public function __invoke(
        #[MapQueryString] GetOrdersQuery $query,
    ): Response {
        // $query is validated and fully typed representation of the query string $request->query->all()
    }
}
```

### Exception handling 💥
- Validation errors will result in an HTTP 422 response, accompanied by a serialized `ConstraintViolationList`.
- Malformed data will be responded to with an HTTP 400 error.
- Unsupported deserialization formats will be responded to with an HTTP 415 error.

## Comparison to another implementations 📑

Differences to #49002:
- separate Attributes for explicit definition of the used source
- no need to define which class use to map because Attributes already associated with typed argument
- used ArgumentValueResolver mechanism instead of Subscribers
- functional tests

Differences to #49134:
- it is possible to map whole query string to object, not per parameter
- there is validation of the mapped object
- supports both `$request->getContent()` and `$request->request->all()` mapping
- functional tests

Differences to #45628:
- separate Attributes for explicit definition of the used source
- supports `$request->request->all()` and `$request->query->all()` mapping
- Exception handling opt-in
- functional tests

## Bonus part 🎁
- Extracted `UnsupportedFormatException` which thrown when there is no decoder for a given format

Commits
-------

d987093 [HttpKernel] Create Attributes `#[MapRequestPayload]` and `#[MapQueryString]` to map Request input to typed objects
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.