Skip to content

Commit

Permalink
AG-24716: fixed checking for unsupported options
Browse files Browse the repository at this point in the history 
Merge in ADGUARD-FILTERS/tsurlfilter from fix/AG-24716 to master

Squashed commit of the following:

commit dda7cd6
Author: Dmitriy Seregin <[email protected]>
Date:   Fri Aug 11 17:28:06 2023 +0400

    fix comment

commit e607edc
Author: Dmitriy Seregin <[email protected]>
Date:   Fri Aug 11 17:16:53 2023 +0400

    cleaned diff

commit 0969a36
Merge: fe3edfe 98a99db
Author: Dmitriy Seregin <[email protected]>
Date:   Fri Aug 11 17:16:11 2023 +0400

    Merge branch 'master' into fix/AG-24716

commit fe3edfe
Author: Dmitriy Seregin <[email protected]>
Date:   Wed Aug 9 18:31:07 2023 +0400

    AG-24716: fixed checking for unsupported options
  • Loading branch information
@105th
105th committed Aug 11, 2023
1 parent 98a99db commit a9ac642
Show file tree
Hide file tree
Showing 5 changed files with 99 additions and 51 deletions.
18 changes: 3 additions & 15 deletions packages/tsurlfilter/src/rules/declarative-converter/README.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,8 @@ into `$elemhide,content,urlblock,jsinject` of which:
- `$jsinject` - not implemented yet,
- `$urlblock` - not implemented yet.

So we still convert rules with `$document`, but not 100% correctly.
So we still convert rules with `$document`, but only part with `$elemhide`
will be applied.

<a name="mv3_specific_limitations__$removeparam__$removeheader__$csp"></a>
## $removeparam, $removeheader, $csp
Expand Down Expand Up @@ -2426,20 +2427,7 @@ example 10
↓↓↓↓ converted to ↓↓↓↓

```json
[
{
"id": 1,
"action": {
"type": "block"
},
"condition": {
"urlFilter": "||facebook.com^",
"domainType": "thirdParty",
"isUrlFilterCaseSensitive": false
},
"priority": 2
}
]
[]

```
<a name="advanced_capabilities__$csp"></a>
Expand Down
58 changes: 50 additions & 8 deletions ...ilter/src/rules/declarative-converter/grouped-rules-converters/abstract-rule-converter.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -664,8 +664,8 @@ export abstract class DeclarativeRuleConverter {
const checkRemoveParamModifierFn = (r: NetworkRule, name: string): UnsupportedModifierError | null => {
const removeParam = r.getAdvancedModifier() as RemoveParamModifier;
if (!removeParam.getMV3Validity()) {
const msg = 'Network rule with $removeparam modifier with '
+ `negation or regexp is not supported: "${r.getText()}"`;
// eslint-disable-next-line max-len
const msg = `Network rule with $removeparam modifier with negation or regexp is not supported: "${r.getText()}"`;

return new UnsupportedModifierError(msg, r);
}
Expand Down Expand Up @@ -750,7 +750,6 @@ export abstract class DeclarativeRuleConverter {
|| restrictedMethods?.some((method) => method === HTTPMethod.TRACE)
) {
return new UnsupportedModifierError(
// eslint-disable-next-line max-len
`Network rule with $method modifier containing 'trace' method is not supported: "${r.getText()}"`,
r,
);
Expand All @@ -759,15 +758,55 @@ export abstract class DeclarativeRuleConverter {
return null;
};

/**
* Checks if rule is a "document"-allowlist and contains all these
* `$elemhide,content,urlblock,jsinject` modifiers at the same time.
* If it is - we allow partially convert this rule, because `$content`
* is not supported in the MV3 at all and `$jsinject` and `$urlblock`
* are not implemented yet, but we can support at least allowlist-rule
* with `$elemhide` modifier (not in the DNR, but with tsurlfilter engine).
*
* TODO: Change the description when `$jsinject` and `$urlblock`
* are implemented.
*
* @param r Network rule.
* @param name Modifier's name.
*
* @returns Error {@link UnsupportedModifierError} or null if rule is supported.
*/
// eslint-disable-next-line @typescript-eslint/no-unused-vars
const checkDocumentAllowlistFn = (r: NetworkRule, name: string): UnsupportedModifierError | null => {
if (rule.isFilteringDisabled()) {
return null;
}

return new UnsupportedModifierError(
`Network rule with "${name}" modifier is not supported: "${r.getText()}"`,
r,
);
};

const unsupportedOptions = [
/* Specific exceptions */
{ option: NetworkRuleOption.Elemhide, name: '$elemhide', skipConversion: true },
{ option: NetworkRuleOption.Generichide, name: '$generichide', skipConversion: true },
{ option: NetworkRuleOption.Specifichide, name: '$specifichide', skipConversion: true },
{ option: NetworkRuleOption.Genericblock, name: '$genericblock' },
{ option: NetworkRuleOption.Jsinject, name: '$jsinject' },
{ option: NetworkRuleOption.Urlblock, name: '$urlblock' },
{ option: NetworkRuleOption.Content, name: '$content' },
{
option: NetworkRuleOption.Jsinject,
name: '$jsinject',
customChecks: [checkDocumentAllowlistFn],
},
{
option: NetworkRuleOption.Urlblock,
name: '$urlblock',
customChecks: [checkDocumentAllowlistFn],
},
{
option: NetworkRuleOption.Content,
name: '$content',
customChecks: [checkDocumentAllowlistFn],
},
{ option: NetworkRuleOption.Extension, name: '$extension' },
{ option: NetworkRuleOption.Stealth, name: '$stealth' },
/* Specific exceptions */
Expand Down Expand Up @@ -815,12 +854,15 @@ export abstract class DeclarativeRuleConverter {
skipConversion,
} = unsupportedOptions[i];

if (!rule.isSingleOptionEnabled(option)) {
if (!rule.isOptionEnabled(option)) {
continue;
}

if (skipConversion) {
return false;
if (rule.isSingleOptionEnabled(option)) {
return false;
}
continue;
}

if (customChecks) {
Expand Down
3 changes: 2 additions & 1 deletion packages/tsurlfilter/src/rules/declarative-converter/readme.txt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@
! - `$jsinject` - not implemented yet,
! - `$urlblock` - not implemented yet.
!
! So we still convert rules with `$document`, but not 100% correctly.
! So we still convert rules with `$document`, but only part with `$elemhide`
! will be applied.
!
! ## $removeparam, $removeheader, $csp
! Rules with `$removeparam` or `$removeheader` or `$csp` which contains the same
Expand Down
49 changes: 33 additions & 16 deletions packages/tsurlfilter/test/rules/declarative-converter/declarative-converter.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -485,8 +485,8 @@ describe('DeclarativeConverter', () => {

// eslint-disable-next-line max-len
const err = new Error('"Unknown modifier: webrtc" in the rule: "@@$webrtc,domain=example.com"');
expect(declarativeRules.length).toBe(0);
expect(errors.length).toBe(1);
expect(declarativeRules).toHaveLength(0);
expect(errors).toHaveLength(1);
expect(errors[0]).toStrictEqual(err);
});

Expand All @@ -507,8 +507,8 @@ describe('DeclarativeConverter', () => {
networkRule,
);

expect(declarativeRules.length).toBe(0);
expect(errors.length).toBe(1);
expect(declarativeRules).toHaveLength(0);
expect(errors).toHaveLength(1);
expect(errors[0]).toStrictEqual(err);
});

Expand All @@ -523,24 +523,41 @@ describe('DeclarativeConverter', () => {
// eslint-disable-next-line max-len
const err = new Error('"modifier $to is not compatible with $denyallow modifier" in the rule: "/ads$to=good.org,denyallow=good.com"');

expect(declarativeRules.length).toBe(0);
expect(errors.length).toBe(1);
expect(declarativeRules).toHaveLength(0);
expect(errors).toHaveLength(1);

console.log('errors: ', errors);
expect(errors[0]).toStrictEqual(err);
});
});

it('use only main_frame or sub_frame for allowAllRequests rules', async () => {
const rule = '@@||example.com/*/search?*&method=HEAD$xmlhttprequest,document';
const filter = createFilter([rule]);
const { ruleSets: [ruleSet] } = await converter.convert(
[filter],
);
describe('test some edge cases', () => {
it('use only main_frame or sub_frame for allowAllRequests rules', async () => {
const rule = '@@||example.com/*/search?*&method=HEAD$xmlhttprequest,document';
const filter = createFilter([rule]);
const { ruleSets: [ruleSet] } = await converter.convert(
[filter],
);

const { declarativeRules } = await ruleSet.serialize();
const { declarativeRules } = await ruleSet.serialize();

expect(declarativeRules).toHaveLength(1);
expect(declarativeRules[0].action.type).not.toContain(RuleActionType.ALLOW_ALL_REQUESTS);
expect(declarativeRules).toHaveLength(1);
expect(declarativeRules[0].action.type).not.toContain(RuleActionType.ALLOW_ALL_REQUESTS);
});

it('test allowlist rule with $document, $csp and $domain', async () => {
const filter = createFilter([
'@@*$document,csp=worker-src \'none\',domain=new.lewd.ninja',
]);
const {
ruleSets: [ruleSet],
errors,
} = await converter.convert(
[filter],
);
const { declarativeRules } = await ruleSet.serialize();

expect(errors).toHaveLength(1);
expect(declarativeRules).toHaveLength(0);
});
});
});
22 changes: 11 additions & 11 deletions packages/tsurlfilter/test/rules/declarative-converter/declarative-rule-converter.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1227,7 +1227,7 @@ describe('DeclarativeRuleConverter', () => {
} = DeclarativeRulesConverter.convert(
[[filterId, rules]],
);
expect(declarativeRules.length).toBe(1);
expect(declarativeRules).toHaveLength(1);
expect(declarativeRules[0]).toEqual({
id: 1,
priority: 1,
Expand Down Expand Up @@ -1262,7 +1262,7 @@ describe('DeclarativeRuleConverter', () => {
const { declarativeRules } = DeclarativeRulesConverter.convert(
[[filterId, rules]],
);
expect(declarativeRules.length).toBe(3);
expect(declarativeRules).toHaveLength(3);
expect(declarativeRules[0]).toStrictEqual({
id: 1,
priority: 1,
Expand Down Expand Up @@ -1350,7 +1350,7 @@ describe('DeclarativeRuleConverter', () => {
} = DeclarativeRulesConverter.convert(
[[filterId, rules]],
);
expect(declarativeRules.length).toBe(1);
expect(declarativeRules).toHaveLength(1);
expect(declarativeRules[0]).toEqual({
id: 1,
priority: 2,
Expand Down Expand Up @@ -1381,7 +1381,7 @@ describe('DeclarativeRuleConverter', () => {
} = DeclarativeRulesConverter.convert(
[[filterId, rules]],
);
expect(declarativeRules.length).toBe(1);
expect(declarativeRules).toHaveLength(1);
expect(declarativeRules[0]).toEqual({
id: 1,
priority: 2,
Expand Down Expand Up @@ -1410,7 +1410,7 @@ describe('DeclarativeRuleConverter', () => {
} = DeclarativeRulesConverter.convert(
[[filterId, rules]],
);
expect(declarativeRules.length).toBe(1);
expect(declarativeRules).toHaveLength(1);
expect(declarativeRules[0]).toEqual({
id: 1,
priority: 2,
Expand Down Expand Up @@ -1443,7 +1443,7 @@ describe('DeclarativeRuleConverter', () => {
} = DeclarativeRulesConverter.convert(
[[filterId, rules]],
);
expect(declarativeRules.length).toBe(1);
expect(declarativeRules).toHaveLength(1);
expect(declarativeRules[0]).toEqual({
id: 1,
priority: 76,
Expand Down Expand Up @@ -1471,7 +1471,7 @@ describe('DeclarativeRuleConverter', () => {
} = DeclarativeRulesConverter.convert(
[[filterId, rules]],
);
expect(declarativeRules.length).toBe(1);
expect(declarativeRules).toHaveLength(1);
expect(declarativeRules[0]).toEqual({
id: 1,
priority: 2,
Expand Down Expand Up @@ -1499,7 +1499,7 @@ describe('DeclarativeRuleConverter', () => {
} = DeclarativeRulesConverter.convert(
[[filterId, rules]],
);
expect(declarativeRules.length).toBe(1);
expect(declarativeRules).toHaveLength(1);
expect(declarativeRules[0]).toEqual({
id: 1,
priority: 100101,
Expand Down Expand Up @@ -1527,7 +1527,7 @@ describe('DeclarativeRuleConverter', () => {
} = DeclarativeRulesConverter.convert(
[[filterId, rules]],
);
expect(declarativeRules.length).toBe(1);
expect(declarativeRules).toHaveLength(1);
expect(declarativeRules[0]).toEqual({
id: 1,
priority: 100002,
Expand All @@ -1554,8 +1554,8 @@ describe('DeclarativeRuleConverter', () => {
} = DeclarativeRulesConverter.convert(
[[filterId, rules]],
);
expect(declarativeRules.length).toBe(0);
expect(errors.length).toBe(1);
expect(declarativeRules).toHaveLength(0);
expect(errors).toHaveLength(1);

const networkRule = new NetworkRule(ruleText, filterId);

Expand Down

0 comments on commit a9ac642

Please sign in to comment.