binding: Creates a reject promise always in the current realm.

binding: Creates a reject promise always in the current realm.

Regular promises are created in the relevant real of the context
object.  However, reject promises are special, they must be
created in the current realm as same as exceptions must be
created in the current realm.

BUG=656274
Committed: https://crrev.com/9e04d69152b9c2ac7b4e395213c0e19c52e69bcc
Cr-Commit-Position: refs/heads/master@{#426171}

[Yuki, 2016-10-18 12:02:59]

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

[commit-bot: I haz the power, 2016-10-18 12:03:15]

Dry run: 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, 2016-10-18 13:00:11]

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

[commit-bot: I haz the power, 2016-10-18 13:00:11]

Dry run: Exceeded global retry quota

[Yuki, 2016-10-18 14:02:52]

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

[commit-bot: I haz the power, 2016-10-18 14:03:08]

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

[Yuki, 2016-10-18 14:04:09]

yukishiino@chromium.org changed reviewers:
+ haraken@chromium.org, peria@chromium.org

[Yuki, 2016-10-18 14:04:10]

Could you guys review this CL?

[commit-bot: I haz the power, 2016-10-18 15:28:38]

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

[commit-bot: I haz the power, 2016-10-18 15:28:39]

Dry run: This issue passed the CQ dry run.

[haraken, 2016-10-18 18:46:57]

https://chromiumcodereview.appspot.com/2418413004/diff/20001/third_party/WebK...
File third_party/WebKit/Source/bindings/core/v8/GeneratedCodeHelper.h (right):

https://chromiumcodereview.appspot.com/2418413004/diff/20001/third_party/WebK...
third_party/WebKit/Source/bindings/core/v8/GeneratedCodeHelper.h:54:
ScriptState* scriptState = ScriptState::forFunctionObject(m_info);

Just help me understand:

- Is ExceptionToRejectPromiseScope the only place that hits this issue? Why are
other ScriptPromise::reject()'s safe?

- If reject promises must be created in the current realm, why is it safe to
create regular promises in the relevant realm? I don't quite understand where
the difference comes from.

[Yuki, 2016-10-19 06:06:58]

https://chromiumcodereview.appspot.com/2418413004/diff/20001/third_party/WebK...
File third_party/WebKit/Source/bindings/core/v8/GeneratedCodeHelper.h (right):

https://chromiumcodereview.appspot.com/2418413004/diff/20001/third_party/WebK...
third_party/WebKit/Source/bindings/core/v8/GeneratedCodeHelper.h:54:
ScriptState* scriptState = ScriptState::forFunctionObject(m_info);
On 2016/10/18 18:46:56, haraken wrote:
> 
> Just help me understand:
> 
> - Is ExceptionToRejectPromiseScope the only place that hits this issue? Why
are
> other ScriptPromise::reject()'s safe?

Other ScriptPromise::reject() are called after we checked the origin, so even if
it's the relevant realm and not the current realm, it must be the same origin at
least.

We may want to fix all reject() callsites so that they create reject promises in
the current realm, not the relevant realm.


> - If reject promises must be created in the current realm, why is it safe to
> create regular promises in the relevant realm? I don't quite understand where
> the difference comes from.

We check the origin in an early stage before starting using the relevant realm,
so it's okay to create regular promises in the relevant realm after we checked
the origin.  It must be the same origin.

[haraken, 2016-10-19 09:28:34]

On 2016/10/19 06:06:58, Yuki wrote:
>
https://chromiumcodereview.appspot.com/2418413004/diff/20001/third_party/WebK...
> File third_party/WebKit/Source/bindings/core/v8/GeneratedCodeHelper.h (right):
> 
>
https://chromiumcodereview.appspot.com/2418413004/diff/20001/third_party/WebK...
> third_party/WebKit/Source/bindings/core/v8/GeneratedCodeHelper.h:54:
> ScriptState* scriptState = ScriptState::forFunctionObject(m_info);
> On 2016/10/18 18:46:56, haraken wrote:
> > 
> > Just help me understand:
> > 
> > - Is ExceptionToRejectPromiseScope the only place that hits this issue? Why
> are
> > other ScriptPromise::reject()'s safe?
> 
> Other ScriptPromise::reject() are called after we checked the origin, so even
if
> it's the relevant realm and not the current realm, it must be the same origin
at
> least.
> 
> We may want to fix all reject() callsites so that they create reject promises
in
> the current realm, not the relevant realm.
> 
> 
> > - If reject promises must be created in the current realm, why is it safe to
> > create regular promises in the relevant realm? I don't quite understand
where
> > the difference comes from.
> 
> We check the origin in an early stage before starting using the relevant
realm,

Would you help me understand where we're checking the origin?

> so it's okay to create regular promises in the relevant realm after we checked
> the origin.  It must be the same origin.

My concern is that I don't think we're doing the origin check everywhere, so
using the relevant realm would not be a safe thing in general.

For example, other than ScriptPromise::reject(), any Get()/Set() can execute an
arbitrary script. Doesn't it mean that it's not safe to use the relevant realm
without having the origin check before almost all V8 APIs?

[Yuki, 2016-10-19 11:23:37]

Do the following comments make sense?

https://chromiumcodereview.appspot.com/2418413004/diff/20001/third_party/WebK...
File third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl (right):

