bugzilla.mozilla.org has resumed normal operation. Attachments prior to 2014 will be unavailable for a few days. This is tracked in Bug 1475801.
Please report any other irregularities here.

Make sessionStorage obey the third-party cookies pref

RESOLVED WONTFIX

Status

()

Core
DOM
RESOLVED WONTFIX
3 years ago
3 years ago

People

(Reporter: Nika, Assigned: Nika)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

3 years ago
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.
(Assignee)

Comment 1

3 years ago
Created attachment 8633880 [details] [diff] [review]
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.
Attachment #8633880 - Flags: feedback?(ehsan)
(Assignee)

Updated

3 years ago
Assignee: nobody → michael
(Assignee)

Updated

3 years ago
Blocks: 1147820
Depends on: 536509

Comment 2

3 years ago
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.
Attachment #8633880 - Flags: feedback?(ehsan) → feedback?(honzab.moz)
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).
Attachment #8633880 - Flags: feedback?(honzab.moz) → feedback+
(Assignee)

Comment 4

3 years ago
Created attachment 8635317 [details] [diff] [review]
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.
Attachment #8635317 - Flags: review?(ehsan)
(Assignee)

Updated

3 years ago
Depends on: 1184973
(Assignee)

Updated

3 years ago
Attachment #8633880 - Attachment is obsolete: true

Updated

3 years ago
Keywords: dev-doc-needed

Comment 5

3 years ago
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.
Attachment #8635317 - Flags: review?(ehsan) → review-
(Assignee)

Comment 6

3 years ago
(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.
(Assignee)

Comment 7

3 years ago
Created attachment 8641986 [details] [diff] [review]
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
Attachment #8635317 - Attachment is obsolete: true
Attachment #8641986 - Flags: review?(ehsan)

Updated

3 years ago
Attachment #8641986 - Flags: review?(ehsan) → review+
(Assignee)

Comment 8

3 years ago
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.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → WONTFIX
Keywords: dev-doc-needed → dev-doc-complete
(Assignee)

Updated

3 years ago
See Also: → bug 1245595
You need to log in before you can comment on or make changes to this bug.