RESOLVED FIXED 167894
Correct File Path Handling in SecurityOrigin and FileSystem 
https://bugs.webkit.org/show_bug.cgi?id=167894
Summary Correct File Path Handling in SecurityOrigin and FileSystem
Brent Fulgham
Reported 2017-02-06 11:35:28 PST
The fix in Bug 167696 was not correct. Rather than adding calls to URL decoding in the FileSystem class, we should be making sure that the callers of these functions are passing well-formed file paths. This change fixes the calls sites in SecurityOrigin to avoid this problem, and removes the improperly URL decoding from FileSystem.cpp.
Attachments
Patch (12.13 KB, patch)
2017-02-06 15:38 PST, Brent Fulgham 		no flags
DetailsFormatted DiffDiff
Patch (removed volume-creation test logic) (9.18 KB, patch)
2017-02-06 17:26 PST, Brent Fulgham 		ap: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2017-02-06 11:36:16 PST
<rdar://problem/30380075>
Radar WebKit Bug Importer
Comment 2 2017-02-06 11:36:24 PST
<rdar://problem/30380080>
Brent Fulgham
Comment 3 2017-02-06 15:38:40 PST
Created attachment 300761 [details] Patch
Alexey Proskuryakov
Comment 4 2017-02-06 16:05:03 PST
Comment on attachment 300761 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=300761&action=review > Tools/TestWebKitAPI/Tests/WebCore/FileSystem.cpp:69 > + system("mkdir resources"); > + system("echo \"<!DOCTYPE html><html><body>Hello</body></html>\" > resources/CrossPartitionFileTest.html"); > + system("hdiutil create otherVolume.dmg -srcfolder resources/ -ov > /dev/null"); > + system("hdiutil attach otherVolume.dmg > /dev/null"); This seems more invasive than I'd like the tests to be. Seems likely to cause more problems that it's worth it.
Brent Fulgham
Comment 5 2017-02-06 16:06:48 PST
(In reply to comment #4) > Comment on attachment 300761 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=300761&action=review > > > Tools/TestWebKitAPI/Tests/WebCore/FileSystem.cpp:69 > > + system("mkdir resources"); > > + system("echo \"<!DOCTYPE html><html><body>Hello</body></html>\" > resources/CrossPartitionFileTest.html"); > > + system("hdiutil create otherVolume.dmg -srcfolder resources/ -ov > /dev/null"); > > + system("hdiutil attach otherVolume.dmg > /dev/null"); > > This seems more invasive than I'd like the tests to be. Seems likely to > cause more problems that it's worth it. Do you have a suggestion for creating multiple volumes during testing? Or do you think the potential downside means we should not test this code path?
Alexey Proskuryakov
Comment 6 2017-02-06 17:08:29 PST
> Or do you think the potential downside means we should not test this code path? That would be my inclination, yes.
Brent Fulgham
Comment 7 2017-02-06 17:26:03 PST
Created attachment 300773 [details] Patch (removed volume-creation test logic)
Brent Fulgham
Comment 8 2017-02-06 17:28:31 PST
Note: The Windows build failure seems to be due to a bad commit to WebCore::CredentialStorage, and not this particular patch.
Brent Fulgham
Comment 9 2017-02-06 17:49:52 PST
Committed r211763: <http://trac.webkit.org/changeset/211763>
Note You need to log in before you can comment on or make changes to this bug.