w3c / webdriver

Remote control interface that enables introspection and control of user agents.
https://w3c.github.io/webdriver/
Other
682 stars 194 forks source link

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

Closed gsnedders closed 6 years ago

gsnedders commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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

cc @annevk

burg commented 6 years ago

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 commented 6 years ago

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.

foolip commented 6 years ago

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.