Closed Bug 1072859 Opened 10 years ago Closed 7 years ago

Disable Geolocation on non-secure origins

Categories

(Core :: DOM: Geolocation, defect)

Product:
Core
Component:
DOM: Geolocation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
relnote-firefox --- 55+
firefox51 --- wontfix
firefox52 --- wontfix
firefox53 --- wontfix
firefox54 --- wontfix
firefox55 + fixed

People

(Reporter: annevk, Assigned: mds)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

(Keywords: dev-doc-complete, site-compat, Whiteboard: parity-chrome,parity-webkit)

Attachments

(1 file)

Bug 1072859 - Disable Geolocation on non-secure origins.
59 bytes, text/x-review-board-request
jdm
: review+
Details 
When geolocation is used on a site that does not use an authenticated origin, we should issue a warning.

I would like the warning to state that we plan to disable this functionality when TLS is not involved on 1 Jan 2016. That way sites have ample time to upgrade.
At the very least, it would make sense to make it so that
 1) You can only make a persistent permission grant on an authenticated origin.
and
 2) The code for unauthenticated origins doesn't check for persistent permissions.
(In reply to Anne (:annevk) from comment #0)
> I would like the warning to state that we plan to disable this functionality
> when TLS is not involved on 1 Jan 2016. That way sites have ample time to
> upgrade.

Do we have such a plan? Major sites would be affected, including Bing Maps and MapQuest.

This breaking change would require more outreach than a console warning. If there is a coordinated plan to restrict more APIs that need user permission to authenticated origins, that program should include outreach for all related APIs at the same time, so individual teams do not need to contact the same sites for each API.

We should collect Firefox telemetry for unauthenticated domains using geolocation.
OS: Mac OS X → All
Hardware: x86 → All
(In reply to Chris Peterson (:cpeterson) from comment #2)
> (In reply to Anne (:annevk) from comment #0)
> > I would like the warning to state that we plan to disable this functionality
> > when TLS is not involved on 1 Jan 2016. That way sites have ample time to
> > upgrade.
> 
> Do we have such a plan? Major sites would be affected, including Bing Maps
> and MapQuest.

We do not have such a plan.  Discussion is ongoing on dev.platform.
Google is removing geolocation support on insecure origins in Chrome 50:

https://codereview.chromium.org/1530403002/
Whiteboard: parity-chrome
Depends on: 1266494
Summary: Deprecate non-TLS usage of geolocation → Disable Geolocation on non-secure origins
Depends on: 1269531
Depends on: 1177957
Assignee: nobody → michelangelo
Depends on: 1278370
WebKit just removed geolocation support on insecure origins:

https://trac.webkit.org/changeset/200686
Whiteboard: parity-chrome → parity-chrome,parity-webkit
We have some telemetry now: HTTP origins account for 57% of getCurrentPosition() calls and 2.48% of watchPosition() calls [1][2].  However, if you compare with the number of pageloads [3], it looks like geolocation requests happen on at most 0.22% of pageloads.  That makes the overall breakage rate would be around 0.13%, which seems within the range of things that are tolerable.

Given that, along with the Chrome and Webkit developments above, I'm going propose that we go ahead and make this change.

Michelangelo, do you want to take a hack at it?  It should just require adding the [SecureContext] attribute to geolocation in the WebIDL, then fixing any tests that breaks.
Flags: needinfo?(mds)
Sorry, forgot telemetry links:

[1] https://mzl.la/2eoipG6
[2] https://mzl.la/2eofJs8
[3] https://mzl.la/2eoiIAw -> 6B page loads, vs. ~10M samples

Also, my math was slightly off:

[3] => ~3B page loads
[2] + [1] => ~10M geolocation calls
=> geolocation call rate < ~0.33%
=> geolocation HTTP breakage rate < ~0.188%

Still seems within tolerances.
Breaking 57% of geolocation requests sounds bad. I don't think the percentage of total page loads is really relevant. Do we know what these HTTP sites are?
(In reply to Chris Peterson [:cpeterson] from comment #10)
> Breaking 57% of geolocation requests sounds bad. I don't think the
> percentage of total page loads is really relevant. 

On the contrary, I think the percentage of total loads is critically important, since it tells us how often this change will actually impact users.

For comparison, when we turned off RC4 and SSLv3, they were both at similar levels of actual usage.

> Do we know what these HTTP sites are?

I don't.  But we do know that these sites won't work in Chrome or Safari right now.
> That makes the overall breakage rate would be around 0.13%, which seems within
> the range of things that are tolerable.

While we have not yet decided on a policy for these sorts of things, note that Chrome uses .02% as its "ok to remove" threshold last I checked.  They made an explicit exception for this particular case (geolocation on insecure origins) but even then it was measuring at .03% of pageloads for them, per <https://groups.google.com/a/chromium.org/forum/#!searchin/blink-dev/insecure$20origin$20geolocation/blink-dev/ylz0Zoph76A/jaMAcld6BQAJ>, which is a good bit lower than what we're apparently seeing.  :(

Also, Chrome was seeing a _much_ higher proportion of uses on https than we are.  Quoting their intent mail from about a year ago:

  Insecure origin use of geolocation is hovering right above the removal threshold, at
  0.033% of page loads. Additionally, this has been trending downward for quite some time,
  and it is dominated by secure use of geolocation 0.4698% of page loads. Thus, only about
  6% of geolocation uses are on insecure origins, which is close to the rate at which we
  removed getUserMedia() from insecure origins (about 4% of total getUserMedia() use).

their overall rate for geolocation use is thus about 0.472% of pageloads, which is at least in the same ballpark as what you're seeing (0.33%).  But their rate for insecure uses was _much_ lower, both as a percentage of geolocation uses and as a percentage of overall pageloads...

Anyway, this discussion should be happening in an intent-to-remove thread on .platform, not in the bug, like any other web-facing removal.
(In reply to Richard Barnes [:rbarnes] from comment #8)

> Michelangelo, do you want to take a hack at it?  It should just require
> adding the [SecureContext] attribute to geolocation in the WebIDL, then
> fixing any tests that breaks.

I'm rather booked but I'd be very happy to schedule this, as there seems to be no target milestone just yet.

As this overall change is just a matter of *when*, do you think it'd make sense to start landing something like bug #1269531 in the meantime?
Flags: needinfo?(mds) → needinfo?(rlb)
(In reply to Michelangelo De Simone [:mds] from comment #13)
> As this overall change is just a matter of *when*, do you think it'd make
> sense to start landing something like bug #1269531 in the meantime?

Honestly, not really.  I have pretty good confidence that we'll be able to just make the switch, and it'll be much simpler to do that than to add the pref.
Flags: needinfo?(rlb)
(In reply to Richard Barnes [:rbarnes] from comment #14)
> (In reply to Michelangelo De Simone [:mds] from comment #13)
> > As this overall change is just a matter of *when*, do you think it'd make
> > sense to start landing something like bug #1269531 in the meantime?
> 
> Honestly, not really.  I have pretty good confidence that we'll be able to
> just make the switch, and it'll be much simpler to do that than to add the
> pref.

Adding the pref though gives us an easy way to recover from potential bad breakages that this may cause by the time it ends up in the release channel.  We have been burnt before with that...
The messages from Mounir in the thread linked to from comment 12 seem to suggest a plausible theory around Chrome seeing much different numbers around this than us, due to the Chrome specific usage of this feature on Google properties.
Depends on: 1313242
Blocks: mgga
See Also: → 1335740
All the groundwork for this has landed as part of bug 1269531; as of now there is NO CHANGE to the behavior of Geo.

Considered the feature has been "disallow insecure requests by default" on Safari and Chrome for quite some time now, the idea is to go on parity starting with the next milestone (Firefox 55) and let it ride the train; this'll give a generous heads-up, even though - IMHO - it's not really stricly needed anymore.

Firefox 55 Relase is due in august.

Before taking any further action (by announcing the intent to deprecate) and considered the current status of the most important consumers of this API, I'd like to know whether there's any *compelling* argument from PM and Security to not go ahead with this.
Flags: needinfo?(rlb)
Flags: needinfo?(cpeterson)
I'm a little mixed on this.  On the one hand, there's still almost half of getCurrentPosition requests being made from non-secure contexts.  WatchPosition [2] is better, but still measurable usage from non-secure.

On the other hand, HTTP is deprecated, and my guess is that most of these uses don't break too badly without automated geo.

So net of those concerns, flipping the switch in 55 sounds fine to me.

[1] https://mzl.la/2kt5Xrd
[2] https://mzl.la/2kt5NQI
Flags: needinfo?(rlb)
SGTM. I don't like breaking websites, but Chrome and WebKit began disallowing insecure geolocation requests last year. I don't see any compelling reason not to secure geolocation in 55 as you suggest.
status-firefox51: --- → wontfix
status-firefox52: --- → wontfix
status-firefox53: --- → wontfix
status-firefox54: --- → wontfix
Flags: needinfo?(cpeterson)
Do we know if Chrome and WebKit are using the full spec definition of a SecureContext or just looking for secure origins? Using a Secure Context check currently can have issues related to an insecure creator browsing context. For example, any page in a *tab* opened via target="_blank" or window.open from an HTTP page will not be considered a secure context even if it's using a secure origin. This means that a site isn't in control of whether they have a secure context since they can't control how the tab gets opened (until CSP adds something like disown-opener[1]).

See https://github.com/w3c/webappsec-secure-contexts/issues/42 and section 3.1.5.2 in the spec https://w3c.github.io/webappsec-secure-contexts/#settings-object

Btw. bug 1329940 implemented a chrome-only window.isSecureContextIfOpenerIgnored if needed here to match other engines.

[1] https://w3c.github.io/webappsec-csp/#directive-disown-opener
(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #20)

> Do we know if Chrome and WebKit are using the full spec definition of a
> SecureContext or just looking for secure origins?

From a quick check WebKit doesn't seem to be using the actual secure context algorithm:

http://trac.webkit.org/browser/trunk/Source/WebCore/Modules/geolocation/Geolocation.cpp#L349
http://trac.webkit.org/browser/trunk/Source/WebCore/page/SecurityOrigin.cpp#L176

OTOH it looks like Blink does:

https://chromium.googlesource.com/chromium/src.git/+/master/third_party/WebKit/Source/modules/geolocation/Geolocation.cpp#199
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/ExecutionContext.h?l=172
We met with :mds and spoke about considering using isSecureContextIfOpenerIgnored() instead of isSecureContext() method to detect based on similarity to Chrome breakage here.
(In reply to Jonathan Kingston [:jkt] from comment #22)

> We met with :mds and spoke about considering using
> isSecureContextIfOpenerIgnored() instead of isSecureContext() method to
> detect based on similarity to Chrome breakage here.

+1
The relevant one-liner will be part of the patch for this bug.
I forgot to ask: do we wanna go ahead and mark it [SecureContext] in the webidl too?

Since bug 1286312 landed some time ago, along with bug 1269531, I don't think it'd be of any use now, but perhaps it may put people's mind at ease...
Flags: needinfo?(MattN+bmo)
(In reply to Michelangelo De Simone [:mds] from comment #24)
> I forgot to ask: do we wanna go ahead and mark it [SecureContext] in the
> webidl too?

Well that would mean we would need to implement [SecureContextIfOpenerIgnored] and then we wouldn't have a pref to easily revert this if needed AFAIK (though I'm no expert on our WebIDL codegen). I think we can use the approach in Init() for now unless we want to start using [SecureContextIfOpenerIgnored] in many places.
Flags: needinfo?(MattN+bmo)
(In reply to Michelangelo De Simone [:mds] from comment #24)
> I forgot to ask: do we wanna go ahead and mark it [SecureContext] in the
> webidl too?

Would that result in the same behavior as in Chrome and Safari in insecure contexts? (We should deny geolocation access in observably the same way as the browsers that have already blocked it.)
(In reply to Henri Sivonen (:hsivonen) from comment #26)
> (In reply to Michelangelo De Simone [:mds] from comment #24)
> > I forgot to ask: do we wanna go ahead and mark it [SecureContext] in the
> > webidl too?
> 
> Would that result in the same behavior as in Chrome and Safari in insecure
> contexts? 

No, not currently (see comment 25) since Chrome and Safari don't implement the spec as currently written since they ignore window.opener (not sure if there are other differences) which is what isSecureContextIfOpenerIgnored does (but isn't yet a WebIDL attribute).

> (We should deny geolocation access in observably the same way as
> the browsers that have already blocked it.)

Right, I agree, that's why I asked the question in comment 20 since what is implemented in bug 1269531 does consider window.opener. :mds is going to change that in his patch for this bug according to comment 23.
Comment on attachment 8844205 [details]
Bug 1072859 - Disable Geolocation on non-secure origins.

https://reviewboard.mozilla.org/r/117728/#review119500

Looks like a bunch of tests don't like this change.
Attachment #8844205 - Flags: review?(josh) → review+
(In reply to Josh Matthews [:jdm] from comment #29)

> Looks like a bunch of tests don't like this change.

Yep, some tests outside of tests/geolocation were still using http.

I have a question though: I had to rename some of the wpt to use https (something.html -> something.https.html); I've included them in the revised patch but I'm not sure about the upstreaming process; specifically: do we land the updated wpt tests to m-c AND upstream them on github.com/w3c/web-platform-tests, OR we upstream them on github.com/w3c/web-platform-tests before and wait for them to be pulled back to m-c before landing this patch?
Flags: needinfo?(josh)
Flags: needinfo?(james)
(In reply to Michelangelo De Simone [:mds] from comment #31)
> (In reply to Josh Matthews [:jdm] from comment #29)
> 
> > Looks like a bunch of tests don't like this change.
> 
> Yep, some tests outside of tests/geolocation were still using http.
> 
> I have a question though: I had to rename some of the wpt to use https
> (something.html -> something.https.html); I've included them in the revised
> patch but I'm not sure about the upstreaming process; specifically: do we
> land the updated wpt tests to m-c AND upstream them on
> github.com/w3c/web-platform-tests, OR we upstream them on
> github.com/w3c/web-platform-tests before and wait for them to be pulled back
> to m-c before landing this patch?

Land the changes in m-c and they will be automatically upstreamed at some point in the future (there is a script I run every couple of weeks).
Flags: needinfo?(james)
(In reply to James Graham [:jgraham] from comment #34)

> Land the changes in m-c and they will be automatically upstreamed at some
> point in the future (there is a script I run every couple of weeks).

Thank you James; de-ni'ing :jdm.
Flags: needinfo?(josh)
Pushed by mdesimone@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8b1c50cdb50c
Disable Geolocation on non-secure origins. r=jdm
Deep inside the devtools' responsive design mode test, there is use of the geolocation api:

https://hg.mozilla.org/integration/autoland/rev/6263649f857f55b11a6060d4a8735bdc8dbb0971

Backed out for failing devtools/client/responsive.html/test/browser/browser_permission_doorhanger.js

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=8b1c50cdb50c7eb65d6ace114b9e1a2b2f86c2f5&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=82541604&repo=autoland

[task 2017-03-08T19:54:21.006773Z] 19:54:21     INFO - TEST-START | devtools/client/responsive.html/test/browser/browser_permission_doorhanger.js
[task 2017-03-08T19:54:21.024081Z] 19:54:21     INFO - Entering test bound 
[task 2017-03-08T19:54:21.024641Z] 19:54:21     INFO - Adding a new tab with URL: http://example.com/
[task 2017-03-08T19:54:21.459934Z] 19:54:21     INFO - Tab added and finished loading
[task 2017-03-08T19:54:21.657367Z] 19:54:21     INFO - Console message: [JavaScript Error: "A Geolocation request can only be fulfilled in a secure context." {file: "http://example.com/browser/devtools/client/responsive.html/test/browser/geolocation.html" line: 10}]
[task 2017-03-08T19:55:06.043736Z] 19:55:06     INFO - Longer timeout required, waiting longer...  Remaining timeouts: 1
[task 2017-03-08T19:55:51.047467Z] 19:55:51     INFO - TEST-INFO | started process screentopng
[task 2017-03-08T19:55:51.731235Z] 19:55:51     INFO - TEST-INFO | screentopng: exit 0
[task 2017-03-08T19:55:51.732107Z] 19:55:51     INFO - TEST-UNEXPECTED-FAIL | devtools/client/responsive.html/test/browser/browser_permission_doorhanger.js | Test timed out -
Flags: needinfo?(mdesimone)
(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #37)

> Backed out for failing
> devtools/client/responsive.html/test/browser/browser_permission_doorhanger.js

Updated; re-landing...
Flags: needinfo?(mdesimone)
Pushed by mdesimone@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/07ead25d0bda
Disable Geolocation on non-secure origins. r=jdm
Backout by kwierso@gmail.com:
https://hg.mozilla.org/mozilla-central/rev/193989276571
Backed out changeset 8b1c50cdb50c for failing devtools/client/responsive.html/test/browser/browser_permission_doorhanger.js. r=backout a=merge
https://hg.mozilla.org/mozilla-central/rev/07ead25d0bda
https://hg.mozilla.org/mozilla-central/rev/3c0908cccac2
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
I'm not sure what happened but there seems to be no trace of the changes on m-c despite comment 43.
:KWierso: can you please shed some light?
Flags: needinfo?(wkocher)
Hrm, looks like the merge automatically undid the changes from the landing somehow. I'll try to reland this over the weekend.
Pushed by kwierso@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2d61eb2ffeac
Disable Geolocation on non-secure origins. r=jdm
Relanded. The followup wasn't needed anymore, apparently.
Flags: needinfo?(wkocher)
Release Note Request (optional, but appreciated)
[Why is this notable]: Some (non-HTTPS) websites will no longer be able to use geolocation, though these websites have been broken in Chrome and Safari since last year.
[Affects Firefox for Android]: Yes
[Suggested wording]: Disallow non-secure sites from using Geolocation APIs.
[Links (documentation, blog post, etc)]:
tracking-firefox55: --- → ?
relnote-firefox: --- → ?
Tracking 55+ for this geolocation issue, which will affect some websites.
tracking-firefox55: ? → +
mds, might I ask how you updated the web platform manifest (just in terms of the mechanics)?  It looks like you only updated parts of it, not everything, leading to merge conflicts for everyone else who touches wpt after that for a while...  If we have a tool that makes broken edits like that, we should fix that tool.
Flags: needinfo?(mdesimone)
(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #52)

> mds, might I ask how you updated the web platform manifest (just in terms of
> the mechanics)?  It looks like you only updated parts of it, not everything,
> leading to merge conflicts for everyone else who touches wpt after that for
> a while... 

As I was unaware there's any tooling involved for that manifest, I've only manually changed the relevant names of the geo test cases; I'm rather surprised this led to merge conflicts. Mind giving me some background info to reproduce it?

ni'ing James for context too.
Flags: needinfo?(mdesimone)
Flags: needinfo?(james)
Flags: needinfo?(bzbarsky)
So the context is that a design goal of wpt was to avoid having people edit fragile manifests by hand. So the wpt manifest is intended to be autogenerated based on the file contents. However it turns out that with no way to prevent people hand editing it, and especially with some recent changes to optimise regenereating it that added hashes to the file, it's now super-fragile and easy to create merge conflicts.

The long term solution is probably to move it into the build system somehow. In the meantime you should either use |mach wpt-manifest-update| to update it or |mach wpt --manifest-update|.
Flags: needinfo?(james)
Right, the merge conflicts came from parts of the manifest that the hand-edit did _not_ update but "mach wpt-manifest-update" and "mach wpt-create" _do_ update.  Every single changeset after the one in this bug that involved running either of those commands would perform the missing updates, and they would all conflict with each other until one of them landed and the others got merged on top...

In particular, if one of them landed on inbound and one on autoland (which is what actually happened), then the sheriffs would be stuck dealing with the conflicts.

Like James says, please use "mach wpt-manifest-update" to update the manifest.
Flags: needinfo?(bzbarsky)
I posted to dev.platform to remind people about the tools we have for this.
Apologies for the snafu, then.
I happens; don't worry about it.  I just wanted to make sure there wasn't something other than lack of knowledge about the mach commands for updating this going on.
(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #59)
> I've documented this update; first of all I've included Geolocation here:
> 
> https://developer.mozilla.org/en-US/docs/Web/Security/Secure_Contexts/
> features_restricted_to_secure_contexts
> 
> I've also put a note in the Fx55 release notes:
> 
> https://developer.mozilla.org/en-US/Firefox/Releases/55#DOM_HTML_DOM

Apologies; I'm setting this back to dev-doc-needed for now. I've filed a PR against the MDN Kuma platform for some macros to display inline and block/banner notices indicating that a feature is secure-context only. Once the PR is accepted, I will implement them on the Geolocation pages, and also update the Geolocation compat tables to mention it.
Keywords: dev-doc-complete → dev-doc-needed
OK, Finally completing the docs on this one. In addition to the above updates, I have added Secure context banners and inline indicators as appropriate to the following pages:

https://developer.mozilla.org/en-US/docs/Web/API/Geolocation (and child pages)
https://developer.mozilla.org/en-US/docs/Web/API/Coordinates (and child pages)
https://developer.mozilla.org/en-US/docs/Web/API/Navigator/geolocation (and child pages)
https://developer.mozilla.org/en-US/docs/Web/API/Position (and child pages)
https://developer.mozilla.org/en-US/docs/Web/API/PositionError (and child pages)
https://developer.mozilla.org/en-US/docs/Web/API/PositionOptions (and child pages)
Keywords: dev-doc-needed → dev-doc-complete
added to the 55beta release notes
relnote-firefox: ? → 55+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: