Implement the $rewrite filter option

[hfiguiere]

Background

Sometimes it's desirable to redirect a request, instead of blocking it entirely. For example perhaps the website detects when requests fail, or perhaps a request is necessary but its URL has tracking information appended which would be desirable to strip. For this we're adding the $rewrite filter option. In #6592 we introduced the option, and here we're going to make use of it.

Take an example $rewrite filter:

/(server\.com\/content\/.*\.m3u8)\?.*$/$rewrite=$1

A request to this URL:

https://server.com/content/foo.m3u8?userId=12345

Would be redirected to would be redirected to

https://server.com/content/foo.m3u8

• The syntax for the rewrite option is the same as ​for Regexp.replace.
• A request will only be redirected if the rewritten URL is of the same origin, yet different to the original URL. In other words a request to https://foo.com/bar could not be redirected to https://bar.com/foo nor https://foo.com/bar but it could be redirected to https://foo.com/foo.

What to change

• Update the adblockpluscore dependency to hg:e59042524857 git:8904bce in order to include the changes for #6592. That will also include:
  • Issue #6437 - Filter out patterns that do not match DOM mutations
  • Issue #6610 - Prefer inline style for :-abp-properties()
  • Issue #6649 - Allow u flag in :-abp-contains()
  • Issue #6652 - Do not push to unconditional selectors array
  • Issue #6592 - Implement $rewrite filter option
• Implement the rewrite in requestBlocker.js by calling the function for core that will perform it. This is handled as an exception of blocking in the same handler.

Hint for testers

• Test that regular request blocking still works.
• Test filters with rewrite option. The developer tool panel relies on changes for issue #6670 that may not have landed at the time of writing.
  • Basic rewrite: add filter /(testpages\.adblockplus\.org\/css\/testpages\.css)\?14$/$rewrite=$1?42 and visit ​https://testpages.adblockplus.org/en/testcases/css/03 . You should notice the rewrite in the Adblock Plus developer tool panel.
  • Rewrite to relative URL. Should succeed. Add filter /testpages\.adblockplus\.org(\/css\/testpages\.css)\?14$/$rewrite=$1?42 (and make sure the previous filter is removed). You should notice the rewrite in the Adblock Plus developer tool panel.
  • Rewrite to different origin: Should fail and the original request let through. Add filter /testpages\.adblockplus\.org(\/css\/testpages\.css)\?14$/$rewrite=bogus.adblockplus.org$1?42 (and make sure the previous filter is removed). You should notice the rewrite in the Adblock Plus developer tool panel, showing that the rewritten URL is the same as the original one.
