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)

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) (deleted) — 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 patch (deleted) — Splinter 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: 23 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 (deleted) —
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: 23 years ago23 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.

Attachment

General

Creator:
Created:
Updated:
Size: