WordPress.org

Make WordPress Core

Opened 4 years ago

Last modified 8 weeks ago

#34406 reopened defect (bug)

wp_kses_hair is too stringent redux

Reported by: travisnorthcutt Owned by:
Milestone: Awaiting Review Priority: normal
Severity: normal Version: 1.5
Component: Formatting Keywords: has-patch, needs-unit-tests, bulk-reopened
Focuses: Cc:

Description

Attributes from custom xml name spaces may use numbers, underscores, en dashes, and em dashes, but the regex used inside wp_kses_hair() doesn't allow them through. Technically, there are other allowed characters, but that's probably getting into very fringe edge-case territory.

Admittedly, en and em dashes may be edge-case territory as well, so two patches are provided: one allowing numbers and underscores, and one additionally allowing en and em dashes.

Related: #17847

Here's a link to the W3C spec for reference.

Attachments (3)

34406.diff (606 bytes) - added by travisnorthcutt 4 years ago.
Allow numbers and underscores in html attribute names
34406.2.diff (618 bytes) - added by travisnorthcutt 4 years ago.
Allow numbers, underscores, and en and em dashes in html attribute names
34406.3.diff (2.0 KB) - added by travisnorthcutt 4 years ago.
Updated patch with tests

Download all attachments as: .zip

Change History (15)

@travisnorthcutt
4 years ago

Allow numbers and underscores in html attribute names

@travisnorthcutt
4 years ago

Allow numbers, underscores, and en and em dashes in html attribute names

#1 @travisnorthcutt
4 years ago

  • Keywords has-patch dev-feedback added

Also, here's a demo on RegExr if anyone wants to run checks against it: http://regexr.com/3c1uq

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


4 years ago

#3 @dd32
4 years ago

  • Keywords needs-unit-tests added

I see no reason why numbers and underscores shouldn't be allowed here, although I don't really see the point in whitelisting en/em dashes if we're not going to support the full set of characters.

Some unit tests would be good for this change I think.

@travisnorthcutt
4 years ago

Updated patch with tests

#4 follow-up: @miqrogroove
4 years ago

Needs more tests. This is the kind of ticket where the tests will get more traction than the patch ever will. :)

For example,

<img src="" data_at:2x="[audio]" alt="blah" />

We expect this shortcode to get stripped automatically upon display, which almost certainly fails in the proposed patch.

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

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

Replying to miqrogroove:

Needs more tests. This is the kind of ticket where the tests will get more traction than the patch ever will. :)

For example,

<img src="" data_at:2x="[audio]" alt="blah" />

We expect this shortcode to get stripped automatically upon display, which almost certainly fails in the proposed patch.

I think that's not current behavior; without applying my patch, try this:

global $allowedposttags;
$test = '<img alt="[blah]" />';
echo wp_kses( $test, $allowedposttags );

In my testing, the brackets are allowed. So, in that case, your example fails with the current patch and on existing core, but only if we do indeed want/expect the shortcode to get stripped. Is that the case, though? From my reading of the Shortcode Roadmap Draft Three it sounds like that restriction shouldn't happen until 4.6.

#6 follow-up: @miqrogroove
4 years ago

Try it in the post editor. Because wp_kses itself doesn't handle shortcodes, there will be some unexpected behaviors without more test coverage here.

#7 in reply to: ↑ 6 @travisnorthcutt
4 years ago

Replying to miqrogroove:

Try it in the post editor. Because wp_kses itself doesn't handle shortcodes, there will be some unexpected behaviors without more test coverage here.

Without my patch applied, <img alt="[audio]" /> in the (text tab of the) post editor results in <img alt="" /> being output.
With my patch applied, <img alt="[audio]" /> in the (text tab of the) post editor results in <img alt="" /> being output.

My patch only affects parsing of attribute names, not attribute values, so I don't see how shortcodes in attribute values should be relevant here. I'm happy to change my mind, of course :).

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


3 years ago

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


3 years ago

#10 @chriscct7
3 years ago

  • Keywords dev-feedback removed

Bug Scrub 1/8/2015 notes:

  • The suggestion was made to add a unit test to avoid regression for the case mentioned in comment 7.
  • Further, the reporter should reach out to miqrogroove for help identifying additional edge cases to develop unit tests for.

#11 @iseulde
4 months ago

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

This ticket has not seen any activity in over *two* years, so I'm closing it as "wontfix".

The ticket may lack decisiveness, may have become irrelevant, or may not have gathered enough interest.

If you think this ticket does deserve some attention again, feel free to reopen.

For bugs, it would be great if you could provide updated steps to reproduce against the latest version of WordPress (5.0.2 at the time of writing). Remember images or a video can be superior to explain a problem. At the very least, quickly test again to make sure the bug still exists.

If it’s an enhancement or feature, some extra motivation may help.

Thank you for your contributions to WordPress! <3

#12 @JeffPaul
8 weeks ago

  • Keywords bulk-reopened added
  • Milestone set to Awaiting Review
  • Resolution wontfix deleted
  • Status changed from closed to reopened

A decision was made to reopen tickets that were closed in the bulk edit that this ticket was affected by. This ticket is being placed back into the Awaiting Review milestone so it can be individually evaluated and verified to determine if it is still relevant/valid or reproducible.

Note: See TracTickets for help on using tickets.