1560623 - First-party isolation makes it impossible to turn off content blocking

Status: RESOLVED DUPLICATE of bug 1572240

(Core :: Privacy: Anti-Tracking, defect, P2)

Description

[SimonP]

Attachment: Note that content blocking appears disabled in the URL bar, but the resource was still blocked.

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:69.0) Gecko/20100101 Firefox/69.0

Steps to reproduce:

• In a fresh profile, set content blocking to custom, change block list to level 2.
• Enable first-party isolation in about:config.
• Open a private window and navigate to https://patrickhlauke.github.io/recaptcha/
• Turn off blocking temporarily.

Actual results:

The Recaptcha widget does not appear. The console notifies that "The resource at “https://www.google.com/recaptcha/api.js” was blocked because content blocking is enabled."

Expected results:

The Recaptcha widget appears because content blocking is turned off.

Comment 1

[SimonP]

Upon further investigation, things are even more screwy: Do the reproduction steps, but use a normal instead of a private window. Now blocking doesn't work even though it should.

Comment 2

[Johann Hofmann [:johannh]]

I can reproduce this in Nightly but not in Beta, which leads me to believe that we broke this in bug 1330467. Because of bug 1557872 the UI picks up the change correctly, but we still check the URI instead of principal here: https://searchfox.org/mozilla-central/rev/543e1fbd04a85bae7b4d38357b185d99bcfc8e4b/toolkit/components/antitracking/AntiTrackingCommon.cpp#1784

Gary, would you be interested in taking this?

Comment 3

[Gary Chen [:xeonchen]]

(In reply to Johann Hofmann [:johannh] (Away until July 3rd) from comment #2)

I can reproduce this in Nightly but not in Beta, which leads me to believe that we broke this in bug 1330467. Because of bug 1557872 the UI picks up the change correctly, but we still check the URI instead of principal here: https://searchfox.org/mozilla-central/rev/543e1fbd04a85bae7b4d38357b185d99bcfc8e4b/toolkit/components/antitracking/AntiTrackingCommon.cpp#1784

Gary, would you be interested in taking this?

Sure, thanks for the investigation.

Comment 4

[Ethan Tseng [:ethan]]

Per discussion offline, Tom will take over this bug.

Comment 5

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

I looked at this a bit last night. I want to make sure I'm on the right track.

I want to change this call to TestPermissionOriginNoSuffix into a call to TestPermissionFromPrincipal.

So I need to change IsOnContentBlockingAllowList (and CheckContentBlockingAllowList) to take a principal instead of a nsIURI.

I'm kind of hunting around for the right principal for all the callsites. For this callsite I got the following code to compile (untested though):

  nsresult rv;
  nsCOMPtr<nsIPrincipal> principal;
  nsCOMPtr<nsIScriptSecurityManager> securityManager =
      do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID, &rv);
  if (NS_WARN_IF(NS_FAILED(rv))) {
    return false;
  }
  securityManager->GetChannelResultPrincipal(aChannel, getter_AddRefs(principal));

Comment 6

[(no longer active)]

Tom, sorry you spent time on this... The principal we use for checking the content blocking allow list isn't the channel result principal, it's a modified version, see https://searchfox.org/mozilla-central/rev/9ae20497229225cb3fa729a787791f424ff8087b/toolkit/components/antitracking/ContentBlockingAllowList.jsm#51.

It turns out that I've been working on bug 1572240, which should fix this, among other things. I have my patches almost ready, they're currently running what I hope would be the final try run but let me post the patches to that bug. Can you please test them out? If they don't fix the bug it's a sign that I've messed up the origin attributes somewhere...

Comment 7

[SimonP]

Fixed for me with bug 1572240.

Comment 8

[(no longer active)]

Awesome, thanks for testing! Marking this a dupe then.

Comment 9

[(no longer active)]

I guess one question here is whether we would want to backport something here for 69?

I think bug 1572240 is a rather risky backport for beta as it changes how we assign the content blocking allow list permission internally, but if we are going to backport it then I'd rather expose it to as many beta releases as possible to catch any potential issues (e.g. crashes) early on. I'm not super comfortable with backporting it to be honest. And I think a smaller scale patch wouldn't be very easy to create either, because we need to make CheckContentBlockingAllowList() accept a principal instead of a URI, and for that to happen we need to be able to get such a principal from a document and a channel and providing mechanisms for a document and a channel is already the majority of the patch series.