EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior

See #2789315-55: Create EntityPublishedInterface and use for Node and Comment, this is now blocking that issue.

Should be fixed in 8.2.x, because it's a bug.

Note this might as well live in the entity system component, because it's EntityResource and unclear stuff in the Entity system.

Per #2789315-57: Create EntityPublishedInterface and use for Node and Comment + 58, this does need to be a blocker.

Thanks, now taking a look at this!

I fixed a few minor things and made the patch apply again.

The PATCHing of a comment via JSON works fine.

The PATCHing of a comment via HAL+JSON fails again, with the same error as before:

A string must be provided as a bundle value

This makes it impossible to update Coment entities via HAL+JSON again, which is exactly what #2631774: Impossible to update Comment entity with REST (HTTP PATCH): bundle field not allowed to be updated, but EntityNormalizer::denormalize() requires it fixed.

The PATCHing of a comment via JSON works fine.

The PATCHing of a comment via HAL+JSON fails again, with the same error as before:

Of course, this was wrong, it's exactly the other way around. It works fine for HAL+JSON, it fails for JSON.

Investigating further.

I found the root cause. The problem is that the new logic in the patch runs after the $original_entity->get($field_name)->access('edit') call. So rather than excluding unchanged read-only fields from "update" field access checks, it's just adding more validation.

That, and the fact that it was looking at the item definition being read-only instead of the field definition being read-only (great catch, @amateescu!), caused the fails.

The good news is that this new patch no longer needs to make any changes to the test coverage :)

+++ b/core/modules/rest/src/Tests/UpdateTest.php
@@ -283,6 +283,7 @@ public function testUpdateComment() {
+    $comment_id = $comment->id();

@@ -300,7 +301,7 @@ public function testUpdateComment() {
-    $comment = Comment::load($comment->id());
+    $comment = Comment::load($comment_id);

These changes are also unnecessary, so reverting them.

Also, made those "if continue" statements clearer.

+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -4,6 +4,7 @@
+use Drupal\Core\Entity\ContentEntityInterface;

@@ -213,23 +214,18 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
+      elseif ($entity instanceof ContentEntityInterface

Finally, none of EntityResource supports config entities yet.

This is why we already have

    // @todo Remove when https://www.drupal.org/node/2164373 is committed.
    if (!$entity instanceof FieldableEntityInterface) {
      return;
    }

So, removing the ContentEntityInterface check for now. Even more so because checking this in a for-loop that's already iterating over fields means we're by definition already dealing with fieldable content entities.

Final review:

1. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -213,23 +213,16 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
   +      // It is not possible to set the language to NULL as it is automatically
   +      // re-initialized. As it must not be empty, skip it if it is.
   +      if (isset($entity_keys['langcode']) && $field_name === $entity_keys['langcode'] && $field->isEmpty()) {
   +        continue;
   

   Why does this entity key field need to be special-cased?

2. This needs extra test coverage: we now need a non-entity key field that's read-only to be sent in the request, to verify that that is now allowed, and wasn't allowed before.

#19:

Where do we prevent read-only fields from being updated if the user has edit access for them?

The responsibility lies with every entity access control handler. For example, Comment's:

  protected function checkFieldAccess($operation, FieldDefinitionInterface $field_definition, AccountInterface $account, FieldItemListInterface $items = NULL) {
    if ($operation == 'edit') {
      // Only users with the "administer comments" permission can edit
      // administrative fields.
      $administrative_fields = array(
        'uid',
        'status',
        'created',
        'date',
      );
      if (in_array($field_definition->getName(), $administrative_fields, TRUE)) {
        return AccessResult::allowedIfHasPermission($account, 'administer comments');
      }

      // No user can change read-only fields.
      $read_only_fields = array(
        'hostname',
        'changed',
        'cid',
        'thread',
      );
      // These fields can be edited during comment creation.
      $create_only_fields = [
        'comment_type',
        'uuid',
        'entity_id',
        'entity_type',
        'field_name',
        'pid',
      ];
…

It's wrong to add mechanism-specific (REST vs form vs …) validation constraints. Because for that, we already have … validation constraints: \Symfony\Component\Validator\Constraint and subclasses.

Yes, we probably should have a generic ReadOnlyConstraint.

#20:

What happens if we skip read-only fields all the time, even if they have a non-matching value?

That would be confusing, because the end user would expect that those values would have changed.

Or throw an access denied exception if the value doesn't match?

That's what this patch does. It does

throw new AccessDeniedHttpException("Access denied on updating field '$field_name'.");

… although, yes, it is indeed possible that a read-only field does not result in such an error because the burden is currently on the entity type's access control handler to ensure that read-only fields are in fact handled as such. What we can do, is either:

1. add a constraint for this
2. Update \Drupal\Core\Entity\EntityAccessControlHandler::checkFieldAccess() to not just blindly return AccessResult::allowed(), but instead returning that only if the field is not read-only. Read-only fields would get AccessResult::forbidden().

Also, how do we test this exactly and what is it that we need to test?

Add a read-only non-entity key base field to EntityTest, and assert that it is possible to send a value for it, as long as it matches the current value.

#22: well, if even you guys can't figure this out, we have a problem in the Entity Field API… tagging accordingly.

Marking NR to gather more feedback.

Interesting. I'd never thought about that distinction before. Denying access to a field results in validation error. So I'm not sure these concepts are entirely independent of each other?

But basically #24 is arguing that what I wrote in #22.2 is what we should do.

#26: so… that means a change/fix in the Moderation module ecosystem?

#29: <blockquote>[verbatim copy]</blockquote> is quickly posted to the other issue :)

So, #29 is pretty convincing. Which means:

1. #2826101: Add a ReadOnly constraint validation and use it automatically for read-only entity fields would have to be marked Closed (works as designed
2. this patch is what we must do instead.
3. However, we should solve it properly. This patch is currently still modifying \Drupal\rest\Plugin\rest\resource\EntityResource::patch with mechanism-specific logic (i.e. it only applies to entity updates via REST and not via forms, rather than being generic).
   So solving this properly means doing three things:
   1. Provide the correct behavior for all read-only fields, by doing what I said in #22.2:
      

      Update \Drupal\Core\Entity\EntityAccessControlHandler::checkFieldAccess() to not just blindly return AccessResult::allowed() for $op === 'edit', but instead returning that only if the field is not read-only. Read-only fields would get AccessResult::forbidden().

   2. Update \Drupal\comment\CommentAccessControlHandler::checkFieldAccess() (and any other checkFieldAccess()), to have all of its read-only fields upon $op === 'edit' return AccessResultAllowed when the pre-update value matches the post-update value.
4. Rather than replacing the special logic in EntityResource::patch() with different logic, removing it altogether. We want only
   

   if (!$updated_entity->get($field_name)->access('edit')) {
     throw new AccessDeniedHttpException("Access denied on updating field '$field_name'.");
   }
   

   Nothing else.

Since @amateescu has indicated at #2789315-66: Create EntityPublishedInterface and use for Node and Comment he does not intend to work on this anymore, picking this up to help unblock the Workflow initiative.

Implementing #31.3.2 seems impossible: \Drupal\Core\Entity\EntityAccessControlHandler::checkFieldAccess() does not receive the pre-update and the post-update values. In fact, it's not defined which value it does receive. Currently, it's the pre-update value. \Drupal\KernelTests\Core\Field\FieldAccessTest::testFieldAccess() asserts this behavior. This seems like an enormous flaw in the Entity Field API. But not one we can fix without breaking BC.

So, alas, we must keep the accept but discard values for read-only fields if they match the current values logic in EntityResource.

I tried to do what the second paragraph of #34 says: I tried to move the logic addition from \Drupal\Core\Entity\EntityAccessControlHandler::checkFieldAccess() to \Drupal\Core\Entity\EntityAccessControlHandler::fieldAccess(). But then I'm left wondering: where should I put it?

This is about the default field access. And every single field type extends FieldItemList. So I figure that FieldItemList::defaultAccess() is actually an even better location for this?

18:19:39 <timmillwood> WimLeers: ok, I need to drop soon, but any info you can add to the issue. I've got a bunch of issues stalled on EntityPublishedInterface.
18:20:03 <WimLeers> timmillwood: I will, and I'll try to push it forward. Hopefully I'll get it to green so you+others just need to review

I would love to see this fixed. Apparently, what we did in #2631774: Impossible to update Comment entity with REST (HTTP PATCH): bundle field not allowed to be updated, but EntityNormalizer::denormalize() requires it was wrong, despite an extensive review round.

The goal here is simple: get it right. That means not having any REST-specific logic (because JSON API, GraphQL, and so on would all have to duplicate it). All of the logic belongs in the Entity API.

We should have only this:

if (!$updated_entity->get($field_name)->access('edit')) {
  throw new AccessDeniedHttpException("Access denied on updating field '$field_name'.");
}

From what I can tell, the Entity API/Entity Field API simply is missing certain capabilities:

1. incomplete metadata to know whether a field is read-only at creation time (fully computed) or read-only after creation time (e.g. hostname for comment author) — see #2350429: Clarify the meaning of read-only fields and properties.
2. none of the docs specify whether the validation logic should receive the pre-update or post-update values — see #32
3. similar bugs to the one in EntityResource::patch() exist in the entity system. Quoting berdir:
   

   11:34:02 <berdir> WimLeers: well. if it makes you fell better, it is not alone. entity storage makes a similar wrong assumption and makes fields that have an entity key pointing to them required/NOT NULL in the schema
   

Should we move this to the entity system component?

#45: I don't quite understand how this solves this issue. The key problem here is when a request body includes a value for a field, and that value matches the current value. It's also about PATCHing, not about POSTing.
So, a clarifying comment would be very welcome.

@arshadcn Thanks for getting this going again!

You can PATCH read-only fields.

Indeed, the current logic allows for that — but only if the value is the same as the existing value:

        // Unchanged values for entity keys don't need access checking.
        if ($original_entity->get($field_name)->getValue() === $entity->get($field_name)->getValue()) {
          continue;
        }

Note that we need some read-only values to be sent on PATCH requests, see #2631774: Impossible to update Comment entity with REST (HTTP PATCH): bundle field not allowed to be updated, but EntityNormalizer::denormalize() requires it.

TIL \Drupal\Core\Field\FieldItemList::equals() exists, which may be useful here.

This is also definitely related to #2456257: [PP-1] Normalizers must set $entity->_restSubmittedFields for entity POSTing/PATCHing to work. Solving that issue may also solve this issue.

#2456257-65: [PP-1] Normalizers must set $entity->_restSubmittedFields for entity POSTing/PATCHing to work has (well, should have) zero failures. I think that will make this patch a lot simpler, because we can rely on Entity API to know which fields have been modified. And read-only fields cannot be modified, therefore much of the trickiness in this issue should go away.

#2456257: [PP-1] Normalizers must set $entity->_restSubmittedFields for entity POSTing/PATCHing to work is now also blocked: it's blocked on #2862574: Add ability to track an entity object's dirty fields (and see if it has changed). So now this has a chain of two blocking issues.

IS rewritten. In doing so, also clarifying the current state of the issue — see below.

As far as I can see, the solution can actually be much simpler if we solve it in the right place:

1. EntityResource::patch() must do its field-level access checking based on $entity->_restSubmittedFields: that is how the REST EntityResource plugin knows which fields were sent (which is itself a hack that is being fixed in #2456257: [PP-1] Normalizers must set $entity->_restSubmittedFields for entity POSTing/PATCHing to work)
2. all of these problems would go away if we had a Entity::getDirtyFields() API, because then we'd know to only check field access for fields that were actually changed

Therefore solving #2862574: Add ability to track an entity object's dirty fields (and see if it has changed) would unblock both #2456257: [PP-1] Normalizers must set $entity->_restSubmittedFields for entity POSTing/PATCHing to work and this issue:

1. #2456257 would be able to remove ->_restSubmittedFields.
2. This issue would be able to remove all of the conditionals to skip field access checking.

This issue is therefore not hard-blocked on #2456257, but "mostly parallel-blocked". But this issue will also need to replace foreach ($entity->_restSubmittedFields as $field_name) { with foreach ($entity->getDirtyFields() as $field_name => $field) {. And that's the entire scope of #2456257. Therefore this issue actually is blocked on #2456257.

We just discussed this in an Entity/Field API API-First Initiative blocker call. It was discussed in tandem with #2862574.

For this issue, the tl;dr is: use FieldItemList::equals(). For the full detail, see #2862574-32: Add ability to track an entity object's dirty fields (and see if it has changed).

Your analysis in #66 is spot-on. I've been working on updated test assertions.

In the mean time, I'm uploading your patch here, because it's an excellent starting point.

@tstoeckler++

Solved the problem @tstoeckler explained in #66: test coverage updated.

I think the updated test coverage also happens to be 10x more elegant. Rather than messing with normalizations, we clone the entity, modify its patch-protected fields (using the wonderful \Drupal\Core\Field\FieldItemListInterface::generateSampleItems() which saved me from creating a lot of insane testing infrastructure here!) and … just normalize that modified entity! Then after seeing one of the error messages, we just restore one of the original values, repeat until no more errors.

👏 Field API 👏

As a bonus, this allowed me to deprecate remove \Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase::removeFieldsFromNormalization(). It's safe to remove because it's entirely internal.

While waiting for test results, here's a review of #67:

1. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -202,41 +202,6 @@ public function post(EntityInterface $entity = NULL) {
   -  protected function getCastedValueFromFieldItemList(FieldItemListInterface $field_item_list) {
   

   This was introduced in #2751325: All serialized values are strings, should be integers/booleans when appropriate, glad to see it removed! Lots of complexity gone!

2. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -262,35 +227,15 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
   -        // @todo Work around the wrong assumption that entity keys need special
   -        // treatment, when only read-only fields need it.
   -        // This will be fixed in https://www.drupal.org/node/2824851.
   -        if ($entity->getEntityTypeId() == 'comment' && $field_name == 'status' && !$original_entity->get($field_name)->access('edit')) {
   -          throw new AccessDeniedHttpException("Access denied on updating field '$field_name'.");
   -        }
   

   OMG, this hacky work-around is no more! 🎉

   Well, hopefully we're nto removing this prematurely :)

3. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
   @@ -262,35 +227,15 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
   +      // Check access for all received fields, but only if the are being
   

   s/the/they/

My enthusiasm in #68 for \Drupal\Core\Field\FieldItemListInterface::generateSampleItems() was perhaps premature.

There's one significant problem with using it in tests. \Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem::generateSampleValue() will use one of the last 50 existing entities in the system.

Which means that for example for Node's revision_uid, it selects one of the 50 users created. Well, there's only 2 users (uid 0 and uid 1), perhaps 3 (uid 2 — if it's a REST test that needs authentication). So… there's an extremely high chance of it picking a value that is in fact not what we want. We could work around this by checking if it's an entity reference field, and then always setting a non-existing entity ID.

Similarly, for Node's promote field: that's a boolean, so there's a 50% chance that it'll pick the current value, rather than the (only) other value. We can work around this too.

So, use generateSampleValue(), but special case certain field types.

But even now, the Node REST test coverage is failing. Just slightly further: with #72, it's now passing the "expected 403 for disallowed field modification" assertions. But the PATCH request for Node that should succeed (200), does not (403). It says the path field is being modified, and this is not allowed. Regardless of whether this should be allowed or not, this looks like a bug in FieldItemList::equals(). #2846554: Make the PathItem field type actually computed and auto-load stored aliases last changed this stuff.

Turns out that we also need to call the ensureLoaded() helper that that issue added for PathFieldItemList::equals(). Then it works fine :)

With this, the Node REST tests should finally pass!

I spoke too soon, Node's HAL+JSON tests are still failing.

Apparently the fixes in this patch also remove the need for Node's HAL+JSON tests to have a different order of PATCH-protected field name validation errors. Simpler, yay!

Looks like #73 also fixed all failing CommentJson* REST tests!

And for CommentHalJson* REST tests, a similar problem to that in #74 exists — which can again be fixed by simply removing the override for HAL+JSON :)

(The special case for CommentJsonAnonTest and CommentHalJsonAnonTest continues to exist though, but at least the validation order for the field names is now consistent across all tests — so CommentHalJsonAnonTest needed a small update. It'd be great if PHP allowed expressions for static properties, then I could've expressed it as "parent property's values minus these", but alas.)

So #75 should be green! Fingers crossed :)

Now cleaning up:

1. extract a big chunk of logic into a helper method
2. clean up comments

Slightly larger patch/diffstat, but it makes for more maintainable tests.

Ready for review!

And while I think that last part in itself still stands, what I did not realize at that moment, is that an error at this point could not only lead to severe data integrity bugs, but more importantly to security exploits: in case we are no longer checking access on something that we should be checking.

I thought I brought that up at the meeting, perhaps I did not — it's been ~10 days at this point. But I remember you had a very convincing argument that made me not worried at all: ::equals() is also used by the SQL-backed entity storage to minimize DB writes. So if there's a bug, that'd also be a critical data integrity problem, and it would likely have already been noticed.
(Of course, the difference is that any data can be sent in a REST request.)

Also:

+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -262,35 +227,15 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
-        if ($this->getCastedValueFromFieldItemList($original_entity->get($field_name)) === $this->getCastedValueFromFieldItemList($entity->get($field_name))) {

This was already doing something very similar, but only for "entity key" fields, now we're doing it for all fields.

Therefore I'm not very concerned. Although also not 0% concerned. We'll never get to 0% though, because in the end we have to accept that there are many layers here: HTTP -> request processing system -> REST module -> Entity API -> Field API -> Typed Data API -> database layer -> database (not 100% accurate, but roughly). That's a lot of layers where things can go wrong, especially considering neither PHP nor Field API nor Typed Data API are strictly typed.

#84++
#85++

👏

Oh and #83++ for being so vigilant!

Here's first the automated test coverage that @tedbow performed manually. This should have lots of failing tests. This was easy because EntityResourceTestBase was already adding a field_rest_test field to every fieldable entity type, and \rest_test_entity_field_access() was already forbidding access to that field, for both view and edit operations.

And here's the fix, pretty much exactly @tstoeckler's proposal from #84, only changes are:

1. expanded comment
2. use the exact same error message in the throw new AccessDeniedHttpException() call, to prevent subtle information disclosure that way

This should be green! And hopefully RTBC'able! I can't believe we finally got to this point! 🎉

Any random fail in any of the REST test coverage will immediately make us suspect this bit. It'd have to be another field type, which we could then also special case there. I'm not too fond of retrying, because that has the potential for an endless loop. I'd rather have a failing test that I can fix.

If you feel very strongly about this though, I'm fine with doing what you suggested.

Yes, but for many field types there's actually not a problem, because they generate sample values that cannot possibly conflict with the test values we set in EntityResourceTestBase::createEntity() implementations. For example, \Drupal\text\Plugin\Field\FieldType\TextItemBase::generateSampleValue() generates values that will never result in a random failure.

The more I think about it, the more I think this is a well-intentioned idea with bad consequences. Calling generateSampleValue() repeatedly, perhaps 5 times like @tedbow suggests, doesn't fix the actual problem: random failures. They'll just happen 5 times less likely. But they may still happen. I'd rather have them happen more often so that we can add more field type-specific logic to that switch statement.

The probability for that converges to 0 and well, there are a lot of other cases where a similar probability causes the world to end.

😂👏

I have a hard time though understanding why adding a unrestricted loop with random values can lead to an infinite loop.

Because many of the generateSampleValue() implementations are not actually random, and can keep failing the ::equals() test. This is true for \Drupal\path\Plugin\Field\FieldType\PathItem::generateSampleValue() and for \Drupal\Core\Field\Plugin\Field\FieldType\EntityReferenceItem::generateSampleValue().

Anyway, I'm sensing most people favor the "retry until different" approach, so let's just go with that. Like I said in #91, I don't feel strongly about this.

1. We have tests, removing Needs tests.
2. This new helper method is not meant as an API, so marking @internal.

❤️ this community and its wonderfully geeky discussions :)

Suddenly, after >4 weeks of being RTBC and green, this patch is failing tests. So something must have changed in HEAD.

Apparently this was a bug in 8.4.x HEAD on July the 31st that has been fixed since. 3 subsequent test runs failed for the same reason: https://www.drupal.org/pift-ci-job/728165 + https://www.drupal.org/pift-ci-job/728200 + https://www.drupal.org/pift-ci-job/728321.

So back to RTBC.

Fixing 2 coding standards violations though — two unused use statements.

Added test runs for both 8.5.x and 8.4.x.

OMG YAY!

🎉

Also very glad to read that the detailed issue summary with patch progression explanation helped :)

@cburschka This was committed. Please create a new follow-up issue for that!

The vulnerability relies on a bug in the behavior of FieldItemList::equals() for computed fields.

In other words: this commit uncovered a bug deep in the Entity Field API. Impressive find, @cburschka! 👏

I've been thinking about how to add test coverage for this. Because:

1. a bug in EntityResourceTestBase::getModifiedEntityForPatchTesting(), which is being added in this patch/issue is what triggered this
2. but that actually just uncovered a root cause, which is FieldItemList::equals() having a bug
3. and @cburschka suggested changing EntityResource::patch() to protect against weaknesses in FieldItemList::equals()

… this is very complex :(

Adding test coverage for the first doesn't really make sense, because that's adding test coverage for test coverage. Adding test coverage for the second does make sense, but it's out of scope for this issue. Created a new issue for that: #2906600: FieldItemList::equals() doesn't work correctly for computed fields with custom storage. So that leaves the third, which is following @cburschka's recommendation and protecting against this general problem by changing EntityResource::patch()'s logic as quoted at the end of #117.

As I feared, this is causing a bunch of patches that were RTBC/close to RTBC to now be blocked on this, because this conflicts heavily with several of those. #2821077-72: PATCHing entities validates the entire entity, also unmodified fields, so unmodified fields can throw validation errors is the first victim.

Same for #2300677-192: JSON:API POST/PATCH support for fully validatable config entities.

Still working on this. I think I see a way forward now.

I found the explanation in #110 very hard to understand/interpret. But the steps to reproduce in #117 are fortunately much easier to understand. GETting a node and sending the GET response unchanged (except for changing its path alias) as a PATCH request results in the path alias being modified, even when the user does not have the create url aliases permission.

That's the security vulnerability that this issue introduced before it was reverted in #115. That should be easy enough to test.

Let's start by adding a test to prove that it was working in HEAD: PATCHing a node's path (URL alias) would result in a 403. Adding the exact same test coverage to the existing patch results in a 200.

So 2824851-123-HEAD_node_path_403_PASS.patch should come back green (it also serves as the interdiff), 2824851-123.patch should fail. (The latter is #106, the patch that was committed, plus this interdiff.)

#123 added sensible security coverage. But to really know for certain that things are working as expected, we should expand the test coverage to verify that granting the create url aliases permission then results in a 200 response: granting the necessary permission does allow the path field to be changed.

Note that there are three entity types that have a path field: node + term + media. The latter doesn't have any REST test coverage yet, so we leave that one out of consideration. Back when we worked on fixing path fields handling in #2846554: Make the PathItem field type actually computed and auto-load stored aliases, we chose to use NodeResourceTestBase to NOT GRANT the create url aliases permission by default, and to use TermResourceTestBase to GRANT the create url aliases permission by default, i.e. they'd be each other opposites.
For that reason, we should also add similar test coverage to TermResourceTestBase as #123 already did for NodeResourceTestBase. But rather than expecting a 403 response, we should expect a 200 response.

We again have a HEAD-relative patch and a patch that expands the committed-but-reverted patch. The interdiff is the same for both.

Now we have the test coverage we need!

The patches in #123 prove that in HEAD, a 403 is returned when appropriate when PATCHing a node's path, and it proves that the committed-but-since-reverted patch caused that to change.

The patches in #124 prove that in HEAD, changing path aliases didn't work at all, and it proves that the committed-but-since-reverted patch caused that to change.

🤔😱😭😩

This is the failure for the tests against HEAD:

1) Drupal\Tests\hal\Functional\EntityResource\Term\TermHalJsonAnonTest::testPatchPath
Failed asserting that Array &0 (
    0 => Array &1 (
        'alias' => '/llama'
        'pid' => 1
        'langcode' => 'en'
        'lang' => 'en'
    )
) is identical to Array &0 (
    0 => Array &1 (
        'alias' => '/llamas-rule-the-world'
        'pid' => 1
        'langcode' => 'en'
        'lang' => 'en'
    )
).

(The latter is what is expected. The former shows what is actually stored — in other words: nothing was changed.)

As you can see, the HEAD patch now has more failures than the committed-but-since-reverted patch, i.e. the committed-but-since-reverted patch fixes something that is broken in HEAD!

So what did the committed-but-since-reverted patch change to accidentally fix the fact that changing a path alias is completely broken in HEAD?

Glad you asked!

The changes in EntityResource::patch() cause PathFieldItemList to trigger PathItem::ensureLoaded() before EntityResource::patch() runs this:

$original_entity->set($field_name, $field->getValue());

That's a call to \Drupal\Core\Entity\ContentEntityBase::set(), which looks like this:

  public function set($name, $value, $notify = TRUE) {
    // Assign the value on the child and overrule notify such that we get
    // notified to handle changes afterwards. We can ignore notify as there is
    // no parent to notify anyway.
    $this->get($name)->setValue($value, TRUE);
    return $this;
  }

Note how the $original_entity->set(…) call results in setValue() being invoked on PathItem via ContentEntityBase::set().

So what's missing, is PathItem::setValue() also calling PathItem::ensureLoaded(). But doing that triggers an infinite loop… 😵 Fortunately that's easy to work around! 🙏

In other words: #2846554: Make the PathItem field type actually computed and auto-load stored aliases fixed many of the rough edges around the craziness abuse of APIs legacy remnants that the Path field type contains, but it still missed many more edge cases.

So this interdiff makes the HEAD-relative patch pass all tests. It makes \Drupal\Tests\rest\Functional\EntityResource\Term\TermJsonBasicAuthTest::testPatchPath() pass also (which is only asserting that the path field can be PATCHed). But it still doesn't make \Drupal\Tests\rest\Functional\EntityResource\Node\NodeJsonBasicAuthTest::testPatchPath() pass, because we still haven't determined the root cause of that.

All that we've done in #123 through #126 is:

1. add test coverage to reproduce the path field PATCHing security vulnerability @cburschka reported (#123
2. in doing so, discovered that path field PATCHing was completely broken in HEAD: it'd happily return a 200 response, but wouldn't actually ever have modified the path field! (#124 + #125)
3. and found a fix for that brokenness (this comment)

Next step: determine root cause + solution of why the committed-but-reverted patch prevents a 403 response from being sent when appropriate. Stay tuned (expect more late tonight or tomorrow).

I forgot to upload #126's attachments. 😅

The patch against HEAD in #126 had 3 more failures than expected: the failures in the NodeHalJson*Test tests. I'd thoroughly tested everything locally, but I forgot to test HAL+JSON locally.

Once that oversight is fixed, the comment in #126 makes sense again :) The solution is simple: don't unset() directly, but use removeFieldsFromNormalization(), so that fields normalized to links in HAL+JSON are also removed.

This interdiff applies only to the "HEAD" patch. The "real" patch is unchanged, because it removes removeFieldsFromNormalization() and the need for it.

#134 proves what we've been looking for: PATCHing of path aliases (PathItem fields) works as expected in HEAD, and no longer works as expected once this patch is committed. What is expected to be a 403 is a 200!

So let's already move that portion out of here, into a new issue. Moved the research + patches from #123 through #134 to #2909183: Add path alias (PathItem) field PATCH test coverage. The patch there is green and ready for final review!

While that's happening, let's still do the next step, quoting #126:

Next step: determine root cause + solution of why the committed-but-reverted patch prevents a 403 response from being sent when appropriate. Stay tuned (expect more late tonight or tomorrow).

#2909183: Add path alias (PathItem) field PATCH test coverage was committed! That basically means #134's 2824851-134-HEAD_paths_200_PASS.patch has been committed.

Let's rebase this :) We should still have the same 6 failures.

Now that that is confirmed, let's drive this to the finish line.

Next step: determine root cause + solution of why the committed-but-reverted patch prevents a 403 response from being sent when appropriate.

"Root cause" aka bug 1: FieldItemList::equals() incorrectly compares path fields because PathItem is unlike every other field

The primary root cause actually was already explained in #117:

The vulnerability relies on a bug in the behavior of FieldItemList::equals() for computed fields.
[…]
$columns is empty for the path field, as it does not use the field storage. Therefore the array_walk lines strip both arrays down to array(array()), returning TRUE regardless of the actual content (as long as both fields have the same number of items).

The general FieldItemList::equals() is still safe and correct. But it's not safe and correct for @FieldType plugins like path, because FieldItemList::equals() determines which values to compare based on $columns = $this->getFieldDefinition()->getFieldStorageDefinition()->getColumns(); — and the path field type is highly atypical there, because its implementation looks like this:

  public static function schema(FieldStorageDefinitionInterface $field_definition) {
    return [];
  }

It does this intentionally, because it doesn't want the Entity/Field storage system to store anything, since it's really referencing external data.

BUT! There's something else, that made the above problem worse.

"Makes things worse" aka bug 2: EntityResource::patch() sets the received value even when the value is supposedly the same

We have the root cause bug above that incorrectly claims the received path field value is equal to the stored value. But even then, EntityResource::patch() sets the received value on the entity object that it will later save. Which means it's overwriting the current value.

It's silly that EntityResource::patch() overwrites the existing value with the newly received value. Not only that, but it allows a simple bug to turn into a security risk (ability to modify data because the server thinks it's the same). (#117 already covered this at a high level.)

So let's first add explicit test coverage for bug 2, and then fix it.

In #137, the failed test results look like this:

1) Drupal\Tests\hal\Functional\EntityResource\Node\NodeHalJsonAnonTest::testPatchPath
Failed asserting that 200 is identical to 403.

/var/www/html/core/modules/rest/tests/src/Functional/ResourceTestBase.php:353
/var/www/html/core/modules/rest/tests/src/Functional/ResourceTestBase.php:372
/var/www/html/core/modules/rest/tests/src/Functional/EntityResource/Node/NodeResourceTestBase.php:261

But in #138, where I added explicit test coverage for the "makes things worse" bug, the failed test results look like this:

1) Drupal\Tests\hal\Functional\EntityResource\Node\NodeHalJsonAnonTest::testPatchPath
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-/llama
+/llamas-rule-the-world

/var/www/html/core/modules/rest/tests/src/Functional/EntityResource/Node/NodeResourceTestBase.php:263

In other words: explicit test coverage for the "makes things worse".

And here's the fix for the "makes things worse" bug!