WordPress.org

Make WordPress Core

Opened 4 years ago

Closed 3 years ago

#29810 closed defect (bug) (duplicate)

Prevent HTML Corruption When Shortcodes Are Not Allowed

Reported by: miqrogroove Owned by:
Milestone: Priority: normal
Severity: normal Version: 2.6
Component: Formatting Keywords: needs-unit-tests
Focuses: Cc:

Description

Bug #12690 is alive and well. Only some specific test cases were resolved in 4.0.

The underlying problem is that wptexturize() is not aware of the Shortcode API status. The function effectively treats all input as though it were provided by administrator level users without adequate regard to HTML sanity.

Here is a strategy I brainstormed with @azaozz :

  1. Add a new global variable or a parameter to the wptexturize() function that would allow shortcode parsing by default, but limit or disable shortcode parsing when not allowed.
  2. Find all instances where wptexturize() is used in core, starting with default-filters.php.
  3. For any instance where do_shortcode() is not also used (presumably every core instance except the_content) set the global variable to limit shortcode parsing.
  4. Inside wptexturize(), limited shortcode parsing means the preg_split expression is modified such that HTML is strictly forbidden inside of shortcodes. Disabled shortcode parsing means the shortcode portion of the preg_split expression is removed completely and all shortcodes will become texturized with other content.

#29661 is related. The strategy described above would have no impact on shortcode parsing, except in those places where shortcodes are already not allowed without a plugin.

The impact on plugins would be as follows. A plugin is broken if all of the following conditions are true:

  1. The plugin has hooked do_shortcode() to a filter that we are securing with this patch.
  2. The shortcodes being filtered by both the plugin and by wptexturize() contain unescaped HTML-special characters such as angle brackets <>.
  3. The plugin is not updated with a new filter to re-enable full shortcode parsing within wptexturize().

This would be necessary because, by default, WordPress does not allow shortcodes in many areas that are impacted by the wptexturize() function.

Attachments (4)

miqro-29810.patch (17.1 KB) - added by miqrogroove 4 years ago.
miqro-29810.2.patch (17.2 KB) - added by miqrogroove 4 years ago.
miqro-29810.3.patch (17.3 KB) - added by miqrogroove 4 years ago.
miqro-29810.4.patch (17.4 KB) - added by miqrogroove 4 years ago.
Refreshed

Download all attachments as: .zip

Change History (8)

#1 follow-up: @kitchin
4 years ago

Are you missing global in the three functions at the end of miqro-29810.2.patch ?

And just as a matter of style at line 214

$pos = strpos( $text, '[' ); 
if ( false !== $pos ) { 
   if ( false !== strpos( $text, ']', $pos ) ) { 
     $find_shortcodes = true; 
   } 
} 

can be one-lined to

$find_shortcodes = is_int( $pos = strpos( $text, '[' ) ) && is_int( strpos( $text, ']', $pos ) );

But note the Wordpress codebase does not use the idiom is_int(strpos()) currently. Just a matter of style vs. false !==. Has better parentheses!

#2 in reply to: ↑ 1 @miqrogroove
4 years ago

Replying to kitchin:

Are you missing global in the three functions at the end of miqro-29810.2.patch ?

Indeed. It's a work in progress.

And just as a matter of style at line 214 can be one-lined

I prefer one-liners myself, especially the if (this) return $that; variety. The committers here usually prefer readability though.

@miqrogroove
4 years ago

Refreshed

#3 @miqrogroove
4 years ago

  • Keywords needs-unit-tests added

#4 @miqrogroove
3 years ago

  • Milestone Awaiting Review deleted
  • Resolution set to duplicate
  • Status changed from new to closed

Duplicate of #30531.

This bug is more trivial than I originally thought, and it will be addressed in the shortcode API roadmap by changing the filter priorities.

Note: See TracTickets for help on using tickets.