-
-
Notifications
You must be signed in to change notification settings - Fork 742
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(plugin-stealth): Add support for UA hints #413
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good stuff! Added few comments/suggestions :-)
packages/puppeteer-extra-plugin-stealth/evasions/user-agent-override/index.js
Show resolved
Hide resolved
packages/puppeteer-extra-plugin-stealth/evasions/user-agent-override/index.test.js
Outdated
Show resolved
Hide resolved
packages/puppeteer-extra-plugin-stealth/evasions/user-agent-override/index.test.js
Outdated
Show resolved
Hide resolved
I just noticed a more common use-case we might want to optimize for: I think deriving the platform from the UA string is neat and was wondering if we can additionally provide a platform override as well (in case the rest of the UA isn't relevant to the user). Maybe there's a lightweight UA string manipulation library out there that allows us to just replace |
That's a good one, let me look into this. Another thing I thought of is skipping the |
How do you mean? :-) We do set |
Correct, and that leads to wrong order for the |
Yeah, that makes sense. I'm a bit concerned as the user-data-dir plugin is quite old and not as well tested but overall it should do the trick :-) It'd be good to detect if we're using |
I pushed some changes in 04445d5: it always sets the preferences - which are ignored on headless. On headless, it uses the Good point about |
Looks good! Do we want to fix the Linux use-case before merging this in? I can't think of a scenario where it'd be desired to signal linux as the platform 🤔 A quick fix could be:
WDYT? |
Hmmm, while it definitely makes sense UX-wise, it can also be confusing. I assume that many users run macOS or Windows to test and Linux to deploy - having the default of this evasion act different depending on the host is a bit strange IMHO. Maybe it makes more sense to output a big fat warning in case the UA contains In any case it's an improvement to the situation before this PR, where the default was to use the host UA + hardcoded |
Replacing Linux as the platform has been the default since the first version of this evasion (in a broken way though lol) :-) There's still no good UX story here when upgrading to this version of the evasion and running things in a docker container (and wanting to hide that) - it'd still be necessary to provide a full UA string which is hard to do when initializing the plugin and if hardcoded will result in outdated/mismatching version numbers quickly 🤔 |
Small thing, we might want to add the |
Are you sure about that? I can't find any place where "Linux" in the UA is replaced with "Windows NT XX" for example. Only the
Hmm yes I agree - it's kind of a breaking change. What do you suggest? There's no way to make "required" options, right? |
Apologies, I didn't formulate that clear enough: Yeah, initially we used the Overall I think it'd be good to replace Linux (both in the UA and the platform) by default, with an opt-out (also something we didn't have before) or automatic opt-out when providing a custom UA string. As for the implementation: Not sure if it's worthwhile pulling in the |
I agree that masking Linux is a fair bit "opinionated" though :-) |
Let's approach this differently, the previous evasion was broken in behavior (spoofing Win32 platform on macOS as well, etc). What I would want an ideal/proper
Except for the last thing we're already there 😄 The reason I think masking Linux by default is worthwhile: Everyone running pptr in docker will have do it anyway and the current ergonomics (removing the We eventually will refactor the stealth plugin to allow for a single config object without needing to remove evasions first - if we had that already I wouldn't mind making this behavior opt-in :-) |
OK cool, that sounds good to me. I'll add a |
Sounds good :-) No strong preference regarding macOS > Linux, though I think it'd be fine not spoofing macOS (as it's considered a regular user platform of sorts lol) |
Good point - added Linux-only spoofing now with a |
Looking good 👍 |
|
packages/puppeteer-extra-plugin-stealth/evasions/user-agent-override/index.js
Show resolved
Hide resolved
Since the |
The dependency on
Since this dependency is meant to avoid initial failure #413 (comment), it may be better to have an option to avoid overriding existing user prefs, thus avoid unexpected side effect. |
This PR adds support for UA hints to the
user-agent-override
evasion. Note that this is a breaking change, I opted to change the functionality of the evasion a bit and detect the platform (as well as the now required version, model, architecture, etc) all from the provided user agent string.Unfortunately UA hints only work in headful (yet another reason not to use headless...), so the test needs to run in headful.