404726 - Addon compatibility check gives many alerts

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect, P3)

Description

[Jeremy Glassenberg]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b1) Gecko/2007110904 Firefox/3.0b1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b1) Gecko/2007110904 Firefox/3.0b1

When installing Firefox 3 B1, I encountered dozens of alerts during the addon compatibility test stage.  Two alerts are displayed repeatedly, with text provided below:

addons.mozilla.org:443 uses an invalid security certificate

The certificate is not trusted because it is self signed
The certificate is only valid for web.auto.configed.certificate.

(Error code: sec_error_untrusted_user)

------------------------------------

addons.mozilla.org:443 uses an invalid security certificate

The certificate is not trusted because it is self signed
The certificate is only valid for web.auto.configed.certificate.

(Error code: sec_error_untrusted_user)

Reproducible: Didn't try

Steps to Reproduce:
1.Use computer with Firefox 2, and many installed add-ons (google toolbar for FF may be necessary)
2.Disconnect computer from the internet
3.Install Firefox Beta 3
4.Observe "Firefox update" "Checking for compatibility of add-ons"
Actual Results:  
Dozens of dialog boxes appear, must be closed for installation to continue

Expected Results:  
Compatibility test either identifies compatibility issues, or realizes that user is not connected to the internet and handles situation unobtrusively

This was my first install of Firefox 3 on this computer.  Firefox 2.0.0.9 was already installed.

Comment 1

[Jeremy Glassenberg]

Attachment: Screenshot of alert

Comment 2

[Jeremy Glassenberg]

Attachment: Screenshot of alert

Comment 4

[Jo Hermans]

The certificate for <https://addons.mozilla.org/en-US/firefox/> is correct (not self-signed), so it must be related to the unavailability of Internet. 

Comment 5

[Reed Loden [:reed]]

Note the "The certificate is only valid for web.auto.configed.certificate." text. My guess is that some type of proxy or something is intercepting the request and using an invalid SSL certificate, which Firefox won't accept.

Comment 6

[Dave Townsend [:mossop]]

I think a lack of internet would not give a certificate error would it? Maybe a proxy error or something?

Comment 7

[Dave Townsend [:mossop]]

Quite by chance I managed to reproduce this yesterday. It is happening in situations where a webrequest gets redirected somewhere else as is commonly the case when using a pay-for wifi service that you haven't signed into yet.

We really should fix this. Regardless of the error returned from background update checks we should not be interrupting the user with them.

Comment 8

[Jeremy Glassenberg]

Actually, I forgot to mention in the initial report that I was trying to connect to a public wi-fi at an airport when this issue came up (I'm new to reporting bugs here).  I did have my wifi card enabled, but did not have internet access.

This airport had free wi-fi, but still required that I sign in by checking off a user agreement.  However, after connecting, I could not get to the sign-in page (I was connected, but was not receiving data).  So it may be specific to installing and running while trying to connect to a pay-for wifi service.  

This is certainly a rare scenario, but an inconvenience for any user under such circumstances.

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

Johnath: is there something Mossop can listen to and just eat these alerts and try again later/next time?

Should we handle repeated fail any differently?

Comment 10

[Johnathan Nightingale [:johnath]]

I don't know the addons code at all, but can we just use an nsIBadCertListener to eat the errors quietly?  Do we already do this, and something else is going wrong?  Apologies if this approach has already been dispensed with...

As for what to do on repeated errors, I dunno.  You could try to make noise after N failed attempts, but I'm not sure what you'd expect users to DO with that information.  Feel less safe?  They can't really debug this, and if they're in a situation like Dave describes, airport wifi or something, it's very likely to fix itself whenever they get to internet which isn't (however non-maliciously) MITM'ng them.

Comment 11

[Dave Townsend [:mossop]]

I've yet to even dig into what's going on here, but I'm pretty sure this is a regression from bug 327181 which removed nsIBadCertListener and our use of it to handle certificate errors. It looks like I can just use nsIBadCertListener2 instead.

Comment 12

[Dave Townsend [:mossop]]

Attachment: Testcase extension

This extension includes an updateurl to an invalid https server (https://www.mozilla.org:80/). Every time it checks for an update (foreground or background) it throws up an ssl error.

Comment 13

[Dave Townsend [:mossop]]

So I've tested with nsIBadCertListener2 and it doesn't allow us to hide the error coming from the attached extension. It looks to me like there is no way to block the displaying of error messages from nsHandleSSLError, kaie, maybe you can shed some light, otherwise I'll be digging into it more next week.

Comment 14

[Kai Engert (:KaiE:)]

Dave, you don't say whether it works partially or not at all.

Did you get a callback into nsIBadCertListener2::NotifyCertProblem ?

If you did, did you return "true" in your implementation?

Comment 15

[Kai Engert (:KaiE:)]

How did you use nsIBadCertListener2 ?

Did you register your implementation object by setting nsISSLSocketControl::notificationCallbacks ?

Comment 16

[Dave Townsend [:mossop]]

Sorry kai,

(In reply to comment #14)
> Did you get a callback into nsIBadCertListener2::NotifyCertProblem ?

No, no callback

> If you did, did you return "true" in your implementation?

Yes the implementation returns true.

(In reply to comment #15)
> How did you use nsIBadCertListener2 ?
> 
> Did you register your implementation object by setting
> nsISSLSocketControl::notificationCallbacks ?

It is registered using channel.notificationCallbacks, this is just the same object that used to implement nsIBadCertListener before bug 327181:

http://mxr.mozilla.org/seamonkey/source/toolkit/mozapps/extensions/src/nsExtensionManager.js.in#6114

From inspection NotifyCertProblem is only called from nsNSSBadCertHandler, protecting the call to nsHandleInvalidCertError however I believe the error I am seeing in my testcase comes from nsHandleSSLError. It looks like what I need to do is enable external error reporting in order to get rid of all error dialogs, but that seems to involve the notificationCallbacks being a docshell.

Comment 17

[Kai Engert (:KaiE:)]

(In reply to comment #16)
> 
> From inspection NotifyCertProblem is only called from nsNSSBadCertHandler,
> protecting the call to nsHandleInvalidCertError
>
> however I believe the error I
> am seeing in my testcase comes from nsHandleSSLError. 

I don't think so. nsHandleSSLError will report a single error reason.

From the screenshot we can see you're getting two different error reasons, which I believe strongly indicates the dialog you see is produced by nsHandleInvalidCertError


> to do is enable external error reporting in order to get rid of all error
> dialogs, but that seems to involve the notificationCallbacks being a docshell.

I think we should avoid to go that path.

Comment 18

[Kai Engert (:KaiE:)]

Please look at my changes to badCertHandler.js:

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=badCertHandler.js&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-09-01&maxdate=&cvsroot=%2Fcvsroot

I think you must add nsIBadCertListener2 to the QI implementation.

I don't see where you implement notifyCertProblem, I believe you must do that in BadCertHandler.prototype ?

Comment 19

[Dave Townsend [:mossop]]

(In reply to comment #17)
> (In reply to comment #16)
> > 
> > From inspection NotifyCertProblem is only called from nsNSSBadCertHandler,
> > protecting the call to nsHandleInvalidCertError
> >
> > however I believe the error I
> > am seeing in my testcase comes from nsHandleSSLError. 
> 
> I don't think so. nsHandleSSLError will report a single error reason.
> 
> From the screenshot we can see you're getting two different error reasons,
> which I believe strongly indicates the dialog you see is produced by
> nsHandleInvalidCertError

The particular error from the testcase is:

SSL received a record that exceeded the maximum permissible length.
(Error code: ssl_error_rx_record_too_long)

(In reply to comment #18)
> I think you must add nsIBadCertListener2 to the QI implementation.
> 
> I don't see where you implement notifyCertProblem, I believe you must do that
> in BadCertHandler.prototype ?

The patch I am working on both adds nsIBadCertListener2 to the QI implementation and implements notifyCertProblem as just |return true;|

Comment 20

[Kai Engert (:KaiE:)]

(In reply to comment #19)
>
> The particular error from the testcase is:
> 
> SSL received a record that exceeded the maximum permissible length.
> (Error code: ssl_error_rx_record_too_long)

Ok, that means you're not getting a "bad cert error".

You're getting an SSL protocol error, and nsIBadCertListener2 is currently not designed to handle that.

(I suspect you wouldn't have gotten a callback using the old interface either.)

I suspect NSS does not call into nsNSSBadCertHandler at all, but rather your error originates at nsSSLThread::checkHandshake (a call stack would confirm that).


Do you really want to suppress SSL communication errors?

If you do, we should either add another function to nsIBadCertListener2, like notifySSLProtocolError, giving you a similar ability to suppress that.

Although this error isn't really a BadCertError, so if we're strict about names, this would mandate for an interface with a different name.

Comment 21

[Dave Townsend [:mossop]]

I think the goal is that background update checks (and I think this applies to livemarks, microsummaries, app updates and extension updates) should not give any kind of error to the user (except maybe an unobtrusive one through the error console for instance). So yeah I think we need a new notification method for it.

Comment 22

[Dave Townsend [:mossop]]

Attachment: security patch rev 1

This implements the security portion of the patch. It adds a new interface, nsISSLErrorListener that is notified when any SSL error not covered by the bad cert listener is encountered. As there returning true will suppress the display of the error.

Comment 23

[Dave Townsend [:mossop]]

Attachment: toolkit patch rev 1

This is the toolkit/browser portion of the patch to update all the background requests I could find to suppress error displaying. Will request review once the security part is reviewed.

Comment 24

[Dave Townsend [:mossop]]

Attachment: xpinstall patch rev 1

This is the xpinstall portion of the patch. Will request review once the security part is reviewed.

Comment 25

[Kai Engert (:KaiE:)]

Comment on attachment 292597 [details] [diff] [review]
security patch rev 1

>Index: security/manager/ssl/public/nsISSLErrorListener.idl
>===================================================================
>+
>+interface nsISSLStatus;

You don't need that.


>+        rv = proxy_sel->NotifySSLError(csi, err, hostWithPortString, 
>+                                       &suppressMessage);
>+        if (suppressMessage)
>+          return rv;

Unless you object, I would have expected this:

  if (NS_SUCCEEDED(rv) && suppressMessage)
    return NS_OK;


>@@ -1176,17 +1203,17 @@ nsHandleInvalidCertError(nsNSSSocketInfo *socketInfo,
>   
>   nsString formattedString;
>   rv = getInvalidCertErrorMessage(multipleCollectedErrors, errorCodeToReport,
>-+                                 errTrust, errMismatch, errExpired,
>+                                  errTrust, errMismatch, errExpired,
>                                   hostU, hostWithPortU, port, 

Oops, thanks for finding this mistake.


r=kengert with the proposed changes.

Comment 26

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

Comment on attachment 292598 [details] [diff] [review]
toolkit patch rev 1

Kai, is there a valid reason why all the original nsIBadCertListener implementations in these files were removed by your patch for bug 327181, instead of being replaced with nsIBadCertListener2 implementations?

>Index: toolkit/components/url-classifier/src/nsUrlClassifierStreamUpdater.cpp

>+TableUpdateListener::GetInterface(const nsIID & eventSinkIID, void* *_retval)
>+{
>+    return QueryInterface(eventSinkIID, (void**)_retval);
>+}

That cast is unneeded, right?

>Index: toolkit/mozapps/shared/src/badCertHandler.js

>+  // Suppress any certificate errors
>+  notifyCertProblem: function(socketInfo, status, targetSite) {
>+    dump("notifyCertProblem\n");
>+    return true;
>+  },
>+
>+  // Suppress any ssl errors
>+  notifySSLError: function(socketInfo, error, targetSite) {
>+    dump("notifySSLError\n");
>+    return true;
>+  },

Don't forget to remove these dump()s before checking in.

I've tested the two search related changes (suggestions and engine loading) with handshake errors and invalid certs. Be sure to test all the other affected users before landing.

Comment 27

[Dave Townsend [:mossop]]

Attachment: security patch for checkin [checked in]

This addresses Kia's comments

Comment 28

[Dave Townsend [:mossop]]

Attachment: toolkit patch rev 2

This is an updated patch after further testing and understanding of exactly where the prompts are coming through. This is teh same as the previous patch except drops the changes for microsummaries, search service, urlclassifierdb and livemarks. It adds a change for the safebrowsing lookup (this is the per page lookup rather than the background db update).

The reason that some requests need fixing in this way and some don't is down to how the notification callbacks works for a http channel. The alerts are displayed using any nsIPrompt implementation available from the callbacks, no implementation means no display at all. The callbacks end up being an aggregation of the notification callbacks set on the channel and the notification callbacks from the channel's loadgroup.

Most of the places where the data is loaded from a bare channel there is no load group set. For the xmlhttprequest case though the loadgroup is set to that of the current page, which makes it's notification callbacks the docshell, which has an nsIPrompt implementation. So you have to do something for any requests made by an xhr.

Comment 29

[Dave Townsend [:mossop]]

Comment on attachment 293562 [details] [diff] [review]
toolkit patch rev 2

Per discussion on IRC will re-add the other cases back in and resubmit this.

Comment 30

[Dave Townsend [:mossop]]

Attachment: toolkit patch rev 3

Hopefully last round. This is just patch 1 but with the added change to xml-fetcher.js and making TableUpdateListener have a threadsafe QI.

Comment 31

[Kai Engert (:KaiE:)]

(In reply to comment #26)
> Kai, is there a valid reason why all the original nsIBadCertListener
> implementations in these files were removed by your patch for bug 327181,
> instead of being replaced with nsIBadCertListener2 implementations?

The old interface gave you a chance to "notify and override".
The new interface no longer offers that choice, by design.

Most implementations of the old interface either did nothing, or presented a UI to ask the user. Neither is necessary any more.

The new interface has a different intention, allowing one to supress unwanted error UI.

Comment 32

[Dave Townsend [:mossop]]

Attachment: toolkit patch rev 4 [checked in]

This is the same as patch 3 except for changes to the nsUrlClassifierStreamUpdater which has since changed on trunk.

Comment 33

[Dave Townsend [:mossop]]

Checked in the security and toolkit portions of this to avoid more bit-rotting while waiting on the xpinstall review.

Checking in browser/components/microsummaries/src/nsMicrosummaryService.js;
/cvsroot/mozilla/browser/components/microsummaries/src/nsMicrosummaryService.js,v  <--  nsMicrosummaryService.js
new revision: 1.82; previous revision: 1.81
done
Checking in browser/components/search/nsSearchService.js;
/cvsroot/mozilla/browser/components/search/nsSearchService.js,v  <--  nsSearchService.js
new revision: 1.106; previous revision: 1.105
done
Checking in browser/components/search/nsSearchSuggestions.js;
/cvsroot/mozilla/browser/components/search/nsSearchSuggestions.js,v  <--  nsSearchSuggestions.js
new revision: 1.18; previous revision: 1.17
done
Checking in security/manager/ssl/public/Makefile.in;
/cvsroot/mozilla/security/manager/ssl/public/Makefile.in,v  <--  Makefile.in
new revision: 1.39; previous revision: 1.38
done
RCS file: /cvsroot/mozilla/security/manager/ssl/public/nsISSLErrorListener.idl,v
done
Checking in security/manager/ssl/public/nsISSLErrorListener.idl;
/cvsroot/mozilla/security/manager/ssl/public/nsISSLErrorListener.idl,v  <--  nsISSLErrorListener.idl
initial revision: 1.1
done
Checking in security/manager/ssl/src/nsNSSIOLayer.cpp;
/cvsroot/mozilla/security/manager/ssl/src/nsNSSIOLayer.cpp,v  <--  nsNSSIOLayer.cpp
new revision: 1.148; previous revision: 1.147
done
Checking in toolkit/components/places/src/nsLivemarkService.js;
/cvsroot/mozilla/toolkit/components/places/src/nsLivemarkService.js,v  <--  nsLivemarkService.js
new revision: 1.34; previous revision: 1.33
done
Checking in toolkit/components/url-classifier/content/xml-fetcher.js;
/cvsroot/mozilla/toolkit/components/url-classifier/content/xml-fetcher.js,v  <--  xml-fetcher.js
new revision: 1.5; previous revision: 1.4
done
Checking in toolkit/components/url-classifier/src/nsUrlClassifierStreamUpdater.cpp;
/cvsroot/mozilla/toolkit/components/url-classifier/src/nsUrlClassifierStreamUpdater.cpp,v  <--  nsUrlClassifierStreamUpdater.cpp
new revision: 1.14; previous revision: 1.13
done
Checking in toolkit/components/url-classifier/src/nsUrlClassifierStreamUpdater.h;
/cvsroot/mozilla/toolkit/components/url-classifier/src/nsUrlClassifierStreamUpdater.h,v  <--  nsUrlClassifierStreamUpdater.h
new revision: 1.10; previous revision: 1.9
done
Checking in toolkit/mozapps/shared/src/badCertHandler.js;
/cvsroot/mozilla/toolkit/mozapps/shared/src/badCertHandler.js,v  <--  badCertHandler.js
new revision: 1.5; previous revision: 1.4
done

Comment 34

[Daniel Veditz [:dveditz]]

Comment on attachment 292599 [details] [diff] [review]
xpinstall patch rev 1

>+NS_IMPL_THREADSAFE_ADDREF(nsXPInstallManager)
>+NS_IMPL_THREADSAFE_RELEASE(nsXPInstallManager)

If you're getting rid of the install thread (in other bugs) these probably don't have to be threadsafe anymore.

r=dveditz

Comment 35

[Daniel Veditz [:dveditz]]

Comment on attachment 292599 [details] [diff] [review]
xpinstall patch rev 1

sr=dveditz too

Comment 36

[Dave Townsend [:mossop]]

Attachment: xpinstall patch for checkin [checked in]

This is updated to trunk and removed the threadsafe QI. Tested this in a debug build to double check there were no assertions. Carrying over r+sr.

Comment 37

[Dave Townsend [:mossop]]

Final part checked in.

Checking in xpinstall/src/nsXPInstallManager.cpp;
/cvsroot/mozilla/xpinstall/src/nsXPInstallManager.cpp,v  <--  nsXPInstallManager.cpp
new revision: 1.159; previous revision: 1.158
done
Checking in xpinstall/src/nsXPInstallManager.h;
/cvsroot/mozilla/xpinstall/src/nsXPInstallManager.h,v  <--  nsXPInstallManager.h
new revision: 1.47; previous revision: 1.46
done

Comment 38

[Ria Klaassen (not reading all bugmail)]

Builds don't start anymore.

Comment 39

[Dave Townsend [:mossop]]

(In reply to comment #38)
> Builds don't start anymore.

Tests appear to have run ok on tinderbox and it is fine on my machine. Is this definately from this checkin? What build are you using?