Bug 167696 – Correct "filesHaveSameVolume" predicate

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brent Fulgham]

In <https://trac.webkit.org/changeset/210599> I added a new file check related to disk volume. Unfortunately, I did not realize that this code could get entered with %-encoded strings. Since the implementation of this check involves calling POSIX and Windows API calls that expect "file system style" strings, rather than %-encoded URL-style strings, files with characters that are escaped in URLs would be improperly treated as if they did not exist.

Instead, we need to decode the URL escape sequences (if any) so that we present the underlying operating system with a correct file path.

[Brent Fulgham]

<rdar://problem/30298722>

[Brent Fulgham]

Created attachment 300337 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 300337 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=300337&action=review

r=me if you add tests that call encodeWithURLEscapeSequences() before calling filesHaveSameVolume().

> Tools/TestWebKitAPI/Tests/WebCore/FileSystem.cpp:119
> +    EXPECT_TRUE(filesHaveSameVolume(tempFilePath(), spaceContainingFilePath()));
> +    EXPECT_TRUE(filesHaveSameVolume(spaceContainingFilePath(), bangContainingFilePath()));
> +    EXPECT_TRUE(filesHaveSameVolume(bangContainingFilePath(), quoteContainingFilePath()));

I don't think these tests are testing the new calls to decodeURLEscapeSequences() since they aren't percent-escaped when being passed to filesHaveSameVolume() because openTemporaryFile() doesn't percent-escape the paths.

I think you should add some statements that call encodeWithURLEscapeSequences() first, like this:

    EXPECT_TRUE(filesHaveSameVolume(encodeWithURLEscapeSequences(tempFilePath()), encodeWithURLEscapeSequences(spaceContainingFilePath())));

[Brent Fulgham]

Committed r211502: <http://trac.webkit.org/changeset/211502>

[Alexey Proskuryakov]

Comment on attachment 300337 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=300337&action=review

> Source/WebCore/platform/FileSystem.cpp:240
> +    auto fsRepFileA = fileSystemRepresentation(decodeURLEscapeSequences(fileA));

Decoding here feels wrong to me. Percent encoding is a feature of urls, but a file path is never percent encoded. So I'd expect decoding to happen where the URL is converted to a path, and in fact I'm surprised that this didn't happen automatically. Perhaps the caller didn't use the right URL API?

[Darin Adler]

Comment on attachment 300337 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=300337&action=review

>> Source/WebCore/platform/FileSystem.cpp:240
>> +    auto fsRepFileA = fileSystemRepresentation(decodeURLEscapeSequences(fileA));
> 
> Decoding here feels wrong to me. Percent encoding is a feature of urls, but a file path is never percent encoded. So I'd expect decoding to happen where the URL is converted to a path, and in fact I'm surprised that this didn't happen automatically. Perhaps the caller didn't use the right URL API?

I agree completely. In fact, I came here to write the same comment. FileSystem.h does not take percent-encoded file names. The error is at the call site.

The incorrect code is in SecurityOrigin::SecurityOrigin and SecurityOrigin::canDisplay. Both functions should be calling URL::fileSystemPath rather than URL::path. Some additional thought and testing might be needed to make sure that’s completely right.

[Darin Adler]

Please roll this out.

[Brent Fulgham]

(In reply to comment #7)
> Please roll this out.

This change was landed and already merged to several branches. Due to the coordination and chance for error in rolling this out, I'm going to write a separate related bug that corrects the call site so that this can be merged on top of the original change.