Add security checks to scheduled actions

Add security checks to scheduled actions

Scheduled actions end up generating code from strings, so we should put
similar restrictions as for eval() in place

R=haraken@chromium.org,dcheng@chromium.org
BUG=693695, 694446
Review-Url: https://codereview.chromium.org/2702213004
Cr-Commit-Position: refs/heads/master@{#452016}
Committed: https://chromium.googlesource.com/chromium/src/+/0ac9c3a4fbf57ad4cc2f954fc452fa62a729e0e1

[jochen (gone - plz use gerrit), 2017-02-20 14:54:35]

The CQ bit was checked by jochen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-20 14:54:48]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jochen (gone - plz use gerrit), 2017-02-20 15:41:48]

hum, maybe a tad too aggressive

[commit-bot: I haz the power, 2017-02-20 15:49:22]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-20 15:49:22]

Dry run: Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jochen (gone - plz use gerrit), 2017-02-21 10:58:29]

Description was changed from

==========
Add security checks to scheduled actions

Scheduled actions end up generating code from strings, so we should put
similar restrictions as for eval() in place

R=haraken@chromium.org,dcheng@chromium.org
BUG=693695
==========

to

==========
Add security checks to scheduled actions

Scheduled actions end up generating code from strings, so we should put
similar restrictions as for eval() in place

R=haraken@chromium.org,dcheng@chromium.org
BUG=693695,694446
==========

[jochen (gone - plz use gerrit), 2017-02-21 10:58:35]

The CQ bit was checked by jochen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-21 10:58:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jochen (gone - plz use gerrit), 2017-02-21 12:02:34]

The CQ bit was checked by jochen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-21 12:02:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jochen (gone - plz use gerrit), 2017-02-21 12:59:27]

green-ish bots (at least on mac)

PTAL

[haraken, 2017-02-21 13:10:20]

https://codereview.chromium.org/2702213004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScheduledAction.cpp (right):

https://codereview.chromium.org/2702213004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScheduledAction.cpp:50:
ExecutionContext* target,

Is the target execution context different from
scriptState->getExecutionContext()?

[jochen (gone - plz use gerrit), 2017-02-21 13:13:03]

https://codereview.chromium.org/2702213004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScheduledAction.cpp (right):

https://codereview.chromium.org/2702213004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScheduledAction.cpp:50:
ExecutionContext* target,
On 2017/02/21 at 13:10:20, haraken wrote:
> Is the target execution context different from
scriptState->getExecutionContext()?

possibly - you could invoke setTimeout on a same-origin window

[Yuki, 2017-02-21 13:17:57]

yukishiino@chromium.org changed reviewers:
+ yukishiino@chromium.org

[Yuki, 2017-02-21 13:17:57]

LGTM on my side.

https://codereview.chromium.org/2702213004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScheduledAction.h (right):

https://codereview.chromium.org/2702213004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScheduledAction.h:54:
ExecutionContext*,
I'd like to have |target| as the argument name so that it's clear that |target|
may be different from the ScriptState.

[jochen (gone - plz use gerrit), 2017-02-21 13:18:20]

The CQ bit was checked by jochen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-21 13:18:29]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jochen (gone - plz use gerrit), 2017-02-21 13:19:18]

https://codereview.chromium.org/2702213004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScheduledAction.h (right):

https://codereview.chromium.org/2702213004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScheduledAction.h:54:
ExecutionContext*,
On 2017/02/21 at 13:17:57, Yuki wrote:
> I'd like to have |target| as the argument name so that it's clear that
|target| may be different from the ScriptState.

done

[jochen (gone - plz use gerrit), 2017-02-21 13:19:21]

The CQ bit was checked by jochen@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-21 13:19:30]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[haraken, 2017-02-21 13:23:10]

LGTM

https://codereview.chromium.org/2702213004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/ScheduledAction.cpp (right):

https://codereview.chromium.org/2702213004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScheduledAction.cpp:50:
ExecutionContext* target,
On 2017/02/21 13:13:02, jochen wrote:
> On 2017/02/21 at 13:10:20, haraken wrote:
> > Is the target execution context different from
> scriptState->getExecutionContext()?
> 
> possibly - you could invoke setTimeout on a same-origin window

Ah, right.

Maybe would it be nicer to move the CHECK to
ScheduledAction::execute(ExecutionContext* context)? Then you have the target
execution context already. Also we already have a branch for the main thread.

[jochen (gone - plz use gerrit), 2017-02-21 13:26:28]

On 2017/02/21 at 13:23:10, haraken wrote:
> LGTM
> 
>
https://codereview.chromium.org/2702213004/diff/40001/third_party/WebKit/Sour...
> File third_party/WebKit/Source/bindings/core/v8/ScheduledAction.cpp (right):
> 
>
https://codereview.chromium.org/2702213004/diff/40001/third_party/WebKit/Sour...
> third_party/WebKit/Source/bindings/core/v8/ScheduledAction.cpp:50:
ExecutionContext* target,
> On 2017/02/21 13:13:02, jochen wrote:
> > On 2017/02/21 at 13:10:20, haraken wrote:
> > > Is the target execution context different from
> > scriptState->getExecutionContext()?
> > 
> > possibly - you could invoke setTimeout on a same-origin window
> 
> Ah, right.
> 
> Maybe would it be nicer to move the CHECK to
ScheduledAction::execute(ExecutionContext* context)? Then you have the target
execution context already. Also we already have a branch for the main thread.

however, I'd have to store a reference to the entered context then, also not
sure what we should do if that context navigated meanwhile and could then no
longer schedule the action?

[haraken, 2017-02-21 13:28:12]

On 2017/02/21 13:26:28, jochen wrote:
> On 2017/02/21 at 13:23:10, haraken wrote:
> > LGTM
> > 
> >
>
https://codereview.chromium.org/2702213004/diff/40001/third_party/WebKit/Sour...
> > File third_party/WebKit/Source/bindings/core/v8/ScheduledAction.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2702213004/diff/40001/third_party/WebKit/Sour...
> > third_party/WebKit/Source/bindings/core/v8/ScheduledAction.cpp:50:
> ExecutionContext* target,
> > On 2017/02/21 13:13:02, jochen wrote:
> > > On 2017/02/21 at 13:10:20, haraken wrote:
> > > > Is the target execution context different from
> > > scriptState->getExecutionContext()?
> > > 
> > > possibly - you could invoke setTimeout on a same-origin window
> > 
> > Ah, right.
> > 
> > Maybe would it be nicer to move the CHECK to
> ScheduledAction::execute(ExecutionContext* context)? Then you have the target
> execution context already. Also we already have a branch for the main thread.
> 
> however, I'd have to store a reference to the entered context then, also not
> sure what we should do if that context navigated meanwhile and could then no
> longer schedule the action?

Ah, you're right.

[jochen (gone - plz use gerrit), 2017-02-21 14:15:56]

The CQ bit was checked by jochen@chromium.org

[jochen (gone - plz use gerrit), 2017-02-21 14:15:56]

The patchset sent to the CQ was uploaded after l-g-t-m from
yukishiino@chromium.org, haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2702213004/#ps100001
(title: "win baseline")

[commit-bot: I haz the power, 2017-02-21 14:16:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-21 17:01:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-21 17:01:31]

Try jobs failed on following builders:
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[dcheng, 2017-02-21 21:15:11]

https://codereview.chromium.org/2702213004/diff/100001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/testing/Internals.cpp (right):

https://codereview.chromium.org/2702213004/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/testing/Internals.cpp:904: // We need to make the
popup same origin so layout tests can access it.
Uhhhh. Wow.

Btw, should we be calling updateSecurityOrigin() here instead of
setSecurityOrigin()?

[jochen (gone - plz use gerrit), 2017-02-22 10:56:35]

The CQ bit was checked by jochen@chromium.org

[jochen (gone - plz use gerrit), 2017-02-22 10:56:36]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/2702213004/#ps120001
(title: "needs-rebaseline")

[commit-bot: I haz the power, 2017-02-22 10:57:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[jochen (gone - plz use gerrit), 2017-02-22 11:05:26]

The CQ bit was unchecked by jochen@chromium.org

[jochen (gone - plz use gerrit), 2017-02-22 11:07:04]

The CQ bit was checked by jochen@chromium.org

[jochen (gone - plz use gerrit), 2017-02-22 11:07:04]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, yukishiino@chromium.org
Link to the patchset: https://codereview.chromium.org/2702213004/#ps140001
(title: "updates")

[commit-bot: I haz the power, 2017-02-22 11:07:30]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-22 12:50:54]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1487761624336560,
"parent_rev": "c688ec0f1e7fa2ba31238c5e316f738cf29bfc14", "commit_rev":
"0ac9c3a4fbf57ad4cc2f954fc452fa62a729e0e1"}

[commit-bot: I haz the power, 2017-02-22 12:51:30]

Description was changed from

==========
Add security checks to scheduled actions

Scheduled actions end up generating code from strings, so we should put
similar restrictions as for eval() in place

R=haraken@chromium.org,dcheng@chromium.org
BUG=693695,694446
==========

to

==========
Add security checks to scheduled actions

Scheduled actions end up generating code from strings, so we should put
similar restrictions as for eval() in place

R=haraken@chromium.org,dcheng@chromium.org
BUG=693695,694446

Review-Url: https://codereview.chromium.org/2702213004
Cr-Commit-Position: refs/heads/master@{#452016}
Committed:
https://chromium.googlesource.com/chromium/src/+/0ac9c3a4fbf57ad4cc2f954fc452...
==========

[commit-bot: I haz the power, 2017-02-22 12:51:31]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/0ac9c3a4fbf57ad4cc2f954fc452...

[jochen (gone - plz use gerrit), 2017-02-23 18:22:24]

A revert of this CL (patchset #8 id:140001) has been created in
https://codereview.chromium.org/2711163003/ by jochen@chromium.org.

The reason for reverting is: Doesn't take microtasks into account - will reland
with fix.