Bug 66505 - Implement allow-popups for iframe@sandbox
: Implement allow-popups for iframe@sandbox
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: WebCore Misc.
: 528+ (Nightly build)
: All All
: P2 Normal
Assigned To: Adam Barth
:
Depends on: 71455
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-18 15:47 PDT by WebKit Review Bot
Modified: 2011-11-04 11:15 PDT (History)
6 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description WebKit Review Bot 2011-08-18 15:47:12 PDT
Implement allow-popups for iframe@sandbox
Requested by abarth on #webkit.
Comment 1 Adam Barth 2011-08-19 23:31:57 PDT
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 Adam Barth 2011-11-02 17:50:27 PDT
Created attachment 113410 [details]
Patch
Comment 4 Eric Seidel 2011-11-02 21:33:33 PDT
Comment on attachment 113410 [details]
Patch

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 Adam Barth 2011-11-02 21:37:37 PDT
Yeah, that would be safer.  I guess it depends on whether you're sandboxing legacy content or not.
Comment 6 Adam Barth 2011-11-02 21:39:39 PDT
Committed r99138: <http://trac.webkit.org/changeset/99138>
Comment 7 Csaba Osztrogonác 2011-11-03 03:38:43 PDT
Reopen, because it was rolled out: http://trac.webkit.org/changeset/99162
Comment 8 Csaba Osztrogonác 2011-11-03 03:41:54 PDT
(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 Darin Adler 2011-11-03 09:45:17 PDT
Comment on attachment 113410 [details]
Patch

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 Csaba Osztrogonác 2011-11-03 09:48:09 PDT
Comment on attachment 113410 [details]
Patch

remove r+ from landed and rolled out patch
Comment 11 Adam Barth 2011-11-03 10:42:39 PDT
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 Adam Barth 2011-11-03 11:09:43 PDT
> 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 Adam Barth 2011-11-03 12:27:04 PDT
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 Adam Barth 2011-11-03 12:29:27 PDT
Committed r99228: <http://trac.webkit.org/changeset/99228>
Comment 15 Adam Barth 2011-11-03 17:40:00 PDT
Created attachment 113593 [details]
Patch
Comment 16 Darin Adler 2011-11-04 10:05:22 PDT
Comment on attachment 113593 [details]
Patch

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 Adam Barth 2011-11-04 10:11:09 PDT
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 WebKit Review Bot 2011-11-04 11:15:15 PDT
Comment on attachment 113593 [details]
Patch

Clearing flags on attachment: 113593

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