WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 4 years ago

#32812 closed defect (bug) (fixed)

Customizer Menus: Escaping inconsistencies

Reported by: swissspidy Owned by: westonruter
Milestone: 4.4 Priority: normal
Severity: normal Version: 4.3
Component: Customize Keywords: has-patch
Focuses: Cc:

Description

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.

Attachments (5)

32812.diff (726 bytes) - added by westonruter 4 years ago.
32812.1.diff (820 bytes) - added by westonruter 4 years ago.
Refresh
32812.2.diff (7.4 KB) - added by westonruter 4 years ago.
32812.3.diff (7.8 KB) - added by westonruter 4 years ago.
32812.4.diff (10.0 KB) - added by westonruter 4 years ago.

Download all attachments as: .zip

Change History (36)

#1 @celloexpressions
5 years ago

  • Keywords needs-patch added
  • Version set to trunk

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.

#2 @westonruter
5 years ago

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?

#3 follow-up: @valendesigns
5 years ago

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

#4 in reply to: ↑ 3 @westonruter
5 years ago

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.

#5 @valendesigns
5 years ago

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

#6 @westonruter
5 years ago

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

#7 @valendesigns
5 years ago

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?

#8 @westonruter
5 years ago

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.

#9 @jorbin
5 years ago

  • Milestone changed from Awaiting Review to 4.3

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.

#10 @westonruter
5 years ago

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

#11 @valendesigns
5 years ago

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

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.

#12 @celloexpressions
5 years ago

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.

#13 @swissspidy
5 years ago

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

sanitize_text_field is perfect for menu titles though.

#14 @valendesigns
5 years ago

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.

#15 @obenland
5 years ago

  • Milestone changed from 4.3 to Future Release

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

#16 @valendesigns
5 years ago

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.

#17 @freetheweb
5 years ago

#33612 was marked as a duplicate.

#18 @jeremyfelt
4 years ago

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
4 years ago

#19 @westonruter
4 years ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Future Release to 4.4

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

#20 @valendesigns
4 years ago

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

#21 @westonruter
4 years ago

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?

#22 @wonderboymusic
4 years ago

  • Owner changed from valendesigns to westonruter

#23 @westonruter
4 years ago

  • Keywords needs-refresh added

@westonruter
4 years ago

Refresh

#24 @westonruter
4 years ago

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

@westonruter
4 years ago

#25 @westonruter
4 years ago

  • Keywords commit added; needs-unit-tests removed

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
4 years ago

#26 @westonruter
4 years ago

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.

#27 @westonruter
4 years ago

  • Keywords needs-unit-tests added; commit removed

I want to add some more tests.

@westonruter
4 years ago

#28 @westonruter
4 years ago

  • Keywords needs-unit-tests removed

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

#29 @westonruter
4 years ago

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

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.

#30 @westonruter
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Unit tests are broken. I'll fix.

#31 @westonruter
4 years ago

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

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.

Note: See TracTickets for help on using tickets.