do_shortcode('<[shortcode]') doesn't work

[Kleor]

Shortcodes preceded by a < are not parsed anymore. It's a side effect of the do_shortcodes_in_html_tags function. Lines 353-355 of the wp-includes/shortcodes.php file:

if ( '<' !== $element[0] ) {
   continue;
}

[miqrogroove]

Could you provide an example of the bug? I'm not sure why we would need do_shortcode('<[shortcode]') but it might be something we just didn't anticipate.

[Kleor]

Email

[Kleor]

See the Sender field in the image in attachment:

[sender first-name] [sender last-name] <[sender email-address]>

With WordPress 4.2.3, users of Contact Manager can't know the email addresses of the people who sent them a message, and of course they can't respond to them.

Several plugins also use shortcodes that are immediately preceded by <.

[miqrogroove]

Attached patch file should be reviewed for 4.2.4. I can't promise anything more than that though.

[markjaquith]

Should we support running shortcodes in non-HTML contexts like this?

[knutsp]

Replying to markjaquith:

Should we support running shortcodes in non-HTML contexts like this?

I would prefer to call it non-web content/context, as emails and other contexts may be HTML, too.

Is it a real opportunity not to without deprecating do_shortcode()?

Anyway, is shortcodes become very limited, plugin developers will have to revert to their own templating system, or clone the shortcode functions and removing the strict limitations. This may not enhance safety, if that is the concern here.

I would like Core to offer a safe, standardized and flexible way of substituting certain patterns in all kinds of content, but with clearly documented (and stable) limitations. This is where shortcodes come in as very handy, and hopefully trustworthy.

It seems the use case in this ticket demonstrates that allowing a < in front of the shortcode should be allowed. I don't know, or understand, the nature of the vulnerability with the shortcode API i 4.2.3, but I hope the above patch doesn't reintroduce something dangerous.

Another concern may be maintainability. I think that when the shortcode API was introduced in such a general way, very liberal, too liberal maybe, Core has no choice without steering up a lot of noise and then desperate workarounds.

The shortcode API is just too useful, and too much used now, to put strict constrains on, if not absolutely necessary.

If I don't know what I'm talking about, please ignore.

[miqrogroove]

Replying to markjaquith:

Should we support running shortcodes in non-HTML contexts like this?

It's a valid question. This one case is so specific that we know we can do it safely. Should we do it? Probably not, but backward compatibility is a big concern on the branch versions.

[knutsp]

Replying to miqrogroove:

Should we do it? Probably not, but backward compatibility is a big concern on the branch versions.

Why not? If there is a good reason why not, then this is a wontfix.

[Kleor]

I totally agree with knutsp. Moreover, we always tell plugin/theme developers to rely on WordPress functions instead of creating their own, so we can't reproach them to use the Shortcode API of WordPress instead of creating their own tag system when they need one.

[miqrogroove]

Replying to knutsp:

Replying to miqrogroove:

Should we do it? Probably not, but backward compatibility is a big concern on the branch versions.

Why not? If there is a good reason why not, then this is a wontfix.

If we had an easy choice about whether or not to encourage plugins to use <[sender email-address]> style and run do_shortcode in email headers, I think the answer would be "no". It's difficult to parse, even more difficult to secure in this version, and we can't allow it in many situations anymore. Especially in a context that is not related to post content, shortcodes are not even needed. One could more easily use str_replace and have an arbitrary template like ##sender-email##. It would run faster, not be constrained by Shortcode filters, and give the plugin full control of the output.

So when I say backward compatibility is a big concern, I mean it might be the only reason to fix this.

[knutsp]

Replying to miqrogroove:

If we had an easy choice about whether or not to encourage plugins to use <[sender email-address]> style and run do_shortcode in email headers, I think the answer would be "no". It's difficult to parse, even more difficult to secure in this version, and we can't allow it in many situations anymore. Especially in a context that is not related to post content, shortcodes are not even needed. One could more easily use str_replace and have an arbitrary template like ##sender-email##. It would run faster, not be constrained by Shortcode filters, and give the plugin full control of the output.

