1378205 - Accept SOCKS5 as an alias for SOCKS

Status: REOPENED

(WebExtensions :: Request Handling, enhancement, P5)

Description

[erosman [:erosman]]

While the documents are not clear, from the feedback I have received, it seems the following may need a correction:

ProxyScriptContext.jsm Line 42-49

> const PROXY_TYPES = Object.freeze({
>   DIRECT: "direct",
>   HTTPS: "https",
>   PROXY: "proxy",
>   HTTP: "http", // Synonym for PROXY_TYPES.PROXY
>   SOCKS: "socks",  // SOCKS5
>   SOCKS4: "socks4",
> });

It seems SOCKS means Socks4 and for sock5, SOCKS5 should be used. i.e.:

> const PROXY_TYPES = Object.freeze({
>   DIRECT: "direct",
>   HTTPS: "https",
>   PROXY: "proxy",
>   HTTP: "http", // Synonym for PROXY_TYPES.PROXY
>   SOCKS: "socks",  // SOCKS4
>   SOCKS5: "socks5",
> });

Ref: Please check the feedback reviews here:
https://addons.mozilla.org/firefox/addon/foxypac/

Comment 1

[Eric Jung [:ericjung]]

SOCKS means SOCKS5, not SOCKS4. The existing code is accurate. No one uses SOCKS4 servers anymore to my knowledge; every SOCKS server out there supports SOCKS5 and has for many, many years.

Comment 2

[erosman [:erosman]]

I have been getting reports that in order to use SOCKS5 in PAC, they need to set it as SOCKS5 and plain SOCKS doesn't work !!?

That was the reason I field this bug.

Comment 3

[Shane Caraveo (:mixedpuppy)]

Eric, is it possible to support both?

Comment 4

[Matthew Wein [:mattw]]

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIProtocolProxyService#newProxyInfo() states that "socks" specifies a SOCKS version 5 proxy while "socks4" specifies a SOCKS version 4 proxy. If that is broken then the bug should be filed against that.

Comment 5

[Eric Jung [:ericjung]]

@Shane, yes, we can support both easily. However, the bug in comment2 does not seem possible with the current implementation--there is no use of "socks5" in the codebase.

Comment 6

[Matthew Wein [:mattw]]

Attachment: Bug 1378205 - Support SOCKS5 as an alias for SOCKS

Review commit: https://reviewboard.mozilla.org/r/160062/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/160062/

Comment 7

[erosman [:erosman]]

ref: https://dxr.mozilla.org/mozilla-central/source/netwerk/base/ProxyAutoConfig.h#61
> NOTE: SOCKS implies SOCKS4

Other C++ code using SOCKS5

https://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/base/proxyinfo.cc
https://dxr.mozilla.org/mozilla-central/source/media/webrtc/trunk/webrtc/base/autodetectproxy.cc

https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsProtocolProxyService.cpp#996
> map "SOCKS5" to "socks" to match contract-id of registered

Comment 8

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8889014 [details]
Bug 1378205 - Support SOCKS5 as an alias for SOCKS

https://reviewboard.mozilla.org/r/160062/#review165878

r- only due to my comment below.

::: toolkit/components/extensions/ProxyScriptContext.jsm:223
(Diff revision 1)
>  
> +        if (parts[0].toLowerCase() == PROXY_TYPES.SOCKS5) {
> +          // PROXY_TYPES.SOCKS5 is an alias for PROXY_TYPES.SOCKS.
> +          type = PROXY_TYPES.SOCKS;
> +        }
> +

Oh, that is strange indeed.  Based on logic here:

https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsProtocolProxyService.cpp#966

it looks like the logic should be:

if (parts[0].toLowerCase() == PROXY_TYPES.SOCKS) {
 type = PROXY_TYPES.SOCKS4
} else
if (parts[0].toLowerCase() == PROXY_TYPES.SOCKS5) {
 type = PROXY_TYPES.SOCKS
}

I think we need to verify what exactly should be happening here.

Comment 9

[Shane Caraveo (:mixedpuppy)]

Eric, not sure of Matt's status with this bug now, can you dig out a solution to the above review comment?

Comment 10

[Eric Jung [:ericjung]]

> Eric, not sure of Matt's status with this bug now, can you dig out a solution to the above review comment?

Yes

> I think we need to verify what exactly should be happening here.

For another point of reference, Chromium maps "SOCKS" to "SOCKS5":

https://cs.chromium.org/chromium/src/net/proxy/proxy_server.cc?sq=package:chromium&l=52
https://cs.chromium.org/chromium/src/net/proxy/proxy_server.h?sq=package:chromium&l=83

This code shows that internally "SOCKS" is used as the equivalent to "SOCKS5":

https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsProtocolProxyService.cpp#987
and then:
https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsProtocolProxyService.cpp#996  (kProxyType_SOCKS is SOCKS5)

Even https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsProtocolProxyService.cpp#1481 only looks for "SOCKS" or "SOCKS4". No mention of SOCKS5 because it is implied as kProxyType_SOCKS is SOCKS5.

This assumption has been made since as least Firefox 1.0 (November 2004). I've pasted nsProtocolProxyService.cpp from Firefox 1.0 here: https://pastebin.mozilla.org/9027956

but the link erosman shows definitely convolutes things:
https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsProtocolProxyService.cpp#966

that clearly says that "SOCKS" as specified in the PAC string "SOCKS foo.bar 1080" means SOCKS4. The code in my links don't parse user-created PAC strings, but they have been used by addons for years and the use of "SOCKS" there means SOCKS5. For example, nsProtocolProxyService::NewProxyInfo() is used by FoxyProxy and other addons. If they use "SOCKS" for the first argument, that results in the assumption of a SOCKS5 server... not SOCKS4.

Another way to resolve is to WONTFIX this bug as suggested in #comment4 when the bug was closed. That would remove any ambiguity by forcing users of this API to use "SOCKS5" or "SOCKS4" only.

Comment 11

[erosman [:erosman]]

Just a Note:

There following APIs use proxies and socks: 

- WebExtension Proxy API (SOCKS & SOCKS4) => (SOCKS = SOCKS5)
- Firefox Direct Proxy implementation (SOCKS4 & SOCKS5) => ( ??! )
- Firefox PAC implementation (SOCKS, SOCKS4 & SOCKS5) => (SOCKS = SOCKS4)


The problem is the disparity and hence the confusion for the users and developers.

Comment 13

[Shane Caraveo (:mixedpuppy)]

I'm not hearing anything is actually broken here, rather that the code and documentation are confusing.  de-prioritizing into the netherlands of P5.