bindings: Refactors BindingSecurity::shouldAllowAccessToXXX.

bindings: Refactors BindingSecurity::shouldAllowAccessToXXX.

This refactoring has two major benefits.
1. This makes the code generator's template code simpler.
2. This makes it easier to use shouldAllowAccessTo in the right way.

From the point of view of shouldAllowAccessToFrame,
before)
The callers must pass the correct Frame object to shouldAllowAccessToFrame.  It's caller's duty to extract the correct Frame object in the right way.
after)
The callers only need to pass the receiver to shouldAllowAccessTo.  No need to understand how to extract the correct SecurityOrigin (or Frame) from the receiver object.  shouldAllowAccessTo functions take care of all the things needed.

This CL is helpful for http://crrev.com/1380503002 which is trying to make non-whitelisted DOM attributes of Window into accessor-type JS properties.

BUG=509936
Committed: https://crrev.com/a4546f721ab58264e2e276425f20d76b5f2ebdcc
Cr-Commit-Position: refs/heads/master@{#361812}

[Yuki, 2015-11-12 05:31:23]

Description was changed from

==========
bindings: Refactors BindingSecurity::shouldAllowAccessToXXX.

BUG=
==========

to

==========
bindings: Refactors BindingSecurity::shouldAllowAccessToXXX.

This refactoring has two major benefits.
1. This makes the code generator's template code simpler.
2. This makes it easier to use shouldAllowAccessTo in the right way.

From the point of view of shouldAllowAccessToFrame,
before)
The callers must pass the correct Frame object to shouldAllowAccessToFrame. 
It's caller's duty to extract the correct Frame object in the right way.
after)
The callers only need to pass the receiver to shouldAllowAccessTo.  No need to
understand how to extract the correct SecurityOrigin (or Frame) from the
receiver object.  shouldAllowAccessTo functions take care of all the things
needed.

This CL is helpful for http://crrev.com/1380503002 which is trying to make
non-whitelisted DOM attributes of Window accessor-type JS properties.

BUG=509936
==========

[Yuki, 2015-11-12 05:33:35]

Description was changed from

==========
bindings: Refactors BindingSecurity::shouldAllowAccessToXXX.

This refactoring has two major benefits.
1. This makes the code generator's template code simpler.
2. This makes it easier to use shouldAllowAccessTo in the right way.

From the point of view of shouldAllowAccessToFrame,
before)
The callers must pass the correct Frame object to shouldAllowAccessToFrame. 
It's caller's duty to extract the correct Frame object in the right way.
after)
The callers only need to pass the receiver to shouldAllowAccessTo.  No need to
understand how to extract the correct SecurityOrigin (or Frame) from the
receiver object.  shouldAllowAccessTo functions take care of all the things
needed.

This CL is helpful for http://crrev.com/1380503002 which is trying to make
non-whitelisted DOM attributes of Window accessor-type JS properties.

BUG=509936
==========

to

==========
bindings: Refactors BindingSecurity::shouldAllowAccessToXXX.

This refactoring has two major benefits.
1. This makes the code generator's template code simpler.
2. This makes it easier to use shouldAllowAccessTo in the right way.

From the point of view of shouldAllowAccessToFrame,
before)
The callers must pass the correct Frame object to shouldAllowAccessToFrame. 
It's caller's duty to extract the correct Frame object in the right way.
after)
The callers only need to pass the receiver to shouldAllowAccessTo.  No need to
understand how to extract the correct SecurityOrigin (or Frame) from the
receiver object.  shouldAllowAccessTo functions take care of all the things
needed.

This CL is helpful for http://crrev.com/1380503002 which is trying to make
non-whitelisted DOM attributes of Window into accessor-type JS properties.

BUG=509936
==========

[Yuki, 2015-11-16 08:38:26]

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

[Yuki, 2015-11-16 08:38:27]

Could you guys review this CL?

Should be no behavioral change.

[haraken, 2015-11-16 08:53:19]

haraken@chromium.org changed reviewers:
+ dcheng@chromium.org

[haraken, 2015-11-16 08:53:20]

Isn't this change assuming that:

  window == window->frame()->domWindow()

?

I believe it _should_ hold, but in practice I don't think it holds. If it does
not hold, this CL can end up with using a wrong window for security checks. For
example, see this CL: https://codereview.chromium.org/1374533002.

Just to clarify: I'm a big fan of this CL (we should make the change at some
point). I'm just not convinced about its correctness at this point.

+dcheng

https://codereview.chromium.org/1417023006/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceCheckSecurity.cpp
(right):

https://codereview.chromium.org/1417023006/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceCheckSecurity.cpp:239:
v8SetReturnValueNull(info);

Is this change intended?

[Yuki, 2015-11-16 09:54:10]

On 2015/11/16 08:53:20, haraken wrote:
> Isn't this change assuming that:
> 
>   window == window->frame()->domWindow()
> 
> ?
> 
> I believe it _should_ hold, but in practice I don't think it holds. If it does
> not hold, this CL can end up with using a wrong window for security checks.
For
> example, see this CL: https://codereview.chromium.org/1374533002.
> 
> Just to clarify: I'm a big fan of this CL (we should make the change at some
> point). I'm just not convinced about its correctness at this point.
> 
> +dcheng

See the implementation of LocalDOMWindow::frame().

LocalFrame* LocalDOMWindow::frame() const
{
    // If the LocalDOMWindow still has a frame reference, that frame must point
    // back to this LocalDOMWindow: otherwise, it's easy to get into a situation
    // where script execution leaks between different LocalDOMWindows.
    if (m_frameObserver->frame())
        ASSERT_WITH_SECURITY_IMPLICATION(m_frameObserver->frame()->domWindow()
== this);
    return m_frameObserver->frame();
}

If a LocalDOMWindow has the frame, then window->frame()->domWindow() must be
equal to |window| itself.  If the window is NOT attached to a frame, then they
may be different.  As long as we check that the window has the frame, there
should be no problem, I think.

[Yuki, 2015-11-16 10:24:33]

https://codereview.chromium.org/1417023006/diff/180001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceCheckSecurity.cpp
(right):

https://codereview.chromium.org/1417023006/diff/180001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/tests/results/core/V8TestInterfaceCheckSecurity.cpp:239:
v8SetReturnValueNull(info);
On 2015/11/16 08:53:19, haraken wrote:
> 
> Is this change intended?

No, not intentional.  Reverted this part.

By the way, the return value should be determined by "return type of DOM
operation".  For example,
    DOMString foo();
should return "" instead of undefined or null (if not throw).  Details depend on
each definition of DOM operations.

[Yuki, 2015-11-16 10:39:04]

On 2015/11/16 09:54:10, Yuki wrote:
> On 2015/11/16 08:53:20, haraken wrote:
> > Isn't this change assuming that:
> > 
> >   window == window->frame()->domWindow()
> > 
> > ?
> > 
> > I believe it _should_ hold, but in practice I don't think it holds. If it
does
> > not hold, this CL can end up with using a wrong window for security checks.
> For
> > example, see this CL: https://codereview.chromium.org/1374533002.
> > 
> > Just to clarify: I'm a big fan of this CL (we should make the change at some
> > point). I'm just not convinced about its correctness at this point.
> > 
> > +dcheng
> 
> See the implementation of LocalDOMWindow::frame().
> 
> LocalFrame* LocalDOMWindow::frame() const
> {
>     // If the LocalDOMWindow still has a frame reference, that frame must
point
>     // back to this LocalDOMWindow: otherwise, it's easy to get into a
situation
>     // where script execution leaks between different LocalDOMWindows.
>     if (m_frameObserver->frame())
>         ASSERT_WITH_SECURITY_IMPLICATION(m_frameObserver->frame()->domWindow()
> == this);
>     return m_frameObserver->frame();
> }
> 
> If a LocalDOMWindow has the frame, then window->frame()->domWindow() must be
> equal to |window| itself.  If the window is NOT attached to a frame, then they
> may be different.  As long as we check that the window has the frame, there
> should be no problem, I think.

Let me add one more reason to justify.

  v8::Local<v8::Object> receiver = ...;
  DOMWindow* theWindow = toImpl(receiver);
  DOMWindow* anotherWindow = theWindow->frame()->domWindow();
  if (!shouldAllowAccessTo(...)) {
    return;
  }
  theWindow->doSomething();

If theWindow and anotherWindow differ, we should check the access to |theWindow|
rather than to |anotherWindow| because the |receiver| points to |theWindow| and
the actual operation will be done on |theWindow|.

Note that |theWindow| and |anotherWindow| must be the same as long as
theWindow->frame() is non-null.  Shouldn't be a case that they differ.

[haraken, 2015-11-16 11:13:06]

On 2015/11/16 10:39:04, Yuki wrote:
> On 2015/11/16 09:54:10, Yuki wrote:
> > On 2015/11/16 08:53:20, haraken wrote:
> > > Isn't this change assuming that:
> > > 
> > >   window == window->frame()->domWindow()
> > > 
> > > ?
> > > 
> > > I believe it _should_ hold, but in practice I don't think it holds. If it
> does
> > > not hold, this CL can end up with using a wrong window for security
checks.
> > For
> > > example, see this CL: https://codereview.chromium.org/1374533002.
> > > 
> > > Just to clarify: I'm a big fan of this CL (we should make the change at
some
> > > point). I'm just not convinced about its correctness at this point.
> > > 
> > > +dcheng
> > 
> > See the implementation of LocalDOMWindow::frame().
> > 
> > LocalFrame* LocalDOMWindow::frame() const
> > {
> >     // If the LocalDOMWindow still has a frame reference, that frame must
> point
> >     // back to this LocalDOMWindow: otherwise, it's easy to get into a
> situation
> >     // where script execution leaks between different LocalDOMWindows.
> >     if (m_frameObserver->frame())
> >        
ASSERT_WITH_SECURITY_IMPLICATION(m_frameObserver->frame()->domWindow()
> > == this);
> >     return m_frameObserver->frame();
> > }
> > 
> > If a LocalDOMWindow has the frame, then window->frame()->domWindow() must be
> > equal to |window| itself.  If the window is NOT attached to a frame, then
they
> > may be different.  As long as we check that the window has the frame, there
> > should be no problem, I think.
> 
> Let me add one more reason to justify.
> 
>   v8::Local<v8::Object> receiver = ...;
>   DOMWindow* theWindow = toImpl(receiver);
>   DOMWindow* anotherWindow = theWindow->frame()->domWindow();
>   if (!shouldAllowAccessTo(...)) {
>     return;
>   }
>   theWindow->doSomething();
> 
> If theWindow and anotherWindow differ, we should check the access to
|theWindow|
> rather than to |anotherWindow| because the |receiver| points to |theWindow|
and
> the actual operation will be done on |theWindow|.
> 
> Note that |theWindow| and |anotherWindow| must be the same as long as
> theWindow->frame() is non-null.  Shouldn't be a case that they differ.

Thanks, convinced. https://codereview.chromium.org/1374533002 guaranteed that
window->frame() is null iff the window is detached from the frame.

Will take a closer look.

[haraken, 2015-11-16 11:34:21]

Mostly looks good.

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp (right):

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:75: return
canAccessFrame(isolate, accessingWindow,
frame->securityContext()->securityOrigin(), target, exceptionState);

Can we add ASSERT(target == target->frame()->domWindow()) to canAccessFrame()?

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h (right):

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h:53: class
BindingSecurity {

Add CORE_EXPORT to the class (and remove CORE_EXPORT from each method).

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h:56: // Check the
access to the receiver.

// Check if the receiver is allowed to access the
DOMWindow/EventTarget/Location.

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h:61: CORE_EXPORT
static bool shouldAllowAccessTo(v8::Isolate*, const LocalDOMWindow*
accessingWindow, const EventTarget* target, ExceptionState&);  //
NOLINT(readability/parameter_name)

I'm just curious but how much is the EventTarget* version used? Hopefully we can
remove it...

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h:71: // Check the
access to the frame rather than to a DOM object.

DOM object => DOMWindow/EventTarget/Location

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h:73: CORE_EXPORT
static bool shouldAllowAccessToFrame(v8::Isolate*, const LocalDOMWindow*
accessingWindow, const Frame*, SecurityReportingOption);

const Frame* target

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/custom/V8EventTargetCustom.cpp
(left):

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/custom/V8EventTargetCustom.cpp:89: if
(!window->document())

Is it ok to remove this check?

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/custom/V8EventTargetCustom.cpp:135:
if (!window->document())

Ditto.

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/core/v8/custom/V8InjectedScriptManager.cpp
(right):

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/custom/V8InjectedScriptManager.cpp:89:
v8::Local<v8::Object> holder = V8Window::findInstanceInPrototypeChain(global,
scriptState->isolate());

We should use toDOMWindow defined in V8Binding.h.

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/templates/attributes.cpp (left):

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/attributes.cpp:87: if
(!window->document())

Is it okay to remove this check?

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/attributes.cpp:308: if
(!window->document())

Ditto.

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/templates/methods.cpp (left):

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/methods.cpp:35: if
(!window->document())

Ditto.

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/tests/results/core/V8TestObject.cpp
(left):

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/tests/results/core/V8TestObject.cpp:10230:
v8SetReturnValueNull(info);

This change is intentional, right?

[jochen (gone - plz use gerrit), 2015-11-16 14:16:59]

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h (right):

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h:58: CORE_EXPORT
static bool shouldAllowAccessTo(v8::Isolate*, const LocalDOMWindow*
accessingWindow, const DOMWindow* target, ExceptionState&);
should we encode what the method does in the name instead of in a comment?

shouldAllowAccessFromWindowTo() and shouldAllowWindowToGetReturnValue() or
something?

[dcheng, 2015-11-17 01:56:59]

I think this change is probably OK, but I need to think over it a bit more
(sorry I haven't had a lot of time to look at this CL yet).

Just a few of my initial thoughts from reading over it.

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp (right):

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:137: if (!target
|| !target->securityContext())
It seems like we make these checks for every Frame*. Can we move these checks
into canAccessFrame() and just pass |frame| directly?

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h (right):

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h:56: // Check the
access to the receiver.
Can we clarify the comments to define what "receiver" means in this context?

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h:66: // Check the
access to the return value.
and "access to the return value". I'm not super familiar with bindings, so I'm
not actually sure what the difference between the receiver and return value
methods is.

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h:72: // You should
check the access to the DOM object as long as it's possible.
Nit: reword this as "Prefer to use the previous overloads instead of falling
back to using Frame*."

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h:73: CORE_EXPORT
static bool shouldAllowAccessToFrame(v8::Isolate*, const LocalDOMWindow*
accessingWindow, const Frame*, SecurityReportingOption);
This is a 'receiver' method, right? Should it be grouped with the other receiver
methods?

[Yuki, 2015-11-17 04:50:21]

Let me clarify what "receiver" and "return value" are.
I'll address each comment separately.

    var x = domObj.attribute;
    var y = domObj.operation();

where |domObj| is "receiver", and |x| and |y| are "return value".
(At least I meant so in this CL.)

I'll make comments clearer.

[Yuki, 2015-11-20 12:27:52]

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp (right):

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:75: return
canAccessFrame(isolate, accessingWindow,
frame->securityContext()->securityOrigin(), target, exceptionState);
On 2015/11/16 11:34:20, haraken wrote:
> 
> Can we add ASSERT(target == target->frame()->domWindow()) to canAccessFrame()?

Done.

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:137: if (!target
|| !target->securityContext())
On 2015/11/17 01:56:59, dcheng wrote:
> It seems like we make these checks for every Frame*. Can we move these checks
> into canAccessFrame() and just pass |frame| directly?

Hmm, we can do it for DOMWindow* and EventTarget*.

For Location, we're passing target->frame()->domWindow() as the target window to
canAccessFrame().  So we need to check if a frame exists or not beforehand.

For Node, we're not using a frame to extract the security origin and the target
window.

I think it's okay as is.  WDYT?

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h (right):

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h:53: class
BindingSecurity {
On 2015/11/16 11:34:20, haraken wrote:
> 
> Add CORE_EXPORT to the class (and remove CORE_EXPORT from each method).

Done.

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h:56: // Check the
access to the receiver.
On 2015/11/17 01:56:59, dcheng wrote:
> Can we clarify the comments to define what "receiver" means in this context?

Done.

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h:56: // Check the
access to the receiver.
On 2015/11/16 11:34:20, haraken wrote:
> 
> // Check if the receiver is allowed to access the
> DOMWindow/EventTarget/Location.

I rewrote the comment in a little bit different way.

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h:58: CORE_EXPORT
static bool shouldAllowAccessTo(v8::Isolate*, const LocalDOMWindow*
accessingWindow, const DOMWindow* target, ExceptionState&);
On 2015/11/16 14:16:58, jochen wrote:
> should we encode what the method does in the name instead of in a comment?
> 
> shouldAllowAccessFromWindowTo() and shouldAllowWindowToGetReturnValue() or
> something?

I agree that we'd better to encode what the method does in the name, however,
this simple overloading makes the generated code much simpler.

    {{cpp_type}} impl = V8T::toImpl(v8Object);
    if (!shouldAllowAccessTo(accessingWindow, impl)) {
        return;  // NOT ALLOWED.
    }
    impl->doSomething();

where there is no need to change the name of shouldAllowAccessToXXX() depending
on the type of |impl|.

In this case, I'd prefer the simple overloading with the same name.  With this
CL, we can unify [CheckSecurity=Window] and [CheckSecurity=Frame] extended
attributes.

What do you guys think?

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h:61: CORE_EXPORT
static bool shouldAllowAccessTo(v8::Isolate*, const LocalDOMWindow*
accessingWindow, const EventTarget* target, ExceptionState&);  //
NOLINT(readability/parameter_name)
On 2015/11/16 11:34:20, haraken wrote:
> 
> I'm just curious but how much is the EventTarget* version used? Hopefully we
can
> remove it...

The generated V8EventTarget.cpp needs the EventTarget* version.  The DOM
attributes and operations of EventTarget, such as {add,remove}EventListener,
dispatchEvent, etc., need to check the origin because the instance can be an
instance of DOMWindow.  So we need this version.

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h:66: // Check the
access to the return value.
On 2015/11/17 01:56:59, dcheng wrote:
> and "access to the return value". I'm not super familiar with bindings, so I'm
> not actually sure what the difference between the receiver and return value
> methods is.

Done.

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h:71: // Check the
access to the frame rather than to a DOM object.
On 2015/11/16 11:34:20, haraken wrote:
> 
> DOM object => DOMWindow/EventTarget/Location

Done.

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h:72: // You should
check the access to the DOM object as long as it's possible.
On 2015/11/17 01:56:59, dcheng wrote:
> Nit: reword this as "Prefer to use the previous overloads instead of falling
> back to using Frame*."

Done.

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h:73: CORE_EXPORT
static bool shouldAllowAccessToFrame(v8::Isolate*, const LocalDOMWindow*
accessingWindow, const Frame*, SecurityReportingOption);
On 2015/11/17 01:56:59, dcheng wrote:
> This is a 'receiver' method, right? Should it be grouped with the other
receiver
> methods?

Done.

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h:73: CORE_EXPORT
static bool shouldAllowAccessToFrame(v8::Isolate*, const LocalDOMWindow*
accessingWindow, const Frame*, SecurityReportingOption);
On 2015/11/16 11:34:20, haraken wrote:
> 
> const Frame* target

Done.

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/custom/V8EventTargetCustom.cpp
(left):

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/custom/V8EventTargetCustom.cpp:89: if
(!window->document())
On 2015/11/16 11:34:21, haraken wrote:
> 
> Is it ok to remove this check?

Yes, it's okay because if |window| does not have the Document, then
shouldAllowAccessToFrame() always fails (no Document == no SecurityContext). 
This line is dead code.

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/custom/V8EventTargetCustom.cpp:135:
if (!window->document())
On 2015/11/16 11:34:20, haraken wrote:
> 
> Ditto.

Ditto.

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/core/v8/custom/V8InjectedScriptManager.cpp
(right):

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/custom/V8InjectedScriptManager.cpp:89:
v8::Local<v8::Object> holder = V8Window::findInstanceInPrototypeChain(global,
scriptState->isolate());
On 2015/11/16 11:34:21, haraken wrote:
> 
> We should use toDOMWindow defined in V8Binding.h.

Done.

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/templates/attributes.cpp (left):

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/attributes.cpp:87: if
(!window->document())
On 2015/11/16 11:34:21, haraken wrote:
> 
> Is it okay to remove this check?

Yes, because shouldAllowAccessToFrame always fails if the Window does not have a
Document.  This line is dead code.

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/attributes.cpp:308: if
(!window->document())
On 2015/11/16 11:34:21, haraken wrote:
> 
> Ditto.

Ditto.

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/templates/methods.cpp (left):

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/templates/methods.cpp:35: if
(!window->document())
On 2015/11/16 11:34:21, haraken wrote:
> 
> Ditto.

Ditto.

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/tests/results/core/V8TestObject.cpp
(left):

https://codereview.chromium.org/1417023006/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/tests/results/core/V8TestObject.cpp:10230:
v8SetReturnValueNull(info);
On 2015/11/16 11:34:21, haraken wrote:
> 
> This change is intentional, right?
> 

Oops, no, not really.
Nice catch, and fixed.

[haraken, 2015-11-22 14:55:01]

LGTM with comments.

https://codereview.chromium.org/1417023006/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp (right):

https://codereview.chromium.org/1417023006/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:129: return
canAccessFrame(isolate, accessingWindow, target->document().securityOrigin(),
target->document().domWindow(), exceptionState);

Is it guaranteed that target->document().frame() is non-null?

I guess this code should be:

  if (!target)
    return false;
  return shouldAllowAccessTo(isolate, accessingWindow,
target->document().frame(), exceptionState);

https://codereview.chromium.org/1417023006/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:136: return
canAccessFrame(isolate, accessingWindow, target->document().securityOrigin(),
target->document().domWindow(), reportingOption);

Ditto.

https://codereview.chromium.org/1417023006/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h (right):

https://codereview.chromium.org/1417023006/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h:56: // Check if the
caller (|accessingWindow|) is allowed to access to the JS

to access the JS receiver object

https://codereview.chromium.org/1417023006/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h:68: static bool
shouldAllowAccessTo(v8::Isolate*, const LocalDOMWindow* accessingWindow, const
EventTarget* target, ExceptionState&);  // NOLINT(readability/parameter_name)

Remove the NOLINT.

https://codereview.chromium.org/1417023006/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h:73: // (|target|)
rather than to a DOMWindow/EventTarget/Location.

Isn't this comment redundant? This is the same one as line 56.

https://codereview.chromium.org/1417023006/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h:83: // where |x| is
the returned object).

Ditto.

[Yuki, 2015-11-24 08:44:12]

https://codereview.chromium.org/1417023006/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp (right):

https://codereview.chromium.org/1417023006/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:129: return
canAccessFrame(isolate, accessingWindow, target->document().securityOrigin(),
target->document().domWindow(), exceptionState);
On 2015/11/22 14:55:00, haraken wrote:
> 
> Is it guaranteed that target->document().frame() is non-null?
> 
> I guess this code should be:
> 
>   if (!target)
>     return false;
>   return shouldAllowAccessTo(isolate, accessingWindow,
> target->document().frame(), exceptionState);

Note that this is not a Window.  I think there is no need to require that the
node's document is attached to a frame.

For Window, WindowProxy must always point to a Window attached to a frame (the
spec requires "the Window object of the browsing context's active document"
where "the browsing context" is close to Frame in our implementation).

However, I don't see such a requirement for non-Window interface objects.

By the way, the old implementation is the same as this CL.  I'm not changing the
implementation here.

https://codereview.chromium.org/1417023006/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:136: return
canAccessFrame(isolate, accessingWindow, target->document().securityOrigin(),
target->document().domWindow(), reportingOption);
On 2015/11/22 14:55:00, haraken wrote:
> 
> Ditto.

Ditto.

https://codereview.chromium.org/1417023006/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h (right):

https://codereview.chromium.org/1417023006/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h:56: // Check if the
caller (|accessingWindow|) is allowed to access to the JS
On 2015/11/22 14:55:00, haraken wrote:
> 
> to access the JS receiver object

Done.

https://codereview.chromium.org/1417023006/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h:68: static bool
shouldAllowAccessTo(v8::Isolate*, const LocalDOMWindow* accessingWindow, const
EventTarget* target, ExceptionState&);  // NOLINT(readability/parameter_name)
On 2015/11/22 14:55:00, haraken wrote:
> 
> Remove the NOLINT.

I need this NOLINT, otherwise the lint tool warns that "EventTarget* target" is
redundant and we should remove "target" argument identifier.

https://codereview.chromium.org/1417023006/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h:73: // (|target|)
rather than to a DOMWindow/EventTarget/Location.
On 2015/11/22 14:55:00, haraken wrote:
> 
> Isn't this comment redundant? This is the same one as line 56.
> 

Removed.

https://codereview.chromium.org/1417023006/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h:83: // where |x| is
the returned object).
On 2015/11/22 14:55:00, haraken wrote:
> 
> Ditto.

Sorry, ditto for what?

[haraken, 2015-11-24 08:47:49]

https://codereview.chromium.org/1417023006/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h (right):

https://codereview.chromium.org/1417023006/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h:83: // where |x| is
the returned object).
On 2015/11/24 08:44:12, Yuki wrote:
> On 2015/11/22 14:55:00, haraken wrote:
> > 
> > Ditto.
> 
> Sorry, ditto for what?

