WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 4 years ago

#33116 closed defect (bug) (fixed)

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

Reported by: Kleor Owned by: miqrogroove
Milestone: 4.2.4 Priority: normal
Severity: minor Version: 4.2.3
Component: Shortcodes Keywords: has-patch
Focuses: Cc:
PR Number:

Description

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;
}

Attachments (7)

contact-manager-email.png (43.5 KB) - added by Kleor 4 years ago.
Email
33116.patch (1.4 KB) - added by miqrogroove 4 years ago.
33116.trunk.patch (1.4 KB) - added by miqrogroove 4 years ago.
33116.branch.patch (1.4 KB) - added by miqrogroove 4 years ago.
33116.branch.2.patch (1.4 KB) - added by miqrogroove 4 years ago.
33116.trunk.2.patch (1.4 KB) - added by miqrogroove 4 years ago.
33116.trunk.3.patch (1.5 KB) - added by dd32 4 years ago.

Download all attachments as: .zip

Change History (51)

#1 @miqrogroove
4 years ago

  • Severity changed from normal to minor

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
4 years ago

Email

#2 @Kleor
4 years ago

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
4 years ago

#3 @miqrogroove
4 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 4.2.4

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

#4 follow-ups: @markjaquith
4 years ago

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

#5 in reply to: ↑ 4 @knutsp
4 years ago

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.

#6 in reply to: ↑ 4 ; follow-up: @miqrogroove
4 years ago

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.

#7 in reply to: ↑ 6 ; follow-up: @knutsp
4 years ago

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.

#8 @Kleor
4 years ago

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.

#9 in reply to: ↑ 7 ; follow-up: @miqrogroove
4 years ago

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.

#10 in reply to: ↑ 9 ; follow-up: @knutsp
4 years ago

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?

#11 in reply to: ↑ 10 ; follow-up: @miqrogroove
4 years ago

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.

#12 in reply to: ↑ 11 @azaozz
4 years ago

  • Milestone changed from 4.2.4 to Future Release

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.

This ticket was mentioned in Slack in #core by miqrogroove. View the logs.


4 years ago

#14 @miqrogroove
4 years ago

  • Milestone changed from Future Release to 4.2.4
  • Owner set to miqrogroove
  • Status changed from new to accepted

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

#15 @azaozz
4 years ago

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

#16 @miqrogroove
4 years ago

@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.

#17 @samuelsidler
4 years ago

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.

#18 @miqrogroove
4 years ago

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.

#19 @helen
4 years ago

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.

#20 @miqrogroove
4 years ago

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.

Last edited 4 years ago by miqrogroove (previous) (diff)

#21 @azaozz
4 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 33499:

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

This ticket was mentioned in Slack in #core by miqrogroove. View the logs.


4 years ago

#23 follow-up: @miqrogroove
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

Trunk patch still needs commit per chat.

#24 in reply to: ↑ 23 @samuelsidler
4 years ago

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.

#25 follow-up: @Kleor
4 years ago

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('&#91;'.$tag, $tag.'&#93;'), $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('&#91;'.$tag, $tag.'&#93;'), $data[$key]); }
		}
	}
	}
	
	return $data;
}

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

#26 in reply to: ↑ 25 @miqrogroove
4 years ago

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.

#27 @dd32
4 years ago

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)

#28 @dd32
4 years ago

In 33564:

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

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

#29 @dd32
4 years ago

In 33565:

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

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

#30 @dd32
4 years ago

In 33566:

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

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

#31 @dd32
4 years ago

In 33567:

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

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

#32 @dd32
4 years ago

In 33568:

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

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

#33 follow-up: @miqrogroove
4 years ago

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.

This ticket was mentioned in Slack in #core by ocean90. View the logs.


4 years ago

#35 in reply to: ↑ 33 @dd32
4 years ago

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]>.

#36 @Jyria
4 years ago

Thanks for fixing this.

#37 @helen
4 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

#38 @miqrogroove
4 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Still broken in RC2.

This ticket was mentioned in Slack in #core by ocean90. View the logs.


4 years ago

@dd32
4 years ago

#40 @dd32
4 years ago

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

#41 @dd32
4 years ago

In 33594:

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

#42 @dd32
4 years ago

[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.

This ticket was mentioned in Slack in #core by obenland. View the logs.


4 years ago

#44 @obenland
4 years ago

  • Resolution set to fixed
  • Status changed from reopened to closed

Closed based on Slack conversation above.

Note: See TracTickets for help on using tickets.