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

$removeparam does not apply to document types by default #247

Closed
ghost opened this issue Jan 12, 2023 · 1 comment
Closed

$removeparam does not apply to document types by default #247

ghost opened this issue Jan 12, 2023 · 1 comment
Assignees
Labels
bug Something isn't working

Comments

@ghost
Copy link

ghost commented Jan 12, 2023

Hi @antonok-edm

I finally made an account to report these incompatibilities in the syntax, which make it impossible to load a list like the one @fmarier mentioned https://github.com/AdguardTeam/FiltersRegistry/blob/master/filters/filter_17_TrackParam/filter.txt, and don't have to modify it in order to use $removeparam.

For example, I noticed that in the PR to push $removeparam to stable brave/brave-browser#23927 that they used ||uni-verse.github.io^$removeparam=test to test it and get approved... I don't know why the $removeparam wasn't tested with more rules and not just the ONLY one that works.

Nothing else will work from the $removeparam list, unless the list is modified, and I am talking that the feature is not compatible with rules like:

generic remove param rules like:
$removeparam=famad_xuid

Or something like ||amazon.*$removeparam=at_link_id or ||bbc. that are made that way so it covers all domains from whatever company, just like happens with the ones for Yandex and Aliexpress as well.

And also it is not compatible with rules like ||mkaff.com/landings/$removeparam or ||alza.de/*.htm$removeparam=kampan or just anything in between.

The removeparam feature works great for the most part, but not when loading those lists because the majority of rules for some reason are expecting to have a $document to make them work (so it looks like example$document,removeparam), and without $document the rule is not going to work, unless it is just a normal simple one like ||example^

This is a big incompatibility for lists, and while I took the Adguard list and added $document to all the rules and they work fine, updating the lists and making sure to remove the ones already included in Query String Filter so it doesn't cause conflict (crashes) takes time.

Hope this helps to improve it so lists can work without problems or modifying anything.

@antonok-edm antonok-edm self-assigned this Jan 14, 2023
@antonok-edm antonok-edm added the bug Something isn't working label Jan 14, 2023
@antonok-edm
Copy link
Collaborator

@Emi-TheDhamphirInLoveUnderTheFrozenStar thanks for reporting.

uBlock Origin only implicitly adds $document to network rules with exact hostname patterns; rules with patterns that aren't just something like ||example.com^$ wouldn't apply to documents by default. Apparently it also adds $document to all removeparam rules too (as does AdGuard).

This should be fixed by 8a755bd, which I'll get released soon.

@antonok-edm antonok-edm changed the title $removeparam not compatible with many rules/lists. $removeparam does not apply to document types by default Jan 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant