1656768 - Should "cross-origin-opener-policy: same-origin" clear sessionStorage after redirects in the same tab (e.g. OAuth flow)?

Status: RESOLVED FIXED

(Core :: Storage: localStorage & sessionStorage, defect, P2)

Description

[David Evans]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/84.0.4147.89 Safari/537.36

Steps to reproduce:

I use window.sessionStorage to store a nonce during an OAuth login flow. The rough flow is:

1. user navigates to http://localhost:8000
2. generate random nonce, store in sessionStorage, redirect user to http://localhost:9000/state=<nonce>
3. user logs in, redirected back to http://localhost:8000/#<state>
4. check state against sessionStorage

(note that I haven't tested this on non-localhost domains or with https. I don't expect that makes a difference but it's worth mentioning)

Actual results:

After adding cross-origin-opener-policy: same-origin, I see that sessionStorage is always empty in step 4. This seems counter-intuitive and does not match Chrome's behaviour (though I don't know if Chrome fully implements cross-origin-opener-policy yet). Without the opener policy, it works fine.

Expected results:

The MDN page for sessionStorage (https://developer.mozilla.org/en-US/docs/Web/API/Window/sessionStorage) says that the storage is cleared when the tab is closed, but survives page reloads and restores. It doesn't explicitly state the behaviour when the tab navigates elsewhere then back.

As a workaround I can use localStorage, but I would prefer to use sessionStorage to avoid letting the data live longer than necessary (and to keep separate tabs independent, in case a user tries to log in using multiple tabs simultaneously).

I could also remove cross-origin-opener-policy: same-origin, but this seems like a sensible new security precaution to add so I'd rather keep it.

Is this intended behaviour or a side-effect of origin isolation?

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Comment 2

[Anne (:annevk)]

sessionStorage should be preserved and I thought the way we accomplished that was through copying (rather than a higher-level data structure).

Comment 3

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

This is an interesting bug. I believe we should be preserving session storage in this case using session restore. Perhaps we're somehow not detecting the new session storage entries before we're doing the process switch? It does seem from SessionStorage.cpp that we notify observers async of changes, which could theoretically mean that session storage is behind. https://searchfox.org/mozilla-central/rev/9d2d8bbeb932e2cfaeea9e5723df85445b36e9b1/dom/storage/SessionStorage.cpp#147-148

This issue should be fixed by bug 1654080, when we decouple Session Storage preservation from session restore, but it would be good to make sure we keep session restore working as well.

Alphan, any chance you could look into why we're not copying session storage between processes in this case?

Comment 4

[Alphan Chen [:alchen]]

I explain the behavior of process switch by test_sessionStorageReplace.html.
https://searchfox.org/mozilla-central/rev/b4f3ce16c5099cf068fb023341959a0457adec9e/dom/tests/mochitest/sessionstorage/test_sessionStorageReplace.html

We can focus on the bottom of this test.

1. Open a new window with "http://example.org/tests/dom/tests/mochitest/sessionstorage/frameReplace.html?init&window"

• https://searchfox.org/mozilla-central/rev/b4f3ce16c5099cf068fb023341959a0457adec9e/dom/tests/mochitest/sessionstorage/test_sessionStorageReplace.html#46
• (In frameReplace.html) We create three items in the sessionStorage. ("A":"1", "B":"2", "C":"3").

2. Change the location to "http://example.com/tests/dom/tests/mochitest/sessionstorage/frameReplace.html?check&window"

• https://searchfox.org/mozilla-central/rev/b4f3ce16c5099cf068fb023341959a0457adec9e/dom/tests/mochitest/sessionstorage/test_sessionStorageReplace.html#31
• (In frameReplace.html) Check if the items that are written in step 1 still exist.

3. Change the location to "http://example.org:80/tests/dom/tests/mochitest/sessionstorage/frameReplace.html?clean&"

• https://searchfox.org/mozilla-central/rev/b4f3ce16c5099cf068fb023341959a0457adec9e/dom/tests/mochitest/sessionstorage/test_sessionStorageReplace.html#36
• (In frameReplace.html) Recheck and clean the sessionStorage

4. Close the window.

Comment 5

[Alphan Chen [:alchen]]

When enabling fission,

1. Open a new window with "http://example.org/tests/dom/tests/mochitest/sessionstorage/frameReplace.html?init&window"

2. Change the location to "http://example.com/tests/dom/tests/mochitest/sessionstorage/frameReplace.html?check&window"

During the process switch, there are three steps related to sessionStorge restore:

1. prepareToChangeRemoteness() would trigger a sessionRestore data collection.
   https://searchfox.org/mozilla-central/rev/b4f3ce16c5099cf068fb023341959a0457adec9e/browser/components/sessionstore/SessionStore.jsm#6056
2. The storage data that sessionRestore collects is "storage":{"http://example.org":{"C":"3","B":"2","A":"1"}}}".
   https://searchfox.org/mozilla-central/rev/b4f3ce16c5099cf068fb023341959a0457adec9e/toolkit/components/sessionstore/SessionStoreListener.cpp#640
   https://searchfox.org/mozilla-central/source/toolkit/components/sessionstore/SessionStoreUtils.cpp#1082-1151,1164
3. finishTabRemotenessChange() trigger the sessionStorage restore with {"http://example.org":{"C":"3","B":"2","A":"1"}}}".
   https://searchfox.org/mozilla-central/rev/b4f3ce16c5099cf068fb023341959a0457adec9e/browser/components/sessionstore/SessionStore.jsm#6092

3. Change the location to "http://example.org:80/tests/dom/tests/mochitest/sessionstorage/frameReplace.html?clean&"

During the process switch, there are three steps related to sessionStorge restore:

1. prepareToChangeRemoteness() would trigger a sessionRestore data collection.
2. The storage data that sessionRestore collects is "storage":null"
3. There is no sessionStorage data to restore when doing finishTabRemotenessChange().

Comment 6

[Alphan Chen [:alchen]]

3. Change the location to "http://example.org:80/tests/dom/tests/mochitest/sessionstorage/frameReplace.html?clean&"

During the process switch, there are three steps related to sessionStorge restore:

1. prepareToChangeRemoteness() would trigger a sessionRestore data collection.
2. The storage data that sessionRestore collects is "storage":null"

In the current implementation, we only get the sessionStoage of the current document.
In this case, the origin of the current document is "http://example.com".
https://searchfox.org/mozilla-central/rev/26b13464c2beb26e0d864d561c30e817a85c348a/toolkit/components/sessionstore/SessionStoreUtils.cpp#1107

Comment 7

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1656768 - Add a test to ensure sessionStorage is propagated from non-cross-origin-isolated to cross-origin-isolated window and vice versa;

Comment 8

[Tom Tung [:tt, :ttung]]

So the added test in D97763 fails while enabling fission. (succeeds while disabling fission)

I took a look into it and found:

1. While navigate/redirect from a normal(non-cross-origin-isolated) window to a cross-origin-isolated window, the top-level browsing context id for the window is changed. From another perspective, the top-level browsing context id changes after process switching in the same tab.
2. In non-fission mode, SessionStorage data is copied through SessionStoreUtilis. However, this function isn't called while enabling fission.

SessionStorage in a tab is preserved based on the top-level BrowsingContext id so that this wasn't completely fixed by bug 1654080 (since the top-level browsing context id has been changed after navigation). Also, note that Bug 1654080 makes decoupling SessionStorage preservation from session restore possible since SessionStorage is now syncing to the parent process frequently, but we are not there yet. To achieve that, we still need to adjust some pieces of code in SessionStore.

To fix the issue here, I think I need one more patch that makes SessionStore be able to copy the SessionStorage to the target global even when fission is enabled.

Another option is to somehow make a singleton on the parent process be able to maintain a hashmap that maps old top-level browsing context id and new top-level browsing context id in a tab. So that, in SessionStorageManager, we can copy the right SessionStorage data into the target content process.

Comment 9

[Tom Tung [:tt, :ttung]]

If I remove this if-check (sessionHistoryInParent), the test will succeed. And it seems that the pref for sessionHistoryInParent is enabled when I add --enable-fission.

Comment 10

[Tom Tung [:tt, :ttung]]

Attachment: Bug 1656768 - Propagate SessionStorage data in BackgroundSessionStorageManager to the new top browsing context while replacing the top context id;

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/312e94b2aad7
Add a test to ensure sessionStorage is propagated from non-cross-origin-isolated to cross-origin-isolated window and vice versa; r=nika
https://hg.mozilla.org/integration/autoland/rev/0109e94bd4f9
Propagate SessionStorage data in BackgroundSessionStorageManager to the new top browsing context while replacing the top context id; r=nika,dom-workers-and-storage-reviewers,asuth

Comment 12

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/312e94b2aad7
https://hg.mozilla.org/mozilla-central/rev/0109e94bd4f9