Make the Geolocation test suite secure-context aware

RESOLVED FIXED in Firefox 54

Status

()

Core
Geolocation
RESOLVED FIXED
2 years ago
10 months ago

People

(Reporter: mds, Assigned: mds)

Tracking

(Blocks: 1 bug)

unspecified
mozilla54
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: btpp-active)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

Geolocation requests (watch/get) on non-secure origins should be protected by a pref, at least for now.
Blocks: 1072859
Assignee: nobody → michelangelo
Whiteboard: btpp-active
Hey Josh,

I have a working patch locally (pretty trivial, though); the real issue is with the mochitests.
As almost all the dom/tests/mochi/geolocation tests fail when requiring https, I asked on #Developers how to deal with https-only mochitests.

froydnj was suggesting that a good approach may be to have every test "cloned" in order to run the secure (and real) test in a secure context, from a non-secure regular test.

For example: 'a.html' can be cloned into 'a-secure.html'. 'a.html' can then call 'a-secure.html' via an HTTPS URL.

This - I think - should work but I wonder whether there's a less intricated way to have HTTPS-only test suite.
Flags: needinfo?(josh)

Comment 2

2 years ago
I do not know of a less invasive way of doing that, unfortunately. The web-platform-tests harness supports adding .https to the filename to run the test from a secure origin, but the mochitest harness does not.
Flags: needinfo?(josh)
right now we test geolocation via mochitests using a .sjs file:
https://dxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/geolocation/network_geolocation.sjs

this is referenced via http.  I know we support https calls via mochitests using ssltunnel, I am not sure if we support https + .sjs files, it appears we do:
https://dxr.mozilla.org/mozilla-central/source/browser/base/content/test/general/browser_bug553455.js#178

do we have a need to test with non https?  If so, then I would agree testing in two modes, or better yet, having each test run in http and https mode.  We set the geo server via preferences:
https://dxr.mozilla.org/mozilla-central/source/dom/tests/mochitest/geolocation/geolocation_common.js

this server could be set in either mode.  There is no advantage that I know of to have duplicated files vs a test which does two modes.

let me know if I added more confusion and I can clarify anything that might be confusing.

Comment 4

2 years ago
It's not the sjs that's the problem; it's the HTML test files that are served from an insecure origin (http://mochi.test:888, iirc).
(In reply to Josh Matthews [:jdm] from comment #4)

> It's not the sjs that's the problem; it's the HTML test files that are
> served from an insecure origin (http://mochi.test:888, iirc).

That's exactly correct. It'd be great to have some sort of pref somewhere in the mochi suite to shape the way the network layer behaves.

Thank you all for all these amazing info, though.
so change the call from http://mochi.test -> https://mochi.test, am I missing something?
(In reply to Joel Maher (:jmaher) from comment #6)

> so change the call from http://mochi.test -> https://mochi.test, am I
> missing something?

That is exactly the point.:)

Take the tests in dom/tests/mochitest/geolocation; test_allowWatch.html for example. Within that test, we reference navigator.geolocation.watchPosition(...) which is the method that we _MUST_ call on HTTPS for the test to succeed.

Whoever calls test_allowWatch.html should do that on HTTPS. This is the blocked I've found myself in.

In short, the question is: can we run the dom/tests/mochitest/geolocation suite on HTTPS by default?
good question and now I see the problem.  right now we build a list of tests up and the mochitest harness runs them all via http://, that doesn't help.

An easy work around here is to make a wrapper and load each test in an iframe- that iframe could load the test (i.e. test_allowWatch.html) via https://.  Would that solve the needs here?

In that case you could make test_secure_allowWatch.html that just is load the test via https:// in the iframe- of course you would need to set the geolocation prefs to be https as well.
(In reply to Joel Maher (:jmaher) from comment #8)

> In that case you could make test_secure_allowWatch.html that just is load
> the test via https:// in the iframe- of course you would need to set the
> geolocation prefs to be https as well.

I really, REALLY, hoped there was some undocumented flip-switch somewhere.:)
Thank you, though...:)
well, we could run ALL tests via https, that would be the super easy hack- not sure if that is what we want to do...although worth considering
Created attachment 8759441 [details]
Bug 1269531 - Adding pref for https-only geo reqs.

For now the pref has been defaulted to true (no change from current
behavior). It'll be flipped to false (disallow all non-secure geo
requests) as part of the patch for #1072859.

Review commit: https://reviewboard.mozilla.org/r/57424/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57424/
As running all the dom/geolocation mochitests under HTTPS by default doesn't seem a trivial thing to do, I've decided to just commit this patch with the actual code for the preference.

Also, the pref is defaulted to 'true', meaning that it still DOES allow non-secure requests.

At least I don't get blocked trying to deal with the mochitests.:)
We should fix up the test harness so that we can run http and/or https. There will be more apis that are only exposed when loaded over secure protocols, thus we'll want the ability to control the context that the test runs in.

Joel, can you provide feedback as to exactly how you want this implemented?
Flags: needinfo?(jmaher)
it appears that just running everything under https://mochi.test:8888/... is not going to work :(  Given that data point, I would argue that we need to load a specific test in an iframe or new window as https and it can do the magic work.

To do that I assume we would just create a http page that then launches in a new window or iframe the https page which will do the test.
Flags: needinfo?(jmaher)
Depends on: 1278370
Attachment #8759441 - Flags: review-
Comment on attachment 8759441 [details]
Bug 1269531 - Adding pref for https-only geo reqs.

https://reviewboard.mozilla.org/r/57424/#review54892

We need to enable the test harnes to be configurable for secure origins before landing this (see #1278370).
(Assignee)

Updated

10 months ago
Blocks: 1333985
(Assignee)

Updated

10 months ago
Depends on: 1286312
No longer depends on: 1278370
(Assignee)

Updated

10 months ago
Summary: Pref switch for non-secure geo requests → Make the Geolocation test suite secure-context aware
Comment hidden (mozreview-request)
(Assignee)

Updated

10 months ago
Attachment #8759441 - Attachment is obsolete: true
Comment hidden (mozreview-request)

Comment 18

10 months ago
mozreview-review
Comment on attachment 8830961 [details]
Bug 1269531 - Make the Geolocation test suite secure-context aware.

https://reviewboard.mozilla.org/r/107622/#review108774

Nice and straightfoward.
Attachment #8830961 - Flags: review?(josh) → review+
(In reply to Josh Matthews [:jdm] from comment #18)

> Nice and straightfoward.

Thanks, I appreciate it!:)

Comment 20

10 months ago
Pushed by mdesimone@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b747db3970bd
Make the Geolocation test suite secure-context aware. r=jdm

Comment 21

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b747db3970bd
Status: NEW → RESOLVED
Last Resolved: 10 months ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.