Opened 9 years ago
Closed 9 years ago
#37208 closed defect (bug) (fixed)
Comment blacklist can be bypassed with HTML
Reported by: |
|
Owned by: |
|
---|---|---|---|
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)
Change History (17)
This ticket was mentioned in Slack in #core-comments by rachelbaker. View the logs.
9 years ago
#4
@
9 years ago
- Keywords needs-unit-tests added
- Milestone changed from Awaiting Review to Future Release
#5
@
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
@
9 years ago
- Owner set to rachelbaker
- Resolution set to fixed
- Status changed from new to closed
In 38047:
#8
follow-up:
↓ 9
@
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
@
9 years ago
Replying to dd32:
@rachelbaker @cfinke Should this just use
wp_strip_all_tags()
instead ofwp_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
@
9 years ago
- Keywords needs-patch added; has-patch commit removed
- Resolution fixed deleted
- Status changed from closed to reopened
#11
@
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
@
9 years ago
- Keywords commit added
37208_strip_tags.diff looks good, passes unit tests, works.
Added unit test