The comment in line 78 - 83 is similar to the one in line 56 - 59.

[haraken, 2015-11-24 08:50:20]

Still LGTM

https://codereview.chromium.org/1417023006/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp (right):

https://codereview.chromium.org/1417023006/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:129: return
canAccessFrame(isolate, accessingWindow, target->document().securityOrigin(),
target->document().domWindow(), exceptionState);
On 2015/11/24 08:44:12, Yuki wrote:
> On 2015/11/22 14:55:00, haraken wrote:
> > 
> > Is it guaranteed that target->document().frame() is non-null?
> > 
> > I guess this code should be:
> > 
> >   if (!target)
> >     return false;
> >   return shouldAllowAccessTo(isolate, accessingWindow,
> > target->document().frame(), exceptionState);
> 
> Note that this is not a Window.  I think there is no need to require that the
> node's document is attached to a frame.
> 
> For Window, WindowProxy must always point to a Window attached to a frame (the
> spec requires "the Window object of the browsing context's active document"
> where "the browsing context" is close to Frame in our implementation).
> 
> However, I don't see such a requirement for non-Window interface objects.
> 
> By the way, the old implementation is the same as this CL.  I'm not changing
the
> implementation here.

I don't fully understand the security implication on it (i.e., we don't need to
check if the node is attached to a frame or not), but it makes sense to keep the
existing behavior in this CL.

[Yuki, 2015-11-24 08:51:13]

https://codereview.chromium.org/1417023006/diff/260001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h (right):

https://codereview.chromium.org/1417023006/diff/260001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h:83: // where |x| is
the returned object).
On 2015/11/24 08:47:48, haraken wrote:
> On 2015/11/24 08:44:12, Yuki wrote:
> > On 2015/11/22 14:55:00, haraken wrote:
> > > 
> > > Ditto.
> > 
> > Sorry, ditto for what?
> 
> The comment in line 78 - 83 is similar to the one in line 56 - 59.

Yes, similar, but it seems better to clarify the difference between "receiver"
and "return value" and/or what they are.  I'd prefer leaving the comment as is.

[Yuki, 2015-11-25 13:24:02]

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

[commit-bot: I haz the power, 2015-11-25 13:24:32]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1417023006/330001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1417023006/330001

[Yuki, 2015-11-26 02:39:00]

The CQ bit was checked by yukishiino@chromium.org

[Yuki, 2015-11-26 02:39:01]

The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/1417023006/#ps370001
(title: "Fixed the assertion condition.")

[commit-bot: I haz the power, 2015-11-26 02:41:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1417023006/370001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1417023006/370001

[commit-bot: I haz the power, 2015-11-26 04:08:08]

Description was changed from

==========
bindings: Refactors BindingSecurity::shouldAllowAccessToXXX.

This refactoring has two major benefits.
1. This makes the code generator's template code simpler.
2. This makes it easier to use shouldAllowAccessTo in the right way.

From the point of view of shouldAllowAccessToFrame,
before)
The callers must pass the correct Frame object to shouldAllowAccessToFrame. 
It's caller's duty to extract the correct Frame object in the right way.
after)
The callers only need to pass the receiver to shouldAllowAccessTo.  No need to
understand how to extract the correct SecurityOrigin (or Frame) from the
receiver object.  shouldAllowAccessTo functions take care of all the things
needed.

This CL is helpful for http://crrev.com/1380503002 which is trying to make
non-whitelisted DOM attributes of Window into accessor-type JS properties.

BUG=509936
==========

to

==========
bindings: Refactors BindingSecurity::shouldAllowAccessToXXX.

This refactoring has two major benefits.
1. This makes the code generator's template code simpler.
2. This makes it easier to use shouldAllowAccessTo in the right way.

From the point of view of shouldAllowAccessToFrame,
before)
The callers must pass the correct Frame object to shouldAllowAccessToFrame. 
It's caller's duty to extract the correct Frame object in the right way.
after)
The callers only need to pass the receiver to shouldAllowAccessTo.  No need to
understand how to extract the correct SecurityOrigin (or Frame) from the
receiver object.  shouldAllowAccessTo functions take care of all the things
needed.

This CL is helpful for http://crrev.com/1380503002 which is trying to make
non-whitelisted DOM attributes of Window into accessor-type JS properties.

BUG=509936
==========

[commit-bot: I haz the power, 2015-11-26 04:08:09]

Committed patchset #20 (id:370001)

[commit-bot: I haz the power, 2015-11-26 04:09:34]

Description was changed from

==========
bindings: Refactors BindingSecurity::shouldAllowAccessToXXX.

This refactoring has two major benefits.
1. This makes the code generator's template code simpler.
2. This makes it easier to use shouldAllowAccessTo in the right way.

