Rework security checks to be based on Window rather than Frame.

Rework security checks to be based on Window rather than Frame.

Previously, the security checks were based on the frame of the window,
rather than the window itself. This is because Location had a pointer
to the Frame, not the DOMWindow. However, this means that checks that
could not distinguish between accesses on same-origin detached frames
vs cross-origin detached frames, since a detached Window/Location has
a nulled out Frame pointer.

Now that Location is associated with a DOMWindow, the security checks
should use DOMWindow. This allows the access checks to distinguish
same origin and cross origin access on a detached window, correctly
throwing an exception on detached cross-origin access. This also
matches Firefox behavior.

BUG=694111
Review-Url: https://codereview.chromium.org/2706923002
Cr-Commit-Position: refs/heads/master@{#472412}
Committed: https://chromium.googlesource.com/chromium/src/+/1d357b6a46d70097aa28428b1f106284f7fd9990

[dcheng, 2017-02-21 06:44:57]

Description was changed from

==========
Rework security checks to be passed on Window rather than Frame.

This makes it easier to distinguish between same-origin and cross-origin
access on a detached Window.

Failing Tests:
Regressions: Unexpected text-only failures (9)
  fast/dom/Window/open-after-frame-detached.html [ Failure ]
  fast/dom/Window/orphaned-frame-access.html [ Failure ]
 
fast/dom/Window/property-access-on-cached-properties-after-frame-navigated.html
[ Failure ]
 
fast/dom/Window/property-access-on-cached-properties-after-frame-removed-and-gced.html
[ Failure ]
  fast/dom/Window/property-access-on-cached-properties-after-frame-removed.html
[ Failure ]
 
fast/dom/Window/property-access-on-cached-window-after-frame-removed-and-gced.html
[ Failure ]
  fast/dom/Window/property-access-on-cached-window-after-frame-removed.html [
Failure ]
  http/tests/fetch/chromium/discarded-window.html [ Failure ]

BUG=694111
==========

to

==========
Rework security checks to be based on Window rather than Frame.

This makes it easier to distinguish between same-origin and cross-origin
access on a detached Window.

Failing Tests:
Regressions: Unexpected text-only failures (9)
  fast/dom/Window/open-after-frame-detached.html [ Failure ]
  fast/dom/Window/orphaned-frame-access.html [ Failure ]
 
fast/dom/Window/property-access-on-cached-properties-after-frame-navigated.html
[ Failure ]
 
fast/dom/Window/property-access-on-cached-properties-after-frame-removed-and-gced.html
[ Failure ]
  fast/dom/Window/property-access-on-cached-properties-after-frame-removed.html
[ Failure ]
 
fast/dom/Window/property-access-on-cached-window-after-frame-removed-and-gced.html
[ Failure ]
  fast/dom/Window/property-access-on-cached-window-after-frame-removed.html [
Failure ]
  http/tests/fetch/chromium/discarded-window.html [ Failure ]

BUG=694111
==========

[dcheng, 2017-02-21 06:45:18]

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

[commit-bot: I haz the power, 2017-02-21 06:45:30]

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, 2017-02-21 08:03:18]

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

[commit-bot: I haz the power, 2017-02-21 08:03:19]

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_...)

[dcheng, 2017-02-28 05:23:27]

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

[commit-bot: I haz the power, 2017-02-28 05:23:50]

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, 2017-02-28 07:12:21]

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

[commit-bot: I haz the power, 2017-02-28 07:12:22]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[dcheng, 2017-03-05 11:46:22]

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

[commit-bot: I haz the power, 2017-03-05 11:46:28]

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, 2017-03-05 13:23:30]

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

[commit-bot: I haz the power, 2017-03-05 13:23:31]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[dcheng, 2017-03-06 02:42:27]

Description was changed from

==========
Rework security checks to be based on Window rather than Frame.

This makes it easier to distinguish between same-origin and cross-origin
access on a detached Window.

Failing Tests:
Regressions: Unexpected text-only failures (9)
  fast/dom/Window/open-after-frame-detached.html [ Failure ]
  fast/dom/Window/orphaned-frame-access.html [ Failure ]
 
fast/dom/Window/property-access-on-cached-properties-after-frame-navigated.html
[ Failure ]
 
fast/dom/Window/property-access-on-cached-properties-after-frame-removed-and-gced.html
[ Failure ]
  fast/dom/Window/property-access-on-cached-properties-after-frame-removed.html
