Skip to content

Commit 02e7f47

Browse files
committed
Address Copilot review: socket error handling, SOCKS5 method validation, cleanup fixes
- SocksPeerConnector.h: Check getsockopt/setsockopt return values, fail negotiation if timeout setup fails, validate socket restore on cleanup - SocksPeerConnector.h: Explicitly reject unsupported SOCKS5 auth methods (e.g. GSSAPI) instead of falling through - generate.php: Remove URL-encoding of SOCKS credentials (RFC1929 uses raw) - squid-build-test.yml: Replace `kill %1` with `pkill -f` for reliable background process cleanup across steps - container-release.yml: Lowercase repository owner for GHCR image name https://claude.ai/code/session_01Tfy3kPd51qRgxpCFXjb2g9
1 parent 80cfad5 commit 02e7f47

File tree

4 files changed

+34
-21
lines changed

4 files changed

+34
-21
lines changed

.github/workflows/container-release.yml

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,7 @@ permissions:
2020

2121
env:
2222
REGISTRY: ghcr.io
23-
IMAGE_NAME: ${{ github.repository_owner }}/squid-socks
23+
IMAGE_NAME: squid-socks
2424

2525
jobs:
2626
build-and-push:
@@ -46,11 +46,14 @@ jobs:
4646
- name: Prepare tags
4747
id: tags
4848
run: |
49-
TAGS="${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}:${{ inputs.version }}"
49+
OWNER=$(echo "${{ github.repository_owner }}" | tr '[:upper:]' '[:lower:]')
50+
FULL_IMAGE="${{ env.REGISTRY }}/${OWNER}/${{ env.IMAGE_NAME }}"
51+
TAGS="${FULL_IMAGE}:${{ inputs.version }}"
5052
if [ "${{ inputs.push_latest }}" = "true" ] && [ "${{ inputs.version }}" != "latest" ]; then
51-
TAGS="${TAGS},${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}:latest"
53+
TAGS="${TAGS},${FULL_IMAGE}:latest"
5254
fi
5355
echo "tags=${TAGS}" >> "$GITHUB_OUTPUT"
56+
echo "image=${FULL_IMAGE}" >> "$GITHUB_OUTPUT"
5457
echo "Tags to push: ${TAGS}"
5558
5659
- name: Build and push
@@ -71,6 +74,6 @@ jobs:
7174
7275
- name: Verify pushed image
7376
run: |
74-
docker pull ${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}:${{ inputs.version }}
75-
docker run --rm ${{ env.REGISTRY }}/${{ env.IMAGE_NAME }}:${{ inputs.version }} squid -v
77+
docker pull ${{ steps.tags.outputs.image }}:${{ inputs.version }}
78+
docker run --rm ${{ steps.tags.outputs.image }}:${{ inputs.version }} squid -v
7679
echo "--- Image verification OK ---"

.github/workflows/squid-build-test.yml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -284,7 +284,7 @@ jobs:
284284
run: |
285285
docker rm -f squid-test 2>/dev/null || true
286286
pkill microsocks 2>/dev/null || true
287-
kill %1 2>/dev/null || true
287+
pkill -f 'python3 -m http.server' 2>/dev/null || true
288288
289289
# ------------------------------------------------------------------
290290
# 4. Test: SOCKS5 with authentication
@@ -442,7 +442,7 @@ jobs:
442442
run: |
443443
docker rm -f squid-auth 2>/dev/null || true
444444
pkill microsocks 2>/dev/null || true
445-
kill %1 2>/dev/null || true
445+
pkill -f 'python3 -m http.server' 2>/dev/null || true
446446
447447
# ------------------------------------------------------------------
448448
# 5. Test: generate.php produces correct config

