Opened 21 months ago

Closed 17 months ago

Last modified 16 months ago

#6009 closed defect (fixed)

Whitelisting (via options) fails in ABP 3.0

Reported by: mapx Assignee: agiammarchi
Priority: P3 Milestone:
Module: User-Interface Keywords:
Cc: greiner, saroyanm Blocked By:
Blocking: Platform: Unknown / Cross platform
Ready: yes Confidential: no
Tester: Ross Verified working: yes
Review URL(s):

https://codereview.adblockplus.org/29693558/

Description (last modified by saroyanm)

Environment

windows 10
ABP 3.0.0.1913beta
FF 57

How to reproduce

go into options
Whitelisted websites
add website: https://www.google.co.uk/

Observed behaviour

Nothing happens, the domain is not whitelisted
(if I add only the domain google.co.uk ABP will work correctly)

Expected behaviour

Like the other method (while on the page / domain to be whitelisted) via clicking ABP icon and then click "Enabled on this site" ABP should extract the domain and add it to the whitelist

..or only the label (ADD WEBSITE => ADD DOMAIN) is wrong. However, would be better having the same behaviour as in the second case.

Specification

https://gitlab.com/eyeo/spec/issues/105

Change History (16)

comment:1 Changed 21 months ago by mapx

  • Description modified (diff)

comment:2 Changed 21 months ago by mapx

  • Cc greiner added

comment:3 Changed 21 months ago by trev

  • Component changed from Adblock-Plus-for-Firefox to User-Interface

comment:4 Changed 21 months ago by trev

  • Cc saroyanm added; sebastian mjethani removed

comment:5 follow-up: Changed 21 months ago by trev

The suggestion sounds reasonable to me. The logic here could detect invalid host names (at the very least, anything with slashes in it) and attempt to interpret them as full URLs (via new URL()). We need to show some kind of error message if it isn't a valid URL or host name is empty.

comment:6 in reply to: ↑ 5 Changed 21 months ago by greiner

Replying to trev:

The suggestion sounds reasonable to me. The logic here could detect invalid host names (at the very least, anything with slashes in it) and attempt to interpret them as full URLs (via new URL()). We need to show some kind of error message if it isn't a valid URL or host name is empty.

In the spec we haven't defined what that validation encompasses since we simply wanted to reflect the previous behavior as closely as possible which barely does any validation.

Note that there is already some error handling in place but it doesn't receive any errors so we first have to further look into where/why things are misbehaving.

Changing the spec to include normalization is definitely something we can do to improve this feature (I created a spec ticket for that) but for now the goal is to restore the currently specified behavior.

comment:7 follow-up: Changed 21 months ago by mapx

Another bug somehow very related:

As I said above when you add to the whitelisted section a normal link (not only the domain) like this 1:
http://edition.cnn.com/2017/11/08/entertainment/all-the-money-in-the-world-kevin-spacey-replaced/index.html

The filter does not disappear but you'll find it under your custom filters (as happens also in chrome).

The fact is the resulting filter is wrong !
The bug is in ABP for chrome too.

You'll see:
@@||http://edition.cnn.com/2017/11/08/entertainment/all-the-money-in-the-world-kevin-spacey-replaced/index.html^$document

Correct filter:
@@|http://edition.cnn.com/2017/11/08/entertainment/all-the-money-in-the-world-kevin-spacey-replaced/index.html^$document

There is a positive part: you could integrate this as the lost function from ABP legacy ("disable only on this page" (the filter produced this way is exactly that for the lost function)

comment:8 in reply to: ↑ 7 Changed 21 months ago by greiner

Replying to mapx:

There is a positive part: you could integrate this as the lost function from ABP legacy ("disable only on this page" (the filter produced this way is exactly that for the lost function)

I think you're right. We could make this feature quite a bit more powerful so I added it to the spec ticket.

comment:9 Changed 19 months ago by fhd

  • Cc trev removed

comment:10 Changed 17 months ago by agiammarchi

  • Owner set to agiammarchi

comment:11 Changed 17 months ago by agiammarchi

  • Review URL(s) modified (diff)

comment:12 Changed 17 months ago by agiammarchi

I am following these specifications: https://gitlab.com/eyeo/spec/issues/105

comment:13 Changed 17 months ago by saroyanm

  • Description modified (diff)
  • Priority changed from Unknown to P3
  • Ready set

Added the specification merge request.

comment:14 Changed 17 months ago by abpbot

A commit referencing this issue has landed:
Issue 6009 - Convert URLs into domains when adding to whitelist

comment:15 Changed 17 months ago by saroyanm

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

comment:16 Changed 16 months ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Fixed. URLs are converted into domains when added via the whitelist tab.

ABP 3.0.2.1998
Firefox 51 / 58 / Windows 10
Chrome 49 / 65 / Windows 7
Opera 36 / 51 / Windows 7

Note: See TracTickets for help on using tickets.