Support `removeparam` #15943

antonok-edm · 2022-11-15T01:03:43Z

Resolves brave/brave-browser#23927

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

Save the following document as index.html and serve it using your preferred method (e.g. python -m http.server).

<html>
    <head></head>
    <body>
        <div id="qstring"></div>
        <a href="/?test2=false">/?test2=false</a>
        <a href="/?test=true">/?test=true</a>
        <a href="/?test=true&test2=false">/?test=true&test2=false</a>
        <script>
            const e = document.getElementById('qstring');
            e.innerText = document.location.search;
        </script>
    </body>
</html>

Add a custom rule to brave://settings/shields/filters corresponding to the location the HTML document is served from. For example, localhost:8000^$document,removeparam=test (modify localhost:8000 as necessary based on your hosting endpoint). Save the custom rules.

Open the page you served in a new tab of the browser. There are 3 links on the page. Click on each one in order and observe the following query string in the URL bar, as well as on the first line of the page:

?test2=false
(empty)
?test2=false

fmarier · 2022-11-15T01:11:00Z

Is this meant to replace the existing query filter? If so, we should make sure it passes the existing browser tests: https://github.com/brave/brave-core/blob/c7a229c7eb8a0c2ad4d2d799c3a7eac2a19da541/browser/net/brave_site_hacks_network_delegate_helper_browsertest.cc

fmarier · 2022-11-15T01:11:33Z

@antonok-edm Can you point to the PR implementing the URL rewriting on the Rust side?

antonok-edm · 2022-11-15T01:51:35Z

@fmarier it's currently in brave/adblock-rust#235

(I will merge that and set the Cargo.toml entry to the official release before merging this PR)

components/adblock_rust_ffi/src/lib.rs

components/adblock_rust_ffi/Cargo.toml

ios/browser/api/brave_shields/adblock_engine.mm

ios/browser/api/brave_shields/adblock_engine.h

fmarier · 2022-11-17T01:21:06Z

Is this meant to replace the existing query filter?

Just noting that the answer for this PR is no. This is adding support for custom lists and rules in order to get parity with uBlock Origin.

Using this support to migrate the existing query string filter to a list will be done in a separate PR.

iefremov · 2022-11-17T11:07:53Z

components/brave_shields/browser/ad_block_service.h

@@ -110,7 +110,8 @@ class AdBlockService {
                          bool* did_match_rule,
                          bool* did_match_exception,
                          bool* did_match_important,
-                          std::string* mock_data_url);
+                          std::string* mock_data_url,
+                          std::string* rewritten_url);


i think we should stop adding new params to this method and introduce a structure instead. Otherwise, it goes out of control - there is no single line of comment describing what is for what, and every new change touches a dozen of irrelevant files

cc @bridiver

Ideally we just need brave/brave-browser#5461, which I'm hoping to get to soon:tm:

or definitely at least before adding any further arguments to these methods

iefremov · 2022-11-17T12:19:38Z

components/brave_shields/browser/domain_block_navigation_throttle.cc

+    CancelDeferredNavigation(content::NavigationThrottle::ThrottleCheckResult(
+        content::NavigationThrottle::CANCEL));
+
+    const GURL new_url(new_url_spec);


need to check if the url is valid

iefremov · 2022-11-17T12:22:09Z

browser/net/brave_ad_block_tp_network_delegate_helper.cc

+
+  if (ctx->method == "GET" || ctx->method == "HEAD" ||
+      ctx->method == "OPTIONS") {
+    ctx->new_url_spec = rewritten_url;


check validity of the url?

Would it also be worth checking that the scheme is HTTP/HTTPS?

updated

@fmarier I think we want it to be able to apply to WS/WSS as well. But anyways, unsupported schemes are already not even passed to the adblock engine so it should be alright here.

iefremov · 2022-11-30T18:33:19Z

components/brave_shields/browser/domain_block_navigation_throttle.cc

+    // technically this is a new navigation
+    params.redirect_chain.clear();
+
+    base::SequencedTaskRunnerHandle::Get()->PostTask(


this approach to filtering is potentially suboptimal, but we can keep it as is as long the filter list consists of really small set of entries

@iefremov What did you have in mind in term of a "really small set of entries"?

Here's an example list a user could add: https://github.com/AdguardTeam/FiltersRegistry/blob/master/filters/filter_17_TrackParam/filter.txt

See brave/brave-browser#26377 (comment)

kjozwiak · 2022-12-09T08:03:58Z

Verification PASSED on Win 11 x64 using the following build(s):

Brave | 1.48.40 Chromium: 108.0.5359.99 (Official Build) nightly (64-bit)
-- | --
Revision | 410951fc34bb4b2cbf182231f9f779efaafaf682-refs/branch-heads/5359_71@{#9}
OS | Windows 11 Version 22H2 (Build 22621.900)

Using the STR/Cases outlined via #15943 (comment), went through the following:

Setup http://localhost:8000 & added custom filter

`Example`	`Example`

Clicking on links via http://localhost:8000/index.html

`Link #1 (?test2=false)`	`Link #2 (Blank)`	`Link #3 (?test2=false)`

antonok-edm requested a review from fmarier November 15, 2022 01:03

antonok-edm requested review from iefremov and a team as code owners November 15, 2022 01:03

antonok-edm self-assigned this Nov 15, 2022

github-actions bot added CI/run-audit-deps Check for known npm/cargo vulnerabilities (audit_deps) CI/run-network-audit Run network-audit labels Nov 15, 2022

antonok-edm mentioned this pull request Nov 15, 2022

Move the query string filter to a proper list brave/brave-browser#10188

Open

antonok-edm force-pushed the removeparam branch from 1628fce to 936cd62 Compare November 15, 2022 01:36

rillian reviewed Nov 15, 2022

View reviewed changes

components/adblock_rust_ffi/src/lib.rs Outdated Show resolved Hide resolved

rillian reviewed Nov 15, 2022

View reviewed changes

components/adblock_rust_ffi/Cargo.toml Outdated Show resolved Hide resolved

antonok-edm requested a review from a team as a code owner November 15, 2022 09:32

rillian approved these changes Nov 15, 2022

View reviewed changes

antonok-edm force-pushed the removeparam branch from 87a8533 to 825902f Compare November 15, 2022 19:41

kylehickinson requested changes Nov 15, 2022

View reviewed changes

ios/browser/api/brave_shields/adblock_engine.mm Show resolved Hide resolved

antonok-edm force-pushed the removeparam branch from 825902f to 999adc1 Compare November 16, 2022 01:51

antonok-edm requested a review from kylehickinson November 16, 2022 01:52

kylehickinson requested changes Nov 16, 2022

View reviewed changes

ios/browser/api/brave_shields/adblock_engine.h Outdated Show resolved Hide resolved

antonok-edm force-pushed the removeparam branch from 999adc1 to 80ac71f Compare November 16, 2022 19:43

antonok-edm requested a review from kylehickinson November 16, 2022 19:44

kylehickinson approved these changes Nov 16, 2022

View reviewed changes

antonok-edm force-pushed the removeparam branch 3 times, most recently from 961843f to 04b2956 Compare November 16, 2022 22:45

fmarier approved these changes Nov 17, 2022

View reviewed changes

iefremov reviewed Nov 17, 2022

View reviewed changes

antonok-edm force-pushed the removeparam branch from 04b2956 to aa1bf3c Compare November 17, 2022 19:53

antonok-edm requested a review from iefremov November 17, 2022 19:55

iefremov reviewed Nov 30, 2022

View reviewed changes

iefremov approved these changes Nov 30, 2022

View reviewed changes

antonok-edm added 5 commits November 30, 2022 15:10

support removeparam

e4ca144

check/apply removeparam in domain blocking throttle

c0f2abe

add tests

49cce79

restrict query parameter stripping to GET, HEAD, and OPTIONS requests

f933b42

See brave/brave-browser#26377 (comment)

ensure URLs are valid

b1f03d4

antonok-edm force-pushed the removeparam branch from aa1bf3c to b1f03d4 Compare December 1, 2022 02:21

antonok-edm enabled auto-merge December 1, 2022 02:56

antonok-edm merged commit 523f4fd into master Dec 1, 2022

antonok-edm deleted the removeparam branch December 1, 2022 04:37

github-actions bot added this to the 1.48.x - Nightly milestone Dec 1, 2022

brave-builds pushed a commit that referenced this pull request Dec 6, 2022

Uplift of #15943 (squashed) to beta

775b255

brave-builds mentioned this pull request Dec 6, 2022

Support removeparam (uplift to 1.47.x) #16268

Merged

7 tasks

stephendonner mentioned this pull request Dec 11, 2022

Add support for $removeparam filter syntax brave/brave-browser#23927

Closed

This was referenced Jan 12, 2023

Fix segfault in DomainBlockNavigationThrottle brave/brave-browser#27791

Closed

Call CancelDeferredNavigation after finishing with the NavigationHandle #16654

Merged

fmarier mentioned this pull request Aug 23, 2023

#brave-domain-block-1pes regressed query filter brave/brave-browser#32462

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support `removeparam` #15943

Support `removeparam` #15943

antonok-edm commented Nov 15, 2022

fmarier commented Nov 15, 2022

fmarier commented Nov 15, 2022

antonok-edm commented Nov 15, 2022 •

edited

Loading

fmarier commented Nov 17, 2022

iefremov Nov 17, 2022

iefremov Nov 17, 2022

antonok-edm Nov 17, 2022

iefremov Nov 17, 2022

antonok-edm Nov 17, 2022

iefremov Nov 17, 2022

fmarier Nov 17, 2022

antonok-edm Nov 17, 2022 •

edited

Loading

iefremov Nov 30, 2022

fmarier Nov 30, 2022

kjozwiak commented Dec 9, 2022

Support removeparam #15943

Support removeparam #15943

Conversation

antonok-edm commented Nov 15, 2022

Submitter Checklist:

Reviewer Checklist:

After-merge Checklist:

Test Plan:

fmarier commented Nov 15, 2022

fmarier commented Nov 15, 2022

antonok-edm commented Nov 15, 2022 • edited Loading

fmarier commented Nov 17, 2022

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

antonok-edm Nov 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kjozwiak commented Dec 9, 2022

Support `removeparam` #15943

Support `removeparam` #15943

antonok-edm commented Nov 15, 2022 •

edited

Loading

antonok-edm Nov 17, 2022 •

edited

Loading