WordPress.org

Make WordPress Core

Opened 5 years ago

Last modified 7 days ago

#15694 assigned defect (bug)

Shortcode I/O Intolerant of "]", "<", Quotes, etc.

Reported by: miqrogroove Owned by: miqrogroove
Milestone: Priority: normal
Severity: normal Version: 3.0.1
Component: Shortcodes Keywords: close
Focuses: javascript Cc:

Description (last modified by miqrogroove)

There are no shortcode input escaping functions available in core even though the Shortcode API is increasingly strict about not allowing special characters inside shortcode attributes.

Common problems for plugin developers include user input containing square braces. This was even a core bug prior to 3.4 where a caption shortcode would be transformed by the Visual Editor from:

[caption id="attachment_3" align="alignnone" width="300" caption="[Test Caption]"]

... to ...

[caption id="attachment_3" align="alignnone" width="300" caption="[Test Caption"]"]

As of 4.2.2, that same shortcode is transformed to:

[caption id="attachment_7" align="alignnone" width="300"]"]

Other common problems include usage of HTML-special characters for quotations or comparison operators that would need to appear in the attribute value.

Attachments (2)

shortcode-prototypes.php (635 bytes) - added by miqrogroove 4 months ago.
shortcodes_broken.php (974 bytes) - added by jadpm 5 weeks ago.

Download all attachments as: .zip

Change History (49)

comment:1 @nacin5 years ago

  • Milestone changed from Awaiting Review to Future Release

comment:2 @solarissmoke4 years ago

The problem is with the regex in get_shortcode_regex() which assumes that the first ] it comes across is the end of a shortcode tag, and it ignores the rest, thus breaking things.

For now I've added a note in the codex saying that the parser can't handle square brackets in attributes. Can't think of a way to fix this without making the regex a whole lot more complicated.

comment:4 @hidgw4 years ago

  • Cc hidgw added

comment:5 @azaozz4 years ago

The shortcodes work similarly to HTML with [ and ] being the equivalent of < and >. In that terms shortcodes cannot contain square brackets the same way HTML tags cannot contain "less than" and "greater than" chars. If they must be used, they need to be encoded/replaced with entities.

comment:6 @ircbot15 months ago

This ticket was mentioned in IRC in #wordpress-dev by miqrogroove. View the logs.

comment:7 @ircbot15 months ago

This ticket was mentioned in IRC in #wordpress-dev by miqrogroove. View the logs.

comment:8 @miqrogroove9 months ago

  • Keywords needs-patch needs-unit-tests 4.2-early added

We need to fix this soon and add appropriate escape/unescape functions to the API. Consider it at the top of my to do list.

comment:9 @obenland4 months ago

  • Owner set to miqrogroove
  • Status changed from new to assigned

comment:10 @obenland4 months ago

  • Keywords 4.2-early removed
  • Milestone changed from Future Release to 4.3

comment:11 @miqrogroove3 months ago

  • Focuses javascript added
  • Keywords changed from needs-patch, needs-unit-tests to needs-patch needs-unit-tests
  • Priority changed from normal to high
  • Summary changed from Caption Shortcode I/O Intolerant of "]" Char to Shortcode I/O Intolerant of "]", "<", Quotes, etc.

comment:12 @miqrogroove3 months ago

#29608 was marked as a duplicate.

comment:13 @miqrogroove3 months ago

#31471 was marked as a duplicate.

comment:14 @miqrogroove3 months ago

Did some research on this today. The original ticket description is obsolete because as of https://codex.wordpress.org/Version_3.4 there are no longer any user inputs in the default shortcode attribute values.

Since this issue does not affect the core shortcodes, this has become purely an API problem for plugin developers.

comment:15 @miqrogroove3 months ago

  • Description modified (diff)

comment:16 @obenland8 weeks ago

  • Milestone changed from 4.3 to Future Release

No movement in 6 weeks. Maybe in a future release.

comment:17 @pento6 weeks ago

In 33359:

Shortcodes: Improve the reliablity of shortcodes inside HTML tags.

Props miqrogroove.

See #15694.

comment:18 @pento6 weeks ago

In 33360:

Shortcodes: Improve the reliablity of shortcodes inside HTML tags.

Merge of [33359] to the 4.2 branch.

Props miqrogroove.

See #15694.

comment:19 @pento5 weeks ago

In 33380:

Shortcodes: Improve the reliablity of shortcodes inside HTML tags.

Merge of [33359] to the 4.1 branch.

Props miqrogroove.

See #15694.

comment:20 @pento5 weeks ago

In 33381:

Shortcodes: Improve the reliablity of shortcodes inside HTML tags.

Merge of [33359] to the 4.0 branch.

Props miqrogroove.

See #15694.

comment:21 @pento5 weeks ago

In 33386:

Shortcodes: Improve the reliablity of shortcodes inside HTML tags.

Merge of [33359] to the 3.9 branch.

Props miqrogroove.

See #15694.

comment:22 @pento5 weeks ago

In 33388:

Shortcodes: Improve the reliablity of shortcodes inside HTML tags.

Merge of [33359] to the 3.8 branch.

Props miqrogroove.

See #15694.

comment:23 @pento5 weeks ago

In 33389:

Shortcodes: Improve the reliablity of shortcodes inside HTML tags.

Merge of [33359] to the 3.7 branch.

Props miqrogroove.

See #15694.

comment:24 @jadpm5 weeks ago

Not sure this is the right place or even moment, but this change, which just landed 33 hours ago in the 4.2 branch and was released with the latest 4.2.3 is causing a massive chain of problems.

I work on a company which products rely a lot on shortcodes, and suddenly all shortcodes used as HTML attributes have stopped working. We are experiencing a huge impact in our support forum and clients are blaming us for not being ready... to test a change that landed just hours before being released. Not fair IMHO.

You can see also some very nice bug reports on products that are not our own, here:
https://wordpress.org/support/topic/wordpress-423-broke-my-code
https://wordpress.org/support/topic/no-buttons-to-see-after-update-wp-423

A simple test case:

  • create a shortcode that returns an image URL
  • add that shortcode as a background-image utl value on a style attribute
  • pull your hair when visiting the frontend

This used to work perfectly fine in 4.2.2 and before. So I do wonder how this can happen. I am attaching a test case with a dummy shortcode (really dummy) and two test cases, with a very nasty result.

Please review this and provide a fix or a revert to a previous state until this gets a proper fix.

@jadpm5 weeks ago

comment:25 @slackbot5 weeks ago

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

comment:26 @khromov5 weeks ago

@pento Where can one find the reasoning behind the 33359 changeset that landed in 4.2.3? Surely there most have been some discussion before landing almost 500 rows of code into do_shortcode()?

Last edited 5 weeks ago by khromov (previous) (diff)

comment:27 @kitchin5 weeks ago

EDIT:
I found out later it was a security patch, which makes more sense. My comments about the spec still stand.

ORIGINAL:

Should never have landed on a release, much less all releases. I've worked on this code and it is complex. I'm not sure there is even a clear spec for how shortcodes-in-html and html-in-shortcode should work. And it's all targeted at a wide variety of legacy uses. I can't imagine a worse bug to promote to releases without landing on core. We last changed the implicit spec for 4.0 I believe.

In addition, patches for this code must always be scored for performance, because that was the point of the last change, which was major work by @miqrogroove.

Last edited 5 weeks ago by kitchin (previous) (diff)

comment:28 follow-up: @dnavarrojr5 weeks ago

The fact that this got out and broke tens of thousands if not hundreds of thousands of sites makes me question the viability of auto-update. This has been a support nightmare for me as well, which makes me lose a lot of confidence in those who make the decisions on rolling out these updates.

comment:29 in reply to: ↑ 28 @kitchin5 weeks ago

Replying to dnavarrojr:

The fact that this got out and broke tens of thousands if not hundreds of thousands of sites makes me question the viability of auto-update. This has been a support nightmare for me as well, which makes me lose a lot of confidence in those who make the decisions on rolling out these updates.

Pardon the off-task comment, I will leave it at this and not clutter the ticket any further, but it is informative.

This is the list of other bugs with patches that landed on trunk and on 4.2.3 on the same day without baking. The reasons may have been explained in Slack, I don't know. All other non-security fixes for 4.2.3 baked for at least 5 weeks on trunk before landing on branch.

Landed 9 days or less ago: #32165 #32198 #32279 #32763

Landed 3 weeks ago: #32127 #32154

Landed 3 months ago, just after 4.2.2: #32310

comment:30 @injira5 weeks ago

  • Severity changed from normal to critical

There really should be some movement from WP on this. The idea of having these auto-updates when they began was not to make any significant changes that would break people’s sites without a dire need and plenty of notice about new guidelines.

comment:31 @kitchin5 weeks ago

  • Severity changed from critical to major

The answer is here, it was a security fix: https://make.wordpress.org/core/2015/07/23/changes-to-the-shortcode-api/ The Shortcode API has changed.

comment:32 @Braad5 weeks ago

+1 for a better solution. This broke a lot of client sites for us, and plugins like Toolset's Views that rely on shortcodes outside of the Visual Editor are seriously affected. The issue with double quotes inside double quotes not being supported is understandable, but allowing the usage of shortcodes inside HTML attributes should be a supported use case.

comment:33 follow-up: @chriscct75 weeks ago

  • Severity changed from major to normal

comment:34 @thatguy3342335 weeks ago

  • Keywords 2nd-opinion added
  • Severity changed from normal to blocker

This track begins 5 years ago.

No pressure at all, no urgency, nothing.

Suddenly, nightly builds are pushed and delivered within 30 hrs before release!

