439062 - Wording in SSL error dialogs contain inappropriate HTML tags

Status: RESOLVED FIXED

(Core Graveyard :: Security: UI, defect, P2)

Description

[Kai Engert [:KaiE:]]

Originally reported in bug 433420, see attachment 320611 [details]

The work in bug 402210 introduced this bug as a regression (as I said in bug 402210 comment 43 and 44).

The solution used in bug 402210 comment 40 was wrong, because it was simply assumed that error pages would be the only consumers of those strings, which was a wrong assumption.

Comment 1

[Kai Engert [:KaiE:]]

I propose to undo http://bonsai-l10n.mozilla.org/cvsquery.cgi?branch=HEAD&branchtype=match&date=explicit&mindate=2008-04-30+13%3A44&maxdate=2008-04-30+13%3A46

and implement the search/replace approach to add the HTML tags in the error page scenario, only.

Comment 2

[Kai Engert [:KaiE:]]

Actually, the screenshot link in comment 0 is not helpful (does not show the html tags).

Comment 3

[Kai Engert [:KaiE:]]

Attachment: screen shot

Comment 4

[Eddy Nigg (StartCom)]

The screen shot is exactly what I meant in bug bug 439028, but the title of this bug is perhaps misleading. There shouldn't be any dialog window at all.

Comment 5

[Johnathan Nightingale [:johnath]]

(In reply to comment #4)
> The screen shot is exactly what I meant in bug bug 439028, but the title of
> this bug is perhaps misleading. There shouldn't be any dialog window at all.

Those are separate issues.  The fact that the dialog is popping up when it shouldn't is bug 431712 and that should indeed be fixed.  But there are times when we fall back on the dialogs because we don't have a content area for displaying error pages.  In those cases, Kai has filed this bug to cope with the fact that we show raw html in the error strings.

Comment 6

[Eddy Nigg (StartCom)]

(In reply to comment #5)
Correct. Confirm this as bug 431712

Comment 7

[Eddy Nigg (StartCom)]

(In reply to comment #5)
> But there are times
> when we fall back on the dialogs because we don't have a content area for
> displaying error pages.
> 

Why don't we open a new tab and focus on it when no content area exists? But I really doubt how Firefox should look like without any content area...

Comment 8

[Johnathan Nightingale [:johnath]]

(In reply to comment #7)
> (In reply to comment #5)
> > But there are times
> > when we fall back on the dialogs because we don't have a content area for
> > displaying error pages.
> > 
> Why don't we open a new tab and focus on it when no content area exists? But I
> really doubt how Firefox should look like without any content area...

In firefox, it would be pretty profoundly unusual and undesirable to have navigation attempts in one tab result in error pages in another tab, but all of this is sort of beside the point.

Typical firefox browsing sessions should not pop up the dialog, and as I say, there are bugs open about that behaviour.  But PSM is used in lots of places that aren't firefox, even in places that aren't web browsers, and this bug is about fixing the alert dialogs that are used in those cases.  

This bug is not about Firefox.

Comment 9

[Kai Engert [:KaiE:]]

Eddy, "no content area exists" includes scenarios like "the application is not firefox".

examples include:
- thundebird ldap entry fetching
- thunderbird mail send/receive
- chatzilla irc over ssl

Comment 10

[Eddy Nigg (StartCom)]

Thanks for clarifying. It wasn't clear to me at previous comments of yours and I thought there might be some ministerial reasons for not have a content area ;-) 

Comment 12

[Kai Engert [:KaiE:]]

Attachment: 1.9.0 branch hack patch v1: strip html

I think it would be a mess to change the strings on the 1.9.0 branch in all locales.

I propose this hack patch for 1.9.0 branch for Firefox 3.0.x releases.

It strips off the html tags when not showing the error externally.

Comment 13

[Kai Engert [:KaiE:]]

Attachment: trunk patch v2

I propose this patch for trunk.
Adding back a variation of the string that does not contain a link.
Use the plain string by default.
Introduce a mechanisms that allows the application to request html tags.

The latter can be achieved if the application uses an nsIBadCertListener2.
Gavin, Johnathan, with this patch we'd revert to the non-html version on trunk.
But I assume it will be trivial for you to implement nsIBadCertListener2 and get the html back.

I want to land this ASAP so thunderbird 3 gets the fix, whether they choose to go with 1.9.0 or 1.9.1

Comment 14

[Johnathan Nightingale [:johnath]]

I agree that we should fix this, indeed this is why I asked for review of the original patch for this issue, since I was worried, but unsure, if it would have this effect.

Having said that though, I'm not sure we CAN implement an nsIBadCertListener2 here, since the error page isn't requesting any strings, they're being fed in from PSM code, and since the error page itself is unprivileged.  Maybe I'm misunderstanding your approach here, though.  Where would you expect this listener to be implemented?

Comment 15

[Kai Engert [:KaiE:]]

PSM's I/O object (nsNSSSocketInfo) carries a context object, that can be supplied by the initiator and can be used to transport callbacks to PSM.

nsIBadCertListener2 is already being used for that.
For example, look at file
  xml-fetcher.js

It already implements notifyCertProblem, also look for the assignment to attribute notificationCallbacks.

I think we must already have code that provides such a context object and would allow PSM to query, whether the caller wants html tags in error messages.

I now realize this is the same challenge I faced when I looked for a way to decide whether or not to suppress PSM's error dialog, and when I had introduced that externalErrorReporting mode, obtained by looking for a docshell.

Comment 16

[timeless]

brother.

the content isn't html. if you're going to make it html, then just fix the content.

please ask me for a review before landing something. thanks.

we could probably just use "" as the marker pair as long as there's a localization note telling localizers that they may not add "" marks anywhere in the error message and must leave the "" marks around %s.

Comment 18

[Kai Engert [:KaiE:]]

Comment on attachment 325374 [details] [diff] [review]
1.9.0 branch hack patch v1: strip html

Gavin, do you agree to this approach for the stable branch?

Comment 19

[Kai Engert [:KaiE:]]

Johnathan, I think you have convinced me that we should strive for a simpler solution (for trunk).

I propose that PSM gives out the html version when we do external error reporting (have docshell), and use the plain text version otherwise.

Do you agree?

Comment 20

[Kai Engert [:KaiE:]]

Timeless, sorry but I don't understand your proposal?

Comment 21

[Robert Relyea]

Comment on attachment 325374 [details] [diff] [review]
1.9.0 branch hack patch v1: strip html

This gets rid of the HTML. 

bob

Comment 22

[Robert Relyea]

The review is only for the branch, not the trunk.

Comment 23

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 325374 [details] [diff] [review]
1.9.0 branch hack patch v1: strip html

+      nsString badString;
+      badString = formattedString;

should be:
  nsString badString(formattedString);

Wouldn't it be better if you didn't add the HTML tags in this case, instead of stripping them later?

Comment 24

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #23)
> Wouldn't it be better if you didn't add the HTML tags in this case, instead of
> stripping them later?

Though I guess it doesn't matter for a branch-only patch

Comment 25

[Kai Engert [:KaiE:]]

(In reply to comment #23)
> should be:
>   nsString badString(formattedString);

Thanks, will change it.

> Wouldn't it be better if you didn't add the HTML tags in this case, instead of
> stripping them later?

I'm not adding the html. Problem is, the locale string has the HTML tags hardcoded in it! So, on the branch, we must strip away the tag(s) in those places where they are not appropriate.

Comment 26

[Christian :Biesinger (don't email me, ping me on IRC)]

fwiw, I think this would be much better handled by not doing any formatting in PSM and instead letting the consumer create the string. anyway, that's certainly not a branch solution.

Comment 27

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 325374 [details] [diff] [review]
1.9.0 branch hack patch v1: strip html

This approach seems reasonable for the branch.

Comment 28

[Kai Engert [:KaiE:]]

Attachment: Trunk patch v3 [Checkin: Comment 33]

proposed trunk patch

Comment 29

[Kai Engert [:KaiE:]]

Comment on attachment 325374 [details] [diff] [review]
1.9.0 branch hack patch v1: strip html

requesting approval for the branch-only patch (1.9.0 for firefox 3)

Comment 30

[Robert Relyea]

Comment on attachment 335434 [details] [diff] [review]
Trunk patch v3
[Checkin: Comment 33]

r+

Comment 31

[Samuel Sidler (old account; do not CC)]

Comment on attachment 325374 [details] [diff] [review]
1.9.0 branch hack patch v1: strip html

We're going to live with this regression in Firefox 3. Fixing this on branch is a bit more risky than on trunk (for localization reasons) and the bug itself is a visible one, not a functional one.

Comment 32

[Kai Engert [:KaiE:]]

Samuel, I'm not sure why you mention localization reasons as a reason to reject the Firefox 3 patch.

The proposed patch for Firefox 3 does not change any strings, it's completely at the runtime level. The patch is simple, it removes the html tag(s) dynamically from the string, if the string is to be shown as plain text. Do you think this code is too risky?

Comment 33

[Serge Gautherie (:sgautherie)]

Comment on attachment 335434 [details] [diff] [review]
Trunk patch v3
[Checkin: Comment 33]

http://hg.mozilla.org/mozilla-central/rev/82522e7b9dd9

Comment 34

[Benjamin Smedberg]

This landed before 1.9.1 branched

Comment 35

[Johnathan Nightingale [:johnath]]

Based on comment 34 and inspection, marking fixed1.9.1

Comment 36

[u279076]

The attached test URL no longer throws the warning.  Is there another test site I can use to verify this?  Has this dialog been replaced with SSL error pages?