#57110 closed defect (bug) (fixed)
Correctly some attributes escape is missing in this ( wp-admin/includes/nav-menu.php ) file.
Reported by: | zenaulislam | Owned by: | SergeyBiryukov |
---|---|---|---|
Milestone: | 6.3 | Priority: | normal |
Severity: | normal | Version: | |
Component: | Menus | Keywords: | has-patch |
Focuses: | administration, coding-standards | Cc: |
Description
I found some missing escape attributes inside this file 'wp-admin/includes/nav-menu.php'
I think we can improve it by escaping the attributes for more consistency.
Change History (10)
This ticket was mentioned in PR #3626 on WordPress/wordpress-develop by @zenaulislam.
2 years ago
#1
#3
@
2 years ago
Hello @SergeyBiryukov,
Thank you so much.
PR has been updated
#5
follow-up:
↓ 7
@
21 months ago
- Component changed from Administration to Menus
- Focuses administration added
- Milestone changed from Awaiting Review to 6.3
Thanks for the PR! It looks good, I only have two minor notes:
- It would be more consistent with some existing instances to only escape the variable, e.g.
<div id="posttype-<?php echo esc_attr( $post_type_name ); ?>" class="posttypediv">
instead of:<div id="<?php echo esc_attr( 'posttype-' . $post_type_name ); ?>" class="posttypediv">
- We should also escape
$taxonomy_name
inwp_nav_menu_item_taxonomy_meta_box()
in a similar way.
This can be addressed on commit.
#6
@
21 months ago
- Owner set to SergeyBiryukov
- Resolution set to fixed
- Status changed from new to closed
In 55615:
#7
in reply to:
↑ 5
@
21 months ago
- Resolution fixed deleted
- Status changed from closed to reopened
Replying to SergeyBiryukov:
It would be more consistent with some existing instances to only escape the variable, e.g.
<div id="posttype-<?php echo esc_attr( $post_type_name ); ?>" class="posttypediv">instead of:
<div id="<?php echo esc_attr( 'posttype-' . $post_type_name ); ?>" class="posttypediv">
Ah, it looks like I missed Juliette's comment here that the latter is actually best practice:
Here and in other places: it is best to always escape the complete value of an attribute, not a partial value, as otherwise the escaping could be (partially) undone when the values are joined together.
While the hard-coded prefix/suffix values in this case don't necessarily create that risk, those may change to values which could be problematic, so making it a habit to escape the value in one go is best practice.
#8
@
21 months ago
@SergeyBiryukov Happens to the best of us ;-)
Escaping - especially for attributes and URLs and such - should always try to escape the complete value in one go as otherwise there is still a security risk.
Not a big risk in this particular case, but better to make it a habit anyway.
Trac ticket: (https://core.trac.wordpress.org/ticket/57110)