Problem/Motivation

Just before the committed #2824851 patch was reverted due to a security vulnerability that it exposed in another subsystem, @cburschka wrote this at #2824851-110: EntityResource::patch() makes an incorrect assumption about entity keys, hence results in incorrect behavior:

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 am currently working on porting REST's integration test coverage to JSON API in #2930028: Comprehensive JSON API integration test coverage phase 1: for every entity type, individual resources only, and this has come back to bite us.

The only reason it isn't a problem in core is that core's normalizers happily attempt to normalize whatever data they're given, including this case, where we're setting a second item on a single-cardinality field.

The PATCH request only fails because core perceives this as attempting to add a second item to this field, which qualifies as a change in value, and hence the edit field access check for the path field fails.

Proposed resolution

Rather than relying on this edge case, we should make this behave as @cburschka proposed. Because in the future, core may very well prevent adding a second item to a single-cardinality field … and in fact I'm surprised it doesn't already. The ->setValue() call quoted above should throw an exception, but doesn't.

Remaining tasks

None.

User interface changes

None.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#2 2939802-2.patch1 KBwim leers

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

Status: Active » Needs review
StatusFileSize
new1 KB
wim leers’s picture

Status: Needs review » Reviewed & tested by the community

Manually tested, works fine.

Please credit @cburschka!

wim leers’s picture

Title: EntityResourceTestBase::getModifiedEntityForPatchTesting() handles @FieldType=path incorrectly » Follow-up for #2824851: EntityResourceTestBase::getModifiedEntityForPatchTesting() handles @FieldType=path incorrectly

catch credited cburschka.

catch’s picture

catch’s picture

Status: Reviewed & tested by the community » Fixed

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

  • catch committed 639755c on 8.6.x
    Issue #2939802 by Wim Leers, cburschka: Follow-up for #2824851:...

  • catch committed b4cf8d0 on 8.5.x
    Issue #2939802 by Wim Leers, cburschka: Follow-up for #2824851:...

Status: Fixed » Closed (fixed)

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