Merge pull request #32 from 39ff/claude/address-review-comments-uEXNB

Fix SOCKS connection pooling and authentication handling

Author: 39ff

setup/generate.php

@@ -81,12 +81,28 @@
     elseif(in_array($proxyInfo['scheme'], ['socks4', 'socks5'], true)){
         // Native SOCKS support via Squid cache_peer patch (no Gost needed)
         $socksOpt = $proxyInfo['scheme'];  // "socks4" or "socks5"
-        if ($proxyInfo['user'] && $proxyInfo['pass']) {
-            // SOCKS5 RFC1929 uses raw username/password; do not URL-encode.
-            $socksOpt .= sprintf(' socks-user=%s socks-pass=%s',
-                $proxyInfo['user'],
-                $proxyInfo['pass']
-            );
+        // socks-user/socks-pass are only valid with socks5 (RFC1929).
+        // The Squid patch rejects them on socks4, so emitting them there
+        // would break config parsing.  Skip and warn for socks4 entries.
+        // Use !== '' rather than truthy checks so values like '0' are
+        // treated as valid credentials, and so a half-filled pair does
+        // not silently fall back to no-auth.
+        $hasUser = ($proxyInfo['user'] !== '');
+        $hasPass = ($proxyInfo['pass'] !== '');
+        if ($proxyInfo['scheme'] === 'socks5') {
+            if ($hasUser xor $hasPass) {
+                fwrite(STDERR, "Skipping SOCKS5 proxy with incomplete credentials: " . $proxyInfo['host'] . PHP_EOL);
+                continue;
+            }
+            if ($hasUser && $hasPass) {
+                // SOCKS5 RFC1929 uses raw username/password; do not URL-encode.
+                $socksOpt .= sprintf(' socks-user=%s socks-pass=%s',
+                    $proxyInfo['user'],
+                    $proxyInfo['pass']
+                );
+            }
+        } elseif ($proxyInfo['scheme'] === 'socks4' && ($hasUser || $hasPass)) {
+            fwrite(STDERR, "Note: SOCKS4 credentials ignored (use socks5 for auth): " . $proxyInfo['host'] . PHP_EOL);
         }
         $squid_conf[] = sprintf($squid_socks,
             $proxyInfo['host'],

squid_patch/patch_apply.sh

@@ -210,6 +210,15 @@ socks_hook = r'''
     /* SOCKS peer negotiation: after TCP connect, before HTTP dispatch */
     if (const auto sp = serverConnection()->getPeer()) {
         if (sp->socks_type) {
+            /* The SOCKS tunnel is bound to (request->url.host():port).
+             * The pconn pool is keyed by peer address, NOT target, so a
+             * pooled SOCKS-negotiated connection would silently route the
+             * next request to the WRONG destination.  Force the upstream
+             * connection to close after this request to keep one tunnel
+             * per target, and to guarantee the next dispatch() runs on a
+             * freshly-connected fd that has not been SOCKS-negotiated yet. */
+            request->flags.proxyKeepalive = false;
+
             const auto targetPort = static_cast<uint16_t>(request->url.port());
             debugs(17, 3, "SOCKS" << sp->socks_type
                    << " negotiation with peer " << sp->host
@@ -290,6 +299,11 @@ socks_tunnel_hook = r'''
     /* SOCKS peer: negotiate tunnel right after TCP connect */
     if (conn->getPeer() && conn->getPeer()->socks_type) {
         const auto sp = conn->getPeer();
+        /* Same rationale as FwdState::dispatch(): the SOCKS tunnel is
+         * bound to one target host, so prevent this connection from being
+         * returned to the pconn pool where another request could pick it
+         * up and silently send data into the previous target's tunnel. */
+        request->flags.proxyKeepalive = false;
         const auto targetPort = static_cast<uint16_t>(request->url.port());
         debugs(26, 3, "SOCKS" << sp->socks_type
                << " tunnel negotiation with peer " << sp->host