Opened on 06/16/2017 at 05:17:40 PM

Closed on 07/13/2017 at 09:57:13 AM

Last modified on 08/02/2017 at 06:30:08 AM

#5325 closed change (fixed)

[abp2blocklist] Add proper support for separator characters

Reported by: mjethani Assignee: mjethani
Priority: P4 Milestone:
Module: Platform Keywords: abp2blocklist, Acceptable Ads
Cc: kzar, sebastian, mario Blocked By:
Blocking: #5464 Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29467595/

Description (last modified by mjethani)

Background

Currently abp2blocklist will convert example.co^ into example\.co and example.co^hello into example\.co.hello. This will match example.co as well as example.com, which is wrong. Based on the filters documentation, the correct patterns to generate are example\.co([^-_.%A-Za-z0-9].*)?$ and example\.co[^-_.%A-Za-z0-9]hello respectively.

What to change

Modify the parseFilterRegexpSource function to generate the correct pattern.

Attachments (0)

Change History (12)

comment:1 Changed on 06/16/2017 at 05:21:31 PM by mjethani

  • Description modified (diff)

comment:2 Changed on 06/16/2017 at 05:22:36 PM by mjethani

  • Cc kzar sebastian mario added

comment:3 Changed on 06/16/2017 at 05:23:57 PM by mjethani

  • Description modified (diff)

comment:4 Changed on 07/07/2017 at 12:33:04 PM by kzar

I think those regexp examples might be wrong, doesn't the . need to be escaped? E.g. example\.co([^\.%A-Za-z0-9_].*)?$

comment:5 Changed on 07/08/2017 at 06:06:19 AM by mjethani

The . must not be escaped inside square brackets.

comment:6 Changed on 07/08/2017 at 06:07:26 AM by mjethani

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

comment:7 Changed on 07/08/2017 at 06:09:21 AM by mjethani

  • Description modified (diff)

I found that we can deal with the - after all. I've updated the description accordingly.

comment:8 Changed on 07/08/2017 at 06:10:49 AM by mjethani

  • Description modified (diff)

comment:9 Changed on 07/11/2017 at 04:16:56 PM by kzar

  • Priority changed from Unknown to P4
  • Ready set

comment:10 Changed on 07/13/2017 at 09:55:01 AM by abpbot

A commit referencing this issue has landed:
Issue 5325 - Add support for separator characters

comment:11 Changed on 07/13/2017 at 09:57:13 AM by mjethani

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

comment:12 Changed on 08/02/2017 at 06:30:08 AM by mjethani

  • Blocking 5464 added

Add Comment

Modify Ticket

Change Properties
Action
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.