Closed Bug 398718 Opened 17 years ago Closed 17 years ago

Better explanatory text for SSL error pages

Categories

(Core :: Security: PSM, defect)

Product:
Core
Component:
Security: PSM
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.9beta1

People

(Reporter: johnath, Assigned: KaiE)

References

Details

Attachments

(5 files, 9 obsolete files)

Current error pages
242.91 KB, image/jpeg
Details
Error pages with modified strings
200.75 KB, image/png
beltzner
: ui-review+
Details
Error page proposal
17.65 KB, image/png
Details
unbitrotten patch
42.45 KB, patch
Details | Diff | Splinter Review
error page in 2010
21.04 KB, image/gif
Details
As a result of bug 327181, it is now significantly more difficult for users to get to sites presenting unverified or invalid SSL credentials. Some portion of this is deliberate, since it replaces the old error dialogs which defaulted to trusting the credentials despite errors - training users to ignore the problem, and presenting malicious sites with the opportunity to deceive users. The new error pages don't offer the same click-through support, instead requiring the user to manually add an exception to their cert store via functionality added in bug 387480, since usability evidence suggests that task-focused users will push through any clickthrough option that suggests itself. These added restrictions do, however, argue for careful attention to the explanatory text provided in the error pages, so that users who do need to understand what's happening, and if necessary respond, have the means to do so. I'll attach screens of the current messages in a follow-up comment, as well as first-pass suggestions for changes.
Attached image Current error pages —
The attachment shows four error pages: Domain Mismatch, Unknown Issuer, Expired, and multiple errors. My chief concern is with the short description (below the title, the part that changes.) It seems over-complicated, and tends to run together, particularly with multiple errors presented. The title seems fine, but it also seems that the "possibly remedies" section needs help. Taking the domain mismatch as an example, the current text is: "An error occurred during a connection to www.kuix.de:443 because it uses an invalid security certificate. The certificate is not valid for domain name www.kuix.de. (ssl_error_bad_cert_domain)" I would propose something more like: == SNIP == Secure Connection Failed ------------------------ www.kuix.de uses an invalid security certificate. The certificate is not valid for this server. ------------------------------------------------- * You are trying to view a secure page, but the server is not identifying itself properly. This could be a problem with the server's configuration, or it could be someone trying to impersonate the server. * If you have connected to this server successfully in the past, the error may be temporary, and you can try again later. * If the problem continues, you can contact the owners of the web site and inform them of the error. * Your current list of servers with known security problems can be accessed from your Advanced preferences. * The error code for this problem is "ssl_error_bad_cert_domain". == ENDSNIP == I know that one of the changes here that might annoy people is the mention of advanced preferences, since that helps users find the exception dialog. I don't think it's at all tantamount to a click-through though, I think it just suggests that a work-around possibility exists, which is something I really don't think we can do without. In the case of multiple errors, I'd recommend line breaks, so that they appear as a list. I notice, looking at the source, that the existing short description *does* have line breaks that aren't rendering for me - so we may need to get netError.xhtml styled to support -moz-pre-wrap for that field.
I would also like to explore the possibility that we include a statement that tells site admins something specific that they can understand. For example: * This server uses a "self-signed" certificate to establish its identity. Firefox cannot verify the identity of self-signed certificates. -or- * This server uses a certificate that chains to an unknown root certification authority (CA). If this server is part of a corporate intranet, you may need to ask your web server administrator how to configure Firefox to trust the corporate CA. I know this is PKI jargon, but it may help for the client to clearly state what went wrong in a way that web site owners can understand, and hopefully take action.
I would like to propose to enhance error page to look similar as in bug 327181 attachment 212595 [details] - Bob Lord draft provides more information and better visibility on what's wrong/ok (more info could be hidden by default and button could have its last state remembered).
It might be good to differentiate between chaining to an unknown root and not chaining at all. See bug 399019.
Maybe we should add a 'View' button to the error-page, so that you can actually view what's going on. It's not possible anymore.
In reply to coment 5: Ability to view the bad cert chain seems like a must. In reply to comment 4: NSS provides separate error codes for cases of - issuer cannot be found - issuer cert is found, but is expired - issuer cert is found, but is not be a CA cert - issuer cert chain constructed all the way to a self-signed but untrusted CA cert. These cases should be distinguished in the UI.
(In reply to comment #6) > NSS provides separate error codes ... The NSS error code is displayed (as a string identifier) Please note, if there are multiple validation errors returned by NSS (from the error log), only one error code will be displayed.
In reply to comment 1, the page for "multiple errors" only gives the error code string for one of the errors, not for each of them.
In comment 6 I wrote: > Ability to view the bad cert chain seems like a must. Now I'm not so sure. I just tried the new add exception dialog in cert manager. Like it very much. Maybe it's enough.
Depends on: 399129
Users don't know about the exception dialog in the cert manager. The error page doesn't mention it at all, not to mention providing a button to open the cert manager.
(In reply to comment #10) > Users don't know about the exception dialog in the cert manager. > The error page doesn't mention it at all, not to mention providing a button to > open the cert manager. I think we should educate people about what's going on, instead of providing a direct link to bypass security. Could we have a help button that explains what is going on, what risks are related to bad certificates, and that rejecting the certificate by default is a good idea, because it helps to protect you from attackers/phishers. That help page could mention the add exception possibility which is available in cert manager.
Flags: in-litmus?
Flags: blocking1.9+
Attached patch WIP patch (obsolete) — — Splinter Review
This patch is incomplete but it makes many of the changes discussed. It includes work from Kai that adds the ability to differentiate between protocol errors and bad cert errors, since only bad cert errors should really be taking about overrides. There are some obviously bad parts, like in nsNSSIOLayer where I have if (codeName && false) to temporarily disable error code printing. It doesn't include a direct link to the override dialog, even though I think that should be discussed again, because I'd like to at least improve the text first here, while we debate the relative merits of the fast path over in bug 399275. I'd like to get nsNSSIOLayer to the point where host:port are shown on protocol errors, but just host on bad cert errors, and where the error code is only shown on protocol errors. The idea is that protocol errors all currently get a generic short-desc, and are not really user-servicable, so it's reasonable to put richer diagnostic information in there for admins. By contrast, bad cert errors each get different short-descriptions, and are, in principle, user-serviceable (when they want to override trust, that is). So I think the emphasis there should be on clarity and simplicity of problem communication, and helping them understand how to "solve" it if they feel confident doing so.
Comment on attachment 284679 [details] [diff] [review] WIP patch >Index: browser/locales/en-US/chrome/overrides/netError.dtd >+ <li>You can see and change your current list of servers with known security problems >+ in your advanced encryption preferences.</li> So where are my "advanced encryption preferences"? You don't happen to mean Tools > Options > Advanced > Encryption > View Certificates > Servers, do you? ;-) As long as we don't have a button opening the Servers tab of the Certificate Manager, we need to tell the users how to find the Add Exception dialog.
I think the same generic text that needs to be in dom/locales/en-US/chrome/netError.dtd is used in the overrides in this patch as well - and the dom/ one needs to be generic, e.g. it's "Edit > Preferences > Privacy & Security > Certificates > Manage Certificates > Servers" here... In any case, it's probably buried deep in prefs in every application.
(In reply to comment #12) > I'd like to get nsNSSIOLayer to the point where host:port are shown on protocol > errors, but just host on bad cert errors You are proposing to drop the port number on bad cert errors. I don't understand why you feel that is necessary. I suspect you think it looks ugly? I believe the port number is a critical piece of information. nsNSSIOLayer is platform code. It's not just Firefox. The platform code has no idea what protocol runs over this raw SSL socket. The very same code is used for all Mozilla applications, which includes, for example, Java applications that implement their own binary communication to an application server (I've done that 10 years ago). Also: Mail, IRC, LDAP, extensions downloading other data. Even when only using https, the port number is helpful. I have seen web applications, where two servers run on the same host, on different port numbers (Java required that all communication goes to the same hostname where the applet was downloaded from, if you want to avoid exceptions). The main port serving static content, another port serving dynamic content. Small business often have a single hosted server for everything. Did that bad cert happen with the web service or something else? How should the user tell the difference? If you really consider to hide the port number in error pages in Firefox, then we should find a way for you to do that without affecting the shared platform code, but I vote for keeping it. If you're only worried about the default case, maybe you could add code to the docshell that detects that all of the following is true: - protocol is https - port is 443 - the error is being reported as an error page in the browser Maybe in that combination it's ok to hide the :443, but I think in all other scenarios it should be shown. > and where the error code is only shown on protocol errors. My argumentation will be similar to what I said before. I think it should be kept, even on the bad cert errors page, for these reasons: - the error code helps to exactly distinguish between the various scenarios why a bad cert is untrusted - the human readable text might be localized, a user sends a screen shot to the company they have a support contract with, and the operator is unable to read that foreign language. Only the always-english error code will enable the operator to make do an exact diagnosis. - this is shared code, also used for showing a dialog box in those situations where an error page is not possible (and therefore the enhanced explanations can not be shown). If you really feel we must hide the error code in Firefox, then we should find a way to do that in Firefox, only. Please note that hiding the error code will make it very hard for us to triage bug reports. We might be unable to. PSM and NSS developers will be frustrated. We get a bug report, from a person who is trying to access an intranet site that we don't have access to. Now the engineers wonder: Is our code broken? Or is that intranet site broken? We really can't tell without full diagnostic information. Please let's keep the error code, maybe smaller, maybe further down on the page. > By contrast, bad cert errors each get different short-descriptions, and are, in > principle, user-serviceable (when they want to override trust, that is). So I > think the emphasis there should be on clarity and simplicity of problem > communication, and helping them understand how to "solve" it if they feel > confident doing so. I don't think omitting a port number and an error code makes it easier for them. How would users compare the error message (without port) against the stored overrides in cert manager (includes port).
(In reply to comment #15) > (In reply to comment #12) > > I'd like to get nsNSSIOLayer to the point where host:port are shown on protocol > > errors, but just host on bad cert errors > > You are proposing to drop the port number on bad cert errors. I don't > understand why you feel that is necessary. I suspect you think it looks ugly? > > I believe the port number is a critical piece of information. > > nsNSSIOLayer is platform code. It's not just Firefox. The platform code has no > idea what protocol runs over this raw SSL socket. >... > > If you're only worried about the default case, maybe you could add code to the > docshell that detects that all of the following is true: > - protocol is https > - port is 443 > - the error is being reported as an error page in the browser I think everything you say here is correct, Kai. That I tried hiding the port number here because it complicates the UI needlessly in the common web case (https on port 443). That the changes absolutely have to account for the fact that these messages are used everywhere (the fact that it didn't is precisely why I called this a work in progress patch.) As for whether this is better handled at the browser level or NSS level, I would appreciate any more thoughts you can offer about implementation. It feels to me like other NSS consumers employing standard-port https would benefit from this change in their error messages as well, but you know the current consumers better than I, and likewise the paths those consumers take. > Maybe in that combination it's ok to hide the :443, but I think in all other > scenarios it should be shown. Yes, I agree - even in the web case, doing things on non-standard ports is noteworthy enough to include. > > and where the error code is only shown on protocol errors. > > My argumentation will be similar to what I said before. > > I think it should be kept, even on the bad cert errors page, for these reasons: > > - the error code helps to exactly distinguish between the various scenarios why > a bad cert is untrusted > > - the human readable text might be localized, a user sends a screen shot to the > company they have a support contract with, and the operator is unable to read > that foreign language. Only the always-english error code will enable the > operator to make do an exact diagnosis. > > - this is shared code, also used for showing a dialog box in those situations > where an error page is not possible (and therefore the enhanced explanations > can not be shown). > > If you really feel we must hide the error code in Firefox, then we should find > a way to do that in Firefox, only. > > Please note that hiding the error code will make it very hard for us to triage > bug reports. > We might be unable to. > PSM and NSS developers will be frustrated. > We get a bug report, from a person who is trying to access an intranet site > that we don't have access to. > Now the engineers wonder: Is our code broken? Or is that intranet site broken? > We really can't tell without full diagnostic information. > Please let's keep the error code, maybe smaller, maybe further down on the > page. I'm certainly open to that approach. My initial mockup included the error code as the last bullet - the only catch being that the current netError code doesn't allow for dynamically populating the long description field, since it's just taken from a DTD. Another option, from a diagnostic point of view, is to log-but-hide the error codes in some way - either in the error console or hidden on the error page. A "More Information" expanding box would be an option, for example, although again, the support for this doesn't currently exist in netError.xhtml. What do you think here about the various options? > > By contrast, bad cert errors each get different short-descriptions, and are, in > > principle, user-serviceable (when they want to override trust, that is). So I > > think the emphasis there should be on clarity and simplicity of problem > > communication, and helping them understand how to "solve" it if they feel > > confident doing so. > > I don't think omitting a port number and an error code makes it easier for > them. > > How would users compare the error message (without port) against the stored > overrides in cert manager (includes port). I think typical users don't really understand the notion of ports in the first place - at least that's been my experience. I think that in the cert override tree, they are very likely to ignore port and focus exclusively on domain name, but see above, I'm open to the idea that only port 443 be hidden here, since that is the case where I think this error page will most often be displayed.
Kai - another thought - what if presence of error codes and port numbers are controlled by prefs? This would give firefox an easy way to override, without making core changes to PSM's behaviour more broadly. It would also give diagnostic ability to anyone who needs it - flip the pref and find the error code. What do you think?
Johnathan, The first time people ask for help, they report whatever information they have at hand. Without the error codes and other info, we cannot help them, so we have to ask them to go do it again and this time, get the secret diagnostic info. That's bad for EVERYONE who is in the help desk, trouble-resolution game. The user should get the info he needs the first time, without having to be hand-held to get it. It's best if the diagnostic info is BOTH localized for the user AND is also available in a form that Mozilla developers/helpers can understand. In the past, error codes were shown in decimal or hex, which no-one understood, so all users either took it as a dead end, or they asked in newsgroups about it. For YEARS, I asked that the diagnostic info be provided to users in language they can understand (their native tongue). Then someone raised the issue that if the error code says "host name mismatch" in Chechoslovakian (sp?) then it might make no sense to the (English speaking) mozilla developers, when they are asked about it. That's how we got into the bizarre state of having a localized text part and the English-like name of the error code in parentheses.
Attached patch tweak error code and port number (obsolete) — — Splinter Review
After having a chat with Johnathan, we consider the following compromise: - continue to show the error code, but state it like this: "If you are speaking with tech support, you can tell them the error code is: SEC_ERROR_WHATEVER" - Only if both of the following is true - the error will be shown as an error page (not a dialog) - the port number is 443 then the port number will not be shown.
Attached patch merged patch (obsolete) — — Splinter Review
This is the latest merged patch.
Attachment #284679 - Attachment is obsolete: true
Attachment #285918 - Attachment is obsolete: true
Thanks for merging the patches, Kai, I think this is close to being ready for review, and regardless of what other UI changes we make in other bugs, this is an important improvement. Two things: >- if (hostWithPort.Length()) >- { >+ if (externalErrorReporting && port == 443) >+ params[0] = host.get(); >+ else > params[0] = hostWithPort.get(); Thing 1: I think I would suggest adding a comment before the added code, so that future generations understand what's happening here, e.g. // Show ports in all cases except 443 (https). Port 443 is the 99% case here, // so removing it doesn't really lose any important debugging information, but // does save non-technical users from having to understand it. Thing 2: The changes in my original patch (and hence in your merged patch as well) in netError.css should be benign, since all they add is whitespace support for respecting "\n" characters. Only one string in appstrings.properties currently contains line breaks anyhow ( http://mxr.mozilla.org/mozilla1.8/source/dom/locales/en-US/chrome/appstrings.properties ) and those are presumably fixed by this change as well. Nevertheless, we should probably get biesi/bz to okay the change. Other than that, I think we need to finesse the wordings to address concerns like comment 14, and even though it's partly your patch, I'd like to get you (or someone else if you prefer) to review the code changes as well, obviously. If you agree with my comment nit, then I think the next step is to get beltzner to provide some ui-r feedback since you and I are too close to the strings, at this point. :)
(In reply to comment #21) > Thing 1: I think I would suggest adding a comment before the added code, so > that future generations understand what's happening here, e.g. > > // Show ports in all cases except 443 (https). Port 443 is the 99% case here, > // so removing it doesn't really lose any important debugging information, but > // does save non-technical users from having to understand it. I'm ok with adding a comment, but I think this comment is not true enough. Right now this code is doing a workaround. It has no knowledge whether we're doing https or something else. We are hiding the port number when we know we are running in a context that is implementing a securityUI object, that's all we really know at this point. // For now, hide port when it's 443 and we're reporting the error using // external reporting. In the future a better mechanism should be used // to make a decision about showing the port number, possibly by requiring // the context object to implement a specific interface. // The motivation is that Mozilla browser would like to hide the port number // in error pages in the common case.
Blocks: 399389
I'm in merging hell. At the same time as this work is happening, I was working on bug 399389 to enhance the error pages further. I had merge conflicts, and it does not make sense to serialize the work, as we are short on time. So I decided to move that patch over to this bug and merge it. Note that some portions have already been reviewed. Please look at bug 399389 comment 16 or attachment 285957 [details] [diff] [review] to see the already-reviewed-portions. That code is about the new certErrorTrust_ strings, the is-self-signed code in nsIX509Cert3 and the code for nsICertOverrideService::ERROR_UNTRUSTED. It also adds a prioritizing for the mutliple-reasons-for-bad-cert scenarios, because we can only show a single error code. In addition, we'd like to do another change at the same time. When there is a domain mismatch, the new error page currently says something like this: www.A.com uses invalid cert. cert not valid for www.A.com Bob Lord pointed out this is confusing. It would be better to say something like: www.A.com uses invalid cert. cert only valid for www.B.com Unfortunately, to get this right, we must pull in some more calls to NSS code, because there are multiple places where the "allowed" names are listed. We can do most right now. (There's a small place that we'll have to fix later (bug 400917) The new code for that is the big block after: (multipleCollectedErrors & nsICertOverrideService::ERROR_MISMATCH) I realized, when we are unable to obtain the certErrorCodePrefix from the bundle (failure), we should append the plain error code (without prefix). Furthermore, this patch introduces a new separate error page for the bad-cert-errors and keeps the other error page for SSL protocol unchanged. I think we should change both pages in the same way, I think: Add the prefix for the error code, add the line breaks for better readability, use the same with-or-without-port logic.
Attached patch Patch v4 (obsolete) — — Splinter Review
Attachment #285923 - Attachment is obsolete: true
(In reply to comment #21) > Thing 2: The changes in my original patch (and hence in your merged patch as > well) in netError.css should be benign, since all they add is whitespace > support for respecting "\n" characters. Only one string in > appstrings.properties currently contains line breaks anyhow ( > http://mxr.mozilla.org/mozilla1.8/source/dom/locales/en-US/chrome/appstrings.properties > ) and those are presumably fixed by this change as well. Nevertheless, we > should probably get biesi/bz to okay the change. Boris, Christian, are you ok with the proposed use of \n\n and the changes at the end of attachment 285960 [details] [diff] [review] to enable them in the SSL error pages?
(In reply to comment #21) > even though it's partly your patch, I'd like to get you > (or someone else if you prefer) to review the code changes as well, obviously. I think Bob R should review it, as it now contains more code from me, too. > I think the next step is to get beltzner > to provide some ui-r feedback since you and I are too close to the strings, at > this point. :) Johnathan, before we ask him, could you please have a look at the additional strings I've added? They start with certErrorTrust_ or certErrorMismatch
> Boris, Christian, are you ok with the proposed use of \n\n Which is what? Using it as a paragraph break, basically? Can we just use actual <p> tags, or is the infrastructure really not set up for it?
(In reply to comment #27) > > Boris, Christian, are you ok with the proposed use of \n\n > > Which is what? Using it as a paragraph break, basically? > > Can we just use actual <p> tags, or is the infrastructure really not set up for > it? I don't know if the error page is able to use html tags. I had tried that in the past in my early phases of working on bug 107491, and I remember it didn't work. Furthermore, the strings we are producing here are being used in both the error page and in error dialogs. Although we might be able to produce separate strings for both scenarios.
I'm fine with using the \n\n as a paragraph separator, yeah. I'm assuming that that's what comment 28 means.... It didn't really answer my question. ;)
(In reply to comment #29) > I'm fine with using the \n\n as a paragraph separator, yeah. I'm assuming that > that's what comment 28 means.... It didn't really answer my question. ;) Boris - yes, it's just using them as paragraph breaks. Using <p> tags would work with netError.xhtml - (we use them all over the place in netError.dtd, for instance.) But the "short description" strings don't tend to include markup in general, or <p> tags in particular, and as kai mentions, that would make it more complicated to re-use the strings in error situations where a page can't be displayed. All of which is fyi, I now realise, since you led off with "I'm fine with it" :)
Comment on attachment 285960 [details] [diff] [review] Patch v4 >Index: mozilla/security/manager/locales/en-US/chrome/pipnss/pipnss.properties >=================================================================== >RCS file: /cvsroot/mozilla/security/manager/locales/en-US/chrome/pipnss/pipnss.properties,v >+certErrorIntro=%S uses an invalid security certificate. >+ >+certErrorTrust_SelfSigned=The certificate is not trusted because it is self signed. >+certErrorTrust_UnknownIssuer=The certificate is not trusted because the issuer certificate is unknown. >+certErrorTrust_CaInvalid=The certificate is not trusted because it was issued by an invalid CA certificate. >+certErrorTrust_Issuer=The certificate is not trusted because the issuer certificate is not trusted. >+certErrorTrust_ExpiredIssuer=The certificate is not trusted because the issuer certificate has expired. >+certErrorTrust_Untrusted=The certificate does not come from a trusted source. Other than certErrorTrust_Untrusted, these still feel sort of technical - they require the user to understand concepts like certificate signing. Maybe that means I'm arguing that we shouldn't actually report these as separate problems after all, I'm not sure, tbh. I think, for example, the difference between _Issuer and _UnknownIssuer will be a hard one to explain to users - I understand that there's a difference between not recognizing an issuer and recognizing, but not trusting. Nevertheless, if that is not a decision most users ever make, the distinction seems likely to be confusing. In a sense, the text in _Untrusted is basically accurate for all of them, though I appreciate it's less specific than one might like. Is this something we want to do for the sake of end users, or web admins debugging? Would it be enough, for the latter case, to rely on the error code? I know a fair bit of work went in to making this kind of reporting possible and I'm not trying to dismiss it, I'm just trying to get my head around the intended usages of the data. >+certErrorMismatch=The certificate is not valid for the name %S. >+certErrorMismatchSingle=The certificate is only valid for name %S. >+certErrorMismatchMultiple=The certificate is only valid for the following names: I think being clearer here is great - particularly with Bob's suggestion to call out who the cert is actually for. What is the purpose for certErrorMismatch though? Won't it always be either certErrorMismatchSingle or certErrorMismatchMultiple? >+certErrorCodePrefix=If you are speaking with tech support, you can tell them the error code is: %S. Whatever text we go with here, if we intend users to see and respond to these error pages, do we have a wiki page or something that they're likely to encounter when searching? A quick google search just now didn't find anything, maybe we need a follow-up bug to create that, or maybe it exists and just doesn't have enough pagerank?
I think the key here is for a significant vocal fraction of the users, the more information is helpful. Even the average user will start to recognize things like 'self-signed'. For those users that won't understand the second part the first part is key (this certificate is not trusted). It's also helpful to administrators when the user send email saying "your site was broken in firefox because: error". The question is does the extra text cause confusion to the user (will the average user think they need to act on the extra data). That's where we have to depend on our UI designer;). For the latter, an error code is less helpful. Someone who knows infrastructure, may still not know what -8135 means (I have to look those up and I use those error codes all the time). Also, we don't have an error code for 'self-signed', though an admin would want to know that. In short, we are doing this for the admin/advanced user and hoping that it doesn't confuse the naive/average user.
(In reply to comment #31) > > >+certErrorMismatch=The certificate is not valid for the name %S. > >+certErrorMismatchSingle=The certificate is only valid for name %S. > >+certErrorMismatchMultiple=The certificate is only valid for the following names: > > I think being clearer here is great - particularly with Bob's suggestion to > call out who the cert is actually for. What is the purpose for > certErrorMismatch though? Won't it always be either certErrorMismatchSingle or > certErrorMismatchMultiple? Correct. certErrorMismatch is the old string and no longer user. I was reluctant to take it out. (Maybe it will be good to have that string available after the string freeze. I'm overly careful here.) > >+certErrorCodePrefix=If you are speaking with tech support, you can tell them the error code is: %S. > > Whatever text we go with here, if we intend users to see and respond to these > error pages, do we have a wiki page or something that they're likely to > encounter when searching? A quick google search just now didn't find anything, What search term did you use?
(In reply to comment #33) > Correct. certErrorMismatch is the old string and no longer user. > I was reluctant to take it out. > (Maybe it will be good to have that string available after the string freeze. > I'm overly careful here.) Overly conservative, when it comes to strings that are already translated and hence probably not an extra hit to localizers, seems like a smart thing to be, yeah. > > Whatever text we go with here, if we intend users to see and respond to these > > error pages, do we have a wiki page or something that they're likely to > > encounter when searching? A quick google search just now didn't find anything, > > What search term did you use? I went to https://www.kuix.de/, and then upon reading the error page message, searched for "ssl_error_bad_cert_domain." There are results there that a technical user could probably understand, but there was not an obvious page establishing each of the expected errors and what they meant to me.
Johnathan asked: >... these still feel sort of technical - they require the user to understand > concepts like certificate signing. Maybe that means I'm arguing that we > shouldn't actually report these as separate problems after all, I'm not sure, > Is this something we want to do for the sake of end users, or web admins > debugging? Our users, or their admins, need some way to figure out what's really wrong. When they can't figure it out with the information we've given them, they write and ask us for help. I have spent FAR too much of my life answering those questions, because historically we have given the users FAR too little information. If you don't mind me forwarding all those inquiries to you, then you may dumb down the messages, but I hope your inbox has large capacity for years to come. Having a web page that answers the question doesn't help. We have web pages that answer many of these questions, and even the people who most frequently answer questions in mozilla newsgroups don't know about them. The best solution is one that obviates that first inquiry email by providing enough information for someone to diagnose and correct them without asking us.
(In reply to comment #34) > I went to https://www.kuix.de/, and then upon reading the error page message, > searched for "ssl_error_bad_cert_domain." There are results there that a > technical user could probably understand, but there was not an obvious page > establishing each of the expected errors and what they meant to me. I get http://www.mozilla.org/projects/security/pki/nss/ref/ssl/sslerr.html as result number 6, which lists all error codes and their explanation. I guess it can't hurt for someone to create a new wiki page and list some of the most popular error codes, and try to make it rank high. (In reply to comment #35) > Having a web page that answers the question doesn't help. We have web pages > that answer many of these questions, and even the people who most frequently > answer questions in mozilla newsgroups don't know about them. Maybe we should try to get a higher rank for those pages? Enabling users to find more information on the web seems a good thing (if they really want to)... > The best solution is one that obviates that first inquiry email by providing > enough information for someone to diagnose and correct them without asking us. ... but I absolute agree here.
Johnathan, please see bug 107491 for some of the history. There is a long history of users begging for more useful information. Lack of useful information with which to solve their own problems is a cause of user dissatisfaction. I've not seen much evidence that too much information causes much user dissatisfaction.
Blocks: 107491
I wouldn't expect too many existing search results for these errors yet, simply because most ends users have never been exposed to them. That problem will apparently be correcting itself. :-) It might be a good idea to "prime the pumps" by getting relevant/appropriate information on support.mozilla.com for end users, and on DevMo or WikiMo for webadmins.
(In reply to comment #31) > Other than certErrorTrust_Untrusted, these still feel sort of technical - they > require the user to understand concepts like certificate signing. Maybe that > means I'm arguing that we shouldn't actually report these as separate problems > after all, I'm not sure, tbh. I think, for example, the difference between > _Issuer and _UnknownIssuer will be a hard one to explain to users - I > understand that there's a difference between not recognizing an issuer and > recognizing, but not trusting. Nevertheless, if that is not a decision most > users ever make, the distinction seems likely to be confusing. In a sense, the > text in _Untrusted is basically accurate for all of them, though I appreciate > it's less specific than one might like. > > Is this something we want to do for the sake of end users, or web admins > debugging? Would it be enough, for the latter case, to rely on the error code? > I know a fair bit of work went in to making this kind of reporting possible > and I'm not trying to dismiss it, I'm just trying to get my head around the > intended usages of the data. I think in most cases, most people won't really be able to understand what's going on, and there's no level of wordsmithing that will help them. The one message we should convey to the general user is "The server is broken, not FF. In fact FF is protecting you". The messages below the line say that. The other target of this information is the web site administrators. The more clearly worded the error, the more likely they are to fix the problem. So when we're blocking the connection because the site uses a self-signed cert (for example), we should say exactly that, using terms the admins will understand. Let's make sure that when the user copy/pastes the text into a trouble ticket report, that it provides clear guidance to the site admins.
Especially for users who are used to clicking through SSL errors, the proposed error page still seems a bit of a frustrating dead end with no immediate option other than "Try Again". Wouldn't it be possible to put a direct link on the error page to a KB article on SUMO which would clearly illustrate how to add a security exception (and how to enable the pref to restore the per-page override option, if bug 399275 gets implemented)? The current mention of "your advanced encryption preferences" seems rather vague to me.
Attached patch A few more string changes (obsolete) — — Splinter Review
This is basically Kai's patch with a couple string changes, specifically: - Remove the first sentence of the first longDesc bullet since it basically just says what has been said already, that the site failed to identify properly. - Shorten the error code string to something which doesn't actively suggest the presence of technical support. - Swap the platform-specific "Preferences" for the platform-agnostic "Settings" I'll attach updated screenshots as well. I think we should get this reviewed & landed, and file follow-ups for other changes as needed. I understand that ports and error codes are points of contention, which is why the current patch hides only port 443, and preserves error codes; I think trying to nail those down here will prevent us from getting better strings in ASAP. Kai - given that both of us have edited this pretty heavily, who should review, BobR? Nelson?
Attachment #285960 - Attachment is obsolete: true
Updated screenshots attached. *Note: Looking at this, I realise that the domain mismatch string should lose the word "name" and just be: +certErrorMismatchSingle=The certificate is only valid for %S.
"amazon uses an invalid security certificate. The certificate is only valid for www.amazon.com." Can't we turn "www.amazon.com" into a link to that site? That would make much more sense than "Try again" on amazon.com.
(In reply to comment #43) > Can't we turn "www.amazon.com" into a link to that site? > That would make much more sense than "Try again" on amazon.com. For example.com to www.example.com this would make sense. But it would have to be special-cased. In the common case where one certificate is used for dozens of virtual hosts this would only confuse the user because the domain of the certificate most often won't have anything to do with the visited site.
Attached image Error page proposal —
There seem to have been quite a few negative comments recently about the new SSL error pages, with many users annoyed that there is seemingly no apparent way to get past the error and view the site. The patch in this bug will certainly improve the situation, but I still think the reference to "advanced encryption settings" is too vague. So, I think that the decision not to provide a direct link to the Add Security Exception dialog should be reconsidered. (Quoting from bug 327181 comment 157, slightly edited): "A button could be provided on the error page that opened the Certificate Manager at the Servers tab and the Add Security Exception dialog in front of it (and perhaps pre-filled the Server Location field?). This would mean that only five clicks would be required to actually see the site if the user is convinced that it's safe and wants to override (click on View Certificate and Add Security Exception... button, Get Certificate, Confirm Security Exception, OK and then Reload); rather than up to 12 (!) steps at the moment. Also, I think that the text warning ("You are about to override how Firefox identifies sites. Legitimate banks, stores, and other public sites will not ask you to do this.") and yellow caution triangle on the Add Security Exception dialog are probably enough to warn casual users from adding an exception unless they're absolutely sure the site is safe. As a side-benefit, this approach would let the user view the invalid certificate in question; this is not very easy at the moment when you hit the error page."
(In reply to comment #45) > a direct link to the Add Security Exception dialog should be reconsidered. See bug 401575
Comment on attachment 286583 [details] [diff] [review] A few more string changes r-, The patch looks generally good, but the ERROR_MISMATCH block as some problems. First, there is a memory leak with the CERT_FindCertExtension. The data in altNameExtension needs to be freed. (you can do this with SECITEM_FreeItem with parameter PR_FALSE). That one must be fixed. Also, the block is a very long code block with lots of extra conditionals embedded in a middle of a switch statement for an alreadly very long function. It's crying out to be broken up into at least one or two extra functions (making getting the SAN it's own function would greatly simplify the logic and get rid of lots of if (useSAN) blocks. Fix the first part immediately for a r+. I'd also like to see an updated patch with fixes the latter (which will help future maintainability). bob
Attachment #286583 - Flags: review-
Attached patch Free altNameExtension (obsolete) — — Splinter Review
Thanks for the review Bob - this patch just adds the one SECITEM_FreeItem call, I'm happy to file a followup to clean up that function, but I don't feel comfortable messing about in that code, tbh; I'm hoping Kai can do it in 3 minutes. :) Let me know if the free looks okay?
Attachment #286583 - Attachment is obsolete: true
Attachment #286710 - Flags: review?(rrelyea)
Comment on attachment 286710 [details] [diff] [review] Free altNameExtension r+ The cleanup can wait for Kai. bob
Attachment #286710 - Flags: review?(rrelyea) → review+
Blocks: 401755
Comment on attachment 286710 [details] [diff] [review] Free altNameExtension r=biesi on the docshell changes
Attachment #286710 - Flags: review+
Attached patch unbitrotten patch (obsolete) — — Splinter Review
for check-in
Attachment #286710 - Attachment is obsolete: true
er, with -u8p this time.
Attachment #286730 - Attachment is obsolete: true
Needed for M9 since the current error messages on trunk are - astoundingly - less comprehensible than the ones they replaced :)
Target Milestone: --- → mozilla1.9 M9
Checking in browser/locales/en-US/chrome/overrides/netError.dtd; /cvsroot/mozilla/browser/locales/en-US/chrome/overrides/netError.dtd,v <-- netError.dtd new revision: 1.11; previous revision: 1.10 done Checking in camino/embed-replacements/locale/en-US/global/netError.dtd; /cvsroot/mozilla/camino/embed-replacements/locale/en-US/global/netError.dtd,v <-- netError.dtd new revision: 1.7; previous revision: 1.6 done Checking in docshell/base/nsDocShell.cpp; /cvsroot/mozilla/docshell/base/nsDocShell.cpp,v <-- nsDocShell.cpp new revision: 1.864; previous revision: 1.863 done Checking in docshell/resources/content/netError.xhtml; /cvsroot/mozilla/docshell/resources/content/netError.xhtml,v <-- netError.xhtml new revision: 1.24; previous revision: 1.23 done Checking in dom/locales/en-US/chrome/netError.dtd; /cvsroot/mozilla/dom/locales/en-US/chrome/netError.dtd,v <-- netError.dtd new revision: 1.11; previous revision: 1.10 done Checking in netwerk/base/public/nsINSSErrorsService.idl; /cvsroot/mozilla/netwerk/base/public/nsINSSErrorsService.idl,v <-- nsINSSErrorsService.idl new revision: 1.2; previous revision: 1.1 done Checking in security/manager/locales/en-US/chrome/pipnss/pipnss.properties; /cvsroot/mozilla/security/manager/locales/en-US/chrome/pipnss/pipnss.properties,v <-- pipnss.properties new revision: 1.29; previous revision: 1.28 done Checking in security/manager/ssl/public/nsIX509Cert3.idl; /cvsroot/mozilla/security/manager/ssl/public/nsIX509Cert3.idl,v <-- nsIX509Cert3.idl new revision: 1.4; previous revision: 1.3 done Checking in security/manager/ssl/src/nsNSSCertificate.cpp; /cvsroot/mozilla/security/manager/ssl/src/nsNSSCertificate.cpp,v <-- nsNSSCertificate.cpp new revision: 1.133; previous revision: 1.132 done Checking in security/manager/ssl/src/nsNSSComponent.cpp; /cvsroot/mozilla/security/manager/ssl/src/nsNSSComponent.cpp,v <-- nsNSSComponent.cpp new revision: 1.156; previous revision: 1.155 done Checking in security/manager/ssl/src/nsNSSErrors.cpp; /cvsroot/mozilla/security/manager/ssl/src/nsNSSErrors.cpp,v <-- nsNSSErrors.cpp new revision: 1.4; previous revision: 1.3 done Checking in security/manager/ssl/src/nsNSSIOLayer.cpp; /cvsroot/mozilla/security/manager/ssl/src/nsNSSIOLayer.cpp,v <-- nsNSSIOLayer.cpp new revision: 1.139; previous revision: 1.138 done Checking in toolkit/themes/pinstripe/global/netError.css; /cvsroot/mozilla/toolkit/themes/pinstripe/global/netError.css,v <-- netError.css new revision: 1.6; previous revision: 1.5 done Checking in toolkit/themes/winstripe/global/netError.css; /cvsroot/mozilla/toolkit/themes/winstripe/global/netError.css,v <-- netError.css new revision: 1.6; previous revision: 1.5 done
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
I had to commit a bustage fix for this that consisted of moving the INET6_ADDRSTRLEN definition from nsNSSCertHelper.cpp to nsNSSCertHelper.h, as the patch used INET6_ADDRSTRLEN in nsNSSIOLayer.cpp. Not sure if this is the best fix, but it works for now. Should the #define have just been added to the top of nsNSSIOLayer.cpp, too, or is there a better fix?
(In reply to comment #55) > I had to commit a bustage fix for this that consisted of moving the > INET6_ADDRSTRLEN definition from nsNSSCertHelper.cpp to nsNSSCertHelper.h, as > the patch used INET6_ADDRSTRLEN in nsNSSIOLayer.cpp. Not sure if this is the > best fix, but it works for now. Should the #define have just been added to the > top of nsNSSIOLayer.cpp, too, or is there a better fix? Not sure. By having it in a .cpp file, after all other includes, we can ensure that our include won't override a definition in a platform include. I'd personally have prefered to be safe and have it in both .cpp files. But on the other hand, the number of bytes required to hold an IPv6 address probably won't change anyway.
Bob, here is the requested cleanup you requested. This patch is a nightmare to review, because it contains moved code and change of indenting. Because of that I will attach a separate patch that displays the changes much more easily.
This patch illustrates what I'm really doing: - moved code to separate 4 functions that each append some text - declare some more local helper variables, where needed - enhanced the code that produces the expiration error message. If we're unable to format the time, we should not abort without a message. We should still display the correct wording, and simply leave the time value empty. Note, in this special version of the patch: - the new functions are below getInvalidCertErrorMessage, so the order of the code is kept, and therefore the diff is minimal - added forward declarations to make it compile - ignores whitespace changes But I really want to check in the other version of the patch, which has correct whitespace and has the new functions before getInvalidCertErrorMessage, so the forward declarations are not necessary.
Attachment #286874 - Flags: review?(rrelyea)
Comment on attachment 286874 [details] [diff] [review] Cleanup Patch v1 (reviewer friendly) Oops, sorry, I didn't see that you had filed 401755 until now :-/ So, let's move the review discussion of the cleanup patch over there.
Attachment #286874 - Flags: review?(rrelyea)
Attachment #286872 - Attachment is obsolete: true
Attachment #286874 - Attachment is obsolete: true
Attachment #286874 - Attachment is patch: true
Attachment #286874 - Attachment mime type: application/octet-stream → text/plain
(In reply to comment #42) > *Note: Looking at this, I realise that the domain mismatch string should lose > the word "name" and just be: > > +certErrorMismatchSingle=The certificate is only valid for %S. This comment did not yet get addressed.
(In reply to comment #60) > (In reply to comment #42) > > +certErrorMismatchSingle=The certificate is only valid for %S. > > This comment did not yet get addressed. bug 401575 picked up this change in the latest patch partly by accident, but I don't think it's particularly inappropriate for it to be there. If the patch lands with that intact, this will be addressed.
Blocks: 411246
Attached image error page in 2010 —
This is how the error page looks like in 2010.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: