1505212 - The Google login button on Quartzy.com doesn't work with strict list cookie blocking

Status: VERIFIED FIXED

(Firefox :: Protections UI, defect, P2)

Description

[(no longer active)]

STR: Go to https://app.quartzy.com/ and try to click on "Login with Google".  The button is disabled with network.cookie.cookieBehavior set to 4.

Comment 1

[(no longer active)]

The most interesting thing I see in the console log is this message:

Google Auth initialization failed 
Object { error: "idpiframe_initialization_failed", details: "sessionStorage is not available in the current environment." }

Googling for it took me to this issue: https://github.com/google/google-api-javascript-client/issues/260

Here, the Google developers responding to the issue say what causes the problem: localStorage.setItem() throwing an exception.  :-(

Andrea, what do you think about a strategy for fixing this issue based on changing how we implement LocalStorage for third-party trackers when cookieBehavior 4 policy is active?  Basically, instead of throwing from LocalStorage methods, we'd return an object with similar semantics as SessionStorage from nsGlobalWindowInner::GetLocalStorage().

Comment 2

[Andrea Marchesini [:baku]]

> Andrea, what do you think about a strategy for fixing this issue based on
> changing how we implement LocalStorage for third-party trackers when
> cookieBehavior 4 policy is active?  Basically, instead of throwing from
> LocalStorage methods, we'd return an object with similar semantics as
> SessionStorage from nsGlobalWindowInner::GetLocalStorage().

This was part of our strategy in case there was failures with cookieBehavior 4. So, yes, we can give a try.
Wondering if we should consider IDB, DOM Cache and anything connected with QuotaManager as well. Maybe this can be a follow up in case we see more regressions.

I take this bug.

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

I'd really like to have a vidyo call with you two and :janv about your plans here and how it might relate to :baku's proposed Storage Partitions.  My concerns specifically in terms of this bug are:
- LocalStorage/SessionStorage aren't just the Storage object, but also the resulting "storage" events.  A naive implementation that shunts GetLocalStorage to just return a SessionStorage might still result in the page receiving "storage" events for LocalStorage instances in the same OriginAttributes subspace and/or cause crashes.
- The landing of LocalStorage NextGen is fairly imminent and I'd like to make sure we don't immediately regress whatever you land.

Comment 4

[Andrea Marchesini [:baku]]

Sure. Let's have a meeting next week.

> - LocalStorage/SessionStorage aren't just the Storage object, but also the
> resulting "storage" events.  A naive implementation that shunts
> GetLocalStorage to just return a SessionStorage might still result in the
> page receiving "storage" events for LocalStorage instances in the same
> OriginAttributes subspace and/or cause crashes.

My approach would be to expose a new SegregatedLocalStorage object, which behaves like SessionStorage but without receiving events. Plus, its SessionStorageCache is not shared.

> - The landing of LocalStorage NextGen is fairly imminent and I'd like to
> make sure we don't immediately regress whatever you land.

This is really exciting! Yes, I'll involve you in the implementation/review process.

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

(In reply to Andrea Marchesini [:baku] from comment #4)
> My approach would be to expose a new SegregatedLocalStorage object, which
> behaves like SessionStorage but without receiving events. Plus, its
> SessionStorageCache is not shared.

That sounds good, especially if you can keep the changes mainly in the SessionStorage files/hunks! :)


Note that we are planning to replace SessionStorage with the new LocalStorage next-gen implementation as part of the fission work to let SessionStorage be persistent without requiring SessionStore to manually extract the data from the SessionStorage and store it in a JSON file.  This implies some additional level of QM keying that looks suspiciously like a Storage Partition (except without the mutation), which is partly why I'd really like for us all to formulate a plan around implement Storage Partitions.  The other reason is that transitive communication issues.

Comment 6

[(no longer active)]

I talked to baku, he has started an experimental patch based on the approach discussed above.  We should setup something next week to discuss the work happening here (and also maybe a general overview of what we've been working on for the past few months and how it interfaces with the storage side of things in Gecko.)

It would be nice if you could kindly set something up Andrea at your earliest convenience please.  Thanks!

Comment 7

[Andrea Marchesini [:baku]]

Attachment: segregated_local_storage.patch

This is what I have so far. It makes the login on that website to pass, but I guess, it breaks several anti-tracking tests. I need a feedback before continuing.

Things to do:
1. behind pref.
2. enable it just for a set of hosts (via pref again)
3. fix tests

Comment 8

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 9023532 [details] [diff] [review]
segregated_local_storage.patch

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

At a lower level, the LocalStorage-related changes seem sane and pleasingly minimal, but obviously I would want comments on SegregateLocalStorage that explain what it is and its semantics are, plus what's happening at the construction site in nsGlobalWindowInner.

Is the intent really for the global to have its own sandboxed storage so that same-origin (third-party) iframes will each have their own SegregatedStorage when they live in the same tab or if navigated?  If so, then I'd doubly suggest a name to SandboxedLocalStorage because it's both more self-explanatory and because as we try to choose more socially aware terms in our code-bases, I think segregation is a powerful term in the history of the US and we have a number of other choices here.  If you think Sandbox collides too much with iframe sandboxes, then perhaps SandtrapLocalStorage or ShredderLocalStorage.


At a higher level, I think this (and the introduction of aIgnoringAntiTracking) really emphasizes the need to implement storage partitions.  Also, I'm more than a little concerned about landing a LocalStorage implementation that explicitly violates all LocalStorage semantics around the same time when we land a completely overhauled LocalStorage implementation.  It makes diagnosing bug reports about potential regressions harder when we have an implementation that explicitly does not persist data across navigations and does not share data with related browsing contexts.  For the sanity of the webdevs out there, I'd strongly request that you ensure that the devtools console gets an explicit message about the page being given a sandboxed, non-persistent LocalStorage implementation for tracking protection reasons.

Comment 9

[(no longer active)]

FWIW, from twitter: https://twitter.com/RReverser/status/1060242747238436864

It seems the same bug also hits Stackdriver (a cloud logging tool.)

Comment 10

[(no longer active)]

(In reply to Andrew Sutherland [:asuth] from comment #8)
> Is the intent really for the global to have its own sandboxed storage so
> that same-origin (third-party) iframes will each have their own
> SegregatedStorage when they live in the same tab or if navigated?

Hmm, I would expect it to work the other way around, that is, exactly like SessionStorage, two third-party iframes from the same origin when living in the same tab should be able to see the same storage space.  (I wouldn't be surprised at all if Google Login somehow relies on the underlying storage properties to hold to some extent...)

> If so,
> then I'd doubly suggest a name to SandboxedLocalStorage because it's both
> more self-explanatory and because as we try to choose more socially aware
> terms in our code-bases, I think segregation is a powerful term in the
> history of the US and we have a number of other choices here.  If you think
> Sandbox collides too much with iframe sandboxes, then perhaps
> SandtrapLocalStorage or ShredderLocalStorage.

Ouch, sigh, America :(

Thanks for pointing this out.  So yeah, r- on using the term "segregated" in any way!  I actually think we should definitely not use the term sandbox either since it at least has two different existing meanings in the codebase already (the iframe sandbox and the process sandbox) and we don't need to add a third.  I like the term "partitioned" better, like PartitionedLocalStorage, it's concise and the object will do what it says on the can.  WDYT?

> At a higher level, I think this (and the introduction of
> aIgnoringAntiTracking) really emphasizes the need to implement storage
> partitions.  Also, I'm more than a little concerned about landing a
> LocalStorage implementation that explicitly violates all LocalStorage
> semantics around the same time when we land a completely overhauled
> LocalStorage implementation.  It makes diagnosing bug reports about
> potential regressions harder when we have an implementation that explicitly
> does not persist data across navigations and does not share data with
> related browsing contexts.  For the sanity of the webdevs out there, I'd
> strongly request that you ensure that the devtools console gets an explicit
> message about the page being given a sandboxed, non-persistent LocalStorage
> implementation for tracking protection reasons.

Yes, definitely.  But please read on:

I thought a bit about how we should do this.  After some thought, I no longer believe this is a good enough idea to be applied in a generic way to all third-party trackers.  (And thinking forward into the future, e.g. when we fix bug 1478427, the impact of this will be extended to non-trackers even!)

Instead, I think we should think of this as a breakage mitigation feature, and a site-specific web compatibility hack which we'll temporarily introduce in the hopes of removing at a future date.

I think what we want to do is to add a pref-based mechanism to control what document URLs will get this treatment, and then look for that in nsGlobalWindowInner::GetLocalStorage() and modify our behavior accordingly.  I suggest you reuse this code <https://searchfox.org/mozilla-central/rev/17f55aee76b7c4610a974cffd3453454e0c8de7b/layout/base/PresShell.cpp#7931> which is able to process wildcards and a comma-separated list of values.  This will enable us to easily use the pref to control how the feature will behave, even after the release of Firefox 65 using Normandy.  For starters, I suggest restricting the hack to accounts.google.com.  Having this pref will also allow people to modify the pref for testing purposes.  It also means we wouldn't need to spend the time to flesh out every single bit of details around for example how this would interact with the Storage Access API and we can choose to do the simplest thing that makes our use case work.

One other thing about the patch which I didn't like was the aIgnoringAntiTracking arguments...  I really preferred if callers couldn't pick which sets of checks they would go through.  I think once you add the pref then those checks will become unnecessary, since your activation logic could just be does my document URI match my pref and whether nsContentUtils::StorageDisabledByAntiTracking() returns true.  If the answer to both is true then you can take the hacky branch.

Thanks a lot for spending time looking into this!  Excited for this to turn into a landable patch soon.  :-)

Comment 11

[Andrea Marchesini [:baku]]

> Hmm, I would expect it to work the other way around, that is, exactly like
> SessionStorage, two third-party iframes from the same origin when living in

I prefer to do not do this. This is a first step in the partitioning direction, but I would not implement the communication between iframe loaded similar top-level contexts, until we have a valid reason to do it.

> I suggest you reuse this code
> <https://searchfox.org/mozilla-central/rev/
> 17f55aee76b7c4610a974cffd3453454e0c8de7b/layout/base/PresShell.cpp#7931>
> which is able to process wildcards and a comma-separated list of values.

wonderful! I don't have to reinvent the wheel.

Comment 12

[Andrea Marchesini [:baku]]

Attachment: part 1 - partitioned LocalStorage

Comment 13

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 9023894 [details] [diff] [review]
part 1 - partitioned LocalStorage

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

This looks great, just needs more tests so that we can make sure the LocalStorage NextGen stuff does not/cannot regress and so that the design decisions are ensured.

Thank you for the excellent comments, and in particular explicitly documenting the upgrade path.  This makes it much easier for me to turn around reviews in a timely fashion.

ePartitionOrDeny seems like a much more elegant check approach!  (And I checked the other comparisons via searchfox.)

My request is that you add additional test coverage to verify the following intentional design decisions you've made:
- A same origin (and same-process via setting "dom.ipc.processCount" to 1) top-level window with access to real localStorage does not share storage with an ePartitionOrDeny iframe that should have PartitionedLocalStorage and no storage events are received in either direction.  (Same-process in order to avoid having to worry about any e10s propagation issues.)
- Two ePartitionOrDeny iframes in the same tab in the same origin don't see the same localStorage values and no storage events are received from each other.
- An ePartitionOrDeny iframe navigated between two distinct pages on the same origin does not see the values stored by the previous iframe.  (And I would explicitly not check bfcache navigation backwards here unless you are intending for there to be some interaction there.)
- An ePartitionOrDeny iframe on the same origin that is navigated to itself via window.location.reload() or equivalent does not see the values stored by its previous self.

Comment 14

[(no longer active)]

(In reply to Andrea Marchesini [:baku] from comment #11)
> > Hmm, I would expect it to work the other way around, that is, exactly like
> > SessionStorage, two third-party iframes from the same origin when living in
> 
> I prefer to do not do this. This is a first step in the partitioning
> direction, but I would not implement the communication between iframe loaded
> similar top-level contexts, until we have a valid reason to do it.

OK, if the bug is fixed without it, then that's fine by me!

Comment 15

[(no longer active)]

Comment on attachment 9023894 [details] [diff] [review]
part 1 - partitioned LocalStorage

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

::: dom/base/nsContentUtils.cpp
@@ +9001,5 @@
> +    return access;
> +  }
> +
> +  static const char* kPrefName =
> +    "privacy.restrict3rdpartystorage.partitionedHosts";

Please set the default value of this pref to "accounts.google.com/o/oauth2" in all.js.  I believe that is the right value to fix this bug based on the iframes injected in this bug, bug 1505585 and bug 1504690 and will ensure that this patch will actually fix the bug then.  :-)

::: dom/base/nsGlobalWindowInner.cpp
@@ +4886,5 @@
>    }
>  
> +  // LocalStorage needs to be exposed in every context except for sandboxes and
> +  // NullPrincipals (data: URLs, for instance). But we need to keep data
> +  // separate in some scenarios: private-browsing and trackers.

s/trackers/partitioned trackers/.

@@ +4890,5 @@
> +  // separate in some scenarios: private-browsing and trackers.
> +  // In private-browsing, LocalStorage keeps data in memory, and it shares
> +  // StorageEvents just with other origins in the same private-browsing
> +  // environment.
> +  // For Trackers, we expose a partitioned LocalStorage, which doesn't share

s/Trackers/Partitioned Trackers/

@@ +4894,5 @@
> +  // For Trackers, we expose a partitioned LocalStorage, which doesn't share
> +  // data with other contexts, and it's just in memory. Partitioned localStorage
> +  // is available only for trackers listed in the
> +  // privacy.restrict3rdpartystorage.partitionedHosts pref. See
> +  // nsContentUtils::IsURIInPrefList to know the syntax for the pref value.

Please also mention that this is a temporary web compatibility hack here somewhere?

Comment 16

[(no longer active)]

Sorry, that should be "accounts.google.com/o/oauth2/" (note the trailing slash!)

Comment 17

[Andrea Marchesini [:baku]]

Attachment: part 2 - tests

Comment 18

[Andrew Sutherland [:asuth] (he/him)]

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

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

Thank you for the fantastic tests and adding the extra coverage for the non-tracking-protection cases!

::: toolkit/components/antitracking/test/browser/browser_partitionedLocalStorage_events.js
@@ +174,5 @@
> +
> +  BrowserTestUtils.removeTab(normalTab);
> +});
> +
> +// Similar tests, but no trackers, and it should work.

I parsed this comment to suggest that the iframes weren't tracking iframes, which does not appear to be the case.  Based on my understanding, I think this might work better:
Same as the previous test but with a cookie behavior of BEHAVIOR_ACCEPT instead of BEHAVIOR_REJECT_TRACKER so the iframes get real, persistent localStorage instead of partitioned localStorage.

Coincidentally, this test does leave the tracker localStorage values around which is interesting for the remainder of the tests.

@@ +323,5 @@
> +
> +  BrowserTestUtils.removeTab(normalTab);
> +});
> +
> +// Like the previous test, but without trackers

Again, I find this comment misleading.  Just s/without/accepting/ might work.

@@ +459,5 @@
> +
> +  BrowserTestUtils.removeTab(normalTab);
> +});
> +
> +// Like the previous test, but without trackers

ditto.

Comment 19

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 9023894 [details] [diff] [review]
part 1 - partitioned LocalStorage

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

(I assume browser_partitionedLocalStorage.js has been removed from this patch since it's now in part 2 as well.)

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ea203032bf7
Partitioned localStorage for 3rd party tracker pages, r=ehsan, r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e5960963eb8
Test for Partitioned localStorage, r=asuth

Comment 21

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/2ea203032bf7
https://hg.mozilla.org/mozilla-central/rev/2e5960963eb8

Comment 25

[David Olah]

Ehsan, this bug is still reproducing in Nightly 65.0a1 (2018-11-28). Should I reopen it?

Comment 26

[(no longer active)]

No, please file a new one.  I suspect what you're seeing is probably a completely different issue.  Thanks!

Comment 28

[Sajedul Islam]

I have reproduced this bug with Nightly 65.0a1 (2018-11-06) on Windows 10, 64 bit.

This bug's fix is verified with Beta 65.0b9 !

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

Comment 29

[Ciprian Georgiu, Desktop QA]

Thanks Sajedul, for verifying this bug!

I can also confirm this fix with Beta 65.0b10 on Windows 7 x64 and macOS 10.13.