From 5e394712bc937ca32a294f2a72a2c6588f496708 Mon Sep 17 00:00:00 2001 From: Dmitry Seregin Date: Fri, 20 Sep 2024 17:09:45 +0300 Subject: [PATCH] AG-36118: fixed duplicate IDs of converted dynamic rules Merge in ADGUARD-FILTERS/tsurlfilter from fix/AG-36118 to release/v3.1 Squashed commit of the following: commit bc1743e51a580cb5df264b44c7f08dca00883652 Merge: 3bff0a647 2d348902d Author: Dmitriy Seregin Date: Fri Sep 20 16:57:05 2024 +0300 Merge branch 'release/v3.1' into fix/AG-36118 commit 3bff0a647c6e9f5d178df294eafbe304a3009f96 Author: Dmitriy Seregin Date: Fri Sep 20 16:41:45 2024 +0300 added some comments commit 7a923a4b2097ec20f77f6c0b9e9fd0a128713631 Author: Dmitriy Seregin Date: Fri Sep 20 14:43:09 2024 +0300 AG-36118: fixed duplicate IDs of converted dynamic rules --- packages/tsurlfilter/CHANGELOG.md | 9 ++++ packages/tsurlfilter/package.json | 2 +- .../declarative-converter/rules-converter.ts | 27 ++++++++++-- .../declarative-converter.test.ts | 42 +++++++++++++++++++ 4 files changed, 75 insertions(+), 5 deletions(-) diff --git a/packages/tsurlfilter/CHANGELOG.md b/packages/tsurlfilter/CHANGELOG.md index f51b8ee13..db6b044f6 100644 --- a/packages/tsurlfilter/CHANGELOG.md +++ b/packages/tsurlfilter/CHANGELOG.md @@ -8,6 +8,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 +## [3.1.0-alpha.4] - 2024-09-20 + +### Fixed + +- In some cases dynamic rules can have not unique IDs [AdguardBrowserExtension#2953]. + +[3.1.0-alpha.4]: https://github.com/AdguardTeam/tsurlfilter/releases/tag/tsurlfilter-v3.1.0-alpha.4 +[AdguardBrowserExtension#2953]: https://github.com/AdguardTeam/AdguardBrowserExtension/issues/2953 + ## [3.0.3] - 2024-09-19 ### Changed diff --git a/packages/tsurlfilter/package.json b/packages/tsurlfilter/package.json index 6a7edd4e5..04900e886 100644 --- a/packages/tsurlfilter/package.json +++ b/packages/tsurlfilter/package.json @@ -1,6 +1,6 @@ { "name": "@adguard/tsurlfilter", - "version": "3.1.0-alpha.3", + "version": "3.1.0-alpha.4", "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/rules/declarative-converter/rules-converter.ts b/packages/tsurlfilter/src/rules/declarative-converter/rules-converter.ts index 8ac124bdc..a995de5ff 100644 --- a/packages/tsurlfilter/src/rules/declarative-converter/rules-converter.ts +++ b/packages/tsurlfilter/src/rules/declarative-converter/rules-converter.ts @@ -158,9 +158,9 @@ export class DeclarativeRulesConverter { }; for (const [filterId, groupedRules] of filters) { - const lastUsedId = converted.declarativeRules.length > 0 - ? converted.declarativeRules[converted.declarativeRules.length - 1].id + 1 - : DeclarativeRulesConverter.START_DECLARATIVE_RULE_ID; + const highestUsedId = converted.declarativeRules.reduce((currentMax, rule) => { + return Math.max(currentMax, rule.id); + }, DeclarativeRulesConverter.START_DECLARATIVE_RULE_ID); const { sourceMapValues, @@ -170,7 +170,7 @@ export class DeclarativeRulesConverter { } = await this.convertRules( filterId, groupedRules, - lastUsedId, + highestUsedId, options, ); @@ -185,6 +185,10 @@ export class DeclarativeRulesConverter { options?.maxNumberOfRegexpRules, ); + if (!this.checkRulesHaveUniqueIds(converted.declarativeRules)) { + throw new Error('Declarative rules have non-unique identifiers.'); + } + return converted; } @@ -235,6 +239,21 @@ export class DeclarativeRulesConverter { return converted; } + /** + * Checks that declarative rules have unique identifiers. + * + * @param rules List of declarative rules. + * + * @returns True if all rules have unique identifiers, otherwise false. + */ + private static checkRulesHaveUniqueIds(rules: DeclarativeRule[]): boolean { + const ids = rules.map(({ id }) => id); + + const uniqueIds = new Set(ids); + + return uniqueIds.size === rules.length; + } + /** * Check that declarative rules matches the specified constraints and * cuts rules if needed as from list also from source map. diff --git a/packages/tsurlfilter/test/rules/declarative-converter/declarative-converter.test.ts b/packages/tsurlfilter/test/rules/declarative-converter/declarative-converter.test.ts index 25adee11d..b1fb02a60 100644 --- a/packages/tsurlfilter/test/rules/declarative-converter/declarative-converter.test.ts +++ b/packages/tsurlfilter/test/rules/declarative-converter/declarative-converter.test.ts @@ -757,5 +757,47 @@ describe('DeclarativeConverter', () => { expect(errors).toHaveLength(1); expect(declarativeRules).toHaveLength(0); }); + + // In old logic we used last rule id from previous filter to calculate + // the id of the first rule in the next filter (last used plus one). + // But in some cases it could lead to the same id for different rules, + // for example when we have grouping rules in the first filter and + // last converted rule will have not the highest id. + it('while convert dynamic rules each rule should have unique id', async () => { + const filter1 = createFilter([ + 'example.com', // id = 5 + '||test.com$removeparam=p1', // id = 27 + '||test.com$removeparam=p2', // id = 70 + 'example.org', // id = 113 + ], 1); + // Length of first rule should be equal to = last rule id from filter1 - second last rule id + bytes needed + // to serialize the rule node (which brings some extra offset, depends on the rule). + const filter2 = createFilter([ + 'm7sk4ubcalu89gp1q1elsulnhslt2vykalsdjfcvkldfncchfcvcc.zsxnbcfidxyhcwhc', + 'bad.rule', + ], 2); + + const { ruleSet } = await converter.convertDynamicRuleSets([ + filter1, filter2, + ], []); + const declarativeRules = await ruleSet.getDeclarativeRules(); + + // 5 is because of 2 removeparam rules are converted into 1 declarative rule. + expect(declarativeRules).toHaveLength(5); + + // Function to bring more human readable error message. + const checkForUniqueRule = (rules: typeof declarativeRules) => { + rules.forEach((rule, index) => { + for (let i = 0; i < rules.length; i += 1) { + if (rule.id === rules[i].id && i !== index) { + // eslint-disable-next-line max-len + throw new Error(`Rules have the same id: ${JSON.stringify(rule, null, 2)}, ${JSON.stringify(rules[i], null, 2)}`); + } + } + }); + }; + + expect(() => checkForUniqueRule(declarativeRules)).not.toThrow(); + }); }); });