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

[nikeo]

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.

[peterwilsoncc]

Thanks for logging this and including some proposed solutions.

I've moved this on to the 5.0 milestone.

[nikeo]

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

[antpb]

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.

[ocean90]

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

[pento]

[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]

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.

[westonruter]

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?

[nikeo]

@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

[peterwilsoncc]

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.

[westonruter]

@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'] );
        }

// ...

[peterwilsoncc]

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.

[peterwilsoncc]

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

[peterwilsoncc]

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.