-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
core(emulation): set client-hints metadata when spoofing the UA #13341
Conversation
const version = fullVersion.split('.', 1)[0]; | ||
const brands = [ | ||
{brand: 'Chromium', version}, | ||
{brand: 'Google Chrome', version}, |
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.
wdyt of adding a entry for LH version here?
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.
and if we do that, we should randomly sort the array, to support the GREASE stuffs (or will chrome do that for us in the request headers?)
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.
wdyt of adding a entry for LH version here?
done
and if we do that, we should randomly sort the array, to support the GREASE stuffs (or will chrome do that for us in the request headers?)
seems like fun.. and its jsut adding a .sort(_ => Math.random() - 0.5)
... but we add some non-determinimism without much gain so i think we should put it off. leave the greasing to the big boiz.
await session.sendCommand('Network.setUserAgentOverride', { | ||
userAgent: /** @type {string} */ (settings.emulatedUserAgent), | ||
userAgent, | ||
userAgentMetadata: parseUseragentIntoMetadata(userAgent, settings.formFactor), | ||
}); | ||
} | ||
// See devtools-entry for one usecase for disabling screenEmulation |
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.
needs tests, esp. for the case where the UA is user-provided and doesn't even have a chrome version in it
lighthouse-core/lib/emulation.js
Outdated
function parseUseragentIntoMetadata(userAgent, formFactor) { | ||
const match = userAgent.match(/Chrome\/([\d.]+)/); // eg 'Chrome/(71.0.3577.0)' | ||
const fullVersion = (match && match[1]) || '99.0.1234.0'; | ||
const version = fullVersion.split('.', 1)[0]; |
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.
const version = fullVersion.split('.', 1)[0]; | |
const [version] = fullVersion.split('.', 1); |
😎
We've discovered that some webservers misbehave when they get a mobile useragent but empty clienthints headers.
And since a while ago... when we set
Emulation.setUserAgentOverride
.. it appropriately sets the useragent string, but since nouserAgentMetadata
was provided, all those values are undefined. As client hints are being adopted, this leads to weird and brokenish scenarios. (depending on how defensive folks are configuring their ua handling on the webserver side).We'll want to make sure clienthint headers are appropriately populated.
I went for making a change that minimizes impact on people who configure lighthouse with custom UA. I think we can revisit exactly how to expose those fields. I'd go for something less verbose than pptr's choice of replicating the exact same payload.