From the point of view of shouldAllowAccessToFrame,
before)
The callers must pass the correct Frame object to shouldAllowAccessToFrame. 
It's caller's duty to extract the correct Frame object in the right way.
after)
The callers only need to pass the receiver to shouldAllowAccessTo.  No need to
understand how to extract the correct SecurityOrigin (or Frame) from the
receiver object.  shouldAllowAccessTo functions take care of all the things
needed.

This CL is helpful for http://crrev.com/1380503002 which is trying to make
non-whitelisted DOM attributes of Window into accessor-type JS properties.

BUG=509936
==========

to

==========
bindings: Refactors BindingSecurity::shouldAllowAccessToXXX.

This refactoring has two major benefits.
1. This makes the code generator's template code simpler.
2. This makes it easier to use shouldAllowAccessTo in the right way.

From the point of view of shouldAllowAccessToFrame,
before)
The callers must pass the correct Frame object to shouldAllowAccessToFrame. 
It's caller's duty to extract the correct Frame object in the right way.
after)
The callers only need to pass the receiver to shouldAllowAccessTo.  No need to
understand how to extract the correct SecurityOrigin (or Frame) from the
receiver object.  shouldAllowAccessTo functions take care of all the things
needed.

This CL is helpful for http://crrev.com/1380503002 which is trying to make
non-whitelisted DOM attributes of Window into accessor-type JS properties.

BUG=509936

Committed: https://crrev.com/a4546f721ab58264e2e276425f20d76b5f2ebdcc
Cr-Commit-Position: refs/heads/master@{#361812}
==========

[commit-bot: I haz the power, 2015-11-26 04:09:34]

Patchset 20 (id:??) landed as
https://crrev.com/a4546f721ab58264e2e276425f20d76b5f2ebdcc
Cr-Commit-Position: refs/heads/master@{#361812}