Bug 66505 - Implement allow-popups for iframe@sandbox
: Implement allow-popups for iframe@sandbox
Status: RESOLVED FIXED
: WebKit
WebCore Misc.
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To:
:
:
: 71455
:
  Show dependency treegraph
 
Reported: 2011-08-18 15:47 PST by
Modified: 2011-11-04 11:15 PST (History)


Attachments
Patch (17.37 KB, patch)
2011-11-02 17:50 PST, Adam Barth
no flags Review Patch | Details | Formatted Diff | Diff
Patch (6.57 KB, patch)
2011-11-03 17:40 PST, Adam Barth
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-08-18 15:47:12 PST
Implement allow-popups for iframe@sandbox
Requested by abarth on #webkit.
------- Comment #1 From 2011-08-19 23:31:57 PST -------
Details about how this is supposed to work:

---8<---
Our current implementation is that popups inherit the same restrictions as their creator. Example:

Parent A sandboxes frame B with "allow-scripts".
Frame B calls window.open(C).
The popup opens for C (normal browser popup blocker still applies).
The popup is sandboxed (unique origin, can't submit forms, etc) but is allowed to execute script.

Because B didn't have "allow-same-origin,"
origin(A) != origin(B) != origin(C)
regardless of their actual origins.

That is to say, B and C are each given a separate unique origin.

In the case that B has "allow-same-origin," then normal (un-sandboxed) origin rules apply for both B and C.

Finally, we do not allow popups to be created that cross integrity levels. So, for example, Internet content sandboxed with "allow-popups" cannot open a popup to an Intranet site.
--->8---

There's also a thread in the HTML bug tracker about this feature.
------- Comment #3 From 2011-11-02 17:50:27 PST -------
Created an attachment (id=113410) [details]
Patch
------- Comment #4 From 2011-11-02 21:33:33 PST -------
(From update of attachment 113410 [details])
I feel kinda mixed about this feature.  It's hard to imagine why you wouldn't just postMessage() back to the main frame.  Pop-ups seem like kinda a strange feature for a sandboxed iframe to want.  But OK.
------- Comment #5 From 2011-11-02 21:37:37 PST -------
Yeah, that would be safer.  I guess it depends on whether you're sandboxing legacy content or not.
------- Comment #6 From 2011-11-02 21:39:39 PST -------
Committed r99138: <http://trac.webkit.org/changeset/99138>
------- Comment #7 From 2011-11-03 03:38:43 PST -------
Reopen, because it was rolled out: http://trac.webkit.org/changeset/99162
------- Comment #8 From 2011-11-03 03:41:54 PST -------
(In reply to comment #7)
> Reopen, because it was rolled out: http://trac.webkit.org/changeset/99162

It made 200+ tests flakey on SL and on Qt too: (and made runtime of layout testing +1 hour)

http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20%28Tests%29/r99148%20%2834414%29/results.html
http://build.webkit.org/results/Qt%20Linux%20Release/r99152%20%2839271%29/results.html
------- Comment #9 From 2011-11-03 09:45:17 PST -------
(From update of attachment 113410 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=113410&action=review

> Source/WebCore/page/SecurityOrigin.cpp:554
> +        while (start < length && isASCIISpace(characters[start]))

Strange to use isASCIISpace here instead of isHTMLSpace.

> Source/WebCore/page/SecurityOrigin.cpp:563
> +        String sandboxToken = String(characters + start, end - start);

This is unnecessarily inefficient in the case of a single token. Better to use the String::substring function rather than always explicitly making a new string which will always require allocating a new buffer and a new StringImpl.

Longer term, better for us to overload the equalIgnoringCase function so that it works directly on a UChar* and thus does not require making a String.
------- Comment #10 From 2011-11-03 09:48:09 PST -------
(From update of attachment 113410 [details])
remove r+ from landed and rolled out patch
------- Comment #11 From 2011-11-03 10:42:39 PST -------
Looks like some sandbox bits got flipped on.  I'm not sure why that didn't happen in the Chromium port, which I was using for testing.  Thanks for rolling out the patch.
------- Comment #12 From 2011-11-03 11:09:43 PST -------
> Strange to use isASCIISpace here instead of isHTMLSpace.

I'll change that in a separate patch so I can add a test.

> > Source/WebCore/page/SecurityOrigin.cpp:563
> > +        String sandboxToken = String(characters + start, end - start);
> 
> This is unnecessarily inefficient in the case of a single token. Better to use the String::substring function rather than always explicitly making a new string which will always require allocating a new buffer and a new StringImpl.

Fixed.

> Longer term, better for us to overload the equalIgnoringCase function so that it works directly on a UChar* and thus does not require making a String.

Yes.
------- Comment #13 From 2011-11-03 12:27:04 PST -------
I'm going to land the bulk of this change, which doesn't actually change behavior.  Then I'm going to work up a smaller patch that actually changes behavior, making sure not to break these tests.
------- Comment #14 From 2011-11-03 12:29:27 PST -------
Committed r99228: <http://trac.webkit.org/changeset/99228>
------- Comment #15 From 2011-11-03 17:40:00 PST -------
Created an attachment (id=113593) [details]
Patch
------- Comment #16 From 2011-11-04 10:05:22 PST -------
(From update of attachment 113593 [details])
This looks OK, but I’m not sure we have enough test coverage. You mention problems with the previous patch and I am having a little trouble seeing the tests that were failing because of those problems, or the new tests that demonstrate them. Maybe it’s just these three tests you are changing here, and I couldn’t connect the dots.
------- Comment #17 From 2011-11-04 10:11:09 PST -------
Oh, the old approach caused many, many tests to fail.  :)

I agree that this change would benefit from more tests.  I'll cook some more up.
------- Comment #18 From 2011-11-04 11:15:15 PST -------
(From update of attachment 113593 [details])
Clearing flags on attachment: 113593

Committed r99301: <http://trac.webkit.org/changeset/99301>
------- Comment #19 From 2011-11-04 11:15:20 PST -------
All reviewed patches have been landed.  Closing bug.