Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Status: Active » Needs review
FileSize
25.65 KB

Even I just copied and pasted the tests they are failing for some odd reasons.

Status: Needs review » Needs work

The last submitted patch, drupal-2006568-1.patch, failed testing.

jhedstrom’s picture

Huh, I'm seeing this failure with and without this patch:

HTML scheme clearing evasion -- spaces and metacharacters before scheme.

This is reproducible using the simpletest UI, and as a quick demo, via drush on 8.x head:

jhedstrom@hyperion:~/work/drupal-8/core (8.x) (@d8)☠ drush php-eval "var_dump(filter_xss('<img src=\" &#14;  javascript:alert(0)\">', array('img')));"
string(43) "<img src=" &amp;#14;  javascript:alert(0)">"

The string javascript should not be in that output. Not sure if there is a duplicate for this specific issue or not.

Seems pretty critical...not sure how those fails aren't appearing on the testbot.

jhedstrom’s picture

Status: Needs work » Needs review

#1: drupal-2006568-1.patch queued for re-testing.

jhedstrom’s picture

Queued for retest, as the only thing I can think of is that this test is failing on php 5.4, but not 5.3.

dawehner’s picture

Wow yeah, I have no idea why this could be the case :(

jhedstrom’s picture

Seems this is a known issue for php 5.4: #1210798: In PHP 5.4+, html_entity_decode() doesn't decode invalid numeric entities. Given that, we could probably proceed with this patch, although switching to use dataproviders might simplify the test a bit.

damiankloip’s picture

Not sure if that will make the test easier to read? As the type of assertion varies too. It would then maybe have to be split into diffrerent tests (also an option)?

dawehner’s picture

Just for the security aspect I totally agree that we should have custom messages for all of them, so yeah we don't really gain something by converting to data providers. Sadly it's impossible to commit this conversion, as this will show a broken test each time you run phpunit.

jhedstrom’s picture

Well, you get a broken simpletest as it is now, so I don't really see the difference (with phpunit, at least it's a much faster failure--and it might draw more attention to #1210798: In PHP 5.4+, html_entity_decode() doesn't decode invalid numeric entities which clearly needs to be resolved before Drupal 8 can ship).

dawehner’s picture

Yeah I agree. So what about a review now?

Let's adapt some of the assertions, all the other ones aren't boolean, so they can't be negated.

jhedstrom’s picture

One change I'd recommend is adding a todo to the issue describing the php 5.4 breakage to avoid concussion and lots of duplicate issues. #1210798: In PHP 5.4+, html_entity_decode() doesn't decode invalid numeric entities

jhedstrom’s picture

One change I'd recommend is adding a todo to the issue describing the php 5.4 breakage to avoid concussion and lots of duplicate issues. #1210798: In PHP 5.4+, html_entity_decode() doesn't decode invalid numeric entities

dawehner’s picture

Though as this is a phpunit test it will be runned way more often and annoy a lot of people.

ParisLiakos’s picture

lets check PHP version in setUp and if its 5.4 dont run the tests? or something like that, with a @todo to restore once the issue above is in.
Also it should definitely be moved to providers

ParisLiakos’s picture

ok, lets see whether i messed anything up when converting to data providers
then i ll fix 5.4

jhedstrom’s picture

We should mark the test skipped so the issue doesn't get completely forgotten. http://phpunit.de/manual/3.0/en/incomplete-and-skipped-tests.html

Status: Needs review » Needs work

The last submitted patch, filter_xss_test-2006568-16.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
1.62 KB
28.33 KB

lets see this one

Status: Needs review » Needs work

The last submitted patch, filter_xss_test-2006568-19.patch, failed testing.

jhedstrom’s picture

+++ b/core/tests/Drupal/Tests/Component/Utility/XssTest.phpundefined
@@ -89,11 +89,6 @@ public function testFilterXssNormalized($value, $expected, $message) {
-        "\xc0aaa",
-        '',
-        'HTML filter -- overlong UTF-8 sequences.',
-      ),
-      array(
         "Who&#039;s Online",

Why was this removed?

16 days to next Drupal core point release.

damiankloip’s picture

See providerTestInvalidMultiByte() and testInvalidMultiByte(). It's already tested there. I'm confused by the current providerTestFilterXssNormalized() data, as Xss::filter (or filter_xss from before) would not give you these expected strings?

ParisLiakos’s picture

Status: Needs work » Needs review
FileSize
1.94 KB
28.55 KB

i messed up the argument order:P
this should be green..it also works under 5.4

ParisLiakos’s picture

also, we missed filter_xss_admin tests

jhedstrom’s picture

I would much prefer that we mark the failing test skipped using markTestSkipped() so it isn't entirely forgotten (see #17).

ParisLiakos’s picture

ah, well turns out, that only this dataset is problematic. i think the test should still run for other datasets, no point to skip it

jhedstrom’s picture

Ah, I didn't realize that had to be called for an entire test, rather than a specific data set.

Status: Needs review » Needs work
Issue tags: -PHPUnit

The last submitted patch, filter_xss_test-2006568-24.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
Issue tags: +PHPUnit

#24: filter_xss_test-2006568-24.patch queued for re-testing.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Skipping them now would mean that it will be harder to correct it later, so I think the current approach is fine.

Just to be sure, I ran them locally and they did not fail (which means that the php 5.4 check works).

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 81f4bc8 and pushed to 8.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.

jhedstrom’s picture