• Ensure that the rewrites are logged accordingly into the developer tools.
• Test other changes from core that might have an impact:
  • Test that :-abp-contains() still works, including with Unicode (details in issue #6649)
  • Check that element hiding emulation filters using :-abp-properties() hide elements using inline "style" attributes.
  • Check that element hiding emulation filters on dynamic content gets reapplied when applicable.

[greiner]

For reference: I've created ​gitlab#76 to come up with a simple design based on the one provided via the review.

While that's being worked on, we can continue the review with the existing code.

[kzar]

Please can you expand on the introduction to explain what the idea of the $rewrite option is and to include an example filter with an explanation of what it does?

Please can you improve the "Hints for testers" section? For example you say "Test filters with rewrite option" without giving any examples or saying how they are expected to work.

[kzar]

Thanks for updating the test instructions. I've fleshed out the description a bit more for you and once the dependency update details are in I'll mark this as ready.

[kzar]

Please could you populate the issue number for the adblockplusui changes to the developer tools? (Both in the Blocked By field and the description.)

[kzar]

What about if a website redirects the user from the rewritten URL back to a URL that will be rewritten again?

[hfiguiere]

Replying to kzar:

Please could you populate the issue number for the adblockplusui changes to the developer tools? (Both in the Blocked By field and the description.)

I think that mean I have to file it. Because the UI change is currently attached this issue.

[hfiguiere]

UI specific issue filed as #6670 and made blocking.

[kzar]

Replying to hfiguiere:

I think that mean I have to file it. Because the UI change is currently attached this issue.

Ah I see. Yes, you'll need to do that since unfortunately we have to have separate issues for changes to different modules. Otherwise things like testing of changes, milestones and dependency updates get extremely confusing. In the past this has led to regressions.

[kzar]

So far it wasn't too clear that the list of issues you posted below the dependency update bullet point was for the incidental adblockpluscore changes included in the dependency update. I've reworded that so it's a bit clearer.

Please could you go through those changes and figure out which are relevant and provide more information? Please could you also add information in the "Hints for testers" section for any incidental changes that touch user facing code? For an example of what I mean see #5241.

[kzar]

Thanks the issue is looking pretty good now, we're mostly just waiting on the adblockplusui dependency update (both in this issue description and the codereview) now I suppose.

[abpbot]

A commit referencing this issue has landed:
​Issue 6622 - Implement $rewrite filter option

[kzar]

I don't see the adblockplusui dependency update in the change. Hubert if you decided to land do that dependency update separately please could you update this issue description to remove the part about updating the adblockplusui dependency and also unmark this issue as being blocked by #6670?

[hfiguiere]

Ah right. Removing.

[kzar]

Thanks but this issue is still marked as being blocked by #6670. Please could you fix that?

[hfiguiere]

Issue #6670 is marked as fixed too. I was told (by sebastian) that if it is in UI branch it is to be marked as fixed.

[kzar]

Issue #6670 being marked fixed or not is something else. I'm asking you to remove #6670 from the "Blocked By" field of this issue.

[mapx]

Some thoughts regarding $rewrite from gorhill (R. Hill) - uBo's creator:

​https://github.com/uBlockOrigin/uBlock-issues/issues/46#issuecomment-391303700

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 aseparated 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.

[kzar]

Thanks for posting that mapx.

Well it seems like Gorhill makes a pretty good point about the security concerns with $rewrite. Perhaps we should remove the filter option entirely, or adjust it before the release.

[kzar]

(I wonder if $rewrite could even be used to redirect filter subscription requests?!)

[sebastian]

Replying to kzar:

(I wonder if $rewrite could even be used to redirect filter subscription requests?!)

Nope, since requests sent by Adblock Plus itself are ignored by the filter matching code.

[mjethani]

Replying to mapx:

Some thoughts regarding $rewrite from gorhill (R. Hill) - uBo's creator:

​https://github.com/uBlockOrigin/uBlock-issues/issues/46#issuecomment-391303700

[...]

Raymond raises some valid points, but let me address where he's factually wrong or where I just disagree:

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.

Fair enough, but if we're concerned about the same origin being controlled by "different people", as is the case many times, we can't have URL rewriting. I understand and appreciate the concern here.

Let's take this example:

<script src="https://raw.githubusercontent.com/alice/good.js"></script>

If a filter list author rewrote that path to /eve/bad.js, and the script ended up doing something malicious with the user's data on the site, then that would be a problem.

From the user's point of view, let's see all the relevant entities the user would be trusting in this case:

1. Naturally the user trusts the document that contains the above HTML tag, let's call it photos.example.com
2. By extension, the user trusts raw.githubusercontent.com
3. By extension, the user trusts /alice
4. The user trusts the filter list

Ideally photos.example.com here would use ​subresource integrity or a ​content security policy, but we don't live in an ideal world.

But is this really a concern in practice? All the top websites that I use personally have sorted this out by now. For other websites of less importance, if they haven't got this together already, I doubt that malicious filter list authors would be able to make things any worse. But I may have got this wrong so it might be worth looking into.

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

If this "bad stuff" is different from what is mentioned above, I'd love to see an example.

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.

Corrections:

• Filters with the $rewrite option don't have to be regular expression filters (e.g. trackingId=^$rewrite=&), strictly speaking (but they may well be in practice most of the time, though certainly not in all cases)
• Unless I have missed something, the content type (e.g. $stylesheet) and the "party-ness" still applies to these filters in ABP

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".

The query string being different than the path in a URL is entirely meaningless from a security point of view. For security purposes, the query string is part of the path.

So you might just want to call it strip instead and include the stripping of any part of the path plus query string.

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 aseparated list of tokens which tells which specific query parameters should be removed.

If this going to be limited to query strings it's not going to be very useful, you might as well not do it at all. But I like the idea of only stripping components of a URL.

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.

We discussed this, the fact that if you have a real blocking filter and a rewrite filter that matches the URL, either one of them may be applied. We can address this if it becomes a real issue. For now what this means though is that some of the performance concerns raised above are not valid for ABP, we're doing something for $rewrite only if there's a match.

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.

This is not relevant for ABP for now, because either the request is blocked or the URL rewritten (i.e. they're mutually exclusive), and an exception may apply in either case.

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.

[mjethani]

Replying to mjethani:

Replying to mapx:
[...]

If this going to be limited to query strings it's not going to be very useful, you might as well not do it at all. But I like the idea of only stripping components of a URL.

On further thought, I think this is not a good idea.

Let's see why:

1. https://example.com/alice/foo.js?showAds=1&v=1.2 can easily be changed to https://example.com/alice/foo.js:showAds=1;v=1.2, so the query stripping won't work, which means you would then have to extend this to the path component of the URL
2. In the path component, only / is a special character, while characters like :, ; and so on have no special meaning, and you cannot predict exactly how the site will choose to encode the query string as part of the path itself (I used : for ? and ; for & only as an example)
3. Since you can't define what a "subcomponent" of the path is (other than to define it as whatever occurs between two /'s), basically you have to give filter authors the flexibility to use patterns (regular expressions)
4. If you still insist on only allowing filter authors to strip out sections of the path (given the preceding), you still have the security issue when /jonathan/script.js is stripped so it becomes /jon/script.js

In other words, there's no way to do URL rewriting in a safe way that is also effective. If you want to be effective, you have to accept the risk. It's a choice.

[hfiguiere]

I was looking into the concerns that gorhill has about the possibility of different entities controlling part that are considered as same origin that could cause a security risk and I don't see an easy technical solution ; there could be blacklist of domains where we can't rewrite (e.g blacklist rewrites on github.com), but even then it is not practical.

I agree with Manish arguments, these concerns may represent a risk, a concious choice we make.

One thing we could do is check $rewrite rules in the known subscriptions (like EasyList) to keep an eye on them.

[kzar]

I think Gorhill's right and it seems like no one disagrees. If one GitHub repository / Gist gets redirected to another with a very similar name and content I likely wouldn't realise.

Replying to mjethani:

In other words, there's no way to do URL rewriting in a safe way that is also effective. If you want to be effective, you have to accept the risk. It's a choice.

Replying to hfiguiere:

I agree with Manish arguments, these concerns may represent a risk, a concious choice we make.

Right, and surely the choice has to be doing what's safe for the user.

I think we should remove the $rewrite option.

[sebastian]

Let's sum up the attack surface we are talking about:

• An attacker would have to get a malicious filter either into a default filter list or trick the user into subscribing to a malicious third-party filter list (which the user has to take a deliberate action for).
• A malicious $rewrite filter CANNOT redirect ...
  • top-level documents.
  • requests sent by Adblock Plus (e.g. filter list downloads) or other extensions.
  • any request to a different origin/domain.
• The worst case scenario appears to be a sub-resource (e.g. an image), on a website with user-contributed content (e.g. GitHub), being replaced with a resource published by a different user.

While this isn't ideal, it might be a manageable risk if there is a strong case for such a filter. However, so far I only have seen one real-world example where the $rewrite filter option would help to effectively block ads. Manish & Hubert, can you please elaborate (and ideally provide some data) on the impact of the $rewrite filter option to counter circumvention across the web?

[hfiguiere]

Replying to sebastian:

While this isn't ideal, it might be a manageable risk if there is a strong case for such a filter. However, so far I only have seen one real-world example where the $rewrite filter option would help to effectively block ads. Manish & Hubert, can you please elaborate (and ideally provide some data) on the impact of the $rewrite filter option to counter circumvention across the web?

I don't think there is a case strong enough to introduce that risk. The query string rewrite could still be performed though and be useful, but that requires a rework.

In order to not delay the release freeze any further, I have submitted a backout patch for the webextension: ​https://codereview.adblockplus.org/29789571/ - shall I file a new issue for it or just attach to this one ? (Also the adblockpluscore change should be backed out as well)

[cjxlist]

$rewrite is very useful to skip video ads or to avoid warning messages on some websites in China,for now.See EasyList China ​https://easylist-downloads.adblockplus.org/easylistchina.txt Already have $rewrite filters to skip video ads on ​http://www.iqiyi.com/ to avoid warning messages on ​https://www.mgtv.com/ ​https://v.qq.com/ ​http://www.dnvod.tv/
These websites are very popular in china,could you releasing an ABP officials version ASAP?
As an maintainer of EasyList China,I had received tons of issue reports about these websites,see
Thanks.

[kzar]

We've been discussing this a little bit in IRC today, here's a demonstration of my fear:

1. Add the filter /(github\.com\/adblockplus\/)adblockpluscore(\/.*)/$rewrite=$1adblockplusui$2
2. Browse to ​https://github.com/adblockplus/adblockpluscore/
3. Click around the project, for example on the commit list.

You're silently taken from the adblockplusui project to the adblockpluscore project. Scary IMO!

[sebastian]

This is an interesting example. What's happening there is that an XHR is redirected, which then causes the top-level address to change (through the history API), even though Adblock Plus itself wouldn't touch/redirect requests loading any top-level document directly.

For reference, another example (brought up by Manish), is that a script delivered by a CDN, could be replaced with another script on the same CDN (which could potentially include malicious code uploaded just for that purpose by an attacker).

Perhaps, we can mitigate these attacks by ignoring $rewrite filters if the request type is xmlhttprequest or script? Would this still be sufficient to tavkle current circumvention?

[arthur]

I would assume in cases like #6242 (to get around the video ad), it is mostly a xmlhttprequest responsible for handling the ad and content delivery. Not sure how it looks like on Chinese sites.

[sebastian]

As discussed on IRC, we will ignore requests of the type script, sub_frame, object and object_subrequest for $rewrite filters. This mitigates the worst case (i.e. malicious filters replacing scripts loaded from a CDN with arbitrary code from the same CDN). I filed issue #6704.

Unfortunately, there doesn't seem to be a mitigation for abusing the $redirect filter option with XHR requests (like demonstrated in comment:48), without rendering the filter option useless, but this scenario seems less critical.

[cjxlist]

Replying to sebastian:

ignore requests of the type script, sub_frame, object and object_subrequest for $rewrite filters. Unfortunately, there doesn't seem to be a mitigation for abusing the $redirect filter option with XHR requests

Sadly can't skip pre-roll ads on iqiyi.com( ​https://www.iqiyi.com/v_19rrd7pl7c.html) A filter /(\.iqiyi\.com)\/show2\?e=./$rewrite=$1/show2?e=. in EasyList China that redirect script(ABP dev panel) isn't working anymore. 105 issue reports about iqiyi in the past 15 days that I can search, and iqiyi issue have been there 4 years.Thousands of issue reports about it.Fortunately a spare filter is available to redirect XHR,but have to wait about 30 sec to watch videos.

Sadly can't avoid warning message on youku.com too. A filter /(\.youku\.com\/ups\/get\.json\?.*?)&os=Windows&osv=.*?&d=0&bt=pc&/$rewrite=$1&os=android&bt=phone&dq=hd2& in EasyList China that redirect object(ABP dev panel) isn't working anymore. Fortunately just not working on a few "youku only" and "IP limited" videos, still using flash player(​http://v.youku.com/v_show/id_XNTQ2NjcyMDAw.html)

How about unlimited $rewrite if $rewrite filters in full trusted filterlists? Such as EasyList China.
or How about unlimited $rewrite if $rewrite filters match a few Domain Name?Such as iqiyi.com youku.com mgtv.com qq.com ...

[Ross]

This has been implemented and looks to be working as expected. The child tickets relating to :-abp filters also look to be working as expected.

ABP 3.1.0.2069
Chrome 67 / 64 / 49 / Windows 7
Firefox 60 / 55 / 51 / Windows 7
Opera 52 / 45 / 38 / Windows 7