User tests: Successful: Unsuccessful:
Pull Request for Issue #32567 .
This pull request fixes the b/c issue with folders for files and images of com_media starting with something else than a letter a-z (upper or lower case) introduced with a security fix into the 3.9.25 release without introducing back the original security issue.
This is done by removing the check for the 1st character which was introduced to disable both full path disclosure by using a path like ./
or .\
and break out of the parent folder by using ../
or ..\
(depending on your server's OS).
The usage of ./
or .\
is handled instead by adding '.'
to the exclude list for the parent folders:
joomla-cms/libraries/src/Form/Rule/FilePathRule.php
Lines 55 to 56 in 754850e
The breakout from the parent folder by using something like ../
or ..\
(depending on your OS) is already handled by the Path::check($value)
a few lines further down. I've added the comment in line 69 to make that more clear:
joomla-cms/libraries/src/Form/Rule/FilePathRule.php
Lines 69 to 78 in 754850e
In addition to this fix, this PR also fixes the splitting of the value into parts for the exclude list check.
When on a Windows server. you perfectly can set and later use a path to images or files separates with either a Linux or a Windows directory separator.
In case if your site has been migrated from a Windows server e.g. for local development to the final Linux server, you still might have a \
in a path, which would lead to false results when splitting the path for the exclude list check.
Therefore the change from $path = explode('/', $value);
to $path = preg_split('/[\/\\\\]/', $value);
.
.mysecretimages
as root.1mississippi
, 2mississippi
, ...Изображений
, Greece Εικόνες
, ...-dada
, _gaga
, ...Result: Same as for step 3.
./
or .\
and use the "Save" button.../
or contains /../
(replace /
by the actual directory separator of your server's OS) and use the "Save" button./
, e.g. administrator/
, and use the "Save" button.\
, e.g. administrator\
, and use the "Save" button.The exclude list for field "Path to Files Folder" can be found here:
https://github.com/joomla/joomla-cms/blob/staging/administrator/components/com_media/config.xml#L41
The exclude list for field "Path to Images Folder" can be found here:
https://github.com/joomla/joomla-cms/blob/staging/administrator/components/com_media/config.xml#L52
You can't save com_media settings when using a Path to Files Folder
or a Path to Images Folder
not starting with one of the letters a to z (case-insensitive).
This means eg the following is not allowed with 3.9.25 or current staging, which is a b/c break because it was allowed with 3.9.24 and older:
.mysecretimages
as root.-
or _
You can save com_media settings when using a Path to Files Folder
or a Path to Images Folder
not starting with one of the letters a to z (case-insensitive).
You still can't save when one of these paths starts with something like ./
or .\
on any OS or when it contains something like ../
on Linux or ..\
on Windows.
The check for the exclude list works with both kinds of directory separators /
and \
.
None.
Status | New | ⇒ | Pending |
Category | ⇒ | Libraries |
Labels |
Added:
?
|
Are there any unit tests that cover this ?
If there were any unit tests for this I would have extended them by new test cases, but I haven't found any, and creating completely new tests is beyond my knowledge (up to now).
I will write the unit test. Please tell me, which of these do you believe should return false and which true:
./images
/images
.\images
../images
.../images
c:\images
\images
ftp://images
http://images
I will write the unit test. Please tell me, which of these do you believe should return false and which true:
Great, i did go througth the stuff and added a few more.
./images => false
/images = true
.\images => false
../images => false
.../images => false
c:\images = true
\images => false
ftp://images => false
http://images => false
/media => false
/administrator => false
/4711images = true
Yes but that can be covered within the unit test too right?
I will write the unit test. Please tell me, which of these do you believe should return false and which true:
Great, i did go througth the stuff and added a few more.
./images => false
/images = true
.\images => false
../images => false
.../images => false
c:\images = true
\images => false
ftp://images => false
http://images => false
/media => false
/administrator => false
/4711images = true
@zero-24 Some of the results depend on how Path::clean works, and this is different on Linux or Windows. Have you tested the above on both kinds of OS, or is your list just based on your assumption how it should be?
I have tested it on Linux and your list is wrong for some values.
P.S.: And shall absolute paths be allowed at this point? Currently they are not because $path[0]
would be empty, and this is how it should be from my point of view.
I will not implement anything or merge anything into this PR which is based on assumptions only.
My unit test will be based on the current staging commit.
You can then quickly merge it - because all the tests pass.
If the tests no longer pass once you have merged staging into your branch, then you have created a b/c break from staging (or fixed the previous b/c that 3.9.25 caused :-) )
Im pushing in 2 mins...
Have you tested the above on both kinds of OS, or is your list just based on your assumption how it should be?
based on my assumptions I might be wrong in some places.
If the tests no longer pass once you have merged staging into your branch, then you have created a b/c break from staging (or fixed the previous b/c that 3.9.25 caused :-) )
They will break if you have a test for false being returned for the b/c that 3.9.25 caused?
@PhilETaylor You run them on Linux? Or Windows? Our testing environments run on Windows as far as I know, but the unit tests should also work on Linux. And there will be a differente due to current behaviour of Path::check
.
This is why they need to be merged, because then they will get run.
I use a Mac.
Also if the Joomla test suites are only being run on windows, then /facepalm ...
This is why they need to be merged, because then they will get run.
They can be run from command line, too, and as I said, some of your tests might fail on one or the other OS.
@PhilETaylor Beside the above, what's the plan now? It needs to merge your PR before we can test and merge mine?
They can be run from command line, too,
They pass at the command line on my Mac, and it looks like the PR is passing unit testing too.
If the tests pass on one platform and fail on another, it would mean that the rule can return true when it should be false or false when it should be true... that's not a very good rule then.
Thats not the tests fault. Thats the rules fault :-)
It needs to merge your PR before we can test and merge mine?
Thats not my decision. I just submit PR's and Joomla does what Joomla wants with them. The unit test submitted passes at the time it was created against staging so it would be easy for a maintainer to merge it once approved by 2 people - or merged without testing it ;-)
I'll get a Joomla 4 one done too.
If the tests pass on one platform and fail on another, it would mean that the rule can return true when it should be false or false when it should be true... that's not a very good rule then.
Thats not the tests fault. Thats the rules fault :-)
The rule used Path::clean
before this PR and before the security fix, and it's that Path::clean
which behaves different e.g. regarding the check for ..\
or ../
.
So if that is a mistake, it doesn't come from this PR here.
Also if the Joomla test suites are only being run on windows, then /facepalm ...
@PhilETaylor I think I was wrong with that and they are running on Linux, at least some of them.
AppVeyor is running windows.
Build started
2SET PATH=C:\Program Files\OpenSSL;C:\tools\php;%PATH%
3SET COMPOSER_NO_INTERACTION=1
4SET PHP=1
5SET ANSICON=121x90 (121x90)
6git clone -q https://github.com/joomla/joomla-cms.git C:\projects\joomla-cms
7git fetch -q origin +refs/pull/32723/merge:
8git checkout -qf FETCH_HEAD
9Running
.....
AppVeyor is running windows.
Yes, I got it now. Thanks.
So to be clear. Is this still a b/c break? Are there any paths that used to work that will no longer work after this?
So to be clear. Is this still a b/c break? Are there any paths that used to work that will no longer work after this?
No, this here should not be a b/c break so that it will make things not work which work without this PR, and if it is because I've missed something, then I have to fix it.
But it will fix the b/c break from 3.9.25.
applying this PR, against the unit tests (which are configured for staging, eg post 3.9.25 release) they fail:
PHPUnit 4.8.36 by Sebastian Bergmann and contributors.
Warning: The Xdebug extension is not loaded
No code coverage will be generated.
.F...........FFFFFF..FF
Time: 78 ms, Memory: 8.00MB
There were 9 failures:
1) FilePathRuleTest::testRule with data set #1 (false, SimpleXMLElement Object (...), '.images')
Failed asserting that true matches expected false.
/Users/phil/Sites/JOOMLA/joomla3/tests/unit/suites/libraries/cms/form/rule/FilePathRuleTest.php:83
2) FilePathRuleTest::testRule with data set #13 (false, SimpleXMLElement Object (...), '4711images')
Failed asserting that true matches expected false.
/Users/phil/Sites/JOOMLA/joomla3/tests/unit/suites/libraries/cms/form/rule/FilePathRuleTest.php:83
3) FilePathRuleTest::testRule with data set #14 (false, SimpleXMLElement Object (...), '1')
Failed asserting that true matches expected false.
/Users/phil/Sites/JOOMLA/joomla3/tests/unit/suites/libraries/cms/form/rule/FilePathRuleTest.php:83
4) FilePathRuleTest::testRule with data set #15 (false, SimpleXMLElement Object (...), '_')
Failed asserting that true matches expected false.
/Users/phil/Sites/JOOMLA/joomla3/tests/unit/suites/libraries/cms/form/rule/FilePathRuleTest.php:83
5) FilePathRuleTest::testRule with data set #16 (false, SimpleXMLElement Object (...), '*')
Failed asserting that true matches expected false.
/Users/phil/Sites/JOOMLA/joomla3/tests/unit/suites/libraries/cms/form/rule/FilePathRuleTest.php:83
6) FilePathRuleTest::testRule with data set #17 (false, SimpleXMLElement Object (...), '%')
Failed asserting that true matches expected false.
/Users/phil/Sites/JOOMLA/joomla3/tests/unit/suites/libraries/cms/form/rule/FilePathRuleTest.php:83
7) FilePathRuleTest::testRule with data set #18 (false, SimpleXMLElement Object (...), '://foo')
Failed asserting that true matches expected false.
/Users/phil/Sites/JOOMLA/joomla3/tests/unit/suites/libraries/cms/form/rule/FilePathRuleTest.php:83
8) FilePathRuleTest::testRule with data set #21 (false, SimpleXMLElement Object (...), 'Εικόνες')
Failed asserting that true matches expected false.
/Users/phil/Sites/JOOMLA/joomla3/tests/unit/suites/libraries/cms/form/rule/FilePathRuleTest.php:83
9) FilePathRuleTest::testRule with data set #22 (false, SimpleXMLElement Object (...), 'Изображений')
Failed asserting that true matches expected false.
/Users/phil/Sites/JOOMLA/joomla3/tests/unit/suites/libraries/cms/form/rule/FilePathRuleTest.php:83
FAILURES!
Tests: 23, Assertions: 23, Failures: 9.
applying this PR, against the unit tests (which are configured for staging, eg post 3.9.25 release) they fail:
@PhilETaylor All these are the fixes for the b/c introduced with 3.9.25, or not? What do your unit tests do on a 3.9.24?
And this people.... is why Unit tests ARE SO IMPORTANT!
And this people.... is why Unit tests ARE SO IMPORTANT!
@PhilETaylor I've never said the opposite. But as you can see I am not an expert in test (yet).
As said, if you run your tests as they are on a 3.9.24 they should fail in the same way like now when you run them on my PR, right?
In order for Joomla 3.9.24 to pass the unit tests they need to be configured like this:
These all pass like this in Joomla 3.9.24
array(true, $xml, ''),
array(true, $xml, '.images'),
array(true, $xml, './images'),
array(true, $xml, '.\images'),
array(false, $xml, '../images'),
array(false, $xml, '.../images'),
array(true, $xml, 'c:\images'),
array(true, $xml, '\\images'), // Means \images
array(true, $xml, 'ftp://images'),
array(true, $xml, 'http://images'),
array(true, $xml, '/media'),
array(true, $xml, '/administrator'),
array(true, $xml, '/4711images'),
array(true, $xml, '4711images'),
array(true, $xml, '1'),
array(true, $xml, '_'),
array(true, $xml, '*'),
array(true, $xml, '%'),
array(true, $xml, '://foo'),
array(true, $xml, '/4711i/images'),
array(false, $xml, '../4711i/images'),
array(true, $xml, 'Εικόνες'),
array(true, $xml, 'Изображений'),
Then when I apply your PR to Joomla 3.9.24 I get failures:
There were 7 failures:
1) FilePathRuleTest::testRule with data set #2 (true, SimpleXMLElement Object (...), './images')
Failed asserting that false matches expected true.
/Users/phil/Sites/JOOMLA/joomla3/tests/unit/suites/libraries/cms/form/rule/FilePathRuleTest.php:83
2) FilePathRuleTest::testRule with data set #3 (true, SimpleXMLElement Object (...), '.\images')
Failed asserting that false matches expected true.
/Users/phil/Sites/JOOMLA/joomla3/tests/unit/suites/libraries/cms/form/rule/FilePathRuleTest.php:83
3) FilePathRuleTest::testRule with data set #7 (true, SimpleXMLElement Object (...), '\images')
Failed asserting that false matches expected true.
/Users/phil/Sites/JOOMLA/joomla3/tests/unit/suites/libraries/cms/form/rule/FilePathRuleTest.php:83
4) FilePathRuleTest::testRule with data set #10 (true, SimpleXMLElement Object (...), '/media')
Failed asserting that false matches expected true.
/Users/phil/Sites/JOOMLA/joomla3/tests/unit/suites/libraries/cms/form/rule/FilePathRuleTest.php:83
5) FilePathRuleTest::testRule with data set #11 (true, SimpleXMLElement Object (...), '/administrator')
Failed asserting that false matches expected true.
/Users/phil/Sites/JOOMLA/joomla3/tests/unit/suites/libraries/cms/form/rule/FilePathRuleTest.php:83
6) FilePathRuleTest::testRule with data set #12 (true, SimpleXMLElement Object (...), '/4711images')
Failed asserting that false matches expected true.
/Users/phil/Sites/JOOMLA/joomla3/tests/unit/suites/libraries/cms/form/rule/FilePathRuleTest.php:83
7) FilePathRuleTest::testRule with data set #19 (true, SimpleXMLElement Object (...), '/4711i/images')
Failed asserting that false matches expected true.
/Users/phil/Sites/JOOMLA/joomla3/tests/unit/suites/libraries/cms/form/rule/FilePathRuleTest.php:83
FAILURES!
Tests: 23, Assertions: 23, Failures: 7.
This tells me that your PR outputs something different than what Joomla 3.9.24 would have...
So... what you need to do is tell me which of these, because of SECURITY REASONS are no longer allowed, and because of SECURITY REASONS you are choosing to change these from true/false to true/false therefore creating a b/c break.
Because the above is the rules that pass on Joomla 3.9.24, they SHOULD pass on 3.9.25 and 3.9.26 else there has been a b.c break introduced somewhere.
So we know /media and /administator are a b/c break - there is another issue about that with a pending won't fix
on it...
So the others that break are:
./images
.\images
\images
/4711images
/4711i/images
Which Im assuming is you making the security fix.... so it looks like the PR is acceptable, and the tests, once merged can be updated to take that into account.
So the others that break are:
./images
.\images
\images
/4711images
/4711i/imagesWhich Im assuming is you making the security fix.... so it looks like the PR is acceptable, and the tests, once merged can be updated to take that into account.
@PhilETaylor Yes, these are the intended ones by the security fix.
Thanks for your help and analysis.
I agree with you that it makes sense to merge the tests first and then update this one here so one can see by the changes in the tests in this PR what this PR does.
I have tested this item
As per my comments above.
I agree with you that it makes sense to merge the tests first and then update this one here so one can see by the changes in the tests in this PR what this PR does.
remembering that the tests created today by me, are valid against the CURRENT staging, and not a released version (and so although they "pass" they "pass" validating the code added in 3.9.26 :-)...
So yes. 1) Merge the working unit tests 2) merge that into this pr 3) update this PR by toggling the true/false on the tests to make them pass again and push this PR again ...
We need to add changes the test for this PR, I would suggest for testing that testers apply first only the changes to the test and see if tests are failing.
Instructions for testing
tests/unit/suites/libraries/joomla/form/rule/FilePathRuleTest.php
from this PR.libraries/src/Form/Rule/FilePathRule.php
.How the execute the tests?
As of the writing of this comment changes for the test are not within the PR
In some 2 hours I will have some time to update this PR by the new tests and adjust the tests. Please wait with the above testing until l’ve done that.
Category | Libraries | ⇒ | Libraries Unit Tests |
@PhilETaylor I've updated this PR and adjusted the tests.
But I've noticed a mistake in your test cases: They are testing /media
and /administrator
, but these don't fail due to the excluded core folder list, these fail because they start with a directory separator, i.e. absolute path, i.e. $path[0]
is an empty string after the split of $value
. So I'll add tests for media
and administrator
(without /
at the beginning) and the exclude
parameter to the form field as well. You should do the same in your PR #32724 .
Update: No, I was wrong with the above, that change should be made in J4 not now but later when merging up the security fix from staging after this PR has been merged. Sorry for confusion.
Labels |
Added:
?
|
Drone failing now for unrelated reasons. Will restart from time to time until ok.
Ready for testing.
@PhilETaylor Could you test again? Thanks in advance, and again thanks for good team work. Sorry, no need for retest, see comment below.
I've restored @PhilETaylor 's test result because all changes after that test were only for the unit tests, not for the rule code.
All your work looks ok, my only note is that the Joomla 3 test suite's PHPUnit version is incompatible with PHP 8 and so this has not been tested (and actually fails in drone) in PHP 8.
Fatal error: Uncaught Error: Call to undefined function each() in /drone/src/libraries/vendor/phpunit/phpunit/src/Util/Getopt.php:38
That is code for "This PHPUnit version is incompatible with PHP 8" ;-)
All your work looks ok, my only note is that the Joomla 3 test suite's PHPUnit version is incompatible with PHP 8 and so this has not been tested (and actually fails in drone) in PHP 8.
@PhilETaylor But I get it right that this is not related to my PR but also happens on current staging?
Correct. This is not your fault, I was nearly translating that error message into English so that you and others dont waste hours trying to fix the PHP 8 unit testing for Joomla 3. It will never work without upgrading PHPUnit and that then will break all the tests as the current PHPUnit version that is compatible with PHP8 is not compatible with the tests (and older PHP versions).
So, just ignore it.
Hint: I wasted hours trying to "fix" the unfixable PHPUnit error message :)
I have tested this item
Status | Pending | ⇒ | Ready to Commit |
RTC
Status | Ready to Commit | ⇒ | Fixed in Code Base |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2021-03-18 18:45:29 |
Closed_By | ⇒ | rdeutz | |
Labels |
Added:
?
?
Removed: ? |
Thank you @richard67 @alikon
@PhilETaylor Thank you Phil.
I just fell foul of this a-z issue in Joomla 4 - does anyone know where @wilsonge is with merging up from staging the commits?
hmm merging up usually happens after a release. So staging -> 3.10-dev -> 4.0-dev and it seems George merged the latest stuff up 7 days from today.
Following that logic would mean we would wait for 3.9.26 to be released, merged up to 3.10-dev and than merged up to 4.0-dev.
Is it documented anywhere?
What kind of docs are you looking for.
is it still a one man effort?
Merging up stuff is usually done by the release leads so all possible merge conflicts can be reviewed by that person too but sometimes other maintainers take that over too.
Well my concern is things might get missed.
However it seems the commits that I was concerned about were merged after the 3.9.25 was released, so from what you say they should not yet be in Joomla 4.
However it seems the commits that I was concerned about were merged after the 3.9.25 was released, so from what you say they should not yet be in Joomla 4.
Now I'm confused what commits are you concerned about than? When its about this PR here it will be shipped tomorrow with 3.9.26 and has not been merged up yet?
Long story. Im having a bad day :)
This is the perfect candidate for unit tests, being a validation rule, and being previously a security issue... Are there any unit tests that cover this ?