1505571 - Tracking cookie blocking + Strict list may break Google sign-in when custom buttons are used.

Status: VERIFIED FIXED

(Firefox :: Protections UI, defect, P2)

Description

[Steven Englehardt [:englehardt]]

Google sign-in's documentation offers instructions for adding sign-in with a custom button: https://developers.google.com/identity/sign-in/web/build-button. The demo code they provide under "Building a button with a custom graphic" is broken when tracking cookie blocking is enabled with the strict list.

See: https://senglehardt.com/test/identity_providers/google_custom.html.

STR:
1. Click the Google button
2. A pop-up is visible
3. Fill out your username and password
4. The pop-up automatically closes.

Actual result:
I receive the following error message:

  "error": "popup_closed_by_user"

This is the same error I'd receive if I manually closed the pop-up prior to completing the authentication.

Note that the console shows: Storage access automatically granted for tracker “https://accounts.google.com” on “https://senglehardt.com”.

Expected result:
When the pop-up automatically closes, I should not see an error message and should instead see the first and last name of my account.

Note this might in some way be related to Bug 1505568.

Comment 1

[Steven Englehardt [:englehardt]]

Note that the older Google Plus sign in flow is also broken, but in different ways.

Comment 2

[(no longer active)]

Andrea, any chance you could test the fix in bug 1505212 to see if it fixes this as well?

Comment 3

[Andrea Marchesini [:baku]]

No, it's unrelated. But debugging this I found this interesting issue:

[Child 11855: Main Thread]: D/AntiTracking Computing whether channel 0x55aa744e8748 has access to URI https://accounts.google.com/o/oauth2/iframe#origin=https%3A%2F%2Fsenglehardt.com&rpcToken=351935897.8093053&clearCache=1
...
[Child 11855: Main Thread]: D/AntiTracking Our channel isn't a tracking channel

[Parent 11727: Main Thread]: D/AntiTracking Computing whether channel 0x55fd4934f920 has access to URI https://accounts.google.com/o/oauth2/iframerpc?action=issueT[...]email&ss_domain=https%3A%2F%2Fsenglehardt.com
[...]
[Parent 11727: Main Thread]: D/AntiTracking Testing permission type 3rdPartyStorage^https://accounts.google.com for https://senglehardt.com/test/identity_providers/google_custom.html resulted in 0 (failure)

Sometimes https://accounts.google.com is seen as tracker, and sometimes it's not.

Note that, a bit before I had the same origin + iframerpc path seen as non-tracker resource:

[Parent 11727: Main Thread]: D/AntiTracking Computing whether channel 0x55fd4ef09ba0 has access to URI https://accounts.google.com/o/oauth2/iframerpc?action=checkOrigin&origin=https%3A%2F%2Fsenglehardt.com&client_id=902141776049-sjne3e0c48hsej7fuf71lgkq2bis8i7g.apps.googleusercontent.com
[...]
[Parent 11727: Main Thread]: D/AntiTracking Our channel isn't a tracking channel

ehsan, can you tell me if we use query parameters to determine if a URL is a tracker or not?
Because if yes, our logic is buggy: we do everything based on origin.

Comment 4

[(no longer active)]

(In reply to Andrea Marchesini [:baku] from comment #3)
> No, it's unrelated. But debugging this I found this interesting issue:
> 
> [Child 11855: Main Thread]: D/AntiTracking Computing whether channel
> 0x55aa744e8748 has access to URI
> https://accounts.google.com/o/oauth2/iframe#origin=https%3A%2F%2Fsenglehardt.
> com&rpcToken=351935897.8093053&clearCache=1
> ...
> [Child 11855: Main Thread]: D/AntiTracking Our channel isn't a tracking
> channel
> 
> [Parent 11727: Main Thread]: D/AntiTracking Computing whether channel
> 0x55fd4934f920 has access to URI
> https://accounts.google.com/o/oauth2/iframerpc?action=issueT[...
> ]email&ss_domain=https%3A%2F%2Fsenglehardt.com
> [...]
> [Parent 11727: Main Thread]: D/AntiTracking Testing permission type
> 3rdPartyStorage^https://accounts.google.com for
> https://senglehardt.com/test/identity_providers/google_custom.html resulted
> in 0 (failure)
> 
> Sometimes https://accounts.google.com is seen as tracker, and sometimes it's
> not.
> 
> Note that, a bit before I had the same origin + iframerpc path seen as
> non-tracker resource:
> 
> [Parent 11727: Main Thread]: D/AntiTracking Computing whether channel
> 0x55fd4ef09ba0 has access to URI
> https://accounts.google.com/o/oauth2/
> iframerpc?action=checkOrigin&origin=https%3A%2F%2Fsenglehardt.
> com&client_id=902141776049-sjne3e0c48hsej7fuf71lgkq2bis8i7g.apps.
> googleusercontent.com
> [...]
> [Parent 11727: Main Thread]: D/AntiTracking Our channel isn't a tracking
> channel
> 
> ehsan, can you tell me if we use query parameters to determine if a URL is a
> tracker or not?
> Because if yes, our logic is buggy: we do everything based on origin.

