Block modal dialogs inside sandboxes.

Block modal dialogs inside sandboxes.

This patch adds a runtime flag to block modal dialogs inside sandboxed
documents, and a corresponding `allow-modals` sandbox flag to re-enable
that functionality.

Intent to Implement at [1], discussion on whatwg@ at [2].

[1]: https://groups.google.com/a/chromium.org/d/msg/blink-dev/mXX0AO6Lioo/ual1B_3IqTYJ
[2]: https://lists.w3.org/Archives/Public/public-whatwg-archive/2015May/0035.html

BUG=483624
R=dcheng@chromium.org

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=195791

[Mike West, 2015-05-18 10:29:46]

mkwst@chromium.org changed reviewers:
+ yoav@yoav.ws

[Mike West, 2015-05-18 10:29:47]

WDYT of this patch, Yoav?

dcheng@, FYI. :)

[Mike West, 2015-05-18 11:00:54]

Yoav is apparently on a plane. :)

Daniel, perhaps you can take a look when you wake up?

[Yoav Weiss, 2015-05-18 11:28:29]

A few minutes at the gate :)

https://codereview.chromium.org/1126253007/diff/1/Source/core/frame/LocalDOMW...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/1126253007/diff/1/Source/core/frame/LocalDOMW...
Source/core/frame/LocalDOMWindow.cpp:792: if
(frame()->document()->isSandboxed(SandboxModals)) {
Where is this flag set? I see it defined, but it's not clear to me what sets it
on Document. I'll look deeper into it during my flight :D

[Mike West, 2015-05-18 11:35:31]

On 2015/05/18 at 11:28:29, yoav wrote:
> A few minutes at the gate :)
> 
>
https://codereview.chromium.org/1126253007/diff/1/Source/core/frame/LocalDOMW...
> File Source/core/frame/LocalDOMWindow.cpp (right):
> 
>
https://codereview.chromium.org/1126253007/diff/1/Source/core/frame/LocalDOMW...
> Source/core/frame/LocalDOMWindow.cpp:792: if
(frame()->document()->isSandboxed(SandboxModals)) {
> Where is this flag set? I see it defined, but it's not clear to me what sets
it on Document. I'll look deeper into it during my flight :D

It, like `SandboxedPlugins`, is set when the `sandbox` attribute is parsed in
`parseSandboxPolicy` (which sets the flags to `SandboxAll`, and then turns off
individual flags based on the attribute's value). So, any time a sandboxing
policy is parsed, this flag will be set. If we decide we want to turn that
behavior off (optionally), then I'll add a line to the parsing code that looks
for `allow-modals`.

[Yoav Weiss, 2015-05-18 11:45:38]

On 2015/05/18 11:35:31, Mike West (holiday in DE) wrote:
> On 2015/05/18 at 11:28:29, yoav wrote:
> > A few minutes at the gate :)
> > 
> >
>
https://codereview.chromium.org/1126253007/diff/1/Source/core/frame/LocalDOMW...
> > File Source/core/frame/LocalDOMWindow.cpp (right):
> > 
> >
>
https://codereview.chromium.org/1126253007/diff/1/Source/core/frame/LocalDOMW...
> > Source/core/frame/LocalDOMWindow.cpp:792: if
> (frame()->document()->isSandboxed(SandboxModals)) {
> > Where is this flag set? I see it defined, but it's not clear to me what sets
> it on Document. I'll look deeper into it during my flight :D
> 
> It, like `SandboxedPlugins`, is set when the `sandbox` attribute is parsed in
> `parseSandboxPolicy` (which sets the flags to `SandboxAll`, and then turns off
> individual flags based on the attribute's value). So, any time a sandboxing
> policy is parsed, this flag will be set. If we decide we want to turn that
> behavior off (optionally), then I'll add a line to the parsing code that looks
> for `allow-modals`.

OK, so this is on by default to all sandboxed iframes? Is there a way for them
to opt out of this?

In any case, LGTM with a few nits:
* Can you add a comment about parseSandboxPolicy implicitly turning on these
flags?
* TODO about onbeforeunload?
* Gather feedback about adding a way for sandboxed iframes to opt-out of that
modal blocking. (Don't know myself if there's a legitimate use-case for that)

[Mike West, 2015-05-18 11:49:38]

On 2015/05/18 at 11:45:38, yoav wrote:
> OK, so this is on by default to all sandboxed iframes? Is there a way for them
to opt out of this?

Not in this patch, no. This changes the behavior of sandboxed frames to exclude
these methods.

> In any case, LGTM with a few nits:
> * Can you add a comment about parseSandboxPolicy implicitly turning on these
flags?

Sure.

> * TODO about onbeforeunload?

I can do that.

> * Gather feedback about adding a way for sandboxed iframes to opt-out of that
modal blocking. (Don't know myself if there's a legitimate use-case for that)

I don't think there is. :) I also don't think the numbers will make it
necessary.

I raised this in the intent to implement thread, and on the whatwg thread. I
hope we'll come to consensus there once we have the metrics in.

[Mike West, 2015-05-18 11:50:39]

On 2015/05/18 at 11:49:38, Mike West (holiday in DE) wrote:
> > * Gather feedback about adding a way for sandboxed iframes to opt-out of
that modal blocking. (Don't know myself if there's a legitimate use-case for
that)
> 
> I don't think there is. :) I also don't think the numbers will make it
necessary.

*sigh* ALL OUR SANDBOX TESTS USE ALERT. I'm going to go back in time and bonk
someone on the head.

[dcheng, 2015-05-18 18:59:29]

FYI, this will require updating content/common/frame_replication_state.h as
well, once this rolls into Chrome.

[Mike West, 2015-05-22 13:49:04]

dcheng@: Thanks! I'll throw a patch at you shortly after this lands.
yoav@: Mind taking another look? The layout tests are pretty much all
`s/alert(/console.log(/`. this patch also adds an `allow-modals` flag, in
response to feedback on the ItI.

[Yoav Weiss, 2015-05-22 14:32:24]

Re-LGTM (with nit/thought)

https://codereview.chromium.org/1126253007/diff/80001/Source/core/frame/Local...
File Source/core/frame/LocalDOMWindow.cpp (right):

https://codereview.chromium.org/1126253007/diff/80001/Source/core/frame/Local...
Source/core/frame/LocalDOMWindow.cpp:797: 
Might be a good idea to count the "sandboxed but allowing modals" crowd as well.
Could be in a separate though.

[Mike West, 2015-05-22 15:08:40]

The CQ bit was checked by mkwst@chromium.org

[commit-bot: I haz the power, 2015-05-22 15:08:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1126253007/80001

[Mike West, 2015-05-22 15:09:14]

On 2015/05/22 at 14:32:24, yoav wrote:
> Re-LGTM (with nit/thought)
> 
>
https://codereview.chromium.org/1126253007/diff/80001/Source/core/frame/Local...
> File Source/core/frame/LocalDOMWindow.cpp (right):
> 
>
https://codereview.chromium.org/1126253007/diff/80001/Source/core/frame/Local...
> Source/core/frame/LocalDOMWindow.cpp:797: 
> Might be a good idea to count the "sandboxed but allowing modals" crowd as
well. Could be in a separate though.

Good idea! New patch soonish!

[commit-bot: I haz the power, 2015-05-22 15:32:15]

Committed patchset #5 (id:80001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=195791