-
Notifications
You must be signed in to change notification settings - Fork 104
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: check domains validity when parsing network filters #4525
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we add some tests for those changes?
@chrmod Added |
@chrmod I realized that it was improper to add a check in the
|
Tests are currently expected to fail: see #4525 (comment) |
Domains.prototype.parse
One more idea to this would be having a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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||
@chrmod I believe adblocker should not be error tolerant here.
For the dynamic delimiter, I agree that we can just accept the string instead of boolean flag. |
Co-authored-by: chrmod <[email protected]>
From #4524, I found that
NetworkFilter.prototype.toString
is 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
.