Bug 39750 – Allow descendant frame navigation for file URLs when allowFileAccessFromFileURLs is false

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Justin Schuh]

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.

[Justin Schuh]

Created attachment 57119 [details]
Patch

[Adam Barth]

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() ?

[Justin Schuh]

(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.

[Justin Schuh]

Created attachment 57125 [details]
Patch

[Adam Barth]

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.

[Justin Schuh]

(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.")

[Adam Barth]

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.

[Justin Schuh]

(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?

[Justin Schuh]

Created attachment 57346 [details]
Patch

[Justin Schuh]

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.

[Justin Schuh]

(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."

[Adam Barth]

Comment on attachment 57346 [details]
Patch

Awesome.  Thanks!

[WebKit Commit Bot]

Comment on attachment 57346 [details]
Patch

Clearing flags on attachment: 57346

Committed r60404: <http://trac.webkit.org/changeset/60404>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.