Remove Selenium webdriver attributes from DOM
#108
Conversation
Also includes automated tests. It seems the order of content script execution is non-deterministic. Sometimes selenium will run first and sometimes openwpm's will run first. We should be able to handle this by monitoring the DOM for changes, but need to confirm the performance degredation isn't too high.
The document attribute is removed with a DOMAttrModified eventListener that removes itself after the first call. The navigator attribute is prevented from being set by altering Object.defineProperty until Selenium attempts to set the attribute (at which point the alteration is reversed).
| driver = kwargs['driver'] | ||
|
|
||
| # Check if document element has `webdriver` attribute | ||
| assert 'true' != driver.execute_script( |
There was a problem hiding this comment.
@englehardt these asserts fail when you have disable_webdriver_self_id=false, right?
Just wanted to make sure since they are called within a function that is passed around.
There was a problem hiding this comment.
Yes, I added a pathway in 994b080 to handle and reraise exceptions from the child process in the main thread/process. In particular, it handles AssertionError used by py.test.
As a sanity check, I just made a new commit that tests both conditions.
…h option enabled and disabled
| assert 'true' != driver.execute_script( | ||
| 'return document.documentElement.getAttribute("webdriver")') | ||
| # Check if navigator has webdriver property | ||
| assert not driver.execute_script('return navigator.webdriver') |
There was a problem hiding this comment.
Since we need to match the exact response from a standard (non-Selenium) browser, I'd do all these checks in JS, using strict equality (=== or !==). Something like:
assert driver.execute_script('return undefined === navigator.webdriver')
assert driver.execute_script('return null === document.documentElement.getAttribute("webdriver")')
| value: originalDefineProperty | ||
| }); | ||
| delete originalDefineProperty; | ||
| return; |
There was a problem hiding this comment.
Could we use return undefined; instead of return;?
The former would be more readable.
| Object.defineProperty(Object, 'defineProperty', { | ||
| value: originalDefineProperty | ||
| }); | ||
| delete originalDefineProperty; |
There was a problem hiding this comment.
I couldn't understand why we have to delete originalDefineProperty. A comment would be great.
Also, what happens when Object.defineProperty is called more than once with the arguments navigator and "webdriver"? Wouldn't it set Object.defineProperty to undefined since originalDefineProperty is deleted in the first call?
There was a problem hiding this comment.
My intention was to prevent that variable from staying around the global namespace. However I tested it and I think that's handled anyway with the way we inject scripts. I've removed the delete statement.
|
This looks way better than my naive approach, it has the resilience we need to cope with various race conditions. My high-level concern is the overwriting of Please see my other, more specific comments next to the code. |
Added a bunch of new tests to ensure Object.defineProperty still works as expected after our instrumentation runs and removes Selenium's webdriver property. Other tests refactored to better handle a few conditions.
|
Thanks for the detailed review. Just pushed a couple commits to address your comments. I tried to avoid messing with
Extending from (3) I think it might be possible to define our own property and have some special handling on deletion that prevents the subsequent re-creation of the property...but my current approach seems cleaner. |
|
Thanks for addressing all the comments, @englehardt. |
Conflicts: automation/Extension/firefox/openwpm.xpi
|
Hi! @gunesacar Is it possible to remove |
See commit comments for more information.
@gunesacar Let me know what you think! Thanks for the work on #105.