Fixing JFilterInput adding byte offsets to character offset #15966

ggppdk · 2017-05-11T14:10:04Z

Pull Request for Issue #15673 , thanks to @csthomas for describing the issue

Summary of Changes

When preg_match() is used with flag: PREG_OFFSET_CAPTURE, the return offsets are in bytes

J3.7.0 made changes to use mult-byte strlen inside Class JFilterInput
-- thus the old code is now broken because it adds character lengths to byte lengths

Solution is to get the substring according to its byte length as provided by preg_match()
and then call multi-byte strlen to get its real character length

Testing Instructions

I can not write now, but someone can contribute, and you can also see the test strings given in the issue (e.g. this one by @tonypartridge
#15673 (comment)
https://github.com/joomla/joomla-cms/files/991337/JFilterInputerCleanMaxExeciutionErrorText.txt

Expected result

With patch form saving (e.g. article edit form) without timeouts (and also unit tests pass ...)

Actual result

Use test strings / examples given above to see the timeout because of inifinite loop in JFilterInput

Documentation Changes Required

None

… an old bug of escapeAttributeValues()

infograf768 · 2017-05-11T14:17:19Z

I would be pleased to test, but I do not get the timeout locally on MAMP php 5.4.4

rdeutz · 2017-05-11T14:22:54Z

@photodude, @PhilETaylor could you make a PR to this PR adding the test case so that we have a prove that this here is working, Thanks

tonypartridge · 2017-05-11T14:24:58Z

I have tested this item ✅ successfully on dd82ff2

Great, this has worked on the test scenario where it was previously failing for me.

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/15966.}

tonypartridge · 2017-05-11T14:28:57Z

And to confirm, you can also use:

JRequest::get('request', JREQUEST_ALLOWHTML);

with this fix too. Where it previously failed when a field contained the link text above.

Same with:
$filter = JFilterInput::getInstance(null, null, 1, 1);
$filter->clean($row, 'HTML');

Is also working now.

Many thanks
Tony

lyquix-owner · 2017-05-11T14:47:52Z

@ggppdk I tested this on my environment trying to save an article in FLEXIcontent and I get the following errors:

Notice: Undefined variable: attributeValueToEnd in libraries/joomla/filter/input.php on line 1103 (repeats thousands of times)

followed by error:

Fatal error: Maximum execution time of 60 seconds exceeded in libraries/vendor/joomla/string/src/phputf8/mbstring/core.php on line 41

Using the same save as before: https://gist.github.com/lyquix-owner/c8c189a016c3d99c1008059920a87787

tonypartridge · 2017-05-11T14:55:15Z

@lyquix-owner did you replace the whole file with the one @ggppdk modified?

https://github.com/ggppdk/joomla-cms/blob/dd82ff21aeb1ad70abb1d4ddee4e77e8861d8584/libraries/joomla/filter/input.php
Line 1103 is: // We have a closing quote, convert its byte position to a UTF-8 string length, using non-multibyte substr()

which is not a variable field.

photodude · 2017-05-11T15:28:54Z

@infograf768 to get the time out you need certain combinations of tags and multibyte characters like Russian, Greek, Arabic, etc. There is a content file in issue #15673
linked here as well content.txt which will reproduce the issue. @ggppdk please include this file as part of your testing instructions.

There is a subsection of that content file which has been turned into a unit test at #15810 ( @ggppdk this is the unit test you need to include as mentioned by @rdeutz in #15966 (comment) )
Additionally, I have a few base case additions to the unit tests with multibyte characters open at #15914 and one merged in the framework joomla-framework/filter#24 which should also be included to verify that any changes do not break existing filtering.

zero-24 · 2017-05-11T15:31:54Z

Notice: Undefined variable: attributeValueToEnd in libraries/joomla/filter/input.php on line 1103

Similiar issues with travis

1) JFilterInputTest::testCleanByCallingMember with data set "Kill script" ('', '<img src="javascript:alert();" />', '', 'From specific cases')
Undefined variable: attributeValueToEnd

/home/travis/build/joomla/joomla-cms/tests/unit/core/helper.php:52
/home/travis/build/joomla/joomla-cms/libraries/joomla/filter/input.php:1104
/home/travis/build/joomla/joomla-cms/libraries/joomla/filter/input.php:1198
/home/travis/build/joomla/joomla-cms/libraries/joomla/filter/input.php:824
/home/travis/build/joomla/joomla-cms/libraries/joomla/filter/input.php:809
/home/travis/build/joomla/joomla-cms/libraries/joomla/filter/input.php:790
/home/travis/build/joomla/joomla-cms/libraries/joomla/filter/input.php:772
/home/travis/build/joomla/joomla-cms/libraries/joomla/filter/input.php:439
/home/travis/build/joomla/joomla-cms/tests/unit/suites/libraries/joomla/filter/JFilterInputTest.php:772

