Removal of geolocation APIs on insecure origins

Removal of geolocation APIs on insecure origins

This disallows the geolocation APIs getCurrentPosition() and
watchPosition() from being used on insecure origins. Adds a console
warning message that the API call has failed because of this.

Note that this is a re-land of
https://codereview.chromium.org/1485973002/. See that CL for full
discussion.

BUG=520765, 561641
TBR=thestig@chromium.org,sgurun@chromium.org,philipj@opera.com,mlamouri@chromium.org

Committed: https://crrev.com/9d4ca2d9838b5f33bdb3f8fcfb8ef381d449b2a1
Cr-Commit-Position: refs/heads/master@{#370185}

[jww, 2015-12-17 02:34:44]

Description was changed from

==========
Removal of geolocation APIs on insecure origins

This disallows the geolocation APIs getCurrentPosition() and
watchPosition() from being used on insecure origins. Adds a console
warning message that the API call has failed because of this.

BUG=520765, 561641
==========

to

==========
Removal of geolocation APIs on insecure origins

This disallows the geolocation APIs getCurrentPosition() and
watchPosition() from being used on insecure origins. Adds a console
warning message that the API call has failed because of this.

Note that this is a re-land of
https://codereview.chromium.org/1485973002/. See that CL for full
discussion.

BUG=520765, 561641
==========

[jww, 2015-12-17 02:37:49]

jww@chromium.org changed reviewers:
+ torne@chromium.org

[jww, 2015-12-17 02:37:50]

torne@, I believe this implements the API we need for this to work with WebKit.
Can you take a look, especially at the android_webview/ and content/ files that
implement the preference? Thanks!

[jww, 2015-12-17 03:11:01]

On 2015/12/17 02:37:50, jww wrote:
> torne@, I believe this implements the API we need for this to work with
WebKit.
> Can you take a look, especially at the android_webview/ and content/ files
that
> implement the preference? Thanks!

Scratch that. I was mistaken. Didn't realize that
GeolocationProvider::GetInstance() had to be called from the UI thread (and then
I tested wrong so I missed it). Will rethink tomorrow.

[jww, 2015-12-17 19:09:02]

torne@, this is ready to review now. Sorry about the false start yesterday. Let
me know what you think.

[jww, 2015-12-17 21:28:58]

The CQ bit was checked by jww@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-17 21:29:41]

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

[commit-bot: I haz the power, 2015-12-17 22:12:34]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-17 22:12:35]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[jww, 2015-12-17 23:33:44]

The CQ bit was checked by jww@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-17 23:37:52]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1530403002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1530403002/60001

[commit-bot: I haz the power, 2015-12-18 00:11:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-18 00:11:39]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[jww, 2015-12-18 01:28:21]

The CQ bit was checked by jww@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-18 01:29:06]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1530403002/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1530403002/60001

[commit-bot: I haz the power, 2015-12-18 01:57:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-18 01:57:41]

Dry run: Try jobs failed on following builders:
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)

[Torne, 2016-01-05 11:21:23]

Sorry, didn't get to this before the holidays. Approach looks okay, but one
question:

https://codereview.chromium.org/1530403002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/UseCounter.cpp (right):

https://codereview.chromium.org/1530403002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/UseCounter.cpp:880: return
"getCurrentPosition() and watchPosition() no longer work on insecure origins. To
use this feature, you should consider switching your application to a secure
origin, such as HTTPS. See https://goo.gl/rStTGz for more details.";
I'm not clear on how the warnings work here, so one question: after this change
lands, will there still be a warning printed to the console in WebView apps
which use geolocation on insecure origins? If so, does this mean the wording of
the warning has changed to say that it doesn't work even though it still does?

[jww, 2016-01-06 02:40:59]

https://codereview.chromium.org/1530403002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/frame/UseCounter.cpp (right):

