Problem/Motivation

Discovered by Berdir at #2789315-52: Create EntityPublishedInterface and use for Node and Comment and subsequent comments.

Where/when was this bug introduced?

#2631774: Impossible to update Comment entity with REST (HTTP PATCH): bundle field not allowed to be updated, but EntityNormalizer::denormalize() requires it introduced this bug. But before describing what bug it introduced, let's explain what that issue was trying to achieve: it wanted to make it possible to PATCH comment entities at all. For the entity denormalizer to succeed, it needs to know the bundle. So, requests must include the comment type. After denormalizing the entity, one must denormalize the fields in the entity. Before denormalizing a field, field access is checked. But modifying an entity's bundle is not allowed. End result: sending the bundle was necessary in step 1, but forbidden in step 2. Hence PATCHing comment entities was impossible.

To solve this, #2631774 needed to disable field access checking for at least the bundle field: as long as it was being sent unchanged, it should be allowed. But Field API does not have a facility for detecting changes made to fields, so some logic had to be added at the REST level to manually check certain fields. But which fields?

To not hardcode this, we tried to find what aspect determined which fields are safe to send, but should match the current value. There already was one case of this: the langcode entity key field. bundle is another key. Therefore it seemed safe to make the jump to assuming that all entity keys are in fact safe to treat this way. After all, their name indicates that they are "keying the entity", i.e. together determine a unique entity. This impression is supported by the fact that:

  1. name — "keys" have a very specific meaning in this software context
  2. the langcode key was already being special-cased in a similar way
  3. we'd want similar special-casing for e.g. the UUID field too, which is another entity key
  4. the same reasoning/assumptions were used by \Drupal\Core\Entity\Sql\SqlContentEntityStorageSchema::getSharedTableFieldSchema() which ensures entity keys are NOT NULL, hence seemingly confirming this is the right way to use it — being fixed in #2841291: [PP-1] Fix NOT NULL handling in the entity storage and 'primary key' changes when updating the storage definition of an identifier field now
  5. … big long-time contributors like @dawehner and @larowlan RTBC'd this, so they also didn't notice…

So this solution was committed.

How did we discover this solution was incorrect?

This worked well for a while: #2631774 was committed on May 31, 2016. It shipped with Drupal 8.2. Nobody complained for a long time.

Then the Workflow Initiative happened, and #2789315: Create EntityPublishedInterface and use for Node and Comment happened for that initiative, in November 2016. That was adding status to the entity keys. Which is an entity key that does not uniquely identify an entity. It can be modified. This then caused test failures, which I was called in to comment on: #2789315-53: Create EntityPublishedInterface and use for Node and Comment. Turns out "entity keys" only exist for "fast access" purposes, or at least that's what they exist for today!

So "entity keys" are misleadingly named — created an issue for that: #2885411: "Entity keys" aren't actually keys — rename to avoid misinterpretation.

Proposed resolution

Attempt 1: insecure
@amateescu first posted a patch in #6 that simply ignores any values that are sent that match the current value. But @Berdir pointed out in #8 that this leads to an information disclosure vulnerability. So this solution was ditched.
Attempt 2: broken
So @amateescu reworked the patch to always check field access. But in doing so, he re-introduced the bug that we originally fixed: it was once again impossible to send an entity's bundle because field access disallowed it, but it was required for denormalization: exactly the same problem that #2631774: Impossible to update Comment entity with REST (HTTP PATCH): bundle field not allowed to be updated, but EntityNormalizer::denormalize() requires it solved!
Attempt 3: poor performance
Then @Wim Leers and @amateescu had a try: @Berdir recommended to rely on \Drupal\Core\TypedData\DataDefinitionInterface::isReadOnly() instead. We tried to do that in #2826101: Add a ReadOnly constraint validation and use it automatically for read-only entity fields. But then @Berdir pointed out in #24 and #29 that using a validation constraint is the wrong tool for the job. Not to mention that #2826101 quickly got stuck on computed fields' semantics being ambiguously defined: #2392845: Add a trait to standardize handling of computed item lists.
Attempt 4: stuck
@Wim Leers tried to take @Berdir's #29 to heart and bring all information together in a new proposal: #31. It involved 3 things:
  1. Providing correct field access control behavior for read-only fields by modifying the default field access logic: forbid them
  2. Updating the comment entity access control handler's field access logic, to allow in case the pre- and post-update values match
  3. Remove all specialty logic in EntityResource::patch()

However, the second point turned out be impossible due to the fact that Field API does not pass pre- and post-update values to field access checking logic.

Attempt 5: unclear
@tedbow tried to solve it by introducing DataDefinitionCreatableInterface, to allow values to be provided during entity creation even if the field is read-only.
Attempt 6: currently proposed solution!
However, as far as @Wim Leers 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 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.

Remaining tasks

  1. Land #2862574: Add ability to track an entity object's dirty fields.
  2. Land #2456257: [PP-1] Normalizers must set $entity->_restSubmittedFields for entity POSTing/PATCHing to work.
  3. Roll patch.
  4. Review.
  5. Commit.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#217 2824851-217.patch21.77 KBWim Leers
#217 interdiff.txt1.19 KBWim Leers
#211 2824851-211.patch21.39 KBWim Leers
#203 interdiff-202-203.txt3.42 KBeffulgentsia
#203 2824851-203.patch21.92 KBeffulgentsia
#202 2824851-196.patch22.86 KBWim Leers
#199 2824851-199-user_pass_edge_case-FAIL.patch25.51 KBWim Leers
#199 2824851-user_pass_edge_case_test_only-FAIL.patch2.71 KBWim Leers
#196 2824851-196.patch22.86 KBWim Leers
#194 2824851-194.patch24.84 KBWim Leers
#194 interdiff.txt1.94 KBWim Leers
#193 2824851-193.patch26.72 KBWim Leers
#193 interdiff.txt3.24 KBWim Leers
#191 2824851-191.patch25.34 KBWim Leers
#191 interdiff.txt4.91 KBWim Leers
#183 2824851-183.patch24.19 KBWim Leers
#183 interdiff.txt3.94 KBWim Leers
#181 2824851-181.patch23.56 KBWim Leers
#181 interdiff.txt1.09 KBWim Leers
#175 2824851-175.patch23.12 KBWim Leers
#169 2824851-169-combined.patch50.92 KBWim Leers
#169 interdiff.txt1.98 KBWim Leers
#167 2824851-167-combined.patch51.74 KBamateescu
#159 2824851-159.patch26.75 KBWim Leers
#159 interdiff.txt3.38 KBWim Leers
#158 2824851-158.patch27.18 KBWim Leers
#158 interdiff.txt3.09 KBWim Leers
#157 2824851-157.patch24.16 KBWim Leers
#157 interdiff.txt1.66 KBWim Leers
#148 2824851-148.patch23.51 KBWim Leers
#148 interdiff-138-148.txt1.31 KBWim Leers
#148 interdiff.txt2.6 KBWim Leers
#142 2824851-142.patch23.44 KBWim Leers
#142 interdiff.txt2.69 KBWim Leers
#138 2824851-138.patch23.27 KBWim Leers
#138 interdiff.txt1.33 KBWim Leers
#137 2824851-137.patch22.44 KBWim Leers
#134 2824851-134.patch28.04 KBWim Leers
#134 2824851-134-HEAD_paths_200_PASS.patch7.82 KBWim Leers
#134 interdiff-HEAD-patch.txt2.21 KBWim Leers
#130 2824851-126.patch28.04 KBWim Leers
#130 2824851-126-HEAD_paths_200_PASS.patch7.8 KBWim Leers
#130 interdiff.txt1.67 KBWim Leers
#124 2824851-124.patch26.43 KBWim Leers
#124 2824851-124-HEAD_paths_200_FAIL.patch6.17 KBWim Leers
#124 interdiff.txt4.39 KBWim Leers
#123 2824851-123.patch23.06 KBWim Leers
#123 2824851-123-HEAD_node_path_403_PASS.patch2.8 KBWim Leers
#112 2824851-112-followup.patch1 KBcburschka
#106 interdiff.txt760 bytesWim Leers
#106 rest_patch__field_equals-2824851-106.patch20.31 KBWim Leers
#99 rest_patch__field_equals-2824851-99.patch19.97 KBWim Leers
#99 interdiff.txt1.24 KBWim Leers
#89 rest_patch__field_equals-2824851-89.patch19.84 KBWim Leers
#89 interdiff.txt2.68 KBWim Leers
#88 rest_patch__field_equals-2824851-88.patch18.45 KBWim Leers
#88 interdiff.txt2.26 KBWim Leers
#76 rest_patch__field_equals-2824851-76.patch17.09 KBWim Leers
#76 interdiff.txt5.07 KBWim Leers
#75 rest_patch__field_equals-2824851-75.patch15.72 KBWim Leers
#75 interdiff.txt1.86 KBWim Leers
#74 rest_patch__field_equals-2824851-74.patch13.93 KBWim Leers
#74 interdiff.txt950 bytesWim Leers
#73 rest_patch__field_equals-2824851-73.patch13.05 KBWim Leers
#73 interdiff.txt886 bytesWim Leers
#72 rest_patch__field_equals-2824851-72.patch12.24 KBWim Leers
#72 interdiff.txt3.78 KBWim Leers
#68 rest_patch__field_equals-2824851-68.patch10.41 KBWim Leers
#68 interdiff.txt5.85 KBWim Leers
#67 rest_patch__field_equals-2824851-67.patch4.62 KBWim Leers
#55 interdiff-2824851-50-55.txt532 bytesarshadcn
#55 2824851-55-nid.patch953 bytesarshadcn
#50 2824851-50-nid.patch944 bytesarshadcn
#50 2824851-50-uuid.patch945 bytesarshadcn
#50 2824851-50-revision-uid.patch953 bytesarshadcn
#45 interdiff-39-45.txt3.99 KBtedbow
#45 2824851-45.patch7.75 KBtedbow
#39 interdiff.txt1.83 KBWim Leers
#39 2824851-39.patch4.15 KBWim Leers
#36 interdiff.txt942 bytestimmillwood
#36 2824851-36.patch3.94 KBtimmillwood
#32 interdiff.txt3.05 KBWim Leers
#32 2824851-32.patch3.21 KBWim Leers
#17 interdiff.txt1.39 KBWim Leers
#17 2824851-17.patch2.32 KBWim Leers
#16 interdiff.txt2.42 KBWim Leers
#16 2824851-16.patch2.69 KBWim Leers
#15 interdiff.txt2.47 KBWim Leers
#15 2824851-15.patch3.55 KBWim Leers
#12 interdiff.txt597 bytesWim Leers
#12 2824851-12.patch4.09 KBWim Leers
#9 interdiff.txt3.31 KBamateescu
#9 2824851-9.patch3.91 KBamateescu
#6 2824851.patch2.45 KBamateescu
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Wim Leers’s picture

Version: 8.3.x-dev » 8.2.x-dev

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

Wim Leers’s picture

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

Wim Leers’s picture

Issue tags: -blocker
amateescu’s picture

Status: Active » Needs review
Issue tags: +Needs tests
FileSize
2.45 KB

Here's how this should work.

Status: Needs review » Needs work

The last submitted patch, 6: 2824851.patch, failed testing.

Berdir’s picture

+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -208,20 +209,19 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
+      // Unchanged values don't need access checking.
+      if ($original_entity->get($field_name)->getValue() === $entity->get($field_name)->getValue()) {
+        continue;
+      }

There is one problem with this, though.

It allows you to figure out the current value of a field that you might not even see but you are allowed to save the entity.

You could just try values until you no longer get an access denied error :)

amateescu’s picture

Assigned: Unassigned » Wim Leers
Status: Needs work » Needs review
FileSize
3.91 KB
3.31 KB

Right, access for a field must be checked regardless whether its value changed or not.

I tried for a few hours to fix the tests without much luck: the problem now is that the bundle field is read-only so the test is trying to unset it in \Drupal\rest\Tests\UpdateTest::patchEntity(), but that makes \Drupal\serialization\Normalizer\EntityNormalizer::denormalize() complain about the missing value for the bundle field. Maybe the people who wrote that test have better chances with it :)

Status: Needs review » Needs work

The last submitted patch, 9: 2824851-9.patch, failed testing.

Wim Leers’s picture

Thanks, now taking a look at this!

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
4.09 KB
597 bytes

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.

Status: Needs review » Needs work

The last submitted patch, 12: 2824851-12.patch, failed testing.

Wim Leers’s picture

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.

Wim Leers’s picture

FileSize
3.55 KB
2.47 KB

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 :)

Wim Leers’s picture

FileSize
2.69 KB
2.42 KB
+++ 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.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
2.32 KB
1.39 KB
+++ 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.

Wim Leers’s picture

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.
amateescu’s picture

+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -213,23 +213,16 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
+      // Allow sending read-only fields, as long as their value is unchanged.
+      elseif ($field->getFieldDefinition()->isReadOnly() && $original_entity->get($field_name)->getValue() === $entity->get($field_name)->getValue()) {
+        continue;
+      }
+      elseif (!$original_entity->get($field_name)->access('edit')) {
         throw new AccessDeniedHttpException("Access denied on updating field '$field_name'.");
       }

This still doesn't sound right. Where do we prevent read-only fields from being updated if the user has edit access for them?

Berdir’s picture

Status: Needs review » Needs work

We don't, but we didn't do that before either. Not sure. What happens if we skip read-only fields all the time, even if they have a non-matching value? Or throw an access denied exception if the value doesn't match?$

We could also document that a bit better.

Also, how do we test this exactly and what is it that we need to test? We could try to change e.g. the UUID with administrative permissions, that might be allowed?

Also, the referenced issue will give us implicit test coverage as we will no longer need to remove status there.

amateescu’s picture

What happens if we skip read-only fields all the time, even if they have a non-matching value? Or throw an access denied exception if the value doesn't match?

I think both these options would make for a very confusing DX :)

Wim Leers’s picture

Status: Needs work » Needs review
Issue tags: +Entity Field API, +Entity validation

#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.

amateescu’s picture

Yes, we probably should have a generic ReadOnlyConstraint.

I was thinking the same thing earlier so I opened #2826101: Add a ReadOnly constraint validation and use it automatically for read-only entity fields.

Berdir’s picture

Constraints are for validation, not access. We could kind of do it, but it's tricky as we would have to load the original entity (we are saving, so ->original does not exist) like the changed constraint, but if we validate forward revisions, we'd have yet another problem with that and it would also fail if you call setNewRevision() and then validate becaue the revision_id was unset :)

So.. I'd say access checking is what we should do, not validating :) And we could do that directly in \Drupal\Core\Entity\EntityAccessControlHandler::fieldAccess().

Wim Leers’s picture

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.

amateescu’s picture

if we validate forward revisions, we'd have yet another problem with that and it would also fail if you call setNewRevision() and then validate becaue the revision_id was unset :)

Actually, this indicates that what we're currently doing in setNewRevision() is wrong, and instead of unsetting the revision_id we should use a $isNewRevision flag :)

Wim Leers’s picture

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

amateescu’s picture

@Wim Leers, not really.. forward revisions is an API provided by the entity system, Content Moderation just uses that API.

Berdir’s picture

Yes, it is wrong and #2809227: Store revision id in originalRevisionId property to use after revision id is updated fixes that.

I still think validation is the wrong approach. You add a loadUchanged() for every single readOnly property, that's a few of them. loadUnchanged() does not have a static cache, in fact it invalidates the persistent cache and has to load the entity again. We have some improvements for that in #2753675: loadUnchanged should use the persistent cache to load the unchanged entity if possible instead of invalidating it but #2825247: loadUnchanged should be loading also the unchanged referenced entities in order to have a complete unchanged entity structure loaded is going to make that worse again.

And loading the revision won't be better as long as we don't have a persistent/static cache for that. And adding static cache would then mean we'd have to load unchanged again :-/

I guess I should comment that over there but everyone there is also in this issue ;)

Wim Leers’s picture

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

Wim Leers’s picture

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.

Wim Leers’s picture

FileSize
3.21 KB
3.05 KB

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.

Status: Needs review » Needs work

The last submitted patch, 32: 2824851-32.patch, failed testing.

tstoeckler’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityAccessControlHandler.php
@@ -350,6 +350,12 @@ public function fieldAccess($operation, FieldDefinitionInterface $field_definiti
   protected function checkFieldAccess($operation, FieldDefinitionInterface $field_definition, AccountInterface $account, FieldItemListInterface $items = NULL) {

+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -213,23 +213,15 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
+      // @see \Drupal\Core\Entity\EntityAccessControlHandlerInterface::fieldAccess()

So there is a mismatch between the @see and the actual code here, unless I'm missing something.

However, it seems more sensible to actually put the code in fieldAccess() instead of checkFieldAccess() as access handlers might not call the parent in checkFieldAccess(). Sorry, if this was discussed above already, I didn't read every single comment.

Berdir’s picture

#34 makes sense (the second paragraph)

See also #2350429: Clarify the meaning of read-only fields and properties, which talks about two different kinds of read-only... some are only read-only after saving, and can still be set/changed on a new entity.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
3.94 KB
942 bytes

Fixed at least one test while trying to understand what needs doing.

\Drupal\Core\Entity\EntityAccessControlHandler::checkFieldAccess doesn't seem to handle computed fields right, for example node path.

Status: Needs review » Needs work

The last submitted patch, 36: 2824851-36.patch, failed testing.

The last submitted patch, 36: 2824851-36.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
4.15 KB
1.83 KB

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?

Wim Leers’s picture

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
    
Wim Leers’s picture

Wim Leers’s picture

Should we move this to the entity system component?

Status: Needs review » Needs work

The last submitted patch, 39: 2824851-39.patch, failed testing.

xjm’s picture

A real IS would be nice. :)

tedbow’s picture

Status: Needs work » Needs review
FileSize
7.75 KB
3.99 KB

Ok this probably is getting to the area of #2350429: Clarify the meaning of read-only fields and properties but I was looking at why CommentFieldAccessTest was failing.

This based off reading @fago's comment: https://www.drupal.org/node/2350429#comment-9214361

Basically introduced DataDefinitionCreatableInterface which allows you to setAlwaysCreatable(). Meaning you could provide a value during entity creation even if the field is read-only.

Status: Needs review » Needs work

The last submitted patch, 45: 2824851-45.patch, failed testing.

Wim Leers’s picture

#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.

tedbow’s picture

@Wim Leers in

+++ b/core/lib/Drupal/Core/Field/FieldItemList.php
@@ -169,6 +169,13 @@ public function access($operation = 'view', AccountInterface $account = NULL, $r
+    if ($operation === 'edit') {
+      $field_definition = $this->getFieldDefinition();
+      $access = $field_definition->isReadOnly() ? AccessResult::forbidden() : AccessResult::allowed();
+      $access->addCacheableDependency($field_definition);
+      return $access;
+    }

From #39 here we enforcing that in $operation 'edit' readonly field get access forbidden.

My reading of the doc for EntityAccessControlHandlerInterface::fieldAccess() there is only ever 'edit' and 'view' operation. So it seems 'edit' would be sent when the parent entity is new.

From fago's comment(probably best to read the whole comment)

Right now, for nodes read-only is set on node nid, uuid, vid and type. However, uuid and type fields should be settable during creating, while nid, vid and more read-only fields like changed, revision-timestamp which shouldn't be settable during creation - unless for migrate use-cases.

Looking at CommentFieldAccessTest which was failing in #39.
The test has $createOnlyFields which includes UUID. but if we look \Drupal\Core\Entity\ContentEntityBase::baseFieldDefinitionslyFields uuid for entities including comments is set to be read-only.

There is no distinction in the field access system now for read-only versus "create-only" which is that CommentFieldAccessTest is testing for.

In 8.3.x why CommentFieldAccessTest is passing is because \Drupal\Core\Entity\EntityAccessControlHandler::fieldAccess

 // Get the default access restriction that lives within this field.
    $default = $items ? $items->defaultAccess($operation, $account) : AccessResult::allowed();

    // Get the default access restriction as specified by the access control
    // handler.
    $entity_default = $this->checkFieldAccess($operation, $field_definition, $account, $items);

    // Combine default access, denying access wins.
    $default = $default->andIf($entity_default);

So now we have change the default access for read-only field to be Forbidden so it wins here.

It seems that defaultAccess() needs a concept of "read-only does not apply when parent entity is new" Hence the probably poorly named

+++ b/core/lib/Drupal/Core/TypedData/DataDefinitionCreatableInterface.php
@@ -0,0 +1,33 @@
+  /**
+   * Sets whether the data is creatable.
+   *
+   * @param bool $creatable
...
+   *
+   * @return static
+   *   The object itself for chaining.
+   */
+  public function setAlwaysCreatable($creatable);

Whoops I just noticed " Whether the data is read-only." is wrong

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

arshadcn’s picture

While working on #2824572: Write EntityResourceTestBase subclasses for every other entity type., I found some info that might be related → You can PATCH read-only fields.

Example for Node, even though \Drupal\Core\Entity\ContentEntityBase::baseFieldDefinitions defines uuid as read-only:

if ($entity_type->hasKey('uuid')) {
      $fields[$entity_type->getKey('uuid')] = BaseFieldDefinition::create('uuid')
        ->setLabel(new TranslatableMarkup('UUID'))
        ->setReadOnly(TRUE);
    }

\Drupal\node\NodeAccessControlHandler::checkFieldAccess hardcodes $read_only_fields = array('revision_timestamp', 'revision_uid'); .

As a result if you try to PATCH any read-only field other than the two above, it goes through.

I'm attaching three tests:

  1. ✔ Test 2824851-50-revision-uid.patch tries patching the revision_uid value for a node. Expecting this to fail. It fails.
  2. ✘ Test 2824851-50-uuid.patch tries patching the uuid value for a node. Expecting this to fail. It passes.
  3. ✘ Test 2824851-50-nid.patch tries patching the nid value for a node. Expecting this to fail with a 403 but it fails with a 500 because $original_entity->get($field_name)->access('edit') in \Drupal\rest\Plugin\rest\resource\EntityResource::patch returns TRUE.

Tested with \Drupal\Tests\rest\Functional\EntityResource\Node\NodeJsonBasicAuthTest.

arshadcn’s picture

Status: Needs work » Needs review

Needs review for the bots.

The last submitted patch, 50: 2824851-50-revision-uid.patch, failed testing.

The last submitted patch, 50: 2824851-50-uuid.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 50: 2824851-50-nid.patch, failed testing.

arshadcn’s picture

Status: Needs work » Needs review
FileSize
953 bytes
532 bytes

Updating patch for nid with a positive value. Let's see if we can get the 500 now.

(Let me know if this should in a new issue instead of here)

arshadcn’s picture

Status: Needs review » Needs work

The last submitted patch, 55: 2824851-55-nid.patch, failed testing.

Wim Leers’s picture

@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.

Wim Leers’s picture

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

Wim Leers’s picture

Wim Leers’s picture

Title: \Drupal\rest\Plugin\rest\resource\EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior » [PP-1] \Drupal\rest\Plugin\rest\resource\EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior
Status: Needs work » Postponed

#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.

Wim Leers’s picture

Title: [PP-1] \Drupal\rest\Plugin\rest\resource\EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior » [PP-2] \Drupal\rest\Plugin\rest\resource\EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior
Related issues: +#2862574: Add ability to track an entity object's dirty fields
Wim Leers’s picture

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 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.

Wim Leers’s picture

Title: [PP-2] \Drupal\rest\Plugin\rest\resource\EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior » EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior
Assigned: Unassigned » Wim Leers
Issue tags: +API-First Initiative

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.

Wim Leers’s picture

tstoeckler’s picture

I posted a patch that I think goes in the direction we discusse over at #2488258-67: Ignore: Patch testing issue, to see how we would do in terms of test failures. Because the way that EntityResourceTestBase::testPatch() is set up, it fails for all entity types because it now succeeds with a PATCH request where it is sending access-protected fields (because it is not actually changing the fields) but it is expecting a 403. I guess we need to expand/adapt the test method for that, to send changed field values, but I wasn't sure how to achieve that, so stopping there for now.

Wim Leers’s picture

Version: 8.3.x-dev » 8.4.x-dev
Status: Postponed » Needs review
FileSize
4.62 KB

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++

Wim Leers’s picture

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.

The last submitted patch, 67: rest_patch__field_equals-2824851-67.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Wim Leers’s picture

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/

Status: Needs review » Needs work

The last submitted patch, 68: rest_patch__field_equals-2824851-68.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
3.78 KB
12.24 KB

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.

Wim Leers’s picture

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!

Wim Leers’s picture

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!

Wim Leers’s picture

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.)

Wim Leers’s picture

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.

The last submitted patch, 72: rest_patch__field_equals-2824851-72.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 73: rest_patch__field_equals-2824851-73.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

The last submitted patch, 74: rest_patch__field_equals-2824851-74.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned

Ready for review!

tstoeckler’s picture

+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -262,35 +227,15 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
-      if (!$original_entity->get($field_name)->access('edit')) {
...
+      if (!$original_field->equals($field) && !$original_field->access('edit')) {

So this is I guess the heart of the patch.

I am by no means a Rest expert, but I think it quite nicely matches what is intended by the HTTP PATCH semantic. So in general I feel like this really makes a lot of sense, and @Wim Leers liking it is great evidence of that.

However, from a practical and security standpoint this does make me a bit uneasy. In terms of raw numbers with the patch we are checking access on a lot less fields. In the entity API triage meeting I said that I think we can accept the possibility of FieldItem(|List)Interface::equals() being incorrect in some circumstances because that would be a critical bug in its own right. 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 don't have any concrete exploits in mind, I just want to point out in general since we are removing access checks here that we need to be very sure to only remove those that we should.

Wim Leers’s picture

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.

tedbow’s picture

Status: Needs review » Needs work

@Wim Leers so I just checked I think the security problem @Berdir mentioned in #8

It allows you to figure out the current value of a field that you might not even see but you are allowed to save the entity.

You could just try values until you no longer get an access denied error :)

So I made test list field field_test with 3 possible values.

I created this hook in test module

function tester_entity_field_access($operation, FieldDefinitionInterface $field_definition, AccountInterface $account, FieldItemListInterface $items = NULL) {
  if ($field_definition->getName() == 'field_test') {
    return AccessResult::forbidden("Because reasons");
  }
  return AccessResult::neutral();
}

Then I did patch requests where I just kept changing the value for field_test(granted I knew the possible values and the field name)
I could try the patch request and would get

Access denied on updating field 'field_test'.

Until I put in the correct value for field_test and then I was able to update the node. Those I now knew the value of field_test.

tstoeckler’s picture

Re #83: I had thought about that as well, but then forgotten about it... So semantically I think it would be correct to only "allow" the equals check if view access is granted for that field. So any field that you either have to or otherwise choose to send, you have to either have to have read access to - in that case the PATCH will only go through if the value matches the original - or edit access to in which case the original value does not matter. I.e. something like:

// If the user has access to view the field, we need to check update access
// regardless of the new view field value to avoid information disclosure.
if (!$original_field->access('view')) {
  if (!$original_field->access('edit')) {
    throw new AccessDeniedHttpException();
  }
}
// If the user has access to view the field, we can check if anything actually changed
// before checking edit access.
elseif (!$original_field->equals($field) && !$original_field->access('edit')) {
    throw new AccessDeniedHttpException();
}

or something like that. That looks a bit convoluted to me right now, but it's too late for me to figure out any neater way to write that...

Edit: Fixed logic in example code (I hope).

Berdir’s picture

Yep, I just had the idea with the additional view access as well today. I think that would make sense. If you can view a field, then it's likely and valid to send it back, if you can't even view a field, then there's no reason for you to send a value to it as part of an entity that you possibly fetched and now want to return again.

Wim Leers’s picture

#84++
#85++

👏

Wim Leers’s picture

Oh and #83++ for being so vigilant!

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
2.26 KB
18.45 KB

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.

Wim Leers’s picture

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! 🎉

tstoeckler’s picture

+++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
@@ -1258,37 +1275,66 @@ protected function getEntityResourcePostUrl() {
+          // BooleanItem::generateSampleValue() picks either 0 or 1. So a 50%
+          // chance of not picking a different value.
+          $field->value = ((int) $field->value) === 1 ? '0' : '1';

This is quite interesting. I think, though, that this shows that this will still currently cause random fails from time to time even for other fields. So I think we need to explicitly check that !$field->equals($original_field) and retry the the generateSampleItems() call otherwise.

Wim Leers’s picture

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.

The last submitted patch, 88: rest_patch__field_equals-2824851-88.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

tstoeckler’s picture

But isn't this potentially problematic for any field type?

Wim Leers’s picture

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.

tedbow’s picture

#90

So I think we need to explicitly check that !$field->equals($original_field) and retry the the generateSampleItems() call otherwise.

I think this wouldn't be bad idea but it should be inside a loop with a limited amount retries to avoid endless loops. 5? I think we still want to only apply to default of the switch statement.

We should not get rid of the special case for BooleanItem::class because of the number test runs will eventually get a test run that generates 5 booleans that match the current value.

Otherwise looks good for RTBC!

Wim Leers’s picture

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.

tedbow’s picture

@Wim Leers ok that makes sense. I would say then leave it the way it is because adding the retry doesn't make sense without a loop and loop with limit will lead likely lead to infinite loop which might be very hard to figure out what the caused the test failure.

dawehner’s picture

I am totally +1 to special case certain field types. The patch as it is still has a chance of a random test failure, given we fallback to generateSampleValue for the default case.

I have a hard time though understanding why adding a unrestricted loop with random values can lead to an infinite loop. 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.

Wim Leers’s picture

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.
tedbow’s picture

Status: Needs review » Reviewed & tested by the community

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.

Well given Butterfly Effect there is a non-zero chance that an infinite loop in our tests could cause the world to end. So just want to but that out there so I can I told you so, you know just incase.

Looks good! RTBC! 🎉

dawehner’s picture

I'm curious, isn't the butterfly effect possible due to the non linearity of complex systems? I would guess that generating random values here is at least sort of linear ;)

Wim Leers’s picture

❤️ this community and its wonderfully geeky discussions :)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 99: rest_patch__field_equals-2824851-99.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

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.

Wim Leers’s picture

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

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

catch’s picture

Version: 8.5.x-dev » 8.4.x-dev
Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs issue summary update +8.4.0 release notes

This looks like a great simplification, also thanks for the detailed issue summary makes it clear what the progression of the patch was.

Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!

Tagging for release notes.

  • catch committed fb7dc06 on 8.4.x
    Issue #2824851 by Wim Leers, arshadcn, amateescu, tedbow, timmillwood,...
Wim Leers’s picture

OMG YAY!

🎉

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

cburschka’s picture

Status: Fixed » Needs work

The PathItem::class case in getModifiedEntityForPatchTesting isn't right:

        case PathItem::class:
          // PathItem::generateSampleValue() doesn't set a PID, which causes
          // PathItem::postSave() to fail. Keep the PID (and other properties),
          // just modify the alias.
          $value = $field->getValue();
          $value['alias'] = str_replace(' ', '-', strtolower((new Random())->sentences(3)));
          $field->setValue($value);
          break;

::getValue() returns the list of items, not the first item. We'd have to set $value[0]['alias'] here to work correctly. Current data sent to ::setValue()

array(2) {
  [0]=>
  array(4) {
    ["pid"]=>
    string(1) "1"
    ["source"]=>
    string(7) "/node/1"
    ["alias"]=>
    string(6) "/llama"
    ["langcode"]=>
    string(2) "en"
  }
  ["alias"]=>
  string(40) "cogo-luctus-mauris-nostrud-tation-valde."
}

(The PATCH still gets rejected as designed. That's why it didn't cause a test failure.)

Can we do this as a follow-up, or does it need a separate issue?

Berdir’s picture

I don't think that is wrong, FieldItemList::setValue() accepts anything from a scalar value for the main property, a single field item and a list of field items values. The entity reference field above is also set as setValue(['target_id' =>..

cburschka’s picture

Status: Needs work » Needs review
FileSize
1 KB

That's true, but here we're giving it neither a valid list nor a single item - we give it a list of field items with an extra array key stuffed into the top level.

In particular, ::setValue differentiates between "list of field items" and "single field item" by checking if the first array key is numeric, which is true here.

Status: Needs review » Needs work

The last submitted patch, 112: 2824851-112-followup.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Fixed

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

  • xjm committed f3af3bf on 8.4.x
    Revert "Issue #2824851 by Wim Leers, arshadcn, amateescu, tedbow,...

  • xjm committed 84bbc57 on 8.5.x
    Revert "Issue #2824851 by Wim Leers, arshadcn, amateescu, tedbow,...
xjm’s picture

Status: Fixed » Needs review
Issue tags: -8.4.0 release notes +Needs tests

I reverted this issue because it introduced an access bypass vulnerability as documented in the private security issue https://security.drupal.org/node/162089. Since the branch has not yet had a release candidate, it is better to resolve this issue in public before committing the patch. Adding some relevant discussion from that issue, all from @cburschka (to whom I'm granting credit here now):

Steps to reproduce

  1. Install a new 8.4.x site (standard profile).
  2. Enable path, node, hal, basic_auth.
  3. Create a user with create/edit node permissions, but no path alias permissions.
  4. Create a node the user can edit.
  5. As that user, submit a GET request to the node to acquire its full JSON.
  6. As that user, submit a PATCH request with the same JSON, but changing the path alias to any valid value.

Expected result: 403
Actual result: 200, path alias gets changed successfully.

(Due to little validation at this level, it is also possible to overwrite any other alias knowing its pid value, as well as creating path aliases that cover system routes.)

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

Specifically:

    $columns = $this->getFieldDefinition()->getFieldStorageDefinition()->getColumns();

    //...

    $value1 = $this->getValue();
    $value2 = $list_to_compare->getValue();

    //...

    // If the values are not equal ensure a consistent order of field item
    // properties and remove properties which will not be saved.
    $callback = function (&$value) use ($columns) {
      if (is_array($value)) {
        $value = array_intersect_key($value, $columns);
        ksort($value);
      }
    };
    array_walk($value1, $callback);
    array_walk($value2, $callback);

    return $value1 == $value2;

$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).

This behavior is meant to be tested by EntityResourceTestBase::testPatch(). However, a bug in EntityResourceTestBase::getModifiedEntityForPatchTesting() results in the path field being modified incorrectly (adding a second item instead of changing an existing one). As a result, the request is denied, and the test passes.

Absolutely, we can use setValue() here and simply change the problematic line to $value[0]['alias'] =. The magic method access only saves two lines. (The problem with the current code is that getValue() returns a list of items, and

$value['alias']
 = 

treats it as a single item.)
(Adding a slash to the value has the DX benefit of actually passing a valid alias. Without it, we get "HTTP 500 instead of 403", which looks like an unrelated crash; with a valid URL the test fails with "HTTP 200 instead of 403", which makes it clear that we accepted a request that should have been rejected.)

---

As for the fix, we can harden against other such errors by only setting the field if it is actually not equal. Right now we do:

if (not equal and no edit access) {
  HTTP 403
}

Set value on $original

Where we could instead do:

if (not equal) {
  if (no edit access) {
    HTTP 403
  }
  set field
}

This way, any bugs in ::equals() will result in a safe failure by ignoring the value completely, rather than bypassing access.

xjm’s picture

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

@catch and I also discussed that this could still go into 8.4.x as a major bugfix if we deprecate this protected method instead of removing it for the 8.4.x version of the patch, and remove it for 8.5.0 instead.

Wim Leers’s picture

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.

Wim Leers’s picture

Issue tags: +blocker

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.

Wim Leers’s picture

Wim Leers’s picture

Assigned: Unassigned » Wim Leers

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

Wim Leers’s picture

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.)

Wim Leers’s picture

#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!

Wim Leers’s picture

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.

🤔😱😭😩

Wim Leers’s picture

Related issues:

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).

The last submitted patch, 123: 2824851-123.patch, failed testing. View results

The last submitted patch, 124: 2824851-124-HEAD_paths_200_FAIL.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 124: 2824851-124.patch, failed testing. View results

Wim Leers’s picture

I forgot to upload #126's attachments. 😅

The last submitted patch, 130: 2824851-126-HEAD_paths_200_PASS.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 130: 2824851-126.patch, failed testing. View results

dawehner’s picture

+++ b/core/modules/path/src/Plugin/Field/FieldType/PathItem.php
@@ -162,7 +173,9 @@ public static function mainPropertyName() {
+    static $is_loading = FALSE;

❓ Couldn't you still have a class based variable? Otherwise for whatever reason two instances could maybe interfere ...

Wim Leers’s picture

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.

Status: Needs review » Needs work

The last submitted patch, 134: 2824851-134.patch, failed testing. View results

Wim Leers’s picture

Title: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior » [PP-1] EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior
Related issues: +#2909183: Add path alias (PathItem) field PATCH test coverage

#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).

Wim Leers’s picture

Title: [PP-1] EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior » EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior
Status: Needs work » Needs review
FileSize
22.44 KB

#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.

Wim Leers’s picture

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.

The last submitted patch, 137: 2824851-137.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 138: 2824851-138.patch, failed testing. View results

Wim Leers’s picture

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".

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
2.69 KB
23.44 KB

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

Status: Needs review » Needs work

The last submitted patch, 142: 2824851-142.patch, failed testing. View results

tstoeckler’s picture

+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -230,24 +230,25 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
+      if (!$original_field->equals($field)) {
...
+          // If the user has access to view the field, we need to check update
+          // access regardless of the field value to avoid information disclosure.

Unless I'm missing something, the comment is no longer true and the interdiff re-introduces the information disclosure that the comment is talking about.

I'm not sure I understand the ""make things worse" bug" your are describing. Is the problem that the ->set() call is reached when it shouldn't be?

amateescu’s picture

I've opened #2916967: FieldItemList::equals() is broken for field types without a schema to fix the first problem reported (with such large and bold letters) in #138.

Side-note for all the other followers of this issue that might be confused by the wording: what #138 describes as "unlike every other field" and "highly atypical" basically means "a computed field type without a schema" :)

Also, I would advise holding off on any more "fixes" to the path field type (like #2909183: Add path alias (PathItem) field PATCH test coverage which was committed yesterday) until the following issues are fixed:

#2392845: Add a trait to standardize handling of computed item lists
#2916300: Use ComputedFieldItemListTrait for the path field type
#2916967: FieldItemList::equals() is broken for field types without a schema

Trust me, that will save you a lot of time and hair pulling over the current implementation of the path field :)

Wim Leers’s picture

Title: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior » [PP-1] EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior
Related issues: +#2916967: FieldItemList::equals() is broken for field types without a schema

Well … clearly #142 didn't work the way I expected it to :P


#144:

I'm not sure I understand the ""make things worse" bug" your are describing. Is the problem that the ->set() call is reached when it shouldn't be?

Yes.


#145: I'd love to not have to fix Field API bugs in this patch! #2916967: FieldItemList::equals() is broken for field types without a schema is indeed the key thing we need to fix here, and the root cause for this patch being committed in #107 and then reverted in #114. Postponing this issue on that one at the very least. I'm not certain that this needs to be blocked on those other issues too?

Trust me, that will save you a lot of time and hair pulling over the current implementation of the path field :)

Indeed :D

tstoeckler’s picture

Re #146:

So going back to the patch in #138 the ->set() called is reach iff the user does not have access to view the field but has access to edit it xor the user has access to view the field, the field is changed and the user has access to edit the field.

Both of those seem reasonable to me. We could wrap the call in another ->equals() check, but it seems that would be more of a micro-optimization. Setting an equal value on an item should be a no-op and not result in any functional change, no?

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
2.6 KB
1.31 KB
23.51 KB

Setting an equal value on an item should be a no-op and not result in any functional change, no?

Correct, it should be. But as @cburschka pointed out in #117 (which was quoted from https://security.drupal.org/node/162089), any time ::equals() incorrectly considers two values equals when they aren't, it allows a malicious user to change the value of a field even when the user isn't allowed to edit the field!

In other words: this is just about being very prudent, and by changing this code slightly, we will not have security vulnerabilities in the future (in core, contrib or custom code) when ::equals() behaves incorrectly.


Also, obviously you're right in #144 where you say that #142 reintroduces the information disclosure. That's what the failed tests are saying, too! :)

Fix attached. The regular interdiff is relative to #142. The 138-148 interdiff is relative to #138, i.e. pretending the flawed interdiff of #142 never happened.

tstoeckler’s picture

any time ::equals() incorrectly considers two values equals when they aren't, it allows a malicious user to change the value of a field even when the user isn't allowed to edit the field!

I don't think that's true. Unless my assessment in #147 is incorrect it ->set() is only ever called if edit access is given. Furthermore I think there is a logical flaw in that argument. if ::equals() considers values equal that aren't with #148 what would happen is that values do not get updated even though they should.

The patch in #148 will now not update the value if the user does not have view access (but does have edit access). So if you insist on only calling ->set() if the values not equal we need another ::equals() check below the whole exception throwing.

Status: Needs review » Needs work

The last submitted patch, 148: 2824851-148.patch, failed testing. View results

cburschka’s picture

Unless my assessment in #147 is incorrect it ->set() is only ever called if edit access is given.

Sorry, does this refer to patch 138? Because that code reaches the ->set() iff (the user can't view AND can edit) OR (the user has view access AND edit access) OR (the user has view access AND ::equals returns true). The third branch is the problematic one that turns bugs like #2916967: FieldItemList::equals() is broken for field types without a schema into security issues like Wim mentioned).

View | Edit | ::equals | Result (#138)
-----+------+----------+------------
   0 |    0 |        0 | Error
   0 |    0 |        1 | Error
   0 |    1 |        0 | Set
   0 |    1 |        1 | Set
   1 |    0 |        0 | Error
   1 |    0 |        1 | Set !!
   1 |    1 |        0 | Set
   1 |    1 |        1 | Set

Patch 142 fixes this problem, but turns "you tried to set a field you can neither view nor edit" into a silent error that simply skips the field. This is still secure - there's no information leakage since the field is skipped whether or not the value is equal - but it feels like poor DX to not throw an error.

View | Edit | ::equals | Result (#142)
-----+------+----------+------------
   0 |    0 |        0 | Ignore !!
   0 |    0 |        1 | Ignore !!
   0 |    1 |        0 | Set
   0 |    1 |        1 | Ignore
   1 |    0 |        0 | Error
   1 |    0 |        1 | Ignore
   1 |    1 |        0 | Set
   1 |    1 |        1 | Ignore

Patch 148 does indeed have the problem pointed out in 149: With blind edit access, the value is not set.

View | Edit | ::equals | Result (#148)
-----+------+----------+------------
   0 |    0 |        0 | Error
   0 |    0 |        1 | Error
   0 |    1 |        0 | Ignore !!
   0 |    1 |        1 | Ignore
   1 |    0 |        0 | Error
   1 |    0 |        1 | Ignore
   1 |    1 |        0 | Set
   1 |    1 |        1 | Ignore

The table we want is really:

View | Edit | ::equals | Result
-----+------+----------+------------
   0 |    0 |        0 | Error
   0 |    0 |        1 | Error
   0 |    1 |        0 | Set
   0 |    1 |        1 | Ignore or Set
   1 |    0 |        0 | Error
   1 |    0 |        1 | Ignore
   1 |    1 |        0 | Set
   1 |    1 |        1 | Ignore or Set

(In cases 011 or 111 it shouldn't matter whether the value is ignored or set.)

I'm sure there is some way to turn that table into a reasonably compact and readable flow. If necessary, we can always store the ::equals() result in a local variable to reuse it.

cburschka’s picture

For example (numbering cases 000-111 according to the truth table from #151):

if (edit) {
  set value // covers case 010, 011, 110, 111.
}
elseif (!view || !equals) {
  Error // covers cases 000, 001, 100.
}
// Do nothing. Covers case 101

If we want, we can put an extra if-not-equals in that first branch to skip setting in cases 011 and 111. It's not required for security (as we do have edit access here), but it might be desirable to make ::equals() bugs "fail more loudly" by preventing changes to the field entirely.

Edit: Slight cleanup by swapping branch order.

tstoeckler’s picture

the user has view access AND ::equals returns true

You're right, I missed that. Thanks for correcting me and for being able to still see clearly through this issue, a capability I have lost a while back.

The way to fix the quoted issue would be to wrap the ->set() call in another equals() or access('edit') check. I could see semantic arguments for both (either "setting an equal value is pointless" or "setting requires edit access").

In theory I don't mind adding such a check. I would like to understand the actual test failures first, though. Because in my apparently still existing naiveté about the entity system I still don't grok what actually breaks when you set an equal value.

These are the test failures of #138, correct? If I understand that correctly that failure is only caused by #2906600: FieldItemList::equals() doesn't work correctly for computed fields with custom storage and the test will pass as soon as that is committed.

So if we were to add such a check as described above would the only reasoning for that be that we don't really trust equals()? As I have mentioned before, we already trust equals() to be correct for many things that are related to storage, data integrity, etc, so I don't think it's very reasonable to not extend that trust here. Maybe I am missing something, to be honest, this issue has me a bit confused...

EDIT: Crosspost with #152

tstoeckler’s picture

Yay, #152 once more proves you see things much more clearly than me. The truth table makes perfect sense and your proposed solution is both adequate and elegant. That definitely is the way to go, @cburschka++++

One note:

If we want, we can put an extra if-not-equals in that first branch to skip setting in cases 011 and 111. It's not required for security (as we do have edit access here), but it might be desirable to make ::equals() bugs "fail more loudly" by preventing changes to the field entirely.

In fact we must not add that check in the first branch as that would re-introduce the information disclosure vulnerability. In other words, we must never check equals without having checked view access first. (Unless, once again, I am missing something ;-))

cburschka’s picture

Thanks :D

You're right that we can't turn the if (edit) into an if (edit && !equals), because then we get an error on an equal&editable&unviewable field while setting an unequal&editable&unviewable field, which discloses information.

The condition would have be added inside the block. (Though it seems likely to be cleaner without, even so.)

tstoeckler’s picture

Ahh I see what you mean. Per #153 I wouldn't be too excited about adding more checks than we theoretically need and your proposal in #152 reads very well, so I'd say let's not add anything to that.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
1.66 KB
24.16 KB

#149—#156/@cburschka/@tstoeckler: I'll get back to your comments in a moment — but I need to tackle something else first.


#148 still does not have the expected failures: it has 16, there should be 12. UserResourceTestBase::testPatchDxForSecuritySensitiveBaseFields() REST tests are failing (and that test doesn't run for anon, only for basic auth and cookie, hence there are 4 failures caused by it, not 6: those two authentication providers times two formats).

This is because \Drupal\user\Plugin\Validation\Constraint\ProtectedUserFieldConstraintValidator::validate() calls \Drupal\user\Entity\User::checkExistingPassword() which checks $user->get('pass')->existing … which … no longer gets set since #148 because it now only sets a value if it is different from the current value. Before, it was always being set.

In fact, it even needs to be set despite view access for the User pass field not being allowed!

For now, the only solution I can think of is to special case the User entity type's pass field. Because, well, it is a very special case.

This should bring the number of failures down to 12.

Wim Leers’s picture

#157 should now have exactly the expected failures: 12. TermResourceTestBase::testPatchPath() and NodeResourceTestBase::testPatchPath() are both failing in the 6 variations they are tested in (2 formats:
json, hal_json; 3 authentication providers: anon, cookie, Basic Auth). 12 = 2*2*3.

The solution lies in fixing FieldItemList::equals() as explained in #138's "root cause" section. That bug merits its own issue since it's a bug in Field API, not in the REST module.

@amateescu created #2916967: FieldItemList::equals() is broken for field types without a schema for that, but that's actually a duplicate of #2906600: FieldItemList::equals() doesn't work correctly for computed fields with custom storage, which both @amateescu and I forgot about, but @tstoeckler thankfully pointed out :)

Adding #2906600: FieldItemList::equals() doesn't work correctly for computed fields with custom storage should make all tests pass. This imports #2906600-14: FieldItemList::equals() doesn't work correctly for computed fields with custom storage.

This should be green!

Wim Leers’s picture

And finally, clean-up!

  1. +++ b/core/modules/hal/tests/src/Functional/EntityResource/Node/NodeHalJsonAnonTest.php
    index a98e718..430adec 100644
    --- a/core/modules/path/src/Plugin/Field/FieldType/PathFieldItemList.php
    

    The changes here are an irrelevant remnant of an earlier patch iteration now.

  2. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    index 492ff64..8474aaf 100644
    --- a/core/modules/rest/tests/src/Functional/EntityResource/Node/NodeResourceTestBase.php
    
    +++ b/core/modules/rest/tests/src/Functional/EntityResource/Node/NodeResourceTestBase.php
    index e0a1581..4516385 100644
    --- a/core/modules/rest/tests/src/Functional/EntityResource/Term/TermResourceTestBase.php
    

    We can now resolve the @todos we have in this patch.

amateescu’s picture

Re #158: if you want to have a green patch you should use the patch from #2906600-17: FieldItemList::equals() doesn't work correctly for computed fields with custom storage, not #14 :)

Wim Leers’s picture

And now back to the super important discussion in #149#156 between @cburschka and @tstoeckler!

@cburschka's super helpful truth tables in #151 indeed explain this tricky problem well. And the solution in #152 is indeed very elegant. It seems to introduce a new failure though, but I do'nt have time at the moment to get to the bottom of that — I will tomorrow. OTOH, a bigger concern is that this no longer introduces the protection against a broken ::equals() implementation, which @cburschka proposed in #117. Do we no longer feel that extra layer of prudence (as explained in #148) is no longer necessary?

Wim Leers’s picture

#160: Yeah I noticed that, but I had this ready locally. Your patch at 17 there fixes the MapItem case, which none of the existing test coverage is exercising AFAIK, which means this patch should still pass.

The last submitted patch, 157: 2824851-157.patch, failed testing. View results

The last submitted patch, 158: 2824851-158.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 159: 2824851-159.patch, failed testing. View results

amateescu’s picture

+++ b/core/modules/path/src/Plugin/Field/FieldType/PathFieldItemList.php
@@ -73,12 +72,4 @@ protected function ensureLoaded() {
-  public function equals(FieldItemListInterface $list_to_compare) {
-    $this->ensureLoaded();
-    return parent::equals($list_to_compare);
-  }

This is the cause of the failures for the patch in #159.

And it's exactly why I said earlier that we should wait for #2392845: Add a trait to standardize handling of computed item lists and #2916300: Use ComputedFieldItemListTrait for the path field type before wasting more time here and playing whack-a-mole with the path field :)

amateescu’s picture

cburschka’s picture

And the solution in #152 is indeed very elegant. It seems to introduce a new failure though, but I do'nt have time at the moment to get to the bottom of that — I will tomorrow. OTOH, a bigger concern is that this no longer introduces the protection against a broken ::equals() implementation, which @cburschka proposed in #117.

Sorry if I'm misunderstanding this, but doesn't the flow in 152 still protect against ::equals bugs? The set depends completely on edit access; the equals check only determines whether to throw an error or silently ignore the value.

Wim Leers’s picture

Title: [PP-1] EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior » [PP-3] EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior
Related issues: +#2916300: Use ComputedFieldItemListTrait for the path field type
FileSize
1.98 KB
50.92 KB

@amateescu:

Hah, happy to be wrong in #162! I wish I'd updated #158 before to use #2906600-17: FieldItemList::equals() doesn't work correctly for computed fields with custom storage instead of #2906600-14: FieldItemList::equals() doesn't work correctly for computed fields with custom storage.

But, what I didn't even realize (because running all tests locally takes too long), is that removing that PathFieldItemList change in #159 caused additional failures, for which we indeed need #2916300-12: Use ComputedFieldItemListTrait for the path field type. And #2916300 is itself blocked on #2392845: Add a trait to standardize handling of computed item lists.

So, that means this is now blocked on three issues:

  1. #2906600: FieldItemList::equals() doesn't work correctly for computed fields with custom storage
  2. #2916300: Use ComputedFieldItemListTrait for the path field type
    1. #2392845: Add a trait to standardize handling of computed item lists

@cburschka: In my attempt to implement #152 yesterday, I got stuck on an error that convinced me to write #161. But now I'm not so sure anymore. I had to leave urgently, so I think that in my haste I misunderstood something :) Let's implement your suggestion in #152 and see what happens!

Wim Leers’s picture

Status: Needs review » Postponed
Wim Leers’s picture

Title: [PP-3] EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior » [PP-2] EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior
Wim Leers’s picture

Title: [PP-2] EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior » [PP-1] EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior
cburschka’s picture

Title: [PP-1] EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior » EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior
Status: Postponed » Needs review

#2916300: Use ComputedFieldItemListTrait for the path field type is in, so I guess this is unblocked!

(Back to NR, but the patch probably needs a reroll by now.)

Wim Leers’s picture

Version: 8.4.x-dev » 8.5.x-dev

Correct! Currently attempting a reroll… for now only against 8.5 (even though both blockers went into 8.4), because EntityResourceTestBase has diverged significantly between 8.4 and 8.5.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
FileSize
23.12 KB
  • #169 (combined, includes blockers): 19 files changed, 546 insertions, 352 deletions.
  • This rebased patch: 9 files changed, 99 insertions, 167 deletions
tedbow’s picture

+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -263,38 +227,26 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
-        if ($entity->getEntityTypeId() == 'comment' && $field_name == 'status' && !$original_entity->get($field_name)->access('edit')) {
-          throw new AccessDeniedHttpException("Access denied on updating field '$field_name'.");
-        }
...
+      // The User entity type's 'pass' field is a very special case: even though
+      // it is not allowed to be viewed nor edited, to be able to modify
+      // security-sensitive fields, the password must be specified.
+      // @see \Drupal\Tests\rest\Functional\EntityResource\User\UserResourceTestBase::testPatchDxForSecuritySensitiveBaseFields()
+      if ($original_entity instanceof UserInterface && $field_name === 'pass') {
+        $original_entity->set('pass', $field->getValue());
+        continue;
       }

We swapping out the need for 1 special case for comment status for another special case for user password field.

Would it make sense to refactor the logic inside this loop to something like:

protected function setOriginalFieldValue($original_entity, $entity_field, $field_name);

Then we could have UserResource class that could handle the 'pass' field case.

Wim Leers’s picture

Green! Now blocked on review :)


I reviewed #175 myself, it looks ready to me. Interestingly, it's pretty much identical to the patch that was committed in #106, in August!

The reason it can be essentially the same patch, is that we created issues to fix the underlying problems, and those issues have been fixed:

  1. #2909183: Add path alias (PathItem) field PATCH test coverage
  2. #2392845: Add a trait to standardize handling of computed item lists
  3. #2906600: FieldItemList::equals() doesn't work correctly for computed fields with custom storage
  4. #2916300: Use ComputedFieldItemListTrait for the path field type

If you diff rest_patch__field_equals-2824851-106.patch 2824851-175.patch , you'll see it's identical, except for:

  1. --- a/core/modules/path/src/Plugin/Field/FieldType/PathFieldItemList.php
    +++ b/core/modules/path/src/Plugin/Field/FieldType/PathFieldItemList.php
    

    [present in #106, absent in #175] The changes here are no longer necessary, they landed in #2916300: Use ComputedFieldItemListTrait for the path field type and its blockers.

  2. --- a/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
    +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
    

    Mostly same changes here, but the logic has changed a bit based on @cburschka's excellent analysis in #151 and proposed implementation in #152, vetted by @tstoeckler.

  3. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Node/NodeResourceTestBase.php
    @@ -235,20 +235,6 @@ public function testPatchPath() {
    -    // @todo In https://www.drupal.org/node/2824851, we will be able to stop
    -    //       unsetting these fields from the normalization, because
    -    //       EntityResource::patch() will ignore any fields that are sent that
    -    //       match the current value (and obviously we're sending the current
    -    //       value).
    -    $normalization = $this->removeFieldsFromNormalization($normalization, [
    -      'revision_timestamp',
    -      'revision_uid',
    -      'created',
    -      'changed',
    -      'promote',
    -      'sticky',
    -    ]);
    
    +++ b/core/modules/rest/tests/src/Functional/EntityResource/Term/TermResourceTestBase.php
    @@ -205,15 +205,6 @@ public function testPatchPath() {
    -    // @todo In https://www.drupal.org/node/2824851, we will be able to stop
    -    //       unsetting these fields from the normalization, because
    -    //       EntityResource::patch() will ignore any fields that are sent that
    -    //       match the current value (and obviously we're sending the current
    -    //       value).
    -    $normalization = $this->removeFieldsFromNormalization($normalization, [
    -      'changed',
    -    ]);
    

    [absent in #106, present in #175] Removing these work-arounds, which were added by one of the blockers (#2909183: Add path alias (PathItem) field PATCH test coverage), explicitly to pass in HEAD and to be removed here.

  4. +++ b/core/modules/rest/tests/src/Functional/EntityResource/Node/NodeResourceTestBase.php
    @@ -258,8 +244,11 @@ public function testPatchPath() {
    -    // PATCH request: 403 when creating URL aliases unauthorized.
    +    // PATCH request: 403 when creating URL aliases unauthorized. Before
    +    // asserting the 403 response, assert that the stored path alias remains
    +    // unchanged.
         $response = $this->request('PATCH', $url, $request_options);
    +    $this->assertSame('/llama', $this->entityStorage->loadUnchanged($this->entity->id())->get('path')->alias);
    

    [absent in #106, present in #175] Tightening this test coverage that was added in #2909183: Add path alias (PathItem) field PATCH test coverage.

I'll leave it to @cburschka, @tstoeckler, @amateescu or somebody else to RTBC.

Wim Leers’s picture

#176: Great review, thank you!

We swapping out the need for 1 special case for comment status for another special case for user password field.

That's not quite true:
The Comment "special case" that we're removing just happens to be the only case we're hitting with core's test suite. It's possible there are dozens of occurrences of this.
The User "special case" is an actual special case: it's fair to assume it's the only case where it's necessary to send a password (a secret) that is not being modified, in order to be allowed to modify another field.

Therefore I think special casing $user->pass is justified, and adding a new (internal?) API for that seems premature. We should only do this once we know of at least 2 use cases IMHO. And even if we find a second example, I think it's out of scope — because it'd merit its own discussion.

tedbow’s picture

#178 thats sounds fine to wait for another special case to refactor out as I suggested in #176

tstoeckler’s picture

Status: Needs review » Needs work

Took a look at the entire patch again. I had the same thought as @tedbow in #176 that I think a dedicated UserResource would be neat, but I don't feel strongly about it, so I'm fine with #178 / the status quo.

Other than that just two remarks:

  1. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
    @@ -263,38 +227,26 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
    +      elseif (!$original_field->access('view') || !$original_field->equals($field)) {
    

    In adapting the flow here, we lost the comment that was there in previous patches. I think because of the not necessarily obvious
    security implications it would be important to add one here, though, that makes it clear that we *must* check view access before
    calling equals().

  2. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -1028,22 +1034,31 @@ public function testPatch() {
    +    // DX: 403 when entity contains field without 'edit' nor 'view' access, even
    +    // when the value for that field matches the current value. This is allowed
    +    // in principle, but leads to information disclosure.
    ...
    +    // 200 for well-formed PATCH request that sends all fields (even including
    +    // read-only ones, but with unchanged values).
    

    So at first I thought that we are now missing a test for the "view access + field value unchanged => 200" case, as the former hunk
    only tests the "no view access + field value uncahnged => 403", but then I saw the latter hunk, so I though I guess we are testing
    that. It seems, however, that since the only difference between those test cases is the access-level of the user, there should be
    some auth change in between the hunks, right? I.e. either a drupalLogin() or a granting of permissions or the like?
    I'm sure I'm just missing something, but can't figure it out right now...

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
1.09 KB
23.56 KB
  1. 👍 Brought back that comment.
  2. Excellent question!

    we are now missing a test for the "view access + field value unchanged => 200" case […] but then I saw the latter hunk, so I though I guess we are testing that

    Correct.

    It seems, however, that since the only difference between those test cases is the access-level of the user, there should be
    some auth change in between the hunks, right?

    No, we're changing the request body that gets sent (look for $request_options[RequestOptions::BODY]):

        $request_options[RequestOptions::BODY] = $parseable_invalid_request_body_3;
    
        // DX: 403 when entity contains field without 'edit' nor 'view' access, even
    …
        $response = $this->request('PATCH', $url, $request_options);
        $this->assertResourceErrorResponse(403, "Access denied on updating field 'field_rest_test'.", $response);
    
        // DX: 403 when sending PATCH request with updated read-only fields.
    …
        for ($i = 0; $i < count(static::$patchProtectedFieldNames); $i++) {
    …
          $request_options[RequestOptions::BODY] = $this->serializer->serialize($modified_entity, static::$format);
          $response = $this->request('PATCH', $url, $request_options);
          $this->assertResourceErrorResponse(403, "Access denied on updating field '" . $patch_protected_field_name . "'.", $response);
          $modified_entity->get($patch_protected_field_name)->setValue($original_values[$patch_protected_field_name]);
        }
    
        // 200 for well-formed PATCH request that sends all fields (even including
    …
        $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->assertResourceResponse(200, FALSE, $response);
    

    The first quoted hunk is sending a modified field_rest_test field, which is forbidden to be edited, always (see rest_test_entity_field_access()). The second quoted hunk is starting out with all read-only fields having been modified, and after verifying that there's a 403 for the next read-only field, it sets the value for that read-only field back to the original value, then tries again with an updated request body, to verify the next read-only field, etc. Until eventually, we send a request body with only the fields modified that $this->getNormalizedPatchEntity() returns, and with all other fields sent in the request body unmodified (+ $this->serializer->normalize($this->entity, static::$format)).

    I'm sure I'm just missing something, but can't figure it out right now...

    So AFAICT you were just missing something, I don't think there's a problem in the test. The test definitely isn't very easy to parse, but then again we are trying lots of variations. It's hard when automatically testing lots of variations, to make it easy to understand. I hope it makes sense now!

tstoeckler’s picture

Thanks for the quick update and for the elaborate response. #181 makes total sense and is fully correct. However, I do think there is one case we are currently not testing, it's just a different one than I thought in #180: "unchanged field value + no view access => 403".

Some further notes on the patch:

  1. +++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
    @@ -263,38 +227,32 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
    +      // If the user has access to view the field, we need to check update
    +      // access regardless of the field value to avoid information disclosure.
    

    This comment no longer matches the code-flow. Maybe something like: "If the user does not have access to edit the field, still allow the field to be sent if it matches the original value. Checking the equality of the field values, however, must only be done if the user has access to view the field to avoid unwanted information disclosure."

  2. +++ b/core/modules/rest/tests/src/Functional/EntityResource/EntityResourceTestBase.php
    @@ -937,6 +942,7 @@ public function testPatch() {
    +    $parseable_invalid_request_body_3 = $this->serializer->encode($this->getNormalizedPatchEntity() + ['field_rest_test' => [['value' => 'All the faith he had had had had no effect on the outcome of his life.', 'format' => NULL]]], static::$format);
    
    @@ -1028,22 +1034,31 @@ public function testPatch() {
    +    $request_options[RequestOptions::BODY] = $parseable_invalid_request_body_3;
    ...
    +    // DX: 403 when entity contains field without 'edit' nor 'view' access, even
    +    // when the value for that field matches the current value. This is allowed
    +    // in principle, but leads to information disclosure.
    ...
    +    $this->assertResourceErrorResponse(403, "Access denied on updating field 'field_rest_test'.", $response);
    

    As far as I can tell, the comment does not match the code, as the value does not match the current value.

Wim Leers’s picture

FileSize
3.94 KB
24.19 KB

Thanks for the quick update and for the elaborate response.

And thank you for the fast & thorough reviews! Makes me hopeful we can finally get this in :)


  1. 👍 Fixed. Made me rethink all this, and man, it's tricky. Made me question again whether this was correct, ended up concluding it was.
    Another half hour down the drain!
  2. For a moment, I thought you were right. But turns out the code is right after all :) $parseable_invalid_request_body_3 is generated with a value for field_rest_test … which matches the stored value. But because it hardcodes the value, you'd think it's not doing that.
    We need to explicitly generate a request body that contains this field, specifically because fields without view access don't end up in the normalization, so we need to add it explicitly. (And field_rest_test disallows view access thanks to rest_test_entity_field_access.)
    I've updated the code in this reroll to make that more explicit, and added a comment.

However, I do think there is one case we are currently not testing, it's just a different one than I thought in #180: "unchanged field value + no view access => 403".

We are testing that:

    // DX: 403 when entity contains field without 'edit' nor 'view' access, even
    // when the value for that field matches the current value. This is allowed
    // in principle, but leads to information disclosure.
    $response = $this->request('PATCH', $url, $request_options);
    $this->assertResourceErrorResponse(403, "Access denied on updating field 'field_rest_test'.", $response);

Which is exactly that case. Also see what I wrote in response to point 2 above.

Let's make sure we're testing all cases. Let's bring back @cburschka's truth table from the bottom of #151:

View | Edit | ::equals | Result
-----+------+----------+------------
   0 |    0 |        0 | Error
   0 |    0 |        1 | Error
   0 |    1 |        0 | Set
   0 |    1 |        1 | Ignore or Set
   1 |    0 |        0 | Error
   1 |    0 |        1 | Ignore
   1 |    1 |        0 | Set
   1 |    1 |        1 | Ignore or Set

We're testing the first two (000 + 001) using field_rest_test and $parseable_invalid_request_body_2 + $parseable_invalid_request_body_3, respectively.

So it seems we're missing the 100 case. But we're not! That's what the // DX: 403 when sending PATCH request with updated read-only fields. hunk is testing.

Status: Needs review » Needs work

The last submitted patch, 183: 2824851-183.patch, failed testing. View results

vaplas’s picture

Status: Needs work » Needs review

Great work! Perhaps RTBC, but leave it for more competent developers. Now just back to NR after random failure with memory.

tstoeckler’s picture

Status: Needs review » Reviewed & tested by the community

Re #183: Thanks for the explanation. You are absolutely right including...

But because it hardcodes the value, you'd think it's not doing that.

you know me too well, that's almost scary ;-)

Also

man, it's tricky

Tell me about it! See all the comments above where I thought I was explaining to @cburschka but it turned out to be the other way around. Having gone through all these iterations, however, I do now feel pretty confident that we have arrived at a correct, secure and thoroughly tested solution in the form of this patch.

Since I haven't written any code for this since the original patch in #66 / #67 of which not much survived, I feel comfortable RTBCing this. Especially since this has gone through all the iterations up to the initial commit and then through the revert and being postponed on the various other bugfixes, I have solely provided reviews here.

It certainly remains both a complex and a sensitive issue so I by all means encourage anyone else to throw another pair of eyes at this. But since the Rest test coverage is now in a pretty awesome place and @Wim Leers verified above that we are testing all the different cases introduced here, I think there really is nothing else to ask for here before this can go in.

Wim Leers’s picture

Having gone through all these iterations, however, I do now feel pretty confident that we have arrived at a correct, secure and thoroughly tested solution in the form of this patch.

Great! I agree :)

Thank you so much for your reviews here — the patch is better for it!

effulgentsia’s picture

I did an initial read of this patch, and it looks great to me so far. I'd like to review it in more depth before committing it, but in the meantime, the current issue summary looks out of date to me. This patch is not implementing approach #6, and the issues listed in the remaining tasks don't appear to be blockers for this anymore.

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -263,38 +227,38 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
-        if ($this->getCastedValueFromFieldItemList($original_entity->get($field_name)) === $this->getCastedValueFromFieldItemList($entity->get($field_name))) {
...
+      elseif (... !$original_field->equals($field)) {

Is it possible to split this change (which is great!) and associated test coverage (if any) into a separate issue that we can commit first? Especially since we had to revert this issue's patch already once due to security considerations, I'd like to make it as much of a single responsibility patch as possible for the next time we commit it.

Setting to Needs work for that, but if doing this is not possible or not wise for some reason, please re-RTBC with an explanation. Thanks!

effulgentsia’s picture

+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -263,38 +227,38 @@ public function patch(EntityInterface $original_entity, EntityInterface $entity
+      if ($original_entity instanceof UserInterface && $field_name === 'pass') {
+        $original_entity->set('pass', $field->getValue());
+        continue;
       }
+      if ($original_field->access('edit')) {
+        $original_entity->set($field_name, $field->getValue());
+      }
+      elseif (!$original_field->access('view') || !$original_field->equals($field)) {
         throw new AccessDeniedHttpException("Access denied on updating field '$field_name'.");
       }

Also, I think there's enough logic here now to warrant a protected method for encapsulating it. My suggestion is perhaps renaming EntityResourceAccessTrait::checkEditFieldAccess() to checkCreateFieldAccess() (perhaps leaving the original name as a BC wrapper if we think there might be contrib users of the trait that we care about) and then adding a checkUpdateFieldAccess() that could similarly throw the exception or return the fields that the caller should update on $original_entity.

This would also then have the benefit of being able to unit test this protected method.

Wim Leers’s picture

Status: Needs work » Needs review
Related issues: +#2929124: Remove EntityResource::getCastedValueFromFieldItemList() helper in favor of FieldItemList::equals()
FileSize
4.91 KB
25.34 KB

#188: I think you'll forgive us not updating this issue summary — this issue has been an absolute nightmare in that respect. Changed direction so many times, so many unknown blockers discovered along the way… :( Will do that later.

#189:

Is it possible to split this change (which is great!) and associated test coverage (if any) into a separate issue that we can commit first?

Created #2929124: Remove EntityResource::getCastedValueFromFieldItemList() helper in favor of FieldItemList::equals() for this. Waiting for that to land to update this patch.

#190:

Also, I think there's enough logic here now to warrant a protected method for encapsulating it.

👍 Done. I do have one concern about this though: it makes it less clear what special security concerns/measures/handling is necessary when PATCHing entities. So no unit test yet. Wondering what @tstoeckler thinks.

My suggestion is perhaps renaming

👎 That'd break BC.

and then adding

👎 That's out of scope here. I've added a @internal protected method to EntityResource, we can choose to move that to a trait later. It's especially out of scope because in all of core there would be only one class using it. Until we have two callers, it doesn't make sense to move this into a trait.

tstoeckler’s picture

Re:

Also, I think there's enough logic here now to warrant a protected method for encapsulating it.

👍 Done. I do have one concern about this though: it makes it less clear what special security concerns/measures/handling is necessary when PATCHing entities. So no unit test yet. Wondering what @tstoeckler thinks.

I can see your point @Wim Leers. It is fairly weird to have a 7 line method that either returns a boolean or throws an exception. One idea to improve this situation is to actually move the ->set() call into that method as well (because $entity->set($field_name) <=> $entity->get($field_name)->setValue() this should not be too difficult). That way the method would consist solely of a series of guard conditions (albeit some of which simply return while others throw an exception, but still) and a final ->set() call. Not sure. Maybe that makes things even more confusing, I'm not married to that idea. Just thinking out loud...

In any case, however, I think this method gives us the chance to move the user password workaround to UserResource, like @tedbow proposed above. I think if we are adding such a method anyway, we should go ahead and do that.

Wim Leers’s picture

FileSize
3.24 KB
26.72 KB

One idea to improve this situation is to actually move the ->set() call into that method as well

I like that even less, because then it becomes checkPatchFieldAccessAndSetIfAllowed()… i.e. it'd no longer be doing one thing: access checking.
(+1 for thinking out loud!)

In any case, however, I think this method gives us the chance to move the user password workaround to UserResource, like @tedbow proposed above. I think if we are adding such a method anyway, we should go ahead and do that.

Oh, that is interesting! I hadn't thought of that. Did that.

Wim Leers’s picture

FileSize
1.94 KB
24.84 KB
In any case, however, I think this method gives us the chance to move the user password workaround to UserResource, like @tedbow proposed above. I think if we are adding such a method anyway, we should go ahead and do that.

Oh, that is interesting! I hadn't thought of that. Did that.

Well … I actually investigated it a bit more, but still wanted to post that. The original reason for introducing that is at #157. And that reasoning was correct: the pass field was no longer being set for fields whose value hadn't changed.
But! Thanks to @cburschka's analysis, we ended up with a different implementation, which does always set the value. So actually, we didn't need the special casing of User's pass field anymore!

effulgentsia’s picture

Thanks to @cburschka's analysis, we ended up with a different implementation, which does always set the value.

How so? If you send a patch request to change the user's email field, but don't have permission to view or edit the user's password, how is it that that works with #194?

Wim Leers’s picture

Wim Leers’s picture

How so? If you send a patch request to change the user's email field, but don't have permission to view or edit the user's password, how is it that that works with #194?

You're right, the simplification in #194 is only possible when a user has edit access for the pass field: EntityResource::patch() only sets the received field value if that field can be edited. So it would seem our test coverage is incomplete.

However… this is what \Drupal\user\UserAccessControlHandler::checkFieldAccess() does:

    // Fields that are not implicitly allowed to administrative users.
    $explicit_check_fields = [
      'pass',
    ];

    // Administrative users are allowed to edit and view all fields.
    if (!in_array($field_definition->getName(), $explicit_check_fields) && $account->hasPermission('administer users')) {
      return AccessResult::allowed()->cachePerPermissions();
    }
…
    switch ($field_definition->getName()) {
…
      case 'pass':
        // Allow editing the password, but not viewing it.
        return ($operation == 'edit') ? AccessResult::allowed() : AccessResult::forbidden();
…

In other words: anybody who is allowed to edit the User entity is also allowed to modify their password. (IOW: all users with administer users permission.) There is explicit unit test coverage for this at \Drupal\Tests\user\Unit\UserAccessControlHandlerTest::testPasswordAccess(). Introduced in #2029855: Missing access control for user base fields + #2388707: UserAccessControlHandler has wrong $explicit_check_fields name for the password field when checking field access.

Conclusion: the simplification in #194 is okay, unless a contrib/custom module implements hook_entity_field_access() or hook_entity_field_access_alter() to change User->pass access.

effulgentsia’s picture

Status: Needs review » Needs work

the simplification in #194 is okay, unless a contrib/custom module implements hook_entity_field_access() or hook_entity_field_access_alter() to change User->pass access

That's not an unreasonable scenario though. Can we add a test that tests such a scenario? If that test fails on HEAD, then we can punt it to a separate issue. But if it passes on HEAD and fails on #196, then I think not regressing that needs to be part of this issue's scope.

Wim Leers’s picture

Fair request. I can't help but remark though, that if the REST module had been reviewed even 1/10th as strictly as this, we wouldn't be dealing with this today… 😩

(The test-only patch that applies to HEAD is also the interdiff.)

The test does fail on HEAD, so punting to a new issue, created here: #2930182: [PP-1] Module forbidding the 'edit' operation on the User entity's 'pass' field would prevent editing security-sensitive base fields.

The last submitted patch, 199: 2824851-user_pass_edge_case_test_only-FAIL.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 199: 2824851-199-user_pass_edge_case-FAIL.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
22.86 KB

As predicted, #199 fails on both HEAD and with the patch. Therefore, per #198, punting that to a separate issue: #2930182: [PP-1] Module forbidding the 'edit' operation on the User entity's 'pass' field would prevent editing security-sensitive base fields.

Reuploading the previous patch (#196), which was green, and moving back to RTBC, because all committer feedback has been addressed.

effulgentsia’s picture

Just some minor cleanup. Leaving at RTBC.

effulgentsia’s picture

Removing self credit.

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -226,38 +227,12 @@ 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.
-        elseif (isset($entity_keys['langcode']) && $field_name === $entity_keys['langcode'] && $field->isEmpty()) {
-          continue;
-        }

I'm trying to understand what this was originally here for. Looks like HEAD is allowing a langcode to be submitted as part of the patch request but allowing it to be set to something that $field->isEmpty() would return true for? (perhaps an empty array or empty string?) Was there any valid use case of clients doing that? Looks like this patch removes that, which seems sensible, but are we sure this isn't a BC break for whatever use case this was added for in the first place?

Setting to NR just for that question.

cburschka’s picture

Status: Needs review » Needs work

Added in #2183231-260: Make ContentEntityDatabaseStorage generate static database schemas for content entities.

Good catch! I just tried to PATCH with langcode:[], and apparently this gets all the way to the database before it runs into the NOT NULL constraint and triggers an SQL error.

The website encountered an unexpected error. Please try again later.Symfony\Component\HttpKernel\Exception\HttpException: Internal Server Error in Drupal\rest\Plugin\rest\resource\EntityResource->patch() (line 248 of core/modules/rest/src/Plugin/rest/resource/EntityResource.php).

Drupal\Core\Database\Statement->execute(Array, Array) (Line: 625)
Drupal\Core\Database\Connection->query('UPDATE {node} SET nid=:db_update_placeholder_0, vid=:db_update_placeholder_1, type=:db_update_placeholder_2, uuid=:db_update_placeholder_3, langcode=:db_update_placeholder_4
WHERE nid = :db_condition_placeholder_0', Array, Array) (Line: 87)
Drupal\Core\Database\Driver\mysql\Connection->query('UPDATE {node} SET nid=:db_update_placeholder_0, vid=:db_update_placeholder_1, type=:db_update_placeholder_2, uuid=:db_update_placeholder_3, langcode=:db_update_placeholder_4
WHERE nid = :db_condition_placeholder_0', Array, Array) (Line: 148)
In the unpatched code, the empty langcode field is simply ignored. So while there may not be a legitimate use case for setting the empty field, it might be desirable to harden it a bit and avoid an internal server error here. ;)
cburschka’s picture

Status: Needs work » Needs review

(Though actually, I'm not sure that's within the scope for this issue. Maybe it should be handled in the entity storage? Given that the rest resource code knows nothing about the langcode field or how it gets stored, trying to treat this special case in the PATCH method feels wrong.)

Wim Leers’s picture

I agree with #207. It's better to get a 500 response with Internal Server Error than having custom code for one particular edge case in REST module. Because this same bug will also exist for https://www.drupal.org/project/jsonapi and https://www.drupal.org/project/graphql.

#206: would you mind either posting a patch that adds test coverage for this, or post exact steps to reproduce? I already created an issue for this: #2930968: PATCHing entities with langcode = [] results in fatal error. (Yes, #2183231: Make ContentEntityDatabaseStorage generate static database schemas for content entities should have added test coverage for this, but since that did not happen, we'll have to do it now.)

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

The failing test coverage added in #2930968-6: PATCHing entities with langcode = [] results in fatal error by @cburschka clearly shows this is a bug in Entity API itself, not in REST.

Therefore moving this back to RTBC, per #207.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 203: 2824851-203.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
21.39 KB

#298702: Properly validate image uploads conflicted with this in a trivial way in rest_test.module (that hunk is now no longer necessary).

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 211: 2824851-211.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

After being RTBC and having green tests for >7 days, #211 failed with "CI aborted" like this:

00:45:21.431 Drupal\Tests\rest\Functional\EntityResource\Term\TermXmlBasi   2 passes                                      
01:50:00.090 Build timed out (after 110 minutes). Marking the build as aborted.
01:50:00.099 Build was aborted

Looks like the AWS instance got frozen. Back to RTBC, and re-testing.

effulgentsia’s picture

Thanks for the investigation in #206 - #209! I opened #2933408: Remove broken allowance of empty langcode in a PATCH request to see if the hunk in #205 can be committed on its own.

larowlan’s picture

Can we get an issue summary update here, the current proposed solution/remaining tasks indicate this is postponed on something else - but given we're at RTBC, I assume things have changed?

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/rest/src/Plugin/rest/resource/EntityResource.php
@@ -226,38 +227,12 @@ 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.
-        elseif (isset($entity_keys['langcode']) && $field_name === $entity_keys['langcode'] && $field->isEmpty()) {
-          continue;
-        }

Per #214, I think this is out of scope for this issue, and spun it out into that child issue. In order to not hold the rest of this up any longer, can this issue be rerolled to still include this workaround? An @todo linking to #2933408: Remove broken allowance of empty langcode in a PATCH request would be great.

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
1.19 KB
21.77 KB

Done.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 217: 2824851-217.patch, failed testing. View results

vaplas’s picture

Status: Needs work » Reviewed & tested by the community

Random fail.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 217: 2824851-217.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 217: 2824851-217.patch, failed testing. View results

Wim Leers’s picture

Status: Needs work » Reviewed & tested by the community

Random infra fail again:

Exception: Warning: apcu_store(): Unable to allocate memory for pool.

  • effulgentsia committed 0dc3093 on 8.5.x
    Issue #2824851 by Wim Leers, arshadcn, amateescu, effulgentsia, tedbow,...
effulgentsia’s picture

Status: Reviewed & tested by the community » Fixed

Thank you to everyone who helped arrive at this solution. I think it's really nice. Pushed to 8.5.x!

Wim Leers’s picture

tstoeckler’s picture

Wim Leers’s picture

"small" he says, whereby he means "f**king impressive" and "scary yet fascinating" ;) :)

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Wim Leers’s picture

@cburschka in #110:

The PathItem::class case in getModifiedEntityForPatchTesting isn't right:

        case PathItem::class:
          // PathItem::generateSampleValue() doesn't set a PID, which causes
          // PathItem::postSave() to fail. Keep the PID (and other properties),
          // just modify the alias.
          $value = $field->getValue();
          $value['alias'] = str_replace(' ', '-', strtolower((new Random())->sentences(3)));
          $field->setValue($value);
          break;

::getValue() returns the list of items, not the first item. We'd have to set $value[0]['alias'] here to work correctly. Current data sent to ::setValue()

array(2) {
  [0]=>
  array(4) {
    ["pid"]=>
    string(1) "1"
    ["source"]=>
    string(7) "/node/1"
    ["alias"]=>
    string(6) "/llama"
    ["langcode"]=>
    string(2) "en"
  }
  ["alias"]=>
  string(40) "cogo-luctus-mauris-nostrud-tation-valde."
}

(The PATCH still gets rejected as designed. That's why it didn't cause a test failure.)

Can we do this as a follow-up, or does it need a separate issue?

You're absolutely right, but due to the security revert and the ensuing complexity and discussion, we completely lost track of this. Fixing it in #2939802: Follow-up for #2824851: EntityResourceTestBase::getModifiedEntityForPatchTesting() handles @FieldType=path incorrectly!

Wim Leers’s picture

Issue tags: +8.5.0 highlights

This so vastly improves the DX for REST API clients that I think it's worth a highlight.