Last Comment Bug 463206 - SessionStore does not correctly restore text data when subframes are involved
: SessionStore does not correctly restore text data when subframes are involved
Status: VERIFIED FIXED
: [sg:high]
: verified1.8.1.19, verified1.9.0.5
Product: Firefox
Classification: Client Software
Component: Session Restore
: unspecified
: All All
: -- normal (vote)
: Firefox 3.1b2
Assigned To: Simon Bünzli
: session.restore
:
:
:
  Show dependency treegraph
 
Reported: 2008-11-05 04:38 PST by moz_bug_r_a4
Modified: 2010-02-13 12:42 PST (History)
13 users (show)
dveditz: blocking1.9.0.5+
dveditz: wanted1.9.0.x+
dveditz: blocking1.8.1.19+
dveditz: wanted1.8.1.x+
asac: wanted1.8.0.x-
See Also:
Crash Signature:


Attachments
trunk patch (for legacy routines) and tests (7.75 KB, patch)
2008-11-05 12:57 PST, Simon Bünzli
no flags Details | Diff | Splinter Review
trunk patch (for legacy routines) and tests (7.75 KB, patch)
2008-11-05 13:07 PST, Simon Bünzli
dietrich: review+
mbeltzner: approval1.9.1b2+
Details | Diff | Splinter Review
branch patch (3.15 KB, patch)
2008-11-05 13:08 PST, Simon Bünzli
dietrich: review+
dveditz: approval1.9.0.5+
Details | Diff | Splinter Review
branch patch (1.8.1) (3.21 KB, patch)
2008-11-12 11:49 PST, Simon Bünzli
dveditz: approval1.8.1.19+
Details | Diff | Splinter Review
[checked in] 1.9 branch patch (1.23 KB, patch)
2008-11-19 16:02 PST, Simon Bünzli
no flags Details | Diff | Splinter Review
[checked in] 1.8.1 branch patch (1.27 KB, patch)
2008-11-19 16:02 PST, Simon Bünzli
no flags Details | Diff | Splinter Review

Summon comment box

Description moz_bug_r_a4 2008-11-05 04:38:39 PST
This is fx3/fx2 only.

There are two problems.

1.
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/components/sessionstore/src/nsSessionStore.js&rev=1.102&mark=1081-1088#1081
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/components/sessionstore/src/nsSessionStore.js&rev=1.102&mark=1730-1734#1730

When saving text data, "0|1|2|" is used as the id prefix for frames[0][1][2].
But when restoring text data, "2|1|0|" is used as the id prefix for
frames[0][1][2].

2.
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/browser/components/sessionstore/src/nsSessionStore.js&rev=1.102&mark=1703#1701

SessionStore tries to inject text data of a top-level document into subframes.
Comment 1 moz_bug_r_a4 2008-11-05 04:40:08 PST
Created attachment 346446 [details]
testcase 1

This demonstrates the problem 1.
Comment 2 moz_bug_r_a4 2008-11-05 04:41:21 PST
Created attachment 346447 [details]
testcase 2

This demonstrates the problem 2.
Comment 3 moz_bug_r_a4 2008-11-05 04:42:42 PST
Created attachment 346448 [details]
testcase 3 - XSS using the problem 1

This tries to inject text data into http://htmledit.squarefree.com/
Comment 4 moz_bug_r_a4 2008-11-05 04:43:53 PST
Created attachment 346449 [details]
testcase 4 - XSS using the problem 2

This tries to inject text data into http://htmledit.squarefree.com/
Comment 5 moz_bug_r_a4 2008-11-05 05:36:06 PST
testcase 1 and 3 do not work in https:.
Comment 6 moz_bug_r_a4 2008-11-05 05:39:20 PST
Created attachment 346454 [details]
testcase 1 (+https)

This demonstrates the problem 1.
Comment 7 moz_bug_r_a4 2008-11-05 05:41:27 PST
Created attachment 346456 [details]
testcase 3 (+https) - XSS using the problem 1

This tries to inject text data into http://htmledit.squarefree.com/
Comment 8 Simon Bünzli 2008-11-05 12:57:01 PST
Created attachment 346519 [details] [review]
trunk patch (for legacy routines) and tests
Comment 9 Simon Bünzli 2008-11-05 13:07:44 PST
Created attachment 346522 [details] [review]
trunk patch (for legacy routines) and tests
Comment 10 Simon Bünzli 2008-11-05 13:08:07 PST
Created attachment 346523 [details] [review]
branch patch
Comment 11 Dietrich Ayala (:dietrich) 2008-11-10 21:14:28 PST
Comment on attachment 346522 [details] [review]
trunk patch (for legacy routines) and tests

r=me
Comment 12 Dietrich Ayala (:dietrich) 2008-11-10 21:18:52 PST
Comment on attachment 346523 [details] [review]
branch patch

>Index: browser/components/sessionstore/src/nsSessionStore.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/sessionstore/src/nsSessionStore.js,v
>retrieving revision 1.102
>diff -u -8 -d -p -r1.102 nsSessionStore.js
>--- browser/components/sessionstore/src/nsSessionStore.js	24 Oct 2008 20:33:25 -0000	1.102
>+++ browser/components/sessionstore/src/nsSessionStore.js	5 Nov 2008 21:06:57 -0000
>@@ -1054,16 +1054,20 @@ SessionStoreService.prototype = {
>   _saveTextData: function sss_saveTextData(aPanel, aTextarea) {
>     var id = aTextarea.id ? "#" + aTextarea.id :
>                             aTextarea.name;
>     if (!id
>       || !(aTextarea instanceof Ci.nsIDOMHTMLTextAreaElement 
>       || aTextarea instanceof Ci.nsIDOMHTMLInputElement && aTextarea.type != "password")) {
>       return false; // nothing to save
>     }
>+    if (/^(?:\d+\|)+/.test(id)) {
>+      // text could be restored into a subframe, so skip it (see bug 463206)
>+      return false;
>+    }
>     
>     if (!aPanel.__SS_text) {
>       aPanel.__SS_text = [];
>       aPanel.__SS_text._refs = [];
>     }
>     
>     // get the index of the reference to the text element
>     var ix = aPanel.__SS_text._refs.indexOf(aTextarea);
>@@ -1695,17 +1699,17 @@ SessionStoreService.prototype = {
>     // wait for the top frame to be loaded completely
>     if (!aEvent || !aEvent.originalTarget || !aEvent.originalTarget.defaultView || aEvent.originalTarget.defaultView != aEvent.originalTarget.defaultView.top) {
>       return;
>     }
>     
>     var textArray = this.__SS_restore_text ? this.__SS_restore_text.split(" ") : [];
>     function restoreTextData(aContent, aPrefix) {
>       textArray.forEach(function(aEntry) {
>-        if (/^((?:\d+\|)*)(#?)([^\s=]+)=(.*)$/.test(aEntry) && (!RegExp.$1 || RegExp.$1 == aPrefix)) {
>+        if (/^((?:\d+\|)*)(#?)([^\s=]+)=(.*)$/.test(aEntry) && RegExp.$1 == aPrefix) {
>           var document = aContent.document;
>           var node = RegExp.$2 ? document.getElementById(RegExp.$3) : document.getElementsByName(RegExp.$3)[0] || null;
>           if (node && "value" in node) {
>             node.value = decodeURI(RegExp.$4);
>             
>             var event = document.createEvent("UIEvents");
>             event.initUIEvent("input", true, true, aContent, 0);
>             node.dispatchEvent(event);
>@@ -1724,17 +1728,17 @@ SessionStoreService.prototype = {
>                 }
>               }, 0, aData.innerHTML);
>       }
>       if (aData.scroll && /(\d+),(\d+)/.test(aData.scroll)) {
>         aContent.scrollTo(RegExp.$1, RegExp.$2);
>       }
>       for (var i = 0; i < aContent.frames.length; i++) {
>         if (aData.children && aData.children[i]) {
>-          restoreTextDataAndScrolling(aContent.frames[i], aData.children[i], i + "|" + aPrefix);
>+          restoreTextDataAndScrolling(aContent.frames[i], aData.children[i], aPrefix + i + "|");
>         }
>       }
>     }
>     
>     var content = aEvent.originalTarget.defaultView;
>     if (this.currentURI.spec == "about:config") {
>       // unwrap the document for about:config because otherwise the properties
>       // of the XBL bindings - as the textbox - aren't accessible (see bug 350718)
Comment 13 Dietrich Ayala (:dietrich) 2008-11-11 14:49:12 PST
Comment on attachment 346522 [details] [review]
trunk patch (for legacy routines) and tests

requesting a= for b2 due to the sg:high.
Comment 14 Mike Beltzner [:beltzner] 2008-11-11 21:12:14 PST
Comment on attachment 346522 [details] [review]
trunk patch (for legacy routines) and tests

a=beltzner, sound rationale
Comment 15 Dietrich Ayala (:dietrich) 2008-11-12 11:21:21 PST
Simon, is 1.8.1 branch affected? If so, can you make a patch for that as well?
Comment 16 Simon Bünzli 2008-11-12 11:49:14 PST
Created attachment 347822 [details] [review]
branch patch (1.8.1)
Comment 17 Daniel Veditz 2008-11-13 10:20:13 PST
Comment on attachment 347822 [details] [review]
branch patch (1.8.1)

Approved for 1.8.1.18, a=dveditz for release-drivers
Comment 18 Daniel Veditz 2008-11-13 10:20:27 PST
Comment on attachment 346523 [details] [review]
branch patch

Approved for 1.9.0.5, a=dveditz for release-drivers
Comment 19 Myk Melez [:myk] 2008-11-14 14:29:19 PST
Trunk patch pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/rev/5e1a889aad0e
Comment 20 Samuel Sidler (old account; do not CC) 2008-11-19 15:33:34 PST
Can we get this landed in 1.9.0 and 1.8.1? I don't think this is part of the roll up patch...
Comment 21 Simon Bünzli 2008-11-19 16:02:19 PST
Created attachment 349067 [details] [review]
[checked in] 1.9 branch patch

That's the only bit that I embarrassingly forgot to include in the roll-up patch.
Comment 22 Simon Bünzli 2008-11-19 16:02:44 PST
Created attachment 349068 [details] [review]
[checked in] 1.8.1 branch patch
Comment 23 Simon Bünzli 2008-11-19 16:03:38 PST
I'll need somebody to land these patches for me, though. Thanks in advance!
Comment 24 Justin Dolske [:Dolske] 2008-11-20 14:05:58 PST
Comment on attachment 349067 [details] [review]
[checked in] 1.9 branch patch

Checked in on branch. (looks like the rest of this previously landed as bug 464620)

Checking in browser/components/sessionstore/src/nsSessionStore.js;
  new revision: 1.107; previous revision: 1.106
Comment 25 Justin Dolske [:Dolske] 2008-11-20 14:13:58 PST
Comment on attachment 349068 [details] [review]
[checked in] 1.8.1 branch patch

Checked in on 1.8 branch. (looks like the rest of this previously landed as bug
464620)

Checking in browser/components/sessionstore/src/nsSessionStore.js;
  new revision: 1.5.2.54; previous revision: 1.5.2.53
Comment 26 Al Billings [:abillings] 2008-11-25 17:32:21 PST
Verified for 1.8.1.19 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.19pre) Gecko/2008112503 BonEcho/2.0.0.19pre. 

Verified for 1.9.0.5 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.5pre) Gecko/2008112505 GranParadiso/3.0.5pre. 

Verified for trunk with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081123 Minefield/3.1b2pre.
Comment 27 Alexander Sack 2008-12-16 11:30:43 PST
no issue on 1.8.0.

Note You need to log in before you can comment on or make changes to this bug.