Skip to content
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

feat: to network modifier #4524

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

seia-soto
Copy link
Member

@seia-soto seia-soto commented Dec 12, 2024

This partially implements https://github.com/gorhill/uBlock/wiki/Static-filter-syntax#to.

In the documentation, to is described as:

to= is a superset of denyallow= with support for Entity-based syntax and also negated hostname.

However, we already support entity-based syntax in Domains and only thing required is negation according to entity documentation: https://github.com/gorhill/uBlock/wiki/Static-filter-syntax#entity

We still don't support regular expression both in domain and denyallow options. Same goes for to: *$to=~/regex.../.

  • denyallow with uBo: ✅ negation and listing ❌ entities ❌ regex
  • to with uBo: ✅ negation and listing ✅ entities ✅ regex
  • denyallow, to with Ghostery: ✅ negation and listing ✅ entities ❌ regex

This PR negates all hostnames in parse-time to flip the behavior of denyallow when to is detected.
Also, rejects to parse filter having both denyallow and to since both are not likely to be used together.
We can merge multiple domains then make it work but then we'll require extra space that allows us to determine which hostname is for to and denyallow when running toString.

This PR also fixes denyallow domain list not being printed in a correct format from toString method. There's a same regression in toString regrading other network modifiers utilizing Domains like from and planned to fix in separate PR.

! incorrect
||domain.tld$denyallow=a.com,b.com

! correct
||domain.tld$denyallow=a.com|b.com

@seia-soto seia-soto added the PR: New Feature 🚀 Increment minor version when merged label Dec 12, 2024
@seia-soto seia-soto self-assigned this Dec 12, 2024
@seia-soto seia-soto requested a review from remusao as a code owner December 12, 2024 07:08
@seia-soto seia-soto changed the title feat: introduce $to in NetworkFilters feat: to network modifier Dec 12, 2024
@seia-soto
Copy link
Member Author

Despite we marked it's not safe to add one more bit before, but it's still safe to use 31st bit given that we're treating the number as unsigned integer.

> const buf = new Uint8Array(4)
undefined
> buf.length
4
> buf[0] = (1 << 31) >>> 24
128
> buf
Uint8Array(4) [ 128, 0, 0, 0 ]
> (buf[0] << 24) >>> 0
2147483648

packages/adblocker/test/parsing.test.ts Outdated Show resolved Hide resolved
@seia-soto seia-soto requested a review from chrmod December 13, 2024 06:22
@seia-soto seia-soto removed the WIP label Dec 19, 2024
@seia-soto seia-soto requested a review from chrmod December 25, 2024 03:59
Comment on lines +737 to +747
for (const part of value.split('|')) {
if (option === 'to') {
if (part.charCodeAt(0) === 126 /* '~' */) {
denyallowEntities.add(part.slice(1));
} else {
denyallowEntities.add(`~${part}`);
}
} else {
denyallowEntities.add(part);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe this would be best handled inside of Domains directly. Maybe with a flag?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The second argument of "Domain" is "debug = false". I don't know if it's still straightforward to have a third argument after this. There's no reason not to do that but need to find how to do that cleanly.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding an argument is only one way to go about it that does not change the main comment. You can also use static method, etc.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is duplicate of #4525 which is already being handled. I don't know which will be merged first but I believe we can put this off for a while.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: New Feature 🚀 Increment minor version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants