167696 2017-02-01 10:14:57 -0800 Correct "filesHaveSameVolume" predicate 2017-02-06 11:32:44 -0800 1 1 1 Unclassified WebKit WebCore Misc. WebKit Nightly Build All All RESOLVED FIXED InRadar P2 Normal --- 1 bfulgham bfulgham achristensen ap bfulgham darin ddkilzer wilander oldest_to_newest 1272186 0 bfulgham 2017-02-01 10:14:57 -0800 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. 1272195 1 bfulgham 2017-02-01 10:41:13 -0800 <rdar://problem/30298722> 1272196 2 300337 bfulgham 2017-02-01 10:46:40 -0800 Created attachment 300337 Patch 1272216 3 300337 ddkilzer 2017-02-01 11:43:04 -0800 Comment on attachment 300337 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()))); 1272222 4 bfulgham 2017-02-01 12:01:43 -0800 Committed r211502: <http://trac.webkit.org/changeset/211502> 1272326 5 300337 ap 2017-02-01 16:25:19 -0800 Comment on attachment 300337 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? 1273353 6 300337 darin 2017-02-04 22:55:24 -0800 Comment on attachment 300337 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. 1273354 7 darin 2017-02-04 22:55:38 -0800 Please roll this out. 1273652 8 bfulgham 2017-02-06 11:32:44 -0800 (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. 300337 2017-02-01 10:46:40 -0800 2017-02-04 22:55:24 -0800 Patch bug-167696-20170201104428.patch text/plain 5978 bfulgham SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDIxMTQ4NCkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIyIEBACisyMDE3LTAyLTAxICBCcmVudCBG dWxnaGFtICA8YmZ1bGdoYW1AYXBwbGUuY29tPgorCisgICAgICAgIENvcnJlY3QgImZpbGVzSGF2 ZVNhbWVWb2x1bWUiIHByZWRpY2F0ZQorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9z aG93X2J1Zy5jZ2k/aWQ9MTY3Njk2CisgICAgICAgIDxyZGFyOi8vcHJvYmxlbS8zMDI5ODcyMj4K KworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBXZSBhcmUg cGFzc2luZyAlLWVuY29kZWQgc3RyaW5ncyB0byB0aGUgdW5kZXJseWluZyBvcGVyYXRpbmcgc3lz dGVtJ3MgZmlsZSBzeXN0ZW0gQVBJcy4KKyAgICAgICAgVGhpcyBkb2Vzbid0IHdvcmsuIEluc3Rl YWQsIHdlIG5lZWQgdG8gcHJlc2VudCBhIGRlY29kZWQgdmVyc2lvbiBvZiB0aGUgZmlsZSBwYXRo CisgICAgICAgIHRoYXQgbWF0Y2hlcyB3aGF0IHRoZSBzeXN0ZW0gQVBJcyBleHBlY3QuCisKKyAg ICAgICAgVGVzdGVkIGJ5IG5ldyBUZXN0V2ViS2l0QVBJIFRlc3QuCisKKyAgICAgICAgKiBwbGF0 Zm9ybS9GaWxlU3lzdGVtLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OmZpbGVzSGF2ZVNhbWVWb2x1 bWUpOiBNYWtlIHN1cmUgdGhlIGZpbGUgcGF0aHMgd2UgZ2l2ZSB0byB0aGUgdW5kZXJseWluZwor ICAgICAgICBvcGVyYXRpbmcgc3lzdGVtIGFyZSBub3QgcGVyY2VudCBlbmNvZGVkLgorICAgICAg ICAqIHBsYXRmb3JtL0ZpbGVTeXN0ZW0uaDogRXhwb3J0ICdmaWxlc0hhdmVTYW1lVm9sdW1lJyBm b3IgdXNlIGJ5IHRlc3Rpbmcgc3lzdGVtLgorCiAyMDE3LTAyLTAxICBSb21haW4gQmVsbGVzc29y dCAgPHJvbWFpbi5iZWxsZXNzb3J0QGNyZi5jYW5vbi5mcj4KIAogICAgICAgICBbUmVhZGFibGUg U3RyZWFtcyBBUEldIEltcGxlbWVudCBSZWFkYWJsZUJ5dGVTdHJlYW1Db250cm9sbGVyIHB1bGwo KQpJbmRleDogU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vRmlsZVN5c3RlbS5jcHAKPT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PQotLS0gU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vRmlsZVN5c3RlbS5jcHAJKHJldmlzaW9uIDIx MTQ4NCkKKysrIFNvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL0ZpbGVTeXN0ZW0uY3BwCSh3b3JraW5n IGNvcHkpCkBAIC0yOCw2ICsyOCw3IEBACiAjaW5jbHVkZSAiRmlsZVN5c3RlbS5oIgogCiAjaW5j bHVkZSAiU2NvcGVHdWFyZC5oIgorI2luY2x1ZGUgIlVSTC5oIgogI2luY2x1ZGUgPHd0Zi9IZXhO dW1iZXIuaD4KICNpbmNsdWRlIDx3dGYvdGV4dC9DU3RyaW5nLmg+CiAjaW5jbHVkZSA8d3RmL3Rl eHQvU3RyaW5nQnVpbGRlci5oPgpAQCAtMjM2LDggKzIzNyw4IEBAIGJvb2wgYXBwZW5kRmlsZUNv bnRlbnRzVG9GaWxlSGFuZGxlKGNvbnMKICAgICAKIGJvb2wgZmlsZXNIYXZlU2FtZVZvbHVtZShj b25zdCBTdHJpbmcmIGZpbGVBLCBjb25zdCBTdHJpbmcmIGZpbGVCKQogewotICAgIGF1dG8gZnNS ZXBGaWxlQSA9IGZpbGVTeXN0ZW1SZXByZXNlbnRhdGlvbihmaWxlQSk7Ci0gICAgYXV0byBmc1Jl cEZpbGVCID0gZmlsZVN5c3RlbVJlcHJlc2VudGF0aW9uKGZpbGVCKTsKKyAgICBhdXRvIGZzUmVw RmlsZUEgPSBmaWxlU3lzdGVtUmVwcmVzZW50YXRpb24oZGVjb2RlVVJMRXNjYXBlU2VxdWVuY2Vz KGZpbGVBKSk7CisgICAgYXV0byBmc1JlcEZpbGVCID0gZmlsZVN5c3RlbVJlcHJlc2VudGF0aW9u KGRlY29kZVVSTEVzY2FwZVNlcXVlbmNlcyhmaWxlQikpOwogICAgIAogICAgIGlmIChmc1JlcEZp bGVBLmlzTnVsbCgpIHx8IGZzUmVwRmlsZUIuaXNOdWxsKCkpCiAgICAgICAgIHJldHVybiBmYWxz ZTsKSW5kZXg6IFNvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL0ZpbGVTeXN0ZW0uaAo9PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 Ci0tLSBTb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9GaWxlU3lzdGVtLmgJKHJldmlzaW9uIDIxMTQ4 NCkKKysrIFNvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL0ZpbGVTeXN0ZW0uaAkod29ya2luZyBjb3B5 KQpAQCAtMTk0LDcgKzE5NCw3IEBAIGJvb2wgdW5sb2FkTW9kdWxlKFBsYXRmb3JtTW9kdWxlKTsK IFdFQkNPUkVfRVhQT1JUIFN0cmluZyBlbmNvZGVGb3JGaWxlTmFtZShjb25zdCBTdHJpbmcmKTsK IFN0cmluZyBkZWNvZGVGcm9tRmlsZW5hbWUoY29uc3QgU3RyaW5nJik7CiAKLWJvb2wgZmlsZXNI YXZlU2FtZVZvbHVtZShjb25zdCBTdHJpbmcmLCBjb25zdCBTdHJpbmcmKTsKK1dFQkNPUkVfRVhQ T1JUIGJvb2wgZmlsZXNIYXZlU2FtZVZvbHVtZShjb25zdCBTdHJpbmcmLCBjb25zdCBTdHJpbmcm KTsKIAogI2lmIFVTRShDRikKIFJldGFpblB0cjxDRlVSTFJlZj4gcGF0aEFzVVJMKGNvbnN0IFN0 cmluZyYpOwpJbmRleDogVG9vbHMvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFRvb2xzL0NoYW5n ZUxvZwkocmV2aXNpb24gMjExNDk4KQorKysgVG9vbHMvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkp CkBAIC0xLDMgKzEsMjAgQEAKKzIwMTctMDItMDEgIEJyZW50IEZ1bGdoYW0gIDxiZnVsZ2hhbUBh cHBsZS5jb20+CisKKyAgICAgICAgQ29ycmVjdCAiZmlsZXNIYXZlU2FtZVZvbHVtZSIgcHJlZGlj YXRlCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNjc2 OTYKKyAgICAgICAgPHJkYXI6Ly9wcm9ibGVtLzMwMjk4NzIyPgorCisgICAgICAgIFJldmlld2Vk IGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIEFkZCBuZXcgdGVzdHMgdGhhdCBjb25maXJt IHRoYXQgdGhlICdmaWxlc0hhdmVTYW1lUGF0aCcgcHJlZGljYXRlIHByb3Blcmx5IGhhbmRsZXMK KyAgICAgICAgcGVyY2VudC1lc2NhcGVkIHBhdGggaW5wdXRzLgorCisgICAgICAgICogVGVzdFdl YktpdEFQSS9UZXN0cy9XZWJDb3JlL0ZpbGVTeXN0ZW0uY3BwOgorICAgICAgICAoVGVzdFdlYktp dEFQSTo6RmlsZVN5c3RlbVRlc3Q6OnNwYWNlQ29udGFpbmluZ0ZpbGVQYXRoKToKKyAgICAgICAg KFRlc3RXZWJLaXRBUEk6OkZpbGVTeXN0ZW1UZXN0OjpiYW5nQ29udGFpbmluZ0ZpbGVQYXRoKToK KyAgICAgICAgKFRlc3RXZWJLaXRBUEk6OkZpbGVTeXN0ZW1UZXN0OjpxdW90ZUNvbnRhaW5pbmdG aWxlUGF0aCk6CisgICAgICAgIChUZXN0V2ViS2l0QVBJOjpURVNUX0YpOgorCiAyMDE3LTAyLTAx ICBUb21hcyBQb3BlbGEgIDx0cG9wZWxhQHJlZGhhdC5jb20+CiAKICAgICAgICAgVW5yZXZpZXdl ZC4gRml4IGNvZGluZyBzdHlsZSBpbiBNaW5pQnJvd3NlcgpJbmRleDogVG9vbHMvVGVzdFdlYktp dEFQSS9UZXN0cy9XZWJDb3JlL0ZpbGVTeXN0ZW0uY3BwCj09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFRvb2xzL1Rl c3RXZWJLaXRBUEkvVGVzdHMvV2ViQ29yZS9GaWxlU3lzdGVtLmNwcAkocmV2aXNpb24gMjExNDg0 KQorKysgVG9vbHMvVGVzdFdlYktpdEFQSS9UZXN0cy9XZWJDb3JlL0ZpbGVTeXN0ZW0uY3BwCSh3 b3JraW5nIGNvcHkpCkBAIC0xLDUgKzEsNiBAQAogLyoKICAqIENvcHlyaWdodCAoQykgMjAxNSBD YW5vbiBJbmMuIEFsbCByaWdodHMgcmVzZXJ2ZWQuCisgKiBDb3B5cmlnaHQgKEMpIDIwMTcgQXBw bGUgSW5jLiBBbGwgcmlnaHRzIHJlc2VydmVkLgogICoKICAqIFJlZGlzdHJpYnV0aW9uIGFuZCB1 c2UgaW4gc291cmNlIGFuZCBiaW5hcnkgZm9ybXMsIHdpdGggb3Igd2l0aG91dAogICogbW9kaWZp Y2F0aW9uLCBhcmUgcGVybWl0dGVkIHByb3ZpZGVkIHRoYXQgdGhlIGZvbGxvd2luZyBjb25kaXRp b25zCkBAIC01MCwyMSArNTEsMzkgQEAgcHVibGljOgogICAgICAgICBjbG9zZUZpbGUoaGFuZGxl KTsgCiAKICAgICAgICAgbV90ZW1wRW1wdHlGaWxlUGF0aCA9IG9wZW5UZW1wb3JhcnlGaWxlKCJ0 ZW1wRW1wdHlUZXN0RmlsZSIsIGhhbmRsZSk7Ci0gICAgICAgIGNsb3NlRmlsZShoYW5kbGUpOyAK LSAgICB9CisgICAgICAgIGNsb3NlRmlsZShoYW5kbGUpOworCisgICAgICAgIG1fc3BhY2VDb250 YWluaW5nRmlsZVBhdGggPSBvcGVuVGVtcG9yYXJ5RmlsZSgidGVtcCBFbXB0eSBUZXN0IEZpbGUi LCBoYW5kbGUpOworICAgICAgICBjbG9zZUZpbGUoaGFuZGxlKTsKKworICAgICAgICBtX2JhbmdD b250YWluaW5nRmlsZVBhdGggPSBvcGVuVGVtcG9yYXJ5RmlsZSgidGVtcCFFbXB0eSFUZXN0IUZp bGUiLCBoYW5kbGUpOworICAgICAgICBjbG9zZUZpbGUoaGFuZGxlKTsKKworICAgICAgICBtX3F1 b3RlQ29udGFpbmluZ0ZpbGVQYXRoID0gb3BlblRlbXBvcmFyeUZpbGUoInRlbXBcIkVtcHR5XCJU ZXN0RmlsZSIsIGhhbmRsZSk7CisgICAgICAgIGNsb3NlRmlsZShoYW5kbGUpOworfQogCiAgICAg dm9pZCBUZWFyRG93bigpIG92ZXJyaWRlCiAgICAgewogICAgICAgICBkZWxldGVGaWxlKG1fdGVt cEZpbGVQYXRoKTsKICAgICAgICAgZGVsZXRlRmlsZShtX3RlbXBFbXB0eUZpbGVQYXRoKTsKKyAg ICAgICAgZGVsZXRlRmlsZShtX3NwYWNlQ29udGFpbmluZ0ZpbGVQYXRoKTsKKyAgICAgICAgZGVs ZXRlRmlsZShtX2JhbmdDb250YWluaW5nRmlsZVBhdGgpOworICAgICAgICBkZWxldGVGaWxlKG1f cXVvdGVDb250YWluaW5nRmlsZVBhdGgpOwogICAgIH0KIAogICAgIGNvbnN0IFN0cmluZyYgdGVt cEZpbGVQYXRoKCkgeyByZXR1cm4gbV90ZW1wRmlsZVBhdGg7IH0KICAgICBjb25zdCBTdHJpbmcm IHRlbXBFbXB0eUZpbGVQYXRoKCkgeyByZXR1cm4gbV90ZW1wRW1wdHlGaWxlUGF0aDsgfQorICAg IGNvbnN0IFN0cmluZyYgc3BhY2VDb250YWluaW5nRmlsZVBhdGgoKSB7IHJldHVybiBtX3NwYWNl Q29udGFpbmluZ0ZpbGVQYXRoOyB9CisgICAgY29uc3QgU3RyaW5nJiBiYW5nQ29udGFpbmluZ0Zp bGVQYXRoKCkgeyByZXR1cm4gbV9iYW5nQ29udGFpbmluZ0ZpbGVQYXRoOyB9CisgICAgY29uc3Qg U3RyaW5nJiBxdW90ZUNvbnRhaW5pbmdGaWxlUGF0aCgpIHsgcmV0dXJuIG1fcXVvdGVDb250YWlu aW5nRmlsZVBhdGg7IH0KIAogcHJpdmF0ZToKICAgICBTdHJpbmcgbV90ZW1wRmlsZVBhdGg7CiAg ICAgU3RyaW5nIG1fdGVtcEVtcHR5RmlsZVBhdGg7CisgICAgU3RyaW5nIG1fc3BhY2VDb250YWlu aW5nRmlsZVBhdGg7CisgICAgU3RyaW5nIG1fYmFuZ0NvbnRhaW5pbmdGaWxlUGF0aDsKKyAgICBT dHJpbmcgbV9xdW90ZUNvbnRhaW5pbmdGaWxlUGF0aDsKIH07CiAKIFRFU1RfRihGaWxlU3lzdGVt VGVzdCwgTWFwcGluZ01pc3NpbmdGaWxlKQpAQCAtOTMsNCArMTEyLDExIEBAIFRFU1RfRihGaWxl U3lzdGVtVGVzdCwgTWFwcGluZ0V4aXN0aW5nRW0KICAgICBFWFBFQ1RfVFJVRSghbWFwcGVkRmls ZURhdGEpOwogfQogCitURVNUX0YoRmlsZVN5c3RlbVRlc3QsIEZpbGVzSGF2ZVNhbWVWb2x1bWUp Cit7CisgICAgRVhQRUNUX1RSVUUoZmlsZXNIYXZlU2FtZVZvbHVtZSh0ZW1wRmlsZVBhdGgoKSwg c3BhY2VDb250YWluaW5nRmlsZVBhdGgoKSkpOworICAgIEVYUEVDVF9UUlVFKGZpbGVzSGF2ZVNh bWVWb2x1bWUoc3BhY2VDb250YWluaW5nRmlsZVBhdGgoKSwgYmFuZ0NvbnRhaW5pbmdGaWxlUGF0 aCgpKSk7CisgICAgRVhQRUNUX1RSVUUoZmlsZXNIYXZlU2FtZVZvbHVtZShiYW5nQ29udGFpbmlu Z0ZpbGVQYXRoKCksIHF1b3RlQ29udGFpbmluZ0ZpbGVQYXRoKCkpKTsKK30KKwogfQo=