David Burns :automatedtester Reporter
	Description • 9 years ago

We should give sites the ability to figure out if a bot is visiting them if they felt the need to block certain features.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 2 • 7 years ago

The WebDriver standard defines a NavigatorAutomationInformation
WebIDL interface [1] that is meant to be exposed when WebDriver
is active in the user agent.  This property defines a standard way
for a co-operating user agent to inform a web document that it is
controlled by WebDriver.

Gecko’s WebDriver implementation is backed by Marionette, and we would
want to expose the navigator.webdriver getter when it is activated.
Marionette is active only when a -marionette flag is passed to Firefox,
e.g. it cannot be enabled or disabled at runtime through a preference [2].

Marionette [3] is implemented as a chrome/system JS module and
shipped with Firefox.  It gets activated in an XPCOM component
after the system notification session-windows-restored fires
[4].  I’m not sure if this is relevant, but it implements
the nsIMarionette IDL interface with a "running" attribute [5]
to indicate that it is enabled.  This is used in Firefox frontend
code to trigger a UX code path that turns the address bar orange
to warn the user that the browser is under remote control.

peterv: I tried reading the WebIDL documentation [6] but I’m
unsure exactly what would be the right way to implement this.
Since navigator.webdriver is meant to just return a static boolean
true when invoked, the implementation part to this would seem straight
forward, but it would be great to have some input from you how we
should go about this and what you think is the appropriate way to
implement this.

Thanks in advance!

  [1] https://w3c.github.io/webdriver/webdriver-spec.html#interface
  [2] In fact, it is considered a security risk because addons can modify preferences.
  [3] https://searchfox.org/mozilla-central/source/testing/marionette
  [4] https://searchfox.org/mozilla-central/source/testing/marionette/components/marionette.js
  [5] https://searchfox.org/mozilla-central/source/testing/marionette/components/nsIMarionette.idl
  [6] https://developer.mozilla.org/en-US/docs/Mozilla/WebIDL_bindings

	Peter Van der Beken [:peterv]
	Comment 3 • 6 years ago

I think in the current state of our WebIDL parser you want a partial Navigator interface with the webdriver attribute. Put a Func on the webdriver attribute that calls a C++ function that just returns the value of nsIMarionette's running attribute. Probably easiest to put the C++ function somewhere in Navigator.cpp.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 4 • 6 years ago

FWIW there is some contention in [1] towards whether the
navigator.webdriver attribute should always be exposed.  The argument
is that selectively exposing it is not comparable to other web
features that are either enabled/disabled.

I don’t think the issue necessarily blocks us from implementing
or shipping this, as it would be backwards compatible to always expose it.

  [1] https://github.com/w3c/webdriver/issues/1214

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 15 • 6 years ago

Manual try run because Otto Länd won’t cooperate: https://treeherder.mozilla.org/#/jobs?repo=try&revision=539a89bbc44cfc60c81316d5e72cceddc0069839

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 16 • 6 years ago

Let’s wait for https://github.com/w3c/webdriver/issues/1214 to
get resolved before exposing the WebIDL attribute.

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 17 • 6 years ago
mozreview-review

Comment on attachment 8946277 [details]
Bug 1169290 - Improve getPrefVal readability.

https://reviewboard.mozilla.org/r/216232/#review223136

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 18 • 6 years ago
mozreview-review

Comment on attachment 8946278 [details]
Bug 1169290 - Define log level conversion table in function scope.

https://reviewboard.mozilla.org/r/216234/#review223138

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 19 • 6 years ago
mozreview-review

Comment on attachment 8946279 [details]
Bug 1169290 - Convert MarionetteComponent to a class.

https://reviewboard.mozilla.org/r/216236/#review223156

::: testing/marionette/components/marionette.js:307
(Diff revision 1)
> +    if (outer) {
> +      throw Cr.NS_ERROR_NO_AGGREGATION;
> +    }
> +
> +    let marionette = new MarionetteComponent();
> +    return marionette.QueryInterface(iid);

This is passing `iid` to a getter, so I don't see how it's used. Typo?

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 20 • 6 years ago
mozreview-review

Comment on attachment 8946280 [details]
Bug 1169290 - Tell running state from whether server is alive.

https://reviewboard.mozilla.org/r/216238/#review223158

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 21 • 6 years ago
mozreview-review

Comment on attachment 8946281 [details]
Bug 1169290 - Fire remote-active observer notification in component.

https://reviewboard.mozilla.org/r/216240/#review223162

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 22 • 6 years ago
mozreview-review

Comment on attachment 8946282 [details]
Bug 1169290 - Handle -marionette flag in observe function.

https://reviewboard.mozilla.org/r/216242/#review223164

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 23 • 6 years ago
mozreview-review

Comment on attachment 8946283 [details]
Bug 1169290 - Reintroduce marionette.enabled pref.

https://reviewboard.mozilla.org/r/216244/#review223168

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 24 • 6 years ago
mozreview-review

Comment on attachment 8946284 [details]
Bug 1169290 - Make Marionette component safe to load in child process.

https://reviewboard.mozilla.org/r/216246/#review223170

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 25 • 6 years ago
mozreview-review

Comment on attachment 8946285 [details]
Bug 1169290 - Allow nsIMarionette to be initialised from C++.

https://reviewboard.mozilla.org/r/216248/#review223172

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 26 • 6 years ago

I have submitted https://github.com/w3c/webdriver/pull/1219 to
change the spec after I found out both Safari and Chrome expose it
unconditionally.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 37 • 6 years ago
mozreview-review-reply

Comment on attachment 8946279 [details]
Bug 1169290 - Convert MarionetteComponent to a class.

https://reviewboard.mozilla.org/r/216236/#review223156

> This is passing `iid` to a getter, so I don't see how it's used. Typo?

The getter returns the XPCOMUtils.generateQI factory, so the argument
ends up being passed to the generated function, not the getter.

	Boris Zbarsky [:bzbarsky]
	Comment 38 • 6 years ago
mozreview-review

Comment on attachment 8946286 [details]
Bug 1169290 - Add navigator.webdriver attribute.

https://reviewboard.mozilla.org/r/216250/#review223636

::: testing/web-platform/tests/webdriver/tests/interface.html:27
(Diff revision 2)
> -if ("webdriver" in navigator) {
> -  test(() => assert_true(navigator.webdriver), "navigator.webdriver is always true");
> +if (navigator.webdriver) {
> +  test(() => assert_true(navigator.webdriver), "navigator.webdriver is true when webdriver-active is set");

This test doesn't make sense to me.  Isn't it tautologically going to pass?

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 39 • 6 years ago
mozreview-review-reply

Comment on attachment 8946286 [details]
Bug 1169290 - Add navigator.webdriver attribute.

https://reviewboard.mozilla.org/r/216250/#review223636

> This test doesn't make sense to me.  Isn't it tautologically going to pass?

If the attribute isn’t exposed, navigator.webdriver will be
undefined ant fail the false assertion on :30.

Unfortunately this test is run using Marionette in WPT, so
navigator.webdriver will always be true in automation.  I’m not
sure there is a good workaround for that?

If the test is run manually, without automation, it is still expected
to pass when navigator.webdriver is false.  All we’re trying to
check here is that (1) the attribute exists, (2) its value is a
boolean, and (3) that it matches the WebIDL definition.

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 50 • 6 years ago

I have corrected the last patch to highlight that this is a
_configurable_ (i.e. not [Unforgeable]) attribute addition,
addressing :bz’s comment from [1].

  [1] https://groups.google.com/d/msg/mozilla.dev.platform/GRNpjC4MuH8/L-pWP6AtAgAJ

	Boris Zbarsky [:bzbarsky]
	Comment 51 • 6 years ago
mozreview-review

Comment on attachment 8946286 [details]
Bug 1169290 - Add navigator.webdriver attribute.

https://reviewboard.mozilla.org/r/216250/#review223638

::: dom/webidl/Navigator.webidl:312
(Diff revision 2)
>  partial interface Navigator {
>    [Pref="security.webauth.webauthn", SecureContext, SameObject]
>    readonly attribute CredentialsContainer credentials;
>  };
> +
> +partial interface Navigator {

This needs to have spec links here and in the header, like every other partial interface in this file.

	Boris Zbarsky [:bzbarsky]
	Comment 52 • 6 years ago
mozreview-review-reply

Comment on attachment 8946286 [details]
Bug 1169290 - Add navigator.webdriver attribute.

https://reviewboard.mozilla.org/r/216250/#review223636

> If the attribute isn’t exposed, navigator.webdriver will be
> undefined ant fail the false assertion on :30.
> 
> Unfortunately this test is run using Marionette in WPT, so
> navigator.webdriver will always be true in automation.  I’m not
> sure there is a good workaround for that?
> 
> If the test is run manually, without automation, it is still expected
> to pass when navigator.webdriver is false.  All we’re trying to
> check here is that (1) the attribute exists, (2) its value is a
> boolean, and (3) that it matches the WebIDL definition.

Ah, assert_false asserts === false.  Alright, that's not a complete tautology, then.  ;)

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 53 • 6 years ago
mozreview-review-reply

Comment on attachment 8946286 [details]
Bug 1169290 - Add navigator.webdriver attribute.

https://reviewboard.mozilla.org/r/216250/#review223636

> Ah, assert_false asserts === false.  Alright, that's not a complete tautology, then.  ;)

I’ve added another assertion before this which explicitly checks
that the attribute is present.  Because this is a bit of a special
test I’ve also added a comment explaining the true/false outcome.

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 64 • 6 years ago
mozreview-review

Comment on attachment 8946286 [details]
Bug 1169290 - Add navigator.webdriver attribute.

https://reviewboard.mozilla.org/r/216250/#review224318

::: testing/web-platform/tests/webdriver/tests/interface.html:34
(Diff revision 4)
> -  var idlArray = new IdlArray();
> -  [].forEach.call(document.querySelectorAll("script[type=text\\/plain]"), function(node) {
> -    if (node.className == "untested") {
> -      idlArray.add_untested_idls(node.textContent);
> -    } else {
> -      idlArray.add_idls(node.textContent);
> +// When test is run in automation navigator.webdriver is likely to
> +// be true because WebDriver controls the browser instance.  To that
> +// extent, this test is a bit special.  It should also be possible to
> +// run the test manually, when WebDriver is not active, and so either
> +// true/false outcome is OK.
> +if (navigator.webdriver) {

Is there any other condition that would indicate automation or lack thereof to avoid the tautology in the test? Maybe an environment variable?

	Boris Zbarsky [:bzbarsky]
	Comment 65 • 6 years ago
mozreview-review

Comment on attachment 8946286 [details]
Bug 1169290 - Add navigator.webdriver attribute.

https://reviewboard.mozilla.org/r/216250/#review224714

::: dom/webidl/Navigator.webidl:312
(Diff revision 4)
>  partial interface Navigator {
>    [Pref="security.webauth.webauthn", SecureContext, SameObject]
>    readonly attribute CredentialsContainer credentials;
>  };
> +
> +// https://w3c.github.io/webdriver/webdriver-spec.html#interface

Please list this at the top of the file too, with the other spec links.

::: dom/webidl/Navigator.webidl:313
(Diff revision 4)
>    [Pref="security.webauth.webauthn", SecureContext, SameObject]
>    readonly attribute CredentialsContainer credentials;
>  };
> +
> +// https://w3c.github.io/webdriver/webdriver-spec.html#interface
> +partial interface Navigator {

Probably better to match the spec more closely, assuming the spec has a reason it's not using a mixin:

    Navigator implements NavigatorAutomationInformation;
    
    [NoInterfaceObject]
    interface NavigatorAutomationInformation {
      ...
    }

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 66 • 6 years ago
mozreview-review-reply

Comment on attachment 8946286 [details]
Bug 1169290 - Add navigator.webdriver attribute.

https://reviewboard.mozilla.org/r/216250/#review224714

> Probably better to match the spec more closely, assuming the spec has a reason it's not using a mixin:
> 
>     Navigator implements NavigatorAutomationInformation;
>     
>     [NoInterfaceObject]
>     interface NavigatorAutomationInformation {
>       ...
>     }

Thanks for the great feedback!

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 68 • 6 years ago
mozreview-review-reply

Comment on attachment 8946286 [details]
Bug 1169290 - Add navigator.webdriver attribute.

https://reviewboard.mozilla.org/r/216250/#review224318

> Is there any other condition that would indicate automation or lack thereof to avoid the tautology in the test? Maybe an environment variable?

I checked with jgraham and we don’t have any way to know if the
automation in use is WebDriver-based or not.  I’m afraid this is
the best we can do.

	Boris Zbarsky [:bzbarsky]
	Comment 81 • 6 years ago
mozreview-review

Comment on attachment 8949765 [details]
Bug 1169290 - Guard navigator.webdriver behind dom.webdriver.enabled pref.

https://reviewboard.mozilla.org/r/219068/#review224820

::: commit-message-b2000:3
(Diff revision 1)
> +Bug 1169290 - Guard navigator.webdriver behind dom.webdriver.enabled pref. r?bz,maja_zf
> +
> +In the off chance exposing navigator.webdriver turns out

s/In/On/

	Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)
	Comment 82 • 6 years ago
mozreview-review

Comment on attachment 8949765 [details]
Bug 1169290 - Guard navigator.webdriver behind dom.webdriver.enabled pref.

https://reviewboard.mozilla.org/r/219068/#review224838

	Pulsebot
	Comment 94 • 6 years ago

Pushed by atolfsen@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e92ad5128f3c
Improve getPrefVal readability. r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/348c46e5edd1
Define log level conversion table in function scope. r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/103ae77be7b8
Convert MarionetteComponent to a class. r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/0f9e1fe6e905
Tell running state from whether server is alive. r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/2365c2d010cb
Fire remote-active observer notification in component. r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/fd299ab7f1c7
Handle -marionette flag in observe function. r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/5f35bfdac21d
Reintroduce marionette.enabled pref. r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/fb6cc1b9e730
Make Marionette component safe to load in child process. r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/05abef0a4f0a
Allow nsIMarionette to be initialised from C++. r=maja_zf
https://hg.mozilla.org/integration/autoland/rev/9ddfd2155429
Add navigator.webdriver attribute. r=bz,maja_zf
https://hg.mozilla.org/integration/autoland/rev/47a5027e0b51
Guard navigator.webdriver behind dom.webdriver.enabled pref. r=bz,maja_zf

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 95 • 6 years ago

Created web-platform-tests PR https://github.com/w3c/web-platform-tests/pull/9482 for changes under testing/web-platform/tests

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 96 • 6 years ago

Upstream web-platform-tests status checked passed, PR will merge once commit reaches central.

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 97 • 6 years ago

Upstream web-platform-tests status checked passed, PR will merge once commit reaches central.

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 98 • 6 years ago

Upstream web-platform-tests status checked passed, PR will merge once commit reaches central.

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 99 • 6 years ago

Upstream web-platform-tests status checked passed, PR will merge once commit reaches central.

	Andreea Pavel [:apavel]
	Comment 100 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/e92ad5128f3c
https://hg.mozilla.org/mozilla-central/rev/348c46e5edd1
https://hg.mozilla.org/mozilla-central/rev/103ae77be7b8
https://hg.mozilla.org/mozilla-central/rev/0f9e1fe6e905
https://hg.mozilla.org/mozilla-central/rev/2365c2d010cb
https://hg.mozilla.org/mozilla-central/rev/fd299ab7f1c7
https://hg.mozilla.org/mozilla-central/rev/5f35bfdac21d
https://hg.mozilla.org/mozilla-central/rev/fb6cc1b9e730
https://hg.mozilla.org/mozilla-central/rev/05abef0a4f0a
https://hg.mozilla.org/mozilla-central/rev/9ddfd2155429
https://hg.mozilla.org/mozilla-central/rev/47a5027e0b51

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 101 • 6 years ago

Can't merge web-platform-tests PR due to failing upstream tests

	Andreas Tolfsen ❲:ato❳ Assignee
	Comment 102 • 6 years ago

(In reply to Web Platform Test Sync Bot from comment #101)

> Can't merge web-platform-tests PR due to failing upstream tests

What action do I need to take here?  Why did it fail to upstream the
tests when it earlier reported that upstream status checks passed?

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 103 • 6 years ago

Merging PR failed: 405 {u'documentation_url': u'https://developer.github.com/v3/pulls/#merge-a-pull-request-merge-button', u'message': u'Base branch was modified. Review and try the merge again.'}

	Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)
	Comment 104 • 6 years ago

Upstream PR merged

	James Graham [:jgraham]
	Comment 105 • 6 years ago

The sync bot had a bug in this case. Various issues are known, and we are going to improve the messages when there are actual upstream test failures.

	Florian Scholz (Open Web Docs)
	Comment 107 • 6 years ago

We should update https://developer.mozilla.org/en-US/docs/Web/API/Navigator/webdriver and/or the compat table on that page about our support of this.