r210531 broke the Epiphany overview (new tab) page, about:overview aka ephy-about://overview. WebKit no longer loads: * The large Epiphany symbolic icon that's displayed on the overview page when no history is available (first run) * Any of the webpage snapshots I added a WTFLogAlways in SecurityOrigin::canDisplay and also in CachedResourceLoader::canRequest. It prints an error like this for each overview snapshot: File overview does not have same volume as /home/mcatanzaro/.cache/thumbnails/large/f19dd053c0b3baa7ad326f9440c1ac44.png, fail! canRequest: security origin ephy-about:// cannot display file:///home/mcatanzaro/.cache/thumbnails/large/f19dd053c0b3baa7ad326f9440c1ac44.png Note that it wouldn't make sense for "overview" to have an associated volume, since it's a resource served via custom protocol and not a file on disk.
This is going to be a blocker for the GTK+ port. I'm not sure the right thing to do here. Is it acceptable to enforce this check only for file URIs and not all local protocols? I'll submit a patch that does that. The downside of this approach is that it's not hard to image a custom local URI scheme that allows loading untrusted data.
Created attachment 298863 [details] Patch
Comment on attachment 298863 [details] Patch Yes, I think this is fine. The intent with the original change was to limit a local HTML file from pulling in content from other volumes.
Comment on attachment 298863 [details] Patch Should find a way to make a regression test so we won’t break this again.
We already have GTK+ API tests /webkit2/WebKitSecurityManager/file-xhr and /webkit2/WebKitWebContext/uri-scheme in TestWebKitWebContext.cpp. I should add a test that combines the two. That said, /webkit2/WebKitSecurityManager/file-xhr happens to be broken right now, due to r210531, so we did have tests to catch it... they were just platform-specific. It also broke /webkit2/WebKitConsoleMessage/network-error (in TestConsoleMessage.cpp). My patch cannot fix either of them, as those tests do not use custom protocols. So I think my patch here is insufficient. Perhaps we need to be more lenient with our same-volume check. What is the volume of the page loaded by WebPage::loadHTMLString("...", "file:///foo")? Could it possibly be expected to match the volume of a file that does not exist? (The current check fails if both files are not real files on disk.) Or perhaps the test needs to be updated to not use this construction.
(In reply to comment #5) > We already have GTK+ API tests /webkit2/WebKitSecurityManager/file-xhr and > /webkit2/WebKitWebContext/uri-scheme in TestWebKitWebContext.cpp. I should > add a test that combines the two. > > That said, /webkit2/WebKitSecurityManager/file-xhr happens to be broken > right now, due to r210531, so we did have tests to catch it... they were > just platform-specific. It also broke > /webkit2/WebKitConsoleMessage/network-error (in TestConsoleMessage.cpp). My > patch cannot fix either of them, as those tests do not use custom protocols. > > So I think my patch here is insufficient. Perhaps we need to be more lenient > with our same-volume check. What is the volume of the page loaded by > WebPage::loadHTMLString("...", "file:///foo")? Could it possibly be expected > to match the volume of a file that does not exist? (The current check fails > if both files are not real files on disk.) Or perhaps the test needs to be > updated to not use this construction. Yes, we are now always failing when a file doesn't exist. I don't think we should fail here in this case, but a 404 should be returned instead. This is also causing the context menu api to fail, because we use a fake video source too ( file:///movie.ogg)
Hey Carlos, I wrote a patch to do that, but I realized it's not the correct solution. It fixes our tests only if the files do not actually exist on disk. Yeah, that's the usual case, but I think it's not the right thing to do. For example: webkit_web_view_load_html (view, "...", "file:///file-that-does-not-exist"); Say there really is somehow a file /file-that-does-not-exist. In that case, it's not loaded from disk, so this volume check should not apply to it, because the actual HTML content is application-controlled. But the file does still exist on disk, which will screw up the permissions check. Furthermore, using the existence of a file on disk for the permissions testing is not good enough as a malicious file could delete itself from disk in order to bypass the permissions checking.
Comment on attachment 298863 [details] Patch I think we definitely want to restrict this permissions check to the "file" protocol, so this patch is correct, but it is not sufficient.
(In reply to comment #8) > Comment on attachment 298863 [details] > Patch > > I think we definitely want to restrict this permissions check to the "file" > protocol, so this patch is correct, but it is not sufficient. Yes, please, at least push this patch.
I'm not sure we should; we need to fix file protocol as well, this patch does nothing for that, and we might even need to roll out the original change. This needs further discussion.
(In reply to comment #9) > (In reply to comment #8) > > Comment on attachment 298863 [details] > > Patch > > > > I think we definitely want to restrict this permissions check to the "file" > > protocol, so this patch is correct, but it is not sufficient. > > Yes, please, at least push this patch. I'd like to land this patch, too, because I found a few places where we do something similar with local URL schemes that should not fall under this Volume check. We (Apple) should have a test case for this case, and I can do so as a separate (related) patch.
Comment on attachment 298863 [details] Patch Talked with Brent on IRC. We're going to land this patch regardless of what we decide regarding how to handle nonexistent files.
Comment on attachment 298863 [details] Patch Clearing flags on attachment: 298863 Committed r210888: <http://trac.webkit.org/changeset/210888>
All reviewed patches have been landed. Closing bug.
(In reply to comment #12) > Comment on attachment 298863 [details] > Patch > > Talked with Brent on IRC. We're going to land this patch regardless of what > we decide regarding how to handle nonexistent files. Brent, we landed https://trac.webkit.org/changeset/210922 I think that until we discover some broken application, we might not need to do anything else here.
<rdar://problem/30068195>