Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
This information is from the code coverage report (see http://coverage.cwgordon.com/coverage).
We need to test:
1) 'Escape all HTML' filter in action.
2) URL filter with rel=nofollow enabled.
3) HTML corrector with unclosed tags.
4) URL filter with insanely long links.
5) Filtering of XSS in attributes (e.g. onClick=steal your cookie)
Comment | File | Size | Author |
---|---|---|---|
#23 | drupal.filter-test-cleanup.patch | 9.46 KB | sun |
#21 | filter-tets-omg-awesome.patch | 30 KB | webchick |
#18 | issue-276597-passed.patch | 24.37 KB | lilou |
#12 | filter_tests_passed.patch | 24.01 KB | jcnventura |
#12 | filter_tests_failed.txt | 5.01 KB | jcnventura |
Comments
Comment #1
floretan CreditAttribution: floretan commentedComment #2
catchURL filter is broken so probably not a good idea to include those additions in the testing party.
So...
1) 'Escape all HTML' filter in action.
2) HTML corrector with unclosed tags.
3) Filtering of XSS in attributes (e.g. onClick=steal your cookie)
And I'll add:
4) HTML corrector with redundant closing tags.
Comment #3
jcnventura CreditAttribution: jcnventura commentedThe patch for the HTML corrector filter in #222926: HTML Corrector filter escapes HTML comments is pending on the existence of filter module tests.
Please add the following to the list to verify the need for that patch and to validate it once it is merged:
5) Multi-line HTML comment with some broken HTML inside. The test should verify that the contents of the comment are not changed nor corrected in any manner. With the current code, this test will fail as the
<!--
tag will be converted to& lt;--
and the contents of the comment will be corrected.João
Comment #4
jhedstromI should have some time for this this week.
Comment #5
jhedstromAttached is my first attempt at this.
I've written unit tests for following filter functions, for a variety of inputs. The XSS examples come from http://ha.ckers.org/xss.html
* _filter_html
* trim(check_plain()) as used for the remove all html filter.
* _filter_htmlcorrector
* _filter_url_trim
* _filter_url
Higher level tests may be needed, for testing the glue that binds all these functions together (e.g., filter_filter is not tested). Also, it would be nice if somebody could review the expected output of the XSS.
As mentioned in comment #3, 2 of the tests currently fail since the HTML comment handling is broken.
Comment #6
wrwrwr CreditAttribution: wrwrwr commentedI've somehow managed to take it over, sorry for that ;) Started with your code but ended with almost a total rewrite:
There are a couple of TODOs:
This is some reasonable amount of work already and could use a review already.
Comment #7
catchwrwrwr, thanks for the work. I only scanned through briefly but a couple of things -
If any tests fail, as you say they should go into their own issue - we don't currently have a way to flag tests which are expected to fail for known bugs within core, so currently they're attached to the patches which fix them. So any failing tests which are commented out should simply be removed from the patch.
Also, partially for this reason, there's already tests for the URL filter over at #161217: URL filter breaks generated href tags - that patch has been stalled for some time, but no need to add extra ones here.
in terms of the patch itself, we don't usually abbreviate variable names - so it'd be breat if $f could be expanded to a real word. Also on first glance it looks like there could be more inline comments - although some of the assertions look fairly self-documenting.
Comment #8
wrwrwr CreditAttribution: wrwrwr commentedThank you for the review. Here's my take two:
Failing tests -- this is the more interesting part -- some of the deficiencies already have an issue, but some are without one:
One thing that doesn't belong here (discovered during testing though and about which I've almost forgot already), is the installation path disclosure due to the database layer throwing an exception when fed with invalid UTF-8 (7.x only).
Any comments, including code style, language corrections and concerning failing tests are welcome.
Comment #9
sunSubscribing
Comment #10
Damien Tournoud CreditAttribution: Damien Tournoud commentedThat's an impressive work. Studying this a while back, I quickly convinced myself that most filters were broken beyond repair. I'm glad that someone came over this initial impression.
Aka; subscribe.
Comment #12
jcnventura CreditAttribution: jcnventura commentedHi,
I have re-rolled the patch from #8 to make it applicable to the latest 7.x-dev. I have executed the tests and they all pass at the moment. I have reviewed them individually and couldn't find a problem in them.
I am removing two tests from the failures and including them in my newest fix for #222926: HTML Corrector filter escapes HTML comments.
João
Comment #13
sunI really like this patch. However, either someone smarter than me or myself needs to have a really thorough look at this patch. I'm not sure whether I agree with all test methods, but I have to think more about it.
Comment #14
catchNo one has looked at this in three months, and jcventura did a thorough review already.
If Dries or webchick need more reviews before commit that's fine - but probably we need to find/ask people individually.
Comment #16
webchickComment #18
lilou CreditAttribution: lilou commentedReroll jcnventura's patch #12 (passed).
Comment #19
pp CreditAttribution: pp commentedI am involved in this issue. #340776: URL filter does not urldecode() the URL for link text
Comment #20
pp CreditAttribution: pp commentedIt work's fine.
pp
Comment #21
webchickHoly frigging crap, these tests are AWESOME!&*^!*@ I'm really, really sorry this has stayed off of my radar for so long. The timing of RTBC queue sprints to testing bot screwups must have been pretty bad. :( Thanks so much for writing these, what a tremendously valuable thing to have!!
There were a few issues with the comments, such as wrapping at 80 chars, being on the same line rather than line above it, and there was a small novel in the middle there about trailing dots which I shortened up, since tests aren't to ask "what if?" questions, but rather to test what's actually there. I'm not about to tell someone who waited like 100 years for a core committer to finally stumble into their patch to fix their comment spacing. ;) So I did that myself.
However, there were enough of these changes and I have enough sleep deprivation that I want to re-upload it and let the testing bot have one last crack at it before committing.
Comment #22
webchickCommitted to HEAD! Woohoo!!! :)
Comment #23
sunThank you! I obviously didn't manage to get back to this issue.
Can we commit a quick follow-up cleaning the coding-style?
Comment #24
webchickWe can!