From c07c2b6b4a7c2ff2c3267e37f3e095083e8ee129 Mon Sep 17 00:00:00 2001 From: Youhai Date: Mon, 11 May 2026 00:09:46 +0800 Subject: [PATCH] fix(proxy): log when SOCKS5 credential auto-encoding rewrites URL (#157) (#209) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(proxy): log when SOCKS5 credential auto-encoding rewrites URL (#157) Auto URL-encoding of SOCKS5 credentials (added in v0.3.26 to fix Chromium's '=' truncation bug) currently happens silently. Users debugging connectivity have no way to know the wrapper rewrote their proxy URL — the original #157 thread took 8 round-trips to surface this exact ambiguity. Emit a log when re-encoding actually changes the URL: INFO on Python's 'cloakbrowser' logger, console.debug in JavaScript. Stays silent on already-encoded inputs and credential-less URLs to avoid false-positive noise. Credentials are not included in the log message. Tests: 3 new cases per language (Python caplog, JS vi.spyOn console.debug) covering trigger / silent-when-encoded / silent-when-no-creds. * fix(proxy): gate log on credential change, not full URL diff Per Copilot review on #209: urlparse cosmetically lowercases scheme and hostname, so comparing the full reconstructed URL to the input would emit "Auto URL-encoded SOCKS5..." even for inputs like `socks5://USER:pass@HOST.com:1080` where no credential encoding happened. Compare raw vs encoded user/password substrings instead. Mirror the same condition in JS for parity (JS's manual parser preserves case today, but the credential-level compare is more robust against future changes). Adds one regression test per language. --- cloakbrowser/browser.py | 22 +++++++++++++-- js/src/proxy.ts | 14 +++++++++- js/tests/proxy.test.ts | 61 ++++++++++++++++++++++++++++++++++++++++- tests/test_proxy.py | 36 ++++++++++++++++++++++++ 4 files changed, 128 insertions(+), 5 deletions(-) diff --git a/cloakbrowser/browser.py b/cloakbrowser/browser.py index 19a4b29..2f5f370 100644 --- a/cloakbrowser/browser.py +++ b/cloakbrowser/browser.py @@ -813,6 +813,10 @@ def _normalize_socks_string_url(url: str) -> str: truncate them at special chars like '='. Idempotent: pre-encoded input stays the same (decoded then re-encoded). + Emits an INFO log when re-encoding actually changes the URL, so users who + previously hit silent SOCKS5 fallback (#157) can see what the wrapper did. + Silent on already-encoded inputs (no false-positive noise). + On unparseable input (invalid port, broken IPv6 literal, etc.) logs a warning and returns the original string — preserves pre-fix pass-through behavior so Chromium's own error handling kicks in. @@ -828,18 +832,30 @@ def _normalize_socks_string_url(url: str) -> str: # urlparse returns None for absent components, "" for present-but-empty. if parsed.username is None and parsed.password is None: return url - enc_user = quote(unquote(parsed.username), safe="") if parsed.username else "" + raw_user = parsed.username or "" + enc_user = quote(unquote(raw_user), safe="") if raw_user else "" # Preserve the colon separator when password component is present, even if # empty, so `user:@host` stays `user:@host`. if parsed.password is not None: - enc_pass = quote(unquote(parsed.password), safe="") if parsed.password else "" + raw_pass = parsed.password + enc_pass = quote(unquote(raw_pass), safe="") if raw_pass else "" else: + raw_pass = None enc_pass = None - return _assemble_socks_url( + normalized = _assemble_socks_url( parsed.scheme, parsed.hostname or "", parsed.port, enc_user, enc_pass, parsed.path, parsed.params, parsed.query, parsed.fragment, ) + # Compare credentials, not the full URL: urlparse cosmetically lowercases + # scheme and hostname, so a full-string compare would falsely fire on + # `socks5://USER:pass@HOST.com:1080` even when no encoding work happened. + if enc_user != raw_user or enc_pass != raw_pass: + logger.info( + "Auto URL-encoded SOCKS5 proxy credentials (special characters " + "detected). Pre-encode the URL to suppress this notice." + ) + return normalized def _extract_proxy_url(proxy: str | ProxySettings | None) -> str | None: diff --git a/js/src/proxy.ts b/js/src/proxy.ts index 0b3ebe2..0f2895d 100644 --- a/js/src/proxy.ts +++ b/js/src/proxy.ts @@ -136,7 +136,19 @@ export function normalizeSocksStringUrl(urlStr: string): string { const encPass = hasPassword ? (rawPassEnc ? encodeURIComponent(lenientDecodeURIComponent(rawPassEnc)) : "") : null; - return assembleSocksUrl(scheme, encUser, encPass, hostAndRest); + const normalized = assembleSocksUrl(scheme, encUser, encPass, hostAndRest); + // Compare credentials, not the full URL: keeps the log condition focused + // on real encoding work, not cosmetic differences (parity with the Python + // implementation, which has to skip urlparse's hostname lowercasing). + const credsChanged = encUser !== rawUserEnc + || (hasPassword ? encPass !== rawPassEnc : false); + if (credsChanged) { + console.debug( + "[cloakbrowser] Auto URL-encoded SOCKS5 proxy credentials (special " + + "characters detected). Pre-encode the URL to suppress this notice.", + ); + } + return normalized; } catch (e) { console.warn(`[cloakbrowser] Could not normalize SOCKS5 proxy URL, passing through unchanged: ${(e as Error).message}`); return urlStr; diff --git a/js/tests/proxy.test.ts b/js/tests/proxy.test.ts index d8aaade..0ccfbbe 100644 --- a/js/tests/proxy.test.ts +++ b/js/tests/proxy.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect } from "vitest"; +import { describe, it, expect, vi } from "vitest"; import { parseProxyUrl, isSocksProxy, resolveProxyConfig } from "../src/proxy.js"; import type { LaunchOptions } from "../src/types.js"; @@ -262,4 +262,63 @@ describe("resolveProxyConfig", () => { const { proxyArgs } = resolveProxyConfig("socks5://user:a@b@c@host:1080"); expect(proxyArgs).toEqual(["--proxy-server=socks5://user:a%40b%40c@host:1080"]); }); + + // Visibility for #157: when wrapper actually rewrites the URL, surface a + // debug log so users debugging silent SOCKS5 fallback can see what happened. + it("logs debug message when SOCKS5 credentials get re-encoded", () => { + const debugSpy = vi.spyOn(console, "debug").mockImplementation(() => {}); + try { + resolveProxyConfig("socks5://user:pass=123@host:1080"); + expect(debugSpy).toHaveBeenCalledWith( + expect.stringContaining("Auto URL-encoded SOCKS5"), + ); + // Credentials must not leak into the log. + const calls = debugSpy.mock.calls.flat().join(" "); + expect(calls).not.toContain("pass=123"); + expect(calls).not.toContain("pass%3D123"); + } finally { + debugSpy.mockRestore(); + } + }); + + it("stays silent when SOCKS5 URL is already encoded (no log spam)", () => { + const debugSpy = vi.spyOn(console, "debug").mockImplementation(() => {}); + try { + resolveProxyConfig("socks5://user:pass%3D123@host:1080"); + const reencodedCalls = debugSpy.mock.calls + .flat() + .filter((arg) => typeof arg === "string" && arg.includes("Auto URL-encoded SOCKS5")); + expect(reencodedCalls).toHaveLength(0); + } finally { + debugSpy.mockRestore(); + } + }); + + it("stays silent when SOCKS5 URL has no credentials", () => { + const debugSpy = vi.spyOn(console, "debug").mockImplementation(() => {}); + try { + resolveProxyConfig("socks5://host:1080"); + const reencodedCalls = debugSpy.mock.calls + .flat() + .filter((arg) => typeof arg === "string" && arg.includes("Auto URL-encoded SOCKS5")); + expect(reencodedCalls).toHaveLength(0); + } finally { + debugSpy.mockRestore(); + } + }); + + it("stays silent when only host case differs (no credential rewrite)", () => { + // Parity with Python: log condition must track credential changes, not + // cosmetic URL-string differences (regression for Copilot's PR #209 review). + const debugSpy = vi.spyOn(console, "debug").mockImplementation(() => {}); + try { + resolveProxyConfig("socks5://USER:pass@HOST.com:1080"); + const reencodedCalls = debugSpy.mock.calls + .flat() + .filter((arg) => typeof arg === "string" && arg.includes("Auto URL-encoded SOCKS5")); + expect(reencodedCalls).toHaveLength(0); + } finally { + debugSpy.mockRestore(); + } + }); }); diff --git a/tests/test_proxy.py b/tests/test_proxy.py index 2e1fc9a..78cad3a 100644 --- a/tests/test_proxy.py +++ b/tests/test_proxy.py @@ -284,6 +284,42 @@ class TestResolveProxyConfig: _, args = _resolve_proxy_config("socks5://user:pass%3D123@host:1080") assert args == ["--proxy-server=socks5://user:pass%3D123@host:1080"] + def test_socks5_string_logs_info_when_reencoding(self, caplog): + # When wrapper actually rewrites the URL (e.g. unencoded '=' in pwd), + # surface an INFO log so users debugging SOCKS5 connectivity (#157) + # can see what the wrapper did instead of being silently surprised. + import logging + with caplog.at_level(logging.INFO, logger="cloakbrowser"): + _resolve_proxy_config("socks5://user:pass=123@host:1080") + assert any("Auto URL-encoded SOCKS5" in r.message for r in caplog.records) + # Credentials must not leak into the log. + for r in caplog.records: + assert "pass=123" not in r.message + assert "pass%3D123" not in r.message + + def test_socks5_string_silent_when_already_encoded(self, caplog): + # Idempotent path: pre-encoded URL produces no log noise. + import logging + with caplog.at_level(logging.INFO, logger="cloakbrowser"): + _resolve_proxy_config("socks5://user:pass%3D123@host:1080") + assert not any("Auto URL-encoded SOCKS5" in r.message for r in caplog.records) + + def test_socks5_string_silent_when_no_credentials(self, caplog): + # No userinfo at all → no encoding work → no log. + import logging + with caplog.at_level(logging.INFO, logger="cloakbrowser"): + _resolve_proxy_config("socks5://host:1080") + assert not any("Auto URL-encoded SOCKS5" in r.message for r in caplog.records) + + def test_socks5_string_silent_when_only_cosmetic_change(self, caplog): + # urlparse lowercases scheme and hostname, but credentials are + # untouched. The log must NOT fire for these cosmetic-only rewrites + # (regression for Copilot's review on PR #209). + import logging + with caplog.at_level(logging.INFO, logger="cloakbrowser"): + _resolve_proxy_config("socks5://USER:pass@HOST.com:1080") + assert not any("Auto URL-encoded SOCKS5" in r.message for r in caplog.records) + def test_socks5_string_no_creds_unchanged(self): _, args = _resolve_proxy_config("socks5://host:1080") assert args == ["--proxy-server=socks5://host:1080"]