1183968 - Make sessionStorage obey the third-party cookies pref

Status: RESOLVED WONTFIX

(Core :: DOM: Core & HTML, defect)

Description

[Nika Layzell [:nika] (ni? for response)]

Currently sessionStorage doesn't check DOMStorage::CanUseStorage when being accessed on the window object. Instead, CanUseStorage is checked whenever a method on the sessionStorage object (such as getItem) is called.

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Attachment: Make sessionStorage obey the third-party cookies pref

This patch changes the behavior to be consistent with localStorage, in that it checks DOMStorage::Can
UseStorage when the sessionStorage property on window is being accessed.

I also copy over some tests from localStorage for these cookie preferences which are checked in CanUs
eStorage.

Comment 2

[(no longer active)]

Comment on attachment 8633880 [details] [diff] [review]
Make sessionStorage obey the third-party cookies pref

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

This looks fine to me, but I think Honza should look at this.

::: dom/tests/mochitest/sessionstorage/interOriginFrame.js
@@ +49,5 @@
> +
> +function finishTest()
> +{
> +  try {
> +    localStorage.clear();

This isn't needed.  Some of the other things in this file aren't used either AFAICT.

Comment 3

[Honza Bambas (:mayhemer)]

Comment on attachment 8633880 [details] [diff] [review]
Make sessionStorage obey the third-party cookies pref

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

Witch this change please also update https://developer.mozilla.org/en-US/docs/Mozilla/Gecko/DOM_Storage_implementation_notes#Access_security_checks

::: dom/base/nsGlobalWindow.cpp
@@ +10401,5 @@
>      }
>    }
>  
>    if (!mSessionStorage) {
> +    if (!DOMStorage::CanUseStorage(this)) {

what is the 'this' here?  This accepts a storage object.  It's passed in only to update the mSessionOnly flag on the storage.  It has no meaning on sessionStorage since that is session-only from its nature.  Just pass null or omit and you are OK to go here.  GetLocalStorage also passes null (a default value).

Comment 4

[Nika Layzell [:nika] (ni? for response)]

Attachment: Update sessionStorage to use common StorageAllowedForWindow logic

Updated version of the patch which instead uses the StorageAllowedForWindow logic being implemented in bug 1184973. The new DOMStorage::CanUseStorage logic is being implemented in bug 536509.

Comment 5

[(no longer active)]

Comment on attachment 8635317 [details] [diff] [review]
Update sessionStorage to use common StorageAllowedForWindow logic

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

Mostly looks good, except for the below.

::: dom/base/nsGlobalWindow.cpp
@@ +10401,5 @@
>      }
>    }
>  
>    if (!mSessionStorage) {
> +    if (!DOMStorage::CanUseStorage(this)) {

Why this function?  Shouldn't we use the one you're adding to nsContentUtils?

@@ +10416,5 @@
>      // don't allow access to sessionStorage.
>      if (!mDoc) {
>        aError.Throw(NS_ERROR_FAILURE);
>        return nullptr;
>      }

This can go away from here.  You should do this check in the other bug.

Comment 6

[Nika Layzell [:nika] (ni? for response)]

(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #5)
> Why this function?  Shouldn't we use the one you're adding to nsContentUtils?

DOMStorage::CanUseStorage dispatches to nsContentUtils::StorageAllowedForWindow as of bug 536509. It just also performs some extra additional logic which I wasn't sure I wanted to move into StorageAllowedForWindow.

> This can go away from here.  You should do this check in the other bug.

It'll be gone in the next patch I put up.

Comment 7

[Nika Layzell [:nika] (ni? for response)]

Attachment: Update sessionStorage to use common StorageAllowedForWindow logic

Updated version of patch. Part 3 of new storage logic. Full tree here: https://github.com/mystor/mozilla-central/tree/storage_pref

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d48360c5cc3e

Comment 8

[Nika Layzell [:nika] (ni? for response)]

After a discussion on dev-platform, we decided that we don't want to prevent sessionstorage in third party iframes, as it is already double-keyed and not persisted. Thus this patch is unnecessary.