Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PlainText block not properly escaping attributes object in db #6181

Closed
dzaikos opened this issue Apr 15, 2018 · 7 comments · Fixed by #6619
Closed

PlainText block not properly escaping attributes object in db #6181

dzaikos opened this issue Apr 15, 2018 · 7 comments · Fixed by #6619
Assignees
Labels
[Type] Bug An existing feature does not function as intended

Comments

@dzaikos
Copy link

dzaikos commented Apr 15, 2018

Issue Overview

  • WordPress 4.9.5.
  • Gutenberg 2.6.0.
  • No other plugins, etc. running.

When registering a custom block type that uses the PlainText block, the object saved into the post_content database field is not a properly escaped JSON object.

Steps to Reproduce (for bugs)

I created a very simple plugin to test the usability of a simple textarea block for entering just plain text. The relevant JavaScript is as follows:

        wp.blocks.registerBlockType( 'my-custom/gutenberg-plain', {

                title: 'Plain Textarea',

                icon: 'format-aside',

                category: 'common',

                attributes: {
                        content: {
                                type: 'string'
                        }
                },

                edit: function ( props ) {
                        return wp.element.createElement(
                                wp.blocks.PlainText, {
                                        className: props.ClassName,
                                        value: props.attributes.content,
                                        onChange: function ( content ) {
                                                props.setAttributes( { content: content } );
                                        }
                                }
                        );
                },

                save: function () {
                        return null;
                }

        } );

This works fine. I can create a text block, enter the text and it stores it. In my case I'm using a render_callback to take the content attribute and manipulate it before displaying it on the front end. So entering this text in the editor works:

This is a sample of some plain text entered into a custom block.

But entering this type of text breaks things when WordPress tries to load the attributes later:

This is a sample of some "plain text" entered into a "custom block".

The problem is if I include any character that has a special meaning in a JSON object: ", ', [, ], {, }

Edit: Looks like it's just the double-quotes that are the problem and it appears the breakdown is here.

These characters are not escaped in the object when it's saved to the database, so when the attributes are reloaded (either to edit the post or display on the frontend), reading the object fails and therefore no content is provided (either in the editor or in the frontend).

Expected Behavior

Database post_content should look like this:

<!-- wp:my-custom/gutenberg-plain {"content":"This is a sample of some \u0022plain text\u0022 entered into a \u0022custom block\u0022."} -->

Current Behavior

Instead database content looks like this:

<!-- wp:my-custom/gutenberg-plain {"content":"This is a sample of some "plain text" entered into a "custom block"."} -->

Possible Solution

Properly escape the correct characters when building the object.

@aduth aduth self-assigned this Apr 17, 2018
@aduth aduth added the [Type] Bug An existing feature does not function as intended label Apr 17, 2018
@aduth
Copy link
Member

aduth commented Apr 17, 2018

Thanks for the report, @dzaikos . I'm not able to reproduce the original issue, given your plugin script.

Here is what I see saved in my database:

<!-- wp:my-custom/gutenberg-plain {"content":"This is a sample of some \"plain text\" entered."} /-->

A couple questions:

  • Do you see any errors with the saved block when trying to reload the editor page?
  • Do you have any other plugins active (particularly those which may modify markup before it is saved)?

@aduth aduth added the [Status] Needs More Info Follow-up required in order to be actionable. label Apr 17, 2018
@dzaikos
Copy link
Author

dzaikos commented Apr 18, 2018

Do you see any errors with the saved block when trying to reload the editor page?

No. The browser inspector shows everything is fine with respect to the request to save the post. In fact, my original suspicion of it being related to the stringify() call was wrong. When I examine the content of the request in the inspector it actually shows properly escaped data.

Do you have any other plugins active

No. Just the Twenty Seventeen theme and the Gutenberg plugin (with a stable version of WordPress, as mentioned in my original post).

So what I did was litter the WordPress core with debug code to track the POSTed data throughout the entire process. And I've narrowed it down to wp_filter_post_kses() here, which runs post_content through PHP's native addslashes() and stripslashes() functions. This line is causing the data to be saved to the database as I originally described and thereby break the JSON encoding Gutenberg uses when it later reads the data back from the database. If I replace line 1562:

return addslashes( wp_kses( stripslashes( $data ), 'post' ) );

With this:

return wp_kses( $data, 'post' );

It works every properly everytime.

Edit: The wp_magic_quotes() function called in wp-settings.php also adds slashes to the POST data, but the fix for me was in the kses function. I'm mentioning this since these functions obviously work a little bit together throughout the whole process of filtering user data.

Edit 2: So while I should be getting some sleep, I delved further and at the end of it all (because the kses functions are crazy numerous) it appears wp_kses_stripslashes() is also a/the culprit. The code change above fixes the issue for me, but bypassing wp_kses_stripslashes() also does.

Now I'm not suggesting we change the WP core, obviously. But hopefully it can give you an idea about why I'm experiencing the issue?

@aduth
Copy link
Member

aduth commented May 7, 2018

I believe user role may be a factor here. I was able to reproduce the original issue when saving a post as contributor, but not as administrator.

@aduth
Copy link
Member

aduth commented May 7, 2018

Easier steps to reproduce:

  1. Log in as contributor
  2. Navigate to Posts > Add New
  3. Insert a paragraph with some text
  4. Under Block > Advanced, set Additional CSS Class to include a double-quote
  5. Save the post

Expected: Quote should be escaped.
Actual: Quote is not escaped and breaks JSON syntax.

Example (from post_content after save):

<!-- wp:paragraph {"className":"additional class""} -->
<p class="additional class&quot;">This is a test.</p>
<!-- /wp:paragraph -->

@aduth
Copy link
Member

aduth commented May 7, 2018

An alternative option is to entity-encode rather than escape the characters, which shouldn't be subject to unslashing.

@aduth
Copy link
Member

aduth commented May 7, 2018

Proposed fix / workaround at #6619

@dzaikos
Copy link
Author

dzaikos commented May 7, 2018

I believe user role may be a factor here. I was able to reproduce the original issue when saving a post as contributor, but not as administrator.

Great work! I had DISALLOW_UNFILTERED_HTML defined in wp-config.php so that's why my admin user account was also experiencing the issue. Commenting that out, as you say, "fixes" it.

Hopefully your pull request fixing the issue can get approved as it seems to work well.

@danielbachhuber danielbachhuber added this to the WordPress 5.0 milestone Jun 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants