141442 - window.openDialog not redefineable by script

Status: VERIFIED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Martin Zidek]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.0.0+)
Gecko/20020430
BuildID:    2002043010

Links with javascript: URL scheme don't work . Eg. <a
HREF="javascript:openDialog('img','viewimage.php?img=4051')">. Function will not
run after click. I have everything enabled in mozzila (popups). Maybe error is
in function openDialog. In NN4.7/IE it works.

Reproducible: Always
Steps to Reproduce:
1. go to
http://news.auto.cz/cgi-bin/main.phtml?rubrika=6&id=277&stamp=1020253666&kam=detail
2. click on small thumbnail images at the bottom of the page


Actual Results:  Nothnig ...

Expected Results:  Window with large image should popup.

They have small function

----SNIP----
<script language="JavaScript">
function openDialog(hwnd,url)
{
  var
v=window.open(url,hwnd,'width=450,height=400,menubar=0,status=0,scrollbars=1,resizable=1');
  v.focus();
}
</script>
---SNIP---

and call thi function from HREF link

---SNIP---
<a HREF="javascript:openDialog('img','viewimage.php?img=4051')"></a>
---SNIP----

Comment 1

[Rainer Bielefeld]

Confirm with Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.0rc1) Gecko/20020425
No reaction if I click on any of the 16 tumbnail-car-pictures

Javascript console tells for tumbnail left/top:
Error: uncaught exception: [Exception... "Component returned failure code:
0x80004005 (NS_ERROR_FAILURE) [nsIDOMJSWindow.openDialog]"  nsresult:
"0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: javascript:
javascript:openDialog('img','viewimage.php?img=4031') :: <TOP_LEVEL> :: line 1"
 data: no]

Javascript-links work fine with NN4.73,  OPERA6,  IE6

Comment 2

[Phil Schwartau]

Browser, not engine ---> DOM Level 0

Comment 3

[Phil Schwartau]

I think Martin and Rainer have found the problem: openDialog()
is a pre-existing function in the DOM. Therefore it's not good
for the developers to define a function called "openDialog".


To see that openDialog() already exists, just do this. 
Load about:blank in Mozilla. Then try these javascript: URLs

javascript: alert('openDialog' in window) --->   true
javascript: alert(typeof openDialog)      --->  'function'
javascript: alert(openDialog)             --->   function openDialog(){
                                                   [native code]
                                                 }


Now load the site for this bug. Now you get

javascript: alert(openDialog)             --->   undefined



But I will let jst make the final decision here -

Comment 4

[Phil Schwartau]

Note Rainer's finding: the site works fine with NN4.73, OPERA6, IE6.
Which I can confirm for NN4.73 and IE6...

Comment 5

[Johnny Stenback (:jst)]

This was caused by bug 56009 where window.openDialog was made noAccess, that's
not a good idea since that makes it impossible to redefine openDialog in web
pages. The patch I'm about to attach makes window.openDialog redefineable from
script and it makes sure that calls that don't come from chrome can't call
through the native openDialog() method. Cc'ing a bunch of security people...

Comment 6

[Johnny Stenback (:jst)]

Attachment: Let scripts redefine window.openDialog()

Comment 7

[Fabian Guisset]

jst, in my opinion the right fix for this bug is to move OpenDialog() to
nsIDOMChromeWindow

Comment 8

[Johnny Stenback (:jst)]

Yeah, that'd be one way to fix this, but I'm not convinced that there's no code
in our XUL/XBL that relies on openDialog() being available on non-chrome
windows. If we're sure that's the case, then I'm fine with moving the method to
nsIDOMChromeWindow. Either way, the current code is broken, either of the two
proposals would solve the problem reported in this bug, one is more riskier than
the other (i.e. the patch is IMO as safe as it could be, moving openDialog to
nsIDOMChromeWindow could cause problems).

Comment 9

[Martin Zidek]

That's great how fast you response ! I'm really impressed ! Keep doing good job !

Comment 10

[Mitchell Stoltz (not reading bugmail)]

Comment on attachment 81856 [details] [diff] [review]
Let scripts redefine window.openDialog()

Yes, this looks safe to me. It has the added benefit of making the protection
for the native openDialog method hard-coded, so that prefs problems won't
reopen the original bug ( bug 56009). r=mstoltz.

Comment 11

[Mitchell Stoltz (not reading bugmail)]

*** Bug 127480 has been marked as a duplicate of this bug. ***

Comment 12

[Boris Zbarsky [:bzbarsky]]

*** Bug 130604 has been marked as a duplicate of this bug. ***

Comment 13

[Boris Zbarsky [:bzbarsky]]

*** Bug 132668 has been marked as a duplicate of this bug. ***

Comment 14

[Boris Zbarsky [:bzbarsky]]

*** Bug 134764 has been marked as a duplicate of this bug. ***

Comment 15

[Johnny Stenback (:jst)]

Comment on attachment 81856 [details] [diff] [review]
Let scripts redefine window.openDialog()

Hmm, jband pointed out that this would not properly detect when a call comes
from chrome and goes through content code that calls window.openDialog(), IOW
this would've made window.openDialog() callable from content code. New patch
coming up.

Comment 16

[Johnny Stenback (:jst)]

Attachment: Make IsCallerChrome() secure.

This uses the security manager to properly detect if a call is coming from
chrome or not. r=/sr=?

Comment 17

[Johnny Stenback (:jst)]

Attachment: Make IsCallerChrome() secure and let scripts override openDialog()

Comment 18

[John Bandhauer]

Comment on attachment 83982 [details] [diff] [review]
Make IsCallerChrome() secure and let scripts override openDialog()

sr=jband. With the removal of the one unnecessary rv check we discussed. I
trust you made sure the secman is not leaked. If Mitch says that this does the
right thing then I'm happy.

Comment 19

[Mitchell Stoltz (not reading bugmail)]

Comment on attachment 83982 [details] [diff] [review]
Make IsCallerChrome() secure and let scripts override openDialog()

This appears to do the right thing. r=mstoltz.

Comment 20

[Johnny Stenback (:jst)]

Fix checked in on the trunk.

Comment 21

[Dawn Endico]

Comment on attachment 83982 [details] [diff] [review]
Make IsCallerChrome() secure and let scripts override openDialog()

a=chofmann,shaver,brendan

Comment 22

[scottputterman]

adding adt1.0.0+.  Please check into the 1.0 branch.

Comment 23

[Johnny Stenback (:jst)]

Fixed on branch.

Comment 24

[Prashant Desale]

verified1.0.0 on 05-22-18-1.0.0 & also Verified on trunk on 05-23-08-trunk.