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

[zenaulislam]

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.

Trac ticket: (https://core.trac.wordpress.org/ticket/57110)

[SergeyBiryukov]

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.

[zenaulislam]

Hello @SergeyBiryukov,

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

[zenaulislam]

Hello Everyone,

Any update or feedback of my PR.

Kind Regards
Zenaul Islam

[SergeyBiryukov]

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.

[SergeyBiryukov]

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.

[SergeyBiryukov]

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.

[jrf]

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

[SergeyBiryukov]

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.

Thanks for the PR! Merged in r55615 and r55616.