If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Accept SOCKS5 as an alias for SOCKS

REOPENED
Unassigned

Status

()

Toolkit
WebExtensions: Request Handling
P3
normal
REOPENED
3 months ago
5 days ago

People

(Reporter: erosman, Unassigned)

Tracking

(Blocks: 1 bug)

55 Branch
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

3 months ago
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/
(Reporter)

Updated

3 months ago
Flags: needinfo?(mwein)

Comment 1

3 months ago
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.
(Reporter)

Comment 2

3 months ago
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.
Eric, is it possible to support both?
Flags: needinfo?(eric)

Comment 4

2 months ago
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.
Status: UNCONFIRMED → RESOLVED
Last Resolved: 2 months ago
Flags: needinfo?(mwein)
Resolution: --- → FIXED

Updated

2 months ago
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: FIXED → ---
Summary: ProxyScriptContext.jsm Correction of PROXY_TYPES → Accept SOCKS5 as an alias for SOCKS

Comment 5

2 months ago
@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.

Updated

2 months ago
Priority: -- → P3

Updated

2 months ago
Blocks: 1283639
Comment hidden (mozreview-request)

Updated

2 months ago
Flags: needinfo?(eric)
(Reporter)

Comment 7

2 months ago
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

2 months ago
mozreview-review
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.
Attachment #8889014 - Flags: review?(mixedpuppy) → review-
Eric, not sure of Matt's status with this bug now, can you dig out a solution to the above review comment?
Flags: needinfo?(eric)
> 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.
Flags: needinfo?(eric)
(Reporter)

Comment 11

2 months ago
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.

Updated

5 days ago
Duplicate of this bug: 1400957
You need to log in before you can comment on or make changes to this bug.