Customizer Menus: Escaping inconsistencies

[swissspidy]

Some strange inconsistencies I noticed while testing the menu customizer today:

1. Add a new menu item, change its title to Test <><. In the menu, only Test < gets displayed.
2. Change the title to Test <=> Test and Test Test gets displayed.
3. Title Test followed by two back ticks gets turned into Test,,

Perhaps applies to the default menu screen as well, so it might not be that related to the customizer.

[celloexpressions]

I believe the changing here is done via jQuery's .text() function, so that would be the place to look first. Using .html() may fix these issues, but could under-escape things.

[westonruter]

Unfortunately the menus admin page “sanitizes” a menu's name via trim( esc_html( $name ) ), when sanitize_text_field() seems to be what should be used instead. Should we continue to emulate the existing behavior of the menus admin page, or should we improve/fix the sanitization logic for the admin page?

[valendesigns]

@westonruter Do you have any backwards compatibility concerns with changing the admin page?

[westonruter]

Replying to valendesigns:

@westonruter Do you have any backwards compatibility concerns with changing the admin page?

Not in particular, but I'm not sure if there would be any plugins that would be assuming the value is “sanitized” in this way.

[valendesigns]

Is there a lot of differences between the two sanitization methods?

[westonruter]

Yes. sanitize_text_field() will strip tags, whereas esc_html() turns them into HTML entities.

[valendesigns]

Yeah I thought so. The only concern I have would be that if someone added HTML to a menu name and expected it to display on the front end and it now doesn't. Like an icon or something. Is that possible to do with menus currently, I've never tried?

[westonruter]

I suppose if anyone _is_ doing that, they're exploiting a bug. Maybe we just fix the sanitization to strip tags and trim, as opposed to escape and trim, and apply this in JS and PHP on the admin page and the Customizer. If someone *is* trying to hack the menu name to include markup, they should expect this to hack to not be long for this world.

[jorbin]

Would this change result in content being lost upon save?

Milestoning so that a decision is made. If it needs to be punted, so be it, but I want to clear out awaiting review against trunk.

[westonruter]

@jorbin Any HTML tags would get used in the name would get stripped, yes.

[valendesigns]

Does anyone have preferences one way or the other? These are our options and they should be the same for both Admin & Customizer Menus.

1) Admin Menus trim( esc_html( $name ) )
2) Customizer Menus sanitize_text_field( $name )

If we were talking about menu item titles and not menu titles I would go with the first option. But since we're talking about menu titles we should probably go with the second option. There's really no go reason to allow HTML here. I can't think of a context in which the title of a menu should be allowed to have HTML in it. Can anyone else?

When we land on a decision I'll create a patch.

[celloexpressions]

For menu titles, I think sanitize_text_field would be the most appropriate. It's generally an administration-specific field, so allowing html doesn't seem necessary, or like a good idea.

[swissspidy]

Well, my report was for menu item titles. They should allow for escaped HTML entities.

sanitize_text_field is perfect for menu titles though.

[valendesigns]

Sorry, I was doing testing and menu items were working with valid HTML in them when I saved in the Admin Menu first. I assumed you were talking about menu titles at that point because I hit save in the Customizer, but stupidly didn't refresh.

After really reading this again, I think it's reasonable, and arguably more appropriate, to convert menu items for Customizer Menus and Admin Menus to sanitize_title. What we're typically adding is post objects, which allow for limited HTML.

I still believe that we should use sanitize_text_field for menu titles. It doesn't really make sense to allow HTML in this context.

[obenland]

No movement in over a week, let's fix this in a future release.

[valendesigns]

After further thought sanitize_title is the wrong escaping function, as well. Though I'm not sure we should push this out to the next release as it's destructive to the menu items when you hit save. This small issue may cause many tickets from confused users.

[anonymized_14235950]

#33612 was marked as a duplicate.

[jeremyfelt]

Ran into this today during our open lab and am passing along a +1 for allowing at least some HTML in menu item link text.

The example use case: Subscribe to <em>CAS Connect</em>

In which CAS Connect is italicized as part of the link text per APA style guidelines to reference the title of a publication.

Right now this works when in admin menus, but is stripped in the Customizer—and then the content.

[westonruter]

I think this one is just a matter of removing the title field from being sanitized, however uneasy it may seem to do so: 32812.diff​

[valendesigns]

Patch works. However, should we be concerned that we're completely removing sanitization or is the menu item being sanitized elsewhere during output?

[westonruter]

The sanitization relies on wp_update_nav_menu_item(), which itself relies on the sanitization in wp_insert_post(), which does:

<?php
$postarr = sanitize_post($postarr, 'db');

Which then then for the nav menu item's label, does:

<?php
$post['post_title'] = sanitize_post_field('post_title', $post['post_title'], $post['ID'], 'db');

This in turn will apply the filters:

• pre_the_title
• title_save_pre

The latter hook gets filtered by wp_filter_kses which is added by kses_init_filters(). Then in turn kses_init() is what calls this, but only if the user can't do unfiltered_html:

<?php
function kses_init() {
        kses_remove_filters();

        if ( ! current_user_can( 'unfiltered_html' ) ) {
                kses_init_filters();
        }
}

(Whew. What a path to go down to ensure something gets sanitized.)

Only admins can access the menus admin page because they have edit_theme_options. Additionally, only admins can have the unfiltered_html capability, and only on non-multisite installs (normally). Users who have unfiltered_html can add arbitrary HTML to post content as well, so allowing arbitrary HTML in menu titles isn't making it any less secure.

So, if you are an administrator user on a non-multisite install you can currently add a script tag to your nav menu item's label. Why would you want to do this? There should be no good reason. For multisite installs, the script tags get stripped out by kses.

In any case, it seems that by having menus in the Customizer just re-use wp_update_nav_menu_item() for sanitization of the title, then we just mirror the functionality of the menus admin page and how it sanitizes (or doesn't sanitize) the data.

@jorbin @jeremyfelt Are you confident in 32812.diff​?

[westonruter]

Refresh

[westonruter]

I realized that my proposed change in 32812.1.diff​ was flawed because it was skipping sanitization on title, excerpt, and content for a given nav_menu_item post during preview… but, when on multisite, or if the user does not have unfiltered_html capability, then they should still not be able to preview markup in these fields. The solution is just to emulate the behavior of wp_insert_post() by applying the title_save_pre, excerpt_save_pre, and content_save_pre filters. This would then just automatically do the right thing based on whether the user has unfiltered_html.

So I've corrected these issues in 32812.2.diff​, including a correction to how the menu item position and status were sanitized. The unit tests were also updated to cross-reference check of the sanitize method by actually saving the menu item and checking its saved results with what the sanitize method returns.

[westonruter]

32812.3.diff​ makes sure that the nav_menu_attr_title and nav_menu_description filters get applied, fixing an issue where HTML in the description can be previewed, but won't appear when saved and viewed outside the Customizer.

[westonruter]

I want to add some more tests.

[westonruter]

32812.4.diff​ adds unit tests for WP_Customize_Nav_Menu_Item_Setting::value_as_wp_post_nav_menu_item().

[westonruter]

In 35580:

Customize: Improve alignment of WP_Customize_Nav_Menu_Item_Setting::sanitize() behavior with wp_update_nav_menu_item().

• Apply title_save_pre, excerpt_save_pre, and content_save_pre filters on a nav menu item's title, attr_title, and description properties respectively. This ensures that arbitrary markup can be supplied if the user has unfiltered_html cap, and for these fields to have markup stripped if not.
• Ensure a nav menu item's post_status is sanitized as publish or draft using the same conditions as wp_update_nav_menu_item().
• Align WP_Customize_Nav_Menu_Item_Setting::sanitize() behavior for sanitizing position to be the same as wp_update_nav_menu_item().
• Also apply nav_menu_attr_title and nav_menu_description filters in WP_Customize_Nav_Menu_Item_Setting::value_as_wp_post_nav_menu_item() to ensure that previewing markup entered into menu item description will preview the same way as when the nav menu item is saved.
• Add unit tests.

Fixes #32812.

[westonruter]

Unit tests are broken. I'll fix.

[westonruter]

In 35583:

Customize: Fix broken unit test for WP_Customize_Nav_Menu_Item_Setting::value_as_wp_post_nav_menu_item().

Fixes issue in [35580] which caused unit tests to fail while run under multisite.

Fixes #32812.