Make WordPress Core

Opened 16 months ago

Closed 16 months ago

Last modified 16 months ago

#57541 closed defect (bug) (fixed)

Missing escaping in admin menu walker file

Reported by: aniketpatel's profile aniketpatel Owned by: audrasjb's profile audrasjb
Milestone: 6.2 Priority: normal
Severity: minor Version: 3.0
Component: Menus Keywords: has-patch commit
Focuses: Cc:

Description (last modified by mukesh27)

I found one escaping issue in class-walker-nav-menu-edit.php admin file.

Screenshot: https://share.cleanshot.com/3Qwfq4XS16gHWH3LD3m9

Change History (12)

#1 @mukesh27
16 months ago

  • Description modified (diff)

#2 @mukesh27
16 months ago

  • Description modified (diff)

This ticket was mentioned in PR #3900 on WordPress/wordpress-develop by aniketpatel32.


16 months ago
#3

  • Keywords has-patch added

#4 @mukesh27
16 months ago

  • Component changed from General to Menus
  • Milestone changed from Awaiting Review to 6.2
  • Version set to 3.0

Thanks @aniketpatel for the ticket and PR!

Moving to 6.2 milestone for visibility.

#5 @audrasjb
16 months ago

  • Owner set to audrasjb
  • Severity changed from normal to minor
  • Status changed from new to reviewing

As $edit_url comes from admin_url() I'm unsure we can get anything else than an URL here, but it looks like a pattern used in some other places so let's consider this.

@mukesh27 commented on PR #3900:


16 months ago
#6

Thanks @aniketpatel32, PR look good to me.

@costdev I did like to get your feedback on this.

#7 @mukesh27
16 months ago

Thanks @audrasjb for taking ownership.

@costdev commented on PR #3900:


16 months ago
#8

Thanks for the ping @mukeshpanchal27!

The PR looks good to me, thanks @aniketpatel32!

#9 @costdev
16 months ago

I agree @audrasjb. I've wondered a few times why we do this.

Here's what I can gather:

  1. This variable is set to admin_url(...), which has apply_filters() as the final return value. Extenders could modify the URL with something unsafe, but if they're filtering, then they already have access to do a lot worse. Like cleaning out the fridge in case a burglar poisons the food.
  1. However, the description of esc_url() states:

eliminates invalid characters and removes dangerous characters

So, even though esc_url() is mainly used for security, this also helps when extenders return a value that has invalid characters. I guess this is one reason why the pattern exists elsewhere in Core.

  1. We tell extenders to sanitize all URLs before output, so maybe this pattern also exists to encourage this.

Given (2) and (3), it makes sense to continue this pattern.

The PR looks good to me. 👍

#10 @audrasjb
16 months ago

  • Keywords commit added

I agree with both points of your comment Colin 👍

#11 @audrasjb
16 months ago

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

In 55135:

Menus: Add missing escaping function in Admin Menu walker.

Props aniketpatel, mukesh27, costdev.
Fixes #57541.

Note: See TracTickets for help on using tickets.