Always return light mode in fingerprinting strict mode #8832

ShivanKaul · 2021-05-17T20:40:44Z

This PR adds a patch to `blink/renderer/core/css/media_values.cc` that always returns `mojom::PreferredColorScheme::kLight` if fingerprinting mode is strict. Note that a page's override media feature will still be respected.

On @bridiver's recommendation, overriding BraveContentBrowserClient::OverrideWebPreferencesAfterNavigation to return light mode if fingerprinting mode = strict instead of patching blink/renderer/core/css/media_values.cc.
Resolves brave/brave-browser#15265.

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
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:

pes10k · 2021-05-17T21:02:03Z

This looks great, though a few Qs:

Is there any issue related to workers here? Is this approach able to find the right origin in a worker process?
I think AllowFingerprinting will return false with default fingerprinting protections. If thats correct, the intended behavior here is to only apply this protection in "strict" mode

cc @pilgrim-brave who's expertise here would be appreciated :)

pilgrim-brave · 2021-05-18T15:18:18Z

I think AllowFingerprinting will return false with default fingerprinting protections. If thats correct, the intended behavior here is to only apply this protection in "strict" mode

Incorrect. For historical reasons, AllowFingerprinting returns true on default settings. Shivan's code is correct.

pilgrim-brave · 2021-05-18T15:25:41Z

Is there any issue related to workers here? Is this approach able to find the right origin in a worker process?

No issues with workers for CSS things.

pilgrim-brave · 2021-05-18T15:28:23Z

Please update the description of this PR to note that we will still respect a page's media feature override. It's just the user's preferred color scheme that is being overridden.

pilgrim-brave · 2021-05-18T15:28:54Z

Also, tests.

ShivanKaul · 2021-05-19T20:48:11Z

Writing the test was a lot trickier than the actual feature 😅 (thanks a lot Mark for the help with figuring out what test dependency to add) and had to run sync again. I took inspiration from the Chromium test. I've updated the PR.

pes10k

Looks good to me (and thanks to @pilgrim-brave for ducking in too)

Are the CI failures unrelated?

ShivanKaul · 2021-05-21T16:16:20Z

Are the CI failures unrelated?

I think so, but digging in - these are the failing tests on Linux:

linux / test-browser / RewardsBrowserTest.ShowACPercentInThePanel | 5.5 sec | 1
linux / test-browser / RewardsNotificationBrowserTest.InsufficientNotificationForACNotEnoughFunds | 5.5 sec | 1
linux / test-browser / BraveAdsBrowserTest/BraveAdsUpgradeBrowserTest.UpgradePath/PreferencesForVersion070WithRewardsAndAdsEnabled_ForSupportedLocale_ForUnsupportedLocale_RewardsShouldBeEnabled_AdsShouldBeEnabled | 0.45 sec | 2
linux / test-browser / BraveAdsBrowserTest/BraveAdsUpgradeBrowserTest.UpgradePath/PreferencesForVersion072WithRewardsAndAdsEnabled_ForSupportedLocale_ForUnsupportedLocale_RewardsShouldBeEnabled_AdsShouldBeEnabled

and on Mac:

macos-x64 / test-browser / AdBlockServiceTest.CnameCloakedRequestsGetBlocked

The latter looks like will be solved by #8883

browser/farbling/brave_dark_mode_fingerprint_protection_browsertest.cc

pilgrim-brave · 2021-05-21T16:41:51Z

Are the CI failures unrelated?

I think so, but digging in - these are the failing tests on Linux:

Those are unrelated.

bridiver · 2021-05-24T16:57:13Z

patches/third_party-blink-renderer-core-css-media_values.cc.patch

+     if (value.IsValid())
+       return CSSValueIDToPreferredColorScheme(value.id);
+   }
+  BRAVE_DARK_MODE_DETECTION_DISABLED


this does not require patching, it can be controlled in the browser process in ContentBrowserClient::OverrideWebPreferencesAfterNavigation

bridiver

preferred color can be changed using web prefs

browser/brave_content_browser_client.cc

bridiver · 2021-05-25T17:30:16Z

browser/farbling/brave_dark_mode_fingerprint_protection_browsertest.cc

