Closed Bug 1502045 Opened Last year Closed Last year

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

Categories

(Firefox :: Protections UI, defect, P1)

63 Branch
Unspecified
All
defect

Tracking

()

VERIFIED FIXED
Firefox 65
Tracking Status
firefox63 --- wontfix
firefox64 --- wontfix
firefox65 --- verified

People

(Reporter: ythanb, Assigned: baku)

Details

(Whiteboard: [privacy65])

Attachments

(2 files, 1 obsolete file)

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.
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.
Status: UNCONFIRMED → NEW
Component: Untriaged → Preferences
Ever confirmed: true
OS: Unspecified → All
Attached image Capture.PNG
Whiteboard: [privacy65][triage]
baku, can you investigate?  Also check if this situation is a problem when All Cookies are blocked.
Flags: needinfo?(amarchesini)
Also, confirm that this is the Cookies and Site Data Exceptions, not the Content Blocking exceptions.
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...
Component: Preferences → Tracking Protection
Summary: New content blocking options clobber cookie exceptions in Firefox 63, even when disabled? → We fail to honour cookie permissions correctly in 3rd party contexts
Attached patch principal.patch (obsolete) — Splinter Review
Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)
Attachment #9021824 - Flags: review?(ehsan)
Status: NEW → ASSIGNED
Priority: -- → P1
Whiteboard: [privacy65][triage] → [privacy65]
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.
Attachment #9021824 - Flags: review?(ehsan) → review-
Attached patch principal.patchSplinter Review
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?
Attachment #9021824 - Attachment is obsolete: true
Attachment #9022076 - Flags: review?(ehsan)
Attachment #9022076 - Flags: feedback?(kmaglione+bmo)
Comment on attachment 9022076 [details] [diff] [review]
principal.patch

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

Thanks!
Attachment #9022076 - Flags: review?(ehsan) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2605ac0d5cd7
Honour cookie permissions correctly in 3rd party contexts, r=ehsan
https://hg.mozilla.org/mozilla-central/rev/2605ac0d5cd7
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
Is this something which should ride the trains?
Flags: needinfo?(amarchesini)
Flags: in-testsuite+
No, this is for custom permissions. I think we don't need to uplift the patch.
Flags: needinfo?(amarchesini)
Verified on Windows 10, Ubuntu 18, Mac OS, on Nightly 65.0a1 (2018-11-27).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.