What started (in forums, slack, etc, after peoples started complaining the today's update) as:
"We are looking into that"
suddenly changed to this:
https://make.wordpress.org/core/tag/shortcodes/
(changes to the ShortCode API)

More or less simultaneously, in forums there are statements that "WordPress has disabled the security update from further being installed while they address this issue"
(example: https://wordpress.org/support/topic/wordpress-423-broke-my-code?replies=15)

I see now 2 possibilities:

Either a BUG was addressed here (and suddenly it was a quiet urgent one, if so)
OR
a API change has been planned (in advance) and updates where pushed, still with a unusual "last minute sprint"

First option would be acceptable (BUG Addressed),
==> but then why was this BUG no issue for 5 years long,
and suddenly (beneath a time range of 30 hours) it becomes a crucial BUG?

If second option is true, IMHO
API Changes *have* to be announced. NO API has ever been receiving such crucial changes in just a few hours and then released as a *BUG fix*.

Please, there are not one, two or 50 sites out there, but K's of sites now broken due to this "API UPDATE"

If it would have been a API UPDATE, every well aware DEV would have been prepared, and could have provided compatibility for their Software (or at least announce this -future- problem)

Furthermore, API Updates usually offer backwards-compatiblity.

*LAST BUT NOT LEAST*

I think we all are waiting for the answer to this:
https://core.trac.wordpress.org/ticket/15694#comment:26

Last edited 5 weeks ago by thatguy334233 (previous) (diff)

comment:35 @miyarakira5 weeks ago

Last edited 5 weeks ago by miyarakira (previous) (diff)

comment:36 in reply to: ↑ 33 @injira5 weeks ago

Replying to chriscct7:
For most of the thousands of users who have broken sites because of this it is a critical issue.

comment:37 @pento5 weeks ago

  • Keywords 2nd-opinion removed
  • Priority changed from high to normal
  • Severity changed from blocker to normal

This ticket is a related, but ultimately different issue to the ones being mentioned in the comments.

This particular ticket will not be fixed in relation to the issues cropping up in 4.2.3, so continuing to change the Priority and Severity will do nothing but fill up the comments.

comment:38 @chriscct75 weeks ago

Replying to injira:

Replying to chriscct7:
For most of the thousands of users who have broken sites because of this it is a critical issue.

Those users will need to wait for an update from their plugin authors to
update their plugins to follow the Shortcode API guidelines.

A lot of the comments on this ticket over the last 24 hours are factually incorrect.

The code committed in this ticket was reviewed by the security team for a very very long time. The policy of the security team is not to comment on security issues until after the team is convinced the majority of the sites that are affected have updated. So I won't discuss the reasoning for the update, other than to point out that as stated on the make.wordpress.org article, there was not an opportunity to alert the plugins authors ahead of time, or to have the code in trunk well ahead of time without putting the security of websites in danger.

That being said this change only broke sites that utilize a handful of plugins that encouraged a use of shortcodes (within HTML attributes) for which the Shortcode API was never intended or designed to be used (see the other make.wordpress.org post for that). Well over 99.99% of plugins will be completely unaffected by these changes.

The idea of having these auto-updates when they began was not to make any significant changes that would break people’s sites without a dire need and plenty of notice about new guidelines.

Out of all of the updates that have been autopushed, personally this is the one I agree with that decision the most.

This track begins 5 years ago.

This ticket 5 years ago had nothing to do with what 4.2.3 was about today. The original reason for the ticket was invalid (as the diff reflects). And this ticket isn't the one that was in 4.2.3.

More or less simultaneously, in forums there are statements that "WordPress has disabled the security update from further being installed while they address this issue"

No core contributor to the project or core team member ever said that. The security release was never at any point disabled. There was a person spreading a rumor about that in the forum. Purely FUD. I believe that user is now on modlock as a result.

And finally this ticket, while related, is not the changes made for 4.2.3, so with that being said, if you would like to talk about the 4.2.3 issues, the proper venue is the forum on WordPress.org, not here. This is an unrelated ticket.

Last edited 5 weeks ago by chriscct7 (previous) (diff)

comment:39 @jadpm5 weeks ago

And finally this ticket, while related, is not the changes made for 4.2.3, so with that being said, if you would like to talk about the 4.2.3 issues, the proper venue is the forum on WordPress.org, not here. This is an unrelated ticket.

Comments 17 to 23 in this very same ticket thread seem to prove you completely wrong, sorry.

comment:41 in reply to: ↑ 40 ; follow-up: @chriscct75 weeks ago

Replying to chriscct7:
Replying to jadpm:

And finally this ticket, while related, is not the changes made for 4.2.3, so with that being said, if you would like to talk about the 4.2.3 issues, the proper venue is the forum on WordPress.org, not here. This is an unrelated ticket.

Comments 17 to 23 in this very same ticket thread seem to prove you completely wrong, sorry.

I'm afraid not. Pento and I are both on the team that wrote the 4.2.3 update portion that dealt with shortcodes. Those commits are related but does not deal with the trac ticket they are on. The commits just needed to be associated with a trac ticket. If this ticket was for the 4.2.3 release it would be tagged for that release. This ticket is not solved yet and is currently milestoned for a future release. Also given 4.2.3 is out if this ticket was in 4.2.3 it would now be closed with a resolution of fixed. It's still open.

Last edited 5 weeks ago by chriscct7 (previous) (diff)

comment:42 in reply to: ↑ 41 ; follow-up: @jadpm5 weeks ago

Replying to chriscct7:

Replying to chriscct7:
Replying to jadpm:

And finally this ticket, while related, is not the changes made for 4.2.3, so with that being said, if you would like to talk about the 4.2.3 issues, the proper venue is the forum on WordPress.org, not here. This is an unrelated ticket.

Comments 17 to 23 in this very same ticket thread seem to prove you completely wrong, sorry.

I'm afraid not. Pento and I are both on the team that wrote the 4.2.3 update portion that dealt with shortcodes. Those commits are related but does not deal with the trac ticket they are on. The commits just needed to be associated with a trac ticket. If this ticket was for the 4.2.3 release it would be tagged for that release. This ticket is not solved yet and is currently milestoned for a future release. Also given 4.2.3 is out if this ticket was in 4.2.3 it would now be closed with a resolution of fixed. It's still open.

I am affraid yes. If commits need to be associates with a ticket, then tickets holding commits that do not want to get their own ticket for obscure reasons must accept comments on the changes they hold. If this is a case of "ticket hijacking just for the purpose", we all should be entitled to do this same thing.

If you think otherwise, I invite you to revert the changes introduced here, create a proper ticket for them and slate another point release as you wish. Doing otherwise would just mean that an OSS project like WordPress can get commits that can not be debated, discussed or even commented, which I am sure it not what you mean, right?

comment:43 in reply to: ↑ 42 @chriscct75 weeks ago

Replying to jadpm:

Replying to chriscct7:

Replying to chriscct7:
Replying to jadpm:

And finally this ticket, while related, is not the changes made for 4.2.3, so with that being said, if you would like to talk about the 4.2.3 issues, the proper venue is the forum on WordPress.org, not here. This is an unrelated ticket.

Comments 17 to 23 in this very same ticket thread seem to prove you completely wrong, sorry.

I'm afraid not. Pento and I are both on the team that wrote the 4.2.3 update portion that dealt with shortcodes. Those commits are related but does not deal with the trac ticket they are on. The commits just needed to be associated with a trac ticket. If this ticket was for the 4.2.3 release it would be tagged for that release. This ticket is not solved yet and is currently milestoned for a future release. Also given 4.2.3 is out if this ticket was in 4.2.3 it would now be closed with a resolution of fixed. It's still open.

I am affraid yes. If commits need to be associates with a ticket, then tickets holding commits that do not want to get their own ticket for obscure reasons must accept comments on the changes they hold. If this is a case of "ticket hijacking just for the purpose", we all should be entitled to do this same thing.

Not true at all. Many security commits don't because it would put sites at risk by identifying security concerns before sites get a chance to recieve the auto updates. Also there's a difference between ticket jacking and architectural necessity. Whether or not this ticket is for the 4.2.3 issue is not up for debate: its not. Again if you'd like to discuss the changes the forum is the most appropriate venue atm.

If you think otherwise, I invite you to revert the changes introduced here, create a proper ticket for them and slate another point release as you wish. Doing otherwise would just mean that an OSS project like WordPress can get commits that can not be debated, discussed or even commented, which I am sure it not what you mean, right?

Architecturally to my knowledge that's not possible due to the way it works behind the scenes for starters.

Bottom line: this ticket is for comments on the subject of this ticket only, not the 4.2.3 changes. That's not what this ticket is about.

comment:44 @jadpm5 weeks ago

So you are stating that by throwing the word "security" in the table, discussion about how the security fix was developed and/or implemented can not be hold on the very ticket used to commit it.

I think it is all clear here and I think I have nothing more to add to this debate. I feel that anyway, ongoing comments on the subject will be either deleted or simply ignored, so no point at all.

Last edited 5 weeks ago by jadpm (previous) (diff)

comment:45 @pento5 weeks ago

Well, this sure was a fun set of comments, ranging from an obscure shortcode usage bug, through to the nature of Open Source Software. Unfortunately, none of them were actually on topic for this ticket.

All further off topic comments will be deleted.

comment:46 @Yapaslfeu5 weeks ago

  • Severity changed from normal to major

Hi,
A new bug appears following this modification: if a shortcode is used as the attribute of an element present inside an enclosing shortcode, then it's treated before the enclosing shortcode itself!

Example:

[posts type="page"]			<— loop for each page
	<a href="[permalink]">link</a>	<— a link to the current page within the loop
[/posts]

In the code above, the [permalink] shortcode is treated first and is replaced with the main page permalink, and not the permalink of the page being looped…

Then, the [post] shortcode loops on the resolved content, and not on the original mask:

function ShortcodePosts ($atts, $content) {
	$query = new WP_Query ($atts);
	while ($query->have_posts ()) {
		$query->the_post ();
		$output .= do_shortcode ($content);	// <— no more shortcode to call
	}
}

The solution would be to ignore all the shortcodes present in enclosing shortcodes, even if they are used as HTML parameters.
(not sure to be very clear)

comment:47 @miqrogroove7 days ago

  • Keywords close added; needs-patch needs-unit-tests removed
  • Milestone changed from Future Release to Awaiting Review
  • Severity changed from major to normal

Ticket derailed.

comment:48 @miqrogroove7 days ago

  • Milestone Awaiting Review deleted
Note: See TracTickets for help on using tickets.