Pull request #1060: AG-36003 Scriptlets exclusion matching is not wor…

…king properly for rules with arguments Merge in ADGUARD-FILTERS/tsurlfilter from fix/AG-36003 to master Squashed commit of the following: commit 2dcd8c6 Author: Kurbanali Ruslan <[email protected]> Date: Thu Sep 26 20:08:10 2024 +0500 fix comments commit 0d1fdbe Author: Kurbanali Ruslan <[email protected]> Date: Thu Sep 26 20:07:23 2024 +0500 fix formatting commit 9546d4f Author: Kurbanali Ruslan <[email protected]> Date: Thu Sep 26 20:06:46 2024 +0500 fixed test case commit 359bb79 Author: Kurbanali Ruslan <[email protected]> Date: Thu Sep 26 19:55:56 2024 +0500 changed comments commit 8a9851c Author: Kurbanali Ruslan <[email protected]> Date: Thu Sep 26 19:53:13 2024 +0500 changed test cases commit 66882b9 Author: Kurbanali Ruslan <[email protected]> Date: Thu Sep 26 18:55:21 2024 +0500 update changelog commit f38b524 Author: Kurbanali Ruslan <[email protected]> Date: Thu Sep 26 18:38:00 2024 +0500 changed comment commit f1b7b8c Author: Kurbanali Ruslan <[email protected]> Date: Thu Sep 26 18:09:20 2024 +0500 fix matching scriptlets with args commit 134c427 Author: Slava Leleka <[email protected]> Date: Tue Sep 24 14:45:16 2024 +0300 add fixme commit 1043052 Author: Slava Leleka <[email protected]> Date: Tue Sep 24 14:45:02 2024 +0300 add one more test commit f9d0d80 Author: kurrx <[email protected]> Date: Tue Sep 24 15:51:51 2024 +0500 added test cases to scriptlet exceptions Co-authored-by: Slava <[email protected]>
AdguardTeam · Sep 26, 2024 · 231db94 · 231db94
1 parent 3438c52
commit 231db94
Show file tree

Hide file tree

Showing 3 changed files with 41 additions and 13 deletions.
diff --git a/packages/tsurlfilter/CHANGELOG.md b/packages/tsurlfilter/CHANGELOG.md
@@ -8,6 +8,14 @@ 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 -->
 
+## Unreleased
+
+### Fixed
+
+- Scriptlets exclusion matching is not working properly for rules with arguments [AdguardBrowserExtension#2947].
+
+[AdguardBrowserExtension#2947]: https://github.com/AdguardTeam/AdguardBrowserExtension/issues/2947
+
 ## [3.0.4] - 2024-09-19
 
 ### Changed

diff --git a/packages/tsurlfilter/src/engine/cosmetic-engine/cosmetic-lookup-table.ts b/packages/tsurlfilter/src/engine/cosmetic-engine/cosmetic-lookup-table.ts
@@ -144,18 +144,18 @@ export class CosmeticLookupTable {
     }
 
     /**
-     * Checks if a scriptlet is allowlisted for a request. It looks up the scriptlet by name in the
-     * allowlistScriptlets map and evaluates two conditions:
+     * Checks if a scriptlet is allowlisted for a request. It looks up the scriptlet
+     * by content in the allowlist map and evaluates two conditions:
      * 1. If there's a generic allowlist rule applicable to all sites.
      * 2. If there's a specific allowlist rule that matches the request.
      *
-     * @param name Name of the scriptlet. Empty string '' searches for scriptlets allowlisted globally.
+     * @param content Content of the scriptlet. Empty string '' searches for scriptlets allowlisted globally.
      * @param request Request details to match against allowlist rules.
      * @returns True if allowlisted by a matching rule or a generic rule. False otherwise.
      */
-    isScriptletAllowlistedByName = (name: string, request: Request) => {
-        // check for rules with names
-        const allowlistScriptletRulesIndexes = this.allowlist.get(name);
+    isScriptletAllowlisted = (content: string, request: Request) => {
+        // check for rules with that content
+        const allowlistScriptletRulesIndexes = this.allowlist.get(content);
         if (allowlistScriptletRulesIndexes) {
             const rules = allowlistScriptletRulesIndexes
                 .map((i) => {
@@ -188,12 +188,23 @@ export class CosmeticLookupTable {
             // Empty string '' is a special case for scriptlet when the allowlist scriptlet has no name
             // e.g. #@%#//scriptlet(); example.org#@%#//scriptlet();
             const EMPTY_SCRIPTLET_NAME = '';
-            if (this.isScriptletAllowlistedByName(EMPTY_SCRIPTLET_NAME, request)) {
+            if (this.isScriptletAllowlisted(EMPTY_SCRIPTLET_NAME, request)) {
                 return true;
             }
 
+            // If scriptlet allowlisted by name
+            // e.g. #@%#//scriptlet('set-cookie'); example.org#@%#//scriptlet('set-cookie');
             if (rule.scriptletParams.name !== undefined
-                && this.isScriptletAllowlistedByName(rule.scriptletParams.name, request)) {
+                && this.isScriptletAllowlisted(rule.scriptletParams.name, request)) {
+                return true;
+            }
+
+            // If scriptlet allowlisted with args, using normalized scriptlet content for better matching
+            // on different quote types (see https://github.com/AdguardTeam/AdguardBrowserExtension/issues/2947)
+            // e.g. #@%#//scriptlet("set-cookie", "arg1"); example.org#@%#//scriptlet('set-cookie', 'arg1');
+            if (rule.scriptletParams.name !== undefined
+                && rule.scriptletParams.args.length > 0
+                && this.isScriptletAllowlisted(rule.scriptletParams.toString(), request)) {
                 return true;
             }
         }

diff --git a/packages/tsurlfilter/test/engine/cosmetic-engine/cosmetic-engine.test.ts b/packages/tsurlfilter/test/engine/cosmetic-engine/cosmetic-engine.test.ts
@@ -405,12 +405,21 @@ describe('Test cosmetic engine - JS rules', () => {
         });
 
         it('checks scriptlet exception even if content has different quotes', () => {
-            const jsRule = String.raw`testcases.adguard.com,surge.sh#%#//scriptlet('abort-on-property-read', 'I10\'C')`;
-            // eslint-disable-next-line max-len
-            const jsExceptionRule = String.raw`testcases.adguard.com,surge.sh#@%#//scriptlet("abort-on-property-read", "I10'C")`;
+            const domains = 'testcases.adguard.com,surge.sh';
+            const singleQuote1 = String.raw`//scriptlet('abort-on-property-read', 'I10\'C')`;
+            const doubleQuote1 = String.raw`//scriptlet("abort-on-property-read", "I10'C")`;
+            const singleQuote2 = String.raw`//scriptlet('set-cookie', 'I10\'C')`;
+            const doubleQuote2 = String.raw`//scriptlet("set-cookie", "I10'C")`;
+
             const rulesLocal = [
-                jsRule,
-                jsExceptionRule,
+                // single quote rule
+                `${domains}#%#${singleQuote1}`,
+                // double quote exception
+                `${domains}#@%#${doubleQuote1}`,
+                // double quote rule
+                `${domains}#%#${doubleQuote2}`,
+                // single quote exception
+                `${domains}#@%#${singleQuote2}`,
             ];
             const processedLocal = FilterListPreprocessor.preprocess(rulesLocal.join('\n'));
             const cosmeticEngine = new CosmeticEngine(createTestRuleStorage(1, processedLocal));