PATCHing entities validates the entire entity, also unmodified fields, so unmodified fields can throw validation errors

Here is some quick start for that.

This really just reuses the methods which have been also used for the HTML form, see \Drupal\Core\Entity\Entity\EntityFormDisplay::validateFormValues.

In general though this should have some form of security review. Its something which should be better safe than sorry.

Yes, conceptually this makes sense. Like #6 says, this is equivalent to the form handling. See also \Drupal\Core\Entity\ContentEntityForm::validateForm() for another usage. Thus, unassigning.

Some notes on the actual patch:

1. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -297,6 +297,7 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
   +    $changed_fields = array_keys($entity->_restSubmittedFields);
        $this->validate($original_entity);
   

   Let's actually use $changed_fields ;-)

2. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResourceValidationTrait.php
   @@ -19,10 +19,14 @@
   +   * @param string[] $changed_fields
   +   *   An array of changed fields. If specified filter out the non applying
   +   *   violations.
   ...
   -  protected function validate(EntityInterface $entity) {
   +  protected function validate(EntityInterface $entity, array $changed_fields = []) {
   

   So passing an empty list list of fields actually does no filtering at all. That kind of makes sense, but is also somewhat confusing so I think we should at least document this. Alternatively we could do some NULL vs. [] dance, but not sure that is better...

3. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResourceValidationTrait.php
   @@ -33,6 +37,13 @@ protected function validate(EntityInterface $entity) {
   +      $entity_level_violations = $violations->getEntityViolations();
   +      $entity_level_violations->addAll($violations->filterByFields($changed_fields));
   +      $violations = $entity_level_violations;
   

   Why do we need to do this "workaround" of fetching the entity-level violations? Why is $violations->filterByFields($changed_fields) not sufficient?

So passing an empty list list of fields actually does no filtering at all. That kind of makes sense, but is also somewhat confusing so I think we should at least document this. Alternatively we could do some NULL vs. [] dance, but not sure that is better...

Good point. Let's refactor the code to no longer require this weird dance ...

Let's actually use $changed_fields ;-)

Heh fair point.

Why do we need to do this "workaround" of fetching the entity-level violations? Why is $violations->filterByFields($changed_fields) not sufficient?

As far as I understand its used in the form to be able to separate the violations which have to apply to certain widgets from other errors.

This will become even better once #2456257: [PP-1] EntityResource relies on a normalizer's deserialize method adding _restSubmittedFields lands, which in turn depends on #2862574: Add ability to track an entity object's dirty fields. And that last issue makes me wonder whether we should actually be updating \Drupal\Core\Entity\ContentEntityBase::validate() instead? Shouldn't that code be responsible for only validating fields that were actually modified?

We could do that in a follow up IMHO, given that the BC evaluation would be a different one.

Here is some test coverage.

Turns out the implementation was wrong. It filters out entries rather does what array_filter does, aka. keep elements unless they meet a certain condition.

I accidentally had some test cases commented out

Let's fix some failing tests ...

That's file it against 8.4.x

Just a reroll

This could fix a lot of the problems ...

+1

Who can we ping to ensure this is fine? I think either berdir or tstoeckler would be fine.
Fago would be nice as well, but

Working on this reroll right now

Fixing some test failures ...

Let's see whether this fixes more.

Ideally, we'd be able to avoid this, because it'd causes REST test failures for contrib/custom entity types.

We could check for $this->entity->hasField('rest_test_validation') or so ..., what do you think?

Let's get this started then.

Oh, interesting, so that means we're now only testing it against \Drupal\Tests\rest\Functional\EntityResource\EntityTest\EntityTestResourceTestBase.

Well you either test against all or again one or none :P So you suggest we add this field to all the core entities?

Otherwise, we could just hardcode this to

I could easily imagine that we want to add this test coverage potentially to more than one entity type, which is why I went with that particular solution.

What about adding it to all core entity types, but ensure that contrib tests not fail?

Let's give it a try ...

This does not make any sense. The constraint should be stored in the FieldConfig config entity.

Right, this is exactly what I added it as a base field.

I should've known you knew about these edge cases :) But I suspect you agree with the approach in #49?

For me both approaches are equal.

Maybe one could argue that testing the capability with custom added base fields is actually some level of test coverage which is worth dealing with.

Maybe one could argue that testing the capability with custom added base fields is actually some level of test coverage which is worth dealing with.

We have quite some test coverage around entities and their basefields as well as configurable fields. We don't have yet an example of a base field added via a hook in rest API. I know this is a small detail, but you never know, there are infinite ways how something could break.

This review starts from #55 I didn't see patches past that. Will check

1. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -1051,23 +1097,41 @@ public function testPatch() {
   +        // Change the rest_test_validation field to prove that then its validation
   +        // does run. In subsequent test assertions, it will not be modified, and
   +        // then should not trigger validation errors.
   +        $valid_request_body = $this->getNormalizedPatchEntity() + $this->serializer->normalize($this->entity, static::$format);
   +        $request_options[RequestOptions::BODY] = $this->serializer->serialize($valid_request_body, static::$format);
   +        $response = $this->request('PATCH', $url, $request_options);
   +        $this->assertResourceErrorResponse(422, "Unprocessable Entity: validation failed.\nrest_test_validation: REST test validation failed\n", $response);
   ...
   +    $valid_request_body = array_diff_key($valid_request_body, ['rest_test_validation' => NULL]);
   

   In this comment we say that we will prove that the validation will run by changing it and setting it to "ALWAYS_FAIL". This does seem to prove that.

   But then we say

   In subsequent test assertions, it will not be modified, and
   then should not trigger validation errors.

   But we change the value of rest_test_validation to NULL.
   So I look at RestTestConstraintValidator and it has

   if (!empty($value[0]['value']) && $value[0]['value'] === 'ALWAYS_FAIL') {
     $this->context->addViolation($constraint->message);
   }
   

   So regardless of the whether rest_test_validation was modified or not the NULL value in the field would never trigger this violation because it is empty, correct?

   So I am not sure what this proves or at least that the comment is correct.

   UPDATE:
   I read this line incorrectly
   $valid_request_body = array_diff_key($valid_request_body, ['rest_test_validation' => NULL]);
   I thought it was setting the field value to NULL when in fact it is removing the rest_test_validation key from the array because we are not all entities will have this field.

   But a few lines before this code we have the comment:

   // 200 for well-formed PATCH request that sends all fields (even including
   // read-only ones, but with unchanged values).
   

   Which is actually not true in the case that the entity has the field rest_test_validation in which case we are not sending all fields because we just removed 1 from the request.

   I think what tripped me up on this is the first comment I mentioned above says:

   In subsequent test assertions, it will not be modified,

   When in fact it is actually not sent so it is confusing to say it is not modified which read to mean sent but has the current value.

   Nit related to my confusion:
   Could we change
   $valid_request_body = array_diff_key($valid_request_body, ['rest_test_validation' => NULL]);
   to
   unset($valid_request_body['rest_test_validation']);
   For me that would be much clear as to what is happening.

2. Another thing that I find confusing
   

   protected function validateWithFilteredFields(EntityInterface $entity
   , array $changed_fields) {

   The phpdoc says

      * @param string[] $changed_fields
      *   An array of changed fields. If specified filter out the non applying
   

   But actually this is called as
   $this->validateWithFilteredFields($original_entity, $entity->_restSubmittedFields);

   So when I first saw $changed_fields I assumed since this issue fields that were modified from their current values, probably since this issue deals with "unmodified fields".

   Would it make sense to change $changed_fields to $submitted_fields?

3. From the issue summary
   

   For example: node body uses a certain format that user A is not allowed to use, but when user A PATCHes only the node title, they still get a validation error.

   So is this suppose to mean that with this patch user A should be able to send an unchanged body field back in the patch and even if the user doesn't have access to the current format, say full_html the patch should still work?

   If so I don't think this patch fixes the problem. I tried this locally and it does not work.
   I still get

   {
     "message": "Unprocessable Entity: validation failed.\nbody.0.format: The value you selected is not a valid choice.\n"
   }
   

   Or is it suppose to mean that User A can patch the entity has long has they do not send back the body field which does work?

   This at least needs an issue summary update to describe what the desired functionality is.

1. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -1093,19 +1093,20 @@ public function testPatch() {
   +        // Set the rest_test_validation field to always fail validation, which
   +        // allows asserting that not modifying that field does not trigger
   +        // validation errors.
   +        $this->entity->set('rest_test_validation', 'ALWAYS_FAIL');
   +        $this->entity->save();
   

   Because we moved this $this->entity->save() to after the expected failed patch request and we removed this line if the previous patch

   $valid_request_body = array_diff_key($valid_request_body, ['rest_test_validation' => NULL]);
   

   Then now we are truly testing sending back an unmodfied field that would fail on validation if validation were called! 🙌

   This address the type of the problem described in the issue summary regarding a format for the body field.

   I tested this manually and now it works! 🎉

2. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -247,11 +248,17 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
   +      // Determine which fields actually changed, i.e. which ones should be
   +      // set & validated.
   +      if (!$original_field->equals($field)) {
   +        $changed_fields[] = $field_name;
   +        $original_entity->set($field_name, $field->getValue());
   +      }
        }
    
        // Validate the received data before saving.
   -    $this->validateWithFilteredFields($original_entity, $entity->_restSubmittedFields);
   +    $this->validateWithFilteredFields($original_entity, $changed_fields);
   

   This looks good so we validate but filter unchanged fields! So the test should pass!

Yeah follow up was mistake because I had the form up open when you removed the tag.

Added a single sentence to the Issue Summary to described desired behavior.

RTBC 😃

+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResourceValidationTrait.php
@@ -23,6 +24,42 @@
   protected function validate(EntityInterface $entity) {

Instead of splitting this method into 4 other methods, wouldn't it be simpler to simply add a new $filter_fields = [] parameter which, if provided, would call filterByFields() on the violations list?

Instead of splitting this method into 4 other methods, wouldn't it be simpler to simply add a new $filter_fields = [] parameter which, if provided, would call filterByFields() on the violations list?

One reason I went with the 4 methods is that their intensions are really clear by their name.

Reading #9.2 again, @tstoeckler actually wanted just better documentation for the new $changed_fields parameter, to make it clear that passing an empty array means that no filtering is done on the violations list.

In addition to that, #9.3 was not really addressed properly with the answer from #10:

As far as I understand its used in the form to be able to separate the violations which have to apply to certain widgets from other errors.

@dawehner based his initial code and the reply from #10 on the code from \Drupal\Core\Entity\ContentEntityForm::flagViolations(), but that code is actually just a helper method for deciding who is responsible for converting entity validation errors to form errors. The form itself is responsible for reporting entity-level violations and field widget are responsible for reporting field-level violations.

Instead, we should base REST's code on \Drupal\Core\Entity\ContentEntityForm::validateForm(), which gets the full list of violations and then filters it by field access and fields that were actually changed by the form.

This means that we can simplify the validateWithFilteredFields() method to this:

  protected function validateWithFilteredFields(EntityInterface $entity, array $changed_fields) {
    if ($violations = $this->doValidation($entity)) {
      $violations->filterByFields(array_diff(array_keys($entity->getFieldDefinitions()), $changed_fields));
      $this->processViolations($violations);
    }
  }

Note that I also removed the instanceof check because doValidation() already has it. Now.. doesn't this look exactly like validate() with an added filterByFields() call? It sure does to me, which made me wonder if we really need a different method for this. My answer was "no", so if we merge validateWithFilteredFields() with validate(), then the new doValidation() doesn't make sense on its own, so we can inline it as well in the parent validate() method.

This brings us to only two methods, validate() and processViolations(). Now, the doxygen block from validate() says that it throws an UnprocessableEntityHttpException if validation errors are found, but it's actually processViolations() the one who throws that exception (and doesn't document it in the docs), so, in my mind, there's no need for that additional call to a different method just to throw an exception that should've been thrown by the parent call.

In conclusion, I think the code from #4 was way better than the changes from #10, and @tstoeckler concerns could easily be addressed by documenting the new $changed_fields param better.

The patch from #62 does not apply to HEAD anymore because #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior was reverted, but here's an interdiff for it which brings us back to one method and also addresses @tstoeckler's review from #9.

I'm trying a reroll now and then address @amateescu latest patch ...
@amateescu I like how your interdiff simplifies things ...

I would actually suggest to postpone this on #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior given the test code conflicts quite heavily ...

So ::checkPatchFieldAccess() contains this:

    // If the user is allowed to edit the field, it is always safe to set the
    // received value. We may be setting an unchanged value, but that is ok.
    if ($original_field->access('edit')) {
      return TRUE;
    }

I feel like we should just change that to:

if ($original_field->access('edit') && !$original_field->equals($field)) {

(and adapt the comment accordingly. Then we can save the if in the calling code. That will mean we will no longer call ->set() on field items that are equal, but we already decided in #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior that that's not a behavior change because ->set() is not a no-op for equal fields that's a bug.

I was about to write a lengthy comment on the reasoning for #90 but in the process I realized one thing. #90 was just about suggesting a different "placement" of the code with no functional change relative to the patch.

However, I am now convinced that the patch itself contains a funtional bug. Given how the #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior I would not be surprised for @cburschka (or anyone else, for that matter) to prove me wrong on this, but for now I am convinced nonetheless 😉.

The issue summary reads:

If a REST request sends a field that the user has access to but is unchanged from the current field value the validation should not be run on this field.

The sublety lies with the word "access". Per #2824851: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior and given that a user does not have access to edit a given field we do not allow submitting that field even if it is unchanged if the user does not have view access on it. As discussed at length in that issue this prevents information disclosure as otherwise the user could just submit a bunch of different values to this field and as soon as they would not get a 403 for a response they would have found out the current field value - in effect having "viewed" it.

This issue is now about running field validation, but I think a similar logic applies there, as well, in a sort of "reverse" way. If a user has "edit" access to a field, but not "view" access, the current patch would not run field validation of the field value matches the current way. That way the resulting response depends (or at least may depend) on the current value which in turn may allow the user to figure the field value.

It is a bit far-fetched - but not impossible - to conjure up an actual "exploit" with this. In particular, if the current value of a field is invalid for whatever reason. This is (not normally the case, but) entirely possible because while forms and Rest do perform validation by default, it is not enforced when saving an entity manually and even through forms there can be situations where not all fields are valid. An attacker (that - again - has edit but not view access to this field) could then try to PATCH all kinds of invalid values to this field and if they hit the current one, a 200 response would be returned, which would neatly inform them of the current (invalid) field value, in effect having granted them view access.

So I think we actually need to check view access before excluding fields from validation because they are unchanged. Luckily, this will not be a very invasive change. In fact, we can just switch the order of the two if-hunks in ::checkPatchFieldAccess() (and I guess we will have to update the respective comments a bit). Then we can just remove the if that the current patch adds entirely and just populate $changed_fields iff we call ::set().

Tagging "Needs tests" because I think we want something along the lines of the above "attack scenario" so that we do not regress here.

The idea in #93 is actually quite neat, I think, but I don't understand how it's related to the suggestion in #90, to be honest, and it also doesn't fix the above. That doesn't mean we shouldn't add it, though, in fact I really see reason not to.

Wow, thanks for the quick turnaround! Looks really good, only minor points.

1. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -238,12 +240,18 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
   +    if (empty($changed_fields)) {
   

   Could just be if($changed_fields)

2. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResourceValidationTrait.php
   @@ -14,16 +15,20 @@
   +   *   list to include only this set of fields. Defaults to an empty array,
   +   *   which means that all violations will be reported.
   

   I know this comment is there because I explicitly requested it, but looking at the code now again, I'm not actually sure it is true?! If $changed_fields is empty, all fields will be passed to filterByFields() which means that no (field-level) violations will be reported at all, unless I'm mistaken.

3. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -1151,6 +1153,36 @@ public function testPatch() {
   +      // does run. In subsequent test assertions, it will not be modified, and
   +      // then should not trigger validation errors.
   

   Seems this is no longer true?

4. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -1151,6 +1153,36 @@ public function testPatch() {
   +      $this->assertResourceErrorResponse(422, "Unprocessable Entity: validation failed.\nrest_test_validation: REST test validation failed\n", $response);
   +
   +    }
   

   Unnecessary empty line.

Thanks, looks great to me. Still needs an issue summary update, but I wouldn't RTBC regardless, because another pair of eyes wouldn't hurt.

Needs work to remove the todo.

The other point may just be me forgetting the details of this issue.

1. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -229,6 +229,8 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
   +    // @todo Remove $changed_fields in https://www.drupal.org/node/2906129.
   +    $changed_fields = [];
   

   #2906129: [PP-1] Update ContentEntityBase::validate() to only validate dirty fields issue was closed as "Works as designed" by @amateescu. So we should probably remove this @todo.

2. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
   @@ -1151,6 +1153,39 @@ public function testPatch() {
   +      // Information disclosure prevented: when a malicious user correctly
   +      // guesses the current invalid value of a field, ensure a 200 is not sent
   +      // because this would disclose to the attacker what the current value is.
   +      // @see rest_test_entity_field_access()
   +      $response = $this->request('PATCH', $url, $request_options);
   +      $this->assertResourceErrorResponse(422, "Unprocessable Entity: validation failed.\nrest_test_validation: REST test validation failed\n", $response);
   

   If the user doesn't have access to view the field should return a 422 response code but not return the actual message. Would there ever be a case where the validation message gives away the current value?

   For instance what if in content_moderation you have transitions
   draft -> publish
   publish -> draft

   but not
   publish -> publish

   i.e. you can't actually make any changes to entity without take it out of published status.

   Would the message "published to publish not a valid transition" give away the current value.?

   We want to run validation to prevent disclosure but for the user without access to view the field what is value of displaying the actually validate message to that user?

@Wim Leers thanks for the explanation. Re-reading the issue and patch that looks good!

🎉RTBC!🎉

+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResourceValidationTrait.php
@@ -14,16 +15,19 @@
+   * @param string[] $changed_fields
+   *   (optional) An array of field names. If specified, filters the violations
+   *   list to include only this set of fields.

@@ -33,6 +37,15 @@ protected function validate(EntityInterface $entity) {
+    // Filter violations by the changed fields. New entities only have changed
+    // fields.
+    if (!$entity->isNew()) {
+      if (empty($changed_fields)) {
+        throw new \LogicException("Saving an entity without changing any fields does not make sense.");
+      }
+      $violations->filterByFields(array_diff(array_keys($entity->getFieldDefinitions()), $changed_fields));
+    }

I think this name could be improved. I.e. be $fields_to_validate As the point here is that this is a list of fields to validate. And we should document the default behaviour is to validate all fields.

Also I'm concerned about BC since I think not providing the value should continue with todays behaviour of validating all the fields. So I think the exception thrown here might be an API break.

@alexpott I think these changes sound good.

1. I changed the name to $fields_to_validate.
2. Because to validate() these is just generic list of field names now I removed the logic of not calling \Drupal\Core\Entity\EntityConstraintViolationListInterface::filterByFields() if the entity is new. This logic only made sense if these were "changed" fields. This should not make a difference because the only place this method is being called with a value for this optional parameter is \Drupal\rest\Plugin\rest\resource\EntityResource::patch() and because it is PATCH request this will never be new entity.

3. And we should document the default behaviour is to validate all fields.

   The documentation of $fields_to_validate provides documentation now but I added more documentation for the method.

   But I also added a bit about fields the users has access to since the default behavior is not to validate all fields.

4. Also I'm concerned about BC since I think not providing the value should continue with todays behaviour of validating all the fields.

   I have remove this exception. I don't think it will cause any test fails but will see.

   I could see where this could be a BC break. If you created a decouple entity entity form.

   1. User loads the edit form
   2. User hits save without making any changes
   3. Decouple app makes a PATCH request with unchanged entity fields
   4. Currently there would no error
   5. Decouple app displays "You entity saved"

   If we threw an exception here that would be a BC break that app would not know how to handle.
   Also maybe the user was just saving the form with no changes to update "changed" field(or another computed field) to get the entity at the top of a list ordered by that field. So it actually could make sense to save with no changes.

Committed and pushed 73769ef5f8 to 8.7.x and 55ffe4fa1f to 8.6.x. Thanks!

Backporting to 8.6.x as an important bug fix for API first. AFAICS the changes here are totally BC compatible and the behaviour changes should only benefit real sites.