Certain error codes are not picked up by onErrorOccured

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
WebExtensions: Request Handling
P3
normal
RESOLVED FIXED
6 months ago
3 months ago

People

(Reporter: jkt, Assigned: mixedpuppy)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

6 months ago
I'm working on this extension: https://github.com/jonathanKingston/fix-my-http

It uses onErrorOccured to navigate to detect issues in forcing pages to be https.

However when typing "npr.com" into the URL bar I get a SSL_ERROR_BAD_CERT_DOMAIN that isn't picked up by the listener.

Error log:
“nsICookieManager2.add()” is changed. Update your code and pass the correct originAttributes. Read more on MDN: https://developer.mozilla.org/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsICookieManager2  HTTPS.js:175:6
NS_ERROR_DOM_BAD_DOCUMENT_DOMAIN: Illegal document.domain value  www.npr.org:22
npr.com:443 uses an invalid security certificate.

The certificate is only valid for *.npr.org

Error code: <a id="errorCode" title="SSL_ERROR_BAD_CERT_DOMAIN">SSL_ERROR_BAD_CERT_DOMAIN</a>
  (unknown)

I'm also seeing the following error fairly often but might be unrelated:
channel.URI is undefined  WebRequest.jsm:798
(Reporter)

Comment 1

6 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f014eda5ece
Comment hidden (mozreview-request)
(Reporter)

Comment 3

6 months ago
The patch solves the issue mentioned, however I dunno if it creates it's own issues etc.
Hi Jonathan, it seems some tests are failing on that try push.  If you are wanna investigate that, and add a test for this bug, I can assign the bug to you.

Feel free to ask if you need help with anything, here or in #webextensions on irc.
(Reporter)

Comment 5

6 months ago
I think there is a lot more to it than the quick patch I wrote, some valid pages are counted as error with that patch and that obviously isn't expected.

I'm still experimenting with my extension and also the patch however I'm not sure if I will get time to work on this (that is why I didn't assign it to me from the get go).

Updated

6 months ago
Assignee: nobody → mixedpuppy
Priority: -- → P3
Whiteboard: triaged
(Assignee)

Updated

5 months ago
webextensions: --- → ?

Updated

5 months ago
webextensions: ? → +
Comment hidden (mozreview-request)
(Assignee)

Updated

3 months ago
Attachment #8865683 - Flags: review?(kmaglione+bmo) → review?(aswan)
(Assignee)

Comment 7

3 months ago
Honza,
I'm trying to figure out if there is a way to surface ssl connection failures.  Looking at httpchannel it doesn't seem so.  The patch in comment 6 is wrong, but illustrates where I'm trying to catch the failure.  Even just getting a connection refused out to the addon would be useful, but more detail would of course be better.  Any thoughts?
Thanks.
Flags: needinfo?(honzab.moz)

Comment 8

3 months ago
mozreview-review
Comment on attachment 8865683 [details]
Bug 1338860 fix onErrorOccurred to handle some additional errors,

https://reviewboard.mozilla.org/r/137308/#review145260

I think Shane is still looking into this so clearing the r?  Please re-set it if I misunderstood...
Attachment #8865683 - Flags: review?(aswan)
(In reply to Shane Caraveo (:mixedpuppy) from comment #7)
> Honza,
> I'm trying to figure out if there is a way to surface ssl connection
> failures.  Looking at httpchannel it doesn't seem so.  The patch in comment
> 6 is wrong, but illustrates where I'm trying to catch the failure.  Even
> just getting a connection refused out to the addon would be useful, but more
> detail would of course be better.  Any thoughts?
> Thanks.

No idea what |channel| at WebRequest.jsm in the patch is.  If it's nsIRequest (nsIChannel) and implemented by nsHttpChannel, you can use |status| [1] property to read nsresult.  SSL errors are kinda "encrypted" to the value, see [2] and the list of errors at [3].

[1] https://dxr.mozilla.org/mozilla-central/rev/5bc1c758ab57c1885dceab4e7837e58af27b998c/netwerk/base/nsIRequest.idl#43
[2] https://dxr.mozilla.org/mozilla-central/rev/5bc1c758ab57c1885dceab4e7837e58af27b998c/security/manager/ssl/NSSErrorsService.cpp#79
[3] https://developer.mozilla.org/en-US/docs/Mozilla/Projects/NSS/SSL_functions/sslerr.html
Flags: needinfo?(honzab.moz)
Comment hidden (mozreview-request)
(Assignee)

Comment 11

3 months ago
The primary problem was that we're not adding the nsIRequestObserver early enough if you only use the onErrorOccurred listener.  Secondary problem was getting the actual error code for connection issues related to ssl/cert/etc.  The new patch addresses those.

Running a try to see if it blows up anywhere, there are is one questionable change that I may work on more.
Comment hidden (mozreview-request)
(Assignee)

Updated

3 months ago
Attachment #8865683 - Flags: review?(aswan)
(Assignee)

Updated

3 months ago
Attachment #8865683 - Flags: review?(kmaglione+bmo)
(Assignee)

Updated

3 months ago
Attachment #8865683 - Flags: review?(aswan)

Comment 13

3 months ago
mozreview-review
Comment on attachment 8865683 [details]
Bug 1338860 fix onErrorOccurred to handle some additional errors,

https://reviewboard.mozilla.org/r/137308/#review149274

It looks good to me but I don't have deep background with webrequest, would be good to get Kris's eyes on this too.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_webrequest_errors.html:16
(Diff revision 3)
> +</head>
> +<body>
> +<script type="text/javascript">
> +"use strict";
> +
> +function* test_connection_refused(url, expectedError) {

use async

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_webrequest_errors.html:20
(Diff revision 3)
> +
> +function* test_connection_refused(url, expectedError) {
> +  async function background(url, expectedError) {
> +    browser.test.log(`background url is ${url}`);
> +    browser.webRequest.onErrorOccurred.addListener(details => {
> +      browser.test.log(`onErrorOccurred ${JSON.stringify(details)}`);

take this out for landing?

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_webrequest_errors.html:55
(Diff revision 3)
> +  yield extension.awaitMessage("done");
> +
> +  yield extension.unload();
> +}
> +
> +add_task(function* test_bad_cert() {

no need for a generator or async here

::: toolkit/modules/addons/WebRequestCommon.jsm:21
(Diff revision 3)
> +const SEC_ERROR_BASE = Ci.nsINSSErrorsService.NSS_SEC_ERROR_BASE;
> +const SSL_ERROR_BASE = Ci.nsINSSErrorsService.NSS_SSL_ERROR_BASE;
> +const MOZILLA_PKIX_ERROR_BASE = Ci.nsINSSErrorsService.MOZILLA_PKIX_ERROR_BASE;
> +
> +const SEC_ERRORS = {};
> +SEC_ERRORS[SEC_ERROR_BASE + 8]   = "SEC_ERROR_INVALID_TIME";

Ugh, is exposing the original definitions of these via idl or something not practical?  Or just a matter of not having time do it right now?
Attachment #8865683 - Flags: review?(aswan) → review+

Comment 14

3 months ago
mozreview-review
Comment on attachment 8865683 [details]
Bug 1338860 fix onErrorOccurred to handle some additional errors,

https://reviewboard.mozilla.org/r/137308/#review149314

r- for the hard coded list of error codes. The rest looks OK to me.

::: toolkit/modules/addons/WebRequest.jsm:1008
(Diff revision 3)
> -      if (!channelData.hasListener && channel instanceof Ci.nsITraceableChannel) {
> +    if (!this.needTracing || channelData.hasListener || !(channel instanceof Ci.nsITraceableChannel)) {
> +      return;
> +    }
> -        let responseStatus = channel.responseStatus;
> +    let responseStatus = channel.responseStatus;
> -        // skip redirections, https://bugzilla.mozilla.org/show_bug.cgi?id=728901#c8
> +    // skip redirections, https://bugzilla.mozilla.org/show_bug.cgi?id=728901#c8
> -        if (responseStatus < 300 || responseStatus >= 400) {
> +    if (!responseStatus || (responseStatus < 300 || responseStatus >= 400)) {

Nit: No need for parens. Although I'm not sure why the `!responseStatus` check is necessary. 0 is < 300, and `responseStatus` should always be a number.

::: toolkit/modules/addons/WebRequestCommon.jsm:20
(Diff revision 3)
> +const SEC_ERRORS = {};
> +SEC_ERRORS[SEC_ERROR_BASE + 8]   = "SEC_ERROR_INVALID_TIME";
> +SEC_ERRORS[SEC_ERROR_BASE + 9]   = "SEC_ERROR_BAD_DER";
> +SEC_ERRORS[SEC_ERROR_BASE + 10]  = "SEC_ERROR_BAD_SIGNATURE";
> +SEC_ERRORS[SEC_ERROR_BASE + 11]  = "SEC_ERROR_EXPIRED_CERTIFICATE";
> +SEC_ERRORS[SEC_ERROR_BASE + 12]  = "SEC_ERROR_REVOKED_CERTIFICATE";
> +SEC_ERRORS[SEC_ERROR_BASE + 13]  = "SEC_ERROR_UNKNOWN_ISSUER";
> +SEC_ERRORS[SEC_ERROR_BASE + 20]  = "SEC_ERROR_UNTRUSTED_ISSUER";
> +SEC_ERRORS[SEC_ERROR_BASE + 21]  = "SEC_ERROR_UNTRUSTED_CERT";
> +SEC_ERRORS[SEC_ERROR_BASE + 30]  = "SEC_ERROR_EXPIRED_ISSUER_CERTIFICATE";
> +SEC_ERRORS[SEC_ERROR_BASE + 36]  = "SEC_ERROR_CA_CERT_INVALID";
> +SEC_ERRORS[SEC_ERROR_BASE + 41]  = "SEC_ERROR_UNKNOWN_CRITICAL_EXTENSION";
> +SEC_ERRORS[SEC_ERROR_BASE + 90]  = "SEC_ERROR_INADEQUATE_KEY_USAGE";
> +SEC_ERRORS[SEC_ERROR_BASE + 91]  = "SEC_ERROR_INADEQUATE_CERT_TYPE";
> +SEC_ERRORS[SEC_ERROR_BASE + 112] = "SEC_ERROR_CERT_NOT_IN_NAME_SPACE";
> +SEC_ERRORS[SEC_ERROR_BASE + 117] = "SEC_ERROR_CERT_BAD_ACCESS_LOCATION";
> +SEC_ERRORS[SEC_ERROR_BASE + 120] = "SEC_ERROR_OCSP_MALFORMED_REQUEST";
> +SEC_ERRORS[SEC_ERROR_BASE + 121] = "SEC_ERROR_OCSP_SERVER_ERROR";
> +SEC_ERRORS[SEC_ERROR_BASE + 122] = "SEC_ERROR_OCSP_TRY_SERVER_LATER";
> +SEC_ERRORS[SEC_ERROR_BASE + 123] = "SEC_ERROR_OCSP_REQUEST_NEEDS_SIG";
> +SEC_ERRORS[SEC_ERROR_BASE + 124] = "SEC_ERROR_OCSP_UNAUTHORIZED_REQUEST";
> +SEC_ERRORS[SEC_ERROR_BASE + 126] = "SEC_ERROR_OCSP_UNKNOWN_CERT";
> +SEC_ERRORS[SEC_ERROR_BASE + 129] = "SEC_ERROR_OCSP_MALFORMED_RESPONSE";
> +SEC_ERRORS[SEC_ERROR_BASE + 130] = "SEC_ERROR_OCSP_UNAUTHORIZED_RESPONSE";
> +SEC_ERRORS[SEC_ERROR_BASE + 132] = "SEC_ERROR_OCSP_OLD_RESPONSE";
> +SEC_ERRORS[SEC_ERROR_BASE + 141] = "SEC_ERROR_UNSUPPORTED_ELLIPTIC_CURVE";
> +SEC_ERRORS[SEC_ERROR_BASE + 144] = "SEC_ERROR_OCSP_INVALID_SIGNING_CERT";
> +SEC_ERRORS[SEC_ERROR_BASE + 160] = "SEC_ERROR_POLICY_VALIDATION_FAILED";
> +SEC_ERRORS[SEC_ERROR_BASE + 157] = "SEC_ERROR_OCSP_BAD_SIGNATURE";
> +SEC_ERRORS[SEC_ERROR_BASE + 176] = "SEC_ERROR_CERT_SIGNATURE_ALGORITHM_DISABLED";
> +
> +SEC_ERRORS[SSL_ERROR_BASE + 2]   = "SSL_ERROR_NO_CYPHER_OVERLAP";
> +SEC_ERRORS[SSL_ERROR_BASE + 12]  = "SSL_ERROR_BAD_CERT_DOMAIN";
> +SEC_ERRORS[SSL_ERROR_BASE + 17]  = "SSL_ERROR_BAD_CERT_ALERT";
> +SEC_ERRORS[SSL_ERROR_BASE + 132] = "SSL_ERROR_WEAK_SERVER_CERT_KEY";
> +
> +SEC_ERRORS[MOZILLA_PKIX_ERROR_BASE + 0]  = "MOZILLA_PKIX_ERROR_KEY_PINNING_FAILURE";
> +SEC_ERRORS[MOZILLA_PKIX_ERROR_BASE + 1]  = "MOZILLA_PKIX_ERROR_CA_CERT_USED_AS_END_ENTITY";
> +SEC_ERRORS[MOZILLA_PKIX_ERROR_BASE + 2]  = "MOZILLA_PKIX_ERROR_INADEQUATE_KEY_SIZE";
> +SEC_ERRORS[MOZILLA_PKIX_ERROR_BASE + 3]  = "MOZILLA_PKIX_ERROR_V1_CERT_USED_AS_CA";
> +SEC_ERRORS[MOZILLA_PKIX_ERROR_BASE + 5]  = "MOZILLA_PKIX_ERROR_NOT_YET_VALID_CERTIFICATE";
> +SEC_ERRORS[MOZILLA_PKIX_ERROR_BASE + 6]  = "MOZILLA_PKIX_ERROR_NOT_YET_VALID_ISSUER_CERTIFICATE";
> +SEC_ERRORS[MOZILLA_PKIX_ERROR_BASE + 8]  = "MOZILLA_PKIX_ERROR_OCSP_RESPONSE_FOR_CERT_MISSING";
> +SEC_ERRORS[MOZILLA_PKIX_ERROR_BASE + 10] = "MOZILLA_PKIX_ERROR_REQUIRED_TLS_FEATURE_MISSING";
> +SEC_ERRORS[MOZILLA_PKIX_ERROR_BASE + 12] = "MOZILLA_PKIX_ERROR_EMPTY_ISSUER_NAME";

Hard coding a list of error codes seems like a Very Bad Idea. Can we just use nsINSSErrorService.getErrorMessage instead? We already document that these messages must not ever be parsed for error codes.
Attachment #8865683 - Flags: review?(kmaglione+bmo) → review-
(Assignee)

Comment 15

3 months ago
mozreview-review-reply
Comment on attachment 8865683 [details]
Bug 1338860 fix onErrorOccurred to handle some additional errors,

https://reviewboard.mozilla.org/r/137308/#review149314

> Nit: No need for parens. Although I'm not sure why the `!responseStatus` check is necessary. 0 is < 300, and `responseStatus` should always be a number.

channel.responseStatus is undefined until there is a response.  channel.status is zero here.

Comment 16

3 months ago
mozreview-review-reply
Comment on attachment 8865683 [details]
Bug 1338860 fix onErrorOccurred to handle some additional errors,

https://reviewboard.mozilla.org/r/137308/#review149314

> channel.responseStatus is undefined until there is a response.  channel.status is zero here.

According to the IDL, it should throw before there's a response:

http://searchfox.org/mozilla-central/source/netwerk/protocol/http/nsIHttpChannel.idl#258

It shouldn't be possible for it to be undefined if the channel is QIed to nsIHttpChannel.
(Assignee)

Comment 17

3 months ago
mozreview-review-reply
Comment on attachment 8865683 [details]
Bug 1338860 fix onErrorOccurred to handle some additional errors,

https://reviewboard.mozilla.org/r/137308/#review149274

> no need for a generator or async here

actuall, async is necessary.

Comment 18

3 months ago
mozreview-review-reply
Comment on attachment 8865683 [details]
Bug 1338860 fix onErrorOccurred to handle some additional errors,

https://reviewboard.mozilla.org/r/137308/#review149274

> actuall, async is necessary.

you can just return the promise instead of yield or await ing it
Comment hidden (mozreview-request)
(Assignee)

Comment 20

3 months ago
> > actuall, async is necessary.
> 
> you can just return the promise instead of yield or await ing it

right.
Comment hidden (mozreview-request)
(Assignee)

Updated

3 months ago
Attachment #8865683 - Flags: review?(kmaglione+bmo)

Comment 22

3 months ago
mozreview-review
Comment on attachment 8865683 [details]
Bug 1338860 fix onErrorOccurred to handle some additional errors,

https://reviewboard.mozilla.org/r/137308/#review149400
Attachment #8865683 - Flags: review?(kmaglione+bmo) → review+

Comment 23

3 months ago
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3c59f4cceb9a
fix onErrorOccurred to handle some additional errors, r=aswan,kmag
https://hg.mozilla.org/mozilla-central/rev/3c59f4cceb9a
Status: NEW → RESOLVED
Last Resolved: 3 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.