WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
171997
navigator.webdriver should return false if the page is not controlled by automation
https://bugs.webkit.org/show_bug.cgi?id=171997
Summary
navigator.webdriver should return false if the page is not controlled by auto...
Sergey Shekyan
Reported
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.
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
Show Obsolete
(4)
View All
Add attachment
proposed patch, testcase, etc.
Chris Dumez
Comment 1
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
Blaze Burg
Comment 2
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.
Sergey Shekyan
Comment 3
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.
Sergey Shekyan
Comment 4
2017-05-18 15:56:09 PDT
Created
attachment 310567
[details]
Patch
Sergey Shekyan
Comment 5
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
)
Build Bot
Comment 6
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.
Sergey Shekyan
Comment 7
2017-05-18 16:12:49 PDT
Created
attachment 310570
[details]
Patch
Chris Dumez
Comment 8
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.
Chris Dumez
Comment 9
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.
Sergey Shekyan
Comment 10
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.
Chris Dumez
Comment 11
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(); }
Sergey Shekyan
Comment 12
2017-05-22 14:12:39 PDT
Created
attachment 310918
[details]
Patch
Sergey Shekyan
Comment 13
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?
Chris Dumez
Comment 14
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.
Chris Dumez
Comment 15
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
Sergey Shekyan
Comment 16
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.
Chris Dumez
Comment 17
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.
Sergey Shekyan
Comment 18
2017-05-22 14:59:25 PDT
Created
attachment 310926
[details]
Patch
Chris Dumez
Comment 19
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.
Chris Dumez
Comment 20
2017-05-22 15:11:19 PDT
Comment on
attachment 310926
[details]
Patch r=me
Blaze Burg
Comment 21
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.
Build Bot
Comment 22
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
Build Bot
Comment 23
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
Build Bot
Comment 24
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
Build Bot
Comment 25
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
Chris Dumez
Comment 26
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
Build Bot
Comment 27
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
Build Bot
Comment 28
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
Build Bot
Comment 29
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
Build Bot
Comment 30
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
Sergey Shekyan
Comment 31
2017-05-23 08:53:36 PDT
Created
attachment 311015
[details]
Patch
WebKit Commit Bot
Comment 32
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
>
WebKit Commit Bot
Comment 33
2017-05-24 16:05:58 PDT
All reviewed patches have been landed. Closing bug.
David Kilzer (:ddkilzer)
Comment 34
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?
Chris Dumez
Comment 35
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.
Radar WebKit Bug Importer
Comment 36
2017-05-30 20:27:20 PDT
<
rdar://problem/32479831
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug