Use GetLastCommittedOrigin instead of GetLastCommittedURL in bluetooth chooser

Use GetLastCommittedOrigin instead of GetLastCommittedURL
in bluetooth chooser

This patch modifies bluetooth chooser code to use
GetLastCommittedOrigin instead of GetLastCommittedURL.

BUG=577451
Committed: https://crrev.com/598777dc91b52b4b50df7dddde64245c05733008
Cr-Commit-Position: refs/heads/master@{#371423}

[juncai, 2016-01-19 20:22:21]

Description was changed from

==========
Use GetLastCommittedOrigin instead of GetLastCommittedURL

This patch modifies bluetooth chooser code to use GetLastCommittedOrigin
instead of GetLastCommittedURL.

BUG=577451
==========

to

==========
Use GetLastCommittedOrigin instead of GetLastCommittedURL
in bluetooth chooser

This patch modifies bluetooth chooser code to use GetLastCommittedOrigin
instead of GetLastCommittedURL.

BUG=577451
==========

[juncai, 2016-01-19 20:22:52]

Description was changed from

==========
Use GetLastCommittedOrigin instead of GetLastCommittedURL
in bluetooth chooser

This patch modifies bluetooth chooser code to use GetLastCommittedOrigin
instead of GetLastCommittedURL.

BUG=577451
==========

to

==========
Use GetLastCommittedOrigin instead of GetLastCommittedURL
in bluetooth chooser

This patch modifies bluetooth chooser code to use
GetLastCommittedOrigin instead of GetLastCommittedURL.

BUG=577451
==========

[juncai, 2016-01-19 20:49:15]

juncai@chromium.org changed reviewers:
+ jyasskin@chromium.org

[juncai, 2016-01-19 20:49:15]

Please review this patch.

[ortuno, 2016-01-19 20:59:07]

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

[palmer, 2016-01-20 19:03:15]

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

[palmer, 2016-01-20 19:03:15]

LGTM. Thanks!

[Jeffrey Yasskin, 2016-01-20 19:34:08]

lgtm

[juncai, 2016-01-20 20:20:58]

juncai@chromium.org changed reviewers:
+ msw@chromium.org, nick@chromium.org, yfriedman@chromium.org

[juncai, 2016-01-20 20:20:58]

msw@chromium.org: Please review changes in 
//chrome/browser/ui/browser.cc
//chrome/browser/ui/browser.h

yfriedman@chromium.org: Please review changes in 
//chrome/browser/android/
//chrome/browser/ui/android/

nick@chromium.org: Please review changes in 
//content/

[ncarter (slow), 2016-01-20 20:59:44]

content lgtm, one question about correctness.

https://codereview.chromium.org/1601323002/diff/1/chrome/browser/ui/android/b...
File chrome/browser/ui/android/bluetooth_chooser_android.cc (right):

https://codereview.chromium.org/1601323002/diff/1/chrome/browser/ui/android/b...
chrome/browser/ui/android/bluetooth_chooser_android.cc:38:
ConvertUTF8ToJavaString(env, origin.Serialize());
What if the origin is unique? Do we still want to show a chooser in that case?

One way that can happen, is with <iframe sandbox>. Also, "data:" URLs are placed
in unique origins.

[Jeffrey Yasskin, 2016-01-20 21:29:58]

https://codereview.chromium.org/1601323002/diff/1/chrome/browser/ui/android/b...
File chrome/browser/ui/android/bluetooth_chooser_android.cc (right):

https://codereview.chromium.org/1601323002/diff/1/chrome/browser/ui/android/b...
chrome/browser/ui/android/bluetooth_chooser_android.cc:38:
ConvertUTF8ToJavaString(env, origin.Serialize());
On 2016/01/20 20:59:44, ncarter wrote:
> What if the origin is unique? Do we still want to show a chooser in that case?
> 
> One way that can happen, is with <iframe sandbox>. Also, "data:" URLs are
placed
> in unique origins.

Good point. We don't want to show the user the string of a unique origin, but we
need to do something principled instead. According to
https://w3c.github.io/webappsec-secure-contexts/#potentially-trustworthy-origin,
data: URLs aren't secure contexts, so they won't show up here, but sandbox'ed
iframes might.

Jun, could you explicitly block unique origins in
BluetoothDispatcherHost::OnRequestDevice(), with a SecurityError ... add it to
https://code.google.com/p/chromium/codesearch/#chromium/src/third_party/WebKi...,
call it RequestDeviceWithUniqueOrigin, and in WebBluetoothError.cpp, give it the
string "requestDevice() called from sandboxed or otherwise unique origin." We'll
probably do something to allow those in the future, but we need to check with
the security-UX folks, and I don't want to block this patch on that.

[Jeffrey Yasskin, 2016-01-20 21:31:24]

On 2016/01/20 21:29:58, Jeffrey Yasskin wrote:
>
https://codereview.chromium.org/1601323002/diff/1/chrome/browser/ui/android/b...
> File chrome/browser/ui/android/bluetooth_chooser_android.cc (right):
> 
>
https://codereview.chromium.org/1601323002/diff/1/chrome/browser/ui/android/b...
> chrome/browser/ui/android/bluetooth_chooser_android.cc:38:
> ConvertUTF8ToJavaString(env, origin.Serialize());
> On 2016/01/20 20:59:44, ncarter wrote:
> > What if the origin is unique? Do we still want to show a chooser in that
case?
> > 
> > One way that can happen, is with <iframe sandbox>. Also, "data:" URLs are
> placed
> > in unique origins.
> 
> Good point. We don't want to show the user the string of a unique origin, but
we
> need to do something principled instead. According to
>
https://w3c.github.io/webappsec-secure-contexts/#potentially-trustworthy-origin,
> data: URLs aren't secure contexts, so they won't show up here, but sandbox'ed
> iframes might.
> 
> Jun, could you explicitly block unique origins in
> BluetoothDispatcherHost::OnRequestDevice(), with a SecurityError ... add it to
>
https://code.google.com/p/chromium/codesearch/#chromium/src/third_party/WebKi...,
> call it RequestDeviceWithUniqueOrigin, and in WebBluetoothError.cpp, give it
the
> string "requestDevice() called from sandboxed or otherwise unique origin."
We'll
> probably do something to allow those in the future, but we need to check with
> the security-UX folks, and I don't want to block this patch on that.

Oh, and then CHECK(!origin.unique()) here and anywhere else you're calling
.Serialize().

[ortuno, 2016-01-20 21:38:29]

On 2016/01/20 at 21:31:24, jyasskin wrote:
> On 2016/01/20 21:29:58, Jeffrey Yasskin wrote:
> >
https://codereview.chromium.org/1601323002/diff/1/chrome/browser/ui/android/b...
> > File chrome/browser/ui/android/bluetooth_chooser_android.cc (right):
> > 
> >
https://codereview.chromium.org/1601323002/diff/1/chrome/browser/ui/android/b...
> > chrome/browser/ui/android/bluetooth_chooser_android.cc:38:
> > ConvertUTF8ToJavaString(env, origin.Serialize());
> > On 2016/01/20 20:59:44, ncarter wrote:
> > > What if the origin is unique? Do we still want to show a chooser in that
case?
> > > 
> > > One way that can happen, is with <iframe sandbox>. Also, "data:" URLs are
> > placed
> > > in unique origins.
> > 
> > Good point. We don't want to show the user the string of a unique origin,
but we
> > need to do something principled instead. According to
> >
https://w3c.github.io/webappsec-secure-contexts/#potentially-trustworthy-origin,
> > data: URLs aren't secure contexts, so they won't show up here, but
sandbox'ed
> > iframes might.
> > 
> > Jun, could you explicitly block unique origins in
> > BluetoothDispatcherHost::OnRequestDevice(), with a SecurityError ... add it
to
> >
https://code.google.com/p/chromium/codesearch/#chromium/src/third_party/WebKi...,
> > call it RequestDeviceWithUniqueOrigin, and in WebBluetoothError.cpp, give it
the
> > string "requestDevice() called from sandboxed or otherwise unique origin."
We'll
> > probably do something to allow those in the future, but we need to check
with
> > the security-UX folks, and I don't want to block this patch on that.
> 
> Oh, and then CHECK(!origin.unique()) here and anywhere else you're calling
.Serialize().

Should we add a layouttest for that as well?

[ncarter (slow), 2016-01-20 22:00:20]

https://codereview.chromium.org/1601323002/diff/1/chrome/browser/ui/android/b...
File chrome/browser/ui/android/bluetooth_chooser_android.cc (right):

https://codereview.chromium.org/1601323002/diff/1/chrome/browser/ui/android/b...
chrome/browser/ui/android/bluetooth_chooser_android.cc:38:
ConvertUTF8ToJavaString(env, origin.Serialize());
On 2016/01/20 21:29:58, Jeffrey Yasskin wrote:
> On 2016/01/20 20:59:44, ncarter wrote:
> > What if the origin is unique? Do we still want to show a chooser in that
case?
> > 
> > One way that can happen, is with <iframe sandbox>. Also, "data:" URLs are
> placed
> > in unique origins.
> 
> Good point. We don't want to show the user the string of a unique origin, but
we
> need to do something principled instead. According to
>
https://w3c.github.io/webappsec-secure-contexts/#potentially-trustworthy-origin,
> data: URLs aren't secure contexts, so they won't show up here, but sandbox'ed
> iframes might.
> 
> Jun, could you explicitly block unique origins in
> BluetoothDispatcherHost::OnRequestDevice(), with a SecurityError ... add it to
>
https://code.google.com/p/chromium/codesearch/#chromium/src/third_party/WebKi...,
> call it RequestDeviceWithUniqueOrigin, and in WebBluetoothError.cpp, give it
the
> string "requestDevice() called from sandboxed or otherwise unique origin."
We'll
> probably do something to allow those in the future, but we need to check with
> the security-UX folks, and I don't want to block this patch on that.

Thanks for the spec link, that was good reading.

This pattern of falling back to the URL origin has come up in a couple different
places already: (see e.g.
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/sit...).
I am starting to wonder if it ought to be a getter that does that by default. In
any case: I'm very much open to suggestions for how to make
GetLastCommittedOrigin() easier for clients to use!

[Jeffrey Yasskin, 2016-01-20 22:08:28]

On 2016/01/20 21:38:29, ortuno wrote:
> On 2016/01/20 at 21:31:24, jyasskin wrote:
> > On 2016/01/20 21:29:58, Jeffrey Yasskin wrote:
> > >
>
https://codereview.chromium.org/1601323002/diff/1/chrome/browser/ui/android/b...
> > > File chrome/browser/ui/android/bluetooth_chooser_android.cc (right):
> > > 
> > >
>
https://codereview.chromium.org/1601323002/diff/1/chrome/browser/ui/android/b...
> > > chrome/browser/ui/android/bluetooth_chooser_android.cc:38:
> > > ConvertUTF8ToJavaString(env, origin.Serialize());
> > > On 2016/01/20 20:59:44, ncarter wrote:
> > > > What if the origin is unique? Do we still want to show a chooser in that
> case?
> > > > 
> > > > One way that can happen, is with <iframe sandbox>. Also, "data:" URLs
are
> > > placed
> > > > in unique origins.
> > > 
> > > Good point. We don't want to show the user the string of a unique origin,
> but we
> > > need to do something principled instead. According to
> > >
>
https://w3c.github.io/webappsec-secure-contexts/#potentially-trustworthy-origin,
> > > data: URLs aren't secure contexts, so they won't show up here, but
> sandbox'ed
> > > iframes might.
> > > 
> > > Jun, could you explicitly block unique origins in
> > > BluetoothDispatcherHost::OnRequestDevice(), with a SecurityError ... add
it
> to
> > >
>
https://code.google.com/p/chromium/codesearch/#chromium/src/third_party/WebKi...,
> > > call it RequestDeviceWithUniqueOrigin, and in WebBluetoothError.cpp, give
it
> the
> > > string "requestDevice() called from sandboxed or otherwise unique origin."
> We'll
> > > probably do something to allow those in the future, but we need to check
> with
> > > the security-UX folks, and I don't want to block this patch on that.
> > 
> > Oh, and then CHECK(!origin.unique()) here and anywhere else you're calling
> .Serialize().
> 
> Should we add a layouttest for that as well?

We should, but I don't want to block this CL on it. I've filed
https://crbug.com/579691 and https://crbug.com/579689 so we don't forget to test
this and get it fully working.

[Jeffrey Yasskin, 2016-01-20 22:29:20]

On 2016/01/20 22:00:20, ncarter wrote:
>
https://codereview.chromium.org/1601323002/diff/1/chrome/browser/ui/android/b...
> File chrome/browser/ui/android/bluetooth_chooser_android.cc (right):
> 
>
https://codereview.chromium.org/1601323002/diff/1/chrome/browser/ui/android/b...
> chrome/browser/ui/android/bluetooth_chooser_android.cc:38:
> ConvertUTF8ToJavaString(env, origin.Serialize());
> On 2016/01/20 21:29:58, Jeffrey Yasskin wrote:
> > On 2016/01/20 20:59:44, ncarter wrote:
> > > What if the origin is unique? Do we still want to show a chooser in that
> case?
> > > 
> > > One way that can happen, is with <iframe sandbox>. Also, "data:" URLs are
> > placed
> > > in unique origins.
> > 
> > Good point. We don't want to show the user the string of a unique origin,
but
> we
> > need to do something principled instead. According to
> >
>
https://w3c.github.io/webappsec-secure-contexts/#potentially-trustworthy-origin,
> > data: URLs aren't secure contexts, so they won't show up here, but
sandbox'ed
> > iframes might.
> > 
> > Jun, could you explicitly block unique origins in
> > BluetoothDispatcherHost::OnRequestDevice(), with a SecurityError ... add it
to
> >
>
https://code.google.com/p/chromium/codesearch/#chromium/src/third_party/WebKi...,
> > call it RequestDeviceWithUniqueOrigin, and in WebBluetoothError.cpp, give it
> the
> > string "requestDevice() called from sandboxed or otherwise unique origin."
> We'll
> > probably do something to allow those in the future, but we need to check
with
> > the security-UX folks, and I don't want to block this patch on that.
> 
> Thanks for the spec link, that was good reading.
> 
> This pattern of falling back to the URL origin has come up in a couple
different
> places already: (see e.g.
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/sit...).
> I am starting to wonder if it ought to be a getter that does that by default.
In
> any case: I'm very much open to suggestions for how to make
> GetLastCommittedOrigin() easier for clients to use!

I don't feel like I've seen enough examples of this to figure out the right
abstraction. I'm not even sure that using the URL origin is the right thing to
do for <iframe sandbox> cases here. It's easy, but maybe we want to force
permissioned things to be able to act as their origin in order to ask the user
for a permission as that origin. If we do want to fall back to the URL,
 * the permission case is about having something human-readable to display (so
data: should be excluded),
 * the Document::isSecureContextImpl() case is about the protocol, which
sandboxing doesn't affect,
 * and the site_details.cc case seems to be about making sure that
extra-powerful contexts can't get forced into a compromised renderer just by
putting them in a sandbox?
They may need different getters or types.

I do wonder if Serialize() should CHECK(!unique()), since unique origins need
special handling in many cases.

[juncai, 2016-01-21 17:47:42]

https://codereview.chromium.org/1601323002/diff/1/chrome/browser/ui/android/b...
File chrome/browser/ui/android/bluetooth_chooser_android.cc (right):

https://codereview.chromium.org/1601323002/diff/1/chrome/browser/ui/android/b...
chrome/browser/ui/android/bluetooth_chooser_android.cc:38:
ConvertUTF8ToJavaString(env, origin.Serialize());
On 2016/01/20 21:29:58, Jeffrey Yasskin wrote:
> On 2016/01/20 20:59:44, ncarter wrote:
> > What if the origin is unique? Do we still want to show a chooser in that
case?
> > 
> > One way that can happen, is with <iframe sandbox>. Also, "data:" URLs are
> placed
> > in unique origins.
> 
> Good point. We don't want to show the user the string of a unique origin, but
we
> need to do something principled instead. According to
>
https://w3c.github.io/webappsec-secure-contexts/#potentially-trustworthy-origin,
> data: URLs aren't secure contexts, so they won't show up here, but sandbox'ed
> iframes might.
> 
> Jun, could you explicitly block unique origins in
> BluetoothDispatcherHost::OnRequestDevice(), with a SecurityError ... add it to
>
https://code.google.com/p/chromium/codesearch/#chromium/src/third_party/WebKi...,
> call it RequestDeviceWithUniqueOrigin, and in WebBluetoothError.cpp, give it
the
> string "requestDevice() called from sandboxed or otherwise unique origin."
We'll
> probably do something to allow those in the future, but we need to check with
> the security-UX folks, and I don't want to block this patch on that.

Done.

[Jeffrey Yasskin, 2016-01-21 17:56:16]

Thank! LGTM

[msw, 2016-01-21 18:28:58]

browser.[h|cc] lgtm

[Yaron, 2016-01-21 18:49:54]

lgtm

[juncai, 2016-01-26 01:52:39]

The CQ bit was checked by juncai@chromium.org

[juncai, 2016-01-26 01:52:40]

The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org,
palmer@chromium.org, msw@chromium.org, yfriedman@chromium.org,
jyasskin@chromium.org
Link to the patchset: https://codereview.chromium.org/1601323002/#ps40001
(title: "changed bluetooth test at: requestDevice.html")

[commit-bot: I haz the power, 2016-01-26 01:54:38]

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

[commit-bot: I haz the power, 2016-01-26 02:10:17]

Description was changed from

==========
Use GetLastCommittedOrigin instead of GetLastCommittedURL
in bluetooth chooser

This patch modifies bluetooth chooser code to use
GetLastCommittedOrigin instead of GetLastCommittedURL.

BUG=577451
==========

to

==========
Use GetLastCommittedOrigin instead of GetLastCommittedURL
in bluetooth chooser

This patch modifies bluetooth chooser code to use
GetLastCommittedOrigin instead of GetLastCommittedURL.

BUG=577451
==========

[commit-bot: I haz the power, 2016-01-26 02:10:18]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-01-26 02:11:32]

Description was changed from

==========
Use GetLastCommittedOrigin instead of GetLastCommittedURL
in bluetooth chooser

This patch modifies bluetooth chooser code to use
GetLastCommittedOrigin instead of GetLastCommittedURL.

BUG=577451
==========

to

==========
Use GetLastCommittedOrigin instead of GetLastCommittedURL
in bluetooth chooser

This patch modifies bluetooth chooser code to use
GetLastCommittedOrigin instead of GetLastCommittedURL.

BUG=577451

Committed: https://crrev.com/598777dc91b52b4b50df7dddde64245c05733008
Cr-Commit-Position: refs/heads/master@{#371423}
==========

[commit-bot: I haz the power, 2016-01-26 02:11:33]

Patchset 3 (id:??) landed as
https://crrev.com/598777dc91b52b4b50df7dddde64245c05733008
Cr-Commit-Position: refs/heads/master@{#371423}