Make WordPress Core

Opened 9 years ago

Closed 9 years ago

#37208 closed defect (bug) (fixed)

Comment blacklist can be bypassed with HTML

Reported by: cfinke's profile cfinke Owned by: rachelbaker's profile rachelbaker
Milestone: 4.6 Priority: normal
Severity: normal Version: 1.5
Component: Comments Keywords: has-unit-tests has-patch commit
Focuses: Cc:

Description

If the comment blacklist contains the entry "bannedword", users can still post comments containing (or appearing to contain) "bannedword" by using HTML:

<b>banned</b>word
banned<b></b>word
banned<b>word</b>

A solution: wp_blacklist_check() could run wp_kses() before looking for blacklist entries. Patch attached.

Attachments (4)

37208.diff (871 bytes) - added by cfinke 9 years ago.
37208.2.diff (3.9 KB) - added by rachelbaker 9 years ago.
Added unit test
37208.3.diff (1.8 KB) - added by ocean90 9 years ago.
37208_strip_tags.diff (491 bytes) - added by rachelbaker 9 years ago.

Download all attachments as: .zip

Change History (17)

@cfinke
9 years ago

#1 @Presskopp
9 years ago

  • Keywords has-patch added

#2 @ocean90
9 years ago

  • Version changed from trunk to 1.5

This ticket was mentioned in Slack in #core-comments by rachelbaker. View the logs.


9 years ago

#4 @rachelbaker
9 years ago

  • Keywords needs-unit-tests added
  • Milestone changed from Awaiting Review to Future Release

@rachelbaker
9 years ago

Added unit test

#5 @rachelbaker
9 years ago

  • Keywords has-unit-tests commit added; needs-unit-tests removed
  • Milestone changed from Future Release to 4.6

In 37208.2.diff I added some basic unit tests for wp_blacklist_check including test_should_return_true_when_content_with_html_matches_blacklist_keys

#6 @rachelbaker
9 years ago

  • Owner set to rachelbaker
  • Resolution set to fixed
  • Status changed from new to closed

In 38047:

Comments: Strip html tags from comment content before blacklist_keys comparison.

Use wp_kses() to clean comment_content for preg_match against the blacklist_keys. Also includes some initial unit tests for wp_blacklist_check().
Previously, if a blacklisted key was used in comment_content split by an html tag the regex in wp_blacklist_check() would not find a match. Example: Where "springfield" was a blacklisted word, if the content of a comment included spring<i>field</i>" wp_blacklist_check()` would not return true.

Props cfinke.
Fixes #37208.

@ocean90
9 years ago

#7 @rachelbaker
9 years ago

In 38048:

Comments: Include comment_content with html and without in blacklist_keys comparison.

After [38047], also include the comment_content with html in the preg_match against blacklist keys to match urls.

Props ocean90.
Fixes #37208.

#8 follow-up: @dd32
9 years ago

@rachelbaker @cfinke Should this just use wp_strip_all_tags() instead of wp_kses()? KSes is designed to limit to a whitelist, if you want no tags, might as well just strip them entirely.

#9 in reply to: ↑ 8 @cfinke
9 years ago

Replying to dd32:

@rachelbaker @cfinke Should this just use wp_strip_all_tags() instead of wp_kses()? KSes is designed to limit to a whitelist, if you want no tags, might as well just strip them entirely.

I wasn't aware of wp_strip_all_tags(), but it does seem more appropriate.

#10 @ocean90
9 years ago

  • Keywords needs-patch added; has-patch commit removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

#11 @rachelbaker
9 years ago

  • Keywords has-patch added; needs-patch removed

@dd32 Thanks for catching this. Added 37208_strip_tags.diff patch that uses wp_strip_all_tags()

#12 @peterwilsoncc
9 years ago

  • Keywords commit added

37208_strip_tags.diff looks good, passes unit tests, works.

#13 @ocean90
9 years ago

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

In 38092:

Comments: Use wp_strip_all_tags() to strip HTML tags.

wp_kses() should only be used if you have a whitelist.

Props rachelbaker.
Fixes #37208.

Note: See TracTickets for help on using tickets.