CleanPath: Allow slashes in path pattern for Windows to be able to handle partly normalized paths

[richard67]

Pull Request for Issue #

Summary of Changes

Fix the regex for Windows so it can handle partly normalized paths like e.g. C:\\xampp\\htdocs\\joomla-cms/tmp, which is the $tmp_path in configuration.php after a new installation of a current staging branch of the CMS on an XAMPP on Windows.

Partly normalized paths like e.g. C:\\xampp\\htdocs\\joomla-cms/tmp are absolutely ok in environments like PHP or like I have it in my job with Java, where paths on any OS are later normalized using "/" as separator (what out Path::clean does).

So we have to stick here to the rules of PHP how it handles Windows paths, and not to the rules of Windows, where a slash in a file or folder name is not allowed.

@zero-24 @nibra If having partly normalized paths like described above is not desired, we can replace the slashes / by backslashes \ at the end of the routine here

filter/src/InputFilter.php

Line 1018 in 872338c

return preg_replace('~\\\\+~', '\\', $source);

by changing it to
return preg_replace('~(\\\\|/)+~', '\\', $source);
Then we would not have 'C:\Documents\Newsletters/tmp' or 'C:\Documents/Newsletters/tmp' but 'C:\Documents\Newsletters\tmp' as result of the 2 new unit tests.

I think we don't need that, i.e. the PR is good as it is, because when appending the update package's file name to the path, the Joomla Update component will anyway use a slash /.

What do you think?

Testing Instructions

Test joomla/joomla-cms#32076 with XAMPP on Windows.

Documentation Changes Required

None.

[richard67]

Merged the tests from #39 into here.

[richard67]

Due to the way how the regex works I have added with my last commit one more test for handling slashes also in on a folder level before the last one.

[richard67]

@nibra Your change on the regex in this PR makes tests fail e.g. for 'C:\Documents\Newsletters\Summer2018.pdf' (test case "windows path"

filter/Tests/InputFilterTest.php

Line 435 in d1bb92b

'windows path' => array(

. You will see that if you debug the regex with a tool like e.g. https://regex101.com/ .

[richard67]

@nibra The regex we have now (without this PR) is wrong anyway because it requires double backslash and so not allows single backslash at certain places, but as you can see the test cases assume that to be valid. The only reason why it worked was that for lucky circumstance we got a partial match for the expression. The right regex in my opinion would be:
^([A-Za-z]:(\\{1,2}|\/))?[A-Za-z0-9_-]+[A-Za-z0-9_\.-]*((\\{1,2}|\/)[A-Za-z0-9_-]+[A-Za-z0-9_\.-]*)*$

[richard67]

@nibra Another mistake in your commit is that it doesn't change the expected result in the unit tests to reflect the replacement of the slashes which you have added.

[richard67]

Will add more test cases to the unit tests soon. Stay tuned.

[richard67]

Now the tests fail but for other reasons than before. It seems the Windows path Documents\Newsletters matches also the Linux pattern and so is handled there without replacement of slashes by backslashes, and so the unit test result is wrong.

[richard67]

Now tests are passing, but for one kind of Windows path Documents\Newsletters the Linux pattern matches. It might be good to move the Windows check before the Linux check.

I will add more test cases, then we might see if something remains to be done.

[richard67]

@nibra I guess I have it for now. Please review again. It might be useful to add more tests for bad paths, but this could be done with some other PR if desired.

[richard67]

@nibra Will you make again a PR for the CMS to update the staging branch? Or shall I do that?

[richard67]

@nibra Ah, and before one of us can make the PR for the CMS to update the staging branch, it needs a new tag or release 1.4.2.

[nibra]

Tagged 1.4.2, upmerged to 2.0-dev, tagged 2.0.0-beta4.
Feel free to update the CMS ;)
Thank you!

[richard67]

Thanks. I'll do the CMS.

[richard67]

PR for the CMS staging branch is joomla/joomla-cms#32206 .

[richard67]

PR for the CMS 4.0-dev branch is joomla/joomla-cms#32207 .

[richard67]

@nibra @zero-24 The PR to update the filter package in J4 has a problem, see joomla/joomla-cms#32207 (comment) . Any idea what could be wrong? Right now I have no clue.