https://codereview.chromium.org/1530403002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/frame/UseCounter.cpp:880: return
"getCurrentPosition() and watchPosition() no longer work on insecure origins. To
use this feature, you should consider switching your application to a secure
origin, such as HTTPS. See https://goo.gl/rStTGz for more details.";
On 2016/01/05 11:21:23, Torne wrote:
> I'm not clear on how the warnings work here, so one question: after this
change
> lands, will there still be a warning printed to the console in WebView apps
> which use geolocation on insecure origins? If so, does this mean the wording
of
> the warning has changed to say that it doesn't work even though it still does?

Indeed, you are correct. How about I change the wording to be a bit more
ambiguous for now (see the new CL), and we can change it to be more explicitly
"no longer work" after WebView is fixed permanently?

[Torne, 2016-01-06 11:59:11]

LGTM.

Once this lands we can talk about when we can try unsetting the flag and and
testing the waters.

[jww, 2016-01-06 18:57:28]

jww@chromium.org changed reviewers:
+ creis@chromium.org, dcheng@chromium.org, tsepez@chromium.org

[jww, 2016-01-06 18:57:29]

Hi everyone. I need a few OWNER reviews here.

tsepez@chromium.org, would you mind checking out the IPC change in:
content/public/common/common_param_traits_macros.h

creis@chromium.org, can you take a look at the following files:
content/public/common/content_switechs.cc
content/public/common/web_preferences.[h,cc]
content/renderer/render_view_impl.cc

dcheng@chromium.org, can you take a look at the following files:
third_party/WebKit/Source/web/*
third_party/WebKit/Source/core/frame/Settings.in
third_party/WebKit/Source/core/frame/UseCounter.cpp

Thanks!

[Tom Sepez, 2016-01-06 19:59:51]

Param traits LGTM.

[Charlie Reis (slow), 2016-01-06 21:39:41]

content/ LGTM.

https://codereview.chromium.org/1530403002/diff/100001/content/public/common/...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/1530403002/diff/100001/content/public/common/...
content/public/common/content_switches.cc:459: // Blocks insecure usage of
number of powerful features (device orientation, for
nit: of a number

[jww, 2016-01-06 23:18:56]

https://codereview.chromium.org/1530403002/diff/100001/content/public/common/...
File content/public/common/content_switches.cc (right):

https://codereview.chromium.org/1530403002/diff/100001/content/public/common/...
content/public/common/content_switches.cc:459: // Blocks insecure usage of
number of powerful features (device orientation, for
On 2016/01/06 21:39:40, Charlie Reis wrote:
> nit: of a number

Done.

[dcheng, 2016-01-06 23:42:37]

https://codereview.chromium.org/1530403002/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebSettings.h (right):

https://codereview.chromium.org/1530403002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebSettings.h:113: virtual void
setAllowGeolocationOnInsecureOrigins(bool) = 0;
Are these mainly intended for Android's WebView? Would it make sense to group
various security downgrade settings for Android WebView together somewhere?

https://codereview.chromium.org/1530403002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/security/powerfulFeatureRestrictions/old-powerful-features-on-insecure-origin.html
(right):

https://codereview.chromium.org/1530403002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/security/powerfulFeatureRestrictions/old-powerful-features-on-insecure-origin.html:56:
// Note that the deprecation message for watchPosition() will be supressed
Nit: suppressed

[jww, 2016-01-07 00:34:53]

On 2016/01/06 23:42:37, dcheng wrote:
>
https://codereview.chromium.org/1530403002/diff/100001/third_party/WebKit/pub...
> File third_party/WebKit/public/web/WebSettings.h (right):
> 
>
https://codereview.chromium.org/1530403002/diff/100001/third_party/WebKit/pub...
> third_party/WebKit/public/web/WebSettings.h:113: virtual void
> setAllowGeolocationOnInsecureOrigins(bool) = 0;
> Are these mainly intended for Android's WebView? Would it make sense to group
> various security downgrade settings for Android WebView together somewhere?
Yes, it's a temporary bypass for WebView. What other security downgrade settings
are there for WebView? And where did you have in mind? This seemed like the
simplest solution I could come up with.
> 
>
https://codereview.chromium.org/1530403002/diff/120001/third_party/WebKit/Lay...
> File
>
third_party/WebKit/LayoutTests/http/tests/security/powerfulFeatureRestrictions/old-powerful-features-on-insecure-origin.html
> (right):
> 
>
https://codereview.chromium.org/1530403002/diff/120001/third_party/WebKit/Lay...
>
third_party/WebKit/LayoutTests/http/tests/security/powerfulFeatureRestrictions/old-powerful-features-on-insecure-origin.html:56:
> // Note that the deprecation message for watchPosition() will be supressed
> Nit: suppressed

[dcheng, 2016-01-07 00:39:08]

On 2016/01/07 at 00:34:53, jww wrote:
> On 2016/01/06 23:42:37, dcheng wrote:
> >
https://codereview.chromium.org/1530403002/diff/100001/third_party/WebKit/pub...
> > File third_party/WebKit/public/web/WebSettings.h (right):
> > 
> >
https://codereview.chromium.org/1530403002/diff/100001/third_party/WebKit/pub...
> > third_party/WebKit/public/web/WebSettings.h:113: virtual void
> > setAllowGeolocationOnInsecureOrigins(bool) = 0;
> > Are these mainly intended for Android's WebView? Would it make sense to
group
> > various security downgrade settings for Android WebView together somewhere?
> Yes, it's a temporary bypass for WebView. What other security downgrade
settings are there for WebView? And where did you have in mind? This seemed like
the simplest solution I could come up with.

setAllowFileAccessFromFileURLs maybe? I don't recall the exact context for this
setting anymore...

(We don't need to create a new object, I was just wondering if it'd make sense
to group the stuff together in Settings.in so we could track the stuff we've had
to add specifically for Android WebView compat) 

> > 
> >
https://codereview.chromium.org/1530403002/diff/120001/third_party/WebKit/Lay...
> > File
> >
third_party/WebKit/LayoutTests/http/tests/security/powerfulFeatureRestrictions/old-powerful-features-on-insecure-origin.html
> > (right):
> > 
> >
https://codereview.chromium.org/1530403002/diff/120001/third_party/WebKit/Lay...
> >
third_party/WebKit/LayoutTests/http/tests/security/powerfulFeatureRestrictions/old-powerful-features-on-insecure-origin.html:56:
> > // Note that the deprecation message for watchPosition() will be supressed
> > Nit: suppressed

[jww, 2016-01-07 00:45:19]

On 2016/01/07 00:39:08, dcheng wrote:
> On 2016/01/07 at 00:34:53, jww wrote:
> > On 2016/01/06 23:42:37, dcheng wrote:
> > >
>
https://codereview.chromium.org/1530403002/diff/100001/third_party/WebKit/pub...
> > > File third_party/WebKit/public/web/WebSettings.h (right):
> > > 
> > >
>
https://codereview.chromium.org/1530403002/diff/100001/third_party/WebKit/pub...
> > > third_party/WebKit/public/web/WebSettings.h:113: virtual void
> > > setAllowGeolocationOnInsecureOrigins(bool) = 0;
> > > Are these mainly intended for Android's WebView? Would it make sense to
> group
> > > various security downgrade settings for Android WebView together
somewhere?
> > Yes, it's a temporary bypass for WebView. What other security downgrade
> settings are there for WebView? And where did you have in mind? This seemed
like
> the simplest solution I could come up with.
> 
> setAllowFileAccessFromFileURLs maybe? I don't recall the exact context for
this
> setting anymore...
Based on the comment in
https://code.google.com/p/chromium/codesearch#chromium/src/content/public/com...,
I think that's just a setting for developers who need the old-style behavior
(that is, it's not WebView specific).

Given that this is a temporary hack until we get WebView sorted out, I don't
think it makes sense to group them all together, especially since I don't know
what are specific to WebView vs used by (but not exclusively for) WebView.
> 
> (We don't need to create a new object, I was just wondering if it'd make sense
> to group the stuff together in Settings.in so we could track the stuff we've
had
> to add specifically for Android WebView compat) 
> 
> > > 
> > >
>
https://codereview.chromium.org/1530403002/diff/120001/third_party/WebKit/Lay...
> > > File
> > >
>
third_party/WebKit/LayoutTests/http/tests/security/powerfulFeatureRestrictions/old-powerful-features-on-insecure-origin.html
> > > (right):
> > > 
> > >
>
https://codereview.chromium.org/1530403002/diff/120001/third_party/WebKit/Lay...
> > >
>
third_party/WebKit/LayoutTests/http/tests/security/powerfulFeatureRestrictions/old-powerful-features-on-insecure-origin.html:56:
> > > // Note that the deprecation message for watchPosition() will be supressed
> > > Nit: suppressed

[dcheng, 2016-01-07 00:46:50]

LGTM with previous nit addressed.

[jww, 2016-01-07 02:25:40]

This should address all the comments. However, I'm going to wait to commit this
until we've moved up one release since we're already past M49 feature freeze,
and this feels like a change that should live on Canary for a while first.

https://codereview.chromium.org/1530403002/diff/100001/third_party/WebKit/pub...
File third_party/WebKit/public/web/WebSettings.h (right):

https://codereview.chromium.org/1530403002/diff/100001/third_party/WebKit/pub...
third_party/WebKit/public/web/WebSettings.h:113: virtual void
setAllowGeolocationOnInsecureOrigins(bool) = 0;
On 2016/01/06 23:42:37, dcheng wrote:
> Are these mainly intended for Android's WebView? Would it make sense to group
> various security downgrade settings for Android WebView together somewhere?

Acknowledged.

https://codereview.chromium.org/1530403002/diff/120001/third_party/WebKit/Lay...
File
third_party/WebKit/LayoutTests/http/tests/security/powerfulFeatureRestrictions/old-powerful-features-on-insecure-origin.html
(right):

https://codereview.chromium.org/1530403002/diff/120001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/http/tests/security/powerfulFeatureRestrictions/old-powerful-features-on-insecure-origin.html:56:
// Note that the deprecation message for watchPosition() will be supressed
On 2016/01/06 23:42:37, dcheng wrote:
> Nit: suppressed

Done.

[jww, 2016-01-07 02:26:32]

Description was changed from

==========
Removal of geolocation APIs on insecure origins

This disallows the geolocation APIs getCurrentPosition() and
watchPosition() from being used on insecure origins. Adds a console
warning message that the API call has failed because of this.

Note that this is a re-land of
https://codereview.chromium.org/1485973002/. See that CL for full
discussion.

BUG=520765, 561641
==========

to

==========
Removal of geolocation APIs on insecure origins

This disallows the geolocation APIs getCurrentPosition() and
watchPosition() from being used on insecure origins. Adds a console
warning message that the API call has failed because of this.

Note that this is a re-land of
https://codereview.chromium.org/1485973002/. See that CL for full
discussion.

BUG=520765, 561641
TBR=thestig@chromium.org,sgurun@chromium.org,philipj@opera.com,mlamouri@chrom...
==========

[jww, 2016-01-19 19:47:08]

The CQ bit was checked by jww@chromium.org

[jww, 2016-01-19 19:47:08]

The patchset sent to the CQ was uploaded after l-g-t-m from tsepez@chromium.org,
creis@chromium.org, torne@chromium.org, dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/1530403002/#ps160001
(title: "Rebase on ToT")

[commit-bot: I haz the power, 2016-01-19 19:48:06]

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

[jww, 2016-01-19 19:48:19]

On 2016/01/07 02:25:40, jww wrote:
> This should address all the comments. However, I'm going to wait to commit
this
> until we've moved up one release since we're already past M49 feature freeze,
> and this feels like a change that should live on Canary for a while first.
> 
>
https://codereview.chromium.org/1530403002/diff/100001/third_party/WebKit/pub...
> File third_party/WebKit/public/web/WebSettings.h (right):
> 
>
https://codereview.chromium.org/1530403002/diff/100001/third_party/WebKit/pub...
> third_party/WebKit/public/web/WebSettings.h:113: virtual void
> setAllowGeolocationOnInsecureOrigins(bool) = 0;
> On 2016/01/06 23:42:37, dcheng wrote:
> > Are these mainly intended for Android's WebView? Would it make sense to
group
> > various security downgrade settings for Android WebView together somewhere?
> 
> Acknowledged.
> 
>
https://codereview.chromium.org/1530403002/diff/120001/third_party/WebKit/Lay...
> File
>
third_party/WebKit/LayoutTests/http/tests/security/powerfulFeatureRestrictions/old-powerful-features-on-insecure-origin.html
> (right):
> 
>
https://codereview.chromium.org/1530403002/diff/120001/third_party/WebKit/Lay...
>
third_party/WebKit/LayoutTests/http/tests/security/powerfulFeatureRestrictions/old-powerful-features-on-insecure-origin.html:56:
> // Note that the deprecation message for watchPosition() will be supressed
> On 2016/01/06 23:42:37, dcheng wrote:
> > Nit: suppressed
> 
> Done.

Now that M49 has fully branched, I'm committing this.

[commit-bot: I haz the power, 2016-01-19 20:59:06]

Description was changed from

==========
Removal of geolocation APIs on insecure origins

This disallows the geolocation APIs getCurrentPosition() and
watchPosition() from being used on insecure origins. Adds a console
warning message that the API call has failed because of this.

Note that this is a re-land of
https://codereview.chromium.org/1485973002/. See that CL for full
discussion.

BUG=520765, 561641
TBR=thestig@chromium.org,sgurun@chromium.org,philipj@opera.com,mlamouri@chrom...
==========

to

==========
Removal of geolocation APIs on insecure origins

This disallows the geolocation APIs getCurrentPosition() and
watchPosition() from being used on insecure origins. Adds a console
warning message that the API call has failed because of this.

Note that this is a re-land of
https://codereview.chromium.org/1485973002/. See that CL for full
discussion.

BUG=520765, 561641
TBR=thestig@chromium.org,sgurun@chromium.org,philipj@opera.com,mlamouri@chrom...
==========

[commit-bot: I haz the power, 2016-01-19 20:59:08]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-01-19 21:00:13]

Description was changed from

==========
Removal of geolocation APIs on insecure origins

This disallows the geolocation APIs getCurrentPosition() and
watchPosition() from being used on insecure origins. Adds a console
warning message that the API call has failed because of this.

Note that this is a re-land of
https://codereview.chromium.org/1485973002/. See that CL for full
discussion.

BUG=520765, 561641
TBR=thestig@chromium.org,sgurun@chromium.org,philipj@opera.com,mlamouri@chrom...
==========

to

==========
Removal of geolocation APIs on insecure origins

This disallows the geolocation APIs getCurrentPosition() and
watchPosition() from being used on insecure origins. Adds a console
warning message that the API call has failed because of this.

Note that this is a re-land of
https://codereview.chromium.org/1485973002/. See that CL for full
discussion.

BUG=520765, 561641
TBR=thestig@chromium.org,sgurun@chromium.org,philipj@opera.com,mlamouri@chrom...

Committed: https://crrev.com/9d4ca2d9838b5f33bdb3f8fcfb8ef381d449b2a1
Cr-Commit-Position: refs/heads/master@{#370185}
==========

[commit-bot: I haz the power, 2016-01-19 21:00:14]

Patchset 9 (id:??) landed as
https://crrev.com/9d4ca2d9838b5f33bdb3f8fcfb8ef381d449b2a1
Cr-Commit-Position: refs/heads/master@{#370185}