Make WordPress Core

Opened 20 months ago

Closed 19 months ago

Last modified 19 months ago

#57042 closed defect (bug) (wontfix)

URL escaping added in 'about.php' file

Reported by: monzuralam's profile monzuralam Owned by:
Milestone: Priority: normal
Severity: normal Version: 6.1
Component: Help/About Keywords: has-patch close
Focuses: administration, coding-standards Cc:

Description

File path: src/wp-admin/about.php

Here URL escaping is missing ( Line: 519, 530 ) without any escaping.

But in the same file use, proper escaping is used ( Line: 219 ) esc_url is used to escape the URL properly.

We need to do the same too.

Thanks.

Change History (8)

#1 @mukesh27
20 months ago

  • Component changed from General to Help/About

This ticket was mentioned in PR #3590 on WordPress/wordpress-develop by @monzuralam.


20 months ago
#2

  • Keywords has-patch added; needs-patch removed

Trac ticket: 57042

#3 @audrasjb
19 months ago

  • Milestone changed from Awaiting Review to 6.1.1

Moving this to milestone 6.1.1 since it wouldn't make much sense to fix it in the next major as the about page content will be replaced.

Not sure this is something worth to be fixed in a minor, though, as polyglots who can edit those links are trusted contributors (they have General Translation Editor permissions). But it wouldn't hurt :)

#4 @audrasjb
19 months ago

I also added a question in the above PR.

#5 @desrosj
19 months ago

  • Keywords close added

Changes have been requested on the pull request. But, it looks like it's fairly mixed throughout the code base for passing translated URLs through esc_url().

  • There are currently 122 instances of __( 'https:// in Core (excluding bundled themes).
  • 24 of them are passed through esc_url().

My recommended course of action is:

  • Closing this as a wontfix as the current code matches the majority of occurrences in Core today.
  • Opening a new ticket to confirm the correct way to handle these scenarios (which I am assuming will follow the "Core translations are trusted" policy)
  • Use that new ticket to correct all instances at once.

#6 @audrasjb
19 months ago

+1, that totally make sense to me.

#7 @peterwilsoncc
19 months ago

  • Milestone 6.1.1 deleted
  • Resolution set to wontfix
  • Status changed from new to closed

I agree this can be closed without a fix.

While the intent is good, the escaping functions are only needed for user input. As the URL is hard coded in this instance, escaping isn't required.

It's a nuanced point and I realise that the inconsistently in WordPress contributes to the confusion, so I really appreciate your taking the time to open the ticket @monzuralam

@desrosj commented on PR #3590:


19 months ago
#8

Closing as the Trac ticket this was meant to address has been closed as wontfix.

Note: See TracTickets for help on using tickets.