469848 - Add bad certificate exceptions without showing a separate UI

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Mark Finkle (:mfinkle) (use needinfo?)]

Even running full screen, the dialog is too large to see the buttons at the bottom.

We could use an overlay to tweak the content and/or style of the dialog

Comment 2

[Aakash Desai [:aakashd]]

This is a duplicate of bug 436076

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

Not a dupe of bug 436076

Comment 4

[Fabrice Desré]

Just to be sure what's needed to do :

When I go to https://mozilla.org, I first get a spontaneous popup dialog (#1) titled "Secure Connection Failed" and that displays just the same information as the web page, and has two buttons : "View Certificate" and "Cancel". Do we need this dialog at all ?

Choosing "Or you can add an exception..." and "Add Exception...", the dialog (#2) is "Add Security Exception", and the "View..." button opens the certificate details dialog which is way too big (#3).

On fennec would this be acceptable :
- get rid of #1
- #2 : rework to blend in fennec UI (eg. using our own dialogs)
- #3 rework or suppress ? At least the "Details" tab doesn't seem very usable on a mobile device.

Comment 5

[Fabrice Desré]

Attachment: Dialog #1 (certerror.xul)

Comment 6

[Fabrice Desré]

Attachment: Dialog #2 (exceptionDialog.xul)

Comment 7

[Fabrice Desré]

Attachment: Dialog #3 (certViewer.xul)

Comment 8

[Fabrice Desré]

Attachment: Dialog #3 (certViewer.xul) details tab

Comment 9

[Fabrice Desré]

All previous screenshots have been captured after opening https://mozilla.org/

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

* Dialog #1, we'd like to remove from Fennec
* Dialog #2, I'd like to discuss whether we need it, or can we just add the exception without a UI

If we need Dialog #2, we'll need to discuss the need for Dialog (#3 & #4)

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

We removed Dialog #1 with bug 515188

We decided to remove Dialog #2 as well and we can use this bug to do it. The plan now is to add a third button to the webcontent page to add a permanent exception.

Adding an exception, temporary or permanent, would silently add the exception. No other UI would be displayed.

We need to determine the final text for the buttons. I believe the current thinking is:
[Get me out of here] [Allow once] [Always allow]

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Changing the buttons here:
https://people.mozilla.com/~gavin/fennec/ssl5.png

Comment 13

[Madhava Enros [:madhava]]

Attachment: proposed buttons in context

It would also be a good idea to use (as the starting point) the improved version of the exception screen now in Firefox.

See attachment for the new proposed buttons in context.

Comment 14

[Madhava Enros [:madhava]]

Revised text for the buttons:

"View site" and "Add permanent exception"

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: patch

This patch creates a copy of the aboutCertError system from Firefox in Fennec. The aboutCertError.xhtml has an additional button and new label in the DTD file.

This patch does not silently add the temporary or permanent exception. I'll add that in a followup patch.

Comment 16

[Axel Hecht [:Pike]]

Comment on attachment 399549 [details] [diff] [review]
patch

I'd like to have a diff between the netError.xhtml and the certError.xhtml page to check the l10n-impact changes here more easily.

Technical bug I found, getURIFlags doesn't have a return anymore.

The comment in mobile.js doesn't match the pref, about:neterror vs certerror.

Comment 17

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

Comment on attachment 399549 [details] [diff] [review]
patch

>diff --git a/components/AboutRedirector.js b/components/AboutRedirector.js

>+  certerror: {
>+    uri: "chrome://browser/content/aboutCertError.xhtml",
>+    privileged: true
>   }

Seems like this page doesn't actually need to be privileged, so try removing this?

>   getURIFlags: function(aURI) {
>-    return Ci.nsIAboutModule.ALLOW_SCRIPT;
>+    let flags = Ci.nsIAboutModule.ALLOW_SCRIPT;
>+    if (this._getModuleInfo(aURI).privileged)
>+      flags |= Ci.nsIAboutModule.URI_SAFE_FOR_UNTRUSTED_CONTENT;
>   },

Don't think we need this change, as discussed on IRC.

>diff --git a/themes/hildon/aboutCertError.css b/themes/hildon/aboutCertError.css
>diff --git a/themes/wince/aboutCertError.css b/themes/wince/aboutCertError.css

>+  background : url("chrome://browser/skin/section_expanded.png") left 0 no-repeat;

>+  background-image: url("chrome://browser/skin/section_collapsed.png");

Need a bug on file for these, I guess?

r=me with that.

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #16)
> (From update of attachment 399549 [details] [diff] [review])
> I'd like to have a diff between the netError.xhtml and the certError.xhtml page
> to check the l10n-impact changes here more easily.


Here is a diff between Firefox'x aboutCertError.xhtml and the Fennec aboutCertError.xhmtl:

--- /home/mfinkle/Source/mozilla-trunk/mozilla/mobile/chrome/content/aboutCertError.xhtml 
+++ /home/mfinkle/Source/mozilla-trunk/mozilla/browser/components/certerror/content/aboutCertError.xhtml 
@@ -58,7 +58,7 @@
   <head>
     <title>&certerror.pagetitle;</title>
     <link rel="stylesheet" href="chrome://browser/skin/aboutCertError.css" type="text/css" media="all" />
-    <link rel="stylesheet" href="chrome://browser/content/aboutCertError.css" type="text/css" media="all" />
+    <link rel="stylesheet" href="chrome://browser/content/certerror/aboutCertError.css" type="text/css" media="all" />
     <!-- This page currently uses the same favicon as neterror.xhtml.
          If the location of the favicon is changed for both pages, the
          FAVICON_ERRORPAGE_URL symbol in toolkit/components/places/src/nsFaviconService.h
@@ -262,8 +262,7 @@
           <div>
             <p>&certerror.expert.content;</p>
             <p>&certerror.expert.contentPara2;</p>
-            <xul:button xmlns:xul='http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul' id='temporaryExceptionButton' label='&certerror.addTemporaryException.label;'/>
-            <xul:button xmlns:xul='http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul' id='permanentExceptionButton' label='&certerror.addPermanentException.label;'/>
+            <xul:button xmlns:xul='http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul' id='exceptionDialogButton' label='&certerror.addException.label;'/>
           </div>
         </div>
       </div>
 
> Technical bug I found, getURIFlags doesn't have a return anymore.

Reverted the method

> The comment in mobile.js doesn't match the pref, about:neterror vs certerror.

No, the comment is right. Without the pref, neterror is used.

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #17)
> (From update of attachment 399549 [details] [diff] [review])
> >diff --git a/components/AboutRedirector.js b/components/AboutRedirector.js
> 
> >+  certerror: {
> >+    uri: "chrome://browser/content/aboutCertError.xhtml",
> >+    privileged: true
> >   }
> 
> Seems like this page doesn't actually need to be privileged, so try removing
> this?

I tried it, but we need it for the chrome:// css files. We could go contentaccessible=yes in the future

> >   getURIFlags: function(aURI) {
> >-    return Ci.nsIAboutModule.ALLOW_SCRIPT;
> >+    let flags = Ci.nsIAboutModule.ALLOW_SCRIPT;
> >+    if (this._getModuleInfo(aURI).privileged)
> >+      flags |= Ci.nsIAboutModule.URI_SAFE_FOR_UNTRUSTED_CONTENT;
> >   },
> 
> Don't think we need this change, as discussed on IRC.

Reverted

> >diff --git a/themes/hildon/aboutCertError.css b/themes/hildon/aboutCertError.css
> >diff --git a/themes/wince/aboutCertError.css b/themes/wince/aboutCertError.css
> 
> >+  background : url("chrome://browser/skin/section_expanded.png") left 0 no-repeat;
> 
> >+  background-image: url("chrome://browser/skin/section_collapsed.png");
> 
> Need a bug on file for these, I guess?

Oops. I just threw them into the patch

Comment 20

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed:
https://hg.mozilla.org/mobile-browser/rev/957598191057

this is only the cert error page part, needed for string freeze. we still need to add the code to silently add the temporary or permanent exception. leaving open for now.

Comment 22

[Aakash Desai [:aakashd]]

I really don't like these meta bugs as its really hard to file, track and then verify. If a patch has been pushed, can we create new bugs for the temp and perm exception patches?

Comment 23

[Steffen Imhof]

Attachment: Directly creates temporary and permanent SSL overrides after the corresponding buttons are pressed

This patch uses code from exceptionDialog.js to add the correct type of SSL exception after one of the buttons is pressed.

The functionality is in a new class in sslException.js which is used from the event handler in browser.js

Some stuff I'd especially like to hear your opinion on:

 - "browser.ssl_override_behavior" is used to change the behaviour of the exception dialog. I'm not sure if and how this applies to Fennec, currently the code is still commented out
 - I was not able to get the certificate to create the override from the connetion info available. So I did what the dialog was doing and fetched the certificate with a new XmlHttpRequest. Is there a better way to get the certificate?
 - The handling of the private browsing state to prevent permanent exceptions.

Comment 24

[Mark Finkle (:mfinkle) (use needinfo?)]

* Removed the commented code about "browser.ssl_override_behavior" pref. Ignoring it seems the best solution for now.
* XMLHttpRequest ssems to work well. Might be another way, but I don't know it off-hand. We can always make it better later.
* The PB check looks OK for now. Fennec currently doesn't support PB, so we get a message in the error console (on purpose), which we might decide to remove.

pushed:
https://hg.mozilla.org/mobile-browser/rev/d357a17b17c8

Thanks Steffen

Comment 25

[Aakash Desai [:aakashd]]

verified FIXED on builds:
Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2b2pre) Gecko/20091104 Firefox/3.6b2pre Fennec/1.0b5

and

Mozilla/5.0 (X11; U; Linux armv6l; Nokia N8xx; en-US; rv:1.9.3a1pre) Gecko/20091104 Firefox/3.7a1pre Fennec/1.0b5

Comment 26

[Martijn Wargers (dead)]

I'm reassigning this to myself for adding litmus testcases for this (aakashd discussed this on irc with blassey and ted on #mobile).

Comment 27

[Martijn Wargers (dead)]

Attachment: Screenshot of "Secure Connection Failed" page for https://mozilla.org