Closed Bug 378160 Opened 17 years ago Closed 15 years ago

tabbrowser.xml needs fix

Categories

(SeaMonkey :: UI Design, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mv_van_rantwijk, Assigned: neil)

Details

(Keywords: fixed-seamonkey2.0, topcrash, Whiteboard: [sg:dos?])

Attachments

(3 files, 7 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a4pre) Gecko/20070409 MultiZilla/1.8.3.0m SeaMonkey/1.5a
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9a4pre) Gecko/20070420 MultiZilla/1.9.0.0a SeaMonkey/1.5a

Method restoreTab in tabbrowser.xml needs to be patched to fix a security issue.  I have patched MultiZilla little over a week ago now, and it has been confirmed to have fixed the bug i.e. I can write a patch for Seamonkey in case you want.

Note: I have contacted Neil Parkway per e-mail about the bug, and decided to add a bug for it (before he replied to my e-mail).

Reproducible: Always

Steps to Reproduce:
1.
2.
3.
Version: unspecified → Trunk
Can you describe the bug, and attach a patch?
Basically everything after this line:

http://lxr.mozilla.org/seamonkey/source/suite/browser/tabbrowser.xml#1167

in method restoreTab will be skipped when you click cancel in SeaMonkeys postdata confirmation dialog.  The result is a crippled tab: without progress listener and security lock!
Attached patch cvs diff -u tabbrowser.xml (obsolete) — Splinter Review
Proposed patch v1.0

This patch changes the order and to checks nsISHEntry.postData
Attached patch cvs diff -u tabbrowser.xml (obsolete) — Splinter Review
Simplified patch
I don't see anything by that name in the toolkit version of tabbrowser.xml -- am I just missing it under a different name or is this a suite-only feature?
Assignee: guifeatures → neil
Status: UNCONFIRMED → NEW
Ever confirmed: true
(In reply to comment #6)
> I don't see anything by that name in the toolkit version of tabbrowser.xml --
> am I just missing it under a different name or is this a suite-only feature?

Yes, this is a SeaMonkey suite only feature.
We have to go back before enabling the filters... that's the whole reason we remove the filters!

Since loading about:blank is a pretty essential part of caching a tab, it would seem that we have three possibilities:
1. Don't allow tabs with postData to be cached
2. Ignore the error and leave the blank page
3. Wait for the postdata cancel error page
(In reply to comment #8)
> We have to go back before enabling the filters... that's the whole reason we
> remove the filters!
> 
> Since loading about:blank is a pretty essential part of caching a tab, it would
> seem that we have three possibilities:
> 1. Don't allow tabs with postData to be cached
> 2. Ignore the error and leave the blank page
> 3. Wait for the postdata cancel error page

Or check for postData and go back one page?

Attached patch cvs diff -u tabbrowser.xml (obsolete) — Splinter Review
This patch (v1.3) restores tabs with of without post data.
Attachment #262252 - Attachment is obsolete: true
Attachment #262255 - Attachment is obsolete: true
Attachment #262399 - Attachment is patch: true
Attachment #262399 - Attachment mime type: text/csv → text/plain
Attached patch cvs diff -u tabbrowser.xml (obsolete) — Splinter Review
Only reattach the old history before gotoIndex(shIndex - 1) and not before goback()
Attachment #262399 - Attachment is obsolete: true
Not bad, although that assumes that the previous index has no postdata ;-)

Note that <form target="_blank" action="POST"> will create a tab with one index with postdata, so (how) do we deal with that?
Attached patch cvs diff -u tabbrowser.xml (obsolete) — Splinter Review
Something like this?
Attachment #262403 - Attachment is obsolete: true
Attached patch cvs diff -u tabbrowser.xml (obsolete) — Splinter Review
The previous patch was really untested, typed out of the blue, but this one seems to work.  What do you think of this one Neil?
Attachment #262433 - Attachment is obsolete: true
Attached patch cvs diff -u tabbrowser.xml (obsolete) — Splinter Review
Err. That should have been this one (:
Attachment #262437 - Attachment is obsolete: true
This patch addresses Neils point about the previous index having postdata, or not.
Attachment #262439 - Attachment is obsolete: true
I think we should back out the feature.  It exposes a gecko crash nobody cares about / knows how to fix.
(In reply to comment #17)
> I think we should back out the feature.  It exposes a gecko crash nobody cares
> about / knows how to fix.

Oh?  I personally haven't seen any crashes so far, so how/when exactly does it crash?  Is there a a bug report files for this crash?  
(In reply to comment #18)
> (In reply to comment #17)
> > I think we should back out the feature.  It exposes a gecko crash nobody cares
> > about / knows how to fix.
> 
> Oh?  I personally haven't seen any crashes so far, so how/when exactly does it
> crash?  Is there a a bug report files for this crash?  
> 

http://talkback-public.mozilla.org/reports/mozilla/ click "MozillaTrunk" - it's our #10 crasher.
sam/ctho, any crash data still around for this?  should this be nomniated for 
Keywords: topcrash
Whiteboard: sg:?
any upcoming seamokey release?
Whiteboard: sg:? → [sg:?]
KaiRo would know better than I. I'm not clear what the crash was since SeaMonkey no longer uses Talkback and the data wasn't pasted in this bug.
Component: XP Apps: GUI Features → UI Design
The crash was fixed some time ago. Do we need to move along with this, if so, who can drive that?
Neil, do we need to do something about this?
Flags: blocking-seamonkey2?
Firefox now has undo close tab too so it would be interesting to compare whether it suffers from a similar problem and how they deal with it.
I can't find a a concise description of what this bug report actually covers, but it appears to be related to the fact that SeaMonkey keeps actual documents around for undo. Firefox's undo close tab uses session restore, which serializes all the needed data to JSON, so I don't think this problem is relevant there.
OK, so here's the concise description:

How do you restore a tab that contains an uncached postdata response?
I think we serialize the postData that was sent, and resend it when reopening:

http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/nsSessionStore.js#1182

Simon knows the details better than I, though.
So, this is not usually a problem with Firefox because a) it defaults to not saving posted pages (which I guess we could fake by clearing the post data on the existing history entry) b) it defaults to not saving https data at all (which reduces the chance of it running in to an uncached page).
(In reply to comment #30)
> So, this is not usually a problem with Firefox

This issue doesn't exist in Firefox at all because we first create an empty tab (through tabbrowser.xml's API) and only afterwards try to restore as much of its state as possible (in nsSessionStore.js).

(In reply to comment #29)
> Looks like it just eats the exception.

... at which point the worst that could happen is that the Back/Forward buttons aren't properly enabled.
I'm going to work around this by copying the sessionstore behaviour, which is to GET the page if it's no longer cached.
Attached patch Proposed patchSplinter Review
Attachment #399274 - Flags: review?(iann_bugzilla)
Attachment #399274 - Flags: review?(iann_bugzilla) → review+
Flags: blocking-seamonkey2.0? → blocking-seamonkey2.0+
Pushed changeset 92ca5d53ce94 to comm-central.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Attachment #399274 - Flags: approval-seamonkey2.0+
Neil, can that fix be applied to 1.x. source for an eventual 1.1.19 release?
Sorry for commenting twice for this, the other question is if/when we should open up this report to the public.
I don't think there's an exploitable vulnerability here. It breaks our security indicators but not in a way that's useful to an attacker
Group: core-security
Whiteboard: [sg:?] → [sg:dos?]
You need to log in before you can comment on or make changes to this bug.