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

uBO should not parse filters inside an invaild !#if directive #2577

Closed
8 tasks done
MasterKia opened this issue Apr 2, 2023 · 14 comments
Closed
8 tasks done

uBO should not parse filters inside an invaild !#if directive #2577

MasterKia opened this issue Apr 2, 2023 · 14 comments
Labels
enhancement New feature or request fixed issue has been addressed

Comments

@MasterKia
Copy link
Member

MasterKia commented Apr 2, 2023

Prerequisites

  • I verified that this is not a filter list issue. Report any issues with filter lists or broken website functionality in the uAssets issue tracker.
  • This is not a support issue or a question. For support, questions, or help, visit /r/uBlockOrigin.
  • I performed a cursory search of the issue tracker to avoid opening a duplicate issue.
  • The issue is not present after disabling uBO in the browser.
  • I checked the documentation to understand that the issue I am reporting is not normal behavior.

I tried to reproduce the issue when...

  • uBO is the only extension.
  • uBO uses default lists and settings.
  • using a new, unmodified browser profile.

Description

Similar but unrelated:
#1712 (comment)

As per uBO docs:

For the time being, only a single token is supported in a `!#if directive uBO supports only the following, and anything else gets ignored

gorhill: Unknown directives should be disregarded

As per AdGuard docs:

!#if (conditions)
rules_list
!#endif

!#if (adguard_app_android)
||example.org^$third-party
!#endif

! for all AdGuard products except AdGuard for Safari
!#if (adguard && !adguard_ext_safari)
||example.org^$third-party
domain.com##div.ad
!#endif

I use uBO's !#if !cap_html_filtering and !#if env_mobile directives in my list.
AdGuard doesn't have an equivalent as of now, they're planning on adding them:

AdguardTeam/AdguardBrowserExtension#2146

AdguardTeam/AdguardBrowserExtension#2259

So as a workaround for AdGuard I use:
!#if (!adguard_app_windows && !adguard_app_mac && !adguard_app_android && !adguard_ext_firefox) and !#if (adguard_app_android || adguard_app_ios)

I had expected uBO to ignore filters inside those, but apparently it doesn't.

A specific URL where the issue occurs.

github.com

Steps to Reproduce

  1. Add:
!#if (adguard_app_android || adguard_app_ios)
github.com##html
!#endif

Or

!#if (something_fake)
github.com##html
!#endif
  1. Go to: github.com

Expected behavior

The page shouldn't be hidden because the directive is invalid and uBO should just ignore filters inside an invalid directives (well, valid according to AdGuard).

Actual behavior

The page is hidden.

uBO version

1.48.3b3 dev

Browser name and version

Firefox 111

Operating System and version

.

@gorhill
Copy link
Member

gorhill commented Apr 2, 2023

It's by design, unknown !#if ... expressions always evaluate to true, as if the line is just a comment. If you do not want uBO to parse the block, use !#if !ext_ublock or !#if adguard:

!#if adguard
!#if (adguard_app_android || adguard_app_ios)
github.com##html
!#endif
!#endif

@MasterKia
Copy link
Member Author

MasterKia commented Apr 2, 2023

It's by design, unknown !#if ... expressions always evaluate to true, as if the line is just a comment.

If it is indeed considered a comment, then in my opinion a comment shouldn't be highlighted as a red, giving the impression that it's not a comment.
Screenshot_20230402_221540

@gorhill
Copy link
Member

gorhill commented Apr 2, 2023

You want me to have the syntax highlighter render it as a comment?

The bottom line is that unparseable directives should have a default which is to not filter out, otherwise uBO is just guessing, and you want that guessing to be false by default because it happens to match your filters, but there is absolutely no way to guarantee that the guessing will always please someone else's filters, it's just luck at the end. Because of this, the default is to do nothing, and nothing means not skipping lines in the filter list.

@gorhill
Copy link
Member

gorhill commented Apr 2, 2023

giving the impression that it's not a comment.

It is not a comment from uBO's point of view. It is a comment from any other blocker which do not parse !#if directives, and not enforcing unknown directives is consistent with not enforcing when directives are not supported.

Solution is simple, just wrap the block around a directive uBO understand.

@gorhill gorhill closed this as completed Apr 2, 2023
@MasterKia
Copy link
Member Author

MasterKia commented Apr 2, 2023

Edit: Okay, I had sent the following before your final reply arrived.


You want me to have the syntax highlighter render it as a comment?

I meant it should ideally be shown by its true nature:

as if the line is just a comment

Syntax highlighter rendering it as a invalid directive gives the wrong impression that down the road uBO will ignore whatever is thrown inside this directive.

If the syntax highlighter were to render it as a comment then I'd have known that I should not count on uBO ignoring this in the future.

@gorhill gorhill added the declined declined label Apr 2, 2023
@gorhill
Copy link
Member

gorhill commented Apr 2, 2023

You are missing that what you ask is essentially that you want uBO to be tailored to your state of mind when you craft your filters. You want uBO to guess false, then it's just a matter of time before somebody come here and ask to reverse the guess to true:

!#if (!adguard_app_android && !adguard_app_ios)
github.com##html
!#endif

Common sense dictate to not make a decision when not knowing what to do with a directive, and not making a decision means behaving as if the directive does not exist.

@MasterKia
Copy link
Member Author

MasterKia commented Apr 2, 2023

Point taken. I'm okay with limiting the invalid directive with !#if adguard if that's what it takes.


unknown !#if ... expressions always evaluate to true

You want uBO to guess false, then it's just a matter of time before somebody come here and ask to reverse the guess to true

You should look at it from a filter author's perspective that is not acquainted with uBO's undocumented inner-evaluation.

The filter author decides to limit a filter using directives, he certainly will not expect that the said limit might be ignored and the filter might be run at places he never intended it to.

Imagine if a programming language that only accepts conditions in the form of !#if condition {}, were to evaluate true for conditions in other undefined forms such as !#if (condition) {}; effectively setting a booby trap for those not familiar with this never-seen behavior.

then it's just a matter of time before somebody come here and ask to reverse the guess to true

I can't imagine the benefits it would have for that person.

@MasterKia
Copy link
Member Author

MasterKia commented Apr 2, 2023

and not making a decision means behaving as if the directive does not exist.

[Side note] Will that trigger this behavior?:

This causes uBO to fail to find a matching !#endif for !#if !env_testmobile, and in such case uBO considers the filter list to have processing directive errors in it and cancels pre-processing as a consequence -- by design.

I've been using those directives for a such long time, does that mean every directives were ignored all these time? Sigh.

MasterKia added a commit to MasterKia/PersianBlocker that referenced this issue Apr 2, 2023
@MasterKia
Copy link
Member Author

I decided to remove all AdGuard-specific directives from my list, for me it's not worth piling up lines of filters for this (with !#if adguard on top). I'll leave it up to AdGuard to improve compatibility with uBO (or not).

@gorhill
Copy link
Member

gorhill commented Apr 2, 2023

Why not just use !#include .. inside !#if adguard? uBO recognizes adguard so it will not parse the !#include and all AdGuard-specific filters will be grouped in one file.

!#if adguard
!#include adguard-filters.txt
!#endif

@gorhill
Copy link
Member

gorhill commented Apr 2, 2023

[Side note] Will that trigger this behavior?:

This means unbalanced !#if-!#endif will cancel pre-parsing. I explain in that thread why. Surely unbalanced !#if-!#endif is a rare occurrence and I don't see why you think this was affecting you.

@MasterKia
Copy link
Member Author

Why not just use !#include ..

I don't want to spend time maintaining the same set of verbatim filters with just different directives.

The issue here was just the tipping point for me to come to that realization.

@krystian3w
Copy link

krystian3w commented Apr 3, 2023

Please do not spoil the operation of this method: https://github.com/FiltersHeroes/PolishAnnoyanceFilters/blob/96d5f4c721f28688e1c1473a9400f9dc28178fe9/templates/PPB.template#L107-L110

! Adding a supplement list to uBlock Origin and AdGuard MV2/App for Windows&macOS&Android so users don't have to subscribe to it
!#if (!adguard_ext_android_cb && !adguard_app_ios)
!#include PAF_supp.txt
!#endif

I use it to exclude the list on the iPhone and iPad (AdGuard has not added an option to detect if someone has an active extension for Safari - Unfortunately they rely on { display: none !important } to make the filter incompatible when someone does not activate advanced filtering: https://adguard.com/en/blog/adguard-4-3-for-ios.html), and prospectively in the too-simple "Android Content Blocker for Samsung Internet and Ynadex Browser" 'plugin'.

In the past, there was an add-on for macOS Safari too (FiltersHeroes/PolishAnnoyanceFilters@d4510ae), but seemingly all of a sudden there was support for :style() syntax so that pages wouldn't break when hiding modals with blocked scrolling in CSS.

@MasterKia
Copy link
Member Author

This should be fixed in:
gorhill/uBlock@194354c

This commit should make uBO fully compatible with the !#if
directives found throughout AdGuard's filter lists.

@MasterKia MasterKia added enhancement New feature or request fixed issue has been addressed and removed declined declined labels Jul 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request fixed issue has been addressed
Projects
None yet
Development

No branches or pull requests

3 participants