Bug 66505 – Implement allow-popups for iframe@sandbox

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[WebKit Review Bot]

Implement allow-popups for iframe@sandbox
Requested by abarth on #webkit.

[Adam Barth]

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.

[Adam Barth]

http://www.w3.org/Bugs/Public/show_bug.cgi?id=12393
http://www.w3.org/html/wg/tracker/issues/180

[Adam Barth]

Created attachment 113410 [details]
Patch

[Eric Seidel (no email)]

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.

[Adam Barth]

Yeah, that would be safer.  I guess it depends on whether you're sandboxing legacy content or not.

[Adam Barth]

Committed r99138: <http://trac.webkit.org/changeset/99138>

[Csaba Osztrogonác]

Reopen, because it was rolled out: http://trac.webkit.org/changeset/99162

[Csaba Osztrogonác]

(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

[Darin Adler]

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.

[Csaba Osztrogonác]

Comment on attachment 113410 [details]
Patch

remove r+ from landed and rolled out patch

[Adam Barth]

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.

[Adam Barth]

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

[Adam Barth]

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.

[Adam Barth]

Committed r99228: <http://trac.webkit.org/changeset/99228>

[Adam Barth]

Created attachment 113593 [details]
Patch

[Darin Adler]

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.

[Adam Barth]

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.

[WebKit Review Bot]

Comment on attachment 113593 [details]
Patch

Clearing flags on attachment: 113593

Committed r99301: <http://trac.webkit.org/changeset/99301>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.