Problem/Motivation

(Why the issue was filed, steps to reproduce the problem, etc.)
Problem:
- After 8.6.10 (SA-CORE-2019-003) there is an issue with site installation for sites that have default content which includes paragraphs. Paragraphs was also updated to 1.6 along with the above mentioned core security release. This release included a change in entity annotation to account for a new `serialized_field_property_names` annotation, which Paragraphs has set for its behavior_settings property in 1.6.

The code changes in hal, serialization modules' FieldItemNormalizer class don't seem to account for the serialized field's value being empty and has an if !empty check before serializing the data, which stops empty values being serialized. This leads to an unserialized empty array being handed to save to the database and there is an exception thrown:

InvalidArgumentException: Placeholders must have a trailing [] if they are to be expanded with an array of values. in /var/www/html/app/core/lib/Drupal/Core/Database/Connection.php:729                                               [error]

Steps to reproduce if you don't have an existing installation with default content:

  1. Install Drupal 8.6.10 using minimal profile with Paragraphs and Default Content latest
  2. Create new paragraph type (paragraph_text) with a single text field
  3. Create new content type (article) with a paragraph reference field
  4. Create new article content, add the paragraph_text and add some content and save.
  5. Create a new simple custom_content module with an info.yml and a content folder. Enable it.
  6. Export the content with default_content into the new module: drush dcer --folder="modules/custom_content/content" (replace the path if necessary)
  7. Export the site configuration into a sync folder
  8. Reinstall the site using the exported configuration: drush si --config-dir="path/to/config/sync" (Note: I had to manually delete the custom_content/content/user folder before installation as this throws a duplicate key error and is completely unrelated imho)

Expected result:
Site installs without exceptions thrown and the default content successfully imported from custom_content module.

Actual result:
Site installation fails and an exception is thrown:

InvalidArgumentException: Placeholders must have a trailing [] if they are to be expanded with an array of values. in /var/www/html/app/core/lib/Drupal/Core/Database/Connection.php:729                                               [error]

Known workarounds:
- Manually editing the exported content's json. Specifically the ones in the paragraphs folder, which have "behavior_settings" and its value set to an empty array. Removing the behavior_settings with empty values from the json of all exported paragraphs makes the site installation successful and produces the Expected result above.

Proposed resolution

The hal and serialization core modules' FieldItemNormalizer` should not check if a value is empty or not. Instead it should normalize/serialize the value regardless if it's empty, and only check if the array key exists. Attaching a patch

In 8.7.x this is handled using a common trait FieldableEntityNormalizerTrait in both modules and uses in_array to check. Currently in 8.6.x !empty is used to check whether to serialize the value or not.

Remaining tasks

(reviews needed, tests to be written or run, documentation to be written, etc.)

User interface changes

(New or changed features/functionality in the user interface, modules added or removed, changes to URL paths, changes to user interface text.)

API changes

(API changes/additions that would affect module, install profile, and theme developers, including examples of before/after code if appropriate.)

Data model changes

(Database or configuration data changes that would make stored data on an existing site incompatible with the site's updated codebase, including changes to hook_schema(), configuration schema or keys, or the expected format of stored data, etc.)

Release notes snippet

(Major and critical issues should have a snippet that can be pulled into the release notes when a release is created that includes the fix)

Original report by [username]

(Text of the original report, for legacy issues whose initial post was not the issue summary. Use rarely.)

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Baysaa created an issue. See original summary.

Baysaa’s picture

Status: Active » Needs review
Berdir’s picture

Priority: Normal » Major

Thanks, great issue summary, I noticed this too but never got around to create an issue and add tests. We have tests for the SA-related changes that we did not yet release, but my understanding is that it is fine to do that now as the exploit is public now. I'll try to provide the tests we have here.

I think this is at least major.

Berdir’s picture

And here are some tests. Only for 8.6 as a first step, as 8.7.x isn't broken apparently but will port the patches to prove that.

The last submitted patch, 4: serialize-empty-values-4-test-only.patch, failed testing. View results

Status: Needs review » Needs work

The last submitted patch, 4: serialize-empty-values-4.patch, failed testing. View results

Baysaa’s picture

Status: Needs work » Needs review

Thanks for the swift response Berdir and the test additions.

I've tried your patch in #4, it applies cleanly to latest 8.6.x. The tests are green if you click "View results" but I think due to an issue with CI yesterday it's showing failed and put back into Needs work. (Looks like it's slowly catching up as I'm clicking around)

The test-only patch seems to correctly demonstrate the issue and the failures indicating that an array is not same as a string (serialized) looks correct to me.

I've re-started the test and tentatively setting this back to Needs review

Berdir’s picture

Thanks, looks like the combined patch now passed. I accidently triggered a test of the test-only patch for 7.x instead of 8.7.x, that's why that didn't apply. Because it looks like the test only patch actually works on 8.7 too and will then show that a fix is not needed there.

Berdir’s picture

Ok, doesn't apply, here's a 8.7.x patch of that.

lpeabody’s picture

Status: Needs review » Needs work

Can't get serialize-empty-values-4.patch to apply against 8.6.10.

EDIT: Ignore me, I commented on a stale form from yesterday, I hadn't looked at your latest post Berdir.

lpeabody’s picture

Status: Needs work » Needs review

And the stale form set it to Needs work... I'm reviewing this morning.

lpeabody’s picture

Hm, strange, still can't get the patch to apply against 8.6.10.

lpeabody’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
1.59 KB

Confirming that the changes made by Berdir to hal and serializer's FieldItemNormalizer fix the issues default_content has with creating Paragraph entities. Attached patch with application changes only, re-rolled for 8.6.10.

Berdir’s picture

Patches always need to target the dev branch because that's where they are committed.

The patches to commit are #9 for 8.7.x and the combined patch in #4 for 8.6.x. No patch yet for 8.5.x, I'm guessing it won't be backported to that as there won't be another non-security release anyway. But I can provide patch for that too.

Also, jsonapi will need the same fix in the 8.x-1.x branch.

Berdir’s picture

Reuploading patches for 8.6, 8.7 (test only, that implementation did not have the bug), and also backported to 8.5.

catch’s picture

Version: 8.6.x-dev » 8.5.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 1506b0af4d to 8.8.x and 9f6c7c1e54 to 8.7.x. and....

Committed 7e521e2 and pushed to 8.6.x. and...

Committed dcafd41 and pushed to 8.5.x. Thanks!

  • catch committed 1506b0a on 8.8.x
    Issue #3037970 by Berdir, Baysaa: Custom serialized field's data should...

  • catch committed 9f6c7c1 on 8.7.x
    Issue #3037970 by Berdir, Baysaa: Custom serialized field's data should...

  • catch committed 7e521e2 on 8.6.x
    Issue #3037970 by Berdir, Baysaa: Custom serialized field's data should...

  • catch committed dcafd41 on 8.5.x
    Issue #3037970 by Berdir, Baysaa: Custom serialized field's data should...

Status: Fixed » Closed (fixed)

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