Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#54362 closed defect (bug) (fixed)

Wrong Escaping Function

Reported by: chintan1896's profile chintan1896 Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 5.9 Priority: normal
Severity: normal Version: 5.9
Component: Plugins Keywords: has-patch needs-refresh good-first-bug
Focuses: administration Cc:

Description

Wrong Escaping Function Was Used in Plugin install File.

Attachments (3)

54362.patch (947 bytes) - added by chintan1896 3 years ago.
54362.2.patch (879 bytes) - added by aezazshekh 3 years ago.
Whenever we use URL in the href, it should always be written in the escaping function
54362.3.patch (960 bytes) - added by chintan1896 3 years ago.
Updated Patch

Download all attachments as: .zip

Change History (11)

@chintan1896
3 years ago

#1 @Presskopp
3 years ago

doesn't look like an error to me. This is the translation function used on several places for the same string

wp-admin/freedoms.php:94
wp-admin/includes/plugin-install.php:273
wp-admin/includes/plugin-install.php:700
wp-admin/plugin-install.php:89
wp-admin/plugins.php:550

#2 @dimadin
3 years ago

__() is function used for translation. In this case, we are allowing translators to change URL (to point to different language version of https://wordpress.org/plugins/, for example https://de.wordpress.org/plugins/, https://sr.wordpress.org/plugins/...).

Output of __() should be escaped. There are some functions that merge translating and escaping functions (esc_attr__(), esc_html__()...).

#3 @henry.wright
3 years ago

  • Keywords 2nd-opinion dev-feedback added

The src attribute value should be escaped. I understand the need to allow translators to change the URL to a different language but a better approach would be to make the URL filterable.

My proposal is this

  1. Make the URL filterable and then
  2. Escape the src attribute value

@aezazshekh
3 years ago

Whenever we use URL in the href, it should always be written in the escaping function

#4 @SergeyBiryukov
3 years ago

  • Component changed from General to Plugins
  • Focuses administration added
  • Keywords needs-refresh added; 2nd-opinion dev-feedback removed
  • Milestone changed from Awaiting Review to 6.0

Thanks for the patch!

As noted above, we can add esc_url() here, but the __() call should not be removed to allow for the URL to be translated. So I think something like this should work here:

<?php echo esc_url( __( 'https://wordpress.org/plugins/' ) . $api->slug ); ?>

#5 @henry.wright
3 years ago

+1 to the approach suggested by @SergeyBiryukov

#6 @Presskopp
3 years ago

  • Keywords good-first-bug added

@chintan1896
3 years ago

Updated Patch

#7 @SergeyBiryukov
3 years ago

  • Milestone changed from 6.0 to 5.9

#8 @SergeyBiryukov
3 years ago

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

In 52419:

Plugins: Escape the WordPress.org plugin page URL in the Plugin Installation modal.

Follow-up to [8540], [38953].

Props chintan1896, Presskopp, dimadin, henry.wright, aezazshekh, SergeyBiryukov.
Fixes #54362.

Note: See TracTickets for help on using tickets.