Closed
Bug 31573
Opened 25 years ago
Closed 23 years ago
Javascript alerts, confirms should be marked as such
Categories
(Core :: DOM: Core & HTML, defect, P3)
Core
DOM: Core & HTML
Tracking
()
VERIFIED
FIXED
mozilla1.0
People
(Reporter: jruderman, Assigned: adamlock)
References
Details
(4 keywords, Whiteboard: [nsbeta3-])
Attachments
(2 files, 3 obsolete files)
(deleted),
patch
|
danm.moz
:
review+
bugzilla
:
superreview+
asa
:
approval+
|
Details | Diff | Splinter Review |
(deleted),
text/html
|
Details |
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.
Comment 1•25 years ago
|
||
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
Comment 2•25 years ago
|
||
changing component to javascript, reassigning.
Assignee: trudelle → rogerl
Component: XP Toolkit/Widgets → Javascript Engine
QA Contact: jrgm → rginda
Comment 3•25 years ago
|
||
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
Comment 4•25 years ago
|
||
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"
Comment 5•25 years ago
|
||
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
Comment 8•25 years ago
|
||
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
Comment 10•25 years ago
|
||
Nominating for nsbeta3. Javascript windows should be clearly marked as such.
Ger
Keywords: nsbeta3
Comment 11•24 years ago
|
||
are these titles localized even?
Comment 12•24 years ago
|
||
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..)
Comment 13•24 years ago
|
||
bug 38523 deals with localizing the title.
Comment 14•24 years ago
|
||
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.
Comment 15•24 years ago
|
||
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
Comment 16•24 years ago
|
||
I added a dialog title parameter which should be used for this.
Reporter | ||
Comment 18•24 years ago
|
||
See also bug 64676. Currently, the page can specify any title for the alert
that it wants.
Assignee | ||
Comment 19•24 years ago
|
||
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.
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.
Comment 21•24 years ago
|
||
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.
Comment 22•23 years ago
|
||
*** Bug 93930 has been marked as a duplicate of this bug. ***
Comment 23•23 years ago
|
||
Small change to fix a significant security problem. nsbeta1.
Keywords: nsbeta1
Assignee | ||
Comment 25•23 years ago
|
||
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.
Assignee | ||
Comment 26•23 years ago
|
||
Assignee | ||
Comment 27•23 years ago
|
||
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.
Assignee | ||
Comment 28•23 years ago
|
||
Reporter | ||
Comment 29•23 years ago
|
||
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...
Assignee | ||
Comment 30•23 years ago
|
||
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.
Comment 31•23 years ago
|
||
Adam,
I'll help with the checking that Jesse suggested. I need to think through how
to do it.
Comment 32•23 years ago
|
||
If this doesn't get fixed first, it definitely needs a relnote.
Keywords: relnote
Comment 33•23 years ago
|
||
See attachment 50131 [details]. I was going to attach it to this bug...
Reporter | ||
Comment 34•23 years ago
|
||
Assignee | ||
Comment 35•23 years ago
|
||
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.
Comment 36•23 years ago
|
||
dan, you have background on who's throwing what. any opinion on adam's last comment?
Comment 37•23 years ago
|
||
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.
Assignee | ||
Comment 38•23 years ago
|
||
Dan can you point me to an existing example? Thanks
Comment 39•23 years ago
|
||
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.
Assignee | ||
Comment 40•23 years ago
|
||
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
Comment 41•23 years ago
|
||
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...)
Assignee | ||
Comment 42•23 years ago
|
||
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.
Assignee | ||
Comment 43•23 years ago
|
||
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 44•23 years ago
|
||
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+
Assignee | ||
Comment 45•23 years ago
|
||
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 46•23 years ago
|
||
Comment on attachment 73870 [details] [diff] [review]
New patch
nice. sr=blake
Attachment #73870 -
Flags: superreview+
Comment 47•23 years ago
|
||
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+
Comment 48•23 years ago
|
||
Waitaminute -- it's "JavaScript", not "Javascript". See 4.x.
/be
Assignee | ||
Comment 49•23 years ago
|
||
Fix checked in with JavaScript correction
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 50•23 years ago
|
||
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.
Comment 51•23 years ago
|
||
Verified in Windows
Reporter | ||
Comment 52•23 years ago
|
||
*** Bug 64676 has been marked as a duplicate of this bug. ***
Comment 53•23 years ago
|
||
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.
Comment 54•23 years ago
|
||
re-opening. please read my comments above & use testcase I attached.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 55•23 years ago
|
||
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.
Comment 56•23 years ago
|
||
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: 23 years ago → 23 years ago
Resolution: --- → FIXED
Comment 57•23 years ago
|
||
Perfect. Got it. Sorry Adam I re-opened it for wrong reasons.
Marking verified.
Status: RESOLVED → VERIFIED
Reporter | ||
Updated•21 years ago
|
Whiteboard: [nsbeta3-] → [nsbeta3-] security
Reporter | ||
Updated•11 years ago
|
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.
Description
•