Variable $input is not updated

[alleknalle]

Hi there,

when working locally with WebP Express on Windows I noticed that it gives me some path errors. When digging into the code I think I found the issues.

Sometimes $input is being changed, but the variable is not updated.

Please see: https://github.com/rosell-dk/webp-express/blob/3ec6d035a4539f40ea9abc7edfbe7e59cf0a8b71/lib/classes/SanityCheck.php#L284 https://github.com/rosell-dk/webp-express/blob/3ec6d035a4539f40ea9abc7edfbe7e59cf0a8b71/lib/classes/SanityCheck.php#L185 https://github.com/rosell-dk/webp-express/blob/3ec6d035a4539f40ea9abc7edfbe7e59cf0a8b71/lib/classes/SanityCheck.php#L191

When I add $input = to these lines it works.

I can see that there are more functions where $input is not updated, but it looks like they are not related to my problems, but it's maybe good to check other functions as well.

[rosell-dk]

It is by design that $input isn't updated in the SanityCheck.php class. The job of these functions is not to sanitize, but to check if input is sane. If everything is ok, these functions must return the exact input that they receive. Otherwise, they must throw an appropriate exception. This allows the UI to show what went wrong (ie when doing a test conversion). Some of the functions does in fact alter $input but this is just to avoid false positives on coderisk.com.

Alterering $input is unsafe as it will not be brought into attention that something that looked like an attempt to do evil was processed.

Can you tell me which path errors you got before doing the changes?

[alleknalle]

I'm getting the following error: Path is outside resolved document root

The error is because from $input there are backslashes in the path and in $docRoot (SanityCheck.php line 296) there are only forward slashes.

It looks like the absPath() function is to 'create' a correct path and that it's not really used to check something. Or at least, that's what it should do in my opinion. But since the $input is not updated, the 'correct' path is never used.

If you take a look at function absPathMicrosoftStyle() you see that the last thing that is done is to replace \ with / and then the updated path is returned. But in absPath() $input is not updated with the result of absPathMicrosoftStyle().

[Ruzgfpegk]

Same problem here.

The Bulk Convert window returned the following error for each file:

Path is outside resolved document root (C:\path\to\DocumentRoot) failed

I also tracked the problem down to SanityCheck.php but ended up fixing (probably badly) in another place.

https://github.com/rosell-dk/webp-express/blob/3ec6d035a4539f40ea9abc7edfbe7e59cf0a8b71/lib/classes/SanityCheck.php#L323

In this line, the values are: $input -> C:\path\to\DocumentRoot\wp-content\uploads\path\to\file.jpg $docRootSymLinksExpanded -> C:\path\to\DocumentRoot

But in self::pathBeginsWith(), called by self::pathBeginsWithSymLinksExpanded(), we have:

https://github.com/rosell-dk/webp-express/blob/3ec6d035a4539f40ea9abc7edfbe7e59cf0a8b71/lib/classes/SanityCheck.php#L142

which returns FALSE because $beginsWith is the result of $docRootSymLinksExpanded . '/' and $input doesn't contain any / as it's fully Windows-style.

My quick and dirty fix was to change: https://github.com/rosell-dk/webp-express/blob/3ec6d035a4539f40ea9abc7edfbe7e59cf0a8b71/lib/classes/SanityCheck.php#L323

into

        $separator = FileHelper::isWindows() ? '\\' : '/';
        self::pathBeginsWithSymLinksExpanded($input, $docRootSymLinksExpanded . $separator, $errorMsg);

I just started looking into the code so someone else may have a way better solution than this, but I hope that helps.

[Ruzgfpegk]

PR #405 made, using the class's own isOnMicrosoft() instead of FileHelper::isWindows() and updating the rtrim just above to stay consistent.