+
+  bool NavigateToURLUntilLoadStop(const GURL& url) {
+    ui_test_utils::NavigateToURL(browser(), url);
+    return WaitForLoadStop(contents());


this should ASSERT_TRUE and vs return since you're not doing anything with the return value anyway

bridiver · 2021-05-25T19:56:04Z

browser/farbling/brave_dark_mode_fingerprint_protection_browsertest.cc

+  void SetUpOnMainThread() override {
+    InProcessBrowserTest::SetUpOnMainThread();
+
+    content_client_.reset(new ChromeContentClient);


this doesn't seem necessary for anything

Agreed, I went and removed a bunch of other unnecessary stuff in the test and tried to prune the includes as much as possible.

bridiver · 2021-05-25T19:57:36Z

browser/farbling/brave_dark_mode_fingerprint_protection_browsertest.cc

+};
+
+IN_PROC_BROWSER_TEST_F(BraveDarkModeFingerprintProtection, DarkModeCheck) {
+  test_theme_.SetDarkMode(true);


you should also test with SetDarkMode(false) to ensure that it isn't affected by this change

ShivanKaul · 2021-05-29T17:40:41Z

The CI is failing on the Android build which seems to be failing since yesterday. @mihaiplesa helped me dig up the failures on Android and they don't seem to be connected to my changes (which should have nothing to do with Android).

[2021-05-28T16:48:39.057Z] FAILED: obj/chrome/android/chrome_app_java_resources.resources.zip obj/chrome/android/chrome_app_java_resources.resources.zip.info gen/chrome/android/chrome_app_java_resources_R.txt 
[2021-05-28T16:48:39.057Z] python ../../build/android/gyp/prepare_resources.py --depfile gen/chrome/android/chrome_app_java_resources.d --res-sources-path=gen/chrome/android/chrome_app_java_resources.res.sources --resource-zip-out obj/chrome/android/chrome_app_java_resources.resources.zip --r-text-out gen/chrome/android/chrome_app_java_resources_R.txt
[2021-05-28T16:48:39.058Z] Error: Found files not listed in the sources list of the BUILD.gn target:
[2021-05-28T16:48:39.058Z] ../../chrome/android/java/res/xml/background_video_playback_preference.xml
[2021-05-28T16:48:39.058Z] ../../chrome/android/java/res/xml/closing_all_tabs_closes_brave_preference.xml
[2021-05-28T16:48:39.058Z] [35438/52923] ACTION //chrome/browser/search_engines/android:java__process__bytecode_rewrite(//build/toolchain/android:android_clang_x86)
[2021-05-28T16:48:39.058Z] change superclass of org/chromium/chrome/browser/search_engines/settings/SearchEngineAdapter to org/chromium/chrome/browser/search_engines/settings/BraveBaseSearchEngineAdapter
[2021-05-28T16:48:39.058Z] make mSearchEngineAdapter public in org/chromium/chrome/browser/search_engines/settings/SearchEngineSettings
[2021-05-28T16:48:39.058Z] use invoke virtual for call to method org/chromium/chrome/browser/search_engines/settings/SearchEngineSettings.createAdapterIfNecessary
[2021-05-28T16:48:39.058Z] use invoke virtual for call to method org/chromium/chrome/browser/search_engines/settings/SearchEngineSettings.createAdapterIfNecessary
[2021-05-28T16:48:39.058Z] use invoke virtual for call to method org/chromium/chrome/browser/search_engines/settings/SearchEngineSettings.createAdapterIfNecessary
[2021-05-28T16:48:39.058Z] make createAdapterIfNecessary public in org/chromium/chrome/browser/search_engines/settings/SearchEngineSettings
[2021-05-28T16:48:39.058Z] redirecting ownership for android/widget/BaseAdapter.<init> in org/chromium/chrome/browser/search_engines/settings/SearchEngineAdapter - new owner org/chromium/chrome/browser/search_engines/settings/BraveBaseSearchEngineAdapter

- Override web preference setting in browser process instead of patch - Add tests for dark mode fingerprinting protection - Add deps in brave/browser/farbling

ShivanKaul · 2021-06-01T16:47:06Z

After rebasing a couple of times, there's only one test crashing on Linux CI: RewardsContributionBrowserTest.AutoContributionMultiplePublishersUphold. This just started failing at brave/components/brave_rewards/browser/test/common/rewards_browsertest_context_util.cc:128 because of Internal Error: ExecuteScriptHelper failed and looks disconnected from my changes, so I'll go ahead and merge my changes in.

ShivanKaul requested a review from pes10k May 17, 2021 20:40

ShivanKaul requested a review from a team as a code owner May 17, 2021 20:40

github-actions bot assigned ShivanKaul May 17, 2021

ShivanKaul force-pushed the fingerprinting/dark-mode-block branch 2 times, most recently from 7e359a3 to ce8cb65 Compare May 17, 2021 23:28

ShivanKaul added the feature/shields label May 19, 2021

ShivanKaul force-pushed the fingerprinting/dark-mode-block branch from ce8cb65 to f66f5bf Compare May 19, 2021 20:44

ShivanKaul requested a review from bridiver as a code owner May 19, 2021 20:44

pes10k approved these changes May 21, 2021

View reviewed changes

pilgrim-brave approved these changes May 21, 2021

View reviewed changes

browser/farbling/brave_dark_mode_fingerprint_protection_browsertest.cc Outdated Show resolved Hide resolved

ShivanKaul force-pushed the fingerprinting/dark-mode-block branch from f66f5bf to fe6c1fc Compare May 21, 2021 17:05

bridiver reviewed May 24, 2021

View reviewed changes

bridiver requested changes May 24, 2021

View reviewed changes

bridiver reviewed May 25, 2021

View reviewed changes

browser/brave_content_browser_client.cc Show resolved Hide resolved

bridiver reviewed May 25, 2021

View reviewed changes

ShivanKaul force-pushed the fingerprinting/dark-mode-block branch from af01a80 to 0668563 Compare May 25, 2021 19:31

bridiver reviewed May 25, 2021

View reviewed changes

ShivanKaul force-pushed the fingerprinting/dark-mode-block branch 2 times, most recently from f134807 to daf0eb3 Compare May 26, 2021 01:00

bridiver approved these changes May 27, 2021

View reviewed changes

ShivanKaul force-pushed the fingerprinting/dark-mode-block branch from daf0eb3 to 15ce4a5 Compare May 28, 2021 16:11

Always return light mode in fingerprinting strict mode

98a2af4

- Override web preference setting in browser process instead of patch - Add tests for dark mode fingerprinting protection - Add deps in brave/browser/farbling

ShivanKaul force-pushed the fingerprinting/dark-mode-block branch from 15ce4a5 to 98a2af4 Compare June 1, 2021 12:54

ShivanKaul merged commit 7a6bae2 into master Jun 1, 2021

ShivanKaul deleted the fingerprinting/dark-mode-block branch June 1, 2021 16:47

ShivanKaul added this to the 1.27.x - Nightly milestone Jun 11, 2021

stephendonner mentioned this pull request Jun 14, 2021

Fingerprinting v3: Dark Mode detection brave/brave-browser#15265

Closed

rebron mentioned this pull request Jun 21, 2021

Built-in dark theme is broken after auto-updating Brave browser beta from 1.26.x to 1.27.69 when strict fingerprinting is enabled brave/brave-browser#16496

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Always return light mode in fingerprinting strict mode #8832

Always return light mode in fingerprinting strict mode #8832

ShivanKaul commented May 17, 2021 •

edited

Loading

pes10k commented May 17, 2021

pilgrim-brave commented May 18, 2021

pilgrim-brave commented May 18, 2021

pilgrim-brave commented May 18, 2021

pilgrim-brave commented May 18, 2021

ShivanKaul commented May 19, 2021 •

edited

Loading

pes10k left a comment

ShivanKaul commented May 21, 2021 •

edited

Loading

pilgrim-brave commented May 21, 2021

bridiver May 24, 2021

bridiver left a comment

bridiver May 25, 2021

bridiver May 25, 2021

ShivanKaul May 26, 2021

bridiver May 25, 2021

ShivanKaul May 26, 2021

ShivanKaul commented May 29, 2021

ShivanKaul commented Jun 1, 2021

Always return light mode in fingerprinting strict mode #8832

Always return light mode in fingerprinting strict mode #8832

Conversation

ShivanKaul commented May 17, 2021 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

pes10k commented May 17, 2021

pilgrim-brave commented May 18, 2021

pilgrim-brave commented May 18, 2021

pilgrim-brave commented May 18, 2021

pilgrim-brave commented May 18, 2021

ShivanKaul commented May 19, 2021 • edited Loading

pes10k left a comment

Choose a reason for hiding this comment

ShivanKaul commented May 21, 2021 • edited Loading

pilgrim-brave commented May 21, 2021

bridiver May 24, 2021

Choose a reason for hiding this comment

bridiver left a comment

Choose a reason for hiding this comment

bridiver May 25, 2021

Choose a reason for hiding this comment

bridiver May 25, 2021

Choose a reason for hiding this comment

ShivanKaul May 26, 2021

Choose a reason for hiding this comment

bridiver May 25, 2021

Choose a reason for hiding this comment

ShivanKaul May 26, 2021

Choose a reason for hiding this comment

ShivanKaul commented May 29, 2021

ShivanKaul commented Jun 1, 2021

ShivanKaul commented May 17, 2021 •

edited

Loading

ShivanKaul commented May 19, 2021 •

edited

Loading

ShivanKaul commented May 21, 2021 •

edited

Loading