https://chromiumcodereview.appspot.com/2418413004/diff/20001/third_party/WebK...
third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:27: {# Security
checks #}
This block is checking the origin.
We're throwing a SecurityError in shouldAllowAccessTo() if it's a cross origin.

https://chromiumcodereview.appspot.com/2418413004/diff/20001/third_party/WebK...
third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:66:
{{function_call | indent}}
All the other hand-written ScriptPromise::reject() or whatever are executed
here, so it's already guaranteed that it's the same origin.

[haraken, 2016-10-19 12:38:48]

https://chromiumcodereview.appspot.com/2418413004/diff/20001/third_party/WebK...
File third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl (right):

https://chromiumcodereview.appspot.com/2418413004/diff/20001/third_party/WebK...
third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:27: {# Security
checks #}
On 2016/10/19 11:23:37, Yuki wrote:
> This block is checking the origin.
> We're throwing a SecurityError in shouldAllowAccessTo() if it's a cross
origin.

However, this security check is generated only when the DOM attribute/operation
has [CheckSecurity], right?

How are ScriptPromise::reject()'s and any V8 APIs that can invoke arbitrary
scripts in other DOM attributes/operations cared?

My worry is that this security issue is not a specific problem of
ExceptionToRejectPromiseScope but a general problem of places that use a
relevant realm.

[Yuki, 2016-10-19 12:48:00]

On 2016/10/19 12:38:48, haraken wrote:
>
https://chromiumcodereview.appspot.com/2418413004/diff/20001/third_party/WebK...
> File third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl (right):
> 
>
https://chromiumcodereview.appspot.com/2418413004/diff/20001/third_party/WebK...
> third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:27: {# Security
> checks #}
> On 2016/10/19 11:23:37, Yuki wrote:
> > This block is checking the origin.
> > We're throwing a SecurityError in shouldAllowAccessTo() if it's a cross
> origin.
> 
> However, this security check is generated only when the DOM
attribute/operation
> has [CheckSecurity], right?

Right (it's [DoNotCheckSecurity] though), and you cannot access to DOM
attributes/operations if it's cross origin and the attribute/operation does not
have [DoNotCheckSecurity].


> How are ScriptPromise::reject()'s and any V8 APIs that can invoke arbitrary
> scripts in other DOM attributes/operations cared?

No problem at all as long as it's same origin.  Nothing has changed.


> My worry is that this security issue is not a specific problem of
> ExceptionToRejectPromiseScope but a general problem of places that use a
> relevant realm.

Yes, this is a specific problem caused with misdesign of
ExceptionToRejectPromiseScope.
It's okay to access to the relevant realm as long as it's same origin.


My old CL wrongly allowed to access to the relevant realm BEFORE we're checking
the origin.

It's totally fine for whatever to access to any realm AFTER we checked the
origin.

[haraken, 2016-10-19 12:49:58]

On 2016/10/19 12:48:00, Yuki wrote:
> On 2016/10/19 12:38:48, haraken wrote:
> >
>
https://chromiumcodereview.appspot.com/2418413004/diff/20001/third_party/WebK...
> > File third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl (right):
> > 
> >
>
https://chromiumcodereview.appspot.com/2418413004/diff/20001/third_party/WebK...
> > third_party/WebKit/Source/bindings/templates/methods.cpp.tmpl:27: {#
Security
> > checks #}
> > On 2016/10/19 11:23:37, Yuki wrote:
> > > This block is checking the origin.
> > > We're throwing a SecurityError in shouldAllowAccessTo() if it's a cross
> > origin.
> > 
> > However, this security check is generated only when the DOM
> attribute/operation
> > has [CheckSecurity], right?
> 
> Right (it's [DoNotCheckSecurity] though), and you cannot access to DOM
> attributes/operations if it's cross origin and the attribute/operation does
not
> have [DoNotCheckSecurity].
> 
> 
> > How are ScriptPromise::reject()'s and any V8 APIs that can invoke arbitrary
> > scripts in other DOM attributes/operations cared?
> 
> No problem at all as long as it's same origin.  Nothing has changed.
> 
> 
> > My worry is that this security issue is not a specific problem of
> > ExceptionToRejectPromiseScope but a general problem of places that use a
> > relevant realm.
> 
> Yes, this is a specific problem caused with misdesign of
> ExceptionToRejectPromiseScope.
> It's okay to access to the relevant realm as long as it's same origin.
> 
> 
> My old CL wrongly allowed to access to the relevant realm BEFORE we're
checking
> the origin.
> 
> It's totally fine for whatever to access to any realm AFTER we checked the
> origin.

Ah, thanks. Now I understand this CL :)

LGTM

[Yuki, 2016-10-19 13:08:27]

The CQ bit was checked by yukishiino@chromium.org

[commit-bot: I haz the power, 2016-10-19 13:08:46]

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, 2016-10-19 13:19:41]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-10-21 13:07:43]

Description was changed from

==========
binding: Creates a reject promise always in the current realm.

Regular promises are created in the relevant real of the context
object.  However, reject promises are special, they must be
created in the current realm as same as exceptions must be
created in the current realm.

BUG=656274
==========

to

==========
binding: Creates a reject promise always in the current realm.

Regular promises are created in the relevant real of the context
object.  However, reject promises are special, they must be
created in the current realm as same as exceptions must be
created in the current realm.

BUG=656274

Committed: https://crrev.com/9e04d69152b9c2ac7b4e395213c0e19c52e69bcc
Cr-Commit-Position: refs/heads/master@{#426171}
==========

[commit-bot: I haz the power, 2016-10-21 13:07:44]

Patchset 2 (id:??) landed as
https://crrev.com/9e04d69152b9c2ac7b4e395213c0e19c52e69bcc
Cr-Commit-Position: refs/heads/master@{#426171}