[ Failure ]
 
fast/dom/Window/property-access-on-cached-window-after-frame-removed-and-gced.html
[ Failure ]
  fast/dom/Window/property-access-on-cached-window-after-frame-removed.html [
Failure ]
  http/tests/fetch/chromium/discarded-window.html [ Failure ]

BUG=694111
==========

to

==========
Rework security checks to be based on Window rather than Frame.

Previously, the security checks were based on the frame of the window,
rather than the window itself. However, this means that access checks
could not distinguish between accesses on same-origin detached frames
vs cross-origin detached frames.

Now that Location is associated with a DOMWindow, switch the security
checks to use the DOMWindow, which matches Edge and Firefox behavior.

BUG=694111
==========

[dcheng, 2017-03-06 03:00:51]

Description was changed from

==========
Rework security checks to be based on Window rather than Frame.

Previously, the security checks were based on the frame of the window,
rather than the window itself. However, this means that access checks
could not distinguish between accesses on same-origin detached frames
vs cross-origin detached frames.

Now that Location is associated with a DOMWindow, switch the security
checks to use the DOMWindow, which matches Edge and Firefox behavior.

BUG=694111
==========

to

==========
Rework security checks to be based on Window rather than Frame.

Previously, the security checks were based on the frame of the window,
rather than the window itself. This is because Location had a pointer
to the Frame, not the DOMWindow. However, this means that checks that
could not distinguish between accesses on same-origin detached frames
vs cross-origin detached frames, since a detached Window/Location has
a nulled out Frame pointer.

Now that Location is associated with a DOMWindow, the security checks
should use DOMWindow. This allows the access checks to distinguish
same origin and cross origin access on a detached window, correctly
throwing an exception on detached cross-origin access. This also
matches Firefox behavior.

BUG=694111
==========

[dcheng, 2017-03-06 03:00:51]

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

[dcheng, 2017-03-06 06:40:30]

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

[commit-bot: I haz the power, 2017-03-06 06:40:38]

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

[dcheng, 2017-03-06 06:52:36]

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

[commit-bot: I haz the power, 2017-03-06 06:52:52]

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

[dcheng, 2017-03-06 06:59:47]

Thoughts? The new layout test in this CL does not pass without this patch. It's
worth noting that our old behavior matched Edge, while our new behavior matches
FF.

I do plan on writing up a PSA about this: this means that DOM objects from
detached Windows are more accessible than before, as they will no longer fail
the security check, if the access is same origin.

https://codereview.chromium.org/2706923002/diff/140001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/fast/dom/Window/orphaned-frame-access.html
(right):

https://codereview.chromium.org/2706923002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/dom/Window/orphaned-frame-access.html:20:
assert_equals(childWindow.Comment, undefined, 'Interface Object should be
gone.');
I don't understand this, btw... but that seems to be the existing behavior, and
is not affected by this cl

https://codereview.chromium.org/2706923002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/dom/Window/property-access-on-cached-properties-after-frame-navigated-expected.txt
(right):

https://codereview.chromium.org/2706923002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/dom/Window/property-access-on-cached-properties-after-frame-navigated-expected.txt:19:
PASS window.cached_location.hash is ''
As navigating doe not clear Location's pointer back to Window, we can now access
these properties (on same origin frames)

https://codereview.chromium.org/2706923002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/dom/Window/property-access-on-cached-properties-after-frame-removed.html
(left):

https://codereview.chromium.org/2706923002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/dom/Window/property-access-on-cached-properties-after-frame-removed.html:16:
// Cached Location properties become undefined.
Ditto: no need to exclude this anymore

https://codereview.chromium.org/2706923002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/dom/Window/property-access-on-cached-window-after-frame-removed.html
(right):

https://codereview.chromium.org/2706923002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/dom/Window/property-access-on-cached-window-after-frame-removed.html:15:
expected = '0';
0 instead of 1 is expected here, since window->m_frame is cleared (so it can't
access the frame tree to get ancestor origins, not to mention it'd be weird for
a detached window to have ancestor origins)

https://codereview.chromium.org/2706923002/diff/140001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/dom/Window/resources/window-property-collector.js
(right):

https://codereview.chromium.org/2706923002/diff/140001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/dom/Window/resources/window-property-collector.js:31:
|| path[0] == 'sessionStorage') {
I plan on fixing this separately

https://codereview.chromium.org/2706923002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp (left):

https://codereview.chromium.org/2706923002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:49: const
SecurityOrigin* targetFrameOrigin,
We delay extraction of the security origin to canAccessFrameInternal. We want to
retrieve this from the SecurityContext, but RemoteDOMWindow doesn't have a
direct pointer--it must go via its back pointer to RemoteFrame. However, for
symmetry with the LocalFrame case, it's nice to not have code like this...

... since RemoteDOMWindow needs a special-case anyway, it's OK to just early
return on the special-case and downcast to Document to get the SecurityOrigin on
line 67

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

https://codereview.chromium.org/2706923002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:56: #endif
I guess we can just delete this assert, since it's no longer based on frame at
all. I'm curious if anyone has thoughts for equivalents?

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

https://codereview.chromium.org/2706923002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h:117:
v8::Local<v8::Object> host);
These additional parameters are so we can lookup the corresponding DOM
implementation, whether it's from v8's access check failed callback, or manual
invocations elsewhere in bindings code.