setup/generate.php

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -45,9 +45,10 @@
4545
// Native SOCKS support via Squid cache_peer patch (no Gost needed)
4646
$socksOpt = $proxyInfo['scheme']; // "socks4" or "socks5"
4747
if ($proxyInfo['user'] && $proxyInfo['pass']) {
48+
// SOCKS5 RFC1929 uses raw username/password; do not URL-encode.
4849
$socksOpt .= sprintf(' socks-user=%s socks-pass=%s',
49-
urlencode($proxyInfo['user']),
50-
urlencode($proxyInfo['pass'])
50+
$proxyInfo['user'],
51+
$proxyInfo['pass']
5152
);
5253
}
5354
$squid_conf[] = sprintf($squid_socks,

squid_patch/src/SocksPeerConnector.h

Lines changed: 21 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -181,10 +181,11 @@ static inline bool socks5Connect(int fd,
181181
if (aResp[0] != 0x01 || aResp[1] != 0x00)
182182
return false; /* auth failed or wrong sub-negotiation version */
183183

184-
} else if (gResp[1] == 0xFF) {
185-
return false; /* no acceptable method */
184+
} else if (gResp[1] == 0x00) {
185+
/* no auth required */
186+
} else {
187+
return false; /* unsupported or unacceptable method (includes 0xFF) */
186188
}
187-
/* else gResp[1] == 0x00 → no auth required */
188189

189190
/* --- connect request --------------------------------------------- */
190191
uint8_t connReq[263];
@@ -286,26 +287,34 @@ static inline bool negotiate(int fd, SocksPeerType type,
286287
/* save original timeouts and set a 10 s limit for the handshake */
287288
struct timeval origRecvTv = {0, 0}, origSendTv = {0, 0};
288289
socklen_t tvLen = sizeof(struct timeval);
289-
getsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, &origRecvTv, &tvLen);
290-
tvLen = sizeof(struct timeval);
291-
getsockopt(fd, SOL_SOCKET, SO_SNDTIMEO, &origSendTv, &tvLen);
290+
if (getsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, &origRecvTv, &tvLen) < 0 ||
291+
getsockopt(fd, SOL_SOCKET, SO_SNDTIMEO, &origSendTv, &tvLen) < 0) {
292+
fcntl(fd, F_SETFL, flags);
293+
return false;
294+
}
292295

293296
struct timeval tv;
294297
tv.tv_sec = 10;
295298
tv.tv_usec = 0;
296-
setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof(tv));
297-
setsockopt(fd, SOL_SOCKET, SO_SNDTIMEO, &tv, sizeof(tv));
299+
if (setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, &tv, sizeof(tv)) < 0 ||
300+
setsockopt(fd, SOL_SOCKET, SO_SNDTIMEO, &tv, sizeof(tv)) < 0) {
301+
fcntl(fd, F_SETFL, flags);
302+
return false;
303+
}
298304

299305
bool ok = false;
300306
if (type == SOCKS_V4)
301307
ok = socks4Connect(fd, targetHost, targetPort, user);
302308
else if (type == SOCKS_V5)
303309
ok = socks5Connect(fd, targetHost, targetPort, user, pass);
304310

305-
/* restore original flags and timeouts */
306-
fcntl(fd, F_SETFL, flags);
307-
setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, &origRecvTv, sizeof(origRecvTv));
308-
setsockopt(fd, SOL_SOCKET, SO_SNDTIMEO, &origSendTv, sizeof(origSendTv));
311+
/* restore original flags and timeouts (best-effort, log-worthy but not fatal) */
312+
int restoreOk = 0;
313+
restoreOk |= fcntl(fd, F_SETFL, flags);
314+
restoreOk |= setsockopt(fd, SOL_SOCKET, SO_RCVTIMEO, &origRecvTv, sizeof(origRecvTv));
315+
restoreOk |= setsockopt(fd, SOL_SOCKET, SO_SNDTIMEO, &origSendTv, sizeof(origSendTv));
316+
if (restoreOk < 0 && ok)
317+
return false; /* negotiation succeeded but socket is in bad state */
309318

310319
return ok;
311320
}

0 commit comments

Comments
 (0)