Make WordPress Core

Opened 21 months ago

Closed 16 months ago

Last modified 16 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
21 months 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
21 months ago

Hello @SergeyBiryukov,

Thank you so much.
PR request has been switched from the master to the trunk.

Last edited 21 months ago by zenaulislam (previous) (diff)

#4 @zenaulislam
19 months ago

Hello Everyone,

Any update or feedback of my PR.

Kind Regards
Zenaul Islam

#5 follow-up: @SergeyBiryukov
16 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
16 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
16 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
16 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
16 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:


16 months ago
#10

Thanks for the PR! Merged in r55615 and r55616.

Note: See TracTickets for help on using tickets.