1269531 - Make the Geolocation test suite secure-context aware

Status: RESOLVED FIXED

(Core :: DOM: Geolocation, defect)

Description

[Michelangelo De Simone [Offline] [:mds]]

Geolocation requests (watch/get) on non-secure origins should be protected by a pref, at least for now.

Comment 1

[Michelangelo De Simone [Offline] [:mds]]

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.

Comment 2

[Josh Matthews [:jdm]]

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.

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

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

[Josh Matthews [:jdm]]

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).

Comment 5

[Michelangelo De Simone [Offline] [:mds]]

(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.

Comment 6

[Joel Maher ( :jmaher ) (UTC -8)]

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

Comment 7

[Michelangelo De Simone [Offline] [:mds]]

(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?

Comment 8

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 9

[Michelangelo De Simone [Offline] [:mds]]

(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...:)

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

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

Comment 11

[Michelangelo De Simone [Offline] [:mds]]

Attachment: 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/

Comment 12

[Michelangelo De Simone [Offline] [:mds]]

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.:)

Comment 13

[Doug Turner (:dougt)]

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?

Comment 14

[Joel Maher ( :jmaher ) (UTC -8)]

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.

Comment 15

[Michelangelo De Simone [Offline] [:mds]]

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).

Comment 16

[Michelangelo De Simone [Offline] [:mds]]

Attachment: Bug 1269531 - Make the Geolocation test suite secure-context aware.

Review commit: https://reviewboard.mozilla.org/r/107622/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/107622/

Comment 17

[Michelangelo De Simone [Offline] [:mds]]

Comment on attachment 8830961 [details]
Bug 1269531 - Make the Geolocation test suite secure-context aware.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107622/diff/1-2/

Comment 18

[Josh Matthews [:jdm]]

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.

Comment 19

[Michelangelo De Simone [Offline] [:mds]]

(In reply to Josh Matthews [:jdm] from comment #18)

> Nice and straightfoward.

Thanks, I appreciate it!:)

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b747db3970bd
Make the Geolocation test suite secure-context aware. r=jdm

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b747db3970bd