### Problem/Motivation

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

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

None.

None.

### Data model changes

None.

CommentFileSizeAuthor
#21721.77 KBWim Leers
#2171.19 KBWim Leers
#21121.39 KBWim Leers
#2033.42 KBeffulgentsia
#20321.92 KBeffulgentsia
#20222.86 KBWim Leers
#19925.51 KBWim Leers
#1992.71 KBWim Leers
#19622.86 KBWim Leers
#19424.84 KBWim Leers
#1941.94 KBWim Leers
#19326.72 KBWim Leers
#1933.24 KBWim Leers
#19125.34 KBWim Leers
#1914.91 KBWim Leers
#18324.19 KBWim Leers
#1833.94 KBWim Leers
#18123.56 KBWim Leers
#1811.09 KBWim Leers
#17523.12 KBWim Leers
#16950.92 KBWim Leers
#1691.98 KBWim Leers
#16751.74 KBamateescu
#15926.75 KBWim Leers
#1593.38 KBWim Leers
#15827.18 KBWim Leers
#1583.09 KBWim Leers
#15724.16 KBWim Leers
#1571.66 KBWim Leers
#14823.51 KBWim Leers
#1481.31 KBWim Leers
#1482.6 KBWim Leers
#14223.44 KBWim Leers
#1422.69 KBWim Leers
#13823.27 KBWim Leers
#1381.33 KBWim Leers
#13722.44 KBWim Leers
#13428.04 KBWim Leers
#1347.82 KBWim Leers
#1342.21 KBWim Leers
#13028.04 KBWim Leers
#1307.8 KBWim Leers
#1301.67 KBWim Leers
#12426.43 KBWim Leers
#1246.17 KBWim Leers
#1244.39 KBWim Leers
#12323.06 KBWim Leers
#1232.8 KBWim Leers
#1121 KBcburschka
#106760 bytesWim Leers
#10620.31 KBWim Leers
#9919.97 KBWim Leers
#991.24 KBWim Leers
#8919.84 KBWim Leers
#892.68 KBWim Leers
#8818.45 KBWim Leers
#882.26 KBWim Leers
#7617.09 KBWim Leers
#765.07 KBWim Leers
#7515.72 KBWim Leers
#751.86 KBWim Leers
#7413.93 KBWim Leers
#74950 bytesWim Leers
#7313.05 KBWim Leers
#73886 bytesWim Leers
#7212.24 KBWim Leers
#723.78 KBWim Leers
#6810.41 KBWim Leers
#685.85 KBWim Leers
#674.62 KBWim Leers
#453.99 KBtedbow
#457.75 KBtedbow
#391.83 KBWim Leers
#394.15 KBWim Leers
#36942 bytestimmillwood
#363.94 KBtimmillwood
#323.05 KBWim Leers
#323.21 KBWim Leers
#171.39 KBWim Leers
#172.32 KBWim Leers
#162.42 KBWim Leers
#162.69 KBWim Leers
#152.47 KBWim Leers
#153.55 KBWim Leers
#12597 bytesWim Leers
#124.09 KBWim Leers
#93.31 KBamateescu
#93.91 KBamateescu
#62.45 KBamateescu

Wim Leers created an issue. See original summary.

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

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

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

 Issue tags: -blocker
 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.

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

 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.

Thanks, now taking a look at this!

 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.

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.

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

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.

 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.

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

 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. 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 :)  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') {
$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.

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

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

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

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

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

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

So, #29 is pretty convincing. Which means:

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

Update \Drupal\Core\Entity\EntityAccessControlHandler::checkFieldAccess() to not just blindly return AccessResult::allowed() for $op === 'edit', but instead returning that only if the field is not read-only. Read-only fields would get AccessResult::forbidden(). 2. Update \Drupal\comment\CommentAccessControlHandler::checkFieldAccess() (and any other checkFieldAccess()), to have all of its read-only fields upon $op === 'edit' return AccessResultAllowed when the pre-update value matches the post-update value.
4. Rather than replacing the special logic in EntityResource::patch() with different logic, removing it altogether. We want only
if (!$updated_entity->get($field_name)->access('edit')) {
throw new AccessDeniedHttpException("Access denied on updating field '$field_name'."); }  Nothing else. 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. +++ 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. #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.  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.  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? 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

 Related issues: +#2350429: Clarify the meaning of read-only fields and properties

Should we move this to the entity system component?

 Status: Needs review » Needs work

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

 Issue tags: +Needs issue summary update

A real IS would be nice. :)

 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.

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

@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. FileSize 953 bytes 945 bytes 944 bytes 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'))
}


\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.  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.  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)  Status: Needs review » Needs work The last submitted patch, 55: 2824851-55-nid.patch, failed testing. @arshadcn Thanks for getting this going again! You can PATCH read-only fields. Indeed, the current logic allows for that — but only if the value is the same as the existing value:  // Unchanged values for entity keys don't need access checking. if ($original_entity->get($field_name)->getValue() ===$entity->get($field_name)->getValue()) { continue; }  Note that we need some read-only values to be sent on PATCH requests, see #2631774: Impossible to update Comment entity with REST (HTTP PATCH): bundle field not allowed to be updated, but EntityNormalizer::denormalize() requires it. TIL \Drupal\Core\Field\FieldItemList::equals() exists, which may be useful here. This is also definitely related to #2456257: [PP-1] Normalizers must set$entity->_restSubmittedFields for entity POSTing/PATCHing to work. Solving that issue may also solve this issue.

 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.  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  Assigned: Wim Leers » Unassigned Issue summary: View changes Issue tags: -Needs issue summary update Related issues: +#2885411: "Entity keys" aren't actually keys — rename to avoid misinterpretation, +#2392845: Add a trait to standardize handling of computed item lists 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.  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.  Issue tags: +Needs issue summary update 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.  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++ FileSize 5.85 KB 10.41 KB 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. 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.

 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.

FileSize
886 bytes
13.05 KB

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!

FileSize
950 bytes
13.93 KB

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!

FileSize
1.86 KB
15.72 KB

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

FileSize
5.07 KB
17.09 KB

So #75 should be green! Fingers crossed :)

Now cleaning up:

1. extract a big chunk of logic into a helper method

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.

 Assigned: Wim Leers » Unassigned

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

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.

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). 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. #84++ #85++ 👏 Oh and #83++ for being so vigilant!  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. FileSize 2.68 KB 19.84 KB 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! 🎉 +++ 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. 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. But isn't this potentially problematic for any field type? 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. #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! 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. @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. 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.  Issue tags: -Needs tests FileSize 1.24 KB 19.97 KB 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.  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! 🎉 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 ;) ❤️ 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.  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. FileSize 20.31 KB 760 bytes Fixing 2 coding standards violations though — two unused use statements. Added test runs for both 8.5.x and 8.4.x.  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,... OMG YAY! 🎉 Also very glad to read that the detailed issue summary with patch progression explanation helped :)  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? 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' =>..  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  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,...  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.  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. 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.  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.  Assigned: Unassigned » Wim Leers Still working on this. I think I see a way forward now.  Status: Needs work » Needs review FileSize 2.8 KB 23.06 KB 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.)  Issue tags: -Needs tests FileSize 4.39 KB 6.17 KB 26.43 KB #123 added sensible security coverage. But to really know for certain that things are working as expected, we should expand the test coverage to verify that granting the create url aliases permission then results in a 200 response: granting the necessary permission does allow the path field to be changed. Note that there are three entity types that have a path field: node + term + media. The latter doesn't have any REST test coverage yet, so we leave that one out of consideration. Back when we worked on fixing path fields handling in #2846554: Make the PathItem field type actually computed and auto-load stored aliases, we chose to use NodeResourceTestBase to NOT GRANT the create url aliases permission by default, and to use TermResourceTestBase to GRANT the create url aliases permission by default, i.e. they'd be each other opposites. For that reason, we should also add similar test coverage to TermResourceTestBase as #123 already did for NodeResourceTestBase. But rather than expecting a 403 response, we should expect a 200 response. We again have a HEAD-relative patch and a patch that expands the committed-but-reverted patch. The interdiff is the same for both. Now we have the test coverage we need! The patches in #123 prove that in HEAD, a 403 is returned when appropriate when PATCHing a node's path, and it proves that the committed-but-since-reverted patch caused that to change. The patches in #124 prove that in HEAD, changing path aliases didn't work at all, and it proves that the committed-but-since-reverted patch caused that to change. 🤔😱😭😩  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

 Status: Needs work » Needs review
FileSize
1.67 KB
7.8 KB
28.04 KB

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

+++ 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 ...  Status: Needs work » Needs review FileSize 2.21 KB 7.82 KB 28.04 KB 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  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).  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. FileSize 1.33 KB 23.27 KB 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 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".  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 +++ 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? 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: Trust me, that will save you a lot of time and hair pulling over the current implementation of the path field :)  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 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?  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. 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 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. 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. 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 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 ;-)) 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.) 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.  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.

FileSize
3.09 KB
27.18 KB

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

This should be green!

FileSize
3.38 KB
26.75 KB

And finally, clean-up!

1. +++ b/core/modules/hal/tests/src/Functional/EntityResource/Node/NodeHalJsonAnonTest.php
--- 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.

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

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?

#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

+++ 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 :)  Status: Needs work » Needs review FileSize 51.74 KB 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.  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: @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!  Status: Needs review » Postponed  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  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  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.)  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.  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 +++ 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. 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: 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.

#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. #178 thats sounds fine to wait for another special case to refactor out as I suggested in #176  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...

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

 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.

 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.

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!

 Issue tags: +Needs issue summary update

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.

 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!

+++ 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.  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. 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. 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. 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! 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? FileSize 22.86 KB 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.

 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.

 Status: Needs work » Needs review Related issues: +#2930182: [PP-1] Module forbidding the 'edit' operation on the User entity's 'pass' field would prevent editing security-sensitive base fields
FileSize
2.71 KB
25.51 KB

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

 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.

FileSize
21.92 KB
3.42 KB

Just some minor cleanup. Leaving at RTBC.

Removing self credit.

 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.  Status: Needs review » Needs work 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. ;)  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.) 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.)  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  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  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. 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. 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?  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.  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  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  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  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,...  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! 🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉🎉 "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. @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!

 Issue tags: +8.5.0 highlights

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