Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#33259 closed defect (bug) (fixed)

Inconsistent KSES Shortcode Result

Reported by: miqrogroove Owned by: miqrogroove
Milestone: 4.3 Priority: normal
Severity: minor Version: 4.2.3
Component: Shortcodes Keywords: has-patch commit dev-reviewed
Focuses: Cc:

Description

In shortcodes.php,

if ( '' !== $new_attr ) {

should be

if ( '' !== trim( $new_attr ) ) {

Otherwise we get two different results depending on how much whitespace surrounds an attribute that contained a shortcode.

Attachments (3)

33259.patch (505 bytes) - added by Ankit K Gupta 6 years ago.
added patch file
33259.diff (1.0 KB) - added by obenland 6 years ago.
33259.2.diff (1.1 KB) - added by miqrogroove 6 years ago.

Download all attachments as: .zip

Change History (16)

#1 @miqrogroove
6 years ago

  • Milestone changed from Awaiting Review to 4.3

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


6 years ago

#3 @obenland
6 years ago

  • Keywords needs-patch added
  • Owner set to miqrogroove
  • Status changed from new to assigned

@Ankit K Gupta
6 years ago

added patch file

#4 @Ankit K Gupta
6 years ago

  • Keywords has-patch added; needs-unit-tests needs-patch removed

Updated the code as per miqrogroove suggestion.

#5 @ocean90
6 years ago

  • Keywords needs-unit-tests added

@obenland
6 years ago

#6 follow-up: @obenland
6 years ago

Added unit test. Not sure if that is enough though.

#7 in reply to: ↑ 6 @miqrogroove
6 years ago

Replying to obenland:

Added unit test. Not sure if that is enough though.

Not quite, but it's always good to have extra tests. I'll get this out tonight.

@miqrogroove
6 years ago

#8 @miqrogroove
6 years ago

  • Keywords needs-unit-tests removed

The added test passes only after patch.

#9 @ocean90
6 years ago

33259.2.diff works for me.

Without the change to trim( $new_attr ) the test fails with

1) Tests_Shortcode::test_escaping with data set #2 ('1 <a noise="[test-shortcode-tag]"> 2 <a noise=" [test-shortcode-tag] " >', '1 <a noise="[test-shortcode-tag]"> 2 <a noise=" [test-shortcode-tag] " >')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'1 <a noise="[test-shortcode-tag]"> 2 <a noise=" [test-shortcode-tag] " >'
+'1 <a noise="[test-shortcode-tag]"> 2 <a  >'

/srv/www/wp-develop/svn/tests/phpunit/tests/shortcode.php:412

#10 @SergeyBiryukov
6 years ago

  • Keywords commit added

33259.2.diff looks good.

#11 @nacin
6 years ago

  • Keywords dev-reviewed added

Looks good. No issues removing a null byte, right?

#12 @miqrogroove
6 years ago

wp_kses_one_attr() is going to strip nulls first. This patch will never see or touch nulls.

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

#13 @ocean90
6 years ago

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

In 33600:

Shortcodes: Trim whitespace after sanitizing the shortcode output.

props Ankit K Gupta, obenland, miqrogroove.
fixes #33259.

Note: See TracTickets for help on using tickets.