Description was changed from ========== Removal of geolocation APIs on insecure origins This disallows the ...
5 years, 2 months ago
(2015-12-17 02:34:44 UTC)
#1
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
==========
torne@, I believe this implements the API we need for this to work with WebKit. ...
5 years, 2 months ago
(2015-12-17 02:37:50 UTC)
#3
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
On 2015/12/17 02:37:50, jww wrote: > torne@, I believe this implements the API we need ...
5 years, 2 months ago
(2015-12-17 03:11:01 UTC)
#4
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
torne@, this is ready to review now. Sorry about the false start yesterday. Let me ...
5 years, 2 months ago
(2015-12-17 19:09:02 UTC)
#5
torne@, this is ready to review now. Sorry about the false start yesterday. Let
me know what you think.
jww
The CQ bit was checked by jww@chromium.org to run a CQ dry run
5 years, 2 months ago
(2015-12-17 21:28:58 UTC)
#6
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
5 years, 2 months ago
(2015-12-17 21:29:41 UTC)
#7
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_chromeos_ozone_rel_ng/builds/103639)
5 years, 2 months ago
(2015-12-17 22:12:35 UTC)
#9
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
5 years, 2 months ago
(2015-12-17 23:37:52 UTC)
#11
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_compile_dbg_ng/builds/138141)
5 years, 2 months ago
(2015-12-18 00:11:39 UTC)
#13
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
5 years, 2 months ago
(2015-12-18 01:29:06 UTC)
#15
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_compile_dbg_ng/builds/138213)
5 years, 2 months ago
(2015-12-18 01:57:41 UTC)
#17
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/Source/core/frame/UseCounter.cpp ...
5 years, 2 months ago
(2016-01-05 11:21:23 UTC)
#18
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
https://codereview.chromium.org/1530403002/diff/60001/third_party/WebKit/Source/core/frame/UseCounter.cpp File third_party/WebKit/Source/core/frame/UseCounter.cpp (right): https://codereview.chromium.org/1530403002/diff/60001/third_party/WebKit/Source/core/frame/UseCounter.cpp#newcode880 third_party/WebKit/Source/core/frame/UseCounter.cpp:880: return "getCurrentPosition() and watchPosition() no longer work on insecure ...
5 years, 1 month ago
(2016-01-06 02:40:59 UTC)
#19
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
LGTM. Once this lands we can talk about when we can try unsetting the flag ...
5 years, 1 month ago
(2016-01-06 11:59:11 UTC)
#20
LGTM.
Once this lands we can talk about when we can try unsetting the flag and and
testing the waters.
Hi everyone. I need a few OWNER reviews here. tsepez@chromium.org, would you mind checking out ...
5 years, 1 month ago
(2016-01-06 18:57:29 UTC)
#22
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
Param traits LGTM.
5 years, 1 month ago
(2016-01-06 19:59:51 UTC)
#23
Param traits LGTM.
Charlie Reis
content/ LGTM. https://codereview.chromium.org/1530403002/diff/100001/content/public/common/content_switches.cc File content/public/common/content_switches.cc (right): https://codereview.chromium.org/1530403002/diff/100001/content/public/common/content_switches.cc#newcode459 content/public/common/content_switches.cc:459: // Blocks insecure usage of number of ...
5 years, 1 month ago
(2016-01-06 21:39:41 UTC)
#24
https://codereview.chromium.org/1530403002/diff/100001/content/public/common/content_switches.cc File content/public/common/content_switches.cc (right): https://codereview.chromium.org/1530403002/diff/100001/content/public/common/content_switches.cc#newcode459 content/public/common/content_switches.cc:459: // Blocks insecure usage of number of powerful features ...
5 years, 1 month ago
(2016-01-06 23:18:56 UTC)
#25
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/public/web/WebSettings.h ...
5 years, 1 month ago
(2016-01-07 00:39:08 UTC)
#28
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
On 2016/01/07 00:39:08, dcheng wrote: > On 2016/01/07 at 00:34:53, jww wrote: > > On ...
5 years, 1 month ago
(2016-01-07 00:45:19 UTC)
#29
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
LGTM with previous nit addressed.
5 years, 1 month ago
(2016-01-07 00:46:50 UTC)
#30
LGTM with previous nit addressed.
jww
This should address all the comments. However, I'm going to wait to commit this until ...
5 years, 1 month ago
(2016-01-07 02:25:40 UTC)
#31
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
Description was changed from ========== Removal of geolocation APIs on insecure origins This disallows the ...
5 years, 1 month ago
(2016-01-07 02:26:32 UTC)
#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
The CQ bit was checked by jww@chromium.org
5 years, 1 month ago
(2016-01-19 19:47:08 UTC)
#33
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
5 years, 1 month ago
(2016-01-19 19:48:06 UTC)
#35
On 2016/01/07 02:25:40, jww wrote: > This should address all the comments. However, I'm going ...
5 years, 1 month ago
(2016-01-19 19:48:19 UTC)
#36
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
Description was changed from ========== Removal of geolocation APIs on insecure origins This disallows the ...
5 years, 1 month ago
(2016-01-19 20:59:06 UTC)
#37
Issue 1530403002: Removal of geolocation APIs on insecure origins
(Closed)
Created 5 years, 2 months ago by jww
Modified 5 years, 1 month ago
Reviewers: dcheng, Charlie Reis, Torne, Tom Sepez
Base URL: https://chromium.googlesource.com/chromium/src@master
Comments: 8