|
|
DescriptionCount cross-origin property access.
It would be interesting to find out how often pages try to access
properties of cross-origin Windows, and equally interesting to determine
how many of those accesses are enabled by `window.open`.
R=jochen@chromium.org
Review-Url: https://codereview.chromium.org/2881393002
Cr-Commit-Position: refs/heads/master@{#473112}
Committed: https://chromium.googlesource.com/chromium/src/+/6dc2830c525f150b928052245fd669834eee5f4c
Patch Set 1 #
Total comments: 4
Patch Set 2 : Tests. #Patch Set 3 : Rebase. #
Total comments: 2
Patch Set 4 : Nits + Rebase. #Messages
Total messages: 45 (24 generated)
The CQ bit was checked by mkwst@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Jochen, WDYT? https://codereview.chromium.org/2881393002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp (right): https://codereview.chromium.org/2881393002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:129: } Does this do what I think it does? https://codereview.chromium.org/2881393002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h (left): https://codereview.chromium.org/2881393002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.h:78: ExceptionState&); // NOLINT(readability/parameter_name) This turns out to be dead code. And it was confusing me, so I killed it.
dcheng@chromium.org changed reviewers: + dcheng@chromium.org
Can we move the recording into CanAccessFrame()?
On 2017/05/16 at 15:19:47, dcheng wrote: > Can we move the recording into CanAccessFrame()? Not without also passing in some sort of flag; I think these four `ShouldAllowAccessTo` variants are the only ones called from `[CrossOrigin]` IDL bindings. Things like the `Node` variant are called for `iframe.contentDocument` and the like (which aren't available cross-origin), for instance. But, honestly, I'm not at all sure what some of these are supposed to be used for, so I might be misinterpreting the call hierarchy. The comments could use some work. :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/05/16 17:07:55, Mike West wrote: > On 2017/05/16 at 15:19:47, dcheng wrote: > > Can we move the recording into CanAccessFrame()? > > Not without also passing in some sort of flag; I think these four > `ShouldAllowAccessTo` variants are the only ones called from `[CrossOrigin]` IDL > bindings. Things like the `Node` variant are called for `iframe.contentDocument` > and the like (which aren't available cross-origin), for instance. > > But, honestly, I'm not at all sure what some of these are supposed to be used > for, so I might be misinterpreting the call hierarchy. The comments could use > some work. :) Maybe I'm excessively jetlagged, but what flag would need to plumbed in?
yukishiino@chromium.org changed reviewers: + yukishiino@chromium.org
LGTM on my side. Defer to dcheng@ and jochen@ for details. https://codereview.chromium.org/2881393002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp (right): https://codereview.chromium.org/2881393002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:146: if (!can_access) { If we're going to put this into CanAccessFrame, then it's fine. Otherwise, can we define a helper function? IIUC, I see the same code many times.
On 2017/05/17 at 04:19:44, dcheng wrote: > On 2017/05/16 17:07:55, Mike West wrote: > > On 2017/05/16 at 15:19:47, dcheng wrote: > > > Can we move the recording into CanAccessFrame()? > > > > Not without also passing in some sort of flag; I think these four > > `ShouldAllowAccessTo` variants are the only ones called from `[CrossOrigin]` IDL > > bindings. Things like the `Node` variant are called for `iframe.contentDocument` > > and the like (which aren't available cross-origin), for instance. > > > > But, honestly, I'm not at all sure what some of these are supposed to be used > > for, so I might be misinterpreting the call hierarchy. The comments could use > > some work. :) > > Maybe I'm excessively jetlagged, but what flag would need to plumbed in? "Is this call the result of bindings code that we generated in response to `[CrossOrigin]`?" That is, I'm not terribly interested in logging accesses that we'd never ever grant. That is, something like `iframe.contentDocument` isn't `[CrossOrigin]`, so the access check we do is a hard failure. I want to figure out how often we do the soft failure of something like `window.length` or `window.location` that's available in some form or another cross-origin.
also note that an access might be denied for same origin but not same origin domain windows, as well as detached frames. https://codereview.chromium.org/2881393002/diff/1/third_party/WebKit/Source/b... File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp (right): https://codereview.chromium.org/2881393002/diff/1/third_party/WebKit/Source/b... third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:129: } On 2017/05/16 at 14:54:31, Mike West wrote: > Does this do what I think it does? not sure, but kCrossOriginPropertyAccessFromOpener might undercount, if target nulled out its opener
The CQ bit was checked by mkwst@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
The CQ bit was checked by mkwst@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
I added tests, and taught SimTest to open windows. How exciting! There's probably a more generic way of turning off the popup blocker... Jochen mentioned something? On 2017/05/17 at 07:36:06, jochen wrote: > also note that an access might be denied for same origin but not same origin domain windows, as well as detached frames. I think we skip out on detached frames before counting. I'd also want the behavior to be the same for domain differences as for host differences, so I'm not terribly concerned at conflating those. > > https://codereview.chromium.org/2881393002/diff/1/third_party/WebKit/Source/b... > File third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp (right): > > https://codereview.chromium.org/2881393002/diff/1/third_party/WebKit/Source/b... > third_party/WebKit/Source/bindings/core/v8/BindingSecurity.cpp:129: } > On 2017/05/16 at 14:54:31, Mike West wrote: > > Does this do what I think it does? > > not sure, but kCrossOriginPropertyAccessFromOpener might undercount, if target nulled out its opener I think that's correct. I don't know how to fix that without storing something like "The Real Opener (Really)" and preventing the frame from clearing that. Not sure that's a good idea, honestly.
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
bindings/ LGTM
On 2017/05/17 15:00:27, haraken1 wrote: > bindings/ LGTM LGTM from a right account.
https://codereview.chromium.org/2881393002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/BindingSecurityTest.cpp (right): https://codereview.chromium.org/2881393002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/BindingSecurityTest.cpp:18: const char* kMainFrame = "https://example.com/main.html"; Nit: const char kMainFrame[] (kMainFrame is actually assignable as written) https://codereview.chromium.org/2881393002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/BindingSecurityTest.cpp:113: "postMessage")); My impression was that the use counter didn't want to measure when JS does something like crossOriginWindow.abc, but I think the current one will. Is that OK? Do we want to explicitly test for that?
On 2017/05/17 at 22:10:45, dcheng wrote: > https://codereview.chromium.org/2881393002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/bindings/core/v8/BindingSecurityTest.cpp (right): > > https://codereview.chromium.org/2881393002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/bindings/core/v8/BindingSecurityTest.cpp:18: const char* kMainFrame = "https://example.com/main.html"; > Nit: const char kMainFrame[] > > (kMainFrame is actually assignable as written) > > https://codereview.chromium.org/2881393002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/bindings/core/v8/BindingSecurityTest.cpp:113: "postMessage")); > My impression was that the use counter didn't want to measure when JS does something like crossOriginWindow.abc, but I think the current one will. Is that OK? Do we want to explicitly test for that? I think accessing the property is enough. I guess we could measure method invocations separately, but the only one I actually care about separately from other properties is `postMessage`, which I think we already have metrics for somewhere.
On 2017/05/18 05:03:38, Mike West wrote: > On 2017/05/17 at 22:10:45, dcheng wrote: > > > https://codereview.chromium.org/2881393002/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/bindings/core/v8/BindingSecurityTest.cpp > (right): > > > > > https://codereview.chromium.org/2881393002/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/bindings/core/v8/BindingSecurityTest.cpp:18: const > char* kMainFrame = "https://example.com/main.html"; > > Nit: const char kMainFrame[] > > > > (kMainFrame is actually assignable as written) > > > > > https://codereview.chromium.org/2881393002/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/bindings/core/v8/BindingSecurityTest.cpp:113: > "postMessage")); > > My impression was that the use counter didn't want to measure when JS does > something like crossOriginWindow.abc, but I think the current one will. Is that > OK? Do we want to explicitly test for that? > > I think accessing the property is enough. I guess we could measure method > invocations separately, but the only one I actually care about separately from > other properties is `postMessage`, which I think we already have metrics for > somewhere. LGTM with the constant nits addressed
The CQ bit was checked by mkwst@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yukishiino@chromium.org, haraken@chromium.org, haraken@google.com, dcheng@chromium.org Link to the patchset: https://codereview.chromium.org/2881393002/#ps60001 (title: "Nits + Rebase.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by mkwst@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by mkwst@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
The CQ bit was checked by mkwst@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 60001, "attempt_start_ts": 1495168626343670, "parent_rev": "7f10adc089153731d35a4a04956d7673342db2e4", "commit_rev": "6dc2830c525f150b928052245fd669834eee5f4c"}
Message was sent while issue was closed.
Description was changed from ========== Count cross-origin property access. It would be interesting to find out how often pages try to access properties of cross-origin Windows, and equally interesting to determine how many of those accesses are enabled by `window.open`. R=jochen@chromium.org ========== to ========== Count cross-origin property access. It would be interesting to find out how often pages try to access properties of cross-origin Windows, and equally interesting to determine how many of those accesses are enabled by `window.open`. R=jochen@chromium.org Review-Url: https://codereview.chromium.org/2881393002 Cr-Commit-Position: refs/heads/master@{#473112} Committed: https://chromium.googlesource.com/chromium/src/+/6dc2830c525f150b928052245fd6... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/6dc2830c525f150b928052245fd6... |