New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing JFilterInput adding byte offsets to character offset #15966

Merged
merged 4 commits into from May 12, 2017

Conversation

Projects
None yet
@ggppdk
Contributor

ggppdk commented May 11, 2017

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

Fixing JFilterInput adding byte offsets to character offset, plus fix…
… an old bug of escapeAttributeValues()
@infograf768

This comment has been minimized.

Show comment
Hide comment
@infograf768

infograf768 May 11, 2017

Member

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

Member

infograf768 commented May 11, 2017

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

@rdeutz

This comment has been minimized.

Show comment
Hide comment
@rdeutz

rdeutz May 11, 2017

Contributor

@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

Contributor

rdeutz commented May 11, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@tonypartridge

tonypartridge May 11, 2017

Contributor

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.

Contributor

tonypartridge commented May 11, 2017

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

This comment has been minimized.

Show comment
Hide comment
@tonypartridge

tonypartridge May 11, 2017

Contributor

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

Contributor

tonypartridge commented May 11, 2017

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

This comment has been minimized.

Show comment
Hide comment
@lyquix-owner

lyquix-owner May 11, 2017

@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

@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

This comment has been minimized.

Show comment
Hide comment
@tonypartridge

tonypartridge May 11, 2017

Contributor

@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.

Contributor

tonypartridge commented May 11, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@photodude

photodude May 11, 2017

Contributor

@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.

Contributor

photodude commented May 11, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@zero-24

zero-24 May 11, 2017

Contributor

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

Contributor

zero-24 commented May 11, 2017

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

This comment has been minimized.

Show comment
Hide comment
@rdeutz

rdeutz May 11, 2017

Contributor

@ggppdk you made a mistake in line 1104

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

$attributeValueToEnd isn't set

Contributor

rdeutz commented May 11, 2017

@ggppdk you made a mistake in line 1104

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

$attributeValueToEnd isn't set

Show outdated Hide outdated 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]);

This comment has been minimized.

@photodude

photodude May 11, 2017

Contributor

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

@photodude

photodude May 11, 2017

Contributor

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

This comment has been minimized.

@ggppdk

ggppdk May 11, 2017

Contributor

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

@ggppdk

ggppdk May 11, 2017

Contributor

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

@photodude

This comment has been minimized.

Show comment
Hide comment
@photodude

photodude May 11, 2017

Contributor

@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

Contributor

photodude commented May 11, 2017

@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

This comment has been minimized.

Show comment
Hide comment
@ggppdk

ggppdk May 11, 2017

Contributor

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

Contributor

ggppdk commented May 11, 2017

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

@PhilETaylor

This comment has been minimized.

Show comment
Hide comment
@PhilETaylor

PhilETaylor May 11, 2017

Contributor

I dont think this fixes everything as the unit tests still fail to finish https://travis-ci.org/joomla/joomla-cms/jobs/231213928

Contributor

PhilETaylor commented May 11, 2017

I dont think this fixes everything as the unit tests still fail to finish https://travis-ci.org/joomla/joomla-cms/jobs/231213928

@PhilETaylor

This comment has been minimized.

Show comment
Hide comment
@PhilETaylor

PhilETaylor May 11, 2017

Contributor

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

for what its worth, my test case 15673.php from https://github.com/PhilETaylor/joomla-cms/blob/6438379dba018e9a3ac72de0c90a17420535eb61/15673.php works with joomla-cms/stable at b3a83cb while it did not work with Joomal 3.7.0 stable, so something has changed in staging already to fix that test case.

Contributor

PhilETaylor commented May 11, 2017

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

for what its worth, my test case 15673.php from https://github.com/PhilETaylor/joomla-cms/blob/6438379dba018e9a3ac72de0c90a17420535eb61/15673.php works with joomla-cms/stable at b3a83cb while it did not work with Joomal 3.7.0 stable, so something has changed in staging already to fix that test case.

@rdeutz

This comment has been minimized.

Show comment
Hide comment
@rdeutz

rdeutz May 11, 2017

Contributor

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

Contributor

rdeutz commented May 11, 2017

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

@PhilETaylor

This comment has been minimized.

Show comment
Hide comment
@PhilETaylor

PhilETaylor May 11, 2017

Contributor

My point is that without my crappy unit test, the current unit tests we had before, that are currently in staging, are failing see: https://travis-ci.org/joomla/joomla-cms/jobs/231213928

..................................................F..F..FF... 2074 / 5777 ( 35%)
............................................................. 2135 / 5777 ( 36%)
.........................F..F..FFFF..F....................... 2196 / 5777 ( 38%)
............................................................. 2257 / 5777 ( 39%)
....F..F..FF................................................. 2318 / 5777 ( 40%)
.....................................F.FFF...FF.............. 2379 / 5777 ( 41%)
............................................................. 2440 / 5777 ( 42%)
............FFFFFF..FFFFFF..FF.F.FFFF

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received
The build has been terminated

So this PR obviously has made things worse than in Joomla 3.7.0, because at least passed the CURRENT set of unit tests...

Contributor

PhilETaylor commented May 11, 2017

My point is that without my crappy unit test, the current unit tests we had before, that are currently in staging, are failing see: https://travis-ci.org/joomla/joomla-cms/jobs/231213928

..................................................F..F..FF... 2074 / 5777 ( 35%)
............................................................. 2135 / 5777 ( 36%)
.........................F..F..FFFF..F....................... 2196 / 5777 ( 38%)
............................................................. 2257 / 5777 ( 39%)
....F..F..FF................................................. 2318 / 5777 ( 40%)
.....................................F.FFF...FF.............. 2379 / 5777 ( 41%)
............................................................. 2440 / 5777 ( 42%)
............FFFFFF..FFFFFF..FF.F.FFFF

No output has been received in the last 10m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received
The build has been terminated

So this PR obviously has made things worse than in Joomla 3.7.0, because at least passed the CURRENT set of unit tests...

@photodude

This comment has been minimized.

Show comment
Hide comment
@photodude

photodude May 11, 2017

Contributor

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

Contributor

photodude commented May 11, 2017

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

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

@ggppdk

This comment has been minimized.

Show comment
Hide comment
@ggppdk

ggppdk May 11, 2017

Contributor

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

Contributor

ggppdk commented May 11, 2017

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

@lyquix-owner

This comment has been minimized.

Show comment
Hide comment
@lyquix-owner

lyquix-owner May 11, 2017

Yes! It worked this time on my end!

Yes! It worked this time on my end!

@rdeutz

This comment has been minimized.

Show comment
Hide comment
@rdeutz

rdeutz May 11, 2017

Contributor

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.

Contributor

rdeutz commented May 11, 2017

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

This comment has been minimized.

Show comment
Hide comment
@infograf768

infograf768 May 11, 2017

Member

do we need new tests?

Member

infograf768 commented May 11, 2017

do we need new tests?

@rdeutz

This comment has been minimized.

Show comment
Hide comment
@rdeutz

rdeutz May 11, 2017

Contributor

@infograf768 let some more people test this

Contributor

rdeutz commented May 11, 2017

@infograf768 let some more people test this

@infograf768

This comment has been minimized.

Show comment
Hide comment
@infograf768

infograf768 May 11, 2017

Member

i meant new unit tests

Member

infograf768 commented May 11, 2017

i meant new unit tests

@rdeutz

This comment has been minimized.

Show comment
Hide comment
@rdeutz

rdeutz May 11, 2017

Contributor

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

Contributor

rdeutz commented May 11, 2017

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

This comment has been minimized.

Show comment
Hide comment
@photodude

photodude May 11, 2017

Contributor

@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

Contributor

photodude commented May 11, 2017

@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

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

@jsubri

This comment has been minimized.

Show comment
Hide comment
@jsubri

jsubri May 12, 2017

Contributor

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

Contributor

jsubri commented May 12, 2017

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

This comment has been minimized.

Show comment
Hide comment
@jsubri

jsubri May 12, 2017

Contributor

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

Contributor

jsubri commented May 12, 2017

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

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

@joomla-cms-bot joomla-cms-bot added the RTC label May 12, 2017

@franz-wohlkoenig

This comment has been minimized.

Show comment
Hide comment
@franz-wohlkoenig

franz-wohlkoenig May 12, 2017

RTC after two successful tests.

RTC after two successful tests.

@wilsonge

This comment has been minimized.

Show comment
Hide comment
@wilsonge

wilsonge May 12, 2017

Contributor

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

Contributor

wilsonge commented May 12, 2017

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

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

4 checks passed

JTracker/HumanTestResults Human Test Results: 2 Successful 0 Failed.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/drone/pr the build was successful
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@joomla-cms-bot joomla-cms-bot removed the RTC label May 12, 2017

@rdeutz

This comment has been minimized.

Show comment
Hide comment
@rdeutz

rdeutz May 12, 2017

Contributor

merged it now we can add test cases later

Contributor

rdeutz commented May 12, 2017

merged it now we can add test cases later

@joomla joomla locked and limited conversation to collaborators May 12, 2017

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.