Closed
Bug 84749
Opened 24 years ago
Closed 24 years ago
BODY onUnload JavaScript gets wrong value for location.href
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
VERIFIED
FIXED
mozilla0.9.3
People
(Reporter: sdh4, Assigned: rpotts)
References
()
Details
(Keywords: privacy, testcase, Whiteboard: PDT+)
Attachments
(2 files)
|
137 bytes,
text/html
|
Details | |
|
11.09 KB,
patch
|
Details | Diff | Splinter Review |
From Bugzilla Helper:
User-Agent: Mozilla/5.0 (X11; U; Linux 2.2.16-3 i686; en-US; rv:0.9.1)
Gecko/20010607
BuildID: 2001060713
When the onUnload JavaScript is executed, location.href is the address of the
new web page, not the old web page. This allows the old web page to spy on where
you go to after you leave it (even if you leave by typing in a URL).
This bug should be severity: security, except that classification does not exist
(bug #84746)
Reproducible: Always
Steps to Reproduce:
1.go to www.weimer.com (one of many sites run by netidentity.com)
2. Wait for an auto-redirect
3. Save the HTML for this webpage in a temporary place
4. go to your home page
5. A tiny hidden window has popped up (it may be difficult/impossible
to find. I can find it easily (~75x75) on Linux with IceWM window manager and
the GNOME pager. It usually hides on the left edge or lower right corner of the
screen.
6. Right click on the hidden window and view page info. You will see your
homepage location encoded into its URL.
7. Look at the source you saved in step 3. In particular the BODY onUnload and
the function MakeRemoteWindow().
8. Quit your browser to get rid of the obnoxious JavaScript web bug.
Actual Results: I got my homepage location URL encoded into the web bug window
that popped up.
Expected Results: Encoded the referring page (weimer.com)
<BODY leftMargin="20" topMargin="0" marginwidth="20" marginheight="0"
onUnload="MakeRemoteWindow();">
function MakeRemoteWindow() {
// allow disabling of console
if (remote == 1){
return;
}
var baseurl = 'http://nitrous.exitfuel.com/';
var console = baseurl +
'?BPROGRAM=' + program +
'&' +
'LINKIN=' + linkin +
'&' +
'REF=' + location.href;
H = window.open(console,"ef","height=1,width=1,left=10000,top=10000,location=0,
menubar=0,statusbar=0,locationbar=0,scrollbars=0,resizable=0");
}
|
||
Comment 1•24 years ago
|
||
Yikes! over to DOM 0, ccing mstoltz and hyatt (since I recall hyatt doing
things with keeping the old doc in place till the new one comes in).
Steve, thanks for the excellent bug report. Our apologies for taking so long to
get to it.
Assignee: joki → jst
Status: UNCONFIRMED → NEW
Component: DOM Events → DOM Level 0
Ever confirmed: true
Keywords: privacy
QA Contact: vladimire → desale
|
||
Comment 2•24 years ago
|
||
|
|
||
Updated•24 years ago
|
|
||
Comment 4•24 years ago
|
||
This should be fixed for mozilla0.9.3.
|
|
||
Comment 5•24 years ago
|
||
Ok, so I talked to Rick about this and he did some investigation and thinks we
can simply move the fireing of the onunload handler earlier in the code so that
it's fired while window.location still has the value of the old page. Reassigning.
Assignee: jst → rpotts
Status: ASSIGNED → NEW
|
|
Assignee | |
Comment 6•24 years ago
|
||
|
|
Assignee | |
Comment 7•24 years ago
|
||
I forgot to mention that the patch is for the trunk :-)
|
|
||
Comment 8•24 years ago
|
||
nsDocShell::FireUnloadNotification() never uses 'rv', maybe remove it?
Do we really care about 'rv' at line ~3815?
In stead of making mFiredUnloadEvent a PRBool cound't you make it a PRPackedBool
and squeese it in with other smaller than 32-bit members?
Looks like we could loose 'rv' in DocumentViewerImpl::Unload() too.
Nit of the day, in nsDocShell::FireUnloadNotification():
+ if (mContentViewer && !mFiredUnloadEvent) {
+ mFiredUnloadEvent = PR_TRUE;
+
+ rv = mContentViewer->Unload();
Fix the indentation just below the if...
Other than that, sr=jst
|
|
Assignee | |
Comment 9•24 years ago
|
||
I guess that in nsDocShell::FireUnloadNotifications(...) i'm not really sure
what the 'failure' policy should be :-)
One option is to fail if any of the nsIContentViewer::Unload() calls fail...
Another option is to always ignore the result code and continue notifying...
I have no idea which is the best option :-) So, for now, I was capturing the
return code but ignoring it - best of both worlds :-)
I've made changes to ignore the return code from FireUnloadNotifications...
I was afraid to lump in the PRBool -> PRPackedBool changes into this patch
because it needs to land on the branch too :-)
I'll open up another bug to address the PRPackedBool issue specifically.
-- rick
|
|
||
Comment 10•24 years ago
|
||
This is Nisheeth typing at jst's computer. r=nisheeth.
|
|
Assignee | |
Comment 11•24 years ago
|
||
I've checked the changes into the trunk... I'm just waiting for approval to
check them into the branch...
|
|
||
Comment 12•24 years ago
|
||
Adding nsBranch keyword to get more eyeballs on this security bug.
Keywords: nsBranch
|
|
||
Comment 13•24 years ago
|
||
The best (only, probably) way to do it is to e-mamil pdt2@netscape.com.
|
|
||
Comment 14•24 years ago
|
||
There's a whole lot going on in that patch. Is there a simple version that
could be made for the branch? Something like "deleted call from old place and
inserted it in new place"? I'm hesitant to plus the existing patch.
|
|
Assignee | |
Comment 15•24 years ago
|
||
hey steve,
believe it or not, that's basically what this patch is :-)
In the current implementation, nsWebShell.cpp has the code to iterate across
DocShells and fire the OnUnload(...) event- FireUnloadForChildrten(...). This
code as been moved into nsDocShell.cpp as the FireUnloadNotification(...) method.
I needed to move this code into the DocShell, because this method needs to be
called from the middle of a method within nsDocShell.cpp
The other big part of the patch is moving the code that used to be in
nsWebShell::FireUnloadEvent(...) into nsDocumentViewer::Unload(...). I decided
to do this (Rather than move it into the nsDocShell) because the OnLoad() event
is fired from the nsDocumentViewer too... and it is easier to get to the
information required to fire the event at the nsDocumentViewer level (than at
the nsDocShell/nsWebShell level).
Finally, the call to FireUnloadNotification() got moved from
nsWebShell::SetupNewViewer(...) into he middle of
nsDocShell::CreateContentViewer(...). This is the critical part of the patch
(ie. the part that actually fixes the bug). The rest of the patch is just the
housekeeping required to make the fix...
|
|
||
Comment 16•24 years ago
|
||
Oops, I forgot to add myself to the CC list and am only just seeing your
response :-(
Adding PDT+, please check in on the branch. What can we do to ensure complete
confidence in this fix? Do we have a targetted set of tests? Can we invent a
set of tests? Have people been stressing this area with trunk builds since the
patch went in?
Whiteboard: PDT+
|
|
Assignee | |
Comment 17•24 years ago
|
||
The upshot of the fix is the thew OnUnload(...) script handler will be fired
earlier than before - assuming that all of the code is correct :-)
jst and i lxr'ed through the codebase to see what code relies on the
OnUnload(...) event... it looked like some of the wizards were the only consumers...
So, i think that if the Profile wizard and the mail account wizard do not show
regressions, then we should be in good shape...
I haven't heard of any regressions in the wizards since i landed this patch on
the trunk...
|
|
Assignee | |
Comment 18•24 years ago
|
||
I've just checked the patch into the branch.
Status: NEW → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
|
|
||
Comment 20•24 years ago
|
||
Is there a connection to bug 88684 ?
|
|
||
Comment 21•23 years ago
|
||
See also bug 145579, another privacy bug with the same impact.
You need to log in
before you can comment on or make changes to this bug.
Description
•