Opened 16 months ago

Closed 14 months ago

Last modified 13 months ago

#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):

https://codereview.adblockplus.org/29801609/

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

Change History (27)

comment:1 Changed 16 months ago by mjethani

@kzar @sebastian @hfiguiere does this sound OK?

comment:2 Changed 16 months ago by mjethani

  • Cc sergz added

comment:3 Changed 16 months ago 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 16 months ago 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 16 months ago by kzar

Fine by me, sounds reasonable.

comment:6 Changed 16 months ago by hfiguiere

sounds reasonable.

comment:7 Changed 16 months ago by mjethani

  • Priority changed from Unknown to P3
  • Ready set

comment:8 Changed 16 months ago by mjethani

  • Cc jsonesen added

comment:9 Changed 16 months ago by mjethani

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

comment:10 Changed 16 months ago by mjethani

  • Status changed from new to reviewing

comment:11 Changed 16 months ago by mjethani

  • Description modified (diff)

comment:12 Changed 16 months ago by arthur

  • Cc arthur added

comment:13 Changed 14 months ago by abpbot

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

comment:14 Changed 14 months ago by mjethani

  • Description modified (diff)

comment:15 Changed 14 months ago by mjethani

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

comment:16 Changed 14 months ago by mjethani

  • Description modified (diff)

comment:17 Changed 14 months ago by mjethani

  • Description modified (diff)

comment:18 Changed 14 months ago by mjethani

  • Description modified (diff)

comment:19 Changed 13 months ago by 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">

comment:20 Changed 13 months ago 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 13 months ago 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 13 months ago 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 13 months ago by mjethani

  • Description modified (diff)

comment:24 Changed 13 months ago by mjethani

  • Description modified (diff)

comment:25 follow-up: Changed 13 months ago 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.

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

comment:26 in reply to: ↑ 25 Changed 13 months ago 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 13 months ago by mjethani

Gotcha, you meant #6871.

Last edited 13 months ago by jsonesen (previous) (diff)
Note: See TracTickets for help on using tickets.