Closed Bug 1234021 Opened 7 years ago Closed 6 years ago

With "Accept cookies from sites" unchecked, history of back/forward buttons fails with "Error: SecurityError: The operation is insecure." in SessionStorage.jsm

Categories

(Core :: DOM: Service Workers, defect)

43 Branch
x86_64
Linux
defect
Not set
minor

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox44 --- wontfix
firefox45 + wontfix
firefox46 + fixed
firefox47 + fixed

People

(Reporter: turbobeholder, Assigned: nika)

References

(Depends on 1 open bug)

Details

(Keywords: regression, Whiteboard: dom-triaged)

Attachments

(1 file, 1 obsolete file)

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:43.0) Gecko/20100101 Firefox/43.0
Build ID: 20151210085006

Steps to reproduce:

1) open a new empty tab, or "open link in new tab" the following URL (this didn't happen when there's another site before it in tab history):
2) visit http://www.openxcom.com/
3) follow a link within the site, e.g. "mods"
4) right click on "Back" or "Forward" button

Whether NoScript is set to allow or deny does not matter, and the bug happens even when FF is started in Safe Mode.
Same happens with http://lparchive.org/X-COM-UFO-Defense/ - but not in Safe Mode, though NoScript status still doesn't matter.
This also doesn't happen when current or previous URL is HTTPS (though this may cause another bug, with History menu being obviously incorrect).


Actual results:

- nothing visible tab history menu does not appear
- console logs error:
Timestamp: ...
Error: SecurityError: The operation is insecure.
Source File: resource://app/modules/sessionstore/SessionStorage.jsm
Line: 148


Expected results:

context menu: tab history should pop up, like in any other tab.
Severity: normal → minor
Component: Untriaged → Security
OS: Unspecified → Linux
Hardware: Unspecified → x86_64
Component: Security → Session Restore
I was not sure - it was sessionstore, but it throws SecurityError when calling something.

Anyway, it appears on different sites, but not every other, and repeatability varies by site. Like with lparchive.org being immune in Safe Mode, but affected in my normal setup (even though there are no unusual settings for it - no greasemonkey scripts, nothing, just one more site with cookies and scripts denied by default). Sometimes the bug disappears in some tab after some browsing around even on an affected site, but appears again in a new tab with the same site.

Aw, forgot to add ufopaedia.org to the list. =)

Also, it doesn't seem to matter whether an immune URL is in back or forward part of the history - it was enough to visit "about:mozilla" and go back to make history menu work in an affected tab.
I've reported a similar problem in bug 1238178, could you see if any of it applies to you? Eg., other problems like Undo Close Tab not working, pages not being restored, or closed windows being re-opened at restart.

I tested the URLs you listed, but only one of them, lparchive.org, caused the problems I described.

And to confirm, you have cookies disabled for these sites? That agrees with my testing.

But disabling javascript doesn't help? That's not what I'd expect. Could you perhaps try turning off javascript another way, eg. with the QuickJS addon or similar?
Flags: needinfo?(turbobeholder)
STR:
1) Uncheck the option "Accept cookies from sites"
2) Open a new tab and load http://www.openxcom.com/
3) Click on a menu entry like "MODS"
4) Right-click on back button to display history

Result:
No context menu with 2 entries like http://i.imgur.com/YoJtoLb.jpg for back button. Same for forward button.

Error: SecurityError: The operation is insecure.
Source File: resource://app/modules/sessionstore/SessionStorage.jsm
Line: 148

Reg range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=cadeefe3e92791a426c3e950b78e4a38a7093e9a&tochange=1056ac371c0468cde5600a7c5765dc170c5f77e5

Bobby Holley — Bug 1201747 - Don't inspect the subject principal in StorageAllowedForPrincipal. r=mystor
Blocks: 1201747
Status: UNCONFIRMED → NEW
Component: Session Restore → DOM: Service Workers
Ever confirmed: true
Flags: needinfo?(turbobeholder) → needinfo?(bobbyholley)
Keywords: regression
Product: Firefox → Core
Summary: [byzantine] tab history menu fails with SecurityError → With "Accept cookies from sites" unchecked, history of back/forward buttons fails with "Error: SecurityError: The operation is insecure." in SessionStorage.jsm
Bug 1201747 was just fixing a mistake in bug 1184973, so given the regression range, that's the real culprit here.
Flags: needinfo?(bobbyholley) → needinfo?(michael)
Depends on: 1245595
I think I've figured out what's causing this - discussing potential solutions in bug 1245595.
Flags: needinfo?(michael)
Whiteboard: dom-triaged
Still happens in FF 44.0 now.

(In reply to Elhem Enohpi from comment #2)
> And to confirm, you have cookies disabled for these sites? That agrees with
> my testing.
I just tested, you're right.
It depends on cookies - switching Cookie Controller settings to "Accept" or "Accept for a session" makes an affected tab's history work normally after the next click on a link (same host).
This is a simple fix which I believe should fix the problem, and be small enough to be safe to uplift.

We still want to find the desired behavior in bug 1245595 for nightly & future releases.

(mconley: I'm not sure if you're the right person to r? on this, but I figure you'll know who to redirect to)

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ebe80820005
Attachment #8717741 - Flags: review?(mconley)
Comment on attachment 8717741 [details] [diff] [review]
Catch exceptions raised by storage.length in SessionStorage.jsm

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

r=me for the SessionStorage.jsm fix only.

I have some feedback about the test though - I don't think it should land as-is. If you'd like, we can land it in a separate patch while we iterate on it.

::: browser/components/sessionstore/SessionStorage.jsm
@@ +150,5 @@
>  
>      try {
>        let storageManager = aDocShell.QueryInterface(Ci.nsIDOMStorageManager);
>        storage = storageManager.getStorage(window, aPrincipal);
> +      storage.length; // XXX: Bug 1232955 - storage.length can throw, catch that failure

I understand from reading bug 1245595 that this is urgent, but I'm curious why we even need to have access to the result of storageManager.getStorage if we can't do anything with it. Shouldn't we make it so that getStorage throws if the principal has no access to it?

(Just a curiosity, but again, due to urgency, this workaround is fine).

::: browser/components/sessionstore/test/browser_1234021.js
@@ +1,1 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Tests shouldn't be MPL'd - they're public domain. It's fine to leave the license off.

@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Please add:

```JavaScript
"use strict";
```

@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "SessionStorage",

This test has the right idea, but it's oriented in a very non-e10s friendly way, since SessionStorage is definitely not supposed to run in the parent process, and access to contentDocuments / docShells on browsers is no bueno.

So I wouldn't import this here - what I'd suggest is that you communicate with content in order to trigger the behaviours you care about. I'll give you an example below.

@@ +9,5 @@
> +const PAGE_URL = 'http://mochi.test:8888/browser/' +
> +        'browser/components/sessionstore/test/browser_1234021_page.html';
> +const BEHAVIOR_REJECT = 2;
> +
> +let backup_pref; // Backup for restoring the preference after the test runs

sessionstore/test/head.js gives you a "pushPrefs" global that allows you to skip this book-keeping.

```JavaScript
yield pushPrefs([PREF, BEHAVIOR_REJECT]);

```

This will ensure that all processes have the pref set, and takes care of resetting the preference once the test ends.

@@ +18,5 @@
> +  yield BrowserTestUtils.withNewTab({
> +    gBrowser: gBrowser,
> +    url: PAGE_URL
> +  }, function* handler(aBrowser) {
> +    let oldCollect = SessionStorage.collect;

Never used

@@ +19,5 @@
> +    gBrowser: gBrowser,
> +    url: PAGE_URL
> +  }, function* handler(aBrowser) {
> +    let oldCollect = SessionStorage.collect;
> +    // info(Object.keys(aBrowser));

Please remove this dead code

@@ +20,5 @@
> +    url: PAGE_URL
> +  }, function* handler(aBrowser) {
> +    let oldCollect = SessionStorage.collect;
> +    // info(Object.keys(aBrowser));
> +    info(aBrowser.contentDocument.documentURIObject);

I don't think either of these info's add much.

@@ +23,5 @@
> +    // info(Object.keys(aBrowser));
> +    info(aBrowser.contentDocument.documentURIObject);
> +    info(aBrowser);
> +
> +    // Create a fake frames object to test the logic.

I assume exercising the following steps will ensure that the logic of your patch is sound:

1) Set the network.cookie.cookieBehavior pref to reject (2)
2) Have content trigger the SessionStorageInternal.collect routine
3) Have a MessageQueue flush succeed without throwing

You've already got part 1 and 2, actually, since after the frame tree is rendered for your browser_1234021_page.html, a SessionStorage collection will automatically be scheduled on a later tick of the event loop.

So what you really need to do, is just make sure that you can get a flush off of the browser, and then you're done.

Like this:

```JavaScript

yield BrowserTestUtils.withNewTab({
  gBrowser: gBrowser,
  url: PAGE_URL,
}, function* handler(browser) {
  // At this point, a SessionStorage collection should have been
  // queued. Let's make sure we can flush it out.
  yield TabStateFlusher.flush(browser);
  // mlayzell: ^-- without your patch, this should time out.
});
```
Attachment #8717741 - Flags: review?(mconley) → review+
I'm so sorry for the delay on this - I thought I had posted this ages ago, but then I realized I hadn't :S
Attachment #8723337 - Flags: review?(mconley)
Attachment #8717741 - Attachment is obsolete: true
Assignee: nobody → michael
Comment on attachment 8723337 [details] [diff] [review]
Catch exceptions raised by storage.length in SessionStorage.jsm

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

If you can confirm that this test fails without your patch, then LGTM.
Attachment #8723337 - Flags: review?(mconley) → review+
Comment on attachment 8723337 [details] [diff] [review]
Catch exceptions raised by storage.length in SessionStorage.jsm

Approval Request Comment
[Feature/regressing bug #]: Bug 1201747
[User impact if declined]: Comment 0
[Describe test coverage new/current, TreeHerder]: Has automated tests
[Risks and why]: Should be pretty safe
[String/UUID change made/needed]: None.
Attachment #8723337 - Flags: approval-mozilla-beta?
Attachment #8723337 - Flags: approval-mozilla-aurora?
This should probably land on m-c first before uplift. We are also pretty late in the beta cycle so this may not make 45. Aurora uplift should be fine.
Is bug 1230769 related to this?
(In reply to xuu from comment #20)
> Is bug 1230769 related to this?

Yes, I believe that this patch should fix that problem.
https://hg.mozilla.org/mozilla-central/rev/76b2ed3339e2
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Comment on attachment 8723337 [details] [diff] [review]
Catch exceptions raised by storage.length in SessionStorage.jsm

This is too late in the 45 cycle as we are now doing RC. We already shipped 44 with it.
But taking it in 46.
Attachment #8723337 - Flags: approval-mozilla-beta?
Attachment #8723337 - Flags: approval-mozilla-beta-
Attachment #8723337 - Flags: approval-mozilla-aurora?
Attachment #8723337 - Flags: approval-mozilla-aurora+
Duplicate of this bug: 1232955
You need to log in before you can comment on or make changes to this bug.