When it checks whether "Untrusted users are allowed to input dangerous HTML tags", Security review just looks for HTML Filter on the relevant input formats. HTML Filter may be replaced with the more robust HTML Purifier... but because Security Review doesn't recognize it, it still declares an input format filtered by it insecure. I would like to enhance the test to include HTML Purifier. Patch to follow immediately.
| Comment | File | Size | Author |
|---|---|---|---|
| #8 | security_review-accept-other-filter-modules-1449332-7.patch | 3.72 KB | gisle |
| #6 | security_review-accept-other-filter-modules-1449332-6.patch | 3.71 KB | gisle |
| #1 | security_review-1449332-1.patch | 1.33 KB | traviscarden |
Comments
Comment #1
traviscarden commentedComment #2
gregglesI'm open to this idea, but...
htmlpurifier has dozens of configs and my sense is that some of them, when configured in a certain way, do present XSS vulnerabilities. So this patch would need to look at all of those to make sure they aren't vulnerabilities.
Am I mistaken?
Comment #3
gregglesNeeds work per #2.
Comment #4
tierso commentedIt's not just HTML pur, but WYSIWIG filter as well. It's the "only" way to enable text formatting with WYSIWYG/ckeditor.
Comment #5
Jorrit commentedI think also the HTML escape filter should allow an input format to be marked as safe. With this format, no HTML will be output directly to the browser, it will be escaped.
Comment #6
gisleHere is another go at getting this feature request implemented.
As indicated in #2 and #3, the module should not have built-in assertions about what alternatives to the HTML Filter are "safe". This is very rigid, and in the case of "HTML purifier" the assertion is also likely to be wrong.
Instead, I've added an option to the "Settings" tab where false positives resulting from any text filter module, including custom modules unknown to the module maintainers, may be muted at the discretion of the site admin. Such an option should obviously be used with caution, and only after the admin as made sure that the HTML Filter replacement he or she has choosen is up to snuff.
However, I find it helpful to have the option to mute false positives that flags a certain custom filter I use on a single field on every site I run, without having to skip or ignore the entire "text format" test - as there a many other input fields where the "text format" test is wanted. This is my use case and my motivation for producing this patch.
Please review!
Comment #8
gisleForgot to account for the eventuality that the variable did not exist. Slightly revised patch attached.
Please review.
Comment #9
smustgrave commentedWill need a reroll if still interest.
Comment #10
gisleNo longer working on this.
Comment #11
smustgrave commenteddo you find this issue for D10, ckeditor, security review latest version?
Comment #12
gisleI think this does not have significance for anything beyond Drupal 7.
Way back then, I maintained Trusted Src, and I wanted that to work with Security Review. That was my motivation for getting involved. Since then, a lot of development, but in particular the core Media module, has provided much better solutions to the problem I tried to solve with Trusted Src.
Drupal 7 is still a supported version, but "fixing" marginal issues for it is not a priority,
Comment #13
smustgrave commentedFully get it. Will close this out as outdated. Keeping an eye on 7.x issues but not working on any.
If anyone wants to reopen and work on it feel free.
2.0.x is active though