Make WordPress Core

Opened 4 weeks ago

Closed 3 weeks ago

Last modified 10 days ago

#63351 closed defect (bug) (fixed)

Incorrect usage of esc_attr() for URL escaping

Reported by: hardik2221's profile hardik2221 Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.9 Priority: normal
Severity: normal Version:
Component: Menus Keywords: has-patch has-test-info
Focuses: administration, coding-standards Cc:

Description

File: src/wp-admin/includes/class-walker-nav-menu-checklist.php
URL is being escaped using esc_attr() instead of esc_url()

Attachments (2)

63351.patch (3.6 KB) - added by hardik2221 4 weeks ago.
63351.2.patch (1.9 KB) - added by nareshbheda 3 weeks ago.
Patch

Download all attachments as: .zip

Change History (9)

@hardik2221
4 weeks ago

#1 @nareshbheda
3 weeks ago

Same issue found in another file: src/wp-admin/includes/class-walker-nav-menu-edit.php

Last edited 3 weeks ago by nareshbheda (previous) (diff)

@nareshbheda
3 weeks ago

Patch

#2 @dilipbheda
3 weeks ago

  • Focuses administration added
  • Keywords has-testing-info added

Test Report

Description

I tested both patches, and they seem to be working well with the following test scenarios:

  1. A valid HTTP URL
  2. A # symbol and an empty string
  3. A #target_element_id

Patch tested:
1) https://core.trac.wordpress.org/attachment/ticket/63351/63351.2.patch
2) https://core.trac.wordpress.org/attachment/ticket/63351/63351.patch

Environment

  • WordPress: 6.9-alpha-60093-src
  • PHP: 8.2.27
  • Server: nginx/1.27.3
  • Database: mysqli (Server: 8.0.40 / Client: mysqlnd 8.2.27)
  • Browser: Chrome 135.0.0.0
  • OS: Linux
  • Theme: Twenty Twenty-One 2.5
  • MU Plugins: None activated
  • Plugins:
    • Test Reports 1.2.0

Actual Results

  1. ✅ Issue resolved with patch.

#3 @SirLouen
3 weeks ago

@hardik2221 @nareshbheda just in case you like to keep sorting this kind of PHPCS issues, its a good thing to know that for these kind of tasks that doesn't provide a big change in the code, there are what they call "Blessed tasks". Every new cycle they open a new task for many things like these.

For 6.9 this is the task for PHPCS: #63168

Once you submit the patch you can post there to inform, and they will be reviewing and committing the new changes

@dilipbheda no big need to test PHPCS patches. They are too trivial. You can test them if you like, but there are many other patches that need priority testing (See for example: #63151 #63316 #63135). If you want to know more about priority testing patches, ping me at slack (same username)

#4 @dilipbheda
3 weeks ago

@dilipbheda no big need to test PHPCS patches. They are too trivial. You can test them if you like, but there are many other patches that need priority testing (See for example: #63151 #63316 #63135). If you want to know more about priority testing patches, ping me at slack (same username)

@SirLouen Thanks for sharing the priority tickets. I’ll take a look at them.

#5 @SergeyBiryukov
3 weeks ago

  • Milestone changed from Awaiting Review to 6.9

#6 @SergeyBiryukov
3 weeks ago

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

In 60213:

Coding Standards: Use correct escaping function for nav menu item URLs.

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

Props hardik2221, nareshbheda, dilipbheda, SirLouen.
Fixes #63351.

#7 @wordpressdotorg
10 days ago

  • Keywords has-test-info added; has-testing-info removed
Note: See TracTickets for help on using tickets.