WordPress.org

Make WordPress Core

Opened 10 months ago

Closed 7 months ago

#45292 closed defect (bug) (fixed)

wp_targeted_link_rel "corrupts" the customized changeset JSON when inserting the changeset post in database

Reported by: nikeo Owned by: peterwilsoncc
Milestone: 5.1 Priority: high
Severity: normal Version: 5.1
Component: Customize Keywords: has-patch has-unit-tests
Focuses: Cc:

Description

The problem

It happen when trying to publish customizations that include a link with a target attribute

When customizing a setting that generates a customized value containing html link strings with target attributes, it breaks the publishing action.
The customizations are visible in the customizer preview, even when published, but the customized data are not actually saved in the database when publishing. This means that when you close the customizer, you don't see your customizations on front.

What happen ?

When the customized JSON includes links with targets attributes, the filter wp_targeted_link_rel "corrupts" the JSON in the post_content of the changeset_post. This occurs when the filter content_save_pre is fired.
It generates a JSON_ERROR_SYNTAX preventing the customizer publish action to be done. see https://core.trac.wordpress.org/browser/trunk/src/wp-includes/class-wp-customize-manager.php#L1119

How to reproduce ?

The problem can be reproduced with any plugin or theme including a customizer setting and control ( textarea or editor ) allowing users to generate html links strings.

Reproduce with the Twentynineteen theme

Copy and paste the following code in functions.php

// Registers a test section, setting and textarea control
add_action('customize_register', function( $wp_customize ) {
  $wp_customize->add_section(
    'test_target_link', array(
      'title'    => __( 'Tests : wp_targeted_link_rel', 'twentyseventeen' ),
      'priority' => 0, // Before Additional CSS.
    )
  );
  $wp_customize->add_setting(
    'test_target_link', array(
      'default'           => '',
      //'transport'         => 'postMessage',
      //'sanitize_callback' => 'twentyseventeen_sanitize_colorscheme',
    )
  );
  $wp_customize->add_control(
    'test_target_link', array(
      'label'           => __( 'Test target link breaks the publish action', 'twentyseventeen' ),
      'section'         => 'test_target_link',
      'type'            => 'textarea',
      'description'     => __( 'Test a link with target attribute not published', 'twentyseventeen' )
    )
  );
});

// Prints once at loop_start
add_action('loop_start', function() {
  if ( ! did_action( 'test_printed') ) {
      printf( '<br>%1$s<br>', get_theme_mod('test_target_link') );
  }
  do_action('test_printed');
});

Open the customizer and navigate to the test control. Paste the following html code in the textarea :

Global Climate Change : this <a href="https://climate.nasa.gov/vital-signs/carbon-dioxide/" target="_blank">time series</a> below shows global distribution and variation of the concentration of mid-tropospheric carbon dioxide in parts per million (ppm).

If you click on publish and quit the customizer, you won't see the text on front. It will be published only if the target attribute is removed.

Reproduce with a plugin

