fix: check domains validity when parsing network filters #4525
Conversation
|
@chrmod Added |
|
|
||
| describe('Domains', () => { | ||
| it('rejects to parse invalid pipes', () => { | ||
| for (const str of ['|foo.com', 'foo.com|', '|foo.com|']) { |
There was a problem hiding this comment.
shouldn't we just drop | and accept foo.com?
There was a problem hiding this comment.
maybe we should support $to=example.com|||||ghostery.com as well
There was a problem hiding this comment.
@chrmod We have tests that defines those cases as an invalid. Please, choose the behavior on this. Personally, I don't feel it's good to accept those typos.
1) #parseFilters
with not supported filters
returns invalid network filters:
AssertionError: expected [] to deeply equal [ { filter: '$domain=|a', …(2) } ]
+ expected - actual
-[]
+[
+ {
+ "filter": "$domain=|a"
+ "filterType": 1
+ "lineNumber": 0
+ }
+]
at Context.<anonymous> (test/lists.test.ts:210:24)
at process.processImmediate (node:internal/timers:491:21)
There was a problem hiding this comment.
Reverting this to keep existing tests: 071550b (#4525)
|
@chrmod I realized that it was improper to add a check in the
|
|
Tests are currently expected to fail: see #4525 (comment) |
seia-soto
changed the title
Domains.prototype.parse
|
One more idea to this would be having a |
| @@ -11,6 +11,24 @@ import { toASCII } from '../punycode.js'; | |||
| import { StaticDataView, sizeOfUint32Array, sizeOfUTF8 } from '../data-view.js'; | |||
| import { binLookup, hasUnicode, HASH_INTERNAL_MULT } from '../utils.js'; | |||
|
|
|||
| export function testNetworkParts(parts: string[]): boolean { | |||
There was a problem hiding this comment.
What is a "network part"? Can we make the naming more specific here as I find it hard to understand without more context.
There was a problem hiding this comment.
I added an option called isNetworkEntities and integrated everything inside of Domains.prototype.parse. The new shape looks like the following. Would like you like to change the shape?
export class Domains {
public static parse(
parts: string[],
{
isNetworkEntities = false,
debug = false,
}: { isNetworkEntities?: boolean | undefined; debug?: boolean | undefined } = {},
): Domains | undefined| return true; | ||
| } | ||
|
|
||
| export function normalizeNetworkPartsLiteral(parts: string) { |
There was a problem hiding this comment.
Let's add some comment and/or make naming more explicit. It's hard to understand what "normalizing" means, I think it's too generic.
There was a problem hiding this comment.
Wouldn't it make more sense to move the splitting to Domain.parse and pass a desired separator as an argument?
The logic for the trailing | may apply to trailing ,.
Or alternatively, we could ignore those trailing characters as filters that include them (if they exist) may still be useful. The question is how fault tolerant we want to be. Eg. Should we drop filter: test$domain=example.com|example.org||
From #4524, I found that
NetworkFilter.prototype.toStringis broken for network modifiers utilizingDomains.This fixes it by replacing all
,into|inDomains.parts.Also, this moves invalid option value check which was used to prevent passing
$domain=|a.com|forms intoDomains.prototype.parse.