Bug 171997 - navigator.webdriver should return false if the page is not controlled by automation
Summary: navigator.webdriver should return false if the page is not controlled by auto...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-05-11 16:11 PDT by Sergey Shekyan
Modified: 2017-05-30 20:27 PDT (History)
9 users (show)

See Also:


Attachments
Patch (1.94 KB, patch)
2017-05-18 15:56 PDT, Sergey Shekyan
no flags Details | Formatted Diff | Diff
Patch (1.95 KB, patch)
2017-05-18 16:12 PDT, Sergey Shekyan
no flags Details | Formatted Diff | Diff
Patch (6.02 KB, patch)
2017-05-22 14:12 PDT, Sergey Shekyan
no flags Details | Formatted Diff | Diff
Patch (6.05 KB, patch)
2017-05-22 14:59 PDT, Sergey Shekyan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-elcapitan (1.52 MB, application/zip)
2017-05-22 15:57 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews104 for mac-elcapitan-wk2 (1.56 MB, application/zip)
2017-05-22 16:03 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-elcapitan (2.41 MB, application/zip)
2017-05-22 16:39 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (4.14 MB, application/zip)
2017-05-22 16:54 PDT, Build Bot
no flags Details
Patch (6.78 KB, patch)
2017-05-23 08:53 PDT, Sergey Shekyan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Sergey Shekyan 2017-05-11 16:11:25 PDT
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.
Comment 1 Chris Dumez 2017-05-11 16:18:22 PDT
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
Comment 2 Brian Burg 2017-05-11 16:25:33 PDT
(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.
Comment 3 Sergey Shekyan 2017-05-11 16:35:25 PDT
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.
Comment 4 Sergey Shekyan 2017-05-18 15:56:09 PDT
Created attachment 310567 [details]
Patch
Comment 5 Sergey Shekyan 2017-05-18 15:57:57 PDT
Current webdriver editor's draft changed to declare navigator.webdriver as non-configurable (https://w3c.github.io/webdriver/webdriver-spec.html#interface)
Comment 6 Build Bot 2017-05-18 15:59:52 PDT
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.
Comment 7 Sergey Shekyan 2017-05-18 16:12:49 PDT
Created attachment 310570 [details]
Patch
Comment 8 Chris Dumez 2017-05-18 16:33:43 PDT
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.
Comment 9 Chris Dumez 2017-05-19 16:25:31 PDT
Let me know if this is more work than expected and you'd like me to take over. I don't mind fixing this.
Comment 10 Sergey Shekyan 2017-05-19 17:18:21 PDT
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.
Comment 11 Chris Dumez 2017-05-19 18:10:58 PDT
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();
}
Comment 12 Sergey Shekyan 2017-05-22 14:12:39 PDT
Created attachment 310918 [details]
Patch
Comment 13 Sergey Shekyan 2017-05-22 14:14:13 PDT
(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?
Comment 14 Chris Dumez 2017-05-22 14:21:47 PDT
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.
Comment 15 Chris Dumez 2017-05-22 14:24:57 PDT
(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
Comment 16 Sergey Shekyan 2017-05-22 14:37:51 PDT
(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.
Comment 17 Chris Dumez 2017-05-22 14:56:40 PDT
(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.
Comment 18 Sergey Shekyan 2017-05-22 14:59:25 PDT
Created attachment 310926 [details]
Patch
Comment 19 Chris Dumez 2017-05-22 15:10:40 PDT
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.
Comment 20 Chris Dumez 2017-05-22 15:11:19 PDT
Comment on attachment 310926 [details]
Patch

r=me
Comment 21 Brian Burg 2017-05-22 15:28:49 PDT
(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.
Comment 22 Build Bot 2017-05-22 15:57:26 PDT
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
Comment 23 Build Bot 2017-05-22 15:57:27 PDT
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
Comment 24 Build Bot 2017-05-22 16:03:04 PDT
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
Comment 25 Build Bot 2017-05-22 16:03:05 PDT
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
Comment 26 Chris Dumez 2017-05-22 16:05:12 PDT
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
Comment 27 Build Bot 2017-05-22 16:39:09 PDT
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
Comment 28 Build Bot 2017-05-22 16:39:10 PDT
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
Comment 29 Build Bot 2017-05-22 16:54:28 PDT
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
Comment 30 Build Bot 2017-05-22 16:54:30 PDT
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
Comment 31 Sergey Shekyan 2017-05-23 08:53:36 PDT
Created attachment 311015 [details]
Patch
Comment 32 WebKit Commit Bot 2017-05-24 16:05:56 PDT
Comment on attachment 311015 [details]
Patch

Clearing flags on attachment: 311015

Committed r217391: <http://trac.webkit.org/changeset/217391>
Comment 33 WebKit Commit Bot 2017-05-24 16:05:58 PDT
All reviewed patches have been landed.  Closing bug.
Comment 34 David Kilzer (:ddkilzer) 2017-05-25 01:15:35 PDT
(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?
Comment 35 Chris Dumez 2017-05-25 07:07:23 PDT
(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.
Comment 36 Radar WebKit Bug Importer 2017-05-30 20:27:20 PDT
<rdar://problem/32479831>