Make WordPress Core

Opened 7 weeks ago

Closed 7 weeks ago

#41488 closed defect (bug) (fixed)

Customize: uncaught JS error with namespaced nav menu fallbacks

Reported by: dlh Owned by: westonruter
Milestone: 4.9 Priority: normal
Severity: normal Version: 4.3
Component: Customize Keywords: has-patch commit
Focuses: Cc:


A wp_nav_menu() call with a namespaced fallback_cb can generate a JavaScript error if the backslash creates invalid JSON. For example:

wp_nav_menu( [
        'fallback_cb' => '\WordPress\my_callback,
        'theme_location' => 'menu-1',
        'menu_id' => 'primary-menu',
] );

throws an error at https://github.com/WordPress/WordPress/blob/82d0a53e77f0eb6a8de60b67a2b5266c4f042741/wp-includes/js/customize-preview-nav-menus.js#L113.

If the fallback happens to generate escape characters, like \birdpress\bird_callback, then the error doesn't occur, but the value of navMenuArgs.fallback_cb is mangled.

The attached patch takes a conservative approach by checking for a \ before determining a nav menu supports partial refresh.

I also tested what happens if the backslashes are padded to \\\\ in $exported_args. It fixed the issue as far as I could tell, but the potential for side-effects seems greater.

Attachments (4)

41488.diff (838 bytes) - added by dlh 7 weeks ago.
41488.2.diff (2.2 KB) - added by dlh 7 weeks ago.
41488.3.diff (1.4 KB) - added by westonruter 7 weeks ago.
41488.4.diff (1.4 KB) - added by westonruter 7 weeks ago.

Download all attachments as: .zip

Change History (15)

7 weeks ago

#1 @westonruter
7 weeks ago

  • Milestone changed from Awaiting Review to 4.9
  • Version set to 4.3

@dlh there should be no problem with accepting backslashes in callbacks. What you've come across may be incorrect use slashing or unslashing somewhere in the stack. How does fallback_cb get mangled?

#2 @dlh
7 weeks ago

Given this call:

wp_nav_menu( array(
	'fallback_cb'    => '\birdpress\bird_callback',
	'theme_location' => 'top',
	'menu_id'        => 'top-menu',
) );

the value of partial.params.navMenuArgs.fallback_cb I see at the line mentioned above is irdpressird_callback.

But with this call:

wp_nav_menu( array(
	'fallback_cb'    => '\WordPress\my_callback',
	'theme_location' => 'top',
	'menu_id'        => 'top-menu',
) );

the value of partial.params.navMenuArgs is a string, which throws the 'Missing navMenuArgs' error.

#3 @westonruter
7 weeks ago

@dlh yeah, I can see that the JSON is getting corrupted. To illustrate that the problem resides in slashing, try adding this patch:

  • src/wp-includes/class-wp-customize-nav-menus.php

    final class WP_Customize_Nav_Menus { 
    12711271                if ( isset( $args->customize_preview_nav_menus_args['can_partial_refresh'] ) && $args->customize_preview_nav_menus_args['can_partial_refresh'] ) { 
    12721272                        $attributes = sprintf( ' data-customize-partial-id="%s"', esc_attr( 'nav_menu_instance[' . $args->customize_preview_nav_menus_args['args_hmac'] . ']' ) ); 
    12731273                        $attributes .= ' data-customize-partial-type="nav_menu_instance"'; 
    1274                         $attributes .= sprintf( ' data-customize-partial-placement-context="%s"', esc_attr( wp_json_encode( $args->customize_preview_nav_menus_args ) ) ); 
     1274                        $attributes .= sprintf( ' data-customize-partial-placement-context="%s"', wp_slash( esc_attr( wp_json_encode( $args->customize_preview_nav_menus_args ) ) ) ); 
    12751275                        $nav_menu_content = preg_replace( '#^(<\w+)#', '$1 ' . $attributes, $nav_menu_content, 1 ); 
    12761276                } 
    12771277                return $nav_menu_content; 

You'll see that by wrapping the escaped attribute in wp_slash, it prevents the slashes from getting dropped. Somewhere somehow there is wp_unslash() or stripslashes() that is causing the JSON string "\\WordPress\\my_callback" to become the invalid "\WordPress\my_callback".

This is the underlying issue that needs to be fixed.

7 weeks ago

#4 @dlh
7 weeks ago

Thanks, I see what you mean now. It looks to me like the preg_replace() might be the culprit. In 41488.2.diff, I tried to rearrange filter_wp_nav_menu() so the preg_replace() happens before injecting $customize_preview_nav_menus_args.

7 weeks ago

#5 @westonruter
7 weeks ago

@dlh you've found something very interesting!

Given preg_replace( '#^(.)#', '$1' . json_encode( '\hello\world' ), ' ' ) I would expect it to output:


But instead it outputs:


Compare this with str_replace( ' ', ' ' . json_encode( '\hello\world' ), ' ' ):


So yeah, it seems that not only does the pattern need escaping with PCRE but also the replacement, even when there are no capture groups!

By using preg_quote() on the replacement value, it also fixes the issue: 41488.3.diff.

This seems like a better approach, yes? I wonder how many other instances of preg_replace() have this problem in code I've written.

#6 @dlh
7 weeks ago

I wasn't sure about preg_quote() because it seems to affect a wider range of characters. For example, the data-customize-partial-id attribute becomes data\-customize\-partial\-id\ (as of PHP 5.3), and : in JSON becomes \:.

7 weeks ago

#7 @westonruter
7 weeks ago

@dlh hummm, you're right:

> echo preg_replace( '/(attr)/', '$1 ' . preg_quote( 'data-customize-partial-id' ) , 'attr' );
< attr data\-customize\-partial\-id

OK, it seems the answer is right in the docs for preg_replace: http://php.net/manual/en/function.preg-replace.php

To use backslash in replacement, it must be doubled.

I've replaced preg_quote() with str_replace() in 41488.4.diff.

#8 @dlh
7 weeks ago

Tested 41488.4.diff and it worked as expected.

#9 @westonruter
7 weeks ago

  • Keywords commit added
  • Owner set to westonruter
  • Status changed from new to accepted

This could be made part of 4.8.2.

#10 @westonruter
7 weeks ago

Added some test assertions to explicitly check for this issue: https://github.com/WordPress/wordpress-develop/pull/2

#11 @westonruter
7 weeks ago

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

In 41204:

Customize: For selective refresh of nav menus, prevent preg_replace() from dropping backslashes in JSON replaced into the data-customize-partial-placement-context HTML attribute.

Props dlh, westonruter.
Fixes #41488.

Note: See TracTickets for help on using tickets.