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

RESOLVED WONTFIX

Status

()

defect
RESOLVED WONTFIX
2 months ago
a month ago

People

(Reporter: david, Unassigned)

Tracking

65 Branch
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

2 months ago

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.

(Reporter)

Comment 1

2 months ago

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

a month ago

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.

Status: UNCONFIRMED → NEW
Component: Untriaged → Networking
Ever confirmed: true
Product: Firefox → Core

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

Flags: needinfo?(valentin.gosu)

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

Thanks for the input Shane.

Status: NEW → RESOLVED
Last Resolved: a month ago
Flags: needinfo?(valentin.gosu)
Resolution: --- → WONTFIX
(Reporter)

Comment 6

a month ago

(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.

(Reporter)

Comment 7

a month ago

(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.

See Also: → 1533505
See Also: → 1533509

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.

You need to log in before you can comment on or make changes to this bug.