https://travis-ci.org/joomla/joomla-cms/jobs/231181384

rdeutz · 2017-05-11T15:33:23Z

@ggppdk you made a mistake in line 1104

'$stringBeforeQuote = substr($attributeValueToEnd, 0, $matches[0][1]);"

$attributeValueToEnd isn't set

photodude · 2017-05-11T15:34:27Z

libraries/joomla/filter/input.php

 			{
-				// We have a closing quote
+				// We have a closing quote, convert its byte position to a UTF-8 string length, using non-multibyte substr()
+				$stringBeforeQuote = substr($attributeValueToEnd, 0, $matches[0][1]);


$attributeValueToEnd is undefined.... did you mean $attributeValueRemainder?

Yes, i meant $attributeValueRemainder, i marginally got some time to make the PR before getting some very much needed sleep, lol

photodude · 2017-05-11T15:36:35Z

@ggppdk does this fix the escaping case for 1>5 as listed in the framework as an issue? see unit test PR at joomla-framework/filter#15

ggppdk · 2017-05-11T15:51:22Z

@rdeutz @lyquix-owner @photodude fixed wrong variable name

rdeutz · 2017-05-11T16:11:37Z

@PhilETaylor I meant the unit tests, added them to my file locally and tests are failing

photodude · 2017-05-11T16:37:03Z

@ggppdk the fixed wrong variable name didn't fix anything. The unit tests now fail to work.

…g comment

ggppdk · 2017-05-11T18:52:26Z

I have fixed it, i hope unit tests will now pass

lyquix-owner · 2017-05-11T18:56:30Z

Yes! It worked this time on my end!

rdeutz · 2017-05-11T20:14:14Z

I have tested this item ✅ successfully on 7679cc2

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/15966.}

infograf768 · 2017-05-11T20:15:39Z

do we need new tests?

rdeutz · 2017-05-11T20:16:51Z

@infograf768 let some more people test this

infograf768 · 2017-05-11T20:17:46Z

i meant new unit tests

rdeutz · 2017-05-11T20:21:51Z

I can merge @photodude tests, so we have some more testing. The real issue is hard to test because you are in an endless loop

photodude · 2017-05-11T21:55:31Z

@infograf768 @rdeutz we need the unit test from #15810 I've been testing this patch and the related unit test on the framework. This seems to solve the issue, but the unit test needs work to be valid.

Here are the results from my tests of this patch with #15810 see link https://travis-ci.org/photodude/filter/jobs/231328081

jsubri · 2017-05-12T09:50:15Z

I have tested this item ✅ successfully on 7679cc2

I've tested successfully with the below test case to reproduce the timeout.
Important is to login with a user member of the Manager group, create a new article cut&paste the below 5 lines:
Test with user group Manager-articles 19 février 2017 mélanie propose

after read more 19 février 2017 mélanie propose
Voilà
./jluc
Then go at the end of the first line add ReadMore (button), Save and/or Save&Close produce the timeout. The patch fix the issue.

_{This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/15966.}
Edited: 5 lines instead of 4

jsubri · 2017-05-12T09:59:26Z

In the above test case, Github skrinked the needed spaces, code available at
https://gist.github.com/jsubri/6d9577cc8b183513d2be9bead005fece

ghost · 2017-05-12T10:01:21Z

RTC after two successful tests.

wilsonge · 2017-05-12T11:29:46Z

@ggppdk please can you merge these regression test cases ggppdk#4 then we can get this merged :)

rdeutz · 2017-05-12T11:39:43Z

merged it now we can add test cases later

Fixing JFilterInput adding byte offsets to character offset, plus fix…

4aa110e

… an old bug of escapeAttributeValues()

joomla-cms-bot added the PR-staging label May 11, 2017

ggppdk mentioned this pull request May 11, 2017

Article can't be saved/updated by the user after 3.7.0 updation #15673

Closed

Fixed getting from wrong sub-string the close quote characters

dd82ff2

photodude reviewed May 11, 2017

View reviewed changes

Fixed wrong variable name

2c98533

This comment was marked as abuse.

