Implement allow-popups for iframe@sandbox Requested by abarth on #webkit.
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.
http://www.w3.org/Bugs/Public/show_bug.cgi?id=12393 http://www.w3.org/html/wg/tracker/issues/180
Created attachment 113410 [details] Patch
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.
Yeah, that would be safer. I guess it depends on whether you're sandboxing legacy content or not.
Committed r99138: <http://trac.webkit.org/changeset/99138>
Reopen, because it was rolled out: http://trac.webkit.org/changeset/99162
(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 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 on attachment 113410 [details] Patch remove r+ from landed and rolled out patch
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.
> 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.
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.
Committed r99228: <http://trac.webkit.org/changeset/99228>
Created attachment 113593 [details] Patch
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.
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 on attachment 113593 [details] Patch Clearing flags on attachment: 113593 Committed r99301: <http://trac.webkit.org/changeset/99301>
All reviewed patches have been landed. Closing bug.