Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1862953002:
Disallow use of Web Bluetooth from cross-origin iframes. (Closed)
|
Created:
4 years, 8 months ago by Jeffrey Yasskin Modified:
4 years, 8 months ago CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, blink-reviews, blink-reviews-api_chromium.org, chromium-reviews, darin (slow to review), darin-cc_chromium.org, dglazkov+blink, haraken, jam, Mike West, ortuno+watch_chromium.org, qsr+mojo_chromium.org, scheib+watch_chromium.org, scheib, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@lkcr Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDisallow use of Web Bluetooth from cross-origin iframes.
This should be a little safer from abuse for the EF launch and more
forward-compatible with
https://noncombatant.github.io/permission-delegation-api/.
Also refactored the tests to let all iframe tests share a single helper resource.
BUG=518042
Committed: https://crrev.com/4eca58f59046e7832e83a13ca150d2380319e0ff
Cr-Commit-Position: refs/heads/master@{#385653}
Patch Set 1 #
Total comments: 8
Patch Set 2 : Use IsSameOriginWith instead of !=; still needs a test before submitting. #
Total comments: 2
Patch Set 3 : Add a test. #Depends on Patchset: Messages
Total messages: 34 (15 generated)
jyasskin@chromium.org changed reviewers: + mkwst@chromium.org, ortuno@chromium.org, palmer@chromium.org, raymes@chromium.org
mkwst for url/origin.h palmer and raymes for this first bad approximation at the delegation API. This doesn't have a cross-origin test because it's tricky to set up a multi-origin LayoutTest, but the sandboxed iframe test covers the same codepath. https://codereview.chromium.org/1862953002/diff/1/content/browser/bluetooth/b... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (left): https://codereview.chromium.org/1862953002/diff/1/content/browser/bluetooth/b... content/browser/bluetooth/bluetooth_dispatcher_host.cc:1141: VLOG(1) << "Request device with unique origin."; I'm dropping the VLOG because we send the same information to the renderer's console, and it's nice to avoid extra strings in the release build. https://codereview.chromium.org/1862953002/diff/1/url/origin.h File url/origin.h (right): https://codereview.chromium.org/1862953002/diff/1/url/origin.h#newcode140 url/origin.h:140: inline URL_EXPORT bool operator!=(const Origin& a, const Origin& b) { I'm not certain it makes sense to have the non-reflexive operator==, but if we have that, we should have != to match.
https://codereview.chromium.org/1862953002/diff/1/content/browser/bluetooth/b... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1862953002/diff/1/content/browser/bluetooth/b... content/browser/bluetooth/bluetooth_dispatcher_host.cc:1143: if (embedding_origin != requesting_origin) { I'd prefer `!embedding_origin.IsSameOriginWith(requesting_origin)` here. As noted down in origin.h, we only added `==` to make test matchers like `EXPECT_EQ` happy, as they provide helpful output. Given the non-reflexive behavior, I think it might be confusing for `==` to spread throughout the codebase. https://codereview.chromium.org/1862953002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/bluetooth/requestDevice-sandboxed-iframe.html (right): https://codereview.chromium.org/1862953002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/bluetooth/requestDevice-sandboxed-iframe.html:17: 'called from cross-origin iframe.'); Hrm. Can you talk about the trickyness a bit? Loading cross-origin frames is certainly possible. For example, you could load `http://127.0.0.1:8080/resources/requestDevice-in-sandboxed-iframe.html` or `https://example.test:8443/resources/requestDevice-in-sandboxed-iframe.html` if you'd like to actually test a cross-origin-but-not-sandboxed frame. I think that would be a good idea, since sandboxing has a lot of side-effects. https://codereview.chromium.org/1862953002/diff/1/url/origin.h File url/origin.h (right): https://codereview.chromium.org/1862953002/diff/1/url/origin.h#newcode140 url/origin.h:140: inline URL_EXPORT bool operator!=(const Origin& a, const Origin& b) { On 2016/04/06 at 05:38:31, Jeffrey Yasskin wrote: > I'm not certain it makes sense to have the non-reflexive operator==, but if we have that, we should have != to match. We only added `==` for nice output in tests from `EXPECT_EQ` and the like. I think it would be confusing for `==` or `!=` to spread further in the codebase, given the non-reflexive behavior you noted. I don't think adding `!=` is a good idea. On a side note: is there a difference in behavior between this form of an operator declaration and the class-bound form used for `==`?
The CQ bit was checked by jyasskin@chromium.org to run a CQ dry run
jyasskin@chromium.org changed reviewers: - mkwst@chromium.org
https://codereview.chromium.org/1862953002/diff/1/content/browser/bluetooth/b... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1862953002/diff/1/content/browser/bluetooth/b... content/browser/bluetooth/bluetooth_dispatcher_host.cc:1143: if (embedding_origin != requesting_origin) { On 2016/04/06 06:06:59, Mike West (OOO through 31st) wrote: > I'd prefer `!embedding_origin.IsSameOriginWith(requesting_origin)` here. As > noted down in origin.h, we only added `==` to make test matchers like > `EXPECT_EQ` happy, as they provide helpful output. Given the non-reflexive > behavior, I think it might be confusing for `==` to spread throughout the > codebase. Done, thanks for the clear opinion and rationale. https://codereview.chromium.org/1862953002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/bluetooth/requestDevice-sandboxed-iframe.html (right): https://codereview.chromium.org/1862953002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/bluetooth/requestDevice-sandboxed-iframe.html:17: 'called from cross-origin iframe.'); On 2016/04/06 06:06:59, Mike West (OOO through 31st) wrote: > Hrm. Can you talk about the trickyness a bit? Loading cross-origin frames is > certainly possible. For example, you could load > `http://127.0.0.1:8080/resources/requestDevice-in-sandboxed-iframe.html` or > `https://example.test:8443/resources/requestDevice-in-sandboxed-iframe.html` if > you'd like to actually test a cross-origin-but-not-sandboxed frame. I think that > would be a good idea, since sandboxing has a lot of side-effects. Yep, I'd missed those options. I'll write that test tomorrow. https://codereview.chromium.org/1862953002/diff/1/url/origin.h File url/origin.h (right): https://codereview.chromium.org/1862953002/diff/1/url/origin.h#newcode140 url/origin.h:140: inline URL_EXPORT bool operator!=(const Origin& a, const Origin& b) { On 2016/04/06 06:06:59, Mike West (OOO through 31st) wrote: > On 2016/04/06 at 05:38:31, Jeffrey Yasskin wrote: > > I'm not certain it makes sense to have the non-reflexive operator==, but if we > have that, we should have != to match. > > We only added `==` for nice output in tests from `EXPECT_EQ` and the like. I > think it would be confusing for `==` or `!=` to spread further in the codebase, > given the non-reflexive behavior you noted. I don't think adding `!=` is a good > idea. Removed. > On a side note: is there a difference in behavior between this form of an > operator declaration and the class-bound form used for `==`? Yes: the member form allows implicit conversions (if any) for the right side of the operator but not the left. The free function form allows implicit conversions (if any) for both sides. For Origin they should be the same.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1862953002/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1862953002/20001
Generally looks good to me, thanks for the FYI!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with 1 question. https://codereview.chromium.org/1862953002/diff/20001/content/browser/bluetoo... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1862953002/diff/20001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_dispatcher_host.cc:1150: // themselves. Therefore I don't understand why line 1151 is necessary. What am I missing?
https://codereview.chromium.org/1862953002/diff/20001/content/browser/bluetoo... File content/browser/bluetooth/bluetooth_dispatcher_host.cc (right): https://codereview.chromium.org/1862953002/diff/20001/content/browser/bluetoo... content/browser/bluetooth/bluetooth_dispatcher_host.cc:1150: // themselves. On 2016/04/06 19:44:35, palmer wrote: > Therefore I don't understand why line 1151 is necessary. What am I missing? It's not necessary; just an executable comment.
The CQ bit was checked by jyasskin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1862953002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1862953002/40001
This depends on https://codereview.chromium.org/1869523003/ now to share the resources with the new HTTP test.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios_dbg_simulator_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...) ios_rel_device_gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_gn...) ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
The CQ bit was checked by jyasskin@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1862953002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1862953002/40001
Description was changed from ========== Disallow use of Web Bluetooth from cross-origin iframes. This should be a little safer from abuse for the EF launch and more forward-compatible with https://noncombatant.github.io/permission-delegation-api/. BUG=518042 ========== to ========== Disallow use of Web Bluetooth from cross-origin iframes. This should be a little safer from abuse for the EF launch and more forward-compatible with https://noncombatant.github.io/permission-delegation-api/. Also refactored the tests to let all iframe tests share a single helper resource. BUG=518042 ==========
lgtm
The CQ bit was unchecked by jyasskin@chromium.org
The CQ bit was checked by jyasskin@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org Link to the patchset: https://codereview.chromium.org/1862953002/#ps40001 (title: "Add a test.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1862953002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1862953002/40001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by jyasskin@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1862953002/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1862953002/40001
Message was sent while issue was closed.
Description was changed from ========== Disallow use of Web Bluetooth from cross-origin iframes. This should be a little safer from abuse for the EF launch and more forward-compatible with https://noncombatant.github.io/permission-delegation-api/. Also refactored the tests to let all iframe tests share a single helper resource. BUG=518042 ========== to ========== Disallow use of Web Bluetooth from cross-origin iframes. This should be a little safer from abuse for the EF launch and more forward-compatible with https://noncombatant.github.io/permission-delegation-api/. Also refactored the tests to let all iframe tests share a single helper resource. BUG=518042 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Failed to apply patch for
third_party/WebKit/LayoutTests/http/tests/bluetooth/https/requestDevice/cross-origin-iframe.html:
While running git apply --index -3 -p1;
Falling back to three-way merge...
Applied patch to
'third_party/WebKit/LayoutTests/http/tests/bluetooth/https/requestDevice/cross-origin-iframe.html'
with conflicts.
U
third_party/WebKit/LayoutTests/http/tests/bluetooth/https/requestDevice/cross-origin-iframe.html
Patch: NR
third_party/WebKit/LayoutTests/bluetooth/requestDevice-sandboxed-iframe.html->third_party/WebKit/LayoutTests/http/tests/bluetooth/https/requestDevice/cross-origin-iframe.html
Index:
third_party/WebKit/LayoutTests/http/tests/bluetooth/https/requestDevice/cross-origin-iframe.html
diff --git
a/third_party/WebKit/LayoutTests/bluetooth/requestDevice-sandboxed-iframe.html
b/third_party/WebKit/LayoutTests/http/tests/bluetooth/https/requestDevice/cross-origin-iframe.html
similarity index 61%
copy from
third_party/WebKit/LayoutTests/bluetooth/requestDevice-sandboxed-iframe.html
copy to
third_party/WebKit/LayoutTests/http/tests/bluetooth/https/requestDevice/cross-origin-iframe.html
index
e75b1312e3da0c0923d33ff03b2d91add74aa364..11917a25e81e30a19c18c0724624b08a07936f93
100644
---
a/third_party/WebKit/LayoutTests/bluetooth/requestDevice-sandboxed-iframe.html
+++
b/third_party/WebKit/LayoutTests/http/tests/bluetooth/https/requestDevice/cross-origin-iframe.html
@@ -1,7 +1,7 @@
<!DOCTYPE html>
-<script src="../resources/testharness.js"></script>
-<script src="../resources/testharnessreport.js"></script>
-<script src="../resources/bluetooth/bluetooth-helpers.js"></script>
+<script src="/js-test-resources/testharness.js"></script>
+<script src="/js-test-resources/testharnessreport.js"></script>
+<script src="/js-test-resources/bluetooth/bluetooth-helpers.js"></script>
<body>
<script>
"use strict";
@@ -14,16 +14,14 @@
});
} else {
assert_equals(messageEvent.data, 'SecurityError: requestDevice() ' +
- 'called from sandboxed or otherwise '
+
- 'unique origin.');
+ 'called from cross-origin iframe.');
test.done();
}
});
setBluetoothFakeAdapter('HeartRateAdapter')
.then(() => {
let iframe = document.createElement('iframe');
- iframe.sandbox.add('allow-scripts');
- iframe.src =
'../resources/bluetooth/requestDevice-in-sandboxed-iframe.html';
+ iframe.src =
'https://localhost:8443/js-test-resources/bluetooth/requestDevice-in-iframe.html';
document.body.appendChild(iframe);
});
}, 'Request device from a unique origin. Should reject with SecurityError.');
Message was sent while issue was closed.
Description was changed from ========== Disallow use of Web Bluetooth from cross-origin iframes. This should be a little safer from abuse for the EF launch and more forward-compatible with https://noncombatant.github.io/permission-delegation-api/. Also refactored the tests to let all iframe tests share a single helper resource. BUG=518042 ========== to ========== Disallow use of Web Bluetooth from cross-origin iframes. This should be a little safer from abuse for the EF launch and more forward-compatible with https://noncombatant.github.io/permission-delegation-api/. Also refactored the tests to let all iframe tests share a single helper resource. BUG=518042 Committed: https://crrev.com/4eca58f59046e7832e83a13ca150d2380319e0ff Cr-Commit-Position: refs/heads/master@{#385653} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/4eca58f59046e7832e83a13ca150d2380319e0ff Cr-Commit-Position: refs/heads/master@{#385653} |
