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

ABP's $rewrite filter option #46

Closed
mapx- opened this Issue May 22, 2018 · 15 comments

Comments

Projects
None yet
7 participants

@mapx- mapx- changed the title ABP's $redirect filter option ABP's $redirect filter option - discussion May 22, 2018

@mapx- mapx- changed the title ABP's $redirect filter option - discussion ABP's $rewrite filter option - discussion May 22, 2018

@gwarser

This comment has been minimized.

Copy link
Member

commented May 22, 2018

Why not:

||server.com/content/*.m3u8?userId=*$rewrite=s/userId=\d+//

@gwarser gwarser added discussion and removed discussion labels May 22, 2018

@jspenguin2017

This comment has been minimized.

Copy link

commented May 23, 2018

So /(^https?:\/\/[^/])/$script,rewrite=$1.evil.com basically let you inject arbitrary script to every page?
This can go so wrong, I'd say just hard code the few special websites that need this option in uBO-Extra.

@gwarser

This comment has been minimized.

Copy link
Member

commented May 23, 2018

Allow only to delete parts of the url?

@gorhill

This comment has been minimized.

Copy link
Member

commented May 23, 2018

Copy/pasting here a comment I made yesterday in Teams after I pondered this for a while:


gorhill May 22, 2018, 10:19 AM EDT

I won't implementing this filter option, I see too many issues with it. I am however open to implement a different filter option with similar purpose, but which would not suffer the issues I see with how rewrite has been designed. However, I need to see more cases being solved by such filter option than the just one case mentioned. So far I don't see this.

My concerns:

Security: testing same origin for redirect URL is not enough: both github.com/gorhill/ and github.com/toto/ have the same origin, however the content of both URLs is not controlled by the same person. My hunch tells me this is not good.

Security: even with strictly same origin, a malicious filter list author could add bad stuff to a network request.

Performance: the rewrite= option requires that the filter is a regex. This is bad, as this prevents such filter from being tokenizable, and as a consequence the filter must be tested against every single network request. With uBO this can be somewhat mitigated (not with ABP) by using type option (ex. xmlhttprequest) and party-ness option (ex. third-party), but this is still be a concern for me.

Given these concerns, I see a better way to implement similar option but with a more focused purpose: to remove specific query parameters from a URL:

||content.uplynk.com/ext/*.m3u8?$querystrip=*

Where the querystrip option would mean: "remove all query parameters matching the given lists of tokens or pattern".

Sticking to remove query parameters takes care of the ownership and malicious filter list author issue for the most part -- the filter removes query parameters, it can never rewrite them into something else.

The performance concern no longer exist with such filter, since it does not have to be a regex. The value of the querystrip option dictates which query parameter must be removed. It could be * to remove all of them, or a |-separated list of tokens which tells which specific query parameters should be removed.

Now this does not remove some other concerns I see with rewrite=.

One is that it is designed as a block filter.

What if I really want to block using ||content.uplynk.com/ext/? The way uBO/ABP and all similar-purpose blockers is to stop trying to find a match when a hit is found. If the filter with the rewrite option is found first, the really blocking filter won't be found and the network request will go through. This can be addressed in uBO using the important filter option, but still, my gut tells me there is something wrong about a blocking filter which actually does not block -- I feel more thinking is needed there.

My current thinking is that a querystrip filter applies if and only if the network request was neither blocked or excepted. Not blocked because block filters must result in the network request not being made by the browser, and not excepted because the purpose of exception filters is strictly to counter block filters. So if a network request goes through both block/exception filters unscathed, than it would be fair game for further handling with a querystrip option.

Anyway, as said I still need more than just one case to be an argument for such filter -- the last thing I want is to add technical debt to uBO for little tangible benefits overall. Note that a site could simply convert their GET request into a POST one and this would bypass both rewrite= or querystrip= filters -- so far the case for such filter options is thin.

@gwarser

This comment has been minimized.

Copy link
Member

commented May 23, 2018

remove specific query parameters

utm_* stuff (more like privacy and tracking)

querystrip may be not enough.
Apart from the POST requests - some pages use path to encode query parameters, for example amazon1:
amazon
And some other pages use hash 2, 3 (not even handled by network code).

@gwarser

This comment has been minimized.

Copy link
Member

commented May 23, 2018

Also ABP code can handle https://server.com/content/foo-with-ads.m3u8 -> .../foo.m3u8

This can go so wrong, I'd say just hard code the few special websites that need this option in uBO-Extra.

👍

and other cases (utm_*) can be handled by specialized URL redirecting extensions.

@jspenguin2017

This comment has been minimized.

Copy link

commented May 23, 2018

@gorhill

Yea, querystrip sounds way better.

Also I don't see how the new ABP build can get pass AMO review since it allow injection of arbitrary script. (Or I'm missed something?)

@gwarser other cases (utm_*) can be handled by specialized URL redirecting extensions.

The background script of uBO-Extra can be made configurable (unlike the content script), so we can include a switch to enable / disable part of its URL rewriting rules.

@gwarser

This comment has been minimized.

Copy link
Member

commented May 23, 2018

One is that it is designed as a block filter.

How to make this exception-only option? Request can be blocked by one filter, and then repaired by @@...$querystrip=*.

@gorhill

This comment has been minimized.

Copy link
Member

commented May 23, 2018

then repaired by @@...$querystrip=*

Exception filters also obey the first-match rule. You could end up with another exception filter such that the querystrip filter is not seen, and you also would be unable to except such filter if needed if it is found to cause issue on one site. Hence the need to lookup such filters after having both gone through block/exception filters and finding that none applied before caring about querystrip filters. This means added overhead overall since in basic settings most network requests are neither blocked or excepted.

A scriptlet for that specific purpose seems to be the right approach (an XMLHttpRequest wrapper to remove query parameters), it's injected only on sites where it's needed, can be excepted, and adds no global overhead to network request handling.

@hfiguiere

This comment has been minimized.

Copy link

commented May 23, 2018

Also I don't see how the new ABP build can get pass AMO review since it allow injection of arbitrary script. (Or I'm missed something?)

No it doesn't inject arbitrary scripts.

@jspenguin2017

This comment has been minimized.

Copy link

commented May 23, 2018

@hfiguiere What mechanism it has to prevent #46 (comment) from happening?

@hfiguiere

This comment has been minimized.

Copy link

commented May 23, 2018

The example in #46 (comment) change the origin, so the rewrite doesn't happen in that case, and the original query is let through.

@jspenguin2017

This comment has been minimized.

Copy link

commented May 23, 2018

@hfiguiere OK, so what about rewriting an URL from RawGit? You can control the content served from RawGit pretty easily, and all files served from RawGit are under cdn.rawgit.com origin. All apps using RawGit CDN can be attacked.

@jspenguin2017

This comment has been minimized.

@uBlock-user uBlock-user changed the title ABP's $rewrite filter option - discussion ABP's $rewrite filter option Apr 16, 2019

@rain-1

This comment was marked as off-topic.

Copy link

commented Apr 17, 2019

Can i just give you a huge thank you for building and maintaining this amazing addon. You're wonderful

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.