issue #88: Prevent infinite looping on empty/short HTML comment #89

TotalWipeOut · 2019-08-15T11:04:43Z

This fixes issue detailed in #88

Prevents infinite loop in short/empty HTML comments
PHPUnit tests

…comment

michalbundyra

@TotalWipeOut Thanks for your contribution.

After your changes the following example is not working correctly:

A <!--My favorite operators are > and <!--> B

After filtering we should have just A B, but with your changes we are getting A only.

This is from: https://www.w3.org/TR/html52/syntax.html#comments

test/StripTagsTest.php

TotalWipeOut · 2019-08-15T11:53:47Z

Thanks for taking a look - Great, leave with me, I will add that to the unit tests and get it working

TotalWipeOut · 2019-08-15T12:09:56Z

@TotalWipeOut Thanks for your contribution.

After your changes the following example is not working correctly:
A  B
After filtering we should have just A B, but with your changes we are getting A only.

This is from: https://www.w3.org/TR/html52/syntax.html#comments

@webimpress Had a look at this, I would suggest this a different bug to this one. The way the code is working is that it strips comments starting from the end of the string, so it finds , which the code is designed to strip to the end of the string - which it does. Fixing this is almost a refactor.
Also, interesting is that running this html snippet on master branch causes and infinite loop - so this particular check has never been run!
I would prefer this issue only fix the infinite loop issue.

UPDATE: @webimpress OK, so I re-wrote the loop that strips HTML comments. Now passing all unit tests including your suggestion. and now no need for a regex...

…ndards compliance

michalbundyra

@TotalWipeOut It looks much better now!
I like that we don't need to use regexp anymore and everything is just simple string operations.

icanhazstring · 2019-08-16T07:35:17Z

test/StripTagsTest.php

