Bug 107963 – Add FrameLoaderClient::didAccessInitialDocument

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Charles Reis]

As part of http://crbug.com/9682, we've been looking for a way to safely display the provisional URL in the address bar when a user navigates to a slow web site in a new tab.  The danger here is that another page (e.g., the opener) can access the initial empty document during the slow navigation.  If that page modifies the new window's contents, it's a URL spoof exploit.  As a result, Chrome currently shows about:blank in these new windows until the navigation commits.

That sort of attack is uncommon, though, so we'd like to show the provisional URL unless we detect that the initial empty document has been accessed.  This is similar to how Opera behaves: it shows the provisional URL until another page tries to modify the initial empty document.  Note that we've explored other options like detecting if the render tree is no longer empty, but that's imperfect: it won't react to permission requests, title changes, audio, etc.

The JavaScript bindings can let us know when the initial empty document is accessed (similar to how it works after document.domain has been modified), at which point we can notify the FrameLoaderClient.  From there, we can show about:blank instead of the provisional URL in the address bar.

[Charles Reis]

Created attachment 184775 [details]
Patch

[Charles Reis]

I've uploaded an initial patch that adds the callback to the V8 bindings.  That could probably use a careful check to see if it's in the right place.  I'm also not sure how to hook it up in JSC, or if there's interest in that.

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Build Bot]

Comment on attachment 184775 [details]
Patch

Attachment 184775 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16113572

New failing tests:
editing/pasteboard/4641033.html

[Sam Weinig]

(In reply to comment #2)
> I've uploaded an initial patch that adds the callback to the V8 bindings.  That could probably use a careful check to see if it's in the right place.  I'm also not sure how to hook it up in JSC, or if there's interest in that.

If it is a good enough idea to go into WebCore, it should be implemented for JSC.  This should also have tests.

[Darin Fisher (:fishd, Google)]

(In reply to comment #5)
> (In reply to comment #2)
> > I've uploaded an initial patch that adds the callback to the V8 bindings.  That could probably use a careful check to see if it's in the right place.  I'm also not sure how to hook it up in JSC, or if there's interest in that.
> 
> If it is a good enough idea to go into WebCore, it should be implemented for JSC.  This should also have tests.

Why do you want unused code?  The current approach adds a little bit of code to FrameLoader to route an event through FrameLoaderClient.  I'd imagine that could be put behind an #ifdef or perhaps factored out.

[Sam Weinig]

(In reply to comment #6)
> (In reply to comment #5)
> > (In reply to comment #2)
> > > I've uploaded an initial patch that adds the callback to the V8 bindings.  That could probably use a careful check to see if it's in the right place.  I'm also not sure how to hook it up in JSC, or if there's interest in that.
> > 
> > If it is a good enough idea to go into WebCore, it should be implemented for JSC.  This should also have tests.
> 
> Why do you want unused code?  The current approach adds a little bit of code to FrameLoader to route an event through FrameLoaderClient.  I'd imagine that could be put behind an #ifdef or perhaps factored out.

I'm not sure I understand.  Are you saying the issue you are solving is somehow specific to Chrome?

[Adam Barth]

> I'm not sure I understand.  Are you saying the issue you are solving is somehow specific to Chrome?

Charlie's patch adds a notification to the embedder, which we plan to use in Chromium for some purpose.  If other embedders of WebKit have need for the same notification, they should feel free to add it themselves.

[Sam Weinig]

(In reply to comment #8)
> > I'm not sure I understand.  Are you saying the issue you are solving is somehow specific to Chrome?
> 
> Charlie's patch adds a notification to the embedder, which we plan to use in Chromium for some purpose.  If other embedders of WebKit have need for the same notification, they should feel free to add it themselves.

Ok.  My main concern here is that this is adding a new FrameLoaderClient function that is not called for those using the WebKit projects JavaScript framework.  That seems like a bad idea.

[Adam Barth]

> Ok.  My main concern here is that this is adding a new FrameLoaderClient function that is not called for those using the WebKit projects JavaScript framework.  That seems like a bad idea.

Would you be willing to withdraw your objection if we put this callback behind an ifdef so as to clearly label that it isn't used by other ports?

[Eric Seidel (no email)]

I think the information that Sam was looking for (but maybe not asking for) is:
- This is to fix a quirky security behavior which Chrome has, which exists to prevent a spoofing attack vector which most other browsers don't bother with, but Chrome does.
- When you click a link in Chrome, it shows "about:blank" during the load.  This is intentional, the navigating page can modify the initial empty document (about:blank) page during load and have the URL bar mislead the user into thinking they're already at the site they mean to be.  I'm dubious that this happens in practice, but Chrome choses to protect against it.
- This security feature doesn't apply to any other WebKIt based browser that I know of, but in order to "fix" this to be more user friendly, Chrome needs to be made aware of any potential accesses to this about:blank page, and fall-back to displaying about:blank during the load.  That notification allows chrome to "lie" to the user (like every other browser does) and show the provisional URL, while being sure that the original page hasn't tampered with the initial empty document.

In short:  I don't think anyone besides chromium cares.  We should just add this flag behind a #ifdef CHROMIUM as abarth suggests (or not, it doesn't really matter).  One could implement this notification for access to the initial empty document via JSC bindings, but since no embedder would need it, it would be dead code.

If Safari wanted to add this security behavior, it would be trivial to do, and they would need to both implement the JSC side of this, as well as the browser logic.  But as noted above, a #ifdef CHROMIUM would prevent any confusion and make sure this dead code is compiled out of other WebKIts.

[Sam Weinig]

Sorry to cause such a fuss, was mainly just overly curious.

[Charles Reis]

Created attachment 191262 [details]
Patch

[Charles Reis]

Sorry for the long delay on this-- I got pulled into some higher priority issues for a while.  I've uploaded a new patch with PLATFORM(CHROMIUM) ifdefs and a test in WebFrameTest.cpp.  Can you take another look?

[Adam Barth]

Comment on attachment 191262 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191262&action=review

LGTM with some nits below.  @Sam: Thoughts?

> Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:405
> +#if PLATFORM(CHROMIUM)

bindings/v8 implies PLATFORM(CHROMIUM).  There's no need for this ifdef in this file.

> Source/WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp:526
> +#if PLATFORM(CHROMIUM)

ditto

> Source/WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp:567
> +#if PLATFORM(CHROMIUM)

ditto

[Charles Reis]

Created attachment 191299 [details]
Patch

[Charles Reis]

(In reply to comment #15)
> (From update of attachment 191262 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191262&action=review
> 
> LGTM with some nits below.  @Sam: Thoughts?
> 
> > Source/WebCore/bindings/v8/V8DOMWindowShell.cpp:405
> > +#if PLATFORM(CHROMIUM)
> 
> bindings/v8 implies PLATFORM(CHROMIUM).  There's no need for this ifdef in this file.
> 
> > Source/WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp:526
> > +#if PLATFORM(CHROMIUM)
> 
> ditto
> 
> > Source/WebCore/bindings/v8/custom/V8DOMWindowCustom.cpp:567
> > +#if PLATFORM(CHROMIUM)
> 
> ditto

Thanks, new patch uploaded.  (I wasn't sure if there were other platforms that used V8.)

[Adam Barth]

Comment on attachment 191299 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191299&action=review

One minor issue below.

> Source/WebCore/loader/FrameLoader.cpp:1594
> +        m_client->didAccessInitialDocument();

It's a bit scary to make this call in the middle of the v8 security check.  I don't think we expect to be re-entered in that state.  Perhaps we should use a timer to make the call asynchronously.

[Charles Reis]

(In reply to comment #18)
> (From update of attachment 191299 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191299&action=review
> 
> One minor issue below.
> 
> > Source/WebCore/loader/FrameLoader.cpp:1594
> > +        m_client->didAccessInitialDocument();
> 
> It's a bit scary to make this call in the middle of the v8 security check.  I don't think we expect to be re-entered in that state.  Perhaps we should use a timer to make the call asynchronously.

Hmm, is there a way to wait for that timer to fire in WebFrameTest?  I haven't found Otherwise the test checks synchronously and fails.

[Charles Reis]

(In reply to comment #19)
> (In reply to comment #18)
> > (From update of attachment 191299 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=191299&action=review
> > 
> > One minor issue below.
> > 
> > > Source/WebCore/loader/FrameLoader.cpp:1594
> > > +        m_client->didAccessInitialDocument();
> > 
> > It's a bit scary to make this call in the middle of the v8 security check.  I don't think we expect to be re-entered in that state.  Perhaps we should use a timer to make the call asynchronously.
> 
> Hmm, is there a way to wait for that timer to fire in WebFrameTest?  I haven't found Otherwise the test checks synchronously and fails.

Nevermind.  Stumbled across runPendingTasks().

[Charles Reis]

Created attachment 191372 [details]
Patch

[Adam Barth]

Comment on attachment 191372 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=191372&action=review

> Source/WebCore/loader/FrameLoader.cpp:1601
> +{

I would have added an ASSERT here that m_didAccessInitialDocument was set, but you don't need to re-spin your patch for that.

> Source/WebKit/chromium/tests/WebFrameTest.cpp:2433
> +    TestAccessInitialDocumentWebFrameClient() : m_didAccessInitialDocument(false)

Technically the : and the characters after that go on a new line.

[WebKit Review Bot]

Comment on attachment 191372 [details]
Patch

Clearing flags on attachment: 191372

Committed r144805: <http://trac.webkit.org/changeset/144805>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.