#57541 closed defect (bug) (fixed)
Missing escaping in admin menu walker file
Reported by: | aniketpatel | Owned by: | audrasjb |
---|---|---|---|
Milestone: | 6.2 | Priority: | normal |
Severity: | minor | Version: | 3.0 |
Component: | Menus | Keywords: | has-patch commit |
Focuses: | Cc: |
Description (last modified by )
I found one escaping issue in class-walker-nav-menu-edit.php
admin file.
Screenshot: https://share.cleanshot.com/3Qwfq4XS16gHWH3LD3m9
Change History (12)
This ticket was mentioned in PR #3900 on WordPress/wordpress-develop by aniketpatel32.
22 months ago
#3
- Keywords has-patch added
#4
@
22 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
@
22 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:
22 months ago
#6
Thanks @aniketpatel32, PR look good to me.
@costdev I did like to get your feedback on this.
22 months ago
#8
Thanks for the ping @mukeshpanchal27!
The PR looks good to me, thanks @aniketpatel32!
#9
@
22 months ago
I agree @audrasjb. I've wondered a few times why we do this.
Here's what I can gather:
- This variable is set to
admin_url(...)
, which hasapply_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.
- 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.
- 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. 👍
@audrasjb commented on PR #3900:
22 months ago
#12
Committed in https://core.trac.wordpress.org/changeset/55135
Trac ticket: https://core.trac.wordpress.org/ticket/57541