Ensure that Firefox doesn't hit remote addresses by default when Marionette tests are run

ASSIGNED
Assigned to

Status

Testing
Marionette
P3
normal
ASSIGNED
9 months ago
4 months ago

People

(Reporter: whimboo, Assigned: whimboo)

Tracking

(Blocks: 1 bug)

55 Branch
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

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

Attachments

(2 attachments)

(Assignee)

Description

9 months ago
In Marionette and Geckodriver we modify some preferences to stop Firefox reaching some production servers like for the add-ons discovery pane:

"extensions.webservice.discoverURL": "http://%(server)s/dummy/discoveryURL"

The problem with this is that Firefox still wants to reach this remote address and is causing network traffic. In case of a proxy in the middle it will be flooded.

To prevent this we should check how to get this replaced with the address from the locally hosted wptserve instance.

Here the known prefs:

> "datareporting.healthreport.documentServerURI": "http://%(server)s/dummy/healthreport/",
> "datareporting.healthreport.about.reportUrl": "http://%(server)s/dummy/abouthealthreport/",
> "extensions.webservice.discoverURL": "http://%(server)s/dummy/discoveryURL",
> "network.sntp.pools": "%(server)s",
> "services.settings.server": "http://%(server)s/dummy/blocklist/",
> "toolkit.telemetry.server": "https://%(server)s/dummy/telemetry/",

For Marionette unit tests we could do a replacement most likely in geckoinstance.py where those prefs are getting set.

But not sure what to do with the recommended prefs we set in Marionette server. When we run through Geckodriver there might not be a local server running. Just using localhost without a port doesn't seem to be a wise idea.

Andreas, any idea?
Flags: needinfo?
(Assignee)

Updated

9 months ago
Flags: needinfo?(ato)
(In reply to Henrik Skupin (:whimboo) from comment #0)

> In Marionette and Geckodriver we modify some preferences to stop
> Firefox reaching some production servers like for the add-ons
> discovery pane:
> 
> "extensions.webservice.discoverURL": "http://%(server)s/dummy/discoveryURL"
> 
> The problem with this is that Firefox still wants to reach this remote
> address and is causing network traffic. In case of a proxy in the
> middle it will be flooded.
> 
> To prevent this we should check how to get this replaced with the
> address from the locally hosted wptserve instance.

Why do we want it to hit the wptserve instance?  Could they not just be
cleared?

> Here the known prefs:
> 
> > "datareporting.healthreport.documentServerURI": "http://%(server)s/dummy/healthreport/",
> > "datareporting.healthreport.about.reportUrl": "http://%(server)s/dummy/abouthealthreport/",

As far as I know, it is datareporting.policy.dataSubmissionEnabled
that controls whether we will submit any data to Telemetry.  We
set this to false, and it would be surprising if this caused
us to hit datareporting.healthreport.documentServerURI or
datareporting.healthreport.about.reportUrl.
Flags: needinfo?(ato)
(Assignee)

Comment 2

8 months ago
(In reply to Andreas Tolfsen ‹:ato› from comment #1)
> Why do we want it to hit the wptserve instance?  Could they not just be
> cleared?

That's how Mochitest is doing it. But maybe they register valid handlers for all the cases immediately. Given that Marionette is more flexible, we might indeed not wanna do it, and indeed could simply set `localhost` with the default port.

> > > "datareporting.healthreport.documentServerURI": "http://%(server)s/dummy/healthreport/",
> > > "datareporting.healthreport.about.reportUrl": "http://%(server)s/dummy/abouthealthreport/",
> 
> As far as I know, it is datareporting.policy.dataSubmissionEnabled
> that controls whether we will submit any data to Telemetry.  We
> set this to false, and it would be surprising if this caused
> us to hit datareporting.healthreport.documentServerURI or
> datareporting.healthreport.about.reportUrl.

This is just an example which might not cause problems, right. But others could, like for the add-ons discovery pane.
Flags: needinfo?
(In reply to Henrik Skupin (:whimboo) from comment #2)
> (In reply to Andreas Tolfsen ‹:ato› from comment #1)
> > Why do we want it to hit the wptserve instance?  Could they not just be
> > cleared?
> 
> That's how Mochitest is doing it. But maybe they register valid handlers for
> all the cases immediately. Given that Marionette is more flexible, we might
> indeed not wanna do it, and indeed could simply set `localhost` with the
> default port.

I would actually suggest clearing the URLs completely so they don’t hit any
accidental endpoint or service on localhost at all.
(Assignee)

Updated

7 months ago
Summary: Replace %server% placeholders in modified preferences with the localhost and port → Replace %(server)s placeholders in modified preferences with the localhost and port
(Assignee)

Updated

7 months ago
Assignee: nobody → hskupin
Status: NEW → ASSIGNED
Summary: Replace %(server)s placeholders in modified preferences with the localhost and port → Replace %(server)s placeholders in modified preferences with an empty string
(Assignee)

Comment 4

7 months ago
Francois, even with the following preferences disabled I can still see that Firefox tries to connect to the remote address as specified in "browser.safebrowsing.provider.mozilla.updateURL":

        "browser.safebrowsing.blockedURIs.enabled": False,
        "browser.safebrowsing.downloads.enabled": False,
        "browser.safebrowsing.malware.enabled": False,
        "browser.safebrowsing.phishing.enabled": False,

What are we missing here? Is there something else which needs explicitly to be disabled?
Flags: needinfo?(francois)
(Assignee)

Comment 5

7 months ago
Lets make this bug broader and finally fix all the necessary preferences.
Blocks: 1272255
Summary: Replace %(server)s placeholders in modified preferences with an empty string → Ensure that Firefox doesn't hit remote addresses by default when Marionette tests are run
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(In reply to Henrik Skupin (:whimboo) from comment #4)
> Francois, even with the following preferences disabled I can still see that
> Firefox tries to connect to the remote address as specified in
> "browser.safebrowsing.provider.mozilla.updateURL":
> 
>         "browser.safebrowsing.blockedURIs.enabled": False,
>         "browser.safebrowsing.downloads.enabled": False,
>         "browser.safebrowsing.malware.enabled": False,
>         "browser.safebrowsing.phishing.enabled": False,
> 
> What are we missing here? Is there something else which needs explicitly to
> be disabled?

For the mozilla provider, you need to disable all of the tracking protection and plugin blocking features:

privacy.trackingprotection.enabled = false
privacy.trackingprotection.pbmode.enabled = false
privacy.trackingprotection.annotate_channels = false
plugins.flashBlock.enabled = false
Flags: needinfo?(francois)
(Assignee)

Comment 9

7 months ago
Hm, so what would actually be better in terms of disabling the service? Switching all flags to false, or better replacing the URLs of each with an empty string so that we do not hit the network? At least for other test harnesses I can see that they are doing the latter.

Would that also make it easier for the safebrowsing firefox ui tests to enable the service without a restart? Or wouldn't it make a difference?
Flags: needinfo?(francois)
(In reply to Henrik Skupin (:whimboo) from comment #9)
> Hm, so what would actually be better in terms of disabling the service?
> Switching all flags to false, or better replacing the URLs of each with an
> empty string so that we do not hit the network? At least for other test
> harnesses I can see that they are doing the latter.
>
> Would that also make it easier for the safebrowsing firefox ui tests to
> enable the service without a restart? Or wouldn't it make a difference?

It would be easier to re-enable the service in the Safe Browsing UI tests if we only disable the feature flags and leave the URLs as they are.

Otherwise we'll have to remember to update the UI tests everytime we change the URL prefs and so it's likely they will get out of sync.
Flags: needinfo?(francois)
(Assignee)

Comment 11

7 months ago
(In reply to François Marier [:francois] from comment #10)
> Otherwise we'll have to remember to update the UI tests everytime we change
> the URL prefs and so it's likely they will get out of sync.

That shouldn't be the case given that we would only have to remove the user defined value (aka reset the preference). Once done the original value (URL) should be immediately available. Would modifying the URLs instead safe us a restart to get safebrowsing enabled?
Flags: needinfo?(francois)
(In reply to Henrik Skupin (:whimboo) from comment #11)
> That shouldn't be the case given that we would only have to remove the user
> defined value (aka reset the preference). Once done the original value (URL)
> should be immediately available. Would modifying the URLs instead safe us a
> restart to get safebrowsing enabled?

I don't think that would change anything because we'd still end up having to re-initialize the list manager with the new URLs.
Flags: needinfo?(francois)
Priority: -- → P3
(Assignee)

Comment 13

5 months ago
In bug 1399628 we got the preference `the extensions.shield-recipe-client.api_url` added, which should disable shield by not allowing it to download studies. As informed by Mike this might not be a good idea, and we shouldn't set the URL to an empty string. Instead the component/extension should have been entirely disabled.

Maybe we have to rethink the strategy here in regards that we cannot use an empty string at all. Also not for the other preferences. Not sure if every component/extension has a knob to easily turn it off.

I'm happy to hear options in what to do with the URL given that we cannot always replace `%(server)s` due to a missing locally running webserver.
(Assignee)

Comment 14

4 months ago
FYI, for shield we should change the preference to use:

> extensions.shield-recipe-client.enabled = false

I will do it with the next update.
You need to log in before you can comment on or make changes to this bug.