Customize: uncaught JS error with namespaced nav menu fallbacks

[dlh]

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.

[westonruter]

@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?

[dlh]

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.

[westonruter]

@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

@@ -1271 +1271 @@
                 if ( isset( $args->customize_preview_nav_menus_args['can_partial_refresh'] ) && $args->customize_preview_nav_menus_args['can_partial_refresh'] ) {
                         $attributes = sprintf( ' data-customize-partial-id="%s"', esc_attr( 'nav_menu_instance[' . $args->customize_preview_nav_menus_args['args_hmac'] . ']' ) );
                         $attributes .= ' data-customize-partial-type="nav_menu_instance"';
-                        $attributes .= sprintf( ' data-customize-partial-placement-context="%s"', esc_attr( wp_json_encode( $args->customize_preview_nav_menus_args ) ) );
+                        $attributes .= sprintf( ' data-customize-partial-placement-context="%s"', wp_slash( esc_attr( wp_json_encode( $args->customize_preview_nav_menus_args ) ) ) );
                         $nav_menu_content = preg_replace( '#^(<\w+)#', '$1 ' . $attributes, $nav_menu_content, 1 );
                 }
                 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.

[dlh]

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.

[westonruter]

@dlh you've found something very interesting!

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

"\\hello\\world"

But instead it outputs:

"\hello\world"

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

"\\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.

[dlh]

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 \:.

[westonruter]

@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​.

[dlh]

Tested 41488.4.diff​ and it worked as expected.

[westonruter]

This could be made part of 4.8.2.

[westonruter]

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

[westonruter]

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.