Make WordPress Core

Opened 12 months ago

Closed 11 months ago

Last modified 11 months ago

#56007 closed defect (bug) (fixed)

Correct the escaping in documentation lookup for plugin and theme editor

Reported by: sergeybiryukov's profile SergeyBiryukov Owned by: sergeybiryukov's profile SergeyBiryukov
Milestone: 6.1 Priority: normal
Severity: normal Version:
Component: Administration Keywords: has-patch commit
Focuses: Cc:

Description

There is some similar code for documentation lookup in plugin and theme code editors.

wp-admin/plugin-editor.php:

if ( '.php' === substr( $real_file, strrpos( $real_file, '.' ) ) ) {
	$functions = wp_doc_link_parse( $content );

	if ( ! empty( $functions ) ) {
		$docs_select  = '<select name="docs-list" id="docs-list">';
		$docs_select .= '<option value="">' . __( 'Function Name&hellip;' ) . '</option>';
		foreach ( $functions as $function ) {
			$docs_select .= '<option value="' . esc_attr( $function ) . '">' . esc_html( $function ) . '()</option>';
		}
		$docs_select .= '</select>';
	}
}

wp-admin/theme-editor.php:

if ( '.php' === substr( $file, strrpos( $file, '.' ) ) ) {
	$functions = wp_doc_link_parse( $content );

	$docs_select  = '<select name="docs-list" id="docs-list">';
	$docs_select .= '<option value="">' . esc_attr__( 'Function Name&hellip;' ) . '</option>';
	foreach ( $functions as $function ) {
		$docs_select .= '<option value="' . esc_attr( urlencode( $function ) ) . '">' . htmlspecialchars( $function ) . '()</option>';
	}
	$docs_select .= '</select>';
}

How many differences can you find? :) Over the years, both of these fragments evolved in slightly different ways:

  • [10607] / #9184 introduced documentation lookup shortcuts both for the plugin and theme editor.
  • [10879] / #9452 replaced urlencode() with attribute_escape(), later esc_attr(), in the plugin editor.
  • [11110] / #9650 added _a(), later esc_attr__(), for "Function Name..." in the theme editor.
  • [11173] added attribute escaping for $function in the theme editor, without removing urlencode().
  • [11204] / #9650 replaced attr() with esc_attr() and _a() with esc_attr__().
  • [11671] / #10262 added a ! empty( $functions ) check for the plugin editor.
  • [14989] replaced htmlspecialchars() with esc_html() in the plugin editor.

The attached patch brings some consistency:

  • The ! empty( $functions ) check only existed in the plugin editor, it should be added to the theme editor too.
  • "Function Name..." is an option label, not an attribute, so esc_html__() would be the correct function here.
  • esc_attr( urlencode( $function ) ) in the theme editor is redundant, just esc_attr() is enough there.
  • htmlspecialchars( $function ) in the theme editor can be replaced with esc_html( $function ).

Noticed and patched in a working session on #53465 with @aristath, @justinahinon, @poena, and @SergeyBiryukov.

Attachments (1)

56007.diff (1.8 KB) - added by SergeyBiryukov 12 months ago.

Download all attachments as: .zip

Change History (6)

#1 @audrasjb
12 months ago

  • Keywords commit added

Looks good to me! Thank you for addressing these inconsistencies @SergeyBiryukov.
Marking for commit.

This ticket was mentioned in PR #2998 on WordPress/wordpress-develop by pratiweb.


11 months ago
#2

Created pull request with correction needed in core for escaping

Trac ticket: https://core.trac.wordpress.org/ticket/56007

#3 @audrasjb
11 months ago

@pratiweb I can't see any difference between your PR and the previous patch (56007.diff, proposed 5 weeks ago in this ticket)?

(also it looks like your PR has several coding standards issues)

#4 @SergeyBiryukov
11 months ago

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

In 53758:

Administration: Correct the escaping in documentation lookup for plugin and theme editor.

This brings consistency to similar code fragments that evolved in slightly different ways over the years:

  • A check that the list of functions is not empty now exists in both editors.
  • "Function Name..." is an option label, not an attribute, so esc_html__() is the correct function here.
  • esc_attr( urlencode( $function ) ) in the theme editor is replaced with esc_attr( $function ).
  • htmlspecialchars( $function ) in the theme editor is replaced with esc_html( $function ).

Follow-up to [10607], [10879], [11110], [11173], [11204], [11671], [14989].

Props aristath, justinahinon, poena, audrasjb, pratiweb, SergeyBiryukov.
Fixes #56007.

SergeyBiryukov commented on PR #2998:


11 months ago
#5

Thanks for the PR! Merged in r53758.

Note: See TracTickets for help on using tickets.