1632821 - navigator.webdriver should no longer depend on the marionette.enabled preference

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

As noticed on bug 1632556 the navigator.webdriver preference is always false. This is because internally it depends on the marionette.enabled preference:

https://searchfox.org/mozilla-central/rev/41c3ea3ee8eab9ce7b82932257cb80b703cbba67/dom/base/Navigator.cpp#2097-2099

We should make it so that the value is retrieved from the enabled property of the Marionette component. By that we could even get rid of the preference at all.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #0)

https://searchfox.org/mozilla-central/rev/41c3ea3ee8eab9ce7b82932257cb80b703cbba67/dom/base/Navigator.cpp#2097-2099

What we have to do in Navigator.cpp is to replace the retrieval of the preference with using an instance of nsIMarionette and checking its running attribute.

Olli, when I'm doing this change should I best cache the instance of Marionette similar to the addon manager and others? Or is it ok to create the instance again whenever the navigator.webdriver property is used?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

What would be the other use of nsIMarionette?
Or could the method just return nsCOMPtr<nsIMarionette> marionette = /some how get instance/;
return marionette->GetRunning();

nsIMarionette seems to be a service, so not caching it is probably fine.
This shouldn't be performance critical code, since the pref should be disabled by default.

Obviously there needs to be some pref to control the web exposed part
https://searchfox.org/mozilla-central/rev/a6db3bd67367aa9ddd9505690cab09b47e65a762/dom/webidl/Navigator.webidl#319

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Olli Pettay [:smaug] from comment #2)

What would be the other use of nsIMarionette?

There is nothing else. Just checking the .running attribute.

Or could the method just return nsCOMPtr<nsIMarionette> marionette = /some how get instance/;
return marionette->GetRunning();

That's what I already tried locally. Doing that I would certainly have to add a marionette.h header file to expose the the interface for C++ code. That's what we currently don't have. If getting the interface is not that dramatic I'm happy to take that path. Also the navigator.webdriver property will most likely only be used once.

nsIMarionette seems to be a service, so not caching it is probably fine.

No, it's not a service.

Obviously there needs to be some pref to control the web exposed part
https://searchfox.org/mozilla-central/rev/a6db3bd67367aa9ddd9505690cab09b47e65a762/dom/webidl/Navigator.webidl#319

That is interesting and I wasn't aware of it. So far I thought that it should not be possible to turn that feature off. I read through the following pages but have problems to understand what we actually have to implement:

https://github.com/w3c/webdriver/pull/1219/files
https://bugzilla.mozilla.org/show_bug.cgi?id=1169290

James, could you please check that part again? Should dom.webdriver.enabled control if we expose the property on navigator? The following comment from Andreas irritates me a bit:

Making the attribute selectively present, which is unlike any other
web features are implemented, forces web authors to detect automation
in the following way:

var underAutomation = ("webdriver" in navigator) ? navigator.webdriver : false;

Following this patch, web authors can do this:

var underAutomation = navigator.webdriver;

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

nsIMarionette is used as a service, so it is a service :)
https://searchfox.org/mozilla-central/rev/a6db3bd67367aa9ddd9505690cab09b47e65a762/security/manager/ssl/nsCertOverrideService.cpp#726
https://searchfox.org/mozilla-central/rev/a6db3bd67367aa9ddd9505690cab09b47e65a762/browser/base/content/browser.js#259,274

But if the idea is to expose .webdriver property always, then better to ensure accessing its value is fast. But I guess there would be still some pref to tell whether nsIMarionette service is available. And usually no, so the .webdriver would return false.
So something like

if (pref_enabled) {
nsCOMPtr<nsIMarionette> marionette = do_GetService(NS_MARIONETTE_CONTRACTID);
if (marionette) {
bool marionetteRunning = false;
marionette->GetRunning(&marionetteRunning);
return marionetteRunning;
}
}
return false;

Always trying to access the service might be too slow.

If normal builds don't have marionette service, then one could of course also just cache the service in a member variable of Navigator and return false, if it is null.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Oh, I see, the pref is always on https://searchfox.org/mozilla-central/rev/a6db3bd67367aa9ddd9505690cab09b47e65a762/modules/libpref/init/StaticPrefList.yaml#3338,3340
So then have
nsCOMPtr<nsIMarionetter> mMarionette in Navigator

Comment 6

[James Graham [:jgraham]]

The attribute is always present; when there's an active marionette session it should return true otherwise it should return false. I don't think it should depend on any pref.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

As discussed with James last week the dom.webdriver.enabled preference needs to be removed. It has been originally introduced to allow a quick and easy disabling of the web exposed navigator.webdriver property in case of web-compat issues. This was landed in Firefox 60 via bug 1169290.

As such I will go ahead and remove any preferences which would allow users to disable the property, which should not be allowed.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Telemetry still has a reference to marionette.enabled:
https://searchfox.org/mozilla-central/source/toolkit/components/telemetry/app/TelemetryEnvironment.jsm#310

Chris, can this be removed or do we need another measure in having this data? By default we disable telemetry, but for sure users could also enable it again.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Please drop the last comment. Completely removing the marionette.enabled preference should happen on it's own bug.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1632821 - [dom] Remove dom.webdriver.enabled preference.

The preference was meant to exist only temporarily when
the "navigator.webdriver" property was added by bug 1169290
in Firefox 60.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Attachment: Bug 1632821 - [dom] Base navigator.webdriver on nsIMarionette::running instead of marionette.enabled perference.

The "marionette.enabled" preference will be removed because it
should no longer be used to determine if Marionette enabled or not.

As such the enabled / running state can be retrieved via the
nsIMarionette XPCOM service.

Depends on D106778

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I don't think we have to keep this bug confidential anymore. Also the right place is Core : DOM.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a2edc1a0a0b2
[dom] Remove dom.webdriver.enabled preference. r=smaug
https://hg.mozilla.org/integration/autoland/rev/d056cc5c66cf
[dom] Base navigator.webdriver on nsIMarionette::running instead of marionette.enabled perference. r=smaug

Comment 14

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/a2edc1a0a0b2
https://hg.mozilla.org/mozilla-central/rev/d056cc5c66cf

Comment 15

[Michel Le Bihan]

Hello,

I believe that this setting should not have been removed. More technical users can simply patch Firefox to not expose that information, while less technical users will be left downloading some shady patched Firefox versions or patchers. Website operators can also believe that checking navigator.webdriver is an effective way to detect bots and scrapping.

Why was it decided to remove that setting?

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Which setting do you mean? The navigator.webdriver property is still there and reports when WebDriver is active, means when Marionette and/or WebDriver BiDi are running. Just checking for the enabled status - as we did before - was basically wrong because it didn't mean that Marionette is really running. If you think that there is something wrong please file a new bug.