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

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

Created:
3 years ago by Yuki
Modified:
3 years 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 ...
3 years ago (2016-08-12 14:14:05 UTC) #26
Torne
android_webview test updates LGTM
3 years ago (2016-08-12 15:41:39 UTC) #27
meacer
chrome/browser/ssl/ LGTM
3 years 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 ...
3 years 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: > ...
3 years ago (2016-08-15 03:42:58 UTC) #32
haraken
LGTM
3 years 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
3 years ago (2016-08-16 05:36:40 UTC) #39
commit-bot: I haz the power
Committed patchset #8 (id:140001)
3 years 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}
3 years ago (2016-08-16 08:57:40 UTC) #43
benwells
3 years 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