Customize Posts plugin ( https://wordpress.org/plugins/customize-posts/ )

Try to edit any post content by adding a link with a target attribute. It won't be published.

Nimble Builder plugin ( https://wordpress.org/plugins/nimble-builder/ )

Try to drag and drop the quote module ( which includes a link with a target attribute ). It won't be saved on publish.

As you will observe it, the json_parse_error customizer notification is displayed sometimes. But there's no error printed in the javascript console and the php debug_log.

How does the corrupted JSON look like when inserted in the changeset post

Corrupted JSON

{
    "twentynineteen::test_target_link": {
        "value": "Global Climate Change : this <a href=\"https://climate.nasa.gov/vital-signs/carbon-dioxide/\" target=\"_blank\" rel="noopener noreferrer">time series</a> below shows global distribution and variation of the concentration of mid-tropospheric carbon dioxide in parts per million (ppm).",
        "type": "theme_mod",
        "user_id": 1,
        "date_modified_gmt": "2018-11-05 20:02:57"
    }
}

As you can see, the attributes noopener noreferrer added by the wp_targeted_link_rel are not escaped with backslashes, which generates a JSON validation error.

Possible solutions

proposition 1

The function wp_targeted_link_rel could check if the $text param is a JSON, and then json_decode before the regex, and json_encode after.
see https://core.trac.wordpress.org/browser/trunk/src/wp-includes/formatting.php#L3036

proposition 2

The preg_replace_callback in the function could be improved to include the JSON case, and handle the escape backslashes.

proposition 3

Inspired by what is already done with the KSES filters here https://core.trac.wordpress.org/browser/trunk/src/wp-includes/class-wp-customize-manager.php#L2890.
The idea would be to remove and add back the wp_targeted_link_rel filter, before and after the insertion of the changeset post in database.

Attachments (1)

45292.diff (5.2 KB) - added by peterwilsoncc 7 months ago.

Download all attachments as: .zip

Change History (16)

#1 @peterwilsoncc
10 months ago

  • Keywords needs-patch needs-testing added
  • Milestone changed from Awaiting Review to 5.0
  • Severity changed from major to normal

Thanks for logging this and including some proposed solutions.

I've moved this on to the 5.0 milestone.

#2 follow-up: @peterwilsoncc
10 months ago

  • Keywords needs-unit-tests added; needs-testing removed

#3 in reply to: ↑ 2 @nikeo
10 months ago

Replying to peterwilsoncc: @peterwilsoncc thanks for taking care of it.

#4 @antpb
9 months ago

  • Milestone changed from 5.0 to Future Release

Work for this is not yet begun so I am moving this to a future release as we are moving toward 5.0 RC currently.

#5 @ocean90
9 months ago

  • Milestone changed from Future Release to 5.0
  • Priority changed from normal to high

The filter was introduced in #43187 so it needs to be fixed or reverted from the branch.

#6 @pento
9 months ago

  • Milestone changed from 5.0 to 5.1
  • Version changed from 5.0 to trunk

[42770] is being reverted from the 5.0 branch, so I'm moving this ticket to 5.1, so it can be addressed there, instead.

@peterwilsoncc
7 months ago

#7 follow-up: @peterwilsoncc
7 months ago

  • Keywords has-patch has-unit-tests added; needs-patch needs-unit-tests removed

In 45292.diff

  • Moves adding the default filters to a new function, wp_init_targeted_link_rel_filters()
  • Introduces wp_remove_targeted_link_rel_filters() for removing filters
  • Duplicates the kses approach to remove the filters when saving customizer change sets in src/wp-includes/class-wp-customize-manager.php.
  • Adds a test to ensure the filters are running when saving a post.

@westonruter I haven't included a test to ensure the filters are properly removed when saving change sets. I couldn't find any prior artwork for testing kses removal, are you able to point me in the right direction?

Tested manually using @nikeo's POC above, thanks boss.

#8 in reply to: ↑ 7 ; follow-up: @westonruter
7 months ago

Replying to peterwilsoncc:

I haven't included a test to ensure the filters are properly removed when saving change sets. I couldn't find any prior artwork for testing kses removal, are you able to point me in the right direction?

I'm not sure, but should the filter removal be tested? Wouldn't it be better to test that the changeset data isn't corrupted?

#9 @nikeo
7 months ago

@peterwilsoncc a quick feedback to let you know that I have successfully tested your patch with the test code proposed in the initial bug description ( see "Reproduce with the Twentynineteen theme" ). I was able to publish and see my result on front without any problems.
I've used a WP build from this point : https://github.com/WordPress/WordPress/tree/c31e8cf756aaec88b27ca175049ecbc859b35011 ( 4days old ) for my test.
thanks

#10 in reply to: ↑ 8 @peterwilsoncc
7 months ago

Replying to westonruter:

I'm not sure, but should the filter removal be tested? Wouldn't it be better to test that the changeset data isn't corrupted?

Yes, that's what I'd like to test but I am not sure how to as I can't find the equivalent tests to ensure KSES doesn't corrupt the data.

#11 @westonruter
7 months ago

@peterwilsoncc If I understand correctly, you can try creating a draft customize_changeset post that has one setting in it which contains a link as shown in the description. Then publish the changeset and make sure that the data in the changeset remains intact as valid JSON, and that it isn't corrupted as @nikeo shows the currently behavior.

For example:

<?php
// ...

        /**
         * Ensure that changeset data is not not corrupted by wp_targeted_link_rel().
         */
        public function test_uncorrpteded_changeset_data() {
                require_once ABSPATH . WPINC . '/class-wp-customize-manager.php';
                wp_set_current_user( $this->factory()->user->create( array( 'role' => 'administrator' ) ) );

                global $wp_customize;
                $wp_customize = new \WP_Customize_Manager();

                $option_name = 'test_target_link';

                $wp_customize->add_setting(
                        $option_name,
                        array(
                                'type' => 'option',
                        )
                );

                $value = 'Global Climate Change : this <a href="https://climate.nasa.gov/vital-signs/carbon-dioxide/" target="_blank">time series</a> below shows global distribution and variation of the concentration of mid-tropospheric carbon dioxide in parts per million (ppm).';
                $wp_customize->set_post_value( $option_name, $value );

                $wp_customize->save_changeset_post(
                        array(
                                'status' => 'draft',
                        )
                );
                $post_id = $wp_customize->changeset_post_id();

                $saved_data = json_decode( get_post( $post_id )->post_content, true );
                $this->assertInternalType( 'array', $saved_data );
                $this->assertEquals( $value, $saved_data[ $option_name ]['value'] );

                $wp_customize->save_changeset_post( array( 'status' => 'publish' ) );

                $this->assertEquals( get_option( $option_name ), $value );

                $saved_data = json_decode( get_post( $post_id )->post_content, true );
                $this->assertInternalType( 'array', $saved_data );
                $this->assertEquals( $value, $saved_data[ $option_name ]['value'] );
        }

// ...

This ticket was mentioned in Slack in #core by peterwilsoncc. View the logs.


7 months ago

#13 @peterwilsoncc
7 months ago

In 44714:

Customize: Remove wp_targeted_link_rel pre-save filter from change-sets.

The pre-save filters added to links in [43732] could invalidate JSON data when saving Customizer change-sets.

This removes the filters when saving and publishing change-sets.

Props peterwilsoncc, nikeo for testing.
See #45292.

#14 @peterwilsoncc
7 months ago

  • Owner set to peterwilsoncc
  • Status changed from new to assigned

To do: write and commit tests suggested in comment #11 prior to WP 5.1 RC1.

#15 @peterwilsoncc
7 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

The test discussed above will be created using the ticket #46198 after the 5.1 branch is forked.

Closing this ticket for the 5.1 milestone.

Note: See TracTickets for help on using tickets.