1502045 - We fail to honour cookie permissions correctly in 3rd party contexts

Status: VERIFIED FIXED

(Firefox :: Protections UI, defect, P1)

Description

[ythanb]

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

Steps to reproduce:

Disable all options under "Privacy & Security" > "Content Blocking".

Set option "Cookies and Site Data" > "Block cookies and site data" to "Cookies from unvisited websites"

Click "Exceptions..." and add "https://keepa.com"

Visit https://codepen.io/anon/pen/XxoMYv/


Actual results:

Keepa displays an error message that cookies are disabled.


Expected results:

The Keepa widget loads in the output frame.

Comment 1

[Paul_B]

Hi reporter,

Thank you for taking the time to add this report.
I verified this issue on Windows 10, Windows 7 and Ubuntu 18.04 and I was able to reproduce it, following the steps provided, on 63.0 Release build (20181018182531), 64.0b4 DevEdition build (20181025233934) and latest 65.0a1 Nightly build (20181030100414) and I was able to reproduce the reported behavior. 
I'm going to add this to Firefox: Preferences component for a more advised input.

Comment 2

[Paul_B]

Attachment: Capture.PNG

Comment 3

[Tanvi Vyas[:tanvi]]

baku, can you investigate?  Also check if this situation is a problem when All Cookies are blocked.

Comment 4

[Tanvi Vyas[:tanvi]]

Also, confirm that this is the Cookies and Site Data Exceptions, not the Content Blocking exceptions.

Comment 5

[(no longer active)]

I expected this to work correctly...  But looking at the code, in AntiTrackingCommon.cpp we only check the cookie permission for the top-level page not for the target of the blocking (window or channel).  So the bug as filed isn't all that surprising...

Comment 6

[Andrea Marchesini [:baku]]

Attachment: principal.patch

Comment 7

[(no longer active)]

Comment on attachment 9021824 [details] [diff] [review]
principal.patch

Review of attachment 9021824 [details] [diff] [review]:
-----------------------------------------------------------------

r- for not having tests.  :-)

Also, please submit a patch on top of m-c.  I think maybe you had some other patches in your queue?

::: toolkit/components/antitracking/AntiTrackingCommon.cpp
@@ +154,5 @@
> +    return nsICookieService::BEHAVIOR_ACCEPT;
> +  }
> +
> +  if (a3rdPartyPrincipal &&
> +      BasePrincipal::Cast(a3rdPartyPrincipal)->AddonPolicy()) {

Can web extension principals be loaded in third-party contexts at all?

Please get review from :kmag on this issue.

@@ +665,5 @@
>      *aRejectedReason = nsIWebProgressListener::STATE_COOKIES_BLOCKED_BY_PERMISSION;
>      return false;
>    }
>  
> +  access = CheckCookiePermissionForPrincipal(windowPrincipal);

What is windowPrincipal? It doesn't exist in m-c.

@@ +667,5 @@
>    }
>  
> +  access = CheckCookiePermissionForPrincipal(windowPrincipal);
> +  if (access != nsICookiePermission::ACCESS_DEFAULT) {
> +    LOG(("CheckCookiePermissionForPrincipal() returned a non-default access code (%d), returning %s",

You should change both this log message and the one for the first-party case, to include first-party and third-party in them.

@@ +877,5 @@
>      LOG(("No channel principal, bail out early"));
>      return false;
>    }
>  
> +  access = CheckCookiePermissionForPrincipal(channelPrincipal);

OK, this principal exists in m-c.  :-)

@@ +879,5 @@
>    }
>  
> +  access = CheckCookiePermissionForPrincipal(channelPrincipal);
> +  if (access != nsICookiePermission::ACCESS_DEFAULT) {
> +    LOG(("CheckCookiePermissionForPrincipal() returned a non-default access code (%d), returning %s",

Same comment as above.

Comment 8

[Andrea Marchesini [:baku]]

Attachment: principal.patch

Kris, question: is it possible that webextensions are playing something in 3rd party contexts? In this patch I check principal->AddonPolicy() for 3rd party context's principal. Is it needed?

Comment 9

[(no longer active)]

Comment on attachment 9022076 [details] [diff] [review]
principal.patch

Review of attachment 9022076 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2605ac0d5cd7
Honour cookie permissions correctly in 3rd party contexts, r=ehsan

Comment 11

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/2605ac0d5cd7

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Is this something which should ride the trains?

Comment 13

[Andrea Marchesini [:baku]]

No, this is for custom permissions. I think we don't need to uplift the patch.

Comment 14

[David Olah]

Verified on Windows 10, Ubuntu 18, Mac OS, on Nightly 65.0a1 (2018-11-27).