Specific redirect filter can't override generic redirection on 1.31.0

Yuki2718 commented 3 years ago

Prerequisites

[x] I verified that this is not a filter issue
- Filter issues MUST be reported at filter issue tracker
[x] This is not a support issue or a question
- Support issues and questions are handled at /r/uBlockOrigin
[ ] I performed a cursory search of the issue tracker to avoid opening a duplicate issue
- Your issue may already be reported.
I tried to reproduce the issue when...
- [x] uBlock Origin is the only extension
- [x] uBlock Origin with default lists/settings
- [x] using a new, unmodified browser profile
[x] I am running the latest version of uBlock Origin
[x] I checked the documentation to understand that the issue I report is not a normal behavior

Description

In older versions, you could override where to redirect by creating a specific filter vs. generic redirect filter. E.g. uBlock Privacy has a generic redirect filter ||googletagmanager.com/gtm.js$script,redirect=googletagmanager.com/gtm.js but you could override it by e.g. ||googletagmanager.com/gtm.js$important,script,redirect=noopjs,domain=tv-asahi.co.jp (taken from the same list) and IIRC $important was not mandatory. However on 1.31.0 it doesn't work any more.

A specific URL where the issue occurs

https://rocketnews24.com/

Steps to Reproduce

Visit the site with default setup (JP filter is not needed)
Scroll down and see some images were not loaded at first visit
Add @@||googletagmanager.com/gtm.js$script,domain=rocketnews24.com, clear cache & cookie, reload the page and see they're now loaded - but actually exception is not needed, redirecting to noop.js works.
Add ||googletagmanager.com/gtm.js$important,script,redirect=noop.js,domain=rocketnews24.com or ||googletagmanager.com/gtm.js$important,script,redirect=none,domain=rocketnews24.com and see the request still redirects to googletagmanager.com/gtm.js. Relaunch browser or whatsoever doesn't help, only way to make it work seems to be badfilter.

Expected behavior:

The specific redirect rule should override generic one.

Actual behavior:

Generic filter wins.

Your environment

uBlock Origin version: 1.31.0
Browser Name and version: Firefox 83.0
Operating System and version: Windows 10

gorhill commented 3 years ago

To neuter redirection, use an exception filter:

@@||googletagmanager.com/gtm.js$script,redirect-rule,domain=rocketnews24.com

To override the current redirection with another redirection, use a priority higher than 0:

||googletagmanager.com/gtm.js$script,redirect-rule=noop.js:10,domain=rocketnews24.com

Yuki2718 commented 3 years ago

So is this by design? Should we change past fixes such as ||googletagmanager.com/gtm.js$important,script,redirect=noopjs,domain=tv-asahi.co.jp too?

gorhill commented 3 years ago

important works also:

||googletagmanager.com/gtm.js$important,script,redirect-rule=noop.js,domain=rocketnews24.com

But it's preferable to use priority, so this leave more control to end users.

Yuki2718 commented 3 years ago

$important doesn't work on my end.

gorhill commented 3 years ago

By the way, there is a regression in dev build which causes the prioritized filter to not take effect, I will fix this.

uBlock-user commented 3 years ago

Should we start prioritising existing redirect-rule=none filters ?

gorhill commented 3 years ago

$important doesn't work on my end.

You're right, I will have to investigate. At least priority works, so this will have to be the fix -- which is preferable anyways as said above.

Yuki2718 commented 3 years ago

All right, closing. I'll add fixes for rocketnews and tv-asahi, but probably there're some more.

gorhill commented 3 years ago

Should we start prioritising existing redirect-rule=none filters ?

These will have to be replaced by the more appropriate exception filter:

@@||googletagmanager.com/gtm.js$script,redirect-rule,domain=...

Cancel the matching redirect-rule, i.e. block without redirecting.

Yuki2718 commented 3 years ago

Whoops, at least $important thing needs to be investigated.

uBlock-user commented 3 years ago

These will have to be replaced by the more appropriate exception filter:

Wouldn't adding a priority :10 work on redirect-rule=none ?

gorhill commented 3 years ago

Well important works in the dev build, not sure what went wrong in the stable build.

gorhill commented 3 years ago

Wouldn't adding a priority :10 work on redirect-rule=none ?

Why not just try it and report result?

uBlock-user commented 3 years ago

I did, since logger now reports both post refactorisation, can't tell which one gets applied on istripper.com --

||google-analytics.com/analytics.js$script,redirect-rule=none,domain=istripper.com,badfilter
||google-analytics.com/analytics.js$script,redirect-rule=none:10,domain=istripper.com

By both I mean the generic existing GA redirection and the redirect-rule=none:10 on istripper.com

gorhill commented 3 years ago

The last one is always the one taken, as confirmed by the yellow line just above it.

uBlock-user commented 3 years ago

Yeah, I saw that, it's google-analytics.com/analytics.js, so redirect-rule=none:10 is not getting priority and not getting applied as a result.

gorhill commented 3 years ago

important does not work in dev build either, it's just luck that it ended up being used, so this also needs fixing.

uBlock-user commented 3 years ago

Oddly, all other WAR resources are getting priority and even without adding :10(tested with noopjs, popads-dummy, nooptext etc), seems only none doesn't work.

gorhill commented 3 years ago

all other WAR resources are getting priority and even without adding :10

When they all have the same priority (default is 0), then which one will be used in the end is undefined, down to luck.

krystian3w commented 3 years ago

Possible add this into wiki https://github.com/uBlockOrigin/uBlock-issues/issues/1388#issuecomment-739989378, rare people read all uBO code (imo also no on releases noted cited).

gwarser commented 3 years ago

Is this right: https://github.com/uBlockOrigin/uBlock-issues/wiki/Static-filter-syntax/_compare/ae1df4fe0413d9b6c2232adfaa5670ff9dc9079b ?

This is not all, I should probably mention about xhr, % magic char and that redirection to data: resources requires must match mime type :/

https://github.com/gorhill/uBlock/commit/c6d0204b236449a8fe055c17c0b10af0446cb9ec#commitcomment-45025274

Yuki2718 commented 3 years ago

While :10 works for noop.js, not for doubleclick_instream_ad_status.js. Is this because the latter is URL-specific resource?

gorhill commented 3 years ago

You have steps to reproduce which demonstrate that priority does not work for doubleclick_instream_ad_status? uBO doesn't care about the name of a token to mind priority, so I don't see how there could be an issue with one resource while not for the other.

uBlock-user commented 3 years ago

Tested on ghacks with *$redirect-rule=doubleclick_instream_ad_status.js:10,script,3p,domain=ghacks.net working as expected -

![Capture](https://user-images.githubusercontent.com/21290713/102717968-816c6d00-430b-11eb-9deb-1705c100282b.PNG)

gorhill commented 3 years ago

I should probably mention about xhr, %

I can't remember why I put this capability in, could have been as a fall back in case of unforeseen issue when redirecting to web-resource-accessible. No need to document, I may just remove this eventually given there has been no use case for this yet.

Yuki2718 commented 3 years ago

URL (probably geo-restricted, use Japanese VPN): https://nba.rakuten.co.jp/videos/3683 (from https://github.com/easylist/easylist/pull/6730)

1st step: add @@||imasdk.googleapis.com/js/sdkloader/ima3.js$domain=nba.rakuten.co.jp otherwise the tab will freeze. And also add ||g.doubleclick.net/gampad/ads?sz=*nba.rakuten.co.jp$xhr,redirect-rule=doubleclick_instream_ad_status.js:10,domain=imasdk.googleapis.com - yes, xhr in this case.

2nd step: visit the page and play the video, see redirect doesn't occur. But occurs for noop.js.

gorhill commented 3 years ago

I can reproduce, I will need to investigate, unexpected.

uBlock-user commented 3 years ago

that redirection to data: resources requires must match mime type

That rule seems to be untrue, as the redirection does occur when noop.js is used for that XHR request.

gorhill commented 3 years ago

Ok, it's because doubleclick_instream_ad_status.js is not described as a resource which can be redirected through data: URI, unlike noop.js, I can change this.

gwarser commented 3 years ago

About %: it was added when XHR was fixed to allow redirect to binary resources https://github.com/gorhill/uBlock/commit/7ac7b027f4786847190cd51d745434c7623de392

gwarser commented 3 years ago

that redirection to data: resources requires must match mime type

That rule seems to be untrue, as the redirection does occur when noop.js is used for that XHR request.

This was based on https://github.com/gorhill/uBlock/commit/c6d0204b236449a8fe055c17c0b10af0446cb9ec#commitcomment-45025274

uBlockOrigin / uBlock-issues