Pass ShieldsSettings to renderer via mojo. #25505

goodov · 2024-09-10T13:54:42Z

This PR introduces a mojo structure to pass Shields settings to the renderer. This is part of the work to support per-site farbling.

Resolves brave/brave-browser#41023

Submitter Checklist:

I confirm that no security/privacy review is needed and no other type of reviews are needed, or that I have requested them
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 wiki
- npm run presubmit wiki, 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:

The associated issue milestone is set to the smallest version that the
changes has landed on
All relevant documentation has been updated, for instance:

Test Plan:

cdesouza-chromium

LGTM % questions

cdesouza-chromium · 2024-09-12T12:35:39Z

browser/brave_content_browser_client.cc

+  if (shields_up) {
+    auto fingerprinting_type = brave_shields::GetFingerprintingControlType(
+        host_content_settings_map, url);
+    if (fingerprinting_type == ControlType::BLOCK) {


Shouldn't we be using a switch here to exhaust all possible options of this control type, and catch any potential cases slipping trough if a new key is added to ControlType.

cdesouza-chromium · 2024-09-12T12:40:20Z

browser/brave_shields/brave_shields_web_contents_observer.cc

+  if (shields_up) {
+    auto fingerprinting_type = brave_shields::GetFingerprintingControlType(
+        host_content_settings_map, primary_url);
+    if (fingerprinting_type == ControlType::BLOCK) {


atuchin-m · 2024-09-12T15:38:17Z

browser/brave_shields/brave_shields_web_contents_observer.cc

-          }
-        }
-      });
+  RenderFrameHost* rfh = navigation_handle->GetRenderFrameHost();


Looks like navigation_handle->HasCommitted() and !navigation_handle->IsSameDocument() should be checked

added navigation_handle->IsSameDocument() check.

navigation_handle->HasCommitted() doesn't make sense in ReadyToCommitNavigation

atuchin-m · 2024-09-12T15:40:40Z

browser/brave_shields/brave_shields_web_contents_observer.cc

-        GetBraveShieldsRemote(rfh)->SetReduceLanguageEnabled(
-            brave_shields::IsReduceLanguageEnabledForProfile(pref_service));
-      }
+  SendShieldsSettingsToFrame(rfh, nullptr);


Do we really need this taking into account we handle ReadyToCommitNavigation() ?

P.S. there is per-RFH structure called DocumentUserData that allows to avoid manual frame tracking.

Do we really need this taking into account we handle ReadyToCommitNavigation() ?

I wanted to keep most things without changes and then do a separate cleanup, but I guess it's fine to cleanup some obvious things right now.

browser/brave_content_browser_client.cc

bridiver · 2024-09-16T19:23:25Z

chromium_src/chrome/renderer/worker_content_settings_client.cc

