AG-31530 do not block should collapse with popup rules

Merge in ADGUARD-FILTERS/tsurlfilter from fix/AG-31530 to master Squashed commit of the following: commit 5a4dd60 Author: Slava Leleka <[email protected]> Date: Fri Mar 29 19:44:24 2024 +0300 packages/tswebextension/CHANGELOG.md edited online with Bitbucket commit c591612 Author: Slava Leleka <[email protected]> Date: Fri Mar 29 19:43:20 2024 +0300 packages/tsurlfilter/CHANGELOG.md edited online with Bitbucket commit d6e5ada Author: Maxim Topciu <[email protected]> Date: Fri Mar 29 18:34:21 2024 +0200 AG-31530 add comment commit ac4b7fa Author: Maxim Topciu <[email protected]> Date: Fri Mar 29 18:29:24 2024 +0200 AG-31530 add changelog commit cb3d9d9 Author: Maxim Topciu <[email protected]> Date: Fri Mar 29 18:02:10 2024 +0200 AG-31530 do not block should collapse with popup rules
AdguardTeam · Mar 29, 2024 · bf8a047 · bf8a047
1 parent c6a697e
commit bf8a047
Show file tree

Hide file tree

Showing 7 changed files with 48 additions and 13 deletions.
diff --git a/packages/tsurlfilter/CHANGELOG.md b/packages/tsurlfilter/CHANGELOG.md
@@ -8,6 +8,12 @@ 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.18] - 2024-03-29
+
+### Fixed
+
+- Do not block "Should collapse" mechanism with `$popup` rules
+
 ## [2.2.17] - 2024-03-28
 
 ### Changed

diff --git a/packages/tsurlfilter/src/engine/matching-result.ts b/packages/tsurlfilter/src/engine/matching-result.ts
@@ -79,7 +79,7 @@ export class MatchingResult {
      * 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;
+    private popupRule: NetworkRule | null;
 
     /**
      * Creates an instance of the MatchingResult struct and fills it with the rules.
@@ -191,6 +191,13 @@ export class MatchingResult {
         }
     }
 
+    /**
+     * Returns popup rule
+     */
+    public getPopupRule(): NetworkRule | null {
+        return this.popupRule;
+    }
+
     /**
      * GetBasicResult returns a rule that should be applied to the web request.
      * Possible outcomes are:

diff --git a/packages/tsurlfilter/test/engine/engine.test.ts b/packages/tsurlfilter/test/engine/engine.test.ts
@@ -418,28 +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);
+        expect(result.getPopupRule()!.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);
+        expect(result.getPopupRule()!.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);
+        expect(result.getPopupRule()!.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(blockingRuleText);
-        expect(result.popupRule!.getText()).toBe(popupBlockingRuleText);
+        expect(result.getPopupRule()!.getText()).toBe(popupBlockingRuleText);
     });
 
     it('match requests against all and popup blocking rules', () => {
@@ -457,28 +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);
+        expect(result.getPopupRule()!.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);
+        expect(result.getPopupRule()!.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);
+        expect(result.getPopupRule()!.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(blockingAllRuleText);
-        expect(result.popupRule!.getText()).toBe(popupBlockingRuleText);
+        expect(result.getPopupRule()!.getText()).toBe(popupBlockingRuleText);
     });
 });
 

diff --git a/packages/tswebextension/CHANGELOG.md b/packages/tswebextension/CHANGELOG.md
@@ -8,6 +8,12 @@ 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.21] - 2024-03-29
+
+### Fixed
+
+- Do not block "Should collapse" mechanism with `$popup` rules
+
 ## [1.0.20] - 2024-03-28
 
 ### Changed

diff --git a/packages/tswebextension/src/lib/mv2/background/request/request-blocking-api.ts b/packages/tswebextension/src/lib/mv2/background/request/request-blocking-api.ts
@@ -73,6 +73,14 @@ export class RequestBlockingApi {
             return false;
         }
 
+        const basicRule = result.getBasicResult();
+        const popupRule = result.getPopupRule();
+
+        // we do not want to block the main page if the rule has only $popup modifier
+        if (basicRule === popupRule) {
+            return false;
+        }
+
         return RequestBlockingApi.isRequestBlockedByRule(result.getBasicResult());
     }
 
@@ -133,7 +141,7 @@ export class RequestBlockingApi {
         }
 
         // popup rule will be handled in the condition with requesttype === document below
-        if (popupRule?.getText() === rule.getText() && requestType !== RequestType.Document) {
+        if (popupRule === rule && requestType !== RequestType.Document) {
             return undefined;
         }
 
@@ -171,7 +179,7 @@ export class RequestBlockingApi {
             }
 
             // we do not want to block the main page if rule has only $popup modifier
-            if (rule.getText() === popupRule?.getText() && !tabsApi.isNewPopupTab(tabId)) {
+            if (rule === popupRule && !tabsApi.isNewPopupTab(tabId)) {
                 return undefined;
             }
 

diff --git a/packages/tswebextension/src/lib/mv2/background/web-request-api.ts b/packages/tswebextension/src/lib/mv2/background/web-request-api.ts
@@ -359,7 +359,7 @@ export class WebRequestApi {
         // the response in order to actually apply $replace rules to it.
         const response = RequestBlockingApi.getBlockingResponse({
             rule: basicResult,
-            popupRule: result.popupRule,
+            popupRule: result.getPopupRule(),
             eventId,
             requestUrl,
             referrerUrl,

diff --git a/packages/tswebextension/test/lib/mv2/background/request/request-blocking-api.test.ts b/packages/tswebextension/test/lib/mv2/background/request/request-blocking-api.test.ts
@@ -36,7 +36,7 @@ const getGetBlockingResponseParamsData = (
         tabId: 1,
         eventId: '1',
         rule: result.getBasicResult(),
-        popupRule: result.popupRule,
+        popupRule: result.getPopupRule(),
         referrerUrl: '',
         requestUrl,
         requestType,
@@ -64,6 +64,14 @@ describe('Request Blocking Api - shouldCollapseElement', () => {
         ).toBe(true);
     });
 
+    it('iframe should not be collapsed by popup rule', () => {
+        mockMatchingResult('$popup,third-party,domain=example.org');
+
+        expect(
+            RequestBlockingApi.shouldCollapseElement(1, 'https://example.com', 'https://example.org', RequestType.SubDocument),
+        ).toBe(false);
+    });
+
     it('element without rule match shouldn`t be collapsed', () => {
         mockMatchingResult();