Opened 14 months ago

Closed 14 months ago

Last modified 14 months ago

#7043 closed change (fixed)

Accept whitelist filters with blank CSPs

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

https://codereview.adblockplus.org/29911604/

Description (last modified by mjethani)

Background

In #6871 we stopped accepting filters with blank CSPs. But there are such filters in the anti-circumvention list, they just happen to be whitelist filters. In the current version development build of Adblock Plus these filters are being considered invalid because of the change for #6871.

It makes sense to allow blank CSPs in whitelist filters, because the value itself has no meaning (the fact that the filter matches the URL and has the $csp option is enough for whitelisting the URL).

What to change

In lib/filterClasses.js reject blank CSPs only if it is a blocking filter, not if it is whitelist filter.

Hints for testers

Make sure that the behavior expected in #6871 still works for blocking filters.

For whitelist filters, after this change, the $csp option should be able to have a blank value. In order to verify this, write two filters ||example.com^$csp=script-src 'none' and @@||example.com^$csp and see that inline scripts are still allowed on the page. You could use the following HTML to test this:

<!DOCTYPE html>
<h1>Test</h1>
<script>
  console.log("Hello from inline script");
</script>

If the second filter is being accepted as valid, then the page should print Hello from inline script to the console. Also verify that removing the second filter then prevents the inline script from executing.

Change History (13)

comment:1 Changed 14 months ago by mjethani

Jon, I have assigned this to you.

comment:2 Changed 14 months ago by mjethani

  • Cc kzar added

comment:3 Changed 14 months ago by amrmak

  • Cc amrmak added

comment:4 Changed 14 months ago by mjethani

  • Priority changed from P2 to P1

Actually, this is a regression since the last release, it should be a P1.

Jon, are you available to work on the patch? If not, I can look into it.

comment:5 Changed 14 months ago by jsonesen

I can do it, thanks for assigning it I'll get right to it today

comment:6 Changed 14 months ago by mjethani

  • Description modified (diff)

comment:7 Changed 14 months ago by mjethani

  • Owner changed from jsonesen to mjethani

comment:8 Changed 14 months ago by mjethani

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

comment:9 Changed 14 months ago by abpbot

A commit referencing this issue has landed:
Issue 7043 - Accept whitelist filters with blank CSPs

comment:10 Changed 14 months ago by mjethani

  • Description modified (diff)
  • Resolution set to fixed
  • Status changed from reviewing to closed

comment:11 Changed 14 months ago by mjethani

  • Blocking 7047 added

comment:12 Changed 14 months ago by abpbot

A commit referencing this issue has landed:
Issue 7043 - Accept whitelist filters with blank CSPs

comment:13 Changed 14 months ago by Ross

  • Tester changed from Unknown to Ross
  • Verified working set

Fixed. Whitelist filters can accept a blank CSP.

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.