[tor-commits] [meek/master] Detect errors of proxy.onRequest.

[dcf at torproject.org]

commit 5622b46cbccd7901bfd8523f08918315801b5cea
Author: David Fifield <david at bamsoftware.com>
Date:   Tue Feb 19 00:58:13 2019 -0700

    Detect errors of proxy.onRequest.
    
    It turns out that if an error occurs in proxy.onRequest, Firefox will
    ignore it and continue on as if there were no proxy. This can happen,
    for example, if the "type" of a ProxyInfo isn't one of the recognized
    types, or if the ProxyInfo is missing a field like "host".
    https://bugzilla.mozilla.org/show_bug.cgi?id=1528873
    To prevent silent failures like this, we register another pair of event
    listeners. Unlike the headers and proxy event listeners, these remain
    static for all requests, and are not overwritten with each new request:
     * proxy.onError detects when an error occurs.
     * webRequest.onBeforeRequest cancels all requests after an error
    I made the error condition be persistent because it's not something that
    should arise during normal operation. proxy.onError only gets called
    when an error occurs in proxy.onRequest, and that only happens when the
    proxy specification is bogus. It doesn't get called when there's an
    network error, for example.
    
    In order to make exceptions in proxy.onRequest be noticed by
    proxy.onError, I had to make it return a rejection promise rather than
    throw an exception. An exception just gets logged to the browser console
    and nothing else.
---
 webextension/background.js | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/webextension/background.js b/webextension/background.js
index 3b24a37..b43e614 100644
--- a/webextension/background.js
+++ b/webextension/background.js
@@ -250,6 +250,30 @@ async function roundtrip(request) {
     }
 }
 
+// If an error occurs in a proxy.onRequest listener (for instance if a ProxyInfo
+// field is missing or invalid), the browser will ignore the proxy and just
+// connect directly. It will, however, call proxy.onError listeners. Register a
+// static proxy.onError listener that sets a global flag if an error ever
+// occurs; and a static browser.onBeforeRequest listener which checks the flag
+// and cancels every request if it is set. We could be less severe here (we
+// probably only need to cancel the *next* request that occurs after a proxy
+// error), but this setup is meant to be a fail-closed safety net for what is
+// essentially a "can't happen" state under correct configuration. Note that
+// proxy.onError doesn't get called for transient errors like a failure to
+// connect to the proxy, only for nonsensical ProxyInfo configurations.
+// https://bugzilla.mozilla.org/show_bug.cgi?id=1528873
+// https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/proxy/onError
+let proxyError = null;
+browser.proxy.onError.addListener(error => {
+    console.log(`proxy error, disabling: ${error.message}`);
+    proxyError = error;
+});
+browser.webRequest.onBeforeRequest.addListener(
+    details => ({cancel: proxyError != null}),
+    {urls: ["http://*/*", "https://*/*"]},
+    ["blocking"]
+);
+
 // Connect to our native process.
 let port = browser.runtime.connectNative("meek.http.helper");