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.

Sign up for GitHub

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

Jump to bottom

Implement "reduce language fingerprinting" #12234

Merged
merged 27 commits into from
Mar 31, 2022
Merged

Implement "reduce language fingerprinting" #12234

merged 27 commits into from
Mar 31, 2022

Conversation

pilgrim-brave
Copy link
Contributor

@pilgrim-brave pilgrim-brave commented Feb 11, 2022 •
edited
Loading

Resolves brave/brave-browser#20096
Resolves brave/brave-browser#816

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:

@pilgrim-brave pilgrim-brave requested a review from a team as a code owner February 11, 2022 23:03
@pilgrim-brave pilgrim-brave self-assigned this Feb 11, 2022
@github-actions github-actions bot added the potential-layer-violation-fixes This PR touches a BUILD.gn file with check_includes=false label Feb 11, 2022
@pilgrim-brave pilgrim-brave marked this pull request as draft February 11, 2022 23:03
@pilgrim-brave pilgrim-brave marked this pull request as ready for review February 14, 2022 22:05
@github-actions github-actions bot removed the rebase label Feb 14, 2022
@pilgrim-brave pilgrim-brave force-pushed the mpilgrim_reduce_language branch 2 times, most recently from 68c05f9 to 2cfb05e Compare February 16, 2022 22:10
@pilgrim-brave pilgrim-brave changed the title Implement navigator.languages farbling Implement Accept-Language farbling Feb 16, 2022
@pilgrim-brave pilgrim-brave force-pushed the mpilgrim_reduce_language branch 3 times, most recently from 2b02ff4 to b48ac40 Compare March 2, 2022 14:43
@pilgrim-brave pilgrim-brave requested a review from a team as a code owner March 22, 2022 19:24
@pilgrim-brave pilgrim-brave changed the title Implement Accept-Language farbling Implement "reduce language fingerprinting" Mar 23, 2022
bridiver
bridiver reviewed Mar 24, 2022
View reviewed changes
browser/about_flags.cc
@@ -60,6 +60,7 @@ using brave_shields::features::kBraveDarkModeBlock;
using brave_shields::features::kBraveDomainBlock;
using brave_shields::features::kBraveDomainBlock1PES;
using brave_shields::features::kBraveExtensionNetworkBlocking;
using brave_shields::features::kBraveReduceLanguage;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should have a more descriptive name for this. Maybe AcceptLanguageFingerprintingProtection or AcceptLanguageFarbling?

bridiver
bridiver reviewed Mar 24, 2022
View reviewed changes
browser/about_flags.cc
@@ -146,6 +147,10 @@ constexpr char kBraveExtensionNetworkBlockingName[] =
constexpr char kBraveExtensionNetworkBlockingDescription[] =
"Enable blocking for network requests initiated by extensions";

constexpr char kBraveReduceLanguageName[] = "Reduce language identifiability";
Copy link
Collaborator

Choose a reason for hiding this comment

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

in all cases I think we should be clear that this is about accept-language

Copy link
Contributor Author

Choose a reason for hiding this comment

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

well except it's more than that because the same setting controls font whitelisting

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, seconding @pilgrim-brave , this setting controls both the explicit language preferneces in accept-language and navigator.languages, but also the implicit language preferences based on fonts available on the system

Copy link
Collaborator

Choose a reason for hiding this comment

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

I still think kBraveReduceLanguage is going to be basically meaningless to anyone looking at the code. Other suggestions?

bridiver
bridiver reviewed Mar 28, 2022
View reviewed changes
browser/farbling/BUILD.gn
@@ -15,6 +15,7 @@ if (!is_android) {
"brave_navigator_devicememory_farbling_browsertest.cc",
"brave_navigator_hardwareconcurrency_farbling_browsertest.cc",
"brave_navigator_keyboard_api_browsertest.cc",
"brave_navigator_languages_farbling_browsertest.cc",
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this in farbling when the other parts of the code are in brave shields?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess more generally why does this directory exist if it's only tests? That seems a bit odd to me

@pilgrim-brave pilgrim-brave added this to the 1.39.x - Nightly milestone Mar 30, 2022
@pilgrim-brave pilgrim-brave requested a review from a team as a code owner March 30, 2022 22:47
@pilgrim-brave pilgrim-brave merged commit f6dddf2 into master Mar 31, 2022
@pilgrim-brave pilgrim-brave deleted the mpilgrim_reduce_language branch March 31, 2022 17:41
@petemill
Copy link
Member

petemill commented Apr 5, 2022

Note that this is not showing up correctly due to syntax issue. If it gets uplifted or anything then it will need to include the fix by @nullhook in #12881

@ShivanKaul ShivanKaul mentioned this pull request Apr 20, 2022
Merged
25 tasks
@ShivanKaul
Copy link
Collaborator

Similar note to ^: if we decide to uplift this, we should make sure that the ReduceLanguage setting is next to De-AMP near the top of brave://settings/shields like it is in 1.39.x. It was moved in #12999 (misc fixes) but could not be part of the De-AMP misc fixes uplift PR (#13080) because the ReduceLang feature doesn't exist in 1.38.x

bsclifton added a commit that referenced this pull request Jun 22, 2022
@bsclifton
Revert Implement "reduce language fingerprinting' and follow ups
10ed0c2 
Fixes brave/brave-browser#23093

Unfixes brave/brave-browser#20096
Unfixes brave/brave-browser#816

Reverts #13807
Which fixed brave/brave-browser#23489

Reverts #13205
Which fixed brave/brave-browser#22605

Reverts #12234
Which fixed brave/brave-browser#20096 and brave/brave-browser#816
bsclifton added a commit that referenced this pull request Jun 22, 2022
@bsclifton
Revert Implement "reduce language fingerprinting' and follow ups
7177a3e 
Fixes brave/brave-browser#23093

Unfixes brave/brave-browser#20096
Unfixes brave/brave-browser#816

Reverts #13807
Which fixed brave/brave-browser#23489

Reverts #13205
Which fixed brave/brave-browser#22605

Reverts #12234
Which fixed brave/brave-browser#20096 and brave/brave-browser#816
@bsclifton bsclifton mentioned this pull request Jun 22, 2022
Closed
25 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pes10k pes10k pes10k left review comments

@bridiver bridiver bridiver approved these changes

Assignees

@pilgrim-brave pilgrim-brave

Labels
potential-layer-violation-fixes This PR touches a BUILD.gn file with check_includes=false
Projects
None yet
Milestone
1.39.x - Release
Development

Successfully merging this pull request may close these issues.

Fingerprinting v3: Accept-Language Fingerprinting v3: Font Fingerprinting
5 participants
@pilgrim-brave @petemill @ShivanKaul @bridiver @pes10k