846734 - Cannot add certificate exception in certificate error page.

Status: VERIFIED FIXED

(Firefox OS Graveyard :: General, defect)

Description

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

We should implement the 'add certificate exception' buttons in certificate error page of b2g.

Comment 1

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch

Comment 2

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: adding certificate exception from certificate error page

Comment 3

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Comment on attachment 721019 [details] [diff] [review]
Patch: adding certificate exception from certificate error page

This is modified from /mobile/android/chrome/content/exceptions.js. The main difference is that this patch uses an async xhr to get the certificate error information.

Hi Fabrice, would you help to review this patch? Thanks.

Comment 4

[Jason Smith [:jsmith]]

We either need to fix in v1.1 or turn the feature off. https://bugzilla.mozilla.org/show_bug.cgi?id=850556#c6 reveals this is definitely exposed UI on v1.1 to users hitting untrusted sites due to cert errors and indicates this is "exposed broken UI."

Comment 5

[[:fabrice] Fabrice Desré]

Comment on attachment 721019 [details] [diff] [review]
Patch: adding certificate exception from certificate error page

Review of attachment 721019 [details] [diff] [review]:
-----------------------------------------------------------------

I think we should not have the BrowserElementChildPreload.js part at all. Since we already ship a custom version of aboutCertError.xhtml for b2g, let's add the needed script here instead.

::: dom/browser-element/BrowserElementChildPreload.js
@@ +447,5 @@
> +  _contentLoadedHandler: function(e) {
> +    let target = e.originalTarget;
> +    let targetDocShell = target.defaultView
> +          .QueryInterface(Ci.nsIInterfaceRequestor)
> +          .getInterface(Ci.nsIWebNavigation);

Nit: indentation is weird here. Align .QueryInterface with .defaultView

@@ +452,5 @@
> +    if (targetDocShell != docShell) {
> +      return;
> +    }
> +
> +    if (/^about:/.test(target.documentURI)) {

Why only test for about: and not about:certerror ?

@@ +479,5 @@
> +      let temp = errorDoc.getElementById("temporaryExceptionButton");
> +      if (target == temp || target == perm) {
> +        sendAsyncMsg("add-cert-exception", {
> +          url: errorDoc.location.href,
> +          isPerm: target == perm

nit: use a full word instead of isPerm (perm is often used for permission and not for permanent)

::: dom/browser-element/BrowserElementParent.jsm
@@ +72,5 @@
> + * The functionality itself is borrowed from exceptionDialog.js.
> + */
> +function SSLExceptions(aCallback, aUri, aWindow) {
> +  this._overrideService = Cc["@mozilla.org/security/certoverride;1"]
> +                          .getService(Ci.nsICertOverrideService);

Use a global lazy getter instead.

@@ +90,5 @@
> +  getInterface: function SSLE_getInterface(aIID) {
> +    return this.QueryInterface(aIID);
> +  },
> +
> +  QueryInterface: function SSLE_QueryInterface(aIID) {

Use XPCOMUtils.generateQI([Ci.nsIBadCertListener2]) instead

@@ +119,5 @@
> +    this._sslStatus = null;
> +
> +    let req = new this._window.XMLHttpRequest();
> +    try {
> +      if (this._uri) {

use an early return if (!this._uri) instead.

@@ +122,5 @@
> +    try {
> +      if (this._uri) {
> +        req.open("GET", this._uri.prePath, true);
> +        req.channel.notificationCallbacks = this;
> +        req.addEventListener("readystatechange", (function() {

I would rather use addEventListener("load") and addEventListener("error")

@@ +147,5 @@
> +   * Internal method to create an override.
> +   */
> +  _addOverride: function SSLE_addOverride() {
> +    let SSLStatus = this._sslStatus;
> +    let certificate = SSLStatus.serverCert;

No need fro this variable.

@@ +152,5 @@
> +    let uri = this._uri;
> +    let flags = 0;
> +
> +    if (SSLStatus.isUntrusted)
> +      flags |= this._overrideService.ERROR_UNTRUSTED;

Here and for other constants, use Ci.nsICertOverrideService.ERROR_XXX

@@ +690,5 @@
>      this._windowUtils.remoteFrameFullscreenReverted();
>    },
>  
> +  _addCertException: function(data) {
> +    let uri = Services.io.newURI(data.json.url, null, null);

Use data.data instead of data.json

Comment 6

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Fabrice Desré [:fabrice] from comment #5)
> I think we should not have the BrowserElementChildPreload.js part at all.
> Since we already ship a custom version of aboutCertError.xhtml for b2g,
> let's add the needed script here instead.

Error page is not able to send message to parent. It may need help of code in BrowserElementChild to accomplish this.

Comment 7

[Gervase Markham [:gerv]]

There aren't any UI mockups; does this patch make it significantly easier to add a cert exception in B2G than it is in Desktop Firefox? The title of the bug implies that it might.

When deciding how easy to make this process, there's a tradeoff between allowing people to access sites using untrusted CAs, and allowing people to shoot themselves in the foot and send their private info to a phisher. We went through several iterations to find the current balance for Firefox Desktop. In B2G, there is much less security UI (e.g. URL display, EV etc.) for them to use to spot when something is wrong, and so adding cert overrides should, if anything, be a more involved process than on Desktop, not less...

Gerv

Comment 8

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Gervase Markham [:gerv] from comment #7)
> There aren't any UI mockups; does this patch make it significantly easier to
> add a cert exception in B2G than it is in Desktop Firefox? The title of the
> bug implies that it might.
> 
> When deciding how easy to make this process, there's a tradeoff between
> allowing people to access sites using untrusted CAs, and allowing people to
> shoot themselves in the foot and send their private info to a phisher. We
> went through several iterations to find the current balance for Firefox
> Desktop. In B2G, there is much less security UI (e.g. URL display, EV etc.)
> for them to use to spot when something is wrong, and so adding cert
> overrides should, if anything, be a more involved process than on Desktop,
> not less...
> 
> Gerv

This bug is for the "add exception" in certificate error page. The error page is based on Android version, which also provides this feature. If we don't want to do this right now, I suggest that we should hide the buttons on the certificate error page.

Comment 9

[Jason Smith [:jsmith]]

leo+ more for a backout of bug 769178. This was unplanned feature for v1.1 added in that has unexpected extra work. Unless there's an explanation of why bug 769178 became a requirement for v1.1.

I don't have power to set flags, so someone else has to set it.

Comment 10

[Gervase Markham [:gerv]]

Why do we need to back out bug 769178? If there is an "Add Permanent Exception" button which doesn't work, and we don't yet have the resources to make it work, we should simply remove the button. Let's not kill our improved error messages because of this.

Gerv

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

Also see https://bugzilla.mozilla.org/show_bug.cgi?id=850556#c6

Comment 13

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: add certificate exception from certificate error page v2

Comment 14

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Comment on attachment 727516 [details] [diff] [review]
Patch: add certificate exception from certificate error page  v2

Hi Fabrice, In this patch the script are moved to b2g/ folder. Added ErrorPage.jsm and ErrorPage.js to handle event from error pages. Could you help to review this patch? Thanks.

Comment 15

[[:fabrice] Fabrice Desré]

Comment on attachment 727516 [details] [diff] [review]
Patch: add certificate exception from certificate error page  v2

Review of attachment 727516 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with comments addressed.

::: b2g/chrome/content/ErrorPage.js
@@ +38,5 @@
> +                               .getInterface(Ci.nsIWebNavigation);
> +    if (targetDocShell != docShell) {
> +      return;
> +    }
> +    

nit: trailing ws

@@ +46,5 @@
> +      let listener = function() {
> +        removeEventListener("click", errorPageEventHandler, true);
> +        removeEventListener("pagehide", listener, true);
> +      }.bind(this);
> +      

nit: trailing ws

::: b2g/components/ErrorPage.jsm
@@ +45,5 @@
> +  /**
> +   * To collect the SSL status we intercept the certificate error here
> +   * and store the status for later use.
> +   */
> +  notifyCertProblem: function SSLE_notifyCertProblem(socketInfo, sslStatus, targetHost) {

nit: aSocketInfo, aSslStatus, aTargetHost

@@ +63,5 @@
> +
> +    let req = new this._window.XMLHttpRequest();
> +    try {
> +      if (!this._uri) {
> +        return;

this check should be done before creating req.

@@ +76,5 @@
> +          }
> +        }
> +      }).bind(this);
> +      req.addEventListener("load", xhrHandler);
> +      req.addEventListener("error", xhrHandler);

Please remove the event listeners in the handler.

@@ +129,5 @@
> +  }
> +};
> +
> +let ErrorPage = {
> +  _addCertException: function(data) {

s/data/aMessage

@@ +147,5 @@
> +  },
> +
> +  init: function errorPageInit() {
> +    Services.obs.addObserver(this, 'in-process-browser-or-app-frame-shown', false);
> +    Services.obs.addObserver(this, 'remote-browser-frame-shown', false);

When do we remove these observers? If you can't do it, make them weak owners.

Comment 16

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

(In reply to Fabrice Desré [:fabrice] from comment #15)
> When do we remove these observers? If you can't do it, make them weak owners.

I just tried to make the observer service weak owner, but ErrorPage instance will soon be destroyed. As ErrorPage should live until the system down, can we keep the ownership strong?

Comment 17

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: add certificate exception from certificate error page. (for central)

Comment 18

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

Attachment: Patch: add certificate exception from certificate error page. (for b2g18)

Comment 19

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/55a8ef1cd22f

Comment 20

[Joe Drew (not getting mail)]

https://hg.mozilla.org/mozilla-central/rev/55a8ef1cd22f

Comment 22

[Patrick Wang (Chih-Kai Wang) (:kk1fff)]

uplift

https://hg.mozilla.org/releases/mozilla-b2g18/rev/879324d41588

Comment 23

[Jason Smith [:jsmith]]

https://moztrap.mozilla.org/manage/cases/?sortfield=created_on&sortdirection=desc&filter-tag=1687&pagenumber=1&pagesize=20

Comment 24

[Jason Smith [:jsmith]]

Did some sanity testing around this - marking as verified with some followup bugs.