Closed Bug 1556082 Opened 6 months ago Closed 6 months ago

Storage::mStoragePrincipal/LSObject::mStoragePrincipalInfo should be changed

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox69 --- fixed

People

(Reporter: ehsan, Assigned: baku)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Document::mStoragePrincipal is mutable, so we shouldn't store copies of it around.

Right?

Flags: needinfo?(amarchesini)

At the moment, mStoragePrincipal is not mutable.
Document::GetEffectiveStoragePrincipal() can return mStoragePrincipal or the NodePrincipal() based on the current storage access policy, but we never update mStoragePrincipal.

Flags: needinfo?(amarchesini)

Yes, you're right. What I meant was "semantically mutable".

Let's consider this sequence of events:

This is all that I could gather by code inspection. Perhaps the last bullet point is fine but it seems like there are some bugs in the other ones. Do you mind taking a careful look at how this setup all fits together and whether these copies of the principal on the storage objects ever will have wrong/unexpected values? (I'm not super familiar with the storage code so some of my conclusions above may be mistaken...)

Flags: needinfo?(amarchesini)
Summary: Storage::mStoragePrincipal should be changed → Storage::mStoragePrincipal/LSObject::mStoragePrincipalInfo should be changed
Blocks: 1556812

This is wrong. When the storage access is granted, the window should instantiate a new LSObject. The 2 objects (the previous localStorage and the new one), will be both active.

Assignee: nobody → amarchesini
Flags: needinfo?(amarchesini)

This behavior is similar to indexedDB and other QuotaManager-related APIs.

And what about the other problems? :-)

Flags: needinfo?(amarchesini)
Keywords: leave-open

My patch is specifically for this bug. Let me comment your STR step by step:

  • Third party page accesses window.localStorage (L1). We examine the effective storage principal and create an LSObject instance using the resulting storage principal (P1).
  • Page sets up a storage event listener on that object.
  • Now storage access is granted in the third-party page. The result of computing the effective storage principal will be different from this point on (P2).

The page must set a new event listener on the new localStorage. The old one will be able to receive events only from the partitioned cookie jar.

This is fine with my patch. EnsureObserver() will be called on the new localStorage.

Same here.

The new L1 will be able to talk with the first-party localStorage objects only. This is expected.

Flags: needinfo?(amarchesini)

Ah very nice! Thanks a lot for explaining, this wasn't really clear to me at all to be honest. :-) Now it all makes perfect sense.

Keywords: leave-open
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1b99db031cc0
LocalStorage must be recreated when storage-access is granted, r=Ehsan,asuth
Status: NEW → RESOLVED
Closed: 6 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
Regressions: 1558420
You need to log in before you can comment on or make changes to this bug.