From d6bad5de261bedf025280ace1d14e800aee13923 Mon Sep 17 00:00:00 2001 From: CloakHQ Date: Fri, 28 Aug 2026 02:18:20 +0200 Subject: [PATCH] fix(geoip): abort launch when resolution fails Increase the default GeoIP resolution timeout from 5 to 20 seconds across the Python, JavaScript, and .NET wrappers. Fail the launch when requested GeoIP resolution times out, cannot load its database, cannot complete a lookup, or leaves timezone or locale unresolved. Explicit timezone and locale overrides remain valid without an egress-IP result. Add parity coverage for the new timeout and failure behavior. --- cloakbrowser/browser.py | 5 ++ cloakbrowser/geoip.py | 18 +++---- dotnet/src/CloakBrowser/CloakLauncher.cs | 10 +++- dotnet/src/CloakBrowser/GeoIp.cs | 16 +++--- dotnet/tests/CloakBrowser.Tests/MiscTests.cs | 10 ++++ js/src/geoip.ts | 37 ++++++++----- js/tests/geoip.test.ts | 48 +++++++++++++++-- tests/test_geoip.py | 56 ++++++++++++++------ 8 files changed, 147 insertions(+), 53 deletions(-) diff --git a/cloakbrowser/browser.py b/cloakbrowser/browser.py index 1c90896..080e520 100644 --- a/cloakbrowser/browser.py +++ b/cloakbrowser/browser.py @@ -1327,6 +1327,11 @@ def maybe_resolve_geoip( timezone = geo_tz if locale is None: locale = geo_locale + missing = [name for name, value in (("timezone", timezone), ("locale", locale)) if value is None] + if missing: + raise RuntimeError( + "GeoIP resolution failed: could not determine " + " and ".join(missing) + ) return timezone, locale, exit_ip diff --git a/cloakbrowser/geoip.py b/cloakbrowser/geoip.py index 25b81a7..576f171 100644 --- a/cloakbrowser/geoip.py +++ b/cloakbrowser/geoip.py @@ -29,7 +29,7 @@ GEOIP_DB_URL = ( ) GEOIP_DB_FILENAME = "GeoLite2-City.mmdb" GEOIP_UPDATE_INTERVAL = 30 * 86_400 # 30 days -DEFAULT_GEOIP_TIMEOUT_SECONDS = 5.0 +DEFAULT_GEOIP_TIMEOUT_SECONDS = 20.0 GEOIP_TIMEOUT_ENV = "CLOAKBROWSER_GEOIP_TIMEOUT_SECONDS" # Serializes GeoIP DB downloads within a process so N concurrent launches @@ -80,8 +80,8 @@ COUNTRY_LOCALE_MAP: dict[str, str] = { def resolve_proxy_geo(proxy_url: str | None) -> tuple[str | None, str | None]: """Resolve timezone and locale from a proxy's IP address. - Returns ``(timezone, locale)`` — either or both may be ``None`` on - failure (missing dep, DB download error, lookup miss). Never raises. + Returns ``(timezone, locale)``. Raises when the exit IP, database, or + database lookup cannot be resolved. When *proxy_url* is falsy, the machine's own public IP is used instead (direct HTTP to the echo services, no proxy). @@ -127,12 +127,11 @@ def resolve_proxy_geo_with_ip( ip = _resolve_proxy_ip(proxy_url) if ip is None or _deadline_expired(deadline): if deadline is not None and _deadline_expired(deadline): - logger.warning("GeoIP resolution timed out after %.1fs; continuing without GeoIP", timeout) - return None, None, None + raise RuntimeError(f"GeoIP resolution timed out after {timeout:.1f}s") + raise RuntimeError("GeoIP resolution failed: could not discover the egress IP") - # DB only drives tz/locale; a missing/failed DB still returns the exit IP. if db_path is None: - return None, None, ip + raise RuntimeError("GeoIP resolution failed: GeoIP database is unavailable") try: with geoip2.database.Reader(str(db_path)) as reader: @@ -146,8 +145,7 @@ def resolve_proxy_geo_with_ip( ) return timezone, locale, ip except Exception as exc: - logger.warning("GeoIP lookup failed for %s: %s", ip, exc) - return None, None, ip + raise RuntimeError(f"GeoIP lookup failed for {ip}: {exc}") from exc # --------------------------------------------------------------------------- @@ -247,7 +245,7 @@ def resolve_proxy_exit_ip(proxy_url: str | None) -> str | None: deadline = _deadline_from_timeout(timeout) ip = _resolve_exit_ip(proxy_url, timeout=timeout) if ip is None and _deadline_expired(deadline): - logger.warning("GeoIP resolution timed out after %.1fs; continuing without GeoIP", timeout) + logger.warning("Proxy exit-IP resolution timed out after %.1fs", timeout) return ip diff --git a/dotnet/src/CloakBrowser/CloakLauncher.cs b/dotnet/src/CloakBrowser/CloakLauncher.cs index 05a69fd..c414109 100644 --- a/dotnet/src/CloakBrowser/CloakLauncher.cs +++ b/dotnet/src/CloakBrowser/CloakLauncher.cs @@ -287,7 +287,15 @@ public static class CloakLauncher } var (geoTz, geoLocale, exitIp) = await GeoIp.ResolveProxyGeoWithIpAsync(proxyUrl).ConfigureAwait(false); - return (timezone ?? geoTz, locale ?? geoLocale, exitIp); + timezone ??= geoTz; + locale ??= geoLocale; + var missing = new List(); + if (timezone == null) missing.Add("timezone"); + if (locale == null) missing.Add("locale"); + if (missing.Count > 0) + throw new InvalidOperationException( + $"GeoIP resolution failed: could not determine {string.Join(" and ", missing)}"); + return (timezone, locale, exitIp); } // ----------------------------------------------------------------------- diff --git a/dotnet/src/CloakBrowser/GeoIp.cs b/dotnet/src/CloakBrowser/GeoIp.cs index 219aa52..5bc1e11 100644 --- a/dotnet/src/CloakBrowser/GeoIp.cs +++ b/dotnet/src/CloakBrowser/GeoIp.cs @@ -23,7 +23,7 @@ public static class GeoIp // Serializes GeoIP DB downloads within the process so N concurrent launches // don't each fetch the same ~70 MB file (issue #458). Per-process only. private static readonly SemaphoreSlim GeoIpDownloadGate = new(1, 1); - private const double DefaultGeoIpTimeoutSeconds = 5.0; + private const double DefaultGeoIpTimeoutSeconds = 20.0; private const string GeoIpTimeoutEnv = "CLOAKBROWSER_GEOIP_TIMEOUT_SECONDS"; // IP echo services - fast, no auth, return just the IP. @@ -78,7 +78,7 @@ public static class GeoIp /// /// Resolve timezone and locale from a proxy's IP address. - /// Returns (timezone, locale) - either or both may be null on failure. Never throws. + /// Throws when the exit IP, database, or database lookup cannot be resolved. /// public static async Task<(string? Timezone, string? Locale)> ResolveProxyGeoAsync( string? proxyUrl, CancellationToken ct = default) @@ -116,13 +116,12 @@ public static class GeoIp if (ip == null || DeadlineExpired(deadline)) { if (deadline != null && DeadlineExpired(deadline)) - CloakLog.Warning("GeoIP resolution timed out after {0:0.0}s; continuing without GeoIP", timeout); - return (null, null, null); + throw new InvalidOperationException($"GeoIP resolution timed out after {timeout:0.0}s"); + throw new InvalidOperationException("GeoIP resolution failed: could not discover the egress IP"); } - // DB only drives tz/locale; a missing/failed DB still returns the exit IP. if (dbPath == null) - return (null, null, ip); + throw new InvalidOperationException("GeoIP resolution failed: GeoIP database is unavailable"); try { @@ -136,8 +135,7 @@ public static class GeoIp } catch (Exception exc) { - CloakLog.Warning("GeoIP lookup failed for {0}: {1}", ip, exc.Message); - return (null, null, ip); + throw new InvalidOperationException($"GeoIP lookup failed for {ip}: {exc.Message}", exc); } } @@ -228,7 +226,7 @@ public static class GeoIp var deadline = DeadlineFromTimeout(timeout); var ip = await ResolveExitIpAsync(proxyUrl, timeout, ct).ConfigureAwait(false); if (ip == null && DeadlineExpired(deadline)) - CloakLog.Warning("GeoIP resolution timed out after {0:0.0}s; continuing without GeoIP", timeout); + CloakLog.Warning("Proxy exit-IP resolution timed out after {0:0.0}s", timeout); return ip; } diff --git a/dotnet/tests/CloakBrowser.Tests/MiscTests.cs b/dotnet/tests/CloakBrowser.Tests/MiscTests.cs index f965d0e..d663cd0 100644 --- a/dotnet/tests/CloakBrowser.Tests/MiscTests.cs +++ b/dotnet/tests/CloakBrowser.Tests/MiscTests.cs @@ -29,6 +29,16 @@ public class GeoIpTests Assert.Equal("es-PE", GeoIp.CountryLocaleMap["PE"]); } + [Fact] + public void DefaultGeoIpTimeout_IsTwentySeconds() + { + var field = typeof(GeoIp).GetField( + "DefaultGeoIpTimeoutSeconds", + System.Reflection.BindingFlags.NonPublic | System.Reflection.BindingFlags.Static); + Assert.NotNull(field); + Assert.Equal(20.0, field.GetRawConstantValue()); + } + [Fact] public async Task MaybeResolveGeoIp_Disabled_ReturnsInputWithoutResolving() { diff --git a/js/src/geoip.ts b/js/src/geoip.ts index efdc3e2..be018f0 100644 --- a/js/src/geoip.ts +++ b/js/src/geoip.ts @@ -22,7 +22,8 @@ const GEOIP_DB_URL = "https://github.com/P3TERX/GeoLite.mmdb/raw/download/GeoLite2-City.mmdb"; const GEOIP_DB_FILENAME = "GeoLite2-City.mmdb"; const GEOIP_UPDATE_INTERVAL_MS = 30 * 86_400_000; // 30 days -const DEFAULT_GEOIP_TIMEOUT_MS = 5_000; +/** @internal Exported for parity testing. */ +export const DEFAULT_GEOIP_TIMEOUT_MS = 20_000; /** Country ISO code → BCP 47 locale (covers ~90% of proxy traffic). */ export const COUNTRY_LOCALE_MAP: Record = { @@ -72,8 +73,8 @@ export interface GeoResult { /** * Resolve timezone and locale from a proxy's IP address. - * Returns `{ timezone, locale }` — either may be null on failure. - * Never throws. + * Returns `{ timezone, locale }`. Throws when the exit IP, database, or + * database lookup cannot be resolved. * * When `proxyUrl` is falsy, the machine's own public IP is used instead * (echo services queried directly, no proxy), so geoip works proxy-free. @@ -108,13 +109,14 @@ export async function resolveProxyGeo( if (!ip && proxyUrl && !deadlineExpired(deadline)) ip = await resolveProxyIp(proxyUrl); if (!ip || deadlineExpired(deadline)) { if (deadlineExpired(deadline)) { - console.warn(`[cloakbrowser] GeoIP resolution timed out after ${timeoutMs}ms; continuing without GeoIP`); + throw new Error(`GeoIP resolution timed out after ${timeoutMs / 1000}s`); } - return { timezone: null, locale: null, exitIp: null }; + throw new Error("GeoIP resolution failed: could not discover the egress IP"); } - // DB only drives tz/locale; a missing/failed DB still returns the exit IP. - if (!dbPath) return { timezone: null, locale: null, exitIp: ip }; + if (!dbPath) { + throw new Error("GeoIP resolution failed: GeoIP database is unavailable"); + } try { const buf = fs.readFileSync(dbPath); @@ -125,8 +127,9 @@ export async function resolveProxyGeo( const locale = countryCode ? (COUNTRY_LOCALE_MAP[countryCode] ?? null) : null; return { timezone, locale, exitIp: ip }; - } catch { - return { timezone: null, locale: null, exitIp: ip }; + } catch (error) { + const detail = error instanceof Error ? error.message : String(error); + throw new Error(`GeoIP lookup failed for ${ip}: ${detail}`, { cause: error }); } } @@ -471,11 +474,19 @@ export async function maybeResolveGeoip( } const { timezone: geoTz, locale: geoLocale, exitIp: geoExitIp } = await resolveProxyGeo(proxyUrl); - const exitIp = geoExitIp ?? undefined; + const resolvedTimezone = timezone ?? geoTz ?? undefined; + const resolvedLocale = locale ?? geoLocale ?? undefined; + const missing = [ + resolvedTimezone ? null : "timezone", + resolvedLocale ? null : "locale", + ].filter((value): value is string => value !== null); + if (missing.length > 0) { + throw new Error(`GeoIP resolution failed: could not determine ${missing.join(" and ")}`); + } return { - timezone: timezone ?? geoTz ?? undefined, - locale: locale ?? geoLocale ?? undefined, - exitIp, + timezone: resolvedTimezone, + locale: resolvedLocale, + exitIp: geoExitIp ?? undefined, }; } diff --git a/js/tests/geoip.test.ts b/js/tests/geoip.test.ts index 21a1090..780a490 100644 --- a/js/tests/geoip.test.ts +++ b/js/tests/geoip.test.ts @@ -3,7 +3,13 @@ import fs from "node:fs"; import http from "node:http"; import os from "node:os"; import path from "node:path"; -import { COUNTRY_LOCALE_MAP, maybeResolveGeoip, resolveProxyGeo, resolveProxyIp } from "../src/geoip.js"; +import { + COUNTRY_LOCALE_MAP, + DEFAULT_GEOIP_TIMEOUT_MS, + maybeResolveGeoip, + resolveProxyGeo, + resolveProxyIp, +} from "../src/geoip.js"; import Stream from "node:stream"; const tempDirs: string[] = []; @@ -53,6 +59,10 @@ describe("resolveProxyIp", () => { }); describe("maybeResolveGeoip", () => { + it("uses a 20-second default timeout", () => { + expect(DEFAULT_GEOIP_TIMEOUT_MS).toBe(20_000); + }); + it("forwards separate HTTP proxy credentials to CONNECT requests", async () => { const authorizationHeaders: Array = []; const sockets = new Set(); @@ -109,13 +119,42 @@ describe("maybeResolveGeoip", () => { }), } as Response); - const result = await resolveProxyGeo("http://203.0.113.10:8080"); + await expect(resolveProxyGeo("http://203.0.113.10:8080")).rejects.toThrow( + "GeoIP resolution", + ); - expect(result).toEqual({ timezone: null, locale: null, exitIp: null }); expect(fetchSpy).toHaveBeenCalledOnce(); expect(fetchSpy.mock.calls[0][1]).toEqual({ redirect: "follow" }); }); + it("throws when the GeoIP database is unavailable", async () => { + const cacheDir = fs.mkdtempSync(path.join(os.tmpdir(), "cloak-geoip-db-failure-")); + tempDirs.push(cacheDir); + process.env.CLOAKBROWSER_CACHE_DIR = cacheDir; + + vi.spyOn(globalThis, "fetch") + .mockRejectedValueOnce(new Error("database unavailable")) + .mockResolvedValue({ ok: true, text: async () => "5.6.7.8" } as Response); + + await expect(resolveProxyGeo(null)).rejects.toThrow("database is unavailable"); + }); + + it("throws when the GeoIP database lookup fails", async () => { + const cacheDir = fs.mkdtempSync(path.join(os.tmpdir(), "cloak-geoip-lookup-failure-")); + tempDirs.push(cacheDir); + const geoipDir = path.join(cacheDir, "geoip"); + fs.mkdirSync(geoipDir, { recursive: true }); + fs.writeFileSync(path.join(geoipDir, "GeoLite2-City.mmdb"), "invalid database"); + process.env.CLOAKBROWSER_CACHE_DIR = cacheDir; + + vi.spyOn(globalThis, "fetch").mockResolvedValue({ + ok: true, + text: async () => "5.6.7.8", + } as Response); + + await expect(resolveProxyGeo(null)).rejects.toThrow("GeoIP lookup failed"); + }); + it("downloads the database once under concurrent first-use launches (#458)", async () => { const cacheDir = fs.mkdtempSync(path.join(os.tmpdir(), "cloak-geoip-concurrent-")); tempDirs.push(cacheDir); @@ -144,10 +183,11 @@ describe("maybeResolveGeoip", () => { ), ); - await Promise.all( + const results = await Promise.allSettled( Array.from({ length: 5 }, () => resolveProxyGeo("http://203.0.113.10:8080")), ); + expect(results.every(result => result.status === "rejected")).toBe(true); // Only one launch actually fetched the shared ~70 MB file. expect(fetchSpy).toHaveBeenCalledOnce(); }); diff --git a/tests/test_geoip.py b/tests/test_geoip.py index fbcf04d..a96087f 100644 --- a/tests/test_geoip.py +++ b/tests/test_geoip.py @@ -9,6 +9,7 @@ import pytest from cloakbrowser.browser import maybe_resolve_geoip from cloakbrowser.geoip import ( COUNTRY_LOCALE_MAP, + DEFAULT_GEOIP_TIMEOUT_SECONDS, _is_private_ip, _resolve_exit_ip, _resolve_proxy_ip, @@ -72,6 +73,10 @@ def test_locale_map_values_are_bcp47(): # --------------------------------------------------------------------------- +def test_default_geoip_timeout_is_twenty_seconds(): + assert DEFAULT_GEOIP_TIMEOUT_SECONDS == 20.0 + + def test_resolve_geo_raises_when_geoip2_missing(): """Should raise ImportError with install instructions when geoip2 not installed.""" with patch.dict("sys.modules", {"geoip2": None, "geoip2.database": None}): @@ -84,29 +89,38 @@ def test_resolve_geo_raises_when_geoip2_missing(): reload(geoip_mod) -def test_resolve_geo_returns_none_when_db_missing(): - """Should return (None, None) when DB file doesn't exist.""" +def test_resolve_geo_raises_when_exit_ip_missing(): + """Requested GeoIP must fail instead of launching without resolved values.""" mock_geoip2 = type("module", (), {"database": type("db", (), {"Reader": None})})() with patch.dict("sys.modules", {"geoip2": mock_geoip2, "geoip2.database": mock_geoip2.database}): - with patch("cloakbrowser.geoip._ensure_geoip_db", return_value=None): + with patch("cloakbrowser.geoip._ensure_geoip_db", return_value=object()): with patch("cloakbrowser.geoip._resolve_exit_ip", return_value=None): from cloakbrowser.geoip import resolve_proxy_geo - assert resolve_proxy_geo("http://10.50.96.5:8888") == (None, None) + with pytest.raises(RuntimeError, match="could not discover the egress IP"): + resolve_proxy_geo(None) -def test_resolve_geo_keeps_exit_ip_when_db_missing(): - """DB missing but IP resolvable → still return the exit IP for WebRTC spoofing. - - Resolving the egress IP does not need the GeoIP DB, so a DB download failure - must not drop it — otherwise WebRTC could fall back to the real IP behind a - proxy while the connection shows the proxy IP (a deanonymization). - """ +def test_resolve_geo_raises_when_db_missing(): + """A database failure must abort requested GeoIP resolution.""" mock_geoip2 = type("module", (), {"database": type("db", (), {"Reader": None})})() with patch.dict("sys.modules", {"geoip2": mock_geoip2, "geoip2.database": mock_geoip2.database}): with patch("cloakbrowser.geoip._ensure_geoip_db", return_value=None): with patch("cloakbrowser.geoip._resolve_exit_ip", return_value="9.8.7.6"): from cloakbrowser.geoip import resolve_proxy_geo_with_ip - assert resolve_proxy_geo_with_ip("http://10.50.96.5:8888") == (None, None, "9.8.7.6") + with pytest.raises(RuntimeError, match="database is unavailable"): + resolve_proxy_geo_with_ip("http://10.50.96.5:8888") + + +def test_resolve_geo_raises_when_lookup_fails(): + """A corrupt or unreadable database must abort requested GeoIP resolution.""" + reader = MagicMock(side_effect=ValueError("corrupt database")) + mock_geoip2 = type("module", (), {"database": type("db", (), {"Reader": reader})})() + with patch.dict("sys.modules", {"geoip2": mock_geoip2, "geoip2.database": mock_geoip2.database}): + with patch("cloakbrowser.geoip._ensure_geoip_db", return_value=object()): + with patch("cloakbrowser.geoip._resolve_exit_ip", return_value="9.8.7.6"): + from cloakbrowser.geoip import resolve_proxy_geo_with_ip + with pytest.raises(RuntimeError, match="corrupt database"): + resolve_proxy_geo_with_ip("http://10.50.96.5:8888") # --------------------------------------------------------------------------- @@ -259,20 +273,30 @@ def test_maybe_resolve_param_beats_raw_flag(): assert loc == "de-DE" -def test_maybe_resolve_geoip_timeout_returns_existing_values(monkeypatch): - """A stalled proxy lookup should not block launch indefinitely.""" +def test_maybe_resolve_geoip_timeout_aborts_launch(monkeypatch): + """A stalled requested GeoIP lookup should fail within its timeout budget.""" mock_geoip2 = type("module", (), {"database": type("db", (), {"Reader": None})})() monkeypatch.setenv("CLOAKBROWSER_GEOIP_TIMEOUT_SECONDS", "0.05") with patch.dict("sys.modules", {"geoip2": mock_geoip2, "geoip2.database": mock_geoip2.database}): with patch("cloakbrowser.geoip._ensure_geoip_db", return_value=object()): start = time.monotonic() - tz, loc, ip = maybe_resolve_geoip(True, "http://203.0.113.10:8080", None, "fr-FR") + with pytest.raises(RuntimeError, match="GeoIP resolution"): + maybe_resolve_geoip(True, "http://203.0.113.10:8080", None, "fr-FR") elapsed = time.monotonic() - start - assert (tz, loc, ip) == (None, "fr-FR", None) assert elapsed < 0.5 +def test_maybe_resolve_geoip_rejects_incomplete_result(): + """A partial result must not silently leave Chromium defaults in use.""" + with patch( + "cloakbrowser.geoip.resolve_proxy_geo_with_ip", + return_value=("Europe/Berlin", None, "5.6.7.8"), + ): + with pytest.raises(RuntimeError, match="could not determine locale"): + maybe_resolve_geoip(True, "http://proxy:8080", None, None) + + # --------------------------------------------------------------------------- # _is_private_ip # ---------------------------------------------------------------------------