[commit-bot: I haz the power, 2017-03-06 08:23:33]

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

[commit-bot: I haz the power, 2017-03-06 08:23:34]

Dry run: This issue passed the CQ dry run.

[Yuki, 2017-03-06 08:42:43]

LGTM.

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

https://codereview.chromium.org/2706923002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:51: bool
canAccessFrameInternal(const LocalDOMWindow* accessingWindow,
Should we rename these helper functions canAccessWindow{Internal,}?

https://codereview.chromium.org/2706923002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:56: #endif
On 2017/03/06 06:59:47, dcheng wrote:
> I guess we can just delete this assert, since it's no longer based on frame at
> all. I'm curious if anyone has thoughts for equivalents?

This test is still effective as is, I think.  The test is
"If the window is attached to a frame, the frame's window must be the window."

This doesn't affect to detached windows.  So we can simply keep this test as is,
I think?

https://codereview.chromium.org/2706923002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:112:
v8::Local<v8::Object> windowWrapper =
I think we should use |host| directly (as same as Location).
IIUC, we don't need to use findInstanceInPrototypeChain even when |host| is the
global proxy.

https://codereview.chromium.org/2706923002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:115: return 0;
nit: s/0/nullptr/

https://codereview.chromium.org/2706923002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:115: return 0;
The call site seems not expecting findWindow to return nullptr at all.
Should this be CHECK(!windowWrapper.IsEmpty()) ?

https://codereview.chromium.org/2706923002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:123:
CHECK(false);
NOTREACHED?

https://codereview.chromium.org/2706923002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:143: bool
BindingSecurity::shouldAllowAccessTo(const LocalDOMWindow* accessingWindow,
Another CL of yours removed the use of shouldAllowAccessTo for EventTarget* from
V8EventTarget.cpp.
We no longer need this version, I think.

[haraken, 2017-03-06 08:45:26]

This CL overall looks good. Would it be possible to split the CL into a couple
of pieces?

[dcheng, 2017-03-06 08:56:30]

On 2017/03/06 08:45:26, haraken wrote:
> This CL overall looks good. Would it be possible to split the CL into a couple
> of pieces?

Hmm... there's not much I can split out. I'll move the change for
shouldAllowAccessToFrame() to take a reference to another CL, but I think that's
about it.

[dcheng, 2017-03-07 02:31:15]

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

[commit-bot: I haz the power, 2017-03-07 02:32:58]

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, 2017-03-07 05:31:51]

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

[commit-bot: I haz the power, 2017-03-07 05:31:52]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[dcheng, 2017-03-07 05:48:10]

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

https://codereview.chromium.org/2706923002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:51: bool
canAccessFrameInternal(const LocalDOMWindow* accessingWindow,
On 2017/03/06 08:42:43, Yuki(slow_til_mar08) wrote:
> Should we rename these helper functions canAccessWindow{Internal,}?

Done.

https://codereview.chromium.org/2706923002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:56: #endif
On 2017/03/06 08:42:43, Yuki(slow_til_mar08) wrote:
> On 2017/03/06 06:59:47, dcheng wrote:
> > I guess we can just delete this assert, since it's no longer based on frame
at
> > all. I'm curious if anyone has thoughts for equivalents?
> 
> This test is still effective as is, I think.  The test is
> "If the window is attached to a frame, the frame's window must be the window."
> 
> This doesn't affect to detached windows.  So we can simply keep this test as
is,
> I think?

Done.

https://codereview.chromium.org/2706923002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:112:
v8::Local<v8::Object> windowWrapper =
On 2017/03/06 08:42:43, Yuki(slow_til_mar08) wrote:
> I think we should use |host| directly (as same as Location).
> IIUC, we don't need to use findInstanceInPrototypeChain even when |host| is
the
> global proxy.

Ah good point, this is leftover from when we didn't have the internal fields set
on the global proxy.

https://codereview.chromium.org/2706923002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:115: return 0;
On 2017/03/06 08:42:42, Yuki(slow_til_mar08) wrote:
> The call site seems not expecting findWindow to return nullptr at all.
> Should this be CHECK(!windowWrapper.IsEmpty()) ?

Oops, good point. I added a CHECK() in failedAccesCheckFor(), to indicate that
we should never get there (since failing to throw an exception may lead to
security issues)

https://codereview.chromium.org/2706923002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:115: return 0;
On 2017/03/06 08:42:43, Yuki(slow_til_mar08) wrote:
> nit: s/0/nullptr/

Done.

https://codereview.chromium.org/2706923002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:123:
CHECK(false);
On 2017/03/06 08:42:43, Yuki(slow_til_mar08) wrote:
> NOTREACHED?

Hmm, I guess that's fine here. Done.

https://codereview.chromium.org/2706923002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:143: bool
BindingSecurity::shouldAllowAccessTo(const LocalDOMWindow* accessingWindow,
On 2017/03/06 08:42:43, Yuki(slow_til_mar08) wrote:
> Another CL of yours removed the use of shouldAllowAccessTo for EventTarget*
from
> V8EventTarget.cpp.
> We no longer need this version, I think.

Done.

[Yuki, 2017-03-07 05:56:23]

LGTM.

[haraken, 2017-03-07 08:53:58]

LGTM

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

https://codereview.chromium.org/2706923002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:109: if
(V8Window::wrapperTypeInfo.equals(type))

Previously we were using findInstanceInPrototypeChain. Would you help me
understand why we can remove the look up now?

https://codereview.chromium.org/2706923002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:215:
v8::Local<v8::Object> host) {

host => holder

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

https://codereview.chromium.org/2706923002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h:110:
v8::Local<v8::Object> host);

host => holder

https://codereview.chromium.org/2706923002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp (right):

https://codereview.chromium.org/2706923002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp:288:
WrapperTypeInfo::unwrap(data), host);

This host refers to a holder object, right? Maybe we should rename it to holder.

[dcheng, 2017-03-07 09:06:47]

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

https://codereview.chromium.org/2706923002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:109: if
(V8Window::wrapperTypeInfo.equals(type))
On 2017/03/07 08:53:57, haraken wrote:
> 
> Previously we were using findInstanceInPrototypeChain. Would you help me
> understand why we can remove the look up now?

Originally, we didn't set the internal fields of the global proxy. So if the
holder object passed in == the global proxy, V8Window::toImpl() will fail.

As we set the internal fields to point to the DOMWindow for everything in
Window's prototype chain now, it's possible to use any object in the prototype
chain to convert to DOMWindow.

https://codereview.chromium.org/2706923002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:215:
v8::Local<v8::Object> host) {
On 2017/03/07 08:53:57, haraken wrote:
> 
> host => holder

Done.

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

https://codereview.chromium.org/2706923002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h:110:
v8::Local<v8::Object> host);
On 2017/03/07 08:53:57, haraken wrote:
> 
> host => holder

Done.

https://codereview.chromium.org/2706923002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp (right):

https://codereview.chromium.org/2706923002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/V8Initializer.cpp:288:
WrapperTypeInfo::unwrap(data), host);
On 2017/03/07 08:53:57, haraken wrote:
> 
> This host refers to a holder object, right? Maybe we should rename it to
holder.

Done.

[dcheng, 2017-03-07 09:47:02]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2017-03-07 09:47:02]

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/2706923002/#ps180001
(title: "host -> holder")

[commit-bot: I haz the power, 2017-03-07 09:47:23]

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

[dcheng, 2017-03-07 10:02:28]

The CQ bit was unchecked by dcheng@chromium.org

[dcheng, 2017-03-07 10:04:19]

On 2017/03/07 10:02:28, dcheng wrote:
> The CQ bit was unchecked by mailto:dcheng@chromium.org

Actually I think I need to start a discussion thread/send a PSA. This does make
things more accessible on detached contexts; developers should already be
careful about what work is performed with a detached frame, but this changes a
longstanding guarantee.

[dcheng, 2017-03-27 18:34:39]

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

[commit-bot: I haz the power, 2017-03-27 18:35:16]

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, 2017-03-27 20:14:33]

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

[commit-bot: I haz the power, 2017-03-27 20:14:34]

Dry run: This issue passed the CQ dry run.

[dcheng, 2017-04-15 20:13:32]

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

[commit-bot: I haz the power, 2017-04-15 20:13:40]

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, 2017-04-15 21:32:57]

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

[commit-bot: I haz the power, 2017-04-15 21:32:57]

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

[dcheng, 2017-05-15 02:06:01]

FWIW, I rebased this across some of the platform bindings refactoring CLs. I'm
going to send out an email about this today, and plan on landing it on Thursday
(Japan/Australia time) if I don't hear any objections.

[dcheng, 2017-05-15 02:06:25]

On 2017/05/15 02:06:01, dcheng (in AEST) wrote:
> FWIW, I rebased this across some of the platform bindings refactoring CLs. I'm
> going to send out an email about this today, and plan on landing it on
Thursday
> (Japan/Australia time) if I don't hear any objections.

(Sorry, I mean FYI)

[dcheng, 2017-05-15 02:06:33]

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

[commit-bot: I haz the power, 2017-05-15 02:06:42]

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, 2017-05-15 02:13:54]

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

[commit-bot: I haz the power, 2017-05-15 02:13:55]

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

[dcheng, 2017-05-15 06:15:32]

https://codereview.chromium.org/2706923002/diff/260001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/dom/Window/orphaned-frame-access-expected.txt
(right):

https://codereview.chromium.org/2706923002/diff/260001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/dom/Window/orphaned-frame-access-expected.txt:3:
FAIL Indexed property access on detached Window assert_equals: Indexed property
should still be accessible. expected (string) "123" but got (undefined)
undefined
I'm not really sure why, but this behavior seems to have changed. I'll try to
bisect it down, but for now, I'm going to check in a failing expectation (it
seems to fail both with and without my patch)

[Yuki, 2017-05-15 06:55:27]

https://codereview.chromium.org/2706923002/diff/260001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/fast/dom/Window/orphaned-frame-access-expected.txt
(right):

https://codereview.chromium.org/2706923002/diff/260001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/fast/dom/Window/orphaned-frame-access-expected.txt:3:
FAIL Indexed property access on detached Window assert_equals: Indexed property
should still be accessible. expected (string) "123" but got (undefined)
undefined
On 2017/05/15 06:15:32, dcheng (in AEST) wrote:
> I'm not really sure why, but this behavior seems to have changed. I'll try to
> bisect it down, but for now, I'm going to check in a failing expectation (it
> seems to fail both with and without my patch)

Probably, this is caused by the revert at
https://codereview.chromium.org/2848503002/
I guess.

[dcheng, 2017-05-15 07:08:19]

On 2017/05/15 06:55:27, Yuki wrote:
>
https://codereview.chromium.org/2706923002/diff/260001/third_party/WebKit/Lay...
> File
>
third_party/WebKit/LayoutTests/fast/dom/Window/orphaned-frame-access-expected.txt
> (right):
> 
>
https://codereview.chromium.org/2706923002/diff/260001/third_party/WebKit/Lay...
>
third_party/WebKit/LayoutTests/fast/dom/Window/orphaned-frame-access-expected.txt:3:
> FAIL Indexed property access on detached Window assert_equals: Indexed
property
> should still be accessible. expected (string) "123" but got (undefined)
> undefined
> On 2017/05/15 06:15:32, dcheng (in AEST) wrote:
> > I'm not really sure why, but this behavior seems to have changed. I'll try
to
> > bisect it down, but for now, I'm going to check in a failing expectation (it
> > seems to fail both with and without my patch)
> 
> Probably, this is caused by the revert at
> https://codereview.chromium.org/2848503002/
> I guess.

Maybe, it's a little weird that only indexed property accesses would be affected
though.

[dcheng, 2017-05-17 05:36:02]

PTAL, I reverted the web-visible changes for now. Diff PS14 and PS15. I tested
to make sure the test behavior is same without and with my patch.

[dcheng, 2017-05-17 05:42:41]

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

[commit-bot: I haz the power, 2017-05-17 05:43:41]

Dry run: CQ is trying da patch.

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

[Yuki, 2017-05-17 06:31:26]

LGTM.

