Open
Bug 1378205
Opened 7 years ago
Updated 1 year ago
Accept SOCKS5 as an alias for SOCKS
Categories
(WebExtensions :: Request Handling, enhancement, P5)
Tracking
(Not tracked)
REOPENED
People
(Reporter: eros_uk, Unassigned)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
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•7 years 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.
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 4•7 years 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
Closed: 7 years ago
Flags: needinfo?(mwein)
Resolution: --- → FIXED
Updated•7 years 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•7 years 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•7 years ago
|
Priority: -- → P3
Updated•7 years ago
|
Blocks: webextensions-proxy-api
Comment hidden (mozreview-request) |
Updated•7 years ago
|
Flags: needinfo?(eric)
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•7 years 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-
Comment 9•7 years ago
|
||
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)
Comment 10•7 years ago
|
||
> 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•7 years 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.
Comment 13•6 years ago
|
||
I'm not hearing anything is actually broken here, rather that the code and documentation are confusing. de-prioritizing into the netherlands of P5.
Priority: P3 → P5
Updated•6 years ago
|
Product: Toolkit → WebExtensions
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•