Bug 167894

Summary:

Correct File Path Handling in SecurityOrigin and FileSystem

Product:

WebKit

Reporter:

Brent Fulgham <bfulgham>

Component:

WebCore Misc.

Assignee:

Brent Fulgham <bfulgham>

Status:

RESOLVED FIXED

Severity:

Critical

CC:

ap, bfulgham, darin, webkit-bug-importer

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

All

OS:

All

Attachments:

Description	Flags
Patch	none
Patch (removed volume-creation test logic)	ap: review+

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	Details Formatted Diff Diff
Patch (removed volume-creation test logic) (9.18 KB, patch) 2017-02-06 17:26 PST, Brent Fulgham	ap: review+	Details Formatted Diff Diff
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.