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

Network filter toString fix #3681

Merged
merged 1 commit into from
Jan 9, 2024
Merged

Network filter toString fix #3681

merged 1 commit into from
Jan 9, 2024

Conversation

chrmod
Copy link
Member

@chrmod chrmod commented Jan 5, 2024

I've run into a weird edge case. NetworkFilter#toString() adds a right anchor to filters ending with a separator.

  1) Network filters
       toString
         pprint anchored hostnames:

      AssertionError: expected '||foo.com^|' to equal '||foo.com^'

This may potentially be correct, but some other part of our toolchain (abp2dnr) does not handle the right anchors and thus ends up with a broken filter (as DNR does not support right anchor either, afaict).

Anyways, resulting syntax ^| is extremely rare, I found only one filter using it in both uAssets and Easylist.

Can we make this change?

@chrmod chrmod requested a review from remusao as a code owner January 5, 2024 17:28
@remusao
Copy link
Collaborator

remusao commented Jan 6, 2024

I think it's this case:

// If the only symbol remaining for the selector is '^' then ignore it

Can you share an example of filter where you encountered the issue?

checkToString('@@||foo.com', '@@||foo.com^');
checkToString('@@||foo.com|', '@@||foo.com^|');
checkToString('@@||foo.com|', '@@||foo.com^');
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think the semantics of this is a bit different now. Although I think it might not be 100% correct in the previous case either. I vaguely remember that this case was a little bit hacky.

Copy link
Member

Choose a reason for hiding this comment

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

uBlock Origin team replied me with a clarification: | is used to match start/end of address.

||hostname^|  => http://example.com/ & not http://example.com/blablabla

Many thanks to @ ghajini from uBO.

@remusao remusao added the PR: Polish 💅 Increment patch version when merged label Jan 6, 2024
@remusao
Copy link
Collaborator

remusao commented Jan 6, 2024

If I remember correctly this combination of flags was used in order to implicitly detect cases like ||foo.com^ (hostname match with no selector) and avoid having to create a specific flag for this. Then we detect and adapt matching logic here:

if (filter.hasFilter() === false) {

Maybe the better way would be to introduce a flag for that and remove the hack. It would also allow to have a 100% correct toString() implementation. But I think your tweak also makes sense as long as it does not break other cases you care about.

@seia-soto
Copy link
Member

I'm leaving a note during investigation on ^| syntax. At the first time, I thought this should be considered as an invalid filter generally.

Also:

Search result on uBO/EL image image

However, there's actually a use of ^| syntax which is not documented.

seia-soto added a commit to seia-soto/adblocker that referenced this pull request Jan 9, 2024
@seia-soto
Copy link
Member

seia-soto commented Jan 9, 2024

If we are going to process | and ^| differently, we also need to fix the following case: '||foo.com^$3p' resolves to '||foo.com^|$3p'.

seia-soto@f82f0a1

Also, see #3681 (comment)

@chrmod chrmod merged commit 13eebe4 into master Jan 9, 2024
3 checks passed
@chrmod chrmod deleted the network-filter-tostring-fix branch January 9, 2024 14:58
seia-soto added a commit to seia-soto/adblocker that referenced this pull request Jan 10, 2024
remusao pushed a commit that referenced this pull request Jan 10, 2024
* feat: modifier replacer for NetworkFilter.toString

To make minimal performance impact on current codes using adblocker library, I chose to use Array.prototype.map function instead of putting `if` statements or using empty proxying function: `(str: text) => str` as default value.

refs #3693

* chore: fix types

use `as const` keyword

* chore(test): fix test output

refs #3681

* fix(test): test expectation to match latest commit

refs #3681
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Polish 💅 Increment patch version when merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants