Problem/Motivation

I think for security purposes we need to be doing json here instead of PHP serialization since this is field values on an entity. It's unfortunate that the serialize blog stuff doesn't just support a format so that we don't have to manually intervene to make json happen.

From #2905922-24: Implementation issue for Layout Builder

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#8 2914503-8.patch676 bytesjibran
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett created an issue. See original summary.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett as a volunteer commented

#2232427: Allow field types to control how properties are mapped to and from storage

samuel.mortenson’s picture

samuel.mortenson
he/him
Primary language English
CreditAttribution: samuel.mortenson at Acquia commented
Status: Active » Needs review

@tim.plunkett I reviewed the module and couldn't find any security issues specifically related to serialization. When you use a serialized column the unserialization/serialization happens before a user gets or sets the value of a field, which means that object injection is not possible by setting the value of the field to a serialized string. That said, I discovered follow up items we should address in new issues:

  1. Add a DataType normalizer for layout_section - when accessing (GET-ing) a LayoutSectionItem via REST today, you see a list of Section objects but each is empty. These should show components with their configuration.
  2. Add REST test coverage for what GET/PATCH/POST looks like with LayoutSectionItem fields.
  3. Consider using JSON instead of storing serialized data - this could make REST easier to support, and if each Section ultimately stores an array of block plugin configuration, which is also an array, JSON seems like a good fit. This would also resolve any security weariness.
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett as a volunteer commented
Status: Needs review » Needs work
Issue tags: +Layout Builder stable blocker

Thanks @samuel.mortenson!

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

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

samuel.mortenson’s picture

samuel.mortenson
he/him
Primary language English
CreditAttribution: samuel.mortenson at Acquia commented
Status: Needs work » Fixed

I've created #2942975: [PP-1] Expose Layout Builder data to REST and JSON:API and #2942976: Add REST test coverage for Layout Builder, which cover points one and two from comment #3.

I don't think we should file an issue for #3.3 until we start work on a Normalizer - if we find that supporting the current storage format is too complicated, we can look at moving to JSON.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
Status: Fixed » Needs work

There is a todo in core pointing towards this issue.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran as a volunteer commented
Status: Needs work » Needs review
FileSize
2914503-8.patch676 bytes
samuel.mortenson’s picture

samuel.mortenson
he/him
Primary language English
CreditAttribution: samuel.mortenson at Acquia commented
Status: Needs review » Reviewed & tested by the community

@jibran Great catch, thanks!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed b5d11aeac5 to 8.6.x and fcd1465565 to 8.5.x. Thanks!

  • alexpott committed b5d11ae on 8.6.x 
    Issue #2914503 by jibran, tim.plunkett, samuel.mortenson: Determine if...

  • alexpott committed fcd1465 on 8.5.x 
    Issue #2914503 by jibran, tim.plunkett, samuel.mortenson: Determine if...
tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett as a volunteer commented
Component: layout.module » layout_builder.module

Status: Fixed » Closed (fixed)

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