AG-30810: save $popup rules into separate field inside MatchingResult

Merge in ADGUARD-FILTERS/tsurlfilter from fix/AG-30810-2 to master Squashed commit of the following: commit a16b77c Merge: 4466b18 ecdc49a Author: Dmitriy Seregin <[email protected]> Date: Thu Mar 28 14:47:11 2024 +0300 Merge branch 'master' into fix/AG-30810-2 commit 4466b18 Author: Dmitriy Seregin <[email protected]> Date: Mon Mar 25 22:28:01 2024 +0300 changes commit 5c67fb7 Merge: 40bff70 459fa5f Author: Dmitriy Seregin <[email protected]> Date: Mon Mar 25 22:27:57 2024 +0300 Merge branch 'master' into fix/AG-30810-2 commit 40bff70 Author: Dmitriy Seregin <[email protected]> Date: Mon Mar 25 22:26:51 2024 +0300 fixes commit 2a5e41b Author: Dmitriy Seregin <[email protected]> Date: Mon Mar 25 15:03:07 2024 +0300 fixed tests, remove garbage commit 3a865c5 Author: Dmitriy Seregin <[email protected]> Date: Thu Mar 14 14:50:35 2024 +0300 changes commit 5e64655 Author: Maxim Topciu <[email protected]> Date: Thu Mar 14 05:05:13 2024 +0200 AG-30810 fix tests commit b31d220 Author: Maxim Topciu <[email protected]> Date: Wed Mar 13 19:26:30 2024 +0200 AG-30810 add one more testcase commit 9de1f45 Author: Dmitriy Seregin <[email protected]> Date: Wed Mar 13 17:16:37 2024 +0300 changes commit 6a1eea7 Author: Dmitriy Seregin <[email protected]> Date: Wed Mar 13 00:47:08 2024 +0300 added changelogs commit 9554b2c Author: Dmitriy Seregin <[email protected]> Date: Wed Mar 13 00:41:55 2024 +0300 AG-30810: save $popup rules into separate field inside MatchingResult
AdguardTeam · Mar 28, 2024 · dec373b · dec373b
1 parent ecdc49a
commit dec373b
Show file tree

Hide file tree

Showing 11 changed files with 257 additions and 103 deletions.
diff --git a/packages/tsurlfilter/CHANGELOG.md b/packages/tsurlfilter/CHANGELOG.md
@@ -8,6 +8,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 <!-- TODO: manually add compare links for version changes -->
 <!-- e.g. [1.0.77]: https://github.com/AdguardTeam/tsurlfilter/compare/tsurlfilter-v1.0.76...tsurlfilter-v1.0.77 -->
 
+## [2.2.16] - 2024-03-26
+
+### Fixed
+
+- Correct work `$all` and `$popup` when they both selected for request [#2620],
+  [#2728].
+
+[2.2.16]: https://github.com/AdguardTeam/tsurlfilter/releases/tag/tsurlfilter-v2.2.16
+[#2620]: https://github.com/AdguardTeam/AdguardBrowserExtension/issues/2620
+[#2728]: https://github.com/AdguardTeam/AdguardBrowserExtension/issues/2728
+
+
 ## [2.2.15] - 2024-03-01
 
 ### Changed

diff --git a/packages/tsurlfilter/package.json b/packages/tsurlfilter/package.json
@@ -1,6 +1,6 @@
 {
   "name": "@adguard/tsurlfilter",
-  "version": "2.2.15",
+  "version": "2.2.16",
   "description": "This is a TypeScript library that implements AdGuard's content blocking rules",
   "main": "dist/es/index.js",
   "module": "dist/es/index.js",

diff --git a/packages/tsurlfilter/src/engine/matching-result.ts b/packages/tsurlfilter/src/engine/matching-result.ts
@@ -2,6 +2,7 @@ import { NetworkRule, NetworkRuleOption } from '../rules/network-rule';
 import { CookieModifier } from '../modifiers/cookie-modifier';
 import { CosmeticOption } from './cosmetic-option';
 import { RedirectModifier } from '../modifiers/redirect-modifier';
+import { RequestType } from '../request-type';
 
 /**
  * MatchingResult contains all the rules matching a web request, and provides methods
@@ -71,6 +72,15 @@ export class MatchingResult {
      */
     public stealthRule: NetworkRule | null;
 
+    /**
+     * PopupRule - this is a rule that specified which way should be used
+     * to blocking document request: close the tab or open dummy blocking page.
+     * We should store it separately from other blocking rules, because $popup
+     * has an intersection by use cases with $all.
+     * https://kb.adguard.com/en/general/how-to-create-your-own-ad-filters#popup-modifier
+     */
+    public popupRule: NetworkRule | null;
+
     /**
      * Creates an instance of the MatchingResult struct and fills it with the rules.
      *
@@ -88,6 +98,7 @@ export class MatchingResult {
         this.removeHeaderRules = null;
         this.redirectRules = null;
         this.stealthRule = null;
+        this.popupRule = null;
 
         // eslint-disable-next-line no-param-reassign
         rules = MatchingResult.removeBadfilterRules(rules);
@@ -102,10 +113,9 @@ export class MatchingResult {
         // basic blocking rules are allowed by default
         let basicAllowed = true;
         if (this.documentRule) {
-            const documentRule = this.documentRule as NetworkRule;
-            if (documentRule.isOptionEnabled(NetworkRuleOption.Urlblock)) {
+            if (this.documentRule.isOptionEnabled(NetworkRuleOption.Urlblock)) {
                 basicAllowed = false;
-            } else if (documentRule.isOptionEnabled(NetworkRuleOption.Genericblock)) {
+            } else if (this.documentRule.isOptionEnabled(NetworkRuleOption.Genericblock)) {
                 genericAllowed = false;
             }
         }
@@ -158,6 +168,12 @@ export class MatchingResult {
                 this.stealthRule = rule;
                 continue;
             }
+            if (rule.isOptionEnabled(NetworkRuleOption.Popup)
+                // This check needed to split $all rules from $popup rules
+                && (rule.getPermittedRequestTypes() & RequestType.Document) !== RequestType.Document) {
+                this.popupRule = rule;
+                continue;
+            }
 
             // Check blocking rules against $genericblock / $urlblock
             if (!rule.isAllowlist() && this.documentRule?.isHigherPriority(rule)) {
@@ -224,6 +240,10 @@ export class MatchingResult {
             return redirectRule;
         }
 
+        if (!basic) {
+            return this.popupRule;
+        }
+
         return basic;
     }
 

diff --git a/packages/tsurlfilter/src/rules/network-rule.ts b/packages/tsurlfilter/src/rules/network-rule.ts
@@ -1348,7 +1348,6 @@ export class NetworkRule implements rule.IRule {
                 break;
             // $popup
             case OPTIONS.POPUP:
-                this.setRequestType(RequestType.Document, true);
                 this.setOptionEnabled(NetworkRuleOption.Popup, true);
                 break;
             // Content type options

diff --git a/packages/tsurlfilter/test/engine/engine.test.ts b/packages/tsurlfilter/test/engine/engine.test.ts
@@ -418,24 +418,28 @@ describe('TestEngineMatchRequest - popup modifier', () => {
         let result = engine.matchRequest(request);
         expect(result.getBasicResult()).not.toBeNull();
         expect(result.getBasicResult()!.getText()).toBe(blockingRuleText);
+        expect(result.popupRule!.getText()).toEqual(popupBlockingRuleText);
 
         // Tests matching a script request; expects to match the basic blocking rule
         request = new Request('http://example.org/', 'http://example.com/', RequestType.Script);
         result = engine.matchRequest(request);
         expect(result.getBasicResult()).not.toBeNull();
         expect(result.getBasicResult()!.getText()).toBe(blockingRuleText);
+        expect(result.popupRule!.getText()).toEqual(popupBlockingRuleText);
 
         // Tests matching an image request; expects to match the basic blocking rule
         request = new Request('http://example.org/', 'http://example.com/', RequestType.Image);
         result = engine.matchRequest(request);
         expect(result.getBasicResult()).not.toBeNull();
         expect(result.getBasicResult()!.getText()).toBe(blockingRuleText);
+        expect(result.popupRule!.getText()).toEqual(popupBlockingRuleText);
 
         // Tests matching a document request; expects to match the popup blocking rule
         request = new Request('http://example.org/', 'http://example.com/', RequestType.Document);
         result = engine.matchRequest(request);
         expect(result.getBasicResult()).not.toBeNull();
-        expect(result.getBasicResult()!.getText()).toBe(popupBlockingRuleText);
+        expect(result.getBasicResult()!.getText()).toBe(blockingRuleText);
+        expect(result.popupRule!.getText()).toBe(popupBlockingRuleText);
     });
 
     it('match requests against all and popup blocking rules', () => {
@@ -453,25 +457,28 @@ describe('TestEngineMatchRequest - popup modifier', () => {
         let result = engine.matchRequest(request);
         expect(result.getBasicResult()).not.toBeNull();
         expect(result.getBasicResult()!.getText()).toBe(blockingAllRuleText);
+        expect(result.popupRule!.getText()).toEqual(popupBlockingRuleText);
 
         // Tests matching a script request; expects to match the all-encompassing blocking rule
         request = new Request('http://example.org/', 'http://example.com/', RequestType.Script);
         result = engine.matchRequest(request);
         expect(result.getBasicResult()).not.toBeNull();
         expect(result.getBasicResult()!.getText()).toBe(blockingAllRuleText);
+        expect(result.popupRule!.getText()).toEqual(popupBlockingRuleText);
 
         // Tests matching an image request; expects to match the all-encompassing blocking rule
         request = new Request('http://example.org/', 'http://example.com/', RequestType.Image);
         result = engine.matchRequest(request);
         expect(result.getBasicResult()).not.toBeNull();
         expect(result.getBasicResult()!.getText()).toBe(blockingAllRuleText);
+        expect(result.popupRule!.getText()).toEqual(popupBlockingRuleText);
 
-        // TODO: In the future it can be $all modifier, if we make it's priority higher than $popup
         // Tests matching a document request; expects to match the popup blocking rule
         request = new Request('http://example.org/', 'http://example.com/', RequestType.Document);
         result = engine.matchRequest(request);
         expect(result.getBasicResult()).not.toBeNull();
-        expect(result.getBasicResult()!.getText()).toBe(popupBlockingRuleText);
+        expect(result.getBasicResult()!.getText()).toBe(blockingAllRuleText);
+        expect(result.popupRule!.getText()).toBe(popupBlockingRuleText);
     });
 });
 

diff --git a/packages/tsurlfilter/test/rules/network-rule.test.ts b/packages/tsurlfilter/test/rules/network-rule.test.ts
@@ -609,12 +609,12 @@ describe('NetworkRule constructor', () => {
         let rule = new NetworkRule('||example.org^$popup', -1);
         expect(rule).toBeTruthy();
         expect(rule.isOptionEnabled(NetworkRuleOption.Popup));
-        expect(rule.getPermittedRequestTypes()).toEqual(RequestType.Document);
+        expect(rule.getPermittedRequestTypes()).toEqual(RequestType.NotSet);
 
         rule = new NetworkRule('||example.org^$script,image,popup', -1);
         expect(rule).toBeTruthy();
         expect(rule.isOptionEnabled(NetworkRuleOption.Popup));
-        expect(rule.getPermittedRequestTypes()).toEqual(RequestType.Script | RequestType.Image | RequestType.Document);
+        expect(rule.getPermittedRequestTypes()).toEqual(RequestType.Script | RequestType.Image);
     });
 });
 
@@ -1287,18 +1287,13 @@ describe('NetworkRule.isHigherPriority', () => {
                 // 1 content-type -> negated content-type
                 ['||example.org$script', '||example.org$~script', true],
                 ['||example.org$document', '||example.org$~document', true],
-                // $popup explicity adds $document content-type
-                ['||example.org$popup', '||example.org$document,subdocument', true],
                 // content-types -> negated domains
                 ['||example.org$script', '||example.org$domain=~example.org', true],
                 ['||example.org$script,stylesheet', '||example.org$domain=~example.org', true],
                 ['||example.org$script,stylesheet,media', '||example.org$domain=~example.org', true],
                 ['||example.org$script,stylesheet,domain=~example.org', '||example.org$domain=~example.org', true],
                 ['||example.org$document', '||example.org$all', true],
                 ['||example.org$script,stylesheet,media', '||example.org$all', true],
-                // for document-requests in this case we want ot show blocking page - that's why $all should be over $popup
-                // TODO: uncomment when make priority of $all higher that $popup
-                // ['||example.org^$all', '||example.org^$popup', true],
                 ['||example.org$script,stylesheet,domain=~example.org', '||example.org$all', true],
                 // 1 method -> 2 methods
                 ['||example.org$method=get', '||example.org$method=get|post', true],

diff --git a/packages/tswebextension/CHANGELOG.md b/packages/tswebextension/CHANGELOG.md
@@ -8,6 +8,18 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 <!-- TODO: manually add compare links for version changes -->
 <!-- e.g. [0.1.2]: https://github.com/AdguardTeam/tsurlfilter/compare/tswebextension-v0.1.1...tswebextension-v0.1.2 -->
 
+## [1.0.19] - 2024-03-26
+
+### Changed
+
+- Analysis of $popup rules (in addition to the basic one) to determine
+  the result of blocking a request [#2620], [#2728].
+
+[1.0.19]: https://github.com/AdguardTeam/tsurlfilter/releases/tag/tswebextension-v1.0.19
+[#2620]: https://github.com/AdguardTeam/AdguardBrowserExtension/issues/2620
+[#2728]: https://github.com/AdguardTeam/AdguardBrowserExtension/issues/2728
+
+
 ## [1.0.18] - 2024-03-25
 
 ### Fixed

diff --git a/packages/tswebextension/package.json b/packages/tswebextension/package.json
@@ -1,6 +1,6 @@
 {
   "name": "@adguard/tswebextension",
-  "version": "1.0.18",
+  "version": "1.0.19",
   "description": "This is a TypeScript library that implements AdGuard's extension API",
   "main": "dist/index.js",
   "typings": "dist/types/src/lib/mv2/background/index.d.ts",