+  if (const auto& shields_settings =                                           \
+          static_cast<content_settings::BraveContentSettingsAgentImpl*>(agent) \
+              ->shields_settings()) {                                          \
+    shields_settings_ = shields_settings->Clone();                             \


why isn't shields_settings part of RendererContentSettingRules?

I'm also a bit confused about why we'd have things in ContentSettingsAgent that aren't content settings like reduce langage and origins to allow scripts and then take something that is a content setting (farbling level) and treat it differently.

I'm in favor of something that gets the "effective" setting, but I'm not sure that ContentSettingsAgent is the right place for this and maybe we should create something new. There are a lot of problems with the farbling content setting (like using CONTENT_SETTINGS_DEFAULT as a distinct value) and the current issue for most settings that we always have to check shields in addition to the content setting. I've been thinking that similar to cookies we should have a real content setting (BRAVE_COOKIES) and an effective content setting (COOKIES) that gives you the value after applying shields.

These settings do make sense as part of WebContentSettingsClient, but I'm not a fan of adding this to ContentSettingsAgent/RendererContentSettingRules by adding a new mojo type for shield settings. If we were going to do something like this (instead of having an effective content setting) then I think it makes more sense to be a new mojo interface

why isn't shields_settings part of RendererContentSettingRules?

mainly because of the patches. This is not the usual ContentSettingsForOneType list that's just prefilled with values from HostContentSettingsMap. To support this as a part of RendererContentSettingRules we will need to add some patches into PageSpecificContentSettings, which we can avoid entirely.

I also need to use this structure for the proper workers farbling. Right now we only have this:

brave-core/chromium_src/third_party/blink/public/mojom/worker/worker_content_settings_proxy.mojom

Lines 9 to 14 in 0955815

interface WorkerContentSettingsProxy {

// Returns whether the worker is allowed access to privileged functions that

// could be used for fingerprinting.

[Sync]

GetBraveFarblingLevel() => (uint8 result);

};

I'm also a bit confused about why we'd have things in ContentSettingsAgent that aren't content settings like reduce langage and origins to allow scripts and then take something that is a content setting (farbling level) and treat it differently.

we may look into creating a separate BraveShieldsSettings class on the blink side, but for now I think it's acceptable to keep it as part of ContentSettingsAgent. It seems like too much plumbing will be required to introduce something like ContentSettingsAgent and wire it will all users in the renderer.

I'm in favor of something that gets the "effective" setting

yep, one of the end goals is to calculate the effective shields configuration before passing it to the renderer, but this is out of the scope of this PR currently.

It seems like too much plumbing will be required to introduce something like ContentSettingsAgent and wire it will all users in the renderer.

Supplement<ExecutionContext> should make it available anywhere that ContentSettingsAgent is available just like we have done in other places like BraveSessionCache? I'm fine following up with something like this and also #25505 (comment) but I think it's something we should do because this seems a bit out of place to me.

bridiver · 2024-09-16T19:25:31Z

browser/brave_shields/brave_shields_web_contents_observer.cc

+  rfh->GetRemoteAssociatedInterfaces()->GetInterface(&agent);
+  agent->SetShieldsSettings(brave_shields::mojom::ShieldsSettings::New(
+      farbling_level, allowed_scripts_,
+      brave_shields::IsReduceLanguageEnabledForProfile(pref_service)));


IsReduceLanguageEnabledForProfile seems like it belongs in BraveRendererUpdater ?

data passed by BraveRendererUpdater is not accessible by blink internals.

not directly, but it could be exposed through our own class just like we have done in other places

patches/chrome-renderer-worker_content_settings_client.cc.patch

github-actions · 2024-09-17T08:23:53Z

[puLL-Merge] - brave/brave-core@25505

Description

This PR modifies the handling of Brave Shields settings, particularly for worker contexts. It introduces a new ShieldsSettings struct to encapsulate various Brave Shields settings and updates the interfaces and implementations to use this new structure. The changes aim to improve the management and propagation of Brave Shields settings across different parts of the browser.

Changes

Changes

browser/brave_content_browser_client.cc & .h:
- Updated WorkerGetBraveFarblingLevel to WorkerGetBraveShieldSettings
- Now returns a ShieldsSettingsPtr instead of just a farbling level
browser/brave_shields/brave_shields_web_contents_observer.cc & .h:
- Removed individual setting methods like SetAllowScriptsFromOriginsOnce
- Introduced SendShieldsSettings method to send all settings at once
- Simplified navigation handling and removed per-frame remotes
chromium_src/chrome/renderer/worker_content_settings_client.cc & .h:
- Updated to use the new ShieldsSettings structure
- Implemented WorkerContentSettingsClient_BraveImpl to handle Brave-specific behavior
components/brave_shields/content/browser/brave_shields_util.cc & .h:
- Added GetFarblingLevel function to determine farbling level based on shields settings
components/brave_shields/core/common/:
- Added new shields_settings.mojom file defining FarblingLevel enum and ShieldsSettings struct
- Updated brave_shields.mojom to use the new ShieldsSettings
components/content_settings/renderer/brave_content_settings_agent_impl.cc & .h:
- Updated to use ShieldsSettings instead of individual settings
- Replaced SetAllowScriptsFromOriginsOnce and SetReduceLanguageEnabled with SetShieldsSettings
Various other files:
- Updated to accommodate the new ShieldsSettings structure and related changes

Possible Issues

The changes to worker content settings might affect the behavior of web workers in unexpected ways, especially regarding script execution permissions.
The transition to using ShieldsSettings could potentially introduce synchronization issues if not all parts of the codebase are updated consistently.

Security Hotspots

The handling of ShieldsSettings in WorkerContentSettingsClient_BraveImpl should be carefully reviewed to ensure it doesn't introduce any security vulnerabilities, especially regarding script execution permissions.
The GetBraveShieldsSettings method in BraveContentSettingsAgentImpl falls back to default settings when HasContentSettingsRules() is false. This fallback behavior should be verified to ensure it doesn't accidentally disable important security features.

github-actions bot assigned goodov Sep 10, 2024

goodov force-pushed the shields-settings branch 3 times, most recently from 45189f3 to f901d18 Compare September 11, 2024 14:34

goodov marked this pull request as ready for review September 12, 2024 12:17

goodov requested review from a team as code owners September 12, 2024 12:17

goodov requested a review from arthuredelstein September 12, 2024 12:17

cdesouza-chromium approved these changes Sep 12, 2024

View reviewed changes

atuchin-m reviewed Sep 12, 2024

View reviewed changes

github-actions bot added the puLL-Merge label Sep 13, 2024

github-actions bot reviewed Sep 13, 2024

View reviewed changes

browser/brave_content_browser_client.cc Show resolved Hide resolved

github-actions bot added the needs-security-review label Sep 13, 2024

github-actions bot assigned thypon Sep 13, 2024

goodov force-pushed the shields-settings branch from 31f115f to 3067fcf Compare September 13, 2024 10:07

goodov added 6 commits September 16, 2024 16:33

Pass ShieldsSettings to renderer via mojo.

364f8ee

Use switch to handle all ControlType values.

b36a04f

Rename origins_to_allow_scripts_once to origins_to_allow_scripts.

8b7cce6

Cleanup RenderFrameHost interaction during ShieldsSettings passing.

cc93b46

Extract GetFarblingLevel into a function.

8ae00d7

Use ShieldsSettings in WorkerContentSettingsClient.

dfe38fa

goodov force-pushed the shields-settings branch from 3067fcf to dfe38fa Compare September 16, 2024 10:48

goodov requested a review from iefremov September 16, 2024 12:48

bridiver reviewed Sep 16, 2024

View reviewed changes

patches/chrome-renderer-worker_content_settings_client.cc.patch Outdated Show resolved Hide resolved

Replace WorkerContentSettingsClient patches with subclass.

818fbe0

bridiver approved these changes Sep 17, 2024

View reviewed changes

goodov merged commit 1c48f00 into master Sep 18, 2024
19 checks passed

goodov deleted the shields-settings branch September 18, 2024 08:55

github-actions bot added this to the 1.72.x - Nightly milestone Sep 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pass ShieldsSettings to renderer via mojo. #25505

Pass ShieldsSettings to renderer via mojo. #25505

goodov commented Sep 10, 2024 •

edited

Loading

cdesouza-chromium left a comment

cdesouza-chromium Sep 12, 2024

cdesouza-chromium Sep 12, 2024

atuchin-m Sep 12, 2024

goodov Sep 13, 2024

atuchin-m Sep 12, 2024

atuchin-m Sep 12, 2024

goodov Sep 13, 2024 •

edited

Loading

bridiver Sep 16, 2024

bridiver Sep 16, 2024

bridiver Sep 16, 2024 •

edited

Loading

bridiver Sep 16, 2024 •

edited

Loading

goodov Sep 17, 2024

bridiver Sep 17, 2024

bridiver Sep 16, 2024

goodov Sep 17, 2024

bridiver Sep 17, 2024

github-actions bot commented Sep 17, 2024

Changes

	interface WorkerContentSettingsProxy {
	// Returns whether the worker is allowed access to privileged functions that
	// could be used for fingerprinting.
	[Sync]
	GetBraveFarblingLevel() => (uint8 result);
	};

Pass ShieldsSettings to renderer via mojo. #25505

Pass ShieldsSettings to renderer via mojo. #25505

Conversation

goodov commented Sep 10, 2024 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

cdesouza-chromium left a 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

goodov Sep 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bridiver Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

bridiver Sep 16, 2024 • edited Loading

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

github-actions bot commented Sep 17, 2024

Description

Changes

Possible Issues

Security Hotspots

goodov commented Sep 10, 2024 •

edited

Loading

goodov Sep 13, 2024 •

edited

Loading

bridiver Sep 16, 2024 •

edited

Loading

bridiver Sep 16, 2024 •

edited

Loading