Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(12)

Issue 2209303002: binding: Moves the check for the first access to the initial document into BindingSecurity. (Closed)

Created:
2 years, 9 months ago by Yuki
Modified:
2 years, 9 months ago
Reviewers:
haraken, benwells, Torne, meacer
CC:
chromium-reviews, blink-reviews, blink-reviews-bindings_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

binding: Moves the check for the first access to the initial document into BindingSecurity. Checks the access to the initial document and reports it not only at securityCheck() in V8Window.cpp but also at every call to BindingSecurity::shouldAllowAccessTo() because V8 only calls back securityCheck() on property lookups, and not for function invocation. BindingSecurity::shouldAllowAccessTo() is called with every possible cross-origin window, which means every possible new window. Thus, shouldAllowAccessTo() should be the right place to check the access to the initial document. BUG=630662 TBR=benwells@chromium.org Committed: https://crrev.com/b8dcfeb065bbfd777cdc5f5433da9a87f25e6ec6 Cr-Commit-Position: refs/heads/master@{#412195}

Patch Set 1 #

Patch Set 2 : Synced. #

Patch Set 3 : . #

Patch Set 4 : . #

Patch Set 5 : Synced. #

Patch Set 6 : Synced. #

Patch Set 7 : Fixed android_webview tests. #

Total comments: 6

Patch Set 8 : Addressed review comments. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+65 lines, -44 lines) Patch
M android_webview/javatests/src/org/chromium/android_webview/test/AwTestBase.java View 1 2 3 4 5 6 1 chunk +9 lines, -2 lines 0 comments Download
M android_webview/javatests/src/org/chromium/android_webview/test/PopupWindowTest.java View 1 2 3 4 5 6 1 chunk +11 lines, -3 lines 0 comments Download
M chrome/browser/apps/app_browsertest.cc View 1 2 3 4 5 6 7 2 chunks +3 lines, -0 lines 0 comments Download
M chrome/browser/ssl/ssl_browser_tests.cc View 1 2 3 4 5 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h View 1 chunk +3 lines, -0 lines 0 comments Download
M third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp View 1 2 3 4 5 6 7 8 chunks +37 lines, -25 lines 0 comments Download
M third_party/WebKit/Source/bindings/templates/interface_base.cpp View 1 2 3 4 1 chunk +1 line, -13 lines 0 comments Download

Messages

Total messages: 44 (34 generated)
Yuki
Could you guys review this CL? haraken@ as the main reviewer This CL makes Blink ...
2 years, 9 months ago (2016-08-12 14:14:05 UTC) #26
Torne
android_webview test updates LGTM
2 years, 9 months ago (2016-08-12 15:41:39 UTC) #27
meacer
chrome/browser/ssl/ LGTM
2 years, 9 months ago (2016-08-12 17:48:41 UTC) #28
haraken
LGTM https://codereview.chromium.org/2209303002/diff/120001/third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp (right): https://codereview.chromium.org/2209303002/diff/120001/third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp#newcode49 third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:49: SECURITY_DCHECK(!(targetWindow && targetWindow->frame()) I think it's worth changing ...
2 years, 9 months ago (2016-08-13 02:29:36 UTC) #29
Yuki
https://codereview.chromium.org/2209303002/diff/120001/third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp (right): https://codereview.chromium.org/2209303002/diff/120001/third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp#newcode49 third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:49: SECURITY_DCHECK(!(targetWindow && targetWindow->frame()) On 2016/08/13 02:29:36, haraken wrote: > ...
2 years, 9 months ago (2016-08-15 03:42:58 UTC) #32
haraken
LGTM
2 years, 9 months ago (2016-08-15 04:32:12 UTC) #33
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2209303002/140001
2 years, 9 months ago (2016-08-16 05:36:40 UTC) #39
commit-bot: I haz the power
Committed patchset #8 (id:140001)
2 years, 9 months ago (2016-08-16 08:55:29 UTC) #41
commit-bot: I haz the power
Patchset 8 (id:??) landed as https://crrev.com/b8dcfeb065bbfd777cdc5f5433da9a87f25e6ec6 Cr-Commit-Position: refs/heads/master@{#412195}
2 years, 9 months ago (2016-08-16 08:57:40 UTC) #43
benwells
2 years, 9 months ago (2016-08-16 23:20:27 UTC) #44
Message was sent while issue was closed.
lgtm

Powered by Google App Engine
This is Rietveld 408576698