| Summary: | Add support to isolate file:/// URIs in their own domain by default | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Chris Evans <scarybeasts> | ||||||||||
| Component: | Platform | Assignee: | Chris Evans <scarybeasts> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | abarth, commit-queue, eric, sam | ||||||||||
| Priority: | P2 | ||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||
| Hardware: | PC | ||||||||||||
| OS: | OS X 10.5 | ||||||||||||
| Attachments: |
|
||||||||||||
|
Description
Chris Evans
2010-02-09 17:28:37 PST
Created attachment 48454 [details]
Add support for putting file:/// URIs into their own unique domain
Adam, can you help me land this? It's mostly trivial / tedious wiring. I think I got all the places. Once landed I can get on with the subsequent changes (Chromium wiring and then add the test, which I promise I won't neglect to do :) Kind of ridiculous what it takes to add one bit of state, huh? One minor issue, you need to add new functions at the bottom of the IDL file because of the way virtual functions work in C++. Other than that, looks reasonable. I'm leaving this r? for now in case other folks want to discuss what we're thinking about doing here. We've had some discussion on #webkit in the past, but Chris and my thinking has evolved slightly since then. Assuming your IDL comment refers to IWebPreferencesPrivate.idl -- new patch forthcoming. NOTE - this patch doesn't change the WebKit default behaviour at all. That's left for clients to do if they want additional security for file:/// URIs. Given that this won't affect WebKit clients, but Chromium wants to use this -- I'm in favour of submitting sooner rather than later :) Created attachment 48459 [details]
Updated so new methods are at bottom of IDL file
Can you explain what issues you are trying to mitigate with this new mode? You say your thinking in this area has changed, what changed your mind? > Can you explain what issues you are trying to mitigate with this new mode? The risk is that a user might download an HTML document in an email. When they viewed that document, the document would be able to steal any file on the their hard drive. > You say your thinking in this area has changed, what changed your mind? A couple things: 1) Chris noticed that he was doing the above a bunch and got scared. 2) He wants to try turning this on in a Chrome Dev channel release (the equivalent of the WebKit nightly builds) to see how the community reacts. My guess is that we'll get a bunch of complaints and will need to back off, but I'm not sure where we'll end up. When we discussed this on #webkit, Maciej suggested some kind of "Developer mode" that would have more permissive file:// URL restrictions. This is all very much open for discussion, but having code seemed like a good way to start the discussion. Regarding 1), the threat I'm worried about is HTML attachments (e.g. repro.html) on bugs in the WebKit and Chromium bug databases. I could feasibly get my files stolen. Not good. I'd rather the browser protected me by default. Regarding 2), I really don't expect much kick back. I have added a Chromium command-line flag override for developers that need communication between local files. 99% of our users likely won't notice or care, and could use the extra protection. Well, there's only one way to find out. If we can get this WebKit patch landed, I'll flip the setting in the next Chromium dev channel build and report back. Thanks for the explanation. Comment on attachment 48459 [details]
Updated so new methods are at bottom of IDL file
Please follow this up with a test case.
Yep, test is pending. I need to get this landed, then the Chromium changes landed, before I can land the test without breaking anything. Joy of dependencies :) Comment on attachment 48459 [details] Updated so new methods are at bottom of IDL file Rejecting patch 48459 from commit-queue. Failed to run "['/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/svn-apply', '--reviewer', 'Sam Weinig', '--force']" exit_code: 1 Last 500 characters of output: e WebKitTools/DumpRenderTree/mac/LayoutTestControllerMac.mm Hunk #1 succeeded at 329 (offset 5 lines). patching file WebKitTools/DumpRenderTree/LayoutTestController.cpp Hunk #1 succeeded at 928 (offset 19 lines). Hunk #2 succeeded at 1380 (offset 20 lines). patching file WebKitTools/DumpRenderTree/win/LayoutTestControllerWin.cpp patching file WebKitTools/DumpRenderTree/win/DumpRenderTree.cpp patching file WebKitTools/DumpRenderTree/LayoutTestController.h Hunk #1 succeeded at 69 (offset 1 line). Full output: http://webkit-commit-queue.appspot.com/results/256695 Grr! Clashes with Changeset 54587 for trunk/WebCore/page/SecurityOrigin.cpp by Mr. Barth :) Updating patch in progress.... Created attachment 48544 [details]
Trivial merge with Changeset 54587. Ready to go to commit queue again.
Adam, Sam or Eric: can you r+, cq? the updated patch? The trivial conflict with SecurityOrigin.cpp has been resolved. Comment on attachment 48544 [details]
Trivial merge with Changeset 54587. Ready to go to commit queue again.
The commit-queue spun on this one for a while, making me think there might be something wrong with the patch.
I'm going to re-mark this r? so that the EWS bots run on it. If those pass, i'm happy to mark it cq+
I'm no expert, but I see green on EWS? :) Comment on attachment 48544 [details]
Trivial merge with Changeset 54587. Ready to go to commit queue again.
Yup. lets do this thang.
Comment on attachment 48544 [details]
Trivial merge with Changeset 54587. Ready to go to commit queue again.
Rejecting patch 48544 from commit-queue.
Unexpected failure when landing patch! Please file a bug against webkit-patch.
Failed to run "['WebKitTools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', 'land-attachment', '--force-clean', '--non-interactive', '--no-update', '--parent-command=commit-queue', '--build-style=both', '--quiet', '48544']" exit_code: 1
Last 500 characters of output:
ebkitpy/statusserver.py", line 87, in update_status
return NetworkTransaction().run(lambda: self._post_to_server(queue_name, status, patch, results_file))
File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/networktransaction.py", line 52, in run
self._check_for_timeout()
File "/Users/eseidel/Projects/CommitQueue/WebKitTools/Scripts/webkitpy/networktransaction.py", line 58, in _check_for_timeout
raise NetworkTimeout()
webkitpy.networktransaction.NetworkTimeout
Argh! The latest reject has no "full output" link, so I can't see what might have gone wrong. Looks like a timeout waiting for the network? Can anyone help me land this? Yeah, that's what happened last time. The commit-queue hung for hours trying to post the actual output. I'll attempt to land this manually this afternoon. Sorry for the runaround. Thanks, Eric. No need to apologize; there's probably something weird about my patch if this has happened twice?? I can't see anything obviously wrong, though. The commit-queue failing to upload the results is a commit queue bug. The actual failure is:
Ld /Users/eseidel/Projects/build/Release/WebKit.framework/Versions/A/WebKit normal i386
cd /Users/eseidel/Projects/WebKit/WebKit
setenv MACOSX_DEPLOYMENT_TARGET 10.5
/Developer/usr/bin/g++-4.2 -arch i386 -dynamiclib -L/Users/eseidel/Projects/build/Release -F/Users/eseidel/Projects/build/Release -F/System/Library/Frameworks/WebKit.framework/Versions/A/Frameworks -F/System/Library/Frameworks/ApplicationServices.framework/Frameworks -F/System/Library/Frameworks/Carbon.framework/Frameworks -F/System/Library/Frameworks/Quartz.framework/Frameworks -F/System/Library/PrivateFrameworks -filelist /Users/eseidel/Projects/build/WebKit.build/Release/WebKit.build/Objects-normal/i386/WebKit.LinkFileList -Wl,--no-demangle -exported_symbols_list mac/WebKit.exp -install_name /Users/eseidel/Projects/build/Release/WebKit.framework/Versions/A/WebKit -mmacosx-version-min=10.5 -Wl,-dead_strip -sub_umbrella WebCore -lWebKitSystemInterfaceLeopard -framework Carbon -framework Cocoa -framework JavaScriptCore -licucore -framework OpenGL -framework QuartzCore -framework Security -framework WebCore -Wl,-single_module -compatibility_version 1 -current_version 533.1 -o /Users/eseidel/Projects/build/Release/WebKit.framework/Versions/A/WebKit
Undefined symbols:
"__ZN7WebCore8Settings30setAllowFileAccessFromFileURLsEb", referenced from:
-[WebView(WebPrivate) _preferencesChangedNotification:] in WebView.o
ld: symbol(s) not found
collect2: ld returned 1 exit status
Thanks! Sigh. I'll look at this after the long weekend. Maybe I have a typo. I'm on Chromium Linux so parts of this patch are blind :( You just have to add __ZN7WebCore8Settings30setAllowFileAccessFromFileURLsEb to WebCore.base.exp. Developing WebKit without a Mac is a minefield. :) Created attachment 48863 [details]
Export new WebCore function to fix Mac link error
Comment on attachment 48863 [details]
Export new WebCore function to fix Mac link error
ok, let's give it a spin
Comment on attachment 48863 [details] Export new WebCore function to fix Mac link error Clearing flags on attachment: 48863 Committed r54873: <http://trac.webkit.org/changeset/54873> All reviewed patches have been landed. Closing bug. |