No, we don't.  It's all based on domain names.

What could be happening is that we may be calling GetIsTrackingResource() before HttpBaseChannel::SetIsTrackingResource() is called on our channel <https://searchfox.org/mozilla-central/rev/a3894a4dcfb5d42f2e6eee6cb9faf7141879ef1a/netwerk/protocol/http/HttpBaseChannel.cpp#322>.  This should be very easy to verify in rr by setting a conditional breakpoint with the channel pointer on HttpBaseChannel::SetIsTrackingResource and HttpBaseChannel::GetIsTrackingResource to see which is called first.  https://bugzilla.mozilla.org/show_bug.cgi?id=1478539#c1 was one similar case where we were being bitten by this sequence happening out of order.

Do you mind checking to see whether that's what's happening?  Thanks!

Comment 5

[Andrea Marchesini [:baku]]

It took me a while, but I think I know what is going on here: basically we don't grant the storage access permission to accounts.google.com. In theory we should do it when window.open() runs:

https://searchfox.org/mozilla-central/rev/a3894a4dcfb5d42f2e6eee6cb9faf7141879ef1a/dom/base/nsGlobalWindowOuter.cpp#7199,7220

But in then we stop here:

https://searchfox.org/mozilla-central/rev/17f55aee76b7c4610a974cffd3453454e0c8de7b/toolkit/components/antitracking/AntiTrackingCommon.cpp#493-500

because the document has not been interacted yet. It's just appeared, it cannot be interacted.

A similar path is done in nsIDocument::SetUserHasInteracted():

https://searchfox.org/mozilla-central/rev/a3894a4dcfb5d42f2e6eee6cb9faf7141879ef1a/dom/base/nsDocument.cpp#12916,12922

Here we call: MaybeStoreUserInteractionAsPermission(), which calls: AntiTrackingCommon::StoreUserInteractionFor:
https://searchfox.org/mozilla-central/rev/17f55aee76b7c4610a974cffd3453454e0c8de7b/toolkit/components/antitracking/AntiTrackingCommon.cpp#1339

The parent process will store the permission. But in the meantime we continue with nsIDocument::SetUserHasIteracted():

https://searchfox.org/mozilla-central/rev/a3894a4dcfb5d42f2e6eee6cb9faf7141879ef1a/dom/base/nsDocument.cpp#12935

And finally here:

https://searchfox.org/mozilla-central/rev/17f55aee76b7c4610a974cffd3453454e0c8de7b/toolkit/components/antitracking/AntiTrackingCommon.cpp#415,492-493

But at this point, the user-interaction permission has not been received yet. We fail, and we do not call this path anymore.

I'm working on a fix.

Comment 6

[Andrea Marchesini [:baku]]

Attachment: test

This is what, I think, it's happening here.
We don't pass this test, but we should. The reason why we fail is that the setting of the permission is async, but, in some circumstances, we require them to be there synchronously.

Comment 7

[Andrea Marchesini [:baku]]

Attachment: part 1 - eOpenerAfterUserInteraction reason

Comment 8

[Andrea Marchesini [:baku]]

Attachment: part 2 - tests

Comment 9

[Andrea Marchesini [:baku]]

Attachment: part 3 - permission in window

Comment 10

[Andrea Marchesini [:baku]]

Probably, we don't need the PartitionedLocalStorage with these patches. I'll debug that after the review.

Comment 11

[(no longer active)]

(In reply to Andrea Marchesini [:baku] from comment #10)
> Probably, we don't need the PartitionedLocalStorage with these patches. I'll
> debug that after the review.

I doubt it.  In bug 1505212 there was no user interaction involved...

Comment 12

[(no longer active)]

Doesn't the part 1 patch regress bug 1505212 for the case where the user is in a new profile?  That is, the user interaction heuristic will now ignore whether the third-party tracker has been interacted with before essentially.  Is there no other way to fix this bug?

Also, I'm not following part 3...  What problem did you see?  And how is that patch solving it?  Why is there no test next to it?  :-)

Comment 13

[(no longer active)]

(In reply to :Ehsan Akhgari from comment #12)
> Doesn't the part 1 patch regress bug 1505212 for the case where the user is
> in a new profile?

Sorry this should have been bug 1494476...

Comment 14

[Andrea Marchesini [:baku]]

> Doesn't the part 1 patch regress bug 1505212 for the case where the user is
> in a new profile?

No, why should it? The bug is that when we store user-interaction permission, that will be received by the current principal by the parent process. But we need it before and patch 1 fixes this issue. No regressions, as far I can see.

> Also, I'm not following part 3...  What problem did you see?  And how is
> that patch solving it?  Why is there no test next to it?  :-)

A similar problem: storage access permissions granted, and synchronously checked. this fails without my patches.

Part 2 is just about tests :)

I'm happy to discuss this on IRC if you want.

Comment 15

[(no longer active)]

(In reply to Andrea Marchesini [:baku] from comment #14)
> > Doesn't the part 1 patch regress bug 1505212 for the case where the user is
> > in a new profile?
> 
> No, why should it? The bug is that when we store user-interaction
> permission, that will be received by the current principal by the parent
> process. But we need it before and patch 1 fixes this issue. No regressions,
> as far I can see.

Bug 1494476 made it so that we never grant the user-interaction permission to a third-party tracker unless if it has been interacted with *beforehand* in a first-party context.

The intention there was for the interaction to happen prior to using the third-party tracker.  For example, if a user doesn't have a Facebook account, and never uses Facebook, the intention was for us to never grant Facebook third-party storage access, even if the user accidentally clicks on a "Login with Facebook" button once and then accidentally clicks somewhere in the opened popup window.

With your patch, this behavior breaks.  As soon as the non-Facebook user clicks somewhere in that popup, Facebook will gain tracking abilities on the website the user came from.

It seems to me that at the very least we want to limit eOpenerAfterUserInteraction to _some_ interactions and not others.  For example, interactions with form controls only, or something like that?

> > Also, I'm not following part 3...  What problem did you see?  And how is
> > that patch solving it?  Why is there no test next to it?  :-)
> 
> A similar problem: storage access permissions granted, and synchronously
> checked. this fails without my patches.

Is that something like https://bugzilla.mozilla.org/show_bug.cgi?id=1505568#c3?

Can you please post a series of steps which results to the case which that part is trying to fix on the bug?

> Part 2 is just about tests :)
> 
> I'm happy to discuss this on IRC if you want.

I was missing that part 2 was testing both cases (I thought the patches are in order.)  Perhaps the test should be the last part in the series.  :-)

Comment 16

[Andrea Marchesini [:baku]]

> Bug 1494476 made it so that we never grant the user-interaction permission
> to a third-party tracker unless if it has been interacted with *beforehand*
> in a first-party context.

Ok, let's start with patch 1. The problem I want to fix with patch 1 is this:

1. fresh profile.
2. the tracker calls window.open().
3. we check if the permission is granted: https://searchfox.org/mozilla-central/rev/007b66c1f5f7a1b9a900a2038652a16a020f010c/dom/base/nsGlobalWindowOuter.cpp#7209
4. But no user-interaction yet: https://searchfox.org/mozilla-central/rev/007b66c1f5f7a1b9a900a2038652a16a020f010c/toolkit/components/antitracking/AntiTrackingCommon.cpp#493

And we don't grant the permission because the tracker has never been interacted. All good.

Then, the user clicks on something in that popup. This is what happens:

1. event is received: https://searchfox.org/mozilla-central/rev/007b66c1f5f7a1b9a900a2038652a16a020f010c/dom/base/nsDocument.cpp#129162
2. it's the first user-interaction: https://searchfox.org/mozilla-central/rev/007b66c1f5f7a1b9a900a2038652a16a020f010c/dom/base/nsDocument.cpp#13196,13203-13207
3. We store the user-interaction permission: https://searchfox.org/mozilla-central/rev/007b66c1f5f7a1b9a900a2038652a16a020f010c/toolkit/components/antitracking/AntiTrackingCommon.cpp#1324

This is an asynchronous operation. This means that the current process doesn't have the permission yet.

We are still in in nsIDocument::SetUserHasInteracted. Let's continue:

1. Maybe we want to grant the storage-access permission: https://searchfox.org/mozilla-central/rev/007b66c1f5f7a1b9a900a2038652a16a020f010c/dom/base/nsDocument.cpp#12924-12929,12935
2. https://searchfox.org/mozilla-central/rev/007b66c1f5f7a1b9a900a2038652a16a020f010c/dom/base/nsDocument.cpp#12982,13038
3. We check the user-interaction: https://searchfox.org/mozilla-central/rev/007b66c1f5f7a1b9a900a2038652a16a020f010c/toolkit/components/antitracking/AntiTrackingCommon.cpp#415,493