+    public function badCommentProvider()
+    {
+        return [
+            ['A <!--> B', 'A '],  // Should be treated as just an open


I love the tests. But just curious, would tests about these scenarios make sense as well:

multiline comments

stacked comments (i know this seems odd, but still it should work A  --> B should return A B

@TotalWipeOut would you be able to add scenarios mentioned above?

I would suggest also: A  C and I believe it should return A C.

@icanhazstring

case with nested comment is invalid (it is not possible to have nested comments).
I agree that we should decide somehow to proceed them, and I would go with the same as modern browser. Just checked the following example on Chrome:

A  D --> E

and as the result I've got:

A D --> E

so not A E as you would expect.

Yes, I will add these extra tests

@webimpress and @icanhazstring We have an issue with this test:
A  D --> E -> A D --> E
After the loop for stripping comments, $value is correctly A D --> E
The following loop strips the lone > so the result is A D -- E which doesn't seem right. But this is existing behaviour

@TotalWipeOut I've checked it, and it looks like desired behaviour. We have tests to cover these scenarios, that > is removed, for example:
testFilterGt: Ensures that any greater-than symbols ‘>’ are removed from text having no tags:

zend-filter/test/StripTagsTest.php

Lines 200 to 211 in 922fe11

/**

* Ensures that any greater-than symbols '>' are removed from text having no tags

*

* @return void

*/

public function testFilterGt()

{

$filter = $this->_filter;

$input = '2 > 1 === true ==> $object->property';

$expected = '2 1 === true == $object-property';

$this->assertEquals($expected, $filter($input));

}

@webimpress totally fine with me to match the filter with modern browser behavior.

@webimpress I agree, it was designed to strip orphan > characters. But I would say that this isn't correct behaviour, considering that strip_tags() and Chrome leave that character untouched.
@icanhazstring If we do that, a lot of tests will fail and need to be re-written.

How do I proceed?

@TotalWipeOut I would keep it for now, as changing it now will be BC Break. We should log another issue and change the behaviour in next major version. So for now - in your test - expected value should be A D -- E as you said.

OK, thanks. I have added the requested tests, 11 in total now 🙂

michalbundyra · 2019-08-16T07:45:33Z

src/StripTags.php

-                $value = '';
+        $open     = '<!--';
+        $openLen  = strlen($open);
+        $close    = '-->';


Not sure why, but before we expect closing tag to match reg exp --\s*>.
Now, we explicitly expecting no any spaces between -- and >.

Do you think expecting some spaces there was a bug? I can't see any other test which fail because of that change, and also I haven't seen anything about these spaces in the specs.

I couldn't see why it was doing that either. Running some additional tests this loops now handles comments in the same way strip_tags does. So i can only think this was a hidden feature/bug?
With the string test the previous version returned test -- -- where as now it returns test

Should the previous behaviour be restored?

Honestly, I think the current behaviour is correct and comply with the html comment spec. Also, as you said, the same way strip_tags behaves, so I think it's right. You can add this test case as well.

OK, cool. Test added

michalbundyra · 2019-08-16T10:39:59Z

test/StripTagsTest.php

+            ['A <!-- --> B', 'A  B'],
+            ['A <!--> B <!--> C', 'A  C'],
+            ['A <!-- -- > -- > -->', 'A '],
+            ["A <!-- B\n C\n D -->", 'A '],


In this two above tests we should have something after --> so we know that not the whole content after opening is removed.

michalbundyra · 2019-08-16T10:44:59Z

@TotalWipeOut It looks good to me, thanks! 👍

@icanhazstring would you mind to have another look, please?

icanhazstring

@TotalWipeOut well done. Thank you 👍

issue #88: Prevent infinite looping on empty/short HTML comment

Forward port #89

michalbundyra · 2019-08-18T19:32:06Z

Thanks, @TotalWipeOut!

issue zendframework#88: Prevent infinite looping on empty/short HTML …

f8a4c92

…comment

michalbundyra suggested changes Aug 15, 2019

View reviewed changes

michalbundyra reviewed Aug 15, 2019

View reviewed changes

test/StripTagsTest.php Outdated Show resolved Hide resolved

TotalWipeOut added 3 commits August 15, 2019 14:38

zendframework#88: Unit tests to use a dataProvider

2ecd932

zendframework#88: refactored loop that strips comments for better sta…

759446f

…ndards compliance

zendframework#88: code tidy

4ddfd2f

michalbundyra approved these changes Aug 15, 2019

View reviewed changes

michalbundyra added the bug label Aug 15, 2019

michalbundyra added this to the 2.9.2 milestone Aug 15, 2019

icanhazstring reviewed Aug 16, 2019

View reviewed changes

michalbundyra reviewed Aug 16, 2019

View reviewed changes

zendframework#88: Extra HTML comment tests

1653fab

michalbundyra reviewed Aug 16, 2019

View reviewed changes

zendframework#88: HTML comment test improvements

0ce8431

icanhazstring approved these changes Aug 16, 2019

View reviewed changes

michalbundyra added a commit that referenced this pull request Aug 18, 2019

Merge pull request #89 from TotalWipeOut/hotfix/88

bf60a41

issue #88: Prevent infinite looping on empty/short HTML comment

michalbundyra added a commit that referenced this pull request Aug 18, 2019

Adds CHANGELOG entry for #89

c8663fa

michalbundyra merged commit 0ce8431 into zendframework:master Aug 18, 2019

michalbundyra added a commit that referenced this pull request Aug 18, 2019

Merge branch 'hotfix/89' into develop

7fa3357

Forward port #89

TotalWipeOut deleted the hotfix/88 branch August 20, 2019 08:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

issue #88: Prevent infinite looping on empty/short HTML comment #89

issue #88: Prevent infinite looping on empty/short HTML comment #89

TotalWipeOut commented Aug 15, 2019 •

edited

Loading

michalbundyra left a comment

TotalWipeOut commented Aug 15, 2019

TotalWipeOut commented Aug 15, 2019 •

edited

Loading

michalbundyra left a comment

icanhazstring Aug 16, 2019

michalbundyra Aug 16, 2019

michalbundyra Aug 16, 2019

TotalWipeOut Aug 16, 2019

TotalWipeOut Aug 16, 2019 •

edited

Loading

michalbundyra Aug 16, 2019

icanhazstring Aug 16, 2019

TotalWipeOut Aug 16, 2019

michalbundyra Aug 16, 2019

TotalWipeOut Aug 16, 2019

michalbundyra Aug 16, 2019

TotalWipeOut Aug 16, 2019 •

edited

Loading

michalbundyra Aug 16, 2019

TotalWipeOut Aug 16, 2019

michalbundyra Aug 16, 2019

TotalWipeOut Aug 16, 2019

michalbundyra commented Aug 16, 2019

icanhazstring left a comment

michalbundyra commented Aug 18, 2019

	/**
	* Ensures that any greater-than symbols '>' are removed from text having no tags
	*
	* @return void
	*/
	public function testFilterGt()
	{
	$filter = $this->_filter;
	$input = '2 > 1 === true ==> $object->property';
	$expected = '2 1 === true == $object-property';
	$this->assertEquals($expected, $filter($input));
	}

issue #88: Prevent infinite looping on empty/short HTML comment #89

issue #88: Prevent infinite looping on empty/short HTML comment #89

Conversation

TotalWipeOut commented Aug 15, 2019 • edited Loading

michalbundyra left a comment

Choose a reason for hiding this comment

TotalWipeOut commented Aug 15, 2019

TotalWipeOut commented Aug 15, 2019 • edited Loading

michalbundyra left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TotalWipeOut Aug 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TotalWipeOut Aug 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michalbundyra commented Aug 16, 2019

icanhazstring left a comment

Choose a reason for hiding this comment

michalbundyra commented Aug 18, 2019

TotalWipeOut commented Aug 15, 2019 •

edited

Loading

TotalWipeOut commented Aug 15, 2019 •

edited

Loading

TotalWipeOut Aug 16, 2019 •

edited

Loading

TotalWipeOut Aug 16, 2019 •

edited

Loading