Closed
Bug 378160
Opened 17 years ago
Closed 15 years ago
tabbrowser.xml needs fix
Categories
(SeaMonkey :: UI Design, defect)
SeaMonkey
UI Design
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)
25.15 KB,
image/png
|
Details | |
3.57 KB,
patch
|
Details | Diff | Splinter Review | |
1018 bytes,
patch
|
iannbugzilla
:
review+
iannbugzilla
:
approval-seamonkey2.0+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•17 years ago
|
Version: unspecified → Trunk
Reporter | ||
Comment 1•17 years ago
|
||
Comment 2•17 years ago
|
||
Can you describe the bug, and attach a patch?
Reporter | ||
Comment 3•17 years ago
|
||
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!
Reporter | ||
Comment 4•17 years ago
|
||
Proposed patch v1.0 This patch changes the order and to checks nsISHEntry.postData
Reporter | ||
Comment 5•17 years ago
|
||
Simplified patch
Comment 6•17 years ago
|
||
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
Reporter | ||
Comment 7•17 years ago
|
||
(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.
Assignee | ||
Comment 8•17 years ago
|
||
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
Reporter | ||
Comment 9•17 years ago
|
||
(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?
Reporter | ||
Comment 10•17 years ago
|
||
This patch (v1.3) restores tabs with of without post data.
Attachment #262252 -
Attachment is obsolete: true
Attachment #262255 -
Attachment is obsolete: true
Reporter | ||
Updated•17 years ago
|
Attachment #262399 -
Attachment is patch: true
Attachment #262399 -
Attachment mime type: text/csv → text/plain
Reporter | ||
Comment 11•17 years ago
|
||
Only reattach the old history before gotoIndex(shIndex - 1) and not before goback()
Attachment #262399 -
Attachment is obsolete: true
Assignee | ||
Comment 12•17 years ago
|
||
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?
Reporter | ||
Comment 13•17 years ago
|
||
Something like this?
Attachment #262403 -
Attachment is obsolete: true
Reporter | ||
Comment 14•17 years ago
|
||
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
Reporter | ||
Comment 15•17 years ago
|
||
Err. That should have been this one (:
Attachment #262437 -
Attachment is obsolete: true
Reporter | ||
Comment 16•17 years ago
|
||
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.
Reporter | ||
Comment 18•17 years ago
|
||
(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.
Comment 20•16 years ago
|
||
sam/ctho, any crash data still around for this? should this be nomniated for
Keywords: topcrash
Whiteboard: sg:?
Comment 21•16 years ago
|
||
any upcoming seamokey release?
Updated•16 years ago
|
Whiteboard: sg:? → [sg:?]
Comment 22•16 years ago
|
||
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.
Comment 23•15 years ago
|
||
The crash was fixed some time ago. Do we need to move along with this, if so, who can drive that?
Assignee | ||
Comment 25•15 years ago
|
||
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.
Comment 26•15 years ago
|
||
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.
Assignee | ||
Comment 27•15 years ago
|
||
OK, so here's the concise description: How do you restore a tab that contains an uncached postdata response?
Comment 28•15 years ago
|
||
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.
Assignee | ||
Comment 29•15 years ago
|
||
http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/nsSessionStore.js#1985 Looks like it just eats the exception.
Assignee | ||
Comment 30•15 years ago
|
||
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).
Comment 31•15 years ago
|
||
(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.
Assignee | ||
Comment 32•15 years ago
|
||
I'm going to work around this by copying the sessionstore behaviour, which is to GET the page if it's no longer cached.
Assignee | ||
Comment 33•15 years ago
|
||
Attachment #399274 -
Flags: review?(iann_bugzilla)
Attachment #399274 -
Flags: review?(iann_bugzilla) → review+
Updated•15 years ago
|
Flags: blocking-seamonkey2.0? → blocking-seamonkey2.0+
Assignee | ||
Comment 34•15 years ago
|
||
Pushed changeset 92ca5d53ce94 to comm-central.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Attachment #399274 -
Flags: approval-seamonkey2.0+
Keywords: fixed-seamonkey2.0
Comment 35•15 years ago
|
||
Neil, can that fix be applied to 1.x. source for an eventual 1.1.19 release?
Comment 36•15 years ago
|
||
Sorry for commenting twice for this, the other question is if/when we should open up this report to the public.
Comment 37•15 years ago
|
||
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.
Description
•