Opened on 06/05/2018 at 02:54:30 PM

Closed on 07/12/2018 at 11:19:56 AM

Last modified on 08/21/2018 at 07:24:50 PM

#6733 closed change (fixed)

Allow empty values in filter options

Reported by: mjethani Assignee: mjethani
Priority: P3 Milestone:
Module: Core Keywords:
Cc: kzar, sebastian, hfiguiere, sergz, jsonesen, arthur Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

Description (last modified by mjethani)


The $rewrite option allows the filter author to replace a part of a URL with a different value. For example, /\/foo\b/$rewrite=/bar will replace /foo in the URL with /bar. The option can also be used to strip out a part of the URL. For example, /(.*)&trackingId=.*/$rewrite=$1 will strip out the &trackingId=123 part at the end of the URL. Unfortunately for stripping to work the URL pattern in the filter must be a regular expression, because the value of the option cannot be blank. If blank values were allowed in filter options, the same filter could be written as &trackingId=*$rewrite=.

Regular expression filters are slow because they are not indexed by keyword.

There's no good reason to require a URL rewriting filter to be a regular expression. In fact, there's no reason to specifically disallow blank values for options in general.

What to change

Modify the options regular expression to allow any option to contain a blank value.

As before, a blank value for the domain, sitekey, or csp option should result in an invalid filter. A blank value should be considered valid for the rewrite option.

Known limitations

The filter &trackingId=*$rewrite= only matches the URL up to the = and leaves out the rest of the text, despite the wildcard at the end (#6868).

Hints for testers

As an example, the filter &trackingId=*$rewrite= should rewrite as (see known limitations).

This affects filter options that take a value, such as domain, sitekey, csp, and rewrite. For the first three blank values should not be allowed (should result in an invalid filter), whereas for the last one a blank value should be considered valid. If no value is specified for any of these options (i.e. $rewrite rather than $rewrite=), it is still an invalid filter.

Attachments (0)

Change History (27)

comment:1 Changed on 06/05/2018 at 02:54:52 PM by mjethani

@kzar @sebastian @hfiguiere does this sound OK?

comment:2 Changed on 06/05/2018 at 02:58:52 PM by mjethani

  • Cc sergz added

comment:3 Changed on 06/05/2018 at 05:26:22 PM by kzar

I don't mind if we start allowing empty $rewrite option values, but I don't see the point of doing that for the $csp, $domain or $sitekey.

comment:4 Changed on 06/06/2018 at 12:42:38 PM by mjethani

@kzar I think we would update Filter.optionsRegExp to allow empty values but then in the filter class constructor we would ignore the value if it's an empty string (except in the case of rewrite).

comment:5 Changed on 06/06/2018 at 12:57:26 PM by kzar

Fine by me, sounds reasonable.

comment:6 Changed on 06/06/2018 at 12:58:10 PM by hfiguiere

sounds reasonable.

comment:7 Changed on 06/06/2018 at 12:58:23 PM by mjethani

  • Priority changed from Unknown to P3
  • Ready set

comment:8 Changed on 06/06/2018 at 01:11:55 PM by mjethani

  • Cc jsonesen added

comment:9 Changed on 06/07/2018 at 11:51:41 AM by mjethani

  • Owner set to mjethani
  • Review URL(s) modified (diff)

comment:10 Changed on 06/07/2018 at 11:51:56 AM by mjethani

  • Status changed from new to reviewing

comment:11 Changed on 06/07/2018 at 11:55:06 AM by mjethani

  • Description modified (diff)

comment:12 Changed on 06/07/2018 at 01:10:14 PM by arthur

  • Cc arthur added

comment:13 Changed on 07/12/2018 at 11:10:15 AM by abpbot

A commit referencing this issue has landed:
Issue 6733 - Allow empty values in filter options

comment:14 Changed on 07/12/2018 at 11:19:17 AM by mjethani

  • Description modified (diff)

comment:15 Changed on 07/12/2018 at 11:19:56 AM by mjethani

  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:16 Changed on 07/12/2018 at 11:24:52 AM by mjethani

  • Description modified (diff)

comment:17 Changed on 07/12/2018 at 11:25:33 AM by mjethani

  • Description modified (diff)

comment:18 Changed on 07/20/2018 at 03:01:03 PM by mjethani

  • Description modified (diff)

comment:19 Changed on 08/17/2018 at 10:36:58 AM by Ross

This seems to work in Firefox (62/61) but not in Chrome (68) in ABP

Filter: &trackingId=*$rewrite=
Page html: <img src="/files/image.png?foo=bar&trackingId=123">

comment:20 Changed on 08/20/2018 at 08:30:42 AM by mjethani

Ross, do you see the filter hit in the DevTools panel?

When there are multiple blocking filters that match a URL, only one of them is applied. This is a known limitation of the $rewrite option. You'll have to make sure that no other filters are matching the URL.

comment:21 Changed on 08/20/2018 at 10:58:03 AM by Ross

In Chrome I don't see the filter hit in the devtools panel. I've tried removing all filter lists and having just the $rewrite= filter active. Do you not see the same?

comment:22 Changed on 08/20/2018 at 11:39:52 AM by mjethani

I have Chrome 70 here and the latest build of ABP, not 3.2. Can you confirm that it works in the 3.3 development build? For me the filter is getting hit.

Note that there's an error in the testing hints: /files/image.png?foo=bar&trackingId=123 will get rewritten as /files/image.png?foo=bar&123 instead because the wildcard appears at the end.

comment:23 Changed on 08/20/2018 at 11:45:27 AM by mjethani

  • Description modified (diff)

comment:24 Changed on 08/20/2018 at 11:54:12 AM by mjethani

  • Description modified (diff)

comment:25 follow-up: Changed on 08/20/2018 at 01:51:18 PM by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Okay, so this wasn't working in my local Chrome install, but after testing it on the VMs it looks fine. After reinstalling my local Chrome it works fine there too. Sorry about that.

The other part mentioned, about other filter options not accepting empty values works as expected except for the $csp filter, which is now at #6781.

Chrome 68 / 55 / 49 / Windows 10
Firefox 61 / 55 / 51 / Windows 10

comment:26 in reply to: ↑ 25 Changed on 08/20/2018 at 03:56:02 PM by mjethani

Replying to Ross:

The other part mentioned, about other filter options not accepting empty values works as expected except for the $csp filter, which is now at #6781.

Did you mention the wrong issue number here? #6781 is for snippets.

comment:27 Changed on 08/20/2018 at 03:58:00 PM by mjethani

Gotcha, you meant #6871.

Last edited on 08/21/2018 at 07:24:50 PM by jsonesen

Add Comment

Modify Ticket

Change Properties
as closed .
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from mjethani.
Note: See TracTickets for help on using tickets.