Bug 39750 - Allow descendant frame navigation for file URLs when allowFileAccessFromFileURLs is false
Summary: Allow descendant frame navigation for file URLs when allowFileAccessFromFileU...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-05-26 10:24 PDT by Justin Schuh
Modified: 2010-05-29 03:59 PDT (History)
3 users (show)

See Also:


Attachments
Patch (4.60 KB, patch)
2010-05-26 11:31 PDT, Justin Schuh
no flags Details | Formatted Diff | Diff
Patch (4.67 KB, patch)
2010-05-26 12:44 PDT, Justin Schuh
no flags Details | Formatted Diff | Diff
Patch (4.85 KB, patch)
2010-05-28 11:10 PDT, Justin Schuh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Justin Schuh 2010-05-26 10:24:20 PDT
The current restrictions when allowFileAccessFromFileURLs is false break most local HTML documentation pacages. Since it shouldn't add additional risk, we should allow navigation by frames when they're all file URLs and share the same top frame. 

Patch forthcoming.
Comment 1 Justin Schuh 2010-05-26 11:31:42 PDT
Created attachment 57119 [details]
Patch
Comment 2 Adam Barth 2010-05-26 11:57:53 PDT
Comment on attachment 57119 [details]
Patch

WebCore/loader/FrameLoader.cpp:2194
 +          && targetFrame->document()->url().protocolIs("file")) {
We should be getting this information out of the securityOrigin() not out of the document's URL.  Don't we just want to ask if securityOrigin()->isLocal() ?
Comment 3 Justin Schuh 2010-05-26 12:38:07 PDT
(In reply to comment #2)
> (From update of attachment 57119 [details])
> WebCore/loader/FrameLoader.cpp:2194
>  +          && targetFrame->document()->url().protocolIs("file")) {
> We should be getting this information out of the securityOrigin() not out of the document's URL.  Don't we just want to ask if securityOrigin()->isLocal() ?

Yes I suppose we do. Posting an updated patch.
Comment 4 Justin Schuh 2010-05-26 12:44:21 PDT
Created attachment 57125 [details]
Patch
Comment 5 Adam Barth 2010-05-27 15:45:52 PDT
Comment on attachment 57125 [details]
Patch

This isn't quite accurate.  This will allow sibling navigation.  We only want to allow descendants.  What you want to do is move this check into canAccessAncestor  after http://trac.webkit.org/browser/trunk/WebCore/loader/FrameLoader.cpp#L2149.  Basically, you want to fake that the call to |canAccess| there succeeds if both security origins are local.
Comment 6 Justin Schuh 2010-05-27 16:00:59 PDT
(In reply to comment #5)
> (From update of attachment 57125 [details])
> This isn't quite accurate.  This will allow sibling navigation.  

Actually, I'm intentionally enabling sibling navigation. If you look at http://crbug.com/39767 the bug reports we're currently getting are for sibling navigation. So, I want to enable both sibling and ancestor navigation. (Although, I should have used the term "sibling" rather than "peer.")
Comment 7 Adam Barth 2010-05-28 01:10:28 PDT
In the case in that bug, aren't all the frame on the page from the local file system?  In which case, the navigation will be allowed if you put the check where I suggest above.

Put another way, they were happy before we locked down file:// URLs, and we used the descendant navigation policy before the lockdown.  It stands to reason that they'll be happy again if we turn on the descendant navigation policy for local URLs.
Comment 8 Justin Schuh 2010-05-28 09:13:17 PDT
(In reply to comment #7)
> Put another way, they were happy before we locked down file:// URLs, and we used the descendant navigation policy before the lockdown.  It stands to reason that they'll be happy again if we turn on the descendant navigation policy for local URLs.

Just to clarify, I was thinking of it as sibling navigation because the active frame and target are siblings. However, all I really need is ancestor navigation because the location of the target is an attribute of the parent frameset? Is that correct?
Comment 9 Justin Schuh 2010-05-28 11:10:54 PDT
Created attachment 57346 [details]
Patch
Comment 10 Justin Schuh 2010-05-28 11:23:14 PDT
Comment on attachment 57346 [details]
Patch

This version allows a file/local frame to navigate another file/local frame as long as the target frame is a descendant of a file/local frame. I did not check for a shared common ancestor in this iteration because I'm not sure it's necessary. However, I can add that if we want to restrict this type of navigation more.
Comment 11 Justin Schuh 2010-05-28 11:26:53 PDT
(In reply to comment #10)
> (From update of attachment 57346 [details])
> This version allows a file/local frame to navigate another file/local frame as long as the target frame is a descendant of a file/local frame.

Sorry, that line should read "This version allows a file/local frame to navigate a target frame as long as the frame is a descendant of a file/local frame."
Comment 12 Adam Barth 2010-05-28 11:28:31 PDT
Comment on attachment 57346 [details]
Patch

Awesome.  Thanks!
Comment 13 WebKit Commit Bot 2010-05-29 03:59:00 PDT
Comment on attachment 57346 [details]
Patch

Clearing flags on attachment: 57346

Committed r60404: <http://trac.webkit.org/changeset/60404>
Comment 14 WebKit Commit Bot 2010-05-29 03:59:06 PDT
All reviewed patches have been landed.  Closing bug.