463206 - SessionStore does not correctly restore text data when subframes are involved

Status: VERIFIED FIXED

(Firefox :: Session Restore, defect)

Description

[moz_bug_r_a4]

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]

Attachment: testcase 1

This demonstrates the problem 1.

Comment 2

[moz_bug_r_a4]

Attachment: testcase 2

This demonstrates the problem 2.

Comment 3

[moz_bug_r_a4]

Attachment: testcase 3 - XSS using the problem 1

This tries to inject text data into http://htmledit.squarefree.com/

Comment 4

[moz_bug_r_a4]

Attachment: testcase 4 - XSS using the problem 2

This tries to inject text data into http://htmledit.squarefree.com/

Comment 5

[moz_bug_r_a4]

testcase 1 and 3 do not work in https:.

Comment 6

[moz_bug_r_a4]

Attachment: testcase 1 (+https)

This demonstrates the problem 1.

Comment 7

[moz_bug_r_a4]

Attachment: testcase 3 (+https) - XSS using the problem 1

This tries to inject text data into http://htmledit.squarefree.com/

Comment 8

[Simon Bünzli]

Attachment: trunk patch (for legacy routines) and tests

Comment 9

[Simon Bünzli]

Attachment: trunk patch (for legacy routines) and tests

Comment 10

[Simon Bünzli]

Attachment: branch patch

Comment 11

[Dietrich Ayala (:dietrich)]

Comment on attachment 346522 [details] [diff] [review]
trunk patch (for legacy routines) and tests

r=me

Comment 12

[Dietrich Ayala (:dietrich)]

Comment on attachment 346523 [details] [diff] [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)]

Comment on attachment 346522 [details] [diff] [review]
trunk patch (for legacy routines) and tests

requesting a= for b2 due to the sg:high.

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 346522 [details] [diff] [review]
trunk patch (for legacy routines) and tests

a=beltzner, sound rationale

Comment 15

[Dietrich Ayala (:dietrich)]

Simon, is 1.8.1 branch affected? If so, can you make a patch for that as well?

Comment 16

[Simon Bünzli]

Attachment: branch patch (1.8.1)

Comment 17

[Daniel Veditz [:dveditz]]

Comment on attachment 347822 [details] [diff] [review]
branch patch (1.8.1)

Approved for 1.8.1.18, a=dveditz for release-drivers

Comment 18

[Daniel Veditz [:dveditz]]

Comment on attachment 346523 [details] [diff] [review]
branch patch

Approved for 1.9.0.5, a=dveditz for release-drivers

Comment 19

[Myk Melez [:myk] [@mykmelez]]

Trunk patch pushed to mozilla-central: http://hg.mozilla.org/mozilla-central/rev/5e1a889aad0e

Comment 20

[Samuel Sidler (old account; do not CC)]

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]

Attachment: [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]

Attachment: [checked in] 1.8.1 branch patch

Comment 23

[Simon Bünzli]

I'll need somebody to land these patches for me, though. Thanks in advance!

Comment 24

[Justin Dolske [:Dolske]]

Comment on attachment 349067 [details] [diff] [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]]

Comment on attachment 349068 [details] [diff] [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 - ex-MoCo]]

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]

no issue on 1.8.0.