Sign in to view

Restore previours behaviour, fixing misleading variable name and wron…

7679cc2

…g comment

ggppdk changed the title ~~Fixing JFilterInput adding byte offsets to character offset, plus fixan old bug of escapeAttributeValues()~~ Fixing JFilterInput adding byte offsets to character offset May 11, 2017

ggppdk changed the title ~~Fixing JFilterInput adding byte offsets to character offset~~ Fixing form saving timeouts, becauce of JFilterInput adding byte offsets to character offset May 12, 2017

joomla-cms-bot changed the title ~~Fixing form saving timeouts, becauce of JFilterInput adding byte offsets to character offset~~ Fixing JFilterInput adding byte offsets to character offset May 12, 2017

joomla-cms-bot added the RTC This Pull Request is Ready To Commit label May 12, 2017

wilsonge mentioned this pull request May 12, 2017

Add test case replicating JFilterInput timeout issue #15810

Closed

rdeutz merged commit ad2bc98 into joomla:staging May 12, 2017

joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label May 12, 2017

joomla locked and limited conversation to collaborators May 12, 2017

zero-24 added this to the Joomla 3.7.1 milestone May 12, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixing JFilterInput adding byte offsets to character offset #15966

Fixing JFilterInput adding byte offsets to character offset #15966

ggppdk commented May 11, 2017 •

edited by joomla-cms-bot

Loading

infograf768 commented May 11, 2017

rdeutz commented May 11, 2017

tonypartridge commented May 11, 2017

tonypartridge commented May 11, 2017

lyquix-owner commented May 11, 2017

tonypartridge commented May 11, 2017

photodude commented May 11, 2017

zero-24 commented May 11, 2017

rdeutz commented May 11, 2017

photodude May 11, 2017

ggppdk May 11, 2017

photodude commented May 11, 2017 •

edited

Loading

ggppdk commented May 11, 2017

This comment was marked as abuse.

This comment was marked as abuse.

rdeutz commented May 11, 2017

This comment was marked as abuse.

photodude commented May 11, 2017

ggppdk commented May 11, 2017

lyquix-owner commented May 11, 2017

rdeutz commented May 11, 2017

infograf768 commented May 11, 2017

rdeutz commented May 11, 2017

infograf768 commented May 11, 2017

rdeutz commented May 11, 2017

photodude commented May 11, 2017 •

edited

Loading

jsubri commented May 12, 2017 •

edited

Loading

jsubri commented May 12, 2017 •

edited

Loading

ghost commented May 12, 2017

wilsonge commented May 12, 2017

rdeutz commented May 12, 2017

Fixing JFilterInput adding byte offsets to character offset #15966

Fixing JFilterInput adding byte offsets to character offset #15966

Conversation

ggppdk commented May 11, 2017 • edited by joomla-cms-bot Loading

Summary of Changes

Testing Instructions

Expected result

Actual result

Documentation Changes Required

infograf768 commented May 11, 2017

rdeutz commented May 11, 2017

tonypartridge commented May 11, 2017

tonypartridge commented May 11, 2017

lyquix-owner commented May 11, 2017

tonypartridge commented May 11, 2017

photodude commented May 11, 2017

zero-24 commented May 11, 2017

rdeutz commented May 11, 2017

photodude May 11, 2017

Choose a reason for hiding this comment

ggppdk May 11, 2017

Choose a reason for hiding this comment

photodude commented May 11, 2017 • edited Loading

ggppdk commented May 11, 2017

This comment was marked as abuse.

This comment was marked as abuse.

rdeutz commented May 11, 2017

This comment was marked as abuse.

photodude commented May 11, 2017

ggppdk commented May 11, 2017

lyquix-owner commented May 11, 2017

rdeutz commented May 11, 2017

infograf768 commented May 11, 2017

rdeutz commented May 11, 2017

infograf768 commented May 11, 2017

rdeutz commented May 11, 2017

photodude commented May 11, 2017 • edited Loading

jsubri commented May 12, 2017 • edited Loading

jsubri commented May 12, 2017 • edited Loading

ghost commented May 12, 2017

wilsonge commented May 12, 2017

rdeutz commented May 12, 2017

ggppdk commented May 11, 2017 •

edited by joomla-cms-bot

Loading

photodude commented May 11, 2017 •

edited

Loading

photodude commented May 11, 2017 •

edited

Loading

jsubri commented May 12, 2017 •

edited

Loading

jsubri commented May 12, 2017 •

edited

Loading