Make WordPress Core

Opened 2 years ago

Closed 21 months ago

Last modified 21 months ago

#57110 closed defect (bug) (fixed)

Correctly some attributes escape is missing in this ( wp-admin/includes/nav-menu.php ) file.

Reported by: zenaulislam's profile zenaulislam Owned by: sergeybiryukov's profile 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)

#2 @SergeyBiryukov
2 years ago

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

It looks like PR 3626 is submitted against the outdated master branch instead of trunk, so it shows 1,884 changed files and could use a refresh to be applied correctly. That said, this appears to be the relevant commit: afe28e0.

#3 @zenaulislam
2 years ago

Hello @SergeyBiryukov,

Thank you so much.
PR has been updated

Version 0, edited 2 years ago by zenaulislam (next)

#4 @zenaulislam
2 years ago

Hello Everyone,

Any update or feedback of my PR.

Kind Regards
Zenaul Islam

#5 follow-up: @SergeyBiryukov
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 in wp_nav_menu_item_taxonomy_meta_box() in a similar way.

This can be addressed on commit.

#6 @SergeyBiryukov
21 months ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 55615:

Coding Standards: Escape some variables in wp-admin/includes/nav-menu.php.

This ensures that post type or taxonomy name is consistently escaped in:

  • wp_nav_menu_item_post_type_meta_box()
  • wp_nav_menu_item_taxonomy_meta_box()

Follow-up to [14248], [23707].

Props zenaulislam, SergeyBiryukov.
Fixes #57110.

#7 in reply to: ↑ 5 @SergeyBiryukov
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 @jrf
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.

#9 @SergeyBiryukov
21 months ago

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

In 55616:

Coding Standards: Escape the whole attributes in wp-admin/includes/nav-menu.php.

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 hardcoded 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.

Includes:

  • Moving a few esc_url() calls closer to the actual output and escaping the hash parts too.
  • Wrapping a few long lines for better readability.

Follow-up to [14248], [23707], [42217], [55615].

Props jrf, SergeyBiryukov.
Fixes #57110.

@SergeyBiryukov commented on PR #3626:


21 months ago
#10

Thanks for the PR! Merged in r55615 and r55616.

Note: See TracTickets for help on using tickets.