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

feat: added hiding of webdriver #70

Merged
merged 4 commits into from
Apr 4, 2022
Merged

feat: added hiding of webdriver #70

merged 4 commits into from
Apr 4, 2022

Conversation

petrpatek
Copy link
Contributor

@petrpatek petrpatek commented Mar 14, 2022

Maybe we could add this as the default launch args, but I feel this kind of changes the browser fingerprint to stick with this.

@petrpatek petrpatek requested a review from B4nan March 14, 2022 14:51
src/fingerprinting/hooks.ts Show resolved Hide resolved
@@ -29,14 +30,17 @@ export function createFingerprintPreLaunchHook(browserPool: BrowserPool<any, any
}

launchContext.extend({ fingerprint });
// hide webdriver with arg.
// IMHO we could do this by default not only with fingerprinting.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, agreed. I don't think it ever makes sense to run with webdriver: true

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we just have to be careful about using the proper method for different browsers

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we have this covered right now. Maybe we could move it to the plugins level. Playwright works only with build-in firefox and WebKit, which has hidden webDriver by default and hides it in chrome/chromium. We also hide it with Puppeteer chromium/chrome. Maybe we could hide it with the Puppeteer firefox. However, then we would have to hide it for safari and more browsers in Playwright, which is more complicated across versions.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Playwright ... hides it in chrome/chromium.

Does it really though? It might hide it for Chromium, but it definitely did not hide it for Chrome the last time I tested it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not because it is not the default browser. My point is that in other default browsers, the webdriver is false. In chromium-based browsers, we can control it by the argument. However, in other browsers, we have to set the webdriver to false, which needs the fingerprint injector toolkit (webdriver is one of the overridden properties). So my point is that my initial proposal was wrong, and hiding webdriver generally is a fingerprint change that requires the stealth override functions 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would release it like this. We want to turn on the fingerprints by default anyway, right?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like webdriver to be false with the default SDK configuration. Without any extra steps needed for the user. Otherwise, I don't care that much how it works 😄

src/fingerprinting/utils.ts Outdated Show resolved Hide resolved
test/browser-pool.test.ts Outdated Show resolved Hide resolved
petrpatek and others added 2 commits March 14, 2022 16:29
Co-authored-by: Martin Adámek <banan23@gmail.com>
@B4nan
Copy link
Member

B4nan commented Apr 4, 2022

Where are we with this? Can we merge, or is there something missing?

@petrpatek
Copy link
Contributor Author

This is done IMHO. We can merge.

@B4nan B4nan changed the title feat: added hiding of webdriver feat: added hiding of webdriver Apr 4, 2022
@B4nan B4nan merged commit 2bcff2a into master Apr 4, 2022
@B4nan B4nan deleted the feature/webdriver branch April 4, 2022 14:24
@B4nan
Copy link
Member

B4nan commented Apr 4, 2022

cc @AndreyBykov, please backport this commit too

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

Successfully merging this pull request may close these issues.

None yet

3 participants