Closed Bug 31573 Opened 22 years ago Closed 20 years ago

Javascript alerts, confirms should be marked as such

Categories

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

defect

Tracking

()

VERIFIED FIXED
mozilla1.0

People

(Reporter: jruderman, Assigned: adamlock)

References

Details

(4 keywords, Whiteboard: [nsbeta3-])

Attachments

(2 files, 3 obsolete files)

Javascript alert and confirm windows are currently titled with only "Alert" 
and "Confirm".  They should say, at the least, "Javascript alert" 
and "Javascript confirmation" to make it clear (to at least experienced 
surfers) that the alerts are coming from the page and not from the browser 
itself.

Confirm('Mozilla has discovered cache corruption.  Correcting this corruption 
will require restarting your computer.  Press OK to restart your computer and 
risk losing unsaved data, or Cancel to continue browsing and risk hard disk 
drive corruption');

Combine that with a 15-second timeout and some extra code to make the disk 
thrash a bit before showing the message, and you can probably scare a few 
people.  Including "javascript" in the window title should help a little.
The alert boxes come from the browser, not the engine. It's the code in 
nsWebShellWindow that provides the default title, I think. i don't know if the 
DOM level had a chance to sepcify something different.
Assignee: rogerl → trudelle
Component: Javascript Engine → XP Toolkit/Widgets
QA Contact: rginda → jrgm
changing component to javascript, reassigning.
Assignee: trudelle → rogerl
Component: XP Toolkit/Widgets → Javascript Engine
QA Contact: jrgm → rginda
Umm, why is this back to the engine? There's nothing I can do to fix this bug, 
the engine has no control over what the embedding calls are doing.

Is XP Toolkit/Widgets not the right component? I'm assigning it to DOM0 then.
Assignee: rogerl → jst
Component: Javascript Engine → DOM Level 0
QA Contact: rginda → desale
FYI, here are the titles for js alert() and confirm() under win98/mac/linux

CONFIRM                   
mozilla     "Confirm"                    
Nav4.73     "Javascript Application"     
IE5(win98)  "Microsoft Internet Explorer"

ALERT                     
mozilla     "Alert"                    
Nav4.73     "Javascript Application"     
IE5(win98)  "Microsoft Internet Explorer"

PROMPT
mozilla     "Prompt"
Nav4.73     "Javascript Application"
IE5(win98)  "Explorer User Prompt"
Nothing I can do about this in the DOM code, it's all in nsWebShellWindow AFAIK.
Over to Don Melton (XPApps module owner).
Assignee: jst → don
Confirming.

Gerv
Status: UNCONFIRMED → NEW
Ever confirmed: true
Reassigning for Don
Assignee: don → ben
Target Milestone: --- → M20
verah@netscape.com - did you mean to assign this bug to Ben Goodger?

Changing severity - this isn't an enhancement IMO. It's 4.xP.

Gerv
Severity: enhancement → normal
Move to M21 target milestone.
Target Milestone: M20 → M21
Nominating for nsbeta3. Javascript windows should be clearly marked as such.

Ger
Keywords: nsbeta3
are these titles localized even?
You can see this dialog eliminating a bookmark in the Bookmark manager (right
click + Delete).

As Joseph points out it's not localized, and currently there's not a way to
localize this title. (so I added rchen also to the CC list..)
bug 38523 deals with localizing the title.
Assignee: ben → rogerl
Whiteboard: [nsbeta3-]
nav triage team:
though we agree that ideally it should say "javascript alert", the fix sounds a 
bit complex for something so small, so nsbeta3-.  

If you want to notify the user that it is a javascript error, it might be easier 
to do that in the content of the alerts message instead of in the title.  
reassigning to rogerl since he may be able to effect the content area if he 
wants to.
Nav 4.x and earlier clearly identify alert/confirm/prompt dialogs originating 
from untrusted JS, using a title suffix string " - [JavaScript Application]" -- 
and so should Mozilla and Nav6.

The " - [JavaScript Application]" or similar identification must be in the 
window title or chrome; it cannot be in the dialog's content, or it can be 
trivially spoofed (erased).  This is not rogerl's bug, it belongs to whoever 
owns and implements the nsIPrompt interface.  Both interface and impls may need 
to be extended to allow a title suffix string, or just a flag, or some bit of 
information signifying "untrusted dialog", to be passed through.

Although warren last touched the nsIPrompt impls in nsWebShellWindow.cpp, it 
seems to me this bug should go to the embedding folks.  I'm giving it to valeski 
for reassignment, and cc'ing the usual suspects.

/be
Assignee: rogerl → valeski
I added a dialog title parameter which should be used for this.
-> adam
Assignee: valeski → adamlock
Target Milestone: M21 → mozilla1.0
See also bug 64676.  Currently, the page can specify any title for the alert 
that it wants.
I believe the solution here is that WindowInternalPrompt in nsJSWindow.cpp 
should prefix the title parameter with "Javascript: " or replace it 
with "Javascript Application" before passing the call onwards to the global 
window impl.
Keywords: dom0
Nominating for mozilla1.0, since this has security implications. (A naive user
can be mislead to compromise security.)

Should the appearance of an untrusted dialogs be more conspicuously different?
Currently JavaScript alerts have a '!' in triagle icon signifying elevated
seriousness of the message. IE 5.1 Pre for Mac OS X, for example, uses a less
dramatic icon ('i' in a circle).

CCing UI design owner to get an opinion on the UI design aspect of this bug.
Keywords: mozilla1.0
OS: Windows 98 → All
Hardware: PC → All
So we have two suggestions: (1) to alter the title, and (2) to alter the icon.

First, the title. Alerts shouldn't have titles at all. The exception is on 
Windows, where alerts should be titled with the name of the application (i.e. 
`Mozilla'), but that should be handled at the XP Toolkit level rather than by 
the calling function.

There are two main reasons for this. Firstly, giving an alert a title is not 
useful for window-switching purposes (since alerts are modal to their parent 
window), and an alert title would practically never give the user enough 
information to allow them to avoid reading the contents of the alert at all. So 
the only thing giving an alert a title would do is slow the user down 
unnecessarily. Secondly, some platforms don't show title bars for alerts at 
all; the twm window manager didn't when I used Solaris, for example, and on Mac 
OS X the preferred method of displaying window-specific alerts is as title-less 
sheets.

For those same two reasons, relying on the alert title to indicate the 
provenance of the alert is probably not a good idea -- it might not be visible 
at all, and people won't read it even if it is.

Second, the icon. There are four types of alert, and these are distinguished by 
their icons as follows.
(1) Error alert: (X) on Windows, hand in a stop sign on Mac OS.
(2) Note alert: `i' in a bubble on Windows, talking face on Mac OS.
(3) Confirmation alert: /!\ on both Windows and Mac OS.
(4) Login/connection alert: bunch of keys on Windows, planet Earth on Mac OS.

If you mix up these alert types and icons, you tend to confuse people. 
Unfortunately, Mozilla is currently hopeless in this regard: we have a bunch of 
the deprecated Windows-95-style `?' icons hanging around (bug 59820), we use 
the confirmation-alert icon for window.alert() when (as Henri noted) we should 
be using the note-alert icon instead (bug 75713); there are a bunch of places 
where we're using an alert icon for a type of window other than an alert (e.g. 
bug 81213); and we have at least one place where we're using a custom icon 
where we shouldn't (the drag-to-the-Home-button alert, for which I am to 
blame). Fixing the window.alert() icon bug would help, but we shouldn't do 
anything special to make icons for script alerts inconsistent with the icons 
for other alerts of the same type.

We're left, then, with changing the content of the alert itself. This could be 
done in a similar fashion to JVMs, which put `Warning: Applet window' or 
similar at the bottom of a window opened by an applet. Brendan says that any 
such modifications can be erased, but I don't see how that could be possible if 
all the Web page can do is specify unformatted text for the alert.

An additional nice touch would be to color the alert using the background and 
foreground colors of the page, rather than with the normal chrome colors (after 
doing a sanity check that the background and foreground colors of the page 
weren't unreadably similar). This would make it fairly obvious that the alert 
was emanating from the page, rather than from Mozilla itself.
*** Bug 93930 has been marked as a duplicate of this bug. ***
Small change to fix a significant security problem. nsbeta1.
Keywords: nsbeta1
Adding dependency on bug 64676.
Depends on: 64676
I don't believe it is a small change because we only want dialogs to have
"[Javascript Application]" prefixed to the title when posed from content and not
chrome. Assuming the prefixing occurred in the global confirm, alert & prompt
methods, the chrome would have to be changed to use the prompt service.



The proof of concept patch fixes the JS functions to prefix "[Javascript
Application]" to the title bar. This string should probably comes from a string
bundle and I'd prefer it to say "[Script Application]" rather than assuming
every script is JS.

Chrome also has several places where it's calling these methods rather than
using the prompt service where its supposed to. An example is the confirmation
dialog that appears when you delete an address book. A new bug should probably
be opened to fix those kind of issues in the chrome.

You could avoid breaking alerts and prompts posed by chrome by asking the
security manager whether untrusted javascript is at the top of the JS stack...
Jesse or Mitch, can you point me to some sample code that does this checking?

Or should we require chrome to use the prompt service? It seems most
of the chrome already does with a few places that call the public JS methods.

Obviously the latter entails more work.
Adam,
 I'll help with the checking that Jesse suggested. I need to think through how
to do it.
If this doesn't get fixed first, it definitely needs a relnote.
Keywords: relnote
See attachment 50131 [details]. I was going to attach it to this bug...
Keywords: testcase
Blocks: 104166
Keywords: nsbeta1
These three bugs are related:
bug 31573 Javascript alerts, confirms should be marked as such
bug 47777 Browser dialogs should not be mimicable
bug 64676 JavaScript Prompt could be confused for system dialog
I'm happy to submit an updated version of my original patch and forget about
doing any clever checks to see what kind of JS is running. 

My opinion is that chrome is wrong if it calls these methods and should be fixed
to use the prompt service instead. New bugs should be opened to cover those
cases but generally the chrome is already doing the right thing.
dan, you have background on who's throwing what. any opinion on adam's last comment?
Keywords: nsbeta1nsbeta1+
  I'm inclined to not impose the restriction of chrome authors -- window.alert
is easy and convenient, and it's being used. I see what looks like 28 .js and
.xul files in Mozilla chrome that use window.alert, sometimes several times.
It's a matter of documentation and expectation -- we haven't been warning off
doing it the easy way in chrome. We'd have to start.
  I'd prefer to look up the call stack. It's not new code; we're already doing
this in several places. nsScriptSecurityManager::IsCapabilityEnabled is doing
something very similar. And it doesn't seem like a performance hit, since it'll
happen fairly rarely and the task of opening an alert is already pretty large.
Dan can you point me to an existing example? Thanks
I and other xpfe super reviewers have been requiring use of the prompt service
for a little while now, primarily because it's embarrassing to have chrome
errors with titles of "Alert".  If window.alert() is changed to have the app
name as the title when called from chrome, then that's fine.  Actually, it'd
have the added benefit of not allowing our chrome authors to specify titles,
which is good, since our titles are currently horribly inconsistent.
Attached patch Updated patch (obsolete) — Splinter Review
This patch checks the call stack to see if it's chrome or content calling
prompt/alert/confirm and appends "[Javascript Application]" accordingly.

Reviews please?
Attachment #45267 - Attachment is obsolete: true
Attachment #45825 - Attachment is obsolete: true
Well yeah, I guess that's what you gotta do. Looks generally good. A couple of
comments:

Just for posterity, I feel like mentioning I have vague uneasiness about
referencing commonDialogs.properties inside nsGlobalWindow. But I asked around
and no one seemed to mind, so that's not a real objection.

A couple of points on efficiency: there's a fair bit of extra work done in
nsGlobalWindow's constructor. It's not as if a typical web page contains so many
DOM windows that this sounds like a big issue, but I do wonder if it'll affect
page load times. And there is that new fetch of of the Script Security Manager
Service for each alert or prompt posted. I suggest considering caching the SSM
service (so alert/prompt doesn't have to fetch it from scratch) and delaying the
construction of mScriptDlgTitle until the first time it's required (often never,
I'd think), (outside the constructor).

One thing, though. Is the DOM window the right place for it? Won't alerts and
prompts posed directly through the Prompt Service miss all this good stuff? I
gather from previous comments that window.alert and promptservice.alert are used
interchangeably throughout our chrome. Since nsGlobalWindow goes through the
prompt service, I'm curious why you didn't put this patch there. (That would of
course add a requirement for all implementors of the prompt service that we'd
have to document...)
The confirm/alert/prompt methods on nsGlobalWindow are the ones called by
ordinary JS. Chrome shouldn't be using these methods, because they don't have a
title parameter. But if chrome does call them, my patch ensures that
"[Javascript Application]" is not appended to the front, only for ordinary JS.
Chrome callers will get the dumb default "Alert" title as before. I shall add
some NS_WARNINGs to these methods to catch chrome callers and encourage using
the prompt service which does allow the title to be specified.

So it is better that this code goes here. The prompt service is general purpose
and not called directly by ordinary JS. It can also be overridden by embedders
which means they wouldn't get this behaviour.

I will move the message string code out of the constructor into a method that is
called when the string is required.

I could cache the security manager but I didn't because nowhere else in this
class seems to. I will raise a new bug on this because there are about 50 or so
do_GetService calls in this class with hardcoded contract ids and worse.
Attached patch New patchSplinter Review
New patch changes the following:

1. New method MakeScriptDialogTitle makes the title instead. Replaces
   the bundle code I had put in the constructor.
2. MakeScriptDialogTitle uses nsIStringBundle::FormatStringFromName for
   localisation purposes.
3. NS_WARN_IF_FALSE assertions for chrome callers to these methods.
Attachment #73716 - Attachment is obsolete: true
Comment on attachment 73870 [details] [diff] [review]
New patch

oh right, duh -- this is only for non-chrome alerts and prompts. This patch
could still cache ScriptDlgTitle like in the previous patch, which kept
mScriptDlgTitle rather than fetching it from the service each time. But r=me.
Attachment #73870 - Flags: review+
Code can no long cache the title because it's now a localized string containing
a %S, so it has to be remade each time.
Comment on attachment 73870 [details] [diff] [review]
New patch

nice. sr=blake
Attachment #73870 - Flags: superreview+
Comment on attachment 73870 [details] [diff] [review]
New patch

a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #73870 - Flags: approval+
Waitaminute -- it's "JavaScript", not "Javascript".  See 4.x.

/be
Fix checked in with JavaScript correction
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Opened bug 132420 to deal with do_GetService mess in nsGlobalWindow.cpp.

Opened bug 132279 to deal with OS X not showing titles for any
alert/prompt/confirm dialog.
Verified in Windows
No longer depends on: 64676
*** Bug 64676 has been marked as a duplicate of this bug. ***
Attached file Testcase
Re-opening.
I tested this with Win-95 & 2002-05-23-08-trunk.
No alert,confirm,prompt dialogs are marked as such.
All dialogs say "Javascript Application".

I'm attaching testcase to test this.
re-opening. please read my comments above & use testcase I attached.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I don't understand why this has been reopened. The very point of this patch was
so these dialogs *would* say "[Javascript Application]". The test case seems to
verify that is what is happening.

There is some feeling that alert/prompt/confirm should look entirely different
from the ones provided through the prompt service (especially on OS X which
doesn't show the title), but that should be a new bug.
This bug is fixed. The alert/prompt/confirm title now includes [JavaScript
Application] on Windows as with Netscape 4.x. There was a concern that less
experienced users would think the prompts were coming from the browser. Changes
to behavior or icons should be entered as new bugs.
Status: REOPENED → RESOLVED
Closed: 20 years ago20 years ago
Resolution: --- → FIXED
Perfect. Got it. Sorry Adam I re-opened it for wrong reasons.
Marking verified.
Status: RESOLVED → VERIFIED
Whiteboard: [nsbeta3-] → [nsbeta3-] security
Keywords: relnote
Keywords: csec-spoof, sec-low
Whiteboard: [nsbeta3-] security → [nsbeta3-]
You need to log in before you can comment on or make changes to this bug.