Opened 4 months ago

Closed 3 months ago

Last modified 7 weeks ago

#6868 closed defect (fixed)

Rewrite filter with wildcard doesn't match end of URL

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

https://codereview.adblockplus.org/29863579/

Description (last modified by mjethani)

Environment

ABP development build as of changeset a87d009836da
All filter lists disabled

How to reproduce

  1. Add the filter &trackingId=*$rewrite= on the options page
  2. Load the HTML <iframe src="https://example.com/?foo=bar&trackingId=123"></iframe>
  3. Observe the network requests for the page in DevTools

Observed behaviour

The frame's URL is rewritten to https://example.com/?foo=bar123

Expected behaviour

The frame's URL is rewritten to https://example.com/?foo=bar

Additional notes

This happens because the wildcard at the end of the URL pattern is ignored since it is considered to be redundant. While this is true for most blocking filters, the wildcard should not be ignored for filters containing the $rewrite option.

Hints for testers

Check for the expected behavior above.

Additionally, make sure that blocking filters with no $rewrite= option, both with and without leading and trailing wildcards, are working. For example, *foo* should block http://example.com/foo/bar.

Change History (10)

comment:1 Changed 4 months ago by mjethani

  • Description modified (diff)

comment:2 Changed 4 months ago by mjethani

  • Cc jsonesen hfiguiere kzar added

comment:3 Changed 4 months ago by mjethani

The first question to answer here is whether we even need the optimizations for leading and trailing wildcards. I wrote this little test to find out:

(function()
{
  let a = new Array(1000000);
  for (let i = 0; i < a.length; i++)
    a[i] = "https://" + Math.random().toString(36).substring(2) + "/foo";

  let r = new RegExp("aa");
  let n = 0;

  let s = Date.now();
  for (let i = 0; i < a.length; i++)
  {
    if (r.test(a[i]))
      n++;
  }

  console.log(Date.now() - s, n);   
})();

If we change the regular expression from aa to .*aa.*, there is a significant negative effect on the performance. Indeed we should keep the optimizations.

comment:4 Changed 4 months ago by mjethani

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

comment:5 Changed 4 months ago by mjethani

  • Status changed from new to reviewing

comment:6 Changed 4 months ago by mjethani

  • Ready set

comment:7 Changed 3 months ago by abpbot

comment:8 Changed 3 months ago by mjethani

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

comment:9 Changed 3 months ago by mjethani

  • Description modified (diff)

comment:10 Changed 7 weeks ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Done. Working as described.

ABP 3.3.2.2175
Firefox 62 / 51 / Windows 10
Chrome 69 / 49 / Windows 10
Opera 56 / 36 / Windows 10

Note: See TracTickets for help on using tickets.