Addon compatibility check gives many alerts

RESOLVED FIXED

Status

()

Toolkit
Add-ons Manager
P3
normal
RESOLVED FIXED
10 years ago
9 years ago

People

(Reporter: Jeremy Glassenberg, Assigned: mossop)

Tracking

Trunk
x86
Windows XP
Points:
---
Bug Flags:
blocking1.9 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 5 obsolete attachments)

(Reporter)

Description

10 years ago
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.
(Reporter)

Comment 1

10 years ago
Created attachment 289633 [details]
Screenshot of alert
(Reporter)

Comment 2

10 years ago
Created attachment 289634 [details]
Screenshot of alert

Updated

10 years ago
Duplicate of this bug: 404730
(Assignee)

Updated

10 years ago
Component: Installer → Extension/Theme Manager
QA Contact: installer → extension.manager

Comment 4

10 years ago
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.
(Assignee)

Comment 6

10 years ago
I think a lack of internet would not give a certificate error would it? Maybe a proxy error or something?
(Assignee)

Comment 7

10 years ago
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?
(Reporter)

Comment 8

10 years ago
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.
(Assignee)

Comment 11

10 years ago
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.
(Assignee)

Comment 12

10 years ago
Created attachment 290939 [details]
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.
(Assignee)

Comment 13

10 years ago
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

10 years ago
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

10 years ago
How did you use nsIBadCertListener2 ?

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

Comment 16

10 years ago
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

10 years ago
(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

10 years ago
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 ?
(Assignee)

Comment 19

10 years ago
(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

10 years ago
(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.
(Assignee)

Comment 21

10 years ago
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)

Updated

10 years ago
Assignee: nobody → dtownsend
(Assignee)

Comment 22

10 years ago
Created attachment 292597 [details] [diff] [review]
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.
Attachment #292597 - Flags: review?
(Assignee)

Comment 23

10 years ago
Created attachment 292598 [details] [diff] [review]
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.
(Assignee)

Comment 24

10 years ago
Created attachment 292599 [details] [diff] [review]
xpinstall patch rev 1

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

Updated

10 years ago
Status: NEW → ASSIGNED
Whiteboard: [has patch]
(Assignee)

Updated

10 years ago
Attachment #292597 - Flags: review? → review?(kengert)

Comment 25

10 years ago
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+
(Assignee)

Updated

10 years ago
Attachment #292598 - Flags: review?(gavin.sharp)
(Assignee)

Updated

10 years ago
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+
(Assignee)

Updated

10 years ago
Attachment #292599 - Attachment is obsolete: true
Attachment #292599 - Flags: review?(dveditz)
(Assignee)

Comment 27

10 years ago
Created attachment 293559 [details] [diff] [review]
security patch for checkin [checked in]

This addresses Kia's comments
Attachment #292597 - Attachment is obsolete: true
Attachment #293559 - Flags: review+
(Assignee)

Comment 28

10 years ago
Created attachment 293562 [details] [diff] [review]
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.
Attachment #292598 - Attachment is obsolete: true
Attachment #293562 - Flags: review?(gavin.sharp)
(Assignee)

Comment 29

10 years ago
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)
(Assignee)

Updated

10 years ago
Attachment #292599 - Attachment is obsolete: false
Attachment #292599 - Flags: review?(dveditz)
(Assignee)

Comment 30

10 years ago
Created attachment 293647 [details] [diff] [review]
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.
Attachment #293647 - Flags: review?(gavin.sharp)
Attachment #293647 - Flags: review?(gavin.sharp) → review+

Comment 31

10 years ago
(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.
(Assignee)

Comment 32

10 years ago
Created attachment 297164 [details] [diff] [review]
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.
Attachment #293647 - Attachment is obsolete: true
Attachment #297164 - Flags: review?(gavin.sharp)
Attachment #297164 - Flags: review?(gavin.sharp) → review+
(Assignee)

Comment 33

10 years ago
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
(Assignee)

Updated

10 years ago
Attachment #293559 - Attachment description: security patch for checkin → security patch for checkin [checked in]
(Assignee)

Updated

10 years ago
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+
(Assignee)

Comment 36

10 years ago
Created attachment 301651 [details] [diff] [review]
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.
Attachment #301651 - Flags: superreview+
Attachment #301651 - Flags: review+
(Assignee)

Updated

10 years ago
Attachment #292599 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Attachment #301651 - Attachment description: xpinstall patch for checkin → xpinstall patch for checkin [checked in]
(Assignee)

Comment 37

10 years ago
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
Last Resolved: 10 years ago
Resolution: --- → FIXED
Whiteboard: [has patch]
Builds don't start anymore.
(Assignee)

Comment 39

10 years ago
(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?
Depends on: 416036
Product: Firefox → Toolkit
You need to log in before you can comment on or make changes to this bug.