Modifies V8WrapperInstantiationScope::SecurityCheck to only call BindingSecurity::shouldAllowAccess…

Modifies V8WrapperInstantiationScope::SecurityCheck to only call BindingSecurity::shouldAllowAccessToFrame if m_context's world is the main world.

BUG=529667
Committed: https://crrev.com/6daf0cab15080f3bc7bca63382cb9c28825cd54a
git-svn-id: svn://svn.chromium.org/blink/trunk@201987 bbb929c8-8fbe-4397-9dbb-9b2b20218538

[epertoso, 2015-09-09 14:49:40]

epertoso@chromium.org changed reviewers:
+ jochen@chromium.org

[jochen (gone - plz use gerrit), 2015-09-09 14:50:40]

lgtm

[epertoso, 2015-09-09 14:55:04]

The CQ bit was checked by epertoso@chromium.org

[commit-bot: I haz the power, 2015-09-09 14:55:19]

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

[haraken, 2015-09-09 15:41:42]

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

[haraken, 2015-09-09 15:41:43]

LGTM

https://codereview.chromium.org/1323453005/diff/1/Source/bindings/core/v8/V8D...
File Source/bindings/core/v8/V8DOMWrapper.cpp (right):

https://codereview.chromium.org/1323453005/diff/1/Source/bindings/core/v8/V8D...
Source/bindings/core/v8/V8DOMWrapper.cpp:133: void
V8WrapperInstantiationScope::SecurityCheck(v8::Isolate* isolate,
v8::Local<v8::Context> contextForWrapper)

SecurityCheck => securityCheck

https://codereview.chromium.org/1323453005/diff/1/Source/bindings/core/v8/V8D...
Source/bindings/core/v8/V8DOMWrapper.cpp:135: if (!m_context.IsEmpty()) {

You can early-return if m_context is empty.

https://codereview.chromium.org/1323453005/diff/1/Source/bindings/core/v8/V8D...
Source/bindings/core/v8/V8DOMWrapper.cpp:142:
RELEASE_ASSERT(currentWorld.worldId() ==
DOMWrapperWorld::world(contextForWrapper).worldId());

Does this need to be RELEASE_ASSERT?

[epertoso, 2015-09-09 15:43:49]

The CQ bit was unchecked by epertoso@chromium.org

[epertoso, 2015-09-09 15:52:11]

https://codereview.chromium.org/1323453005/diff/1/Source/bindings/core/v8/V8D...
File Source/bindings/core/v8/V8DOMWrapper.cpp (right):

https://codereview.chromium.org/1323453005/diff/1/Source/bindings/core/v8/V8D...
Source/bindings/core/v8/V8DOMWrapper.cpp:133: void
V8WrapperInstantiationScope::SecurityCheck(v8::Isolate* isolate,
v8::Local<v8::Context> contextForWrapper)
On 2015/09/09 at 15:41:43, haraken wrote:
> SecurityCheck => securityCheck

Done.

https://codereview.chromium.org/1323453005/diff/1/Source/bindings/core/v8/V8D...
Source/bindings/core/v8/V8DOMWrapper.cpp:135: if (!m_context.IsEmpty()) {
On 2015/09/09 at 15:41:43, haraken wrote:
> You can early-return if m_context is empty.

Done.

https://codereview.chromium.org/1323453005/diff/1/Source/bindings/core/v8/V8D...
Source/bindings/core/v8/V8DOMWrapper.cpp:142:
RELEASE_ASSERT(currentWorld.worldId() ==
DOMWrapperWorld::world(contextForWrapper).worldId());
On 2015/09/09 at 15:41:42, haraken wrote:
> Does this need to be RELEASE_ASSERT?

I think it's a valid assumption.

[epertoso, 2015-09-09 15:52:17]

The CQ bit was checked by epertoso@chromium.org

[epertoso, 2015-09-09 15:52:17]

The patchset sent to the CQ was uploaded after l-g-t-m from
haraken@chromium.org, jochen@chromium.org
Link to the patchset: https://codereview.chromium.org/1323453005/#ps20001
(title: "addressed haraken@'s comments.")

[commit-bot: I haz the power, 2015-09-09 15:52:30]

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

[commit-bot: I haz the power, 2015-09-09 18:24:15]

Committed patchset #2 (id:20001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=201987

[haraken, 2015-09-09 23:16:15]

BTW, would you help me understand why we need to limit the security check to the
main world? What's an issue of doing the check in other worlds?

[jochen (gone - plz use gerrit), 2015-09-10 07:53:04]

On 2015/09/09 at 23:16:15, haraken wrote:
> BTW, would you help me understand why we need to limit the security check to
the main world? What's an issue of doing the check in other worlds?

the security token of the isolated world is chrome-extension://abcd..., so the
security check against the Frame's security origin will always fail.

so for isolated worlds, all we check is that the access is within the same
isolated world.

yes, not being able to do proper security checks on isolated worlds is a
problem, cc'd you on a bug

[haraken, 2015-09-10 07:55:30]

On 2015/09/10 07:53:04, jochen wrote:
> On 2015/09/09 at 23:16:15, haraken wrote:
> > BTW, would you help me understand why we need to limit the security check to
> the main world? What's an issue of doing the check in other worlds?
> 
> the security token of the isolated world is chrome-extension://abcd..., so the
> security check against the Frame's security origin will always fail.
> 
> so for isolated worlds, all we check is that the access is within the same
> isolated world.
> 
> yes, not being able to do proper security checks on isolated worlds is a
> problem, cc'd you on a bug

Thanks for the context!

[commit-bot: I haz the power, 2015-09-23 12:01:07]

Patchset 2 (id:??) landed as
https://crrev.com/6daf0cab15080f3bc7bca63382cb9c28825cd54a