So when I say backward compatibility is a big concern, I mean it might be the only reason to fix this.

Thank you. Great answer. Do you see problems with plugins having to revert to using str_replace() and arbitrary placeholder formats? I mean, not just just for backwards compatiblity, but forward looking?

[miqrogroove]

Replying to knutsp:

Thank you. Great answer. Do you see problems with plugins having to revert to using str_replace() and arbitrary placeholder formats? I mean, not just just for backwards compatiblity, but forward looking?

That discussion is ongoing in terms of feature support. It's something we would prefer to look at several versions into the future when it doesn't involve security patches.

[azaozz]

Replying to miqrogroove:

Right, it will be much easier for plugins to handle this themselves. They know what to expect and can sanitize it much better.

That discussion is ongoing in terms of feature support.

Lets move this to "Future Release" pending the outcome of that discussion.

[miqrogroove]

We had a misunderstanding there. See Slack. Restoring ticket.

[azaozz]

Yeah, this is a regression fix for 4.2.4, but will probably not be in 4.3.

[miqrogroove]

@azaozz It would also be possible to slip a user notice or deprecated message inside that block for 4.3 and plan to break it in 4.4 or 4.5. Since we are close to RC, let's try to decide which way to go on that.

[samuelsidler]

We should fix this in 4.2.4, as mentioned, but we should not fix this in 4.3. Plugin authors will have ~3 weeks to update for this change.

[miqrogroove]

New patches refreshed for a 4.2 branch fix, and added _doing_it_wrong() to trunk, specifying 4.4 as the version that will officially break these weird shortcodes.

[helen]

Let's not call them weird things even in code comments :) Shortcodes are overkill for string token replacements, but it's not really all that weird of a jump to make, especially for devs who are accustomed to looking for a WordPress API first and might not (yet) know about things that likely should be done in raw PHP.

I agree with fixing this in branches but not in trunk and calling this unsupported for 4.3. That means getting a post with the what, why, and how to fix (and how to think about approaching this type of usage) written now.

[miqrogroove]

Refreshed both patches without the word "weird".

I still think we do not have a good reason for breaking this feature in 4.3. We can safely remove support after 4.3 is branched from trunk.

[azaozz]

In 33499:

Fix do_shortcode('<[shortcode]') edge case.
Props miqrogroove. Fixes #33116 for 4.2.

[miqrogroove]

@azaozz Do we need to backport this to other branches?

Trunk patch still needs commit per chat.

[samuelsidler]

Replying to miqrogroove:

Trunk patch still needs commit per chat.

Per the devchat today, we are not taking this in 4.3 RC1. We'll spend more time considering the consequences of this patch and any edge cases that we might need to support / run into and consider it for any subsequent RC.

[Kleor]

If the patch done by miqrogroove doesn't reintroduce something dangerous, is there any good reason to remove it in a future version of WordPress?

do_shortcode is actually fast, even if it's not as fast as str_replace. Plugins/themes developers can prefer to use the Shortcode API because it's more flexible than a basic string replacement, and they need some features of this API. In the same way, a WordPress site is not as fast as a completely static website, but it doesn't mean that it's bad to build a website with WordPress.

Instead of adding limitations that affected a lot of websites, plugins and themes in 4.2.3, I suggest an other way to fix the security issues with shortcodes:

function add_shortcode($tag, $func, $capability = 'manage_options') {
	global $shortcode_tags;

	if ( is_callable($func) )
		$shortcode_tags[$tag] = array('function' => $func, 'capability' => $capability);
}


function disable_shortcodes_when_editing_post($data) {
	global $shortcode_tags;
	
	if (function_exists('user_can')) {
	foreach ($shortcode_tags as $tag => $value) {
		if (!user_can($data['post_author'], $value['capability'])) {
			foreach (array('post_content', 'post_content_filtered', 'post_excerpt', 'post_title') as $key) {
			$data[$key] = str_replace(array('['.$tag, $tag.']'), array('['.$tag, $tag.']'), $data[$key]); }
		}
	}
	}
	
	return $data;
}

add_filter('wp_insert_post_data', 'disable_shortcodes_when_editing_post', 10, 1);

If it's important to not affect the $shortcode_tags variable:

$shortcode_tags = array();
$shortcode_capabilities = array();


function add_shortcode($tag, $func, $capability = 'manage_options') {
	global $shortcode_tags, $shortcode_capabilities;

	if ( is_callable($func) ) {
		$shortcode_tags[$tag] = $func;
		$shortcode_capabilities[$tag] = $capability;
	}
}


function disable_shortcodes_when_editing_post($data) {
	global $shortcode_capabilities;
	
	if (function_exists('user_can')) {
	foreach ($shortcode_capabilities as $tag => $capability) {
		if (!user_can($data['post_author'], $capability)) {
			foreach (array('post_content', 'post_content_filtered', 'post_excerpt', 'post_title') as $key) {
			$data[$key] = str_replace(array('['.$tag, $tag.']'), array('['.$tag, $tag.']'), $data[$key]); }
		}
	}
	}
	
	return $data;
}

add_filter('wp_insert_post_data', 'disable_shortcodes_when_editing_post', 10, 1);

[miqrogroove]

Replying to Kleor:

If the patch done by miqrogroove doesn't reintroduce something dangerous, is there any good reason to remove it in a future version of WordPress?

We want to move away from raw shortcodes that can't pass a reasonable HTML filter. There are benefits to doing this in the long run, in version 4.4 or later. I would prefer to push that back to 4.5 or 4.6, but the reaction to this ticket has been so hostile that I think we will be lucky to get 4.4.

[dd32]

I support merging this to trunk, we have a patch and no good excuse to break it (Although I can recognise that the behaviour isn't seen as the "expected" use-cases of shortcodes originally).

My main question however, is that this now allows for things like <[shortcode]> to work, but yet it doesn't restore the handling of a closing HTML tag: </[shortcode]>, which is stupidly simple to support here by using %^<\s*/?\s*\[\[?[^\[\]]+\]% instead (Addition of \s*/?). Is there any good reason to skip that? (other than not being the use-case reported here)

[dd32]

In 33564:

Fix do_shortcode('<[shortcode]') edge case.
Props miqrogroove.

Merges [33499] to the 4.1 branch.
See #33116.

[dd32]

In 33565:

Fix do_shortcode('<[shortcode]') edge case.
Props miqrogroove.

Merges [33499] to the 4.0 branch.
See #33116.

[dd32]

In 33566:

Fix do_shortcode('<[shortcode]') edge case.
Props miqrogroove.

Merges [33499] to the 3.9 branch.
See #33116.

[dd32]

In 33567:

Fix do_shortcode('<[shortcode]') edge case.
Props miqrogroove.

Merges [33499] to the 3.8 branch.
See #33116.

[dd32]

In 33568:

Fix do_shortcode('<[shortcode]') edge case.
Props miqrogroove.

Merges [33499] to the 3.7 branch.
See #33116.

[miqrogroove]

The extra slash simply didn't get any traction. It was discussed in #core and yet we still don't have any example of it being used in the wild.

[dd32]

Replying to miqrogroove:

The extra slash simply didn't get any traction. It was discussed in #core and yet we still don't have any example of it being used in the wild.

I'm almost certain I've seen plugins, and some non-wordpress.org themes using it before, although can't think of one off the top of my head.
Really rather surprised that there hasn't been any non-email use-case examples of the opening <[shortcode]>.

[Jyria]

Thanks for fixing this.

[miqrogroove]

Still broken in RC2.

[dd32]

33116.trunk.3.patch​ tweaks the wording slightly, removing the mention of 4.4, and instead replacing it with "a future release".

[dd32]

In 33594:

Fix do_shortcode('<[shortcode]') edge case.
Props miqrogroove.
Merges [33499] trunk.
See #33116.

[dd32]

[33594] doesn't add the _doing_it_wrong() as I don't feel this is a doing it wrong case, nor am I confident that it should be removed.
I've left the ticket open for another lead to step in and follow that direction though.

[obenland]

Closed based on Slack conversation above.