Opened 19 months ago

Closed 18 months ago

Last modified 17 months ago

#6622 closed change (fixed)

Implement the $rewrite filter option — at Version 31

Reported by: hfiguiere Assignee: hfiguiere
Priority: P2 Milestone: Adblock-Plus-3.2-for-Chrome-Opera-Firefox
Module: Platform Keywords: circumvention
Cc: sebastian, kzar, Ross, mapx, arthur Blocked By: #6592, #6670
Blocking: #6242 Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29760707/

Description (last modified by 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.

Change History (31)

comment:1 Changed 19 months ago by hfiguiere

  • Review URL(s) modified (diff)
  • Status changed from new to reviewing

comment:2 Changed 19 months ago by hfiguiere

  • Keywords circumvention added

comment:3 Changed 19 months ago by hfiguiere

  • Blocking 6242 added

comment:4 Changed 19 months ago by hfiguiere

  • Description modified (diff)

comment:5 Changed 19 months ago by hfiguiere

  • Description modified (diff)
  • Review URL(s) modified (diff)

comment:6 Changed 19 months ago by 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.

comment:7 Changed 19 months ago by hfiguiere

  • Description modified (diff)

comment:8 Changed 19 months ago by hfiguiere

  • Description modified (diff)

comment:9 Changed 18 months ago by 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.

comment:10 Changed 18 months ago by hfiguiere

  • Description modified (diff)

comment:11 Changed 18 months ago by kzar

  • Cc sebastian kzar Ross added
  • Description modified (diff)
  • Priority changed from Unknown to P2

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.

comment:12 follow-up: Changed 18 months ago by kzar

  • Description modified (diff)

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

comment:13 Changed 18 months ago by kzar

  • Description modified (diff)
  • Summary changed from Implement rewrite filter option to Implement the $rewrite filter option

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

comment:14 Changed 18 months ago by kzar

  • Description modified (diff)

comment:15 Changed 18 months ago by kzar

  • Description modified (diff)

comment:16 Changed 18 months ago by kzar

  • Description modified (diff)

comment:17 in reply to: ↑ 12 ; follow-up: Changed 18 months ago by 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.

comment:18 Changed 18 months ago by hfiguiere

  • Blocked By 6670 added

comment:19 Changed 18 months ago by hfiguiere

  • Review URL(s) modified (diff)

UI specific issue filed as #6670 and made blocking.

comment:20 in reply to: ↑ 17 Changed 18 months ago by 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.

comment:21 Changed 18 months ago by hfiguiere

  • Description modified (diff)

comment:22 Changed 18 months ago by kzar

  • Ready set

comment:23 Changed 18 months ago by hfiguiere

  • Description modified (diff)

comment:24 Changed 18 months ago by hfiguiere

  • Description modified (diff)

comment:25 Changed 18 months ago by kzar

  • Description modified (diff)

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.

comment:26 Changed 18 months ago by hfiguiere

  • Description modified (diff)

comment:27 Changed 18 months ago by 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.

comment:28 Changed 18 months ago by abpbot

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

comment:29 Changed 18 months ago by sebastian

  • Milestone set to Adblock-Plus-for-Chrome-Opera-Firefox-next
  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:30 Changed 18 months ago by 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?

comment:31 Changed 18 months ago by hfiguiere

  • Description modified (diff)

Ah right. Removing.

Note: See TracTickets for help on using tickets.