Bug 171997 – navigator.webdriver should return false if the page is not controlled by automation

• Component: DOM
• Status: RESOLVED FIXED

[Sergey Shekyan]

WebDriver specification defines (https://www.w3.org/TR/webdriver/#interface) webdriver IDL attribute for navigator as boolean. If session is controlled by automation, getter in current implementation returns undefined.

Considering that property descriptor is unconditionally defined on navigator prototype [Object.getOwnPropertyDescriptor(navigator.__proto__, "webdriver")], I believe it should return false if session is not controlled by automation.

I'll be happy to submit a patch if change seems reasonable.

[Chris Dumez]

Brian Burg is the WebDriver expert but I agree that our implementation does not seem to match the spec:
1. We return undefined instead of false when not controlled by automation
2. The property is not enumerable but should be

[Blaze Burg]

(In reply to Sergey Shekyan from comment #0)
> WebDriver specification defines (https://www.w3.org/TR/webdriver/#interface)
> webdriver IDL attribute for navigator as boolean. If session is controlled
> by automation, getter in current implementation returns undefined.
> 
> Considering that property descriptor is unconditionally defined on navigator
> prototype [Object.getOwnPropertyDescriptor(navigator.__proto__,
> "webdriver")], I believe it should return false if session is not controlled
> by automation.
> 
> I'll be happy to submit a patch if change seems reasonable.

Patches welcome.

As far as I know, nobody ships a W3C-compatible WebDriver implementation based on WebKit, but this shouldn't stop you from making this particular piece better.

[Sergey Shekyan]

I believe there is no reason to not having [Unforgeable] attribute on the property, and was about to open an issue in the spec repo.

[Sergey Shekyan]

Created attachment 310567 [details]
Patch

[Sergey Shekyan]

Current webdriver editor's draft changed to declare navigator.webdriver as non-configurable (https://w3c.github.io/webdriver/webdriver-spec.html#interface)

[Build Bot]

Attachment 310567 [details] did not pass style-queue:


ERROR: Source/WebCore/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
ERROR: Source/WebCore/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Sergey Shekyan]

Created attachment 310570 [details]
Patch

[Chris Dumez]

Comment on attachment 310570 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=310570&action=review

This needs a layout test to check that:
1. The property is false instead of being undefined now
2. The property is not configurable (Assuming this part of your change is right)
3. The property is enumerable (as I think it should be as per the spec).

> Source/WebCore/ChangeLog:8
> +

Please add a link to the spec in your changelog.

> Source/WebCore/Modules/webdriver/NavigatorWebDriver.idl:27
> +    [Unforgeable, NotEnumerable, Custom] readonly attribute boolean webdriver;

We no longer need custom code at all. [Custom] should be dropped and the implementation should be updated accordingly.

Could you also point to the spec that says it should be Unforgeable? I do know that [NotEnumerable] is not as per spec and should be dropped but last time I checked, there was no [Unforgeable] in the spec.

[Chris Dumez]

Let me know if this is more work than expected and you'd like me to take over. I don't mind fixing this.

[Sergey Shekyan]

Not at all, have the test ready. But apparently removing [Custom] is not as easy as I expected and I hope to get some insights from browsing the code on the weekend.

[Chris Dumez]

Comment on attachment 310570 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=310570&action=review

> Source/WebCore/Modules/webdriver/NavigatorWebDriver.cpp:72
>  JSValue JSNavigator::webdriver(ExecState&) const

If you drop the [Custom], I believe you'll need to drop this method and add a new one:
static bool NavigatorWebDriver::webdriver(Navigator&) const 
{
    return isControlledByAutomation();
}

[Sergey Shekyan]

Created attachment 310918 [details]
Patch

[Sergey Shekyan]

(In reply to Chris Dumez from comment #11)
> Comment on attachment 310570 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=310570&action=review
> 
> > Source/WebCore/Modules/webdriver/NavigatorWebDriver.cpp:72
> >  JSValue JSNavigator::webdriver(ExecState&) const
> 
> If you drop the [Custom], I believe you'll need to drop this method and add
> a new one:
> static bool NavigatorWebDriver::webdriver(Navigator&) const 
> {
>     return isControlledByAutomation();
> }

It was a little more than that, but I think it works now. What should we do about worker navigator?

[Chris Dumez]

Comment on attachment 310918 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=310918&action=review

Patch looks good otherwise.

> Source/WebCore/ChangeLog:5
> +        Per WebDriver Specification at https://www.w3.org/TR/webdriver/#interface

The description should come *after* the reviewed by line.

> Source/WebCore/Modules/webdriver/NavigatorWebDriver.cpp:52
> +    Frame* frame = navigator.frame();

auto*

> Source/WebCore/Modules/webdriver/NavigatorWebDriver.cpp:53
> +    if (frame || !frame->page())

This check seems wrong, should probably !frame.

> LayoutTests/js/dom/navigator-webdriver.html:8
> +description(

Just description("Check that navigator.webdriver has the right attributes.");

> LayoutTests/js/dom/navigator-webdriver.html:12
> +if (window.testRunner) {

We don't use curly brackets for one-liners.

[Chris Dumez]

(In reply to Sergey Shekyan from comment #13)
> (In reply to Chris Dumez from comment #11)
> > Comment on attachment 310570 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=310570&action=review
> > 
> > > Source/WebCore/Modules/webdriver/NavigatorWebDriver.cpp:72
> > >  JSValue JSNavigator::webdriver(ExecState&) const
> > 
> > If you drop the [Custom], I believe you'll need to drop this method and add
> > a new one:
> > static bool NavigatorWebDriver::webdriver(Navigator&) const 
> > {
> >     return isControlledByAutomation();
> > }
> 
> It was a little more than that, but I think it works now. What should we do
> about worker navigator?

I do not understand the question, we do not seem to expose navigator.webdriver to workers. The spec does not seem to say we should expose this to workers either. The specs has a partial interface for Navigator but not WorkerNavigator [1].

[1] https://html.spec.whatwg.org/multipage/workers.html#workernavigator

[Sergey Shekyan]

(In reply to Chris Dumez from comment #15)
> (In reply to Sergey Shekyan from comment #13)
> > (In reply to Chris Dumez from comment #11)
> > > Comment on attachment 310570 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=310570&action=review
> > > 
> > > > Source/WebCore/Modules/webdriver/NavigatorWebDriver.cpp:72
> > > >  JSValue JSNavigator::webdriver(ExecState&) const
> > > 
> > > If you drop the [Custom], I believe you'll need to drop this method and add
> > > a new one:
> > > static bool NavigatorWebDriver::webdriver(Navigator&) const 
> > > {
> > >     return isControlledByAutomation();
> > > }
> > 
> > It was a little more than that, but I think it works now. What should we do
> > about worker navigator?
> 
> I do not understand the question, we do not seem to expose
> navigator.webdriver to workers. The spec does not seem to say we should
> expose this to workers either. The specs has a partial interface for
> Navigator but not WorkerNavigator [1].
> 
> [1] https://html.spec.whatwg.org/multipage/workers.html#workernavigator

Oh, I CCed you to the discussion at https://groups.google.com/a/chromium.org/forum/?utm_medium=email&utm_source=footer&pli=1#!msg/blink-dev/6GysDZCWwB8/rXbGoRohBgAJ . 

Anne reported an issue https://github.com/w3c/webdriver/issues/923 which is likely be implemented.

[Chris Dumez]

(In reply to Sergey Shekyan from comment #16)
> (In reply to Chris Dumez from comment #15)
> > (In reply to Sergey Shekyan from comment #13)
> > > (In reply to Chris Dumez from comment #11)
> > > > Comment on attachment 310570 [details]
> > > > Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=310570&action=review
> > > > 
> > > > > Source/WebCore/Modules/webdriver/NavigatorWebDriver.cpp:72
> > > > >  JSValue JSNavigator::webdriver(ExecState&) const
> > > > 
> > > > If you drop the [Custom], I believe you'll need to drop this method and add
> > > > a new one:
> > > > static bool NavigatorWebDriver::webdriver(Navigator&) const 
> > > > {
> > > >     return isControlledByAutomation();
> > > > }
> > > 
> > > It was a little more than that, but I think it works now. What should we do
> > > about worker navigator?
> > 
> > I do not understand the question, we do not seem to expose
> > navigator.webdriver to workers. The spec does not seem to say we should
> > expose this to workers either. The specs has a partial interface for
> > Navigator but not WorkerNavigator [1].
> > 
> > [1] https://html.spec.whatwg.org/multipage/workers.html#workernavigator
> 
> Oh, I CCed you to the discussion at
> https://groups.google.com/a/chromium.org/forum/
> ?utm_medium=email&utm_source=footer&pli=1#!msg/blink-dev/6GysDZCWwB8/
> rXbGoRohBgAJ . 
> 
> Anne reported an issue https://github.com/w3c/webdriver/issues/923 which is
> likely be implemented.

Ok. In any case, exposing this to workers would need to be addressed separately. Brian Burg should probably comment on wether we want to exposed navigator.webDriver to workers or not.

[Sergey Shekyan]

Created attachment 310926 [details]
Patch

[Chris Dumez]

Hmm, looks like the GTK bot does not link:
lib/libWebCoreDerivedSources.a(lib/../Source/WebCore/CMakeFiles/WebCoreDerivedSources.dir/__/__/DerivedSources/WebCore/JSNavigator.cpp.o):JSNavigator.cpp:function WebCore::jsNavigatorWebdriver(JSC::ExecState*, long, JSC::PropertyName): error: undefined reference to 'WebCore::JSNavigator::webdriver(JSC::ExecState&) const'
collect2: error: ld returned 1 exit status

Seems odd. I bet that bot merely needs a clean build.

[Chris Dumez]

Comment on attachment 310926 [details]
Patch

r=me

[Blaze Burg]

(In reply to Chris Dumez from comment #17)
> (In reply to Sergey Shekyan from comment #16)
> > (In reply to Chris Dumez from comment #15)
> > > (In reply to Sergey Shekyan from comment #13)
> > > > (In reply to Chris Dumez from comment #11)
> > > > > Comment on attachment 310570 [details]
> > > > > Patch
> > > > > 
> > > > > View in context:
> > > > > https://bugs.webkit.org/attachment.cgi?id=310570&action=review
> > > > > 
> > > > > > Source/WebCore/Modules/webdriver/NavigatorWebDriver.cpp:72
> > > > > >  JSValue JSNavigator::webdriver(ExecState&) const
> > > > > 
> > > > > If you drop the [Custom], I believe you'll need to drop this method and add
> > > > > a new one:
> > > > > static bool NavigatorWebDriver::webdriver(Navigator&) const 
> > > > > {
> > > > >     return isControlledByAutomation();
> > > > > }
> > > > 
> > > > It was a little more than that, but I think it works now. What should we do
> > > > about worker navigator?
> > > 
> > > I do not understand the question, we do not seem to expose
> > > navigator.webdriver to workers. The spec does not seem to say we should
> > > expose this to workers either. The specs has a partial interface for
> > > Navigator but not WorkerNavigator [1].
> > > 
> > > [1] https://html.spec.whatwg.org/multipage/workers.html#workernavigator
> > 
> > Oh, I CCed you to the discussion at
> > https://groups.google.com/a/chromium.org/forum/
> > ?utm_medium=email&utm_source=footer&pli=1#!msg/blink-dev/6GysDZCWwB8/
> > rXbGoRohBgAJ . 
> > 
> > Anne reported an issue https://github.com/w3c/webdriver/issues/923 which is
> > likely be implemented.
> 
> Ok. In any case, exposing this to workers would need to be addressed
> separately. Brian Burg should probably comment on wether we want to exposed
> navigator.webDriver to workers or not.

I don't think that would be necessary. This property exists to mitigate people using WebDriver for click fraud or other unintended purposes on arbitrary websites. By checking this the page content can perform a mitigation if the "user" is a script. Web content can check this in the main context and propagate to workers if needed.

[Build Bot]

Comment on attachment 310926 [details]
Patch

Attachment 310926 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3796504

New failing tests:
js/dom/navigator-webdriver.html
fast/dom/navigator-detached-no-crash.html

[Build Bot]

Created attachment 310941 [details]
Archive of layout-test-results from ews102 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 310926 [details]
Patch

Attachment 310926 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3796515

New failing tests:
js/dom/navigator-webdriver.html
fast/dom/navigator-detached-no-crash.html

[Build Bot]

Created attachment 310944 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Chris Dumez]

I quickly looked at the test failures and they look expected. I think you merely need to rebaseline those:
Tools/Scripts/run-webkit-tests --reset-results js/dom/navigator-webdriver.html fast/dom/navigator-detached-no-crash.html

[Build Bot]

Comment on attachment 310926 [details]
Patch

Attachment 310926 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3796529

New failing tests:
js/dom/navigator-webdriver.html
fast/dom/navigator-detached-no-crash.html

[Build Bot]

Created attachment 310954 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 310926 [details]
Patch

Attachment 310926 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3796605

New failing tests:
js/dom/navigator-webdriver.html

[Build Bot]

Created attachment 310956 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Sergey Shekyan]

Created attachment 311015 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 311015 [details]
Patch

Clearing flags on attachment: 311015

Committed r217391: <http://trac.webkit.org/changeset/217391>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[David Kilzer (:ddkilzer)]

(In reply to WebKit Commit Bot from comment #32)
> Comment on attachment 311015 [details]
> Patch
> 
> Clearing flags on attachment: 311015
> 
> Committed r217391: <http://trac.webkit.org/changeset/217391>

This appears to have broken the build on our internal clang static analyzer bot:

Undefined symbols for architecture x86_64:
  "WebCore::JSNavigator::webdriver(JSC::ExecState&) const", referenced from:
      WebCore::jsNavigatorWebdriverGetter(JSC::ExecState&, WebCore::JSNavigator&, JSC::ThrowScope&) in JSNavigator.o
ld: symbol(s) not found for architecture x86_64

I'm not sure why this hasn't been seen on any other bots, though, except GTK+ EWS (although trunk builders seem fine):

lib/libWebCoreDerivedSources.a(lib/../Source/WebCore/CMakeFiles/WebCoreDerivedSources.dir/__/__/DerivedSources/WebCore/JSNavigator.cpp.o):JSNavigator.cpp:function WebCore::jsNavigatorWebdriver(JSC::ExecState*, long, JSC::PropertyName): error: undefined reference to 'WebCore::JSNavigator::webdriver(JSC::ExecState&) const'
collect2: error: ld returned 1 exit status

Maybe it needs a clean build?

[Chris Dumez]

(In reply to David Kilzer (:ddkilzer) from comment #34)
> (In reply to WebKit Commit Bot from comment #32)
> > Comment on attachment 311015 [details]
> > Patch
> > 
> > Clearing flags on attachment: 311015
> > 
> > Committed r217391: <http://trac.webkit.org/changeset/217391>
> 
> This appears to have broken the build on our internal clang static analyzer
> bot:
> 
> Undefined symbols for architecture x86_64:
>   "WebCore::JSNavigator::webdriver(JSC::ExecState&) const", referenced from:
>       WebCore::jsNavigatorWebdriverGetter(JSC::ExecState&,
> WebCore::JSNavigator&, JSC::ThrowScope&) in JSNavigator.o
> ld: symbol(s) not found for architecture x86_64
> 
> I'm not sure why this hasn't been seen on any other bots, though, except
> GTK+ EWS (although trunk builders seem fine):
> 
> lib/libWebCoreDerivedSources.a(lib/../Source/WebCore/CMakeFiles/
> WebCoreDerivedSources.dir/__/__/DerivedSources/WebCore/JSNavigator.cpp.o):
> JSNavigator.cpp:function WebCore::jsNavigatorWebdriver(JSC::ExecState*,
> long, JSC::PropertyName): error: undefined reference to
> 'WebCore::JSNavigator::webdriver(JSC::ExecState&) const'
> collect2: error: ld returned 1 exit status
> 
> Maybe it needs a clean build?

Yes, likely needs a clean build.

[Radar WebKit Bug Importer]

<rdar://problem/32479831>