add flag for first-party blocking in standard blocking mode #9673

antonok-edm · 2021-08-05T17:47:49Z

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:

Disable the new #brave-adblock-default-1p-blocking flag, then visit https://dev-pages.bravesoftware.com/filtering/network-requests.html and follow the directions, ensuring that the images are shown as described.

mkarolin

chromium_src changes ++

iefremov · 2021-08-13T17:25:51Z

components/brave_shields/common/features.cc

@@ -10,6 +10,10 @@
 namespace brave_shields {
 namespace features {

+// When disabled, Brave will allow first-party requests in "standard" blocking


I would rather rephrase it as "When enabled ..." :)

I think the "disabled" state is less obvious behavior to those unfamiliar with adblocking - I added a description for both.

iefremov · 2021-08-13T17:27:22Z

we need tests for this change

iefremov · 2021-08-13T17:31:44Z

browser/net/url_context.h

@@ -76,6 +76,7 @@ struct BraveRequestInfo {
  // TODO(iefremov): rename to shields_up.
  bool allow_brave_shields = true;
  bool allow_ads = false;
+  bool aggressive_blocking = false;


we need a comment for this

iefremov · 2021-08-13T17:36:12Z

browser/net/url_context.cc

@@ -138,6 +138,9 @@ std::shared_ptr<brave::BraveRequestInfo> BraveRequestInfo::MakeCTX(
      brave_shields::GetBraveShieldsEnabled(map, ctx->tab_origin);
  ctx->allow_ads = brave_shields::GetAdControlType(map, ctx->tab_origin) ==
                   brave_shields::ControlType::ALLOW;
+  ctx->aggressive_blocking =
+      brave_shields::GetCosmeticFilteringControlType(map, ctx->tab_origin) ==


this assignment is not really readable - I had to search through the source to understand that "aggressive" flag is stored as cosmetic filtering content setting, which we reuse here... So definitely needs a comment

added as well

iefremov · 2021-08-13T17:37:21Z

browser/net/brave_ad_block_tp_network_delegate_helper.cc

@@ -180,7 +180,7 @@ EngineFlags ShouldBlockRequestOnTaskRunner(

  SCOPED_UMA_HISTOGRAM_TIMER("Brave.Adblock.ShouldBlockRequest");
  g_brave_browser_process->ad_block_service()->ShouldStartRequest(
-      url_to_check, ctx->resource_type, source_host,
+      url_to_check, ctx->resource_type, source_host, ctx->aggressive_blocking,


I gather that at some point we can craft a struct with needed parameters, to avoid changing all of the signatures

iefremov · 2021-08-13T18:05:10Z

components/brave_shields/browser/ad_block_service.cc

+          brave_shields::features::kBraveAdblockDefault1pBlocking) ||
+      !SameDomainOrHost(
+          url,
+          url::Origin::CreateFromNormalizedTuple("https", tab_host.c_str(), 80),


no need for c_str()

what are our expectations if tab_host is empty? AFAIR this is the SW case?

also, just curious whether we want to skip the check for all of the subresource types? Or maybe just scripts/whatever else?

removed c_str()

this is the same logic that's passed to the adblock engine already. It should be possible to refactor it to only happen in a single place later, but at the moment it's not trivial.

I'll leave @pes10k to comment on the resource types we want to apply this to.

I see this change is outdated, so apologies if this is talking into the void, but i dont see any reason to not apply to all requests. The "what does shields block in what configurations" logic is already complicated as it is. I dont think we should make the decision tree more complicated unless there is a compelling reason

iefremov · 2021-08-13T18:07:53Z

components/brave_shields/browser/ad_block_service.cc

@@ -66,27 +66,36 @@ void AdBlockService::ShouldStartRequest(
    const GURL& url,
    blink::mojom::ResourceType resource_type,
    const std::string& tab_host,


maybe we could pass Origin here...

iefremov · 2021-08-13T18:16:15Z

components/brave_shields/browser/ad_block_service.cc

@@ -66,27 +66,36 @@ void AdBlockService::ShouldStartRequest(
    const GURL& url,
    blink::mojom::ResourceType resource_type,
    const std::string& tab_host,
+    bool aggressive_blocking,


@bridiver we have to refactor this hierarchy so that the facade method doesn't share its signature with general adblock / regional ablock / etc.

Here we add a parameter to the facade method, and that parameter is useless for all other methods

I agree a lot of refactoring is needed in general. Several PRs are in the works

iefremov · 2021-08-13T18:18:48Z

generally looks good, but we need some tests, since this code is a critical part of the browser

pes10k · 2021-08-13T18:23:41Z

FWIW, i added a QA test too: https://dev-pages.brave.software/filtering/network-requests.html

iefremov

Unblocking the merge since this looks urgent. Minors and tests can be a f/u

antonok-edm requested review from bridiver, petemill, iefremov, yrliou and pilgrim-brave August 5, 2021 17:47

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

antonok-edm self-assigned this Aug 5, 2021

pilgrim-brave approved these changes Aug 5, 2021

View reviewed changes

mkarolin approved these changes Aug 5, 2021

View reviewed changes

antonok-edm force-pushed the 1p-blocking-flag branch 3 times, most recently from 3729731 to 5c46bda Compare August 9, 2021 18:14

iefremov reviewed Aug 13, 2021

View reviewed changes

iefremov approved these changes Aug 14, 2021

View reviewed changes

antonok-edm added 2 commits August 14, 2021 18:56

add flag for first-party blocking in standard blocking mode

a6c790b

address review comments

d66d36a

antonok-edm force-pushed the 1p-blocking-flag branch from 5c46bda to d66d36a Compare August 15, 2021 05:40

antonok-edm merged commit a9f8e12 into master Aug 16, 2021

antonok-edm deleted the 1p-blocking-flag branch August 16, 2021 03:22

antonok-edm added this to the 1.30.x - Nightly milestone Aug 16, 2021

antonok-edm mentioned this pull request Aug 18, 2021

Prevent 1p requests from being CNAME uncloaked in standard shields mode #9784

Merged

24 tasks

antonok-edm mentioned this pull request Aug 24, 2021

YouTube pre-roll ads appearing in certain cases brave/brave-browser#17673

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add flag for first-party blocking in standard blocking mode #9673

add flag for first-party blocking in standard blocking mode #9673

antonok-edm commented Aug 5, 2021 •

edited

Loading

mkarolin left a comment

iefremov Aug 13, 2021

antonok-edm Aug 15, 2021

iefremov commented Aug 13, 2021

iefremov Aug 13, 2021

antonok-edm Aug 15, 2021

iefremov Aug 13, 2021

antonok-edm Aug 15, 2021

iefremov Aug 13, 2021

iefremov Aug 13, 2021

iefremov Aug 13, 2021

iefremov Aug 13, 2021

antonok-edm Aug 15, 2021

pes10k Aug 15, 2021

iefremov Aug 13, 2021

iefremov Aug 13, 2021

bridiver Aug 14, 2021

iefremov commented Aug 13, 2021

pes10k commented Aug 13, 2021

iefremov left a comment

add flag for first-party blocking in standard blocking mode #9673

add flag for first-party blocking in standard blocking mode #9673

Conversation

antonok-edm commented Aug 5, 2021 • edited Loading

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

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 commented Aug 13, 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

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

iefremov commented Aug 13, 2021

pes10k commented Aug 13, 2021

iefremov left a comment

Choose a reason for hiding this comment

antonok-edm commented Aug 5, 2021 •

edited

Loading