https://codereview.chromium.org/2706923002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp
(right):

https://codereview.chromium.org/2706923002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp:162: if
(!impl->GetFrame())
Do you want the same comment here, too?

IIUC, the spec doesn't say that we should reject setting the value when
detached.  I guess that we'd like to remove this in future?

https://codereview.chromium.org/2706923002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp:347:
info.GetIsolate(), &V8Window::wrapperTypeInfo, info.Holder());
nit: window->GetWrapperTypeInfo()
ScriptWrappable::GetWrapperTypeInfo() returns the WrapperTypeInfo you should use
(without knowing about V8Window).

[dcheng, 2017-05-17 07:13:24]

https://codereview.chromium.org/2706923002/diff/280001/third_party/WebKit/Sou...
File third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp
(right):

https://codereview.chromium.org/2706923002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp:162: if
(!impl->GetFrame())
On 2017/05/17 06:31:25, Yuki wrote:
> Do you want the same comment here, too?
> 
> IIUC, the spec doesn't say that we should reject setting the value when
> detached.  I guess that we'd like to remove this in future?

This is an implementation limitation of Chrome -- the frame tree is maintained
on frame. So if Window->Frame() is null, there is no opener to clear (by
definition, it has been cleared by removing the frame from the frame tree
already)

https://codereview.chromium.org/2706923002/diff/280001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/custom/V8WindowCustom.cpp:347:
info.GetIsolate(), &V8Window::wrapperTypeInfo, info.Holder());
On 2017/05/17 06:31:25, Yuki wrote:
> nit: window->GetWrapperTypeInfo()
> ScriptWrappable::GetWrapperTypeInfo() returns the WrapperTypeInfo you should
use
> (without knowing about V8Window).

Done.

[dcheng, 2017-05-17 07:14:42]

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

[commit-bot: I haz the power, 2017-05-17 07:15:32]

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, 2017-05-17 09:19:05]

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

[commit-bot: I haz the power, 2017-05-17 09:19:07]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[dcheng, 2017-05-17 09:37:52]

The CQ bit was checked by dcheng@chromium.org

[dcheng, 2017-05-17 09:37:54]

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/2706923002/#ps300001
(title: "Do not hardcode V8Window::wrapperTypeInfo")

[commit-bot: I haz the power, 2017-05-17 09:38:18]

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-05-17 10:46:25]

CQ is committing da patch.

Bot data: {"patchset_id": 300001, "attempt_start_ts": 1495013872645470,
"parent_rev": "9e3c1f13fb959c98bdb6aa91aa510e51e635eac7", "commit_rev":
"1d357b6a46d70097aa28428b1f106284f7fd9990"}

[commit-bot: I haz the power, 2017-05-17 10:46:36]

Description was changed from

==========
Rework security checks to be based on Window rather than Frame.

Previously, the security checks were based on the frame of the window,
rather than the window itself. This is because Location had a pointer
to the Frame, not the DOMWindow. However, this means that checks that
could not distinguish between accesses on same-origin detached frames
vs cross-origin detached frames, since a detached Window/Location has
a nulled out Frame pointer.

Now that Location is associated with a DOMWindow, the security checks
should use DOMWindow. This allows the access checks to distinguish
same origin and cross origin access on a detached window, correctly
throwing an exception on detached cross-origin access. This also
matches Firefox behavior.

BUG=694111
==========

to

==========
Rework security checks to be based on Window rather than Frame.

Previously, the security checks were based on the frame of the window,
rather than the window itself. This is because Location had a pointer
to the Frame, not the DOMWindow. However, this means that checks that
could not distinguish between accesses on same-origin detached frames
vs cross-origin detached frames, since a detached Window/Location has
a nulled out Frame pointer.

Now that Location is associated with a DOMWindow, the security checks
should use DOMWindow. This allows the access checks to distinguish
same origin and cross origin access on a detached window, correctly
throwing an exception on detached cross-origin access. This also
matches Firefox behavior.

BUG=694111

Review-Url: https://codereview.chromium.org/2706923002
Cr-Commit-Position: refs/heads/master@{#472412}
Committed:
https://chromium.googlesource.com/chromium/src/+/1d357b6a46d70097aa28428b1f10...
==========

[commit-bot: I haz the power, 2017-05-17 10:46:38]

Committed patchset #16 (id:300001) as
https://chromium.googlesource.com/chromium/src/+/1d357b6a46d70097aa28428b1f10...