Closed Bug 1338860 Opened 7 years ago Closed 7 years ago

Certain error codes are not picked up by onErrorOccured

Categories

(WebExtensions :: Request Handling, defect, P3)

Product:
WebExtensions
Component:
Request Handling
Type:
defect

Tracking

(firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed
Blocking Flags:
webextensions +

People

(Reporter: jkt, Assigned: mixedpuppy)

Details

(Whiteboard: triaged)

Attachments

(2 files)

Bug 1338860 - Fixing missing onErrorOcurred in WebRequest WebExtensions
59 bytes, text/x-review-board-request
Details
Bug 1338860 fix onErrorOccurred to handle some additional errors,
59 bytes, text/x-review-board-request
aswan
: review+
kmag
: review+
Details 
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
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.
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).
Assignee: nobody → mixedpuppy
Priority: -- → P3
Whiteboard: triaged
webextensions: --- → ?
webextensions: ? → +
Attachment #8865683 - Flags: review?(kmaglione+bmo) → review?(aswan)
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 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)
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.
Attachment #8865683 - Flags: review?(aswan)
Attachment #8865683 - Flags: review?(kmaglione+bmo)
Attachment #8865683 - Flags: review?(aswan)
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 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-
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 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.
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 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
> > actuall, async is necessary.
> 
> you can just return the promise instead of yield or await ing it

right.
Attachment #8865683 - Flags: review?(kmaglione+bmo)
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+
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
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: