Bug 83721

Summary: Framesniffing defense is too aggressive.
Product: WebKit Reporter: Thomas Sepez <tsepez>
Component: WebCore Misc.Assignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, bugzilla, cevans, gns, inferno, jamesr, japhet, jchaffraix, jer.noble, tsepez, webkit.review.bot, xan.lopez
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Test case from Paul.
none
One possible approach for discussion.
none
Another possible patch for discussion.
none
Patch
jamesr: review+
Patch, fix nits. none

Description Thomas Sepez 2012-04-11 14:49:39 PDT
Follow on from bug 73083 comment #49 From Paul Stone 2012-04-10 09:10:34 PST:

Broken testcase (works in Firefox)

I just tested this in the Chrome Canary builds, and it seems that the fix is a bit too agressive. When navigating to a fragment in a cross-origin frame, it prevents the frame itself from scrolling. The frame itself should scroll (there's no leak there, Firefox still allows this), but it should prevent any any ancestor frames from scrolling if they're cross-origin.

I've attached a simple testcase that works in Firefox, but is broken in the Canary build. I think this could break some websites  - for example API documentation that uses frames, where the table-of-contents pane is on a different (sub)domain than the main frame.
Comment 1 Thomas Sepez 2012-04-11 15:08:53 PDT
Created attachment 136760 [details]
Test case from Paul.
Comment 2 Thomas Sepez 2012-04-11 15:12:20 PDT
Created attachment 136761 [details]
One possible approach for discussion.

(Tests still need to be hammered into submission if we go with something like this). 
Appears to work properly against Paul's example.
Comment 3 Adam Barth 2012-04-11 16:40:53 PDT
Comment on attachment 136761 [details]
One possible approach for discussion.

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

> Source/WebCore/rendering/RenderLayer.cpp:1680
> +                        // Block parent scroll one time only.

Why only once?
Comment 4 Thomas Sepez 2012-04-11 16:47:32 PDT
> Why only once?

Only scrolls initiated by fragment navigations in frameloader are unsafe.  Frameloader will set this, but we need some way to clear it out for subsequent use.  Doing so when we hit it seemed pragmatic.
Comment 5 Adam Barth 2012-04-11 16:49:20 PDT
I see, so we set the boundary when we kick off the scroll and then remove it once we hit it?  Hum...  Should we have some sort of ASSERT that makes sure we don't accidentally leave it in?
Comment 6 James Robinson 2012-04-12 14:41:51 PDT
Comment on attachment 136761 [details]
One possible approach for discussion.

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

Other than needing some layout test coverage this seems to be on track.

>> Source/WebCore/rendering/RenderLayer.cpp:1680
>> +                        // Block parent scroll one time only.
> 
> Why only once?

Could we instead clear this flag with a call in FrameLoader::scrollToFragmentWithParentBoundary after view->scrollToFragment() ?
Comment 7 Thomas Sepez 2012-04-12 14:58:04 PDT
> Could we instead clear this flag with a call in FrameLoader::scrollToFragmentWithParentBoundary after view->scrollToFragment() ?

That is much cleaner, and I thought about doing it that way.  But here's why I held off

There's a case in FrameView::maintainScrollPositionAtAnchor() where we take a detour into Layout() rather than falling into the (no-argument form of) FrameView::ScrollToAnchor().  It wasn't clear to me how long this path would postpone the actual scroll.  Could it return to FrameLoader, clear the flag (in your proposal), and then have the scroll implied by the anchor fire some time later (like after some other style sheet load)?


Also, I wanted to log a console message (once) when this occurs, so that anyone opening the error console would know this is deliberate (vs. a silent breakage).  I got the feeling that logging from something like RenderLayer would be seriously frowned upon.  How to arrange for that to happen?
Comment 8 Thomas Sepez 2012-04-13 10:56:30 PDT
Ok.  I don't think I can make the logging happen in a way that matches visual expectations - in Paul's test case, for example, there is a failure to propagate the scroll across the boundary, but we really don't care because the top frame didn't have to scroll.  Short of asking the frame that didn't scroll whether it or its parents would have had to scroll ... seems hard.  And probably too much of a hit for sites that simulate postmessage with fragment navs.

I'll also go with the reset in the FrameLoader as James suggested as the first fix for simplicity.  The path though Layout() may likely be hard to trigger reliably for exploitation.
Comment 9 Thomas Sepez 2012-04-13 12:39:51 PDT
Created attachment 137127 [details]
Another possible patch for discussion.
Comment 10 Thomas Sepez 2012-04-13 16:30:50 PDT
Created attachment 137174 [details]
Patch

Patch plus reworked tests.  Adds a refptr per conversation with Julien.
Comment 11 David Kilzer (:ddkilzer) 2012-04-13 20:49:03 PDT
<rdar://problem/11250009>
Comment 12 James Robinson 2012-04-16 11:26:30 PDT
Comment on attachment 137174 [details]
Patch

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

This looks pretty good to me. One style nitpick.

> Source/WebCore/loader/FrameLoader.cpp:2693
> +    if (boundaryFrame.get())

You can (and WebKit code typically does) null-check smart pointers by just doing:

if (boundaryFrame)

since RefPtr/OwnPtr implement operator bool()

> LayoutTests/http/tests/navigation/anchor-frames-same-origin.html:1
> +<html>

For new tests, we typically add a <!DOCTYPE html> declaration to put the page in standards (as opposed to quirks) mode unless we specifically want to check a quirks behavior. most of the time it doesn't make a difference, but it's useful to have the lack of a doctype mean that there is something quirks-mode-specific going on
Comment 13 Thomas Sepez 2012-04-16 11:38:16 PDT
Created attachment 137369 [details]
Patch, fix nits.

Thanks James, heres' the same patch with the nits cleaned up.
Comment 14 Gustavo Noronha (kov) 2012-04-16 14:03:51 PDT
Comment on attachment 137369 [details]
Patch, fix nits.

Attachment 137369 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/12416240
Comment 15 Thomas Sepez 2012-04-16 16:58:39 PDT
Build error is in dependencies.  If I reset cq=? does it try again?
Comment 16 James Robinson 2012-04-17 10:31:28 PDT
(In reply to comment #15)
> Build error is in dependencies.  If I reset cq=? does it try again?

Sadly no, you have to upload a new patch to get the EWS to run again.
Comment 17 James Robinson 2012-04-17 10:31:54 PDT
Comment on attachment 137369 [details]
Patch, fix nits.

Looks like we're good now
Comment 18 WebKit Review Bot 2012-04-17 12:03:59 PDT
Comment on attachment 137369 [details]
Patch, fix nits.

Clearing flags on attachment: 137369

Committed r114406: <http://trac.webkit.org/changeset/114406>
Comment 19 WebKit Review Bot 2012-04-17 12:04:05 PDT
All reviewed patches have been landed.  Closing bug.
Comment 20 Jer Noble 2012-04-17 22:16:19 PDT
This commit added tests which fail on all WebKit2 test bots, namely http/tests/navigation/anchor-frames-same-origin.html. 

Filed <https://bugs.webkit.org/show_bug.cgi?id=84227> to track the broken test.