But the current process doesn't have the user-interaction yet. We just sent the permission to the parent process and the permission propagation is not completed yet. We fail there. No storage-access permission is granted.

Just because there is this: https://searchfox.org/mozilla-central/rev/007b66c1f5f7a1b9a900a2038652a16a020f010c/dom/base/nsDocument.cpp#12924-12929

We are not going to grant the permission in the next user-interactions. Never.

> The intention there was for the interaction to happen prior to using the
> third-party tracker.  For example, if a user doesn't have a Facebook
> account, and never uses Facebook, the intention was for us to never grant

This makes totally sense for facebook, but people do not open accounts.google.com as first-party context directly. So, that approach would not work here. The starting point if a fresh profile, or a profile without a user-interaction stored for accounts.google.com, which is normal, for existing profiles, with active google accounts coming from older firefox versions.

> It seems to me that at the very least we want to limit
> eOpenerAfterUserInteraction to _some_ interactions and not others.  For
> example, interactions with form controls only, or something like that?

This seems a good approach. I'll think about it.

> > > Also, I'm not following part 3...  What problem did you see?  And how is

> Is that something like
> https://bugzilla.mozilla.org/show_bug.cgi?id=1505568#c3?

Yes. Here the steps. Let's assume that the tracker has been already interacted as a first-party context (or we have patch 1):

1. a tracking iframe, blocked by cookieBehavior=4, does |window.onstorage = () => {popup.postMessage('all good here!'); }|.
2. there is a window.open(), which opens popup.html.
3. the popup.html has a code like this: |inputElement.onkeydown = () => {localStorage.foo = 42; }|
4. the user clicks on the inputElement, this triggers: https://searchfox.org/mozilla-central/rev/007b66c1f5f7a1b9a900a2038652a16a020f010c/dom/base/nsDocument.cpp#12916,12935
5. we grant the permission. We send a message to the parent to store the permission.
6. In the same onkeydown event propagation, we exec: "localStorage.foo = 42", which dispatches a StorageEvent.
7. the parent window (the iframe) receives the StorageEvent: https://searchfox.org/mozilla-central/rev/007b66c1f5f7a1b9a900a2038652a16a020f010c/dom/base/nsGlobalWindowInner.cpp#5712
8. We try to see if we have to propagate the event to parent.localStorage: https://searchfox.org/mozilla-central/rev/007b66c1f5f7a1b9a900a2038652a16a020f010c/dom/base/nsGlobalWindowInner.cpp#5790
9. We retrieve the current localStorage: https://searchfox.org/mozilla-central/rev/007b66c1f5f7a1b9a900a2038652a16a020f010c/dom/base/nsGlobalWindowInner.cpp#5840
10. this can be a PartitionedLocalStorage or an exception because the permission has not been received yet by the current process.

Instead, just because the permission has been sent to the parent, we must consider storage-permission as granted on the current process.

Comment 17

[(no longer active)]

(In reply to Andrea Marchesini [:baku] from comment #16)
> > Bug 1494476 made it so that we never grant the user-interaction permission
> > to a third-party tracker unless if it has been interacted with *beforehand*
> > in a first-party context.
> 
> Ok, let's start with patch 1. The problem I want to fix with patch 1 is this:
> 
> 1. fresh profile.
> 2. the tracker calls window.open().
> 3. we check if the permission is granted:
> https://searchfox.org/mozilla-central/rev/
> 007b66c1f5f7a1b9a900a2038652a16a020f010c/dom/base/nsGlobalWindowOuter.
> cpp#7209
> 4. But no user-interaction yet:
> https://searchfox.org/mozilla-central/rev/
> 007b66c1f5f7a1b9a900a2038652a16a020f010c/toolkit/components/antitracking/
> AntiTrackingCommon.cpp#493
> 
> And we don't grant the permission because the tracker has never been
> interacted. All good.
> 
> Then, the user clicks on something in that popup. This is what happens:
> 
> 1. event is received:
> https://searchfox.org/mozilla-central/rev/
> 007b66c1f5f7a1b9a900a2038652a16a020f010c/dom/base/nsDocument.cpp#129162
> 2. it's the first user-interaction:
> https://searchfox.org/mozilla-central/rev/
> 007b66c1f5f7a1b9a900a2038652a16a020f010c/dom/base/nsDocument.cpp#13196,13203-
> 13207
> 3. We store the user-interaction permission:
> https://searchfox.org/mozilla-central/rev/
> 007b66c1f5f7a1b9a900a2038652a16a020f010c/toolkit/components/antitracking/
> AntiTrackingCommon.cpp#1324
> 
> This is an asynchronous operation. This means that the current process
> doesn't have the permission yet.
> 
> We are still in in nsIDocument::SetUserHasInteracted. Let's continue:
> 
> 1. Maybe we want to grant the storage-access permission:
> https://searchfox.org/mozilla-central/rev/
> 007b66c1f5f7a1b9a900a2038652a16a020f010c/dom/base/nsDocument.cpp#12924-12929,
> 12935
> 2.
> https://searchfox.org/mozilla-central/rev/
> 007b66c1f5f7a1b9a900a2038652a16a020f010c/dom/base/nsDocument.cpp#12982,13038
> 3. We check the user-interaction:
> https://searchfox.org/mozilla-central/rev/
> 007b66c1f5f7a1b9a900a2038652a16a020f010c/toolkit/components/antitracking/
> AntiTrackingCommon.cpp#415,493
> 
> But the current process doesn't have the user-interaction yet. We just sent
> the permission to the parent process and the permission propagation is not
> completed yet. We fail there. No storage-access permission is granted.
> 
> Just because there is this:
> https://searchfox.org/mozilla-central/rev/
> 007b66c1f5f7a1b9a900a2038652a16a020f010c/dom/base/nsDocument.cpp#12924-12929
> 
> We are not going to grant the permission in the next user-interactions.
> Never.

Right.  I think you probably haven't read bug 1505568 comment 3.  If you read it again you'll see that I'm explaining this exact behavior there.  This is the *intentional behavior* I implemented in bug 1494476.

The goal there was for interactions *before the popup gets opened* to count for whether we should grant the user interaction permission.  Obviously when a third-party tracker opens a pop-up the user may interact with it in the first-party context.  Checking for interactions *in the pop-up* defeats the entire purpose behind bug 1494476 as far as tightening our heuristics go.

(Of course, that bug also tightens how we grant the storage access API permissions, which this bug doesn't affect.)

I just want to highlight that I understand very well what is happening here and why this case isn't working in the first place, since I wrote the code such that it doesn't work.  The question in front of us is, whether it _should_ be working this way or not.

The only reason I can see for changing our behavior here is bug 1505568 comment 4.  The use case there is as follows:

  * The user visits tracker.example in the first-party context (no popups involved here) and interacts with them.
  * Then they clear their history.
  * Then they encounter tracker.example in the third-party context, they click on a button and get a pop-up.  There, our heuristics do not work.

This is the use case to focus on.

The use case comment 0 is filed on is:

  * The user has never visited tracker.example in the first-party context (no popups involved here).
  * Then they encounter tracker.example in the third-party context, they click on a button and get a pop-up.  There, our heuristics should not work.

It would be ideal if we can address the second use case without breaking the first use case.

Your current patch is fixing the first use case *and* breaking the second one.

> > The intention there was for the interaction to happen prior to using the
> > third-party tracker.  For example, if a user doesn't have a Facebook
> > account, and never uses Facebook, the intention was for us to never grant
> 
> This makes totally sense for facebook, but people do not open
> accounts.google.com as first-party context directly.

Sure, they do.  If you go to any Google property (google.com, gmail.com, youtube.com, etc.) and click on Sign In, you will be redirected to accounts.google.com where you have to interact with the domain on the first-party basis.  A Google user who has ever signed in to their account has no way but to interact with accounts.google.com.

> So, that approach would
> not work here. The starting point if a fresh profile, or a profile without a
> user-interaction stored for accounts.google.com, which is normal, for
> existing profiles, with active google accounts coming from older firefox
> versions.

There are other sources of information besides just clicking on a random place in the popup that we can use for the heuristic.  Like I suggested, interacting with a form control is one.  Submitting a form (e.g. when logging in) is another.

Why is it better to grant this permission to the pop-up as soon as the user clicks anywhere on it?  I don't really understand what about that approach wouldn't work, can you please expand?

> > It seems to me that at the very least we want to limit
> > eOpenerAfterUserInteraction to _some_ interactions and not others.  For
> > example, interactions with form controls only, or something like that?
> 
> This seems a good approach. I'll think about it.

Great, thanks! (It seems like above you're talking about a different approach not working, but I only suggested this one idea, FWIW)

> > > > Also, I'm not following part 3...  What problem did you see?  And how is
> 
> > Is that something like
> > https://bugzilla.mozilla.org/show_bug.cgi?id=1505568#c3?
> 
> Yes. Here the steps. Let's assume that the tracker has been already
> interacted as a first-party context (or we have patch 1):
> 
> 1. a tracking iframe, blocked by cookieBehavior=4, does |window.onstorage =
> () => {popup.postMessage('all good here!'); }|.
> 2. there is a window.open(), which opens popup.html.
> 3. the popup.html has a code like this: |inputElement.onkeydown = () =>
> {localStorage.foo = 42; }|
> 4. the user clicks on the inputElement, this triggers:
> https://searchfox.org/mozilla-central/rev/
> 007b66c1f5f7a1b9a900a2038652a16a020f010c/dom/base/nsDocument.cpp#12916,12935
> 5. we grant the permission. We send a message to the parent to store the
> permission.
> 6. In the same onkeydown event propagation, we exec: "localStorage.foo =
> 42", which dispatches a StorageEvent.
> 7. the parent window (the iframe) receives the StorageEvent:
> https://searchfox.org/mozilla-central/rev/
> 007b66c1f5f7a1b9a900a2038652a16a020f010c/dom/base/nsGlobalWindowInner.
> cpp#5712
> 8. We try to see if we have to propagate the event to parent.localStorage:
> https://searchfox.org/mozilla-central/rev/
> 007b66c1f5f7a1b9a900a2038652a16a020f010c/dom/base/nsGlobalWindowInner.
> cpp#5790
> 9. We retrieve the current localStorage:
> https://searchfox.org/mozilla-central/rev/
> 007b66c1f5f7a1b9a900a2038652a16a020f010c/dom/base/nsGlobalWindowInner.
> cpp#5840
> 10. this can be a PartitionedLocalStorage or an exception because the
> permission has not been received yet by the current process.
> 
> Instead, just because the permission has been sent to the parent, we must
> consider storage-permission as granted on the current process.

Oh wow, nice catch, this was really subtle.  Thanks for explaining this.  It seems like we want some form of part 3 no matter how we decide to fix part 1 then.

Comment 18

[Andrea Marchesini [:baku]]

Ok. We have a plan. do you mind to review part 2 and 3? I'll rework on part 1. Note that this bug is fixed by part 2 and 3, without part 1, if the user has visited accounts.google.com before.

Comment 19

[Steven Englehardt [:englehardt]]

>There are other sources of information besides just clicking on a random place in the popup that we can use for the heuristic.  Like I suggested, interacting with a form control is one.  Submitting a form (e.g. when logging in) is another.

>Why is it better to grant this permission to the pop-up as soon as the user clicks anywhere on it?  I don't really understand what about that approach wouldn't work, can you please expand?

In Bug 1505568 Comment 8 I described one use case that will break if we require form interaction / submission.

Comment 21

[(no longer active)]

(In reply to Steven Englehardt [:englehardt] from comment #19)
> >There are other sources of information besides just clicking on a random place in the popup that we can use for the heuristic.  Like I suggested, interacting with a form control is one.  Submitting a form (e.g. when logging in) is another.
> 
> >Why is it better to grant this permission to the pop-up as soon as the user clicks anywhere on it?  I don't really understand what about that approach wouldn't work, can you please expand?
> 
> In Bug 1505568 Comment 8 I described one use case that will break if we
> require form interaction / submission.

Yeah that's a valid point.

Andrea and I talked about how to address this at the heuristics level on IRC today and didn't come up with any genius solutins.  Instead we decided to take the approach of part 1, and add a disaster recovery pref to add a blocklist of URLs which the user interaction heuristic wouldn't get applied to.  The pref can be used if a bad actor tracker starts to abuse this heuristic in the wild in order to trick users into granting it storage access rights on the sites it is embedded in.

Comment 22

[(no longer active)]

Comment on attachment 9024490 [details] [diff] [review]
part 1 - eOpenerAfterUserInteraction reason

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

::: toolkit/components/antitracking/AntiTrackingCommon.cpp
@@ +332,5 @@
>        messageWithSameOrigin = "CookieAllowedForTrackerByStorageAccessAPI";
>        break;
>  
> +    case AntiTrackingCommon::eOpenerAfterUserInteraction:
> +      // fall through

MOZ_FALLTHROUGH; please.

Technically that's only needed on case branches that have code, but it's better than a comment IMO.

Comment 23

[(no longer active)]

Comment on attachment 9024491 [details] [diff] [review]
part 2 - tests

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

::: toolkit/components/antitracking/test/browser/localStorage.html
@@ +1,4 @@
> +<h1>Here a tracker!</h1>
> +<script>
> +
> +if (opener) {

Nit: window.opener

@@ +16,5 @@
> +        status = true;
> +      } catch (e) {
> +        status = false;
> +      }
> +      

Trailing whitespace.

Comment 24

[(no longer active)]

Comment on attachment 9024493 [details] [diff] [review]
part 3 - permission in window

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

::: dom/base/nsGlobalWindowInner.cpp
@@ +8056,5 @@
> +{
> +  nsAutoCString key;
> +  key.Assign(aTrackingOrigin);
> +
> +  if (aTrackingOrigin != aGrantedOrigin) {

Is there any reason why you don't call CreatePermissionKey() in the caller and pass the result here?  In at least one of the call sites that function would be called already, so you already have the key there...

@@ +8059,5 @@
> +
> +  if (aTrackingOrigin != aGrantedOrigin) {
> +    key.AppendLiteral("^");
> +    key.Append(aGrantedOrigin);
> +  }

Please convert the key to lower case here.

@@ +8076,5 @@
> +
> +  if (aTrackingOrigin != aGrantedOrigin) {
> +    key.AppendLiteral("^");
> +    key.Append(aGrantedOrigin);
> +  }

Please convert the key to lower case here.

::: toolkit/components/antitracking/AntiTrackingCommon.cpp
@@ +504,5 @@
> +
> +  NS_ConvertUTF16toUTF8 grantedOrigin(origin);
> +
> +  // Let's store the permission in the current parent window.
> +  LOG(("Storing permission in the parent window."));

This would log a message in this function unconditionally, making our logs needlessly verbose.  Please remove this line.

@@ +505,5 @@
> +  NS_ConvertUTF16toUTF8 grantedOrigin(origin);
> +
> +  // Let's store the permission in the current parent window.
> +  LOG(("Storing permission in the parent window."));
> +  topInnerWindow->SaveStorageAccessGranted(trackingOrigin, grantedOrigin);

It is still possible after this point to bail out early, in which case recording this would be wrong.  Please move the call to SaveStorageAccessGranted() down to before calling NotifyContentBlockingState().

@@ +813,5 @@
> +    LOG(("No top inner window."));
> +    return false;
> +  }
> +
> +  LOG(("Checking permission in the parent window"));

Ditto.

Comment 25

[Andrea Marchesini [:baku]]

Attachment: part 3 - permission in window

Comment 26

[(no longer active)]

Comment on attachment 9025602 [details] [diff] [review]
part 3 - permission in window

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

::: toolkit/components/antitracking/AntiTrackingCommon.cpp
@@ +526,5 @@
>  
> +  NS_ConvertUTF16toUTF8 grantedOrigin(origin);
> +
> +  nsAutoCString permissionKey;
> +  CreatePermissionKey(trackingOrigin, grantedOrigin, permissionKey);

Now that you are computing the permission key here, please remove the trackingOrigin and grantedOrigin arguments from SaveFirstPartyStorageAccessGrantedForOriginOnParentProcess and instead just pass the permissionKey directly into it.

Comment 27

[Andrea Marchesini [:baku]]

> Now that you are computing the permission key here, please remove the
> trackingOrigin and grantedOrigin arguments from
> SaveFirstPartyStorageAccessGrantedForOriginOnParentProcess and instead just
> pass the permissionKey directly into it.

No, because otherwise, this method can be used to set any permission by a compromised content process.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/be17a8b7aefa
Don't chcek user-interaction permission when the operation starts from user-interaction, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/88af3329ee16
Tests of localStorage access immediatelly after a user-interaction, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/838dfe679fdd
Store access-storage permissions in the top-level window too, r=ehsan

Comment 29

[Andrei Ciure[:aciure]]

Backed out 4 changesets (bug 1507769, bug 1505571) for browser_storageAccessWithHeuristics.js failures

push that caused the backout: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&group_state=expanded&selectedJob=212682973&revision=401763b97e54cd99c6a5ad2a617379da011a57e5

failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&group_state=expanded&fromchange=d32110a492f5eece3697fd039d53dba247f9f202&selectedJob=212675524&searchStr=os%2Cx%2C10.10%2Cdebug%2Cmochitests%2Cwith%2Ce10s%2Ctest-macosx64%2Fdebug-mochitest-browser-chrome-e10s-2%2Cm-e10s%28bc2%29

backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/29ff944efa40e929995f6b2660383041738266f0

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/657e6091286d
Don't chcek user-interaction permission when the operation starts from user-interaction, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff115c0a1586
Tests of localStorage access immediatelly after a user-interaction, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/da7dfaf0e9a2
Store access-storage permissions in the top-level window too, r=ehsan

Comment 31

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/657e6091286d
https://hg.mozilla.org/mozilla-central/rev/ff115c0a1586
https://hg.mozilla.org/mozilla-central/rev/da7dfaf0e9a2

Comment 32

[Mohammad Maruf Rahman]

I have reproduced this bug with Nightly 65.0a1 (2018-11-07) on Windows 7, 64 Bit!
This bug's fix is verified with latest Nightly!

Build ID 	20181122220059
User Agent 	Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:65.0) Gecko/20100101 Firefox/65.0

Comment 33

[Timea Cernea [:tbabos][inactive]]

Reproduced the issue on affected Nightly 65.0a1 (2018-11-02) (64-bit) on Windows 10 x64.
Verified - Fixed on latest Nightly 65.0a1 (2018-11-28) (64-bit) on Windows 7/10 x64, Ubuntu 18.04. and Mac OS 10.14.

Comment 34

[Neal]

Should Steven's original testcase still fail?  (https://senglehardt.com/test/identity_providers/google_custom.html)

I'm still getting the "error": "popup_closed_by_user" alert.  This is with Firefox Developer Edition 65.0b2.

In the console: "Request to access cookie or storage on “https://apis.google.com/js/api:client.js” was blocked because it came from a tracker and content blocking is enabled."

Google Sign-In and Google Classroom Import stopped working on my site when my Firefox DE updated, and I've spent the day trying to track it down to not-yet-fixed bugs... but maybe this version doesn't have the fix yet?  Luckily (I think) the full content blocking hasn't hit non-DE yet, so my users haven't been unable to login to my site.

Thanks for any insight you can give!

Comment 35

[Neal]

Ah, I didn't remember that Developer Edition is on the beta channel.  So I should expect some bugs (and report them) :)

Comment 36

[Neal]

Folks in #firefox told me that this fix should be in 65.0b2.  Steven's test page fails for me in 65.0b3 as well.

Comment 37

[Steven Englehardt [:englehardt]]

I can confirm that this is still broken in 65.0b4, but works in the current Nightly.

Ehsan, do we expect this to be fixed in the current beta?

Comment 38

[(no longer active)]

This is weird, I can reproduce the same results (working on Nightly, failing on 65.0b4.)  :-(

Let's see what running |mozregression -b 2018-11-20 -g 2018-12-13 --find-fix| will discover...

Comment 39

[(no longer active)]

No results, it seems like this isn't reproducible on trunk, only on beta.  This is really strange.

Running with MOZ_LOG=AntiTracking:5, I see messages like the following:

2018-12-14 00:23:37.508631 UTC - [Child 26765: Main Thread]: D/AntiTracking Computing whether window 0x7ffac3179820 has access to URI https://accounts.google.com/o/oauth2/iframe#origin=https%3A%2F%2Fsenglehardt.com&rpcToken=743486374.7004637
2018-12-14 00:23:37.508677 UTC - [Child 26765: Main Thread]: D/AntiTracking Deciding whether the user has overridden content blocking for https://senglehardt.com/test/identity_providers/google_custom.html
2018-12-14 00:23:37.508710 UTC - [Child 26765: Main Thread]: D/AntiTracking No user override found
2018-12-14 00:23:37.508715 UTC - [Child 26765: Main Thread]: D/AntiTracking Failed to obtain the parent principal and the tracking origin

That's kind of weird since that window must be a third-party in that case.

Andrea, can you please investigate what's going on here?

Comment 40

[Andrea Marchesini [:baku]]

Here is my debugging:

google has a feature-detection code that changes the sandbox attribute value based on storage-access API existence. If storage-access API is detected, the iframe will have these sandbox attribute value: "allow-scripts allow-same-origin allow-storage-access-by-user-activation", otherwise it will have: "allow-scripts allow-same-origin".

Now, the bug is here: https://searchfox.org/mozilla-central/rev/2e5e28f518524f8af5b158ddb605022b6a2d68cf/toolkit/components/antitracking/AntiTrackingCommon.cpp#77-78

By default, sandbox flags is set to SANDBOX_ALL_FLAGS. Then, based on the sandbox attribute value, we set to 0 the single allowed 'permission/feature'. In beta we have this scenario:

1. storage-access API is disabled
2. allow-storage-access-by-user-activation is not part of the sandbox attribute value
3. consequentially, doc->GetSandboxFlags() will contain SANDBOXED_STORAGE_ACCESS

The result is that GetParentPrincipalAndTrackingOrigin doesn't work in beta.
I already wrote the patch. I want to test it a bit more, and then I'll ask ehsan to review it.

Comment 41

[(no longer active)]

Aha!  That check (and probably the other sandbox flag checks we have in that file) should depend on the dom.storage_access.enabled pref.

Comment 42

[Liz Henry (:lizzard) (relman/hg->git project)]

Does this need a new bug if there is some remaining issue on beta?

Comment 43

[Liz Henry (:lizzard) (relman/hg->git project)]

Or, is that bug 1515693?

Comment 44

[Andrea Marchesini [:baku]]

It's bug 1515693. Beta should be fine now.