Remove esc_url() and esc_url_raw() from list of escaping functions

[westonruter]

Is your feature request related to a problem?

Consider the following code (similar to what is found in core's WP_Scripts::do_item() method:

printf( "<script src='%s' id='%s-js'></script>\n", esc_url( $src ), esc_attr( $handle ) );

Looks good, right? (Ignoring WordPress.WP.EnqueuedResources.NonEnqueuedScript.) Actually, no. It turns out that plugins have been abusing esc_url() here to inject additional attributes. For example, WP Engine includes the following snippet in their article How To Defer Parsing of JavaScript in WordPress:

function defer_parsing_of_js ( $url ) {
    if ( FALSE === strpos( $url, '.js' ) ) return $url;
    if ( strpos( $url, 'jquery.js' ) ) return $url;
    return "$url' defer ";
}
add_filter( 'clean_url', 'defer_parsing_of_js', 11, 1 );

It turns out that before esc_url() function returns, it passes its return value through a clean_url filter. Plugins can take this filter and, assuming the context that the URL is going to be output in (including single- vs double-quoted attributes), and it can manipulate the value to introduce new attributes on the tag. Such hacks should not be allowed, as they are terribly brittle (and plain terrible).

This came up recently as well for the admin_body_class filter (Core-58336) where the lack of esc_attr() allows for additional attributes to be added to the body (as well as potential security vulnerabilities). I recall this was also a hack that used to be employed to add attributes to the body tag, by (ab)using the body_class filter (as well as adding attributes to div.post wrappers via post_class filter) which was stopped three years ago in Core-20009. See WordPress/wordpress-develop@d17a57a.

Describe the solution you'd like

Given that attribute injection hacks involving post_class, body_class, and (soon) admin_body_class were all put an end to in core, I think WPCS should perhaps consider doing the same for cases involving esc_url(). This can be achieved by removing esc_url() and esc_url_raw() from the list of escaping functions.

Therefore, the above snippet would need to be rewritten as in order to pass the WordPress.Security.EscapeOutput sniff:

printf( "<script src='%s' id='%s-js'></script>\n", esc_attr( esc_url_raw( $src ) ), esc_attr( $handle ) );

I suggest to remove these lines or set them to false:

WordPress-Coding-Standards/WordPress/Sniff.php

Lines 63 to 64 in fca9d9e

	'esc_url_raw' => true,
	'esc_url' => true,

[smileBeda]

How would we then escape an URL?
wp_redirect( $dynamic_url );?

esc_url and _raw are the way to do that. If these functions are removed from valid escaping functions, what would we then use?

[westonruter]

@smileBeda In this case, no escaping is involved since nothing is being printed. In your case, esc_url_raw() is correct to use. This function is actually misnamed; it is actually a sanitizing function and not an escaping function.

I'm only raising the issue for esc_url() being used directly for printing out in an HTML attribute. It should be wrapped in esc_attr() in this case.

[joemcgill]

I appreciate the intent of this issue, but currently disagree. As far as I can see, all escaping functions are passed through a filter prior to returning, so there's always the possibility that a plugin or theme is returning unescaped output by filtering the return value.

• esc_attr
• esc_html

If WP core wants to avoid this, we'd need to add some validation and cleanup to the filtered value or deprecate and remove the filters entirely.

[jrfnl]

I agree with @joemcgill and from a security perspective would be very much in favour of removing filters from these type of escaping functions. Unfortunately, I doubt such a change would be accepted in WP as it would be regarded as a BC break.