Update adblock-rust with CSP rule support #8281

antonok-edm · 2021-03-18T00:24:29Z

Resolves brave/brave-browser#14792

Security/privacy review at https://github.com/brave/security/issues/374

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:

Visit http://pirateiro.com/. Refresh the page while the developer console is open, and check in the console to verify that the browser refused to load scripts violating the policy script-src 'self' 'unsafe-inline' https://hcaptcha.com *.hcaptcha.com.

On Android, it will be sufficient to observe 0 blocked items in the Shields panel rather than 5 (the requests are still blocked, but will not increment the counter).

antonok-edm · 2021-03-18T02:49:43Z

There are currently some lint errors due to brave/brave-browser#14803.

I will get that sorted out first and rebase before merging, but this is ready for review otherwise.

browser/net/brave_ad_block_tp_network_delegate_helper.cc

antonok-edm · 2021-04-08T20:27:11Z

browser/net/brave_ad_block_tp_network_delegate_helper.cc

+    // If the override_response_headers have already been populated, we should
+    // use those directly.  Otherwise, we populate them from the original
+    // headers.
+    if (!*override_response_headers) {
+      *override_response_headers =
+          new net::HttpResponseHeaders(response_headers->raw_headers());
+    }


@iefremov it was confusing to me at first that override_response_headers is not reset in between OnHeadersReceived callbacks. The other callbacks are written to override the headers directly from the original response headers, which is okay when they're the first/only callback but problematic for any further callbacks.

Based on how this is all set up, I would have expected BraveRequestHandler to update response_headers with any non-null override_response_headers in between each callback. Just wanted to point this out in case it's unintentional or needs further work.

yeah this probably can be improved

components/brave_shields/browser/ad_block_base_service.h

iefremov · 2021-04-12T09:14:05Z

browser/net/brave_ad_block_tp_network_delegate_helper.h

@@ -16,6 +16,13 @@ int OnBeforeURLRequest_AdBlockTPPreWork(
    const ResponseCallback& next_callback,
    std::shared_ptr<BraveRequestInfo> ctx);

+int OnHeadersReceived_AdBlockCspWork(


can we please move this to a separate file and also wrap with a base::Feature flag

FWIW I was modeling this off of ipfs_redirect_network_delegate_helper.h which has both the OnBeforeURLRequest and OnHeadersReceived callbacks in the same file.

components/brave_shields/browser/csp_merge_unittest.cc

mkarolin · 2021-04-12T18:07:40Z

browser/net/brave_request_handler.cc

@@ -21,6 +22,7 @@
 #include "brave/common/pref_names.h"
 #include "brave/components/brave_referrals/buildflags/buildflags.h"
 #include "brave/components/brave_rewards/browser/buildflags/buildflags.h"
+#include "brave/components/brave_shields/common/features.h"


Needs a dep in browser/net/BUILD.gn?

mkarolin

chromium_src changes LGTM

components/brave_shields/common/features.cc

components/brave_shields/browser/ad_block_service_helper.cc

iefremov · 2021-04-13T08:24:13Z

components/brave_shields/browser/ad_block_service_helper.h

@@ -25,6 +25,9 @@ std::vector<adblock::FilterList>::const_iterator FindAdBlockFilterListByLocale(
 std::vector<adblock::FilterList> RegionalCatalogFromJSON(
    const std::string& catalog_json);

+void MergeCspDirectiveInto(base::Optional<std::string> from,


personally I don't see much sense in using Optional here and in the service (also CC recommends against it https://chromium.googlesource.com/chromium/src/+/master/docs/optional.md#when-not-to-use).
IMO just (potentially, empty) strings would be more readable.

I think I disagree here. CSP is a security-sensitive topic, and it's important to make sure these directives are handled with appropriate care. Having the Optional enforces a stronger contract than just passing strings around, with a distinct semantic difference between "there is a directive here" and "no directive". I think it'd be tempting to bypass the merge function here altogether if working directly with strings; we have already run into some issues with incorrectly concatenated CSP directives in the past (e.g. brave/brave-browser#14752).

iefremov · 2021-04-13T08:39:57Z

components/brave_shields/browser/ad_block_base_service.cc

+  // a URL or origin and not a string to a host name.
+  bool is_third_party = !SameDomainOrHost(
+      url,
+      url::Origin::CreateFromNormalizedTuple("https", tab_host.c_str(), 80),


you should std::move(tab_host), as described in the CreateFromNormalizedTuple description

why not pass the full URL or Origin there from the very beginning?

I'm just using the same implementation as ShouldStartRequest directly above; would be good to keep the same behavior between the two since I use both the same way in adblock-rust. I think it's worth addressing in a followup.

yeah, ok - could you file a follow-up to change tab_host from string to GURL in ShouldStartRequest ?

Filed brave/brave-browser#15291

browser/net/brave_ad_block_csp_network_delegate_helper.h

iefremov · 2021-04-13T10:21:17Z

browser/net/brave_ad_block_csp_network_delegate_helper.cc

+
+    (*override_response_headers)->RemoveHeader("Content-Security-Policy");
+
+    task_runner->PostTaskAndReplyWithResult(


I don't really like the idea of adding new thread hops, but we probably can leave it for now until bigger refactoring. I think at some point we should allow querying certain things (e.g. csp rules) from the service on UI thread to avoid jumping between threads, since the jump adds more delay than the lookup

antonok-edm added the enhancement label Mar 18, 2021

antonok-edm requested a review from iefremov as a code owner March 18, 2021 00:24

antonok-edm self-assigned this Mar 18, 2021

antonok-edm requested review from bbondy and emerick March 18, 2021 00:27

antonok-edm force-pushed the csp-filter-rules branch 4 times, most recently from 1e99555 to 814ec95 Compare March 18, 2021 02:28

iefremov reviewed Mar 19, 2021

View reviewed changes

browser/net/brave_ad_block_tp_network_delegate_helper.cc Outdated Show resolved Hide resolved

antonok-edm force-pushed the csp-filter-rules branch 2 times, most recently from 12dec05 to e475c29 Compare March 23, 2021 03:45

antonok-edm requested a review from iefremov March 30, 2021 18:32

antonok-edm force-pushed the csp-filter-rules branch 2 times, most recently from 30dc53d to 86154a9 Compare April 8, 2021 19:11

antonok-edm commented Apr 8, 2021

View reviewed changes

iefremov reviewed Apr 12, 2021

View reviewed changes

components/brave_shields/browser/ad_block_base_service.h Outdated Show resolved Hide resolved

iefremov reviewed Apr 12, 2021

View reviewed changes

components/brave_shields/browser/csp_merge_unittest.cc Outdated Show resolved Hide resolved

iefremov reviewed Apr 12, 2021

View reviewed changes

components/brave_shields/browser/csp_merge_unittest.cc Show resolved Hide resolved

iefremov reviewed Apr 12, 2021

View reviewed changes

components/brave_shields/browser/csp_merge_unittest.cc Show resolved Hide resolved

antonok-edm force-pushed the csp-filter-rules branch from 86154a9 to 045bd4f Compare April 12, 2021 17:47

antonok-edm requested a review from a team as a code owner April 12, 2021 17:47

antonok-edm requested a review from iefremov April 12, 2021 17:47

mkarolin reviewed Apr 12, 2021

View reviewed changes

mkarolin approved these changes Apr 12, 2021

View reviewed changes

antonok-edm force-pushed the csp-filter-rules branch from 045bd4f to 4e9f881 Compare April 12, 2021 18:12

iefremov reviewed Apr 13, 2021

View reviewed changes

components/brave_shields/common/features.cc Outdated Show resolved Hide resolved

iefremov reviewed Apr 13, 2021

View reviewed changes

components/brave_shields/browser/ad_block_service_helper.cc Show resolved Hide resolved

iefremov reviewed Apr 13, 2021

View reviewed changes

browser/net/brave_ad_block_csp_network_delegate_helper.h Outdated Show resolved Hide resolved

iefremov reviewed Apr 13, 2021

View reviewed changes

antonok-edm added 4 commits April 13, 2021 12:24

Update adblock-rust with CSP rule support

d0d845b

correct interaction with previously overridden headers

3eeb2c9

move CSP work to separate file

0cdcb68

add feature flag to control CSP rule application

01b3e0a

antonok-edm force-pushed the csp-filter-rules branch from 4e9f881 to 01b3e0a Compare April 13, 2021 22:22

antonok-edm requested a review from iefremov April 13, 2021 22:23

iefremov approved these changes Apr 14, 2021

View reviewed changes

antonok-edm merged commit a27e101 into master Apr 14, 2021

antonok-edm deleted the csp-filter-rules branch April 14, 2021 15:07

antonok-edm mentioned this pull request Apr 14, 2021

Change tab_host from string to GURL in AdBlockBaseService::ShouldStartRequest brave/brave-browser#15291

Open

antonok-edm added this to the 1.25.x - Nightly milestone Apr 14, 2021

btlechowski mentioned this pull request Apr 15, 2021

Add support for $csp filter rules brave/brave-browser#14792

Closed

antonok-edm mentioned this pull request Apr 23, 2021

Don't pass async override_response_headers ptr #8619

Merged

antonok-edm mentioned this pull request Jun 8, 2021

Tie CSP rule injection to Shields ads/tracker setting #9053

Merged

24 tasks

srirambv mentioned this pull request Jun 11, 2021

Fix $csp rules being applied even when Shields are down brave/brave-browser#16283

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update adblock-rust with CSP rule support #8281

Update adblock-rust with CSP rule support #8281

antonok-edm commented Mar 18, 2021 •

edited

Loading

antonok-edm commented Mar 18, 2021

antonok-edm Apr 8, 2021

iefremov Apr 12, 2021

iefremov Apr 12, 2021

antonok-edm Apr 12, 2021

mkarolin Apr 12, 2021

mkarolin left a comment

iefremov Apr 13, 2021

antonok-edm Apr 13, 2021

iefremov Apr 13, 2021 •

edited

Loading

antonok-edm Apr 13, 2021

iefremov Apr 14, 2021

antonok-edm Apr 14, 2021

iefremov Apr 13, 2021


		(*override_response_headers)->RemoveHeader("Content-Security-Policy");

		task_runner->PostTaskAndReplyWithResult(

Update adblock-rust with CSP rule support #8281

Update adblock-rust with CSP rule support #8281

Conversation

antonok-edm commented Mar 18, 2021 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

antonok-edm commented Mar 18, 2021

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

mkarolin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iefremov Apr 13, 2021 • 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

antonok-edm commented Mar 18, 2021 •

edited

Loading

iefremov Apr 13, 2021 •

edited

Loading