Fix SOCKS connection pooling and authentication handling#32
Merged
39ff merged 2 commits intoMay 3, 2026
Merged
Conversation
- 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
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@setup/generate.php`:
- Around line 87-95: The current SOCKS5 check uses truthy tests so missing/empty
or '0' values silently disable auth; update the condition on $proxyInfo in
generate.php to explicitly check for non-empty strings (use !== '' on both
$proxyInfo['user'] and $proxyInfo['pass']) and handle partial credentials: if
only one of user/pass is provided emit a warning to STDERR about the invalid
partial credential for that proxy host and skip (or throw) the entry instead of
falling back to no-auth; keep the SOCKS4 branch behavior but ensure it triggers
when either credential is non-empty string as well.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a06a5d62-c588-4804-90fe-8da11d492bc9
📒 Files selected for processing (2)
setup/generate.phpsquid_patch/patch_apply.sh
Comment on lines
87
to
95
| 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); | ||
| } |
There was a problem hiding this comment.
SOCKS5 の片側資格情報を黙って no-auth にフォールバックしないでください。
Line 87 の真偽値判定だと、user または pass の片方だけ指定されたときに警告なしで認証なし接続になります。'0' も偽扱いされるため、値として有効でも落ちます。!== '' で明示判定し、片側のみ指定時は警告して当該エントリをスキップ（または明示エラー）にしてください。
修正例
- if ($proxyInfo['scheme'] === 'socks5' && $proxyInfo['user'] && $proxyInfo['pass']) {
+ if ($proxyInfo['scheme'] === 'socks5') {
+ $hasUser = ($proxyInfo['user'] !== '');
+ $hasPass = ($proxyInfo['pass'] !== '');
+ 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' && ($proxyInfo['user'] || $proxyInfo['pass'])) {
fwrite(STDERR, "Note: SOCKS4 credentials ignored (use socks5 for auth): " . $proxyInfo['host'] . PHP_EOL);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 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); | |
| } | |
| if ($proxyInfo['scheme'] === 'socks5') { | |
| $hasUser = ($proxyInfo['user'] !== ''); | |
| $hasPass = ($proxyInfo['pass'] !== ''); | |
| 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' && ($proxyInfo['user'] || $proxyInfo['pass'])) { | |
| fwrite(STDERR, "Note: SOCKS4 credentials ignored (use socks5 for auth): " . $proxyInfo['host'] . PHP_EOL); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@setup/generate.php` around lines 87 - 95, The current SOCKS5 check uses
truthy tests so missing/empty or '0' values silently disable auth; update the
condition on $proxyInfo in generate.php to explicitly check for non-empty
strings (use !== '' on both $proxyInfo['user'] and $proxyInfo['pass']) and
handle partial credentials: if only one of user/pass is provided emit a warning
to STDERR about the invalid partial credential for that proxy host and skip (or
throw) the entry instead of falling back to no-auth; keep the SOCKS4 branch
behavior but ensure it triggers when either credential is non-empty string as
well.
…rtials 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
39ff
merged commit 2c1f95d
into
claude/squid-socks-proxy-support-CqoL5
3 of 11 checks passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This PR addresses two issues with SOCKS proxy support: improper connection reuse that could route requests to wrong destinations, and incorrect authentication credential handling for SOCKS4 vs SOCKS5.
Key Changes
Connection pooling fix: Disable HTTP keep-alive (
proxyKeepalive = false) for SOCKS-negotiated connections in bothFwdState::dispatch()and tunnel negotiation paths. This prevents pooled connections from being reused for different target hosts, which would silently route subsequent requests through the wrong SOCKS tunnel.SOCKS authentication validation: Updated configuration generation to only emit
socks-userandsocks-passoptions for SOCKS5 connections, as these credentials are only valid under RFC1929 (SOCKS5 authentication). SOCKS4 entries with credentials now trigger a warning message instead of generating invalid configuration.Implementation Details
The SOCKS tunnel is bound to a specific target host:port combination, but the connection pool is keyed by peer address only. Without disabling keep-alive, a subsequent request could reuse a pooled connection and inadvertently send data through the previous target's tunnel.
SOCKS4 does not support username/password authentication per its specification, so the Squid patch correctly rejects these options. The PHP configuration generator now respects this limitation and warns users to use SOCKS5 if authentication is needed.
https://claude.ai/code/session_01Cz4tkSZKDhTPnBx8YsYoyg
Summary by CodeRabbit
バグ修正