Allow empty values in filter options

[mjethani]

Background

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 https://example.com/?foo=bar&trackingId=123 as https://example.com/?foo=bar https://example.com/?foo=bar123 (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.

[mjethani]

@kzar @sebastian @hfiguiere does this sound OK?

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

[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).

[kzar]

Fine by me, sounds reasonable.

[hfiguiere]

sounds reasonable.

[abpbot]

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

[Ross]

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

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

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

[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?

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

[Ross]

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.

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

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

[mjethani]

Gotcha, you meant #6871.