1528873 - When proxy.onRequest returns a bogus ProxyInfo, Firefox connects directly instead of failing

Status: RESOLVED WONTFIX

(Core :: Networking, defect)

Description

[David Fifield]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/60.0

Steps to reproduce:

Open about:debugging and temporarily load the following extension:

manifest.json

{
    "manifest_version": 2,
    "name": "ProxyInfo test",
    "version": "1.0",

    "background": {
        "scripts": ["background.js"]
    },

    "permissions": [
        "proxy",
        "<all_urls>"
    ]
}

background.js

browser.proxy.onError.addListener(error => console.log("proxy error:", error.message));

browser.proxy.onRequest.addListener(
    // details => ({}),
    // details => ({type: "nonsense"}),
    // details => ({type: "socks"}),
    // details => ({type: "http"}),
    // details => ({type: "socks", host: "127.0.0.1"}),
    details => ({type: "socks", host: "127.0.0.1", port: 99999}),
    {urls: ["<all_urls>"]}
);

fetch("https://www.example.com/", {cache: "no-store"})
    .then(resp => console.log("fetch status:", resp.status))
    .catch(error => console.log("fetch error:", error.message));

Actual results:

The proxy.onRequest listener returns various incorrect or incomplete ProxyInfos. For each of them, the proxy.onError listener is called, but then Firefox goes ahead and does the fetch anyway, behaving as if there were no proxy configured. In the console I see:

proxy error: ProxyInfoData: Proxy server port 99999 outside range 1 to 65535 background.js:1:44
XHR GET https://www.example.com/ [HTTP/2.0 200 OK 324ms]
fetch status: 200 background.js:14:19

I was able to provoke several different error messages, but they all result in the fetch happening despite the error:

{}
	ProxyInfoData: Invalid proxy server type: "undefined"

{type: "nonsense"}
	ProxyInfoData: Invalid proxy server type: "nonsense"

{type: "socks"}
{type: "http"}
	ProxyInfoData: Invalid proxy server host: "undefined"

{type: "socks", host: "127.0.0.1"}
	ProxyInfoData: Invalid proxy server port: "NaN"

{type: "socks", host: "127.0.0.1", port: 99999}
	ProxyInfoData: Proxy server port 99999 outside range 1 to 65535

Expected results:

I expected that a bogus ProxyInfo would prevent the fetch from happening, in order to prevent accidental proxy leaks.

I can imagine a workaround of setting a flag in proxy.onError and then checking in webRequest.onBeforeRequest that the flag is not set.

Comment 1

[David Fifield]

Related, if the proxy.onRequest listener throws an exception, the fetch proceeds without a proxy, and without calling the proxy.onError listener:

browser.proxy.onError.addListener(error => console.log("proxy error:", error.message));

browser.proxy.onRequest.addListener(
    details => {
        throw new Error("my error");
        return {type: "socks", host: "127.0.0.1", port: 9};
    },
    {urls: ["<all_urls>"]}
);

fetch("https://www.example.com/", {cache: "no-store"})
    .then(resp => console.log("fetch status:", resp.status))
    .catch(error => console.log("fetch error:", error.message));

Making the proxy.onRequest listener async; i.e., having it return a rejected promise instead of throw an exception, still lets the fetch happen without a proxy, but ''does'' call the proxy.onError listener.

browser.proxy.onError.addListener(error => console.log("proxy error:", error.message));

browser.proxy.onRequest.addListener(
    async details => {
        throw new Error("my error");
        return {type: "socks", host: "127.0.0.1", port: 9};
    },
    {urls: ["<all_urls>"]}
);

fetch("https://www.example.com/", {cache: "no-store"})
    .then(resp => console.log("fetch status:", resp.status))
    .catch(error => console.log("fetch error:", error.message));

Comment 2

[Damira Akh]

Reproducible.

Tested on the following build.
Build ID 20180503143129
Update History
Update Channel release
User Agent Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/60.0

Placing it on Cor: Networking
Please feel free to correct the component if it's not in the right place.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:valentin, could you have a look please?

Comment 4

[Shane Caraveo (:mixedpuppy)]

AFAIK this is expected behavior. If a proxy handler is returning invalid data or throwing exceptions, and we didn't fallback, the browser would be broken for the user, potentially without and good indication as to why.

The browser.proxy.onRequest API in use explicitly returns the "default proxy" that is passed into it by the proxy service. If we have a failure it is passed back to the proxy service.

https://searchfox.org/mozilla-central/rev/b2d35912da5b2acecb0274eb113777d344ffb75e/toolkit/components/extensions/ProxyScriptContext.jsm#348

Comment 5

[Valentin Gosu [:valentin] (he/him)]

Thanks for the input Shane.

Comment 6

[David Fifield]

(In reply to Shane Caraveo (:mixedpuppy) from comment #4)

AFAIK this is expected behavior. If a proxy handler is returning invalid data or throwing exceptions, and we didn't fallback, the browser would be broken for the user, potentially without and good indication as to why.

This is a good point, but there's a caveat: "fallback to default proxy" can't always be assumed to be a safe default. A user may be using an extension for safety or privacy reasons (perhaps in a separate profile). Turn the situation around: if a proxy handler is returning invalid data or throwing exceptions, then an implicit fallback means the user's traffic is exposed to an eavesdropper, potentially without any indication that this has happened.

A bug in a proxy handler could equally cause it to return syntactically valid but incorrect data, like a nonexistent host or the wrong port number. This, too, would leave the user with a broken browser. But an error of that sort doesn't cause the browser to fall back to a direct connection; rather you get "fetch error: NetworkError when attempting to fetch resource".

I think the current fail-open design is actually workable, because it's probably possible to build in an adequate failsafe by detecting errors in proxy.onError and canceling requests in webRequest.onBeforeRequest whenever an error has occurred. I won't say it's necessarily a bug (but speaking for myself, it's contrary to what I expected from reading the documentation). It's at least consistent with my testing just now of webRequest.onBeforeRequest and webRequest.onBeforeSendHeaders—an exception or rejected promise in those listeners doesn't cancel the request (or even call webRequest.onErrorOccurred).

The fact that proxy.onError is called for rejected promises but not for exceptions (comment 1) still seems like a bug, though.

Comment 7

[David Fifield]

(In reply to david from comment #6)

The fact that proxy.onError is called for rejected promises but not for exceptions (comment 1) still seems like a bug, though.

I'll open a separate report for that, if you like.

Comment 8

[Shane Caraveo (:mixedpuppy)]

For onError,

added bug 1533505

caveat: "fallback to default proxy" can't always be assumed to be a safe default."

added bug 1533509

Both of these would be handled in the webextensions side, nothing dependent on core networking code. However, core networking people may have formed opinions on what the behavior should be, if so please comment on the new bugs.