Opened 9 months ago

Closed 9 months ago

#4902 closed defect (fixed)

sitekey whitelisting broken in Adblock Plus for Safari

Reported by: arthur Assignee: kzar
Priority: P4 Milestone: Adblock-Plus-for-Safari-next
Module: Platform Keywords:
Cc: kzar, sebastian, bastian@… Blocked By:
Blocking: Platform: Safari
Ready: yes Confidential: no
Tester: Unknown Verified working: no
Review URL(s):

https://codereview.adblockplus.org/29375851/

Description (last modified by kzar)

Environment

Mac Mini
macOS Sierra 10.12.3
Safari 10.0.3
ABP 1.12.4
EasyList
Acceptable Ads

How to reproduce

Go to any of the following websites, which all have Acceptable Ads, whitelisted by $sitekey:

Observed behaviour

No ads show.

Expected behaviour

Text ads should show.

Notes

This regression was caused by Issue 4466 - Remove rsa.js, update core + buildtools and is resulting in the following console message: Invalid RSA public key: Unexpected content.

Change History (9)

comment:1 Changed 9 months ago by arthur

  • Cc bastian@… added

comment:2 Changed 9 months ago by sebastian

Do you experience the same issue on other websites that rely on $sitekey whitelisting as well?

comment:4 Changed 9 months ago by sebastian

  • Description modified (diff)
  • Priority changed from Unknown to P4
  • Ready set
  • Summary changed from sitekey whitelisting broken in current Safari version to sitekey whitelisting broken in Adblock Plus for Safari

I was able to reproduce the issue with Safari 10.2 on macOS 10.12.2. I also tested some other websites, and it seems the $sitekey option is completly broken in Adblock Plus 1.12.4 for Safari.

However, if the plan to migrate away from a traditional Safari extension, to a native app which relies on WebKit Content Blockers, goes ahead, we might have to give up key-based whitelisting on Safari, anyway. Potentially, even before we'd get an update to the old Safari extension out (given that it took two months alone, to get our last update rolled out by Apple).

But let's keep the bug open, with low priority, for now. If plans change, we can prioritize it again.

comment:5 Changed 9 months ago by kzar

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

Rather than wasting time debugging exactly why those changes broke sitekey verification for Safari I suggest we simply role them back for the branch. I've tested already and that fixed the problem for me.

comment:6 Changed 9 months ago by kzar

  • Owner set to kzar

comment:7 Changed 9 months ago by sebastian

Yeah, I agree, less because it is a pragmatic solution, but rather because otherwise we might have to address it in adblockpluscore where we no longer want to target legacy browsers. Not sure if it's worth a bugfix release though, given the plans, outlined above.

comment:8 Changed 9 months ago by abpbot

comment:9 Changed 9 months ago by kzar

  • Milestone set to Adblock-Plus-for-Safari-next
  • Resolution set to fixed
  • Status changed from reviewing to closed
Note: See TracTickets for help on using tickets.