Make WordPress Core

Opened 14 years ago

Closed 9 years ago

Last modified 9 years ago

#15694 closed defect (bug) (maybelater)

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

Reported by: miqrogroove's profile miqrogroove Owned by: miqrogroove's profile miqrogroove
Milestone: Priority: normal
Severity: normal Version: 3.0.1
Component: Shortcodes Keywords:
Focuses: 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 9 years ago.
shortcodes_broken.php (974 bytes) - added by jadpm 9 years ago.

Download all attachments as: .zip

Change History (54)

#1 @nacin
14 years ago

  • Milestone changed from Awaiting Review to Future Release

#2 @solarissmoke
14 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.

#4 @hidgw
13 years ago

  • Cc hidgw added

#5 @azaozz
13 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.

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


10 years ago

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


10 years ago

#8 @miqrogroove
10 years 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.

#9 @obenland
10 years ago

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

#10 @obenland
10 years ago

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

#11 @miqrogroove
9 years 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.

#12 @miqrogroove
9 years ago

#29608 was marked as a duplicate.

#13 @miqrogroove
9 years ago

#31471 was marked as a duplicate.

#14 @miqrogroove
9 years 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.

#15 @miqrogroove
9 years ago

  • Description modified (diff)

#16 @obenland
9 years ago

  • Milestone changed from 4.3 to Future Release

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

#17 @pento
9 years ago

In 33359:

Shortcodes: Improve the reliablity of shortcodes inside HTML tags.

Props miqrogroove.

See #15694.

#18 @pento
9 years ago

In 33360:

Shortcodes: Improve the reliablity of shortcodes inside HTML tags.

Merge of [33359] to the 4.2 branch.

Props miqrogroove.

See #15694.

#19 @pento
9 years ago

In 33380:

Shortcodes: Improve the reliablity of shortcodes inside HTML tags.

Merge of [33359] to the 4.1 branch.

Props miqrogroove.

See #15694.

#20 @pento
9 years ago

In 33381:

Shortcodes: Improve the reliablity of shortcodes inside HTML tags.

Merge of [33359] to the 4.0 branch.

Props miqrogroove.

See #15694.

#21 @pento
9 years ago

In 33386:

Shortcodes: Improve the reliablity of shortcodes inside HTML tags.

Merge of [33359] to the 3.9 branch.

Props miqrogroove.

See #15694.

#22 @pento
9 years ago

In 33388:

Shortcodes: Improve the reliablity of shortcodes inside HTML tags.

Merge of [33359] to the 3.8 branch.

Props miqrogroove.

See #15694.

#23 @pento
9 years ago

In 33389:

Shortcodes: Improve the reliablity of shortcodes inside HTML tags.

Merge of [33359] to the 3.7 branch.

Props miqrogroove.

See #15694.

#24 @jadpm
9 years 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.

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


9 years ago

#26 @khromov
9 years 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 9 years ago by khromov (previous) (diff)

#27 @kitchin
9 years 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 9 years ago by kitchin (previous) (diff)

#28 follow-up: @dnavarrojr
9 years 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.

#29 in reply to: ↑ 28 @kitchin
9 years 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

#30 @injira
9 years 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.

#31 @kitchin
9 years 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.

#32 @Braad
9 years 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.

#33 follow-up: @chriscct7
9 years ago

  • Severity changed from major to normal

#34 @thatguy334233
9 years 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 9 years ago by thatguy334233 (previous) (diff)

#35 @miyarakira
9 years ago

Last edited 9 years ago by miyarakira (previous) (diff)

#36 in reply to: ↑ 33 @injira
9 years ago

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

#37 @pento
9 years 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.

#38 @chriscct7
9 years 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 9 years ago by chriscct7 (previous) (diff)

#39 @jadpm
9 years 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.

#41 in reply to: ↑ 40 ; follow-up: @chriscct7
9 years 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 9 years ago by chriscct7 (previous) (diff)

#42 in reply to: ↑ 41 ; follow-up: @jadpm
9 years 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?

#43 in reply to: ↑ 42 @chriscct7
9 years 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.

#44 @jadpm
9 years 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 9 years ago by jadpm (previous) (diff)

#45 @pento
9 years 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.

#46 @Yapaslfeu
9 years 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)

#47 @miqrogroove
9 years 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.

#48 @miqrogroove
9 years ago

  • Milestone Awaiting Review deleted

#49 @SergeyBiryukov
9 years ago

  • Milestone set to Awaiting Review

#50 @Yapaslfeu
9 years ago

  • Focuses javascript removed
  • Keywords close removed

#51 @miqrogroove
9 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to maybelater
  • Status changed from assigned to closed

#52 @SergeyBiryukov
9 years ago

In 34222:

Fix a typo in wptexturize() and wp_replace_in_html_tags() comments.

Props bobbingwide.
See #15694.

#53 @rrhobbs
9 years ago

Gallery shortcode is still broken! [gallery ids="xxxxxxx,xxxxxx] = 404

Note: See TracTickets for help on using tickets.