Opened 2 years ago

Last modified 13 months ago

#17821 new defect (bug)

wp_get_nav_menu_object() doesn't check when passing to get_term()

Reported by: kawauso Owned by:
Priority: normal Milestone: Future Release
Component: Menus Version: 3.1.4
Severity: normal Keywords: has-patch
Cc:

Description

wp_get_nav_menu_object() initially passes the menu string to get_term() to see if it's an ID. However, get_term() expects an integer or object and type-casts any non-objects passed into integers. This results in a menu name i.e. '10-ton elephant' being reduced to '10', which it then uses as a term ID, potentially producing unexpected results.

Attachments (3)

17821.diff (636 bytes) - added by kawauso 2 years ago.
17821.2.diff (1.7 KB) - added by kawauso 2 years ago.
17821.3.diff (1.7 KB) - added by kawauso 2 years ago.
s/menu_obj/menu

Download all attachments as: .zip

Change History (13)

kawauso2 years ago

Patch sets $menu_obj to false initially and removes the redundant end false check (get_term_by() will return an object or false, if get_term() has passed null then it causes a get_term_by() call). Also adds an is_numeric() check to get_term().

Steps to reproduce:

  1. Add a nav menu and find its term ID
  2. Call wp_get_nav_menu_object() with a string beginning with that ID
  3. The nav menu will be returned despite the passed string not being a valid menu name

comment:3 follow-up: ↓ 6   kawauso2 years ago

wp_get_nav_menu_to_edit() incorrectly passed the menu object to is_nav_menu() (it is only meant to take an id, slug or name). Following patch adds check logic from is_nav_menu() in place of the is_nav_menu() call.

wp_get_nav_menu_to_edit() can still return null when the conditions are not met, so the PHPDoc @return should be expanded to reflect this or handling added to convert null values into WP_Error objects.

kawauso2 years ago

kawauso2 years ago

s/menu_obj/menu

comment:4 follow-up: ↓ 5   cburtbcit20 months ago

I can confirm that I have run into this bug as well. The diff provided appears to solve the problem. I don't think it's that unlikely that someone would name a page with an integer as the first character or two in the title so in my opinion it's important that this patch be included in the next release of WordPress. I see that the version number for the bug is listed as 3.1.4 but I'm experiencing this in 3.2.1 as well. Thank you!

comment:5 in reply to: ↑ 4   kawauso20 months ago

  • Milestone changed from Awaiting Review to 3.3

Replying to cburtbcit:
The Version refers to the earliest version affected by the bug.

comment:6 in reply to: ↑ 3   duck_18 months ago

Replying to kawauso:

wp_get_nav_menu_to_edit() incorrectly passed the menu object to is_nav_menu() (it is only meant to take an id, slug or name). Following patch adds check logic from is_nav_menu() in place of the is_nav_menu() call.

Although passing an object was undocumented should we be careful about backwards compatibility here? As you say even core passed an object to is_nav_menu() / wp_get_nav_menu_object().

  • Milestone changed from 3.3 to Future Release

I've recently had to apply the patch provided by kawauso once again. It still works and does not seem to produce any unexpected behavior. What's keeping this from being included in the next update? Thanks!

  • Milestone changed from Future Release to 3.4
  • Milestone changed from 3.4 to Future Release
Note: See TracTickets for help on using tickets.