Closed Bug 439062 Opened 12 years ago Closed 11 years ago

Wording in SSL error dialogs contain inappropriate HTML tags

Categories

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

defect

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9.1b2

People

(Reporter: KaiE, Assigned: KaiE)

References

()

Details

(Keywords: fixed1.9.1, regression)

Attachments

(3 files, 1 obsolete file)

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.
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.
Actually, the screenshot link in comment 0 is not helpful (does not show the html tags).
Version: 1.9.0 Branch → Trunk
Flags: wanted1.9.1?
Flags: wanted1.9.0.x?
Attached image screen shot
Depends on: 402210
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.
(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.
(In reply to comment #5)
Correct. Confirm this as bug 431712
(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 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.
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
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 ;-) 
Duplicate of this bug: 438692
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.
Attachment #325374 - Flags: review?(rrelyea)
Attachment #325374 - Flags: approval1.9?
Attached patch trunk patch v2 (obsolete) — Splinter Review
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
Assignee: johnath → kaie
Status: NEW → ASSIGNED
Attachment #325375 - Flags: review?(rrelyea)
Attachment #325375 - Attachment description: trunk patch → trunk patch v2
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?
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.

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.
Attachment #325374 - Flags: approval1.9?
Flags: blocking1.9.1?
Blocks: 439028
Duplicate of this bug: 444227
Blocks: 402210
No longer depends on: 402210
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?
Attachment #325374 - Flags: review?(gavin.sharp)
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?
Timeless, sorry but I don't understand your proposal?
Attachment #325375 - Attachment is obsolete: true
Attachment #325375 - Flags: review?(rrelyea)
Comment on attachment 325374 [details] [diff] [review]
1.9.0 branch hack patch v1: strip html

This gets rid of the HTML. 

bob
Attachment #325374 - Flags: review?(rrelyea) → review+
The review is only for the branch, not the trunk.
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?
(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

(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.
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 on attachment 325374 [details] [diff] [review]
1.9.0 branch hack patch v1: strip html

This approach seems reasonable for the branch.
Attachment #325374 - Flags: review?(gavin.sharp) → review+
proposed trunk patch
Attachment #335434 - Flags: review?(rrelyea)
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)
Attachment #325374 - Flags: approval1.9.0.3?
Flags: wanted1.9.0.x?
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.3?
Comment on attachment 335434 [details] [diff] [review]
Trunk patch v3
[Checkin: Comment 33]

r+
Attachment #335434 - Flags: review?(rrelyea) → review+
Flags: blocking1.9.0.4?
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.
Attachment #325374 - Flags: approval1.9.0.4? → approval1.9.0.4-
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 on attachment 335434 [details] [diff] [review]
Trunk patch v3
[Checkin: Comment 33]

http://hg.mozilla.org/mozilla-central/rev/82522e7b9dd9
Attachment #335434 - Attachment description: Trunk patch v3 → Trunk patch v3 [Checkin: Comment 33]
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b2
Flags: wanted1.9.0.x+
This landed before 1.9.1 branched
Flags: wanted1.9.1?
Flags: blocking1.9.1?
Flags: blocking1.9.1+
Priority: -- → P2
Based on comment 34 and inspection, marking fixed1.9.1
Keywords: fixed1.9.1
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?
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.