Closed Bug 404726 Opened 13 years ago Closed 13 years ago

Addon compatibility check gives many alerts

Categories

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

x86
Windows XP
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: jglassenberg, Assigned: mossop)

References

Details

Attachments

(6 files, 5 obsolete files)

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.
Attached image Screenshot of alert
Attached image Screenshot of alert
Duplicate of this bug: 404730
Component: Installer → Extension/Theme Manager
QA Contact: installer → extension.manager
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. 
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.
I think a lack of internet would not give a certificate error would it? Maybe a proxy error or something?
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking-firefox3?
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.
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?
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P3
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.
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.
Attached file 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.
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.
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?
How did you use nsIBadCertListener2 ?

Did you register your implementation object by setting nsISSLSocketControl::notificationCallbacks ?
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.
(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.
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 ?
(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;|
(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.
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.
Assignee: nobody → dtownsend
Attached patch security patch rev 1 (obsolete) — Splinter Review
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.
Attachment #292597 - Flags: review?
Attached patch toolkit patch rev 1 (obsolete) — Splinter Review
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.
Attached patch xpinstall patch rev 1 (obsolete) — Splinter Review
This is the xpinstall portion of the patch. Will request review once the security part is reviewed.
Status: NEW → ASSIGNED
Whiteboard: [has patch]
Attachment #292597 - Flags: review? → review?(kengert)
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.
Attachment #292597 - Flags: review?(kengert) → review+
Attachment #292598 - Flags: review?(gavin.sharp)
Attachment #292599 - Flags: review?(dveditz)
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.
Attachment #292598 - Flags: review?(gavin.sharp) → review+
Attachment #292599 - Attachment is obsolete: true
Attachment #292599 - Flags: review?(dveditz)
This addresses Kia's comments
Attachment #292597 - Attachment is obsolete: true
Attachment #293559 - Flags: review+
Attached patch toolkit patch rev 2 (obsolete) — Splinter Review
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.
Attachment #292598 - Attachment is obsolete: true
Attachment #293562 - Flags: review?(gavin.sharp)
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.
Attachment #293562 - Attachment is obsolete: true
Attachment #293562 - Flags: review?(gavin.sharp)
Attachment #292599 - Attachment is obsolete: false
Attachment #292599 - Flags: review?(dveditz)
Attached patch toolkit patch rev 3 (obsolete) — Splinter Review
Hopefully last round. This is just patch 1 but with the added change to xml-fetcher.js and making TableUpdateListener have a threadsafe QI.
Attachment #293647 - Flags: review?(gavin.sharp)
Attachment #293647 - Flags: review?(gavin.sharp) → review+
(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.
This is the same as patch 3 except for changes to the nsUrlClassifierStreamUpdater which has since changed on trunk.
Attachment #293647 - Attachment is obsolete: true
Attachment #297164 - Flags: review?(gavin.sharp)
Attachment #297164 - Flags: review?(gavin.sharp) → review+
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
Attachment #293559 - Attachment description: security patch for checkin → security patch for checkin [checked in]
Attachment #297164 - Attachment description: toolkit patch rev 4 → toolkit patch rev 4 [checked in]
Version: unspecified → Trunk
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
Attachment #292599 - Flags: review?(dveditz) → review+
Comment on attachment 292599 [details] [diff] [review]
xpinstall patch rev 1

sr=dveditz too
Attachment #292599 - Flags: superreview+
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.
Attachment #301651 - Flags: superreview+
Attachment #301651 - Flags: review+
Attachment #292599 - Attachment is obsolete: true
Attachment #301651 - Attachment description: xpinstall patch for checkin → xpinstall patch for checkin [checked in]
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
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Whiteboard: [has patch]
Builds don't start anymore.
(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?
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.