Filter 'kses_allowed_protocols' is only applied once in function wp_allowed_protocols() & function esc_url() returns empty string;

[Anatta]

Files: wp-includes/functions.php: Lines:4665-4674 and wp-includes/formatting.php Lines: 2339 to 2342.

apply_filters( 'kses_allowed_protocols', $protocols ); is only called when wp_allowed_protocols() is first initiated. This makes it impossible to grant a temporary protocol exception.

I discovered this in a plugin trying to add a javascript link to the admin bar. Function esc_url() (wp-includes/formatting.php) would normally return blank for javascript:myFunction(/* code */);, however, I want to temporarily allow the javascript protocol between the wp_before_admin_bar_render and wp_after_admin_bar_render actions by applying a filter.

This is currently impossible because the other possible filter, clean_url is not fired in esc_url if wp_kses_bad_protocol() returns false. The only current option to allow javascript as a protocol on the admin bar is to add a permanent filter - not great for security should it become known that plugins with working javascript on the admin bar (e.g. a Facebook Connect logout function) must have the javascript protocol allowed site-wide.

The current wp_allowed_protocols() in functions.php:

function wp_allowed_protocols() {
	static $protocols;

	if ( empty( $protocols ) ) {
		$protocols = array( 'http', 'https', 'ftp', 'ftps', 'mailto', 'news', 'irc', 'gopher', 'nntp', 'feed', 'telnet', 'mms', 'rtsp', 'svn' );
		$protocols = apply_filters( 'kses_allowed_protocols', $protocols );
	}

	return $protocols;
}

Fix suggestion 1: Move the apply_filter outside the if ( empty( $protocols ) ) statement:

Patched wp_allowed_protocols()

function wp_allowed_protocols() {
	static $protocols;

	if ( empty( $protocols ) ) {
		$protocols = array( 'http', 'https', 'ftp', 'ftps', 'mailto', 'news', 'irc', 'gopher', 'nntp', 'feed', 'telnet', 'mms', 'rtsp', 'svn' );
	}

	$protocols = apply_filters( 'kses_allowed_protocols', $protocols );

	return $protocols;
}

Fix suggestion 2: Change esc_url() in wp-includes/formatting.php not to return directly, but to set $url to empty instead of directly returning an empty string, allowing $url to pass through the clean_url filter.

Patched esc_url() (Changed line 2342 from return to $url =)

function esc_url( $url, $protocols = null, $_context = 'display' ) {
	$original_url = $url;

	if ( '' == $url )
		return $url;
	$url = preg_replace('|[^a-z0-9-~+_.?#=!&;,/:%@$\|*\'()\\x80-\\xff]|i', '', $url);
	$strip = array('%0d', '%0a', '%0D', '%0A');
	$url = _deep_replace($strip, $url);
	$url = str_replace(';//', '://', $url);
	/* If the URL doesn't appear to contain a scheme, we
	 * presume it needs http:// appended (unless a relative
	 * link starting with /, # or ? or a php file).
	 */
	if ( strpos($url, ':') === false && ! in_array( $url[0], array( '/', '#', '?' ) ) &&
		! preg_match('/^[a-z0-9-]+?\.php/i', $url) )
		$url = 'http://' . $url;

	// Replace ampersands and single quotes only when displaying.
	if ( 'display' == $_context ) {
		$url = wp_kses_normalize_entities( $url );
		$url = str_replace( '&', '&', $url );
		$url = str_replace( "'", ''', $url );
	}

	if ( ! is_array( $protocols ) )
		$protocols = wp_allowed_protocols();
	if ( wp_kses_bad_protocol( $url, $protocols ) != $url )
		$url = '';

	return apply_filters('clean_url', $url, $original_url, $_context);
}

One of both of these minor patches before 3.3 final would be greatly appreciated.

[duck_]

First off you can easily fix this by using the second argument, $protocols, of esc_url() in your call to it. This allows you to completely bypass wp_allowed_protocols().

esc_url( $url, array( 'javascript' ) )

The single call to apply_filters() was intentional, see #18268, for performance reasons. Also, it was impossible to filter the array of allowed protocols in esc_url() prior to 3.3 anyway, so no regression in your specific use case.

P.S. depending on your code you might want to look into esc_js() and not just esc_url().

[Anatta]

Patches for wp-includes/functions.php and wp-includes/formatting.php

[Anatta]

@duck I agree for custom calls but in this case the esc_url() call is being made during the rendering of the admin-bar, modifying the esc_url() call in this case is not possible without modifying wp-includes/class-wp-admin-bar.php. There are no other actions or filters between the wp_before_admin_bar_render and wp_after_admin_bar_render hooks that can be used to enable a javascript action on an admin-bar link.

Given the prominence and focus of the new admin bar, I can imagine increased instances of developers wishing to add more functionality to it. Given that the only workaround is currently to globally enable the javascript protocol, any plugin with admin-bar javascript would be advertising a vulnerability.

Either a patch to allow more targeted filtering of wp_allowed_protocols(), or amendments to allow targeted exceptions for the admin bar (such as appending allowed protocols to the admin_bar hooks) seem justified.

[duck_]

Replying to Anatta:

@duck I agree for custom calls but in this case the esc_url() call is being made during the rendering of the admin-bar, modifying the esc_url() call in this case is not possible without modifying wp-includes/class-wp-admin-bar.php.

Ah sorry, I misunderstood your usage there.

[nacin]

It's helpful to report problems rather than solutions. esc_url() is a fix, sure, but there's a better way to solve your problem: There's a 'meta' argument for add_node() that allows for an 'onclick' argument. Example:

$wp_admin_bar->add_node( array(
   'id' => 'some-id',
   'title' => 'some-title',
   'parent' => 'some-parent',
   'meta' => array( 'onclick' => 'return;' ),
) );

[Anatta]

Replying to nacin:

There's a 'meta' argument for add_node() that allows for an 'onclick' argument.

Thanks nacin; code is indeed poetry - there's just a lot of it to read!, I got caught up tracing why my filtered logout url was not being passed to the my-account admin bar menu and didn't consider the simpler solution of replacing the menu item.

For anyone googling similar symptoms, this ticket should serve as a helpful resource. I've successfully added Facebook javascript to the admin bar logout with the following:

add_action( 'wp_before_admin_bar_render', 'fb_admin_bar_logout');

function fb_admin_bar_logout() {
	
	global $wp_admin_bar
	
	$wp_admin_bar->add_menu( array(
			'parent' => 'my-account',
			'id'     => 'logout',
			'title'  => __( 'Logout' ),
			'href'   => wp_logout_url(), 
			'meta' => array( 'onclick' => 'FB.logout();'
		) ); 
}

[nacin]

All you'll probably need is array( 'id' => 'logout', 'meta' => array( 'onclick' => 'FB.logout();' ) ).