Allow certain client hints in request headers. #14155

mkarolin · 2022-07-13T05:14:08Z

Per @pes10k we can allow sec-ch-ua, sec-ch-ua-mobile, and sec-ch-ua-platform client hints since they aren’t privacy harming, since they’re already in the UA effectively. This change makes us fall back onto Chromium code for those 3 client hints.

Submitter Checklist:

I confirm that no security/privacy review is needed, or that I have requested one
There is a ticket for my issue
Used Github auto-closing keywords in the PR description above
Wrote a good PR/commit description
Squashed any review feedback or "fixup" commits before merge, so that history is a record of what happened in the repo, not your PR
Added appropriate labels (QA/Yes or QA/No; release-notes/include or release-notes/exclude; OS/...) to the associated issue
Checked the PR locally: npm run test -- brave_browser_tests, npm run test -- brave_unit_tests, npm run lint, npm run gn_check, npm run tslint
Ran git rebase master (if needed)

Reviewer Checklist:

A security review is not needed, or a link to one is included in the PR description
New files have MPL-2.0 license header
Adequate test coverage exists to prevent regressions
Major classes, functions and non-trivial code blocks are well-commented
Changes in component dependencies are properly reflected in gn
Code follows the style guide
Test plan is specified in PR before merging

After-merge Checklist:

Test Plan:

fmarier · 2022-07-13T18:02:35Z

chromium_src/third_party/blink/common/client_hints/enabled_client_hints.cc

@@ -15,7 +15,15 @@ namespace blink {

 void EnabledClientHints::SetIsEnabled(const WebClientHintsType type,
                                      const bool should_send) {
-  enabled_types_[static_cast<int>(type)] = false;
+  switch (type) {
+    case WebClientHintsType::kUA:


What value do we send for this one? The Chromium one or the Chrome one?
https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Sec-CH-UA#examples

We send "Chromium";v="103", ".Not/A)Brand";v="99"

Ok, so we will look different from Chrome then.

@mkarolin shouldn't this be the same as whats in navigator.userAgentData (which is different from both Chrome and Chromium)

Doesn't seem like it. There's a whole bunch of code that determines branding for client hints, specifically in blink::UserAgentBrandList GetUserAgentBrandList. We get the Chromium one because we don't unset CHROMIUM_BRANDING when building. If we unset it we would get ".Not/A)Brand";v="99", "Brave Browser";v="103", "Chromium";v="103"

I think thats what we want, as long as it wouldn't change the standard UA string (do you know if it would?)

Doesn't appear so, the UA string is Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/103.0.0.0 Safari/537.36

Added a patch to apply branding to the UA CH.

pes10k

This looks good to me. Thanks Max!

pes10k · 2022-07-13T21:24:13Z

(slightly modified from Slack)

Looks great, though, either in this or in a follow up PR, would it be possible to:

could we have the change behind a griffin flag, so we can remote-disable it if it turns out that partial CH breaks things
Could we uplift the change and have it on in nightly, but off in Beta and release?

mkarolin · 2022-08-15T22:59:13Z

@iefremov would you mind taking another look, please?

Allowed hints are: kUA kUAMobile kUAPlatform Fixes brave/brave-browser#24009

Allows using brand name in the client hint so that our UACH is ".Not/A)Brand";v="99", "Brave Browser";v="103", "Chromium";v="103" instead of "Chromium";v="103", ".Not/A)Brand";v="99"

We will roll out via Griffin.

…opyValues Fixes brave/brave-browser#23491 It seems uaFullVersion was always leaking but the fullVersionList started leaking because of the change in #14155 where brand was added to GetUserAgentBrandList function in components/embedder_support/user_agent_utils.cc which broke the BraveContentBrowserClient::GetUserAgentMetadata expectation that the brand list would only contain 2 items (instead of now 3). This fix adjusts the BraveContentBrowserClient::GetUserAgentMetadata expectations and removes adding the Brave brand to the lists because it's already there. Now we just need to zero out 3 last components of the full versions list and uaFullVersion string. Also, adds a browser test to check the sizes of the lists and versions values.

mkarolin requested review from pes10k and fmarier July 13, 2022 05:14

mkarolin requested a review from a team as a code owner July 13, 2022 05:14

mkarolin self-assigned this Jul 13, 2022

fmarier reviewed Jul 13, 2022

View reviewed changes

pes10k approved these changes Jul 13, 2022

View reviewed changes

iefremov approved these changes Jul 13, 2022

View reviewed changes

mkarolin requested a review from a team as a code owner July 14, 2022 17:21

mkarolin force-pushed the maxk-allow-certain-client-hints branch from a1f6c08 to 28219c5 Compare July 14, 2022 17:26

mkarolin requested a review from iefremov July 14, 2022 17:26

mkarolin force-pushed the maxk-allow-certain-client-hints branch from 28219c5 to b373ae1 Compare July 14, 2022 18:23

mkarolin force-pushed the maxk-allow-certain-client-hints branch from b373ae1 to f5737fa Compare July 27, 2022 13:28

iefremov approved these changes Aug 17, 2022

View reviewed changes

mkarolin added 3 commits August 18, 2022 14:28

Allow certain client hints in request headers.

d178f3a

Allowed hints are: kUA kUAMobile kUAPlatform Fixes brave/brave-browser#24009

User branding in the UA client hint.

8e64270

Allows using brand name in the client hint so that our UACH is ".Not/A)Brand";v="99", "Brave Browser";v="103", "Chromium";v="103" instead of "Chromium";v="103", ".Not/A)Brand";v="99"

Puts client hints behind a feature flag.

b6cdc2d

mkarolin force-pushed the maxk-allow-certain-client-hints branch from f5737fa to b6cdc2d Compare August 18, 2022 20:17

mkarolin mentioned this pull request Aug 19, 2022

Adds a 50/50 rollout of certain client hints in Nightly. brave/brave-variations#332

Merged

Disable AllowCertainClientHints feature by default.

2f57245

We will roll out via Griffin.

mkarolin merged commit 488026f into master Aug 19, 2022

mkarolin deleted the maxk-allow-certain-client-hints branch August 19, 2022 22:53

mkarolin added this to the 1.44.x - Nightly milestone Aug 19, 2022

This was referenced Sep 12, 2022

Adds a 50/50 rollout of certain client hints in Nightly (production). brave/brave-variations#363

Merged

sec-ch-ua client hint should use the same product name regardless of the channel. brave/brave-browser#25459

Closed

mkarolin mentioned this pull request Nov 30, 2022

Fixes leaking of full version via navigator.userAgentData.getHighEntropyValues #16177

Merged

25 tasks

ShivanKaul mentioned this pull request Dec 1, 2023

Client Hints API is not disabled brave/brave-browser#34644

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow certain client hints in request headers. #14155

Allow certain client hints in request headers. #14155

mkarolin commented Jul 13, 2022

fmarier Jul 13, 2022

mkarolin Jul 13, 2022

fmarier Jul 13, 2022

pes10k Jul 13, 2022

mkarolin Jul 14, 2022

pes10k Jul 14, 2022

mkarolin Jul 14, 2022

mkarolin Jul 14, 2022

pes10k left a comment

pes10k commented Jul 13, 2022

mkarolin commented Aug 15, 2022

Allow certain client hints in request headers. #14155

Allow certain client hints in request headers. #14155

Conversation

mkarolin commented Jul 13, 2022

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pes10k left a comment

Choose a reason for hiding this comment

pes10k commented Jul 13, 2022

mkarolin commented Aug 15, 2022