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

Specific redirect filter can't override generic redirection on 1.31.0 #1388

Closed
7 of 8 tasks
Yuki2718 opened this issue Dec 7, 2020 · 31 comments
Closed
7 of 8 tasks
Labels
bug Something isn't working fixed issue has been addressed

Comments

@Yuki2718
Copy link

Yuki2718 commented Dec 7, 2020

Prerequisites

  • I verified that this is not a filter issue
  • This is not a support issue or a question
  • 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...
    • uBlock Origin is the only extension
    • uBlock Origin with default lists/settings
    • using a new, unmodified browser profile
  • I am running the latest version of uBlock Origin
  • 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

  1. Visit the site with default setup (JP filter is not needed)
  2. Scroll down and see some images were not loaded at first visit
  3. 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.
  4. 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
@Yuki2718 Yuki2718 changed the title Can't override redirect resource Specific redirect filter can't override generic redirection on 1.31.0 Dec 7, 2020
@gorhill
Copy link
Member

gorhill commented Dec 7, 2020

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
Copy link
Author

Yuki2718 commented Dec 7, 2020

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
Copy link
Member

gorhill commented Dec 7, 2020

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
Copy link
Author

Yuki2718 commented Dec 7, 2020

$important doesn't work on my end.

@gorhill
Copy link
Member

gorhill commented Dec 7, 2020

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
Copy link
Contributor

uBlock-user commented Dec 7, 2020

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

@gorhill
Copy link
Member

gorhill commented Dec 7, 2020

$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
Copy link
Author

Yuki2718 commented Dec 7, 2020

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

@Yuki2718 Yuki2718 closed this as completed Dec 7, 2020
@gorhill
Copy link
Member

gorhill commented Dec 7, 2020

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
Copy link
Author

Yuki2718 commented Dec 7, 2020

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

@Yuki2718 Yuki2718 reopened this Dec 7, 2020
@uBlock-user
Copy link
Contributor

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

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

@gorhill
Copy link
Member

gorhill commented Dec 7, 2020

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

@gorhill
Copy link
Member

gorhill commented Dec 7, 2020

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

Why not just try it and report result?

Yuki2718 added a commit to uBlockOrigin/uAssets that referenced this issue Dec 7, 2020
@uBlock-user
Copy link
Contributor

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
Copy link
Member

gorhill commented Dec 7, 2020

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

gorhill added a commit to gorhill/uBlock that referenced this issue Dec 7, 2020
@uBlock-user
Copy link
Contributor

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
Copy link
Member

gorhill commented Dec 7, 2020

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 uBlock-user added the bug Something isn't working label Dec 7, 2020
@uBlock-user
Copy link
Contributor

uBlock-user commented Dec 7, 2020

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 added a commit to gorhill/uBlock that referenced this issue Dec 7, 2020
Related issue:
- uBlockOrigin/uBlock-issues#1388

Fixed the special `none` redirect resource no longer being
enforced.

Fixed the enforcement of `important` redirect rules over
exceptions and non-important ones.
@gorhill
Copy link
Member

gorhill commented Dec 7, 2020

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
Copy link

Possible add this into wiki #1388 (comment), rare people read all uBO code (imo also no on releases noted cited).

@uBlock-user uBlock-user added the fixed issue has been addressed label Dec 7, 2020
Yuki2718 referenced this issue in uBlockOrigin/uAssets Dec 9, 2020
@gwarser
Copy link

gwarser commented Dec 15, 2020

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 :/

gorhill/uBlock@c6d0204#commitcomment-45025274

@Yuki2718
Copy link
Author

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

@gorhill
Copy link
Member

gorhill commented Dec 20, 2020

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
Copy link
Contributor

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

Capture

@gorhill
Copy link
Member

gorhill commented Dec 20, 2020

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
Copy link
Author

Yuki2718 commented Dec 20, 2020

URL (probably geo-restricted, use Japanese VPN): https://nba.rakuten.co.jp/videos/3683 (from easylist/easylist#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
Copy link
Member

gorhill commented Dec 20, 2020

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

@uBlock-user
Copy link
Contributor

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
Copy link
Member

gorhill commented Dec 20, 2020

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.

gorhill added a commit to gorhill/uBlock that referenced this issue Dec 20, 2020
@gwarser
Copy link

gwarser commented Dec 20, 2020

About %: it was added when XHR was fixed to allow redirect to binary resources gorhill/uBlock@7ac7b02

@gwarser
Copy link

gwarser commented Dec 20, 2020

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 gorhill/uBlock@c6d0204#commitcomment-45025274

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working fixed issue has been addressed
Projects
None yet
Development

No branches or pull requests

5 participants