Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose navigator.webdriver regardless of whether we're running under automation or not #1214

Closed
gsnedders opened this issue Jan 29, 2018 · 5 comments

Comments

@gsnedders
Copy link
Contributor

This was changed as a result of #947, which came from a response from @foolip to the Blink Intent to Ship.

To quote him from there:

I do think it'd be an improvement if the attribute doesn't exist on the prototype chain at all when not controlled by webdriver. That way, there's no chance that content in the wild could come to depend on it in any way for the majority cases where it's a real user. Not incidentally, that's also how I think we should expose any other test-only APIs that only exist while under WebDriver control, like the APIs in https://wicg.github.io/webusb/test/.

I don't see how making the attribute selectively present (and unlike anything else) achieves that goal. Content in the wild can easily come to rely on this:

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

v, without this behaviour:

var underAutomation = navigator.webdriver;
@gsnedders
Copy link
Contributor Author

From discussion on IRC:

16:04 < gsnedders> I have more sympathy for not exposing test-only APIs
16:05 < gsnedders> Because that seems more like a pref'd on/off thing
16:05 < gsnedders> Unlike a "am I under automation" flag
16:05 < gsnedders> Like, you could imagine a browser having them behind a pref so they could write (trusted) tests that used that API without going over WebDriver
16:06 < gsnedders> It's the fact it's literally an on/off flag, and we already have that in the spec today (is the property present or not).
16:06 < gsnedders> Just the way we have it in the spec today is /weird/.

@andreastt
Copy link
Member

It might make sense to loop in @burg, @kereliuk, and @thejohnjansen here.

cc @annevk

@burg
Copy link
Contributor

burg commented Jan 29, 2018

In my initial implementation (Safari 10 era), I made it behave as @foolip suggested, where the property seemed to not exist if running a normal session. This simply confused people who expected it to exist given that Safari 10 introduced 1st-party WebDriver support. It was changed in Safari 11 era to be always exposed with value true|false since there was no real benefit (privacy, implementation ease, performance, or otherwise) to making its existence conditional. Currently, it is conditional on whether WebKit is built with WebDriver support (ENABLE_REMOTE_AUTOMATION), just like any number of other feature-dependent IDL bindings.

I am in favor of making it always enumerable, and return true or false.

@InstyleVII
Copy link
Contributor

Edge also has the property exposed regardless of automation so if I run "navigator.webdriver" right now we return false. It just seems weird to have conditional properties in general only exposed when WebDriver is running and would probably cause more harm than good.

AutomatedTester pushed a commit that referenced this issue Feb 2, 2018
The navigator.webdriver WebIDL attribute is currently only meant
to be exposed when the webdriver-active flag is true.  Safari and
Edge expose the attribute unconditionally, and Gecko plans to follow
suit.

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;

Fixes: #1214
@foolip
Copy link
Member

foolip commented Feb 23, 2018

I'm late to the party, but want to say that I'm OK with this change, if it means we'll align on something that others have already implemented. Back when I suggested making it conditionally exposed, I put a rather high likelihood on us wanting to expose more test-only APIs in the same way, and also thought it's just as well to use a single mechanism. That hasn't happened, and if it does we'll just have some inconsistency, that's OK.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants