From 10b34f0b2fac6149d11d5e3d33c32cea599e2be8 Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 3 May 2026 14:39:17 +0000 Subject: [PATCH 1/2] Address review: gate socks auth to socks5, disable pconn for SOCKS peers - generate.php: only emit socks-user/socks-pass for socks5 entries. The Squid patch rejects these options on socks4, so a socks4 line with credentials would break config parsing. - patch_apply.sh: set request->flags.proxyKeepalive=false on SOCKS peer requests in both FwdState::dispatch() and TunnelStateData:: connectDone(). Without this, the pconn pool (keyed by peer address, not target) could hand a SOCKS-negotiated connection to a request for a different target, silently routing data to the wrong host. https://claude.ai/code/session_01Cz4tkSZKDhTPnBx8YsYoyg --- setup/generate.php | 7 ++++++- squid_patch/patch_apply.sh | 14 ++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/setup/generate.php b/setup/generate.php index e901bbc..5d14e67 100644 --- a/setup/generate.php +++ b/setup/generate.php @@ -81,12 +81,17 @@ 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']) { + // 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. + if ($proxyInfo['scheme'] === 'socks5' && $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'] ); + } elseif ($proxyInfo['scheme'] === 'socks4' && ($proxyInfo['user'] || $proxyInfo['pass'])) { + fwrite(STDERR, "Note: SOCKS4 credentials ignored (use socks5 for auth): " . $proxyInfo['host'] . PHP_EOL); } $squid_conf[] = sprintf($squid_socks, $proxyInfo['host'], diff --git a/squid_patch/patch_apply.sh b/squid_patch/patch_apply.sh index aecaf95..f2a9501 100755 --- a/squid_patch/patch_apply.sh +++ b/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(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(request->url.port()); debugs(26, 3, "SOCKS" << sp->socks_type << " tunnel negotiation with peer " << sp->host From 882531d3fbdafd7d38c06cb6d9902417e08301df Mon Sep 17 00:00:00 2001 From: Claude Date: Sun, 3 May 2026 14:46:15 +0000 Subject: [PATCH 2/2] Address review: validate SOCKS5 credentials with !== '' and reject partials CodeRabbit flagged that the truthy check would (a) treat valid '0' values as missing and (b) silently emit a no-auth peer when only one of user/pass is supplied. Switch to explicit empty-string checks and skip SOCKS5 entries with incomplete credentials with a STDERR warning. https://claude.ai/code/session_01Cz4tkSZKDhTPnBx8YsYoyg --- setup/generate.php | 25 ++++++++++++++++++------- 1 file changed, 18 insertions(+), 7 deletions(-) diff --git a/setup/generate.php b/setup/generate.php index 5d14e67..b549c53 100644 --- a/setup/generate.php +++ b/setup/generate.php @@ -84,13 +84,24 @@ // 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. - if ($proxyInfo['scheme'] === 'socks5' && $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'] - ); - } elseif ($proxyInfo['scheme'] === 'socks4' && ($proxyInfo['user'] || $proxyInfo['pass'])) { + // 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,