Make WordPress Core

Opened 2 years ago

Closed 2 years ago

Last modified 2 years ago

#56128 closed defect (bug) (fixed)

Wrong escaping in 'class-wp-nav-menu-widget.php' file

Reported by: hztyfoon's profile hztyfoon Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.1 Priority: normal
Severity: normal Version: 4.3
Component: Widgets Keywords: has-patch
Focuses: administration, coding-standards Cc:


Hey everyone. 🙂
It's my first ticket & hopefully my first contribution to WordPress 'core'.

I have found a wrong URL escaping in the 'src/wp-includes/widgets/class-wp-nav-menu-widget.php' file.

Change History (6)

This ticket was mentioned in PR #2936 on WordPress/wordpress-develop by hz-tyfoon.

2 years ago

  • Keywords has-patch added

Trac ticket: 56128

#2 @SergeyBiryukov
2 years ago

  • Milestone changed from Awaiting Review to 6.1
  • Version changed from trunk to 4.3

Hi there, welcome to WordPress Trac! Thanks for the ticket.

Introduced in [33488] / #32814 for WordPress 4.3, setting the version accordingly.

It looks like esc_url() cannot be used here, as the URL can be a javascript: link, see line 169 above. Using esc_url() would turn that into an empty string.

That said, we should be able to add an inline comment to expain the esc_attr() usage.

#3 @SergeyBiryukov
2 years ago

I think something like this would work here, it follows the precedent of adding a similar comment in [53092]:

	/* translators: %s: URL to create a new menu. */
	__( 'No menus have been created yet. <a href="%s">Create some</a>.' ),
	// The URL can be a `javascript:` link, so esc_attr() is used here instead of esc_url().
	esc_attr( $url )

hz-tyfoon commented on PR #2936:

2 years ago

Sorry. Just updated the PR.
Looks like it wasn't "wrong escaping". So, added inline comment to explain why 'esc_attr()' used there instead of 'esc_url()'.
Thanks @SergeyBiryukov

#5 @SergeyBiryukov
2 years ago

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

In 53630:

Widgets: Add a comment in WP_Nav_Menu_Widget::form() to clarify the esc_attr() usage.

The URL to create a new menu from the the Navigation Menu widget can be a javascript: link to the Customizer Menus panel, so esc_attr() is used here instead of esc_url().

Follow-up to [53092].

Props hztyfoon.
Fixes #56128.

SergeyBiryukov commented on PR #2936:

2 years ago

Thanks for the PR! Merged in r53630.

Note: See TracTickets for help on using tickets.