s9y / Serendipity

A PHP blog software
https://s9y.org
BSD 3-Clause "New" or "Revised" License
203 stars 86 forks source link

question regarding serendipity_uploadSecure() #699

Open mmitch opened 4 years ago

mmitch commented 4 years ago

In the aftermath of c694fb0f4533eacd33423217a7ba21fdc7054d06 I want to donate the test code that I had written for the regular expression as a pull request. Now that I don't test only the regular expression but the whole call to serendipity_uploadSecure(), I have a question:

Basic question: Are the input and output parameters $var allowed to have Windows path separators (backslash \) or not?

There is an initial regular expression that filters out any \, but that might be by error (\\\. could have been meant instead of \., as a dot does not have to be quoted inside of a character group []):

$var = preg_replace('@[^0-9a-z\._/-]@i', '', $var);

Later regular expressions like these two work explicitly on both \ and /, but if all \ are already stripped, the extra step to handle \ is not needed:

$var = preg_replace('@(\.+[/\\\\]+)@', '/', $var);

$var = preg_replace('@(\.[^./\\\]{5})[^./\\\]+$@', '$1', $var);

Then there is other code that only expects or even adds / which could result in mixed slashes if \ were allowed in the initial regular expression:

$var = preg_replace('@(\.+[/\\\\]+)@', '/', $var);

$var = preg_replace('@^(/+)@', '', $var);

if (!empty($var) && substr($var, -1, 1) != '/') {
    $var .= '/';
}

So what is the intention here?

I'll try to include patches for either case in my upcoming Pull Request, but I don't know which way to go.

PS: $strip_paths is meant to remove empty extensions (just a dot) in front of path separators?

mmitch commented 4 years ago

source link for convenience: https://github.com/s9y/Serendipity/blob/master/include/functions_images.inc.php#L2026-L2051