Closed Bug 521137 Opened 15 years ago Closed 14 years ago

Improve content of certificate failure overlay page (and allow localization)

Categories

(Camino Graveyard :: General, defect)

Product:
Camino Graveyard
Component:
General
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Camino2.1

People

(Reporter: stuart.morgan+bugzilla, Assigned: alqahira)

References

(
URL
)

Details

(Whiteboard: l10n 1.9.1)

Attachments

(6 files, 5 obsolete files)

A case of the uglies
128.67 KB, image/png
Details
Supplemental diff, certError.xhtml vs Firefox 3.6 version
6.67 KB, patch
Details | Diff | Splinter Review
untrusted overlay, sprinkled with some colors
173.63 KB, image/png
Details
Possible strings, as diff against Caminified Firefox strings
2.67 KB, patch
stuart.morgan+bugzilla
: feedback+
Details | Diff | Splinter Review
neterror.css patch v0.4
3.06 KB, patch
alqahira
: review+
stuart.morgan+bugzilla
: superreview+
Details | Diff | Splinter Review
Implements about:certerror for Camino v1.0
44.02 KB, patch
stuart.morgan+bugzilla
: superreview+
Details | Diff | Splinter Review 
The current cert failure overlay is terrible: high on technobabble and low on understandability. Apparently the Firefox folks realized that, since the one they use in 3.5 is actually quite nice. We should do something similar, giving useful instructions/description, and hiding the technobabble.
For reference, Firefox forked and completely reimplemented the cert error page as their own component: bug 431826

I had thought that I had remembered that there were additional Core changes required to support the new design (other than just the docshell change to allow registering a new component to handle the override), but the final patch and the deps in the bug look otherwise.  

I think that means we could just fork netError.html entirely (although we'd then have to maintain it in the face of ongoing regular error page changes) rather than add a component and depend on 1.9.1-only changes in docshell.

OTOH, a separate, specific component is cleaner and less onerous to maintain, and the docshell change looks very minimal and safe, such that we might be able to argue for its inclusion on 1.9.0.
URL: http://blog.johnath.com/2008/11/06/ss...
More and more I think we should just fork netError.xhtml entirely, so that we can fix it to use localizable strings for network errors and cert errors, either in the manner we did with blockedSite.xhtml (our own component parses the file before showing it and inserts the strings directly from our own Localizable.strings, bug 476095) or the way we handled about:crashes (adding some Gecko JS code to the page to load a stringbundle and then moving all the strings out of a .dtd into a CHStringBundleOverride properties.strings file, bug 489512).

I really wish I had thought of this before l10n freeze for 2.0, though :(
We should do this for 2.1; I'm currently polling l10n to see if they can handle taking it in 2.0.1 if we can implement it in the next couple of weeks.

It slipped my mind earlier that this was new, untranslatable security UI we were adding, so we really ought to fix it, and get the old network errors while we're at it.
Target Milestone: --- → Camino2.1
I started poking at this, and I have most of hendy's stuff from bug 489512 ported into a basic embed-replacements netError.properties to start out, but nothing's running because

JavaScript error: about:neterror?e=connectionFailure&u=https%3A//krak-des-chevaliers.local/&c=UTF-8&d=The%20connection%20was%20refused%20when%20attempting%20to%20contact%20krak-des-chevaliers.local., line 78: Permission denied to get property XPCComponents.classes

(that's where we define
       const Cc = Components.classes;
for later use by the stringbundle-fetching code).
My guess is that the security context of the error pages prevents that from working, which means we're back to "can't localize".
The other thing we could try is to load a list of strings via JS (via chrome), sort of how Safari does their Safe Browsing l10n; it would be more hacky, and I'm not sure it would work, but it might be worth poking.
Flags: camino2.0.2?
Flags: camino2.0.1?
Flags: camino2.0.1-
For 2.1, if we're on 1.9.1 or higher and we have cycles, we should just make our own component and reuse murph's code from safebrowsing.
Whiteboard: l10n
(In reply to comment #6)
> The other thing we could try is to load a list of strings via JS (via chrome),
> sort of how Safari does their Safe Browsing l10n; it would be more hacky, and
> I'm not sure it would work, but it might be worth poking.

I'm not going to have time to get to this any more, and we're now 3+ months in to 2.0.x, so I think we should just minus it for good for 2.0.x and do comment 7 for 2.1.
Flags: camino2.0.2? → camino2.0.2-
Whiteboard: l10n → l10n 1.9.1
I had been thinking, as a first step, of just copying Firefox's JS certerror component/aboutModule introduced in bug 431826; it wouldn't get us localization, but it would let us do a new design (and we could later migrate to a version of murph's safebrowsing aboutModule, which would then get us localization).

I see now that bug 499123 zapped all of the Firefox JS component nsAboutModules and coalesced them into a single C++ nsAboutModule (basically a fork of docshell's nsAboutModule impl.  I wonder if we should move in that direction, too; we currently have two nsAboutModule impls, SafeBrowsingAboutModule and nsAboutBookmarks; we want a third one for this bug, and a fourth one for about:crashes (short-term, it lets us set --disable-crashreporter as the default and save compile time; long-term, it lets us rewrite about:crashes in Cocoa to have better UI and support more things, like resending).
While we maybe should do the combined thing, my friends "Find" and "Replace" and I can do the "copy murph's code" thing.

This is a mostly-functional WIP.

1) I shoved the aboutModule implementation in src/browser.  Unlike safebrowsing, there aren't a ton of implementation files, so it didn't make sense for them to have their own subdir, and browser/ seemed like the most-correct location for them.

2) The patch actually shows a 'hg cp' of the aboutModule files, followed by the changes (relative to the safebrowsing originals); this seems like it makes a saner diff.

3) The "Technical Details" are being shown by default; I'm not sure why yet.

4) The "Get me out of here!" button doesn't work; I assume we have to wire it up separately to our XUL button handler, and then decide if if closes the page or goes back (and label it accordingly).

5) The text is strictly the Firefox text (with s/&brandShortName;/Camino/g and s/'/’/g; we should audit it and correct as warranted.

6) I included netError.css, so we seem to have picked up some of the styles that we used to use on the certificate error version of about:netError, but not most of them.  We'll need some major CSS love, either in our forked netError.css or in additional certError.css files (note that Firefox includes both base styles in content/aboutCertError.css and additional styles in skin/aboutCertError.css).

7) The page seems to pick up the right site icon; I don't know if that's by accident, or if it sends the same load flags we were watching for before.

8) I told hg to diff the files in a sane order, but it insisted on diffing them in some random order anyway :/
Assignee: nobody → alqahira
Status: NEW → ASSIGNED
Attachment #465542 - Flags: feedback?
Attached image A case of the uglies — 
You can see we get the general page style, but the styles inside the box are entirely wrong and very bad (compare to a nightly/2.0.x).
Just for reference, here's a diff of our xhtml file against the Firefox 3.6 copy in hg.mozilla.org/releases/mozilla-1.9.2:browser/components/certerror/content/aboutCertError.xhtml
(In reply to comment #10)
> 3) The "Technical Details" are being shown by default; I'm not sure why yet.

This is controlled by CSS; we'll have to port at least this bit of CSS from the Firefox sheets.

> 4) The "Get me out of here!" button doesn't work; I assume we have to wire it
> up separately to our XUL button handler, and then decide if if closes the page
> or goes back (and label it accordingly).

+
+  if ([elementIdentifier isEqualToString:@"getMeOutOfHereButton"]) {
+    [self runAwayFromBlockedSite:self];
+  }

in BrowserWrapper's performCommandForXULElementWithID will let us close the page; we'll have to do something different if we want to go back/home (like safebrowsing did originally) or something else.

> 6) I included netError.css, so we seem to have picked up some of the styles
> that we used to use on the certificate error version of about:netError, but not
> most of them.  We'll need some major CSS love

philippe said he will work on this. :-)
Last bugspam of the night here, I promise.

I'd like to get this in for a1 if possible, since it's another large batch of strings, and landing for a1 will let l10n get another big chunk out of the way (vs. landing right after a1, when this would otherwise be ready, which would provide no benefit at all to l10n).
Flags: camino2.1a1?
Summary: Improve content of certificate failure overlay page → Improve content of certificate failure overlay page (and allow localization)
Attached patch neterror.css patch, wip v 0.2 (obsolete) — — Splinter Review 
This brings back the overall look and feel of the overlay

* #introContent replaces #errorShortDesc for this particular overlay, but #errorShortDesc is still used over other neterror overlays (and the malware overlays). I needed to reshuffle things a little to please everybody.

* I've used a glyph in generated content for the toggle arrow. The color is the same as list markers on other pages and the old overlay. Advantage of a glyph: it scales with the rest of the text.

* The last warning box (#expertContent div) uses the same display as the old overlay. I moved it a bit to the left to keep the text aligned correctly on the left - and adjusted the margin-right to match.

--

* Some rule sets (and/or selectors) can almost certainly be removed: all the #securityOverride** ones are obsolete I think. I'll remove them in the next iteration of this patch, unless someone finds any use for them.


--------------
I had some problem building with Smokey's patch (v 0.5). Patch didn't copy/rename the two SafeBrowsingAboutModule files with the predictable results. Manually copying / moving/ renaming the 2 files solved it.

Otherwise, the new overlay works well on the various sites I tried.
Attachment #465600 - Flags: review?
(In reply to comment #15)
> I had some problem building with Smokey's patch (v 0.5). Patch didn't
> copy/rename the two SafeBrowsingAboutModule files with the predictable results.
> Manually copying / moving/ renaming the 2 files solved it.

I think you have to use 'hg import --no-commit /path/to/patch' (possibly also with '--force' if you have local changes in other files) rather than 'patch' here because of the file copies.
Comment on attachment 465600 [details] [diff] [review]
neterror.css patch, wip v 0.2

(In reply to comment #15)
> * I've used a glyph in generated content for the toggle arrow. The color is the
> same as list markers on other pages and the old overlay. Advantage of a glyph:
> it scales with the rest of the text.

That works marvellously :D  Is using the actual glyph safe?  I.e., why do we use escapes in the cbo stylesheet where we do this for quotes; was that just for old Geckos/other browsers?  Obviously using the actual glyph is much more developer-friendly :)

> * Some rule sets (and/or selectors) can almost certainly be removed: all the
> #securityOverride** ones are obsolete I think. I'll remove them in the next
> iteration of this patch, unless someone finds any use for them.

Originally we wanted to keep all of the rulesets/selectors from the Core/Firefox versions of the file to make comparison/backporting easier, but I think that ship has long since sailed, and backporting hasn't been much of a problem as far as I can tell.

So right now let's remove the #securityOverride** ones that have been obsoleted by this new, separate error page, but then file a follow-up bug for cleaning up other unused rulesets/selectors in this file.

(The only argument for not removing them is if someone were to un-set the pref that selects about:certerror as the certificate error page handler, they'd fall back to about:netError, which would then be missing the requisite styles.  I don't think that's a likely event, though; Stuart, do you agree?)

The only other thing I would like to do now (i.e., in the patch for this bug, as opposed to the future cleanup bug) is cleaning up the comment at the top of the file, which is really dated.

I'd say something like:

/*
 *  This file defines the styles of the network, blocked site
 *  (safe browsing), and certificate error pages.  See 
 *  netError.xhtml (in docshell), blockedSite.xhtml, and 
 *  certError.xhtml. 
 */

So r=ardissone with the aforementioned changes and the one question answered, and thanks!
Attachment #465600 - Flags: review? → review+
(In reply to comment #18)
> Comment on attachment 465600 [details] [diff] [review]
> neterror.css patch, wip v 0.2
> 
 
> That works marvellously :D 
:-)

> Is using the actual glyph safe?  I.e., why do we
> use escapes in the cbo stylesheet where we do this for quotes; was that just
> for old Geckos/other browsers?
I never had a problem with this in Gecko browsers. Safari is another story - although it is fixed since, iirc, Safari 3.1.
CBO might be using the escaped characters for backwards compatibility reasons.

> (The only argument for not removing them is if someone were to un-set the pref
> that selects about:certerror as the certificate error page handler, they'd fall
> back to about:netError, which would then be missing the requisite styles.  I
> don't think that's a likely event, though; Stuart, do you agree?)
Pending Stuarts answer, I'll spin another patch later today, including an updated comment.

And I've filed bug 587238 for further clean up of neterror.css
Attached patch neterror.css patch - v0.3a (obsolete) — — Splinter Review 
Same as the previous one, but we the 'securityOverride***' selectors removed, and the comment at top of the page.

I also removed the following comment, as it is not really relevant anymore.
/* Ensure all text is left-aligned to the same position - bug 451131 comment 9 */

Stuart, could you give some feedback on this removal ?

From comment 18
> (The only argument for not removing them is if someone were to un-set the pref
> that selects about:certerror as the certificate error page handler, they'd fall
> back to about:netError, which would then be missing the requisite styles.  I
> don't think that's a likely event, though; Stuart, do you agree?)
Attachment #465959 - Flags: feedback?(stuart.morgan+bugzilla)
(As a follow-up to this bug, we should fix bug 575670 by adding a button in the technical details section that, when clicked, calls our code to show the cert window for inspection.)
Blocks: 575670
I've gone a couple of rounds with the strings.  This is a diff of my rewording of the strings against the strings used by Firefox (as transformed for Camino in attachment 465542 [details] [diff] [review]).  Interestingly, I found myself moving towards most of the wording suggestions made by Jesse and by Eddy Nigg in bug 431826, and the objections raised by Sam there ;-)

A couple of birds-eye notes: 

A) I zapped most of the contractions, since they were a primary contributor to the language sounding too informal (and we tend to use contractions more sparingly).

B) In the existing Firefox strings, there's a problem with uses of "trust" being ambiguous, in that sometimes it's used to mean the Cert/CA system's (technical) definition of trust, and sometimes it's used to mean the common notion of trust.  It's so bad that in one case, I had to guess which one the Firefox people intended, because the sentence could be interpreted with either meaning.  I tried to break that confusion up by eliminating both usages where possible.

C) The whole purpose of these error pages is to indicate that, because of either technical errors on the site's part, or because of an active attack, Camino can't tell whether or not it's really connecting (securely) to the site you've asked it to connect to.  Bringing "trust" in to that picture continues to muddy the waters. 

Specifics:

1) I couldn't come up with a good (more accurate) phrase to use as the title, so this still says "Untrusted Connection".  I'd like to use something like "The identity of this site can't be verified" for the in-page title <H1>, but I don't have a corresponding idea for the short-form <title>.

2) For the intro, I replaced "connection is secure" with "can't be sure this is the site you want".  I continued to focus on the idea we can't tell if this is actually the site, because the cert isn't verified by a third party Camino trusts.  I used "CA" there to mirror the language we added in bug 522780 for untrusted certs, but we may just want to use "source" here instead, since the error page should be a little bit less technical than the exceptions sheet.

3) I have a bit of an issue with CertErrorAdviceText, because it only makes a suggestion for sites the user has previously visited.  That could imply (by omission) that error-producing sites the user has not visited are safe.  I'm not sure how to handle that.  Maybe generalize the text, and then add "this is especially true if you've connected before" at the end, for emphasis on that case? E.g.

"This error could mean that someone is trying to impersonate the site, and you should not continue.  This is especially true if you usually connect to this site without problems."

4) Firefox and Chrome both make the button go back-if-possible-else-home; I think we should do that, too.  "Leave Site" seems like good button text for that; it sanely tells the user what will happen, but also doesn't say specific things we can't verify (e.g., since you won't always go back, and also since we don't really know that the previous site was actually "safe").

5) In CertErrorExpertPara1, I tried again to break the "two different definitions of 'trust' used adjacently" confusion by swapping the user's "trust" with "user thinks the site is trustworthy".  I suppose the other way of going about that would be to have "Camino accept the site's identification" and let the site be the thing the user trusts.  Or maybe we could have Camino accept the id and still have the user consider the site trustworthy, for only one "trust" of any sort there (though I do think that having Camino "trust" the site's unverified identification is the right idea to convey here).

6) For CertErrorExpertPara2, I really liked the ideas included in our current error page's Exception warning (connection you don't trust completely, not used to seeing this warning), but I also liked the idea in Firefox's language about "knowing a good reason why the site doesn't use identification that can be verified".  However, when I combined the two, i) it seems very long on the page, and ii) making the Firefox language fit the style of the other text means the Firefox language ends up with a double negative ("if you do not know that there is a good reason the site does not use").

So, I think this is better (even if "better" is only that the text is now less overly-informal), at least on the right track, but I'd like to get some other eyes and brains on them, particularly on the places where I was less-than-fully-happy with my end results.
Attachment #466216 - Flags: feedback?(stuart.morgan+bugzilla)
(In reply to comment #17)
> (In reply to comment #15)
> > I had some problem building with Smokey's patch (v 0.5). Patch didn't
> > copy/rename the two SafeBrowsingAboutModule files with the predictable results.
> > Manually copying / moving/ renaming the 2 files solved it.
> 
> I think you have to use 'hg import --no-commit /path/to/patch' (possibly also
> with '--force' if you have local changes in other files) rather than 'patch'
> here because of the file copies.

Documented this on http://wiki.caminobrowser.org/Development:Helpful_Mercurial_Commands#Applying_a_patch_with_renames.2C_copies.2C_or_binary_files  (I need to get that paged linked from the Coding, Reviewing, and possibly Committing pages, too.)
This is a complete patch of all my changes (including the strings awaiting feedback, and the go-back-if-possible-else-home implementation), just so the bug has a complete backup of what I have done and for my own patched-files-tracking-sanity.
Attachment #465542 - Attachment is obsolete: true
Attachment #465542 - Flags: feedback?
(In reply to comment #24)
> Created attachment 467281 [details] [diff] [review]
> Implements about:certerror for Camino (v0.9)

You left the 2 Fx stylesheet links in it. Intentional ?
(In reply to comment #25)
> You left the 2 Fx stylesheet links in it. Intentional ?

No, that was me forgetting to remove them after we had talked :(  Thanks for catching that!  v0.9 also had the original strings, too, not the ones from attachment 466216 [details] [diff] [review].  0.9.1 has both of those fixed, which is hopefully the last of my diffing mistakes on this bug ;-)
Attachment #467281 - Attachment is obsolete: true
(In reply to comment #19)
> > (The only argument for not removing them is if someone were to un-set the pref
> > that selects about:certerror as the certificate error page handler, they'd fall
> > back to about:netError, which would then be missing the requisite styles.  I
> > don't think that's a likely event, though; Stuart, do you agree?)
> Pending Stuarts answer, I'll spin another patch later today, including an
> updated comment.

Yeah, I don't care if that case is ugly.
Comment on attachment 466216 [details] [diff] [review]
Possible strings, as diff against Caminified Firefox strings

These changes all look good to me!
Attachment #466216 - Flags: feedback?(stuart.morgan+bugzilla) → feedback+
Comment on attachment 465959 [details] [diff] [review]
neterror.css patch - v0.3a

+div[collapsed] > p,
+div[collapsed] > div {
+  display: none;
+}

Why not just display:none on the div?

Looks good though, other than the tab than snuck into one of the rules.
Attachment #465959 - Flags: feedback?(stuart.morgan+bugzilla) → feedback+
(In reply to comment #29)
> Comment on attachment 465959 [details] [diff] [review]
> neterror.css patch - v0.3a
> 
> +div[collapsed] > p,
> +div[collapsed] > div {
> +  display: none;
> +}
> 
> Why not just display:none on the div?

Because the h2 is a child of the div ;-)
 
> Looks good though, other than the tab than snuck into one of the rules.

I'll hunt for that one.
updated neterror.css patch per feedback comments
Attachment #465600 - Attachment is obsolete: true
Attachment #465959 - Attachment is obsolete: true
Attachment #469285 - Flags: review?
Comment on attachment 469285 [details] [diff] [review]
neterror.css patch v0.4

r=ardissone, then, since I've already looked at this ;)
Attachment #469285 - Flags: review? → review+
OK, I think this is ready now.

It's almost entirely copy+paste+find+replace of code murph wrote for safebrowsing (aboutModule, button behavior from the original safebrowsing button behavior) or of xhtml+js people wrote for Firefox (certError.xhtml).

In that regard, note that CHCertErrorAboutModule.{h|mm} were generated from 'hg copy', so the patch shows only the changed code (the results of the "find/replace").
Attachment #467478 - Attachment is obsolete: true
Attachment #469743 - Flags: superreview?(stuart.morgan+bugzilla)
Attachment #469285 - Flags: superreview?(stuart.morgan+bugzilla)
Comment on attachment 469285 [details] [diff] [review]
neterror.css patch v0.4

sr=smorgan
Attachment #469285 - Flags: superreview?(stuart.morgan+bugzilla) → superreview+
Comment on attachment 469743 [details] [diff] [review]
Implements about:certerror for Camino v1.0

sr=smorgan. We should look into factoring a common subclass out of these, but there's not enough code being duplicated here that it's a blocker for proceeding.
Attachment #469743 - Flags: superreview?(stuart.morgan+bugzilla) → superreview+
http://hg.mozilla.org/camino/rev/2d667f1fba6f (me)
http://hg.mozilla.org/camino/rev/7e5b4c5b9ad6 (philippe)

(In reply to comment #35)
> We should look into factoring a common subclass out of these, but
> there's not enough code being duplicated here that it's a blocker for
> proceeding.

Filed bug 593656 for that.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Flags: camino2.1a1? → camino2.1a1+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: