procedural filtering by antonok-edm · Pull Request #24688 · brave/brave-core

antonok-edm · 2024-07-16T20:59:17Z

Resolves brave/brave-browser#16935

Please see self-review notes here before reviewing!

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:

In brave://settings/shields/filters, subscribe to https://antonok.com/tmp/procedural-filter-tests/test-extended-css-rules.txt.
Visit https://antonok.com/tmp/procedural-filter-tests/index.html
All of the following numbered test cases should pass:

1
2
3
6
7
8
9
20
21
24
26
28
29

Test notes

The tests are from https://testcases.agrd.dev/Filters/extended-css-rules/test-extended-css-rules, with slight modifications to disable autostart. Autostart must be disabled because Brave executes some cosmetic filters with batched operations for performance reasons. The test code runs immediately and synchronously, but those batched operations can occur after the page test logic already declares the tests failed.

Other failures on the test page are expected. Many of the test cases use AdGuard-specific syntax extensions or other features that are not currently supported by this initial implementation - we'll get to those in some followups 😄

brave-builds · 2024-07-16T21:57:36Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2024-07-16T23:20:05Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2024-07-17T00:56:09Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2024-07-18T18:40:01Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2024-07-18T21:52:24Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2024-07-26T00:01:56Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2024-07-26T02:29:49Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2024-08-01T04:34:07Z

A Storybook has been deployed to preview UI for the latest push

brave-builds · 2024-08-08T16:52:54Z

A Storybook has been deployed to preview UI for the latest push

bridiver · 2024-09-17T17:54:35Z

components/brave_shields/core/browser/ad_block_service_helper.cc

+// These filters are represented as JSON provided by adblock-rust. The format
+// is documented at:
+// https://docs.rs/adblock/latest/adblock/cosmetic_filter_cache/struct.ProceduralOrActionFilter.html
+void StripProceduralFilters(base::Value::Dict& resources) {


Why is this in a separate helper file? If this is just for testing, you should add a friend test so it can be a private method in the adblock service

or is this to make it accessible to ios?

We have two other unit-tested helper methods in there too, seemed like it fit with those.

I don't think we have a mockable version of AdBlockService just yet, moving it there would mean we need to use browsertests.

I guess this is fine for now, but I'm not a fan of exposing these as public methods. Can we at least move it into an internal target and restrict visibility? This should ideally be the case with any classes that shouldn't be used outside the component.

github-actions · 2024-09-17T20:55:21Z

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

Description

This PR introduces support for procedural cosmetic filtering in Brave's ad-blocking engine. It updates the adblock Rust library from version 0.8.12 to 0.9.0 and adds new functionality to handle procedural filters like :has-text(), :upward(), and others. The changes span across multiple components, including the core ad-blocking logic, content scripts, and test files.

Changes

Changes

browser/about_flags.cc:
- Added a new feature flag for procedural filtering.
browser/brave_shields/ad_block_service_browsertest.cc:
- Added new tests for procedural filtering functionality.
components/brave_shields/adblock/rs/Cargo.toml:
- Updated adblock dependency to version 0.9.0.
components/brave_shields/content/browser/ad_block_service.cc:
- Modified UrlCosmeticResources to handle procedural filters.
components/brave_shields/core/browser/internal/ad_block_service_helper.cc:
- Added StripProceduralFilters function to handle standard blocking mode.
components/cosmetic_filters/browser/cosmetic_filters_resources.cc:
- Updated to handle procedural actions in cosmetic resources.
components/cosmetic_filters/renderer/cosmetic_filters_js_handler.cc:
- Modified to support procedural filtering in the renderer.
components/cosmetic_filters/resources/data/content_cosmetic.ts:
- Significant updates to support procedural filtering in content scripts.
components/cosmetic_filters/resources/data/procedural_filters.ts:
- Added new file to implement procedural filtering logic in JavaScript.
ios/brave-ios/:
- Various updates to support procedural filtering on iOS.
test/data/cosmetic_filtering.html:
- Added test cases for procedural filtering.

Possible Issues

Performance impact of procedural filtering, especially on large web pages or with complex filters.
Potential for increased memory usage due to the additional JavaScript logic for procedural filters.
Compatibility issues with existing content scripts or extensions that may interact with the new filtering logic.

Security Hotspots

components/cosmetic_filters/resources/data/procedural_filters.ts:
- The new procedural filtering logic introduces complex DOM manipulation. Ensure proper sanitization and error handling to prevent potential XSS vulnerabilities.
components/cosmetic_filters/renderer/cosmetic_filters_js_handler.cc:
- The changes in script injection and evaluation could potentially expose new attack surfaces. Careful review of the script execution context and permissions is necessary.
components/brave_shields/content/browser/ad_block_service.cc:
- The modification of cosmetic resources to include procedural actions should be carefully reviewed to ensure no malicious filters can be injected.

Overall, this PR significantly enhances Brave's ad-blocking capabilities but introduces complexity that requires careful testing and security review, especially around the new JavaScript-based procedural filtering logic.

atuchin-m · 2024-09-18T20:11:54Z

components/cosmetic_filters/resources/data/content_cosmetic.ts

 // - for cosmetic filters work with CSS and stylesheet. That work itself
 //   could call the script several times.

+import { applyCompiledSelector, compileProceduralSelector } from './procedural_filters'


@antonok-edm @bridiver
This results in significant increasing the JS inject size to every frame.

I hope you estimated the extra loading delay somehow. Could you share the numbers?

@atuchin-m to make sure I understand, there's an increase in load times? Does that happen on every page, or only pages with an active filter rule that uses a procedural filter? And does it happen in Standard mode, or also Aggressive mode?

@ShivanKaul
It add an overhead to any page load (we always inject the script if Shields aren't down)
The script size changed in 2x times:

I don't see any major changes in high-level metrics on the dashboard now, but every such change (+10k JS inject) makes Brave harder to compete against Chrome.

This reverts commit 121a325, reversing changes made to c1da125.

This PR unfortunately has broken `gnrt` runs on `master`. This reverts commit 121a325, reversing changes made to c1da125.

antonok-edm self-assigned this Jul 16, 2024

github-actions bot added CI/run-audit-deps Check for known npm/cargo vulnerabilities (audit_deps) CI/storybook-url Deploy storybook and provide a unique URL for each build labels Jul 16, 2024

antonok-edm force-pushed the procedural-filtering branch from bdcdf39 to 802bf11 Compare July 16, 2024 21:48

antonok-edm force-pushed the procedural-filtering branch from d31ba6a to b4c8b5f Compare July 18, 2024 17:38

antonok-edm added CI/skip-android Do not run CI builds for Android CI/skip-macos-x64 CI/skip-ios Do not run CI builds for iOS CI/skip-windows-x64 Do not run CI builds for Windows x64 CI/skip-macos-arm64 Do not run CI builds for macOS arm64 labels Jul 18, 2024

antonok-edm force-pushed the procedural-filtering branch 2 times, most recently from 8881250 to de95d87 Compare July 25, 2024 23:07

antonok-edm removed CI/skip-android Do not run CI builds for Android CI/skip-macos-x64 CI/skip-ios Do not run CI builds for iOS CI/skip-windows-x64 Do not run CI builds for Windows x64 CI/skip-macos-arm64 Do not run CI builds for macOS arm64 labels Jul 26, 2024

antonok-edm force-pushed the procedural-filtering branch from de95d87 to f5ba907 Compare July 26, 2024 01:34

antonok-edm force-pushed the procedural-filtering branch from f5ba907 to 6da2aa6 Compare August 1, 2024 03:39

cuba mentioned this pull request Aug 8, 2024

[iOS] Support for :remove/:remove-attr/:remove-class brave/brave-browser#38381

Closed

antonok-edm force-pushed the procedural-filtering branch from 1d3a3c3 to 7a7b371 Compare August 15, 2024 03:53

antonok-edm added 2 commits September 16, 2024 21:11

add unit tests for stripping out procedural filters

3605b4e

use a const for the procedural_actions key

ae695ee

antonok-edm force-pushed the procedural-filtering branch from fed2070 to ae695ee Compare September 17, 2024 04:17

antonok-edm requested a review from bridiver September 17, 2024 04:19

bridiver reviewed Sep 17, 2024

View reviewed changes

antonok-edm requested a review from bridiver September 17, 2024 18:26

antonok-edm requested a review from iefremov as a code owner September 17, 2024 20:54

github-actions bot added the CI/run-network-audit Run network-audit label Sep 17, 2024

antonok-edm force-pushed the procedural-filtering branch 5 times, most recently from 757454e to bfa38f0 Compare September 17, 2024 21:48

antonok-edm requested a review from a team as a code owner September 17, 2024 21:48

bridiver approved these changes Sep 17, 2024

View reviewed changes

move MergeResourcesInto and StripProceduralFilters to AdBlockService

5f555c9

antonok-edm force-pushed the procedural-filtering branch from bfa38f0 to 5f555c9 Compare September 17, 2024 22:30

antonok-edm enabled auto-merge September 17, 2024 22:57

antonok-edm merged commit 121a325 into master Sep 18, 2024

antonok-edm deleted the procedural-filtering branch September 18, 2024 00:30

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

This was referenced Sep 18, 2024

Facebook Ad Content Evades Aggressive Mode brave/brave-browser#15869

Closed

Anti-adblock on washingtonpost.com brave/brave-browser#21212

Closed

atuchin-m reviewed Sep 18, 2024

View reviewed changes

cdesouza-chromium added a commit that referenced this pull request Sep 18, 2024

Revert "Merge pull request #24688 from brave/procedural-filtering"

b39ce1b

This reverts commit 121a325, reversing changes made to c1da125.

cdesouza-chromium added a commit that referenced this pull request Sep 18, 2024

Revert "Merge pull request #24688 from brave/procedural-filtering"

46fea41

This PR unfortunately has broken `gnrt` runs on `master`. This reverts commit 121a325, reversing changes made to c1da125.

antonok-edm mentioned this pull request Sep 19, 2024

Add .cargo-checksum.json to adblock-0.9.0 #25641

Merged

24 tasks

Uni-verse mentioned this pull request Nov 1, 2024

Support procedural cosmetic filtering brave/brave-browser#16935

Closed

StephenHeaps mentioned this pull request Jan 13, 2025

Reuse procedural_filters.ts for ios #25694

Merged

24 tasks

Conversation

antonok-edm commented Jul 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

Test notes

Uh oh!

brave-builds commented Jul 16, 2024

Uh oh!

brave-builds commented Jul 16, 2024

Uh oh!

brave-builds commented Jul 17, 2024

Uh oh!

brave-builds commented Jul 18, 2024

Uh oh!

brave-builds commented Jul 18, 2024

Uh oh!

brave-builds commented Jul 26, 2024

Uh oh!

brave-builds commented Jul 26, 2024

Uh oh!

brave-builds commented Aug 1, 2024

Uh oh!

brave-builds commented Aug 8, 2024

Uh oh!

bridiver Sep 17, 2024

Choose a reason for hiding this comment

Uh oh!

bridiver Sep 17, 2024

Choose a reason for hiding this comment

Uh oh!

antonok-edm Sep 17, 2024

Choose a reason for hiding this comment

Uh oh!

bridiver Sep 17, 2024

Choose a reason for hiding this comment

Uh oh!

antonok-edm Sep 17, 2024

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Sep 17, 2024

Description

Changes

Possible Issues

Security Hotspots

Uh oh!

atuchin-m Sep 18, 2024

Choose a reason for hiding this comment

Uh oh!

ShivanKaul Sep 18, 2024

Choose a reason for hiding this comment

Uh oh!

atuchin-m Sep 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

antonok-edm commented Jul 16, 2024 •

edited

Loading

atuchin-m Sep 19, 2024 •

edited

Loading