diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5e2762d01..879c6e37f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -168,6 +168,7 @@ jobs: os: macos-15 ERROR_ON_WARNINGS: 1 SYSTEM_VERSION_COMPAT: 0 + TEST_SYSTEM_PROXY: 1 - name: macOS 26 (xcode llvm) os: macos-26 ERROR_ON_WARNINGS: 1 @@ -194,6 +195,7 @@ jobs: TEST_X86: 1 - name: Windows (latest) os: windows-latest + TEST_SYSTEM_PROXY: 1 - name: Windows (MSVC analyzer) os: windows-2022 RUN_ANALYZER: msvc @@ -259,6 +261,7 @@ jobs: TEST_WINE: ${{ matrix.TEST_WINE }} TEST_ARM32: ${{ matrix.TEST_ARM32 }} TEST_QEMU: ${{ matrix.TEST_QEMU }} + TEST_SYSTEM_PROXY: ${{ matrix.TEST_SYSTEM_PROXY }} USE_SCCACHE: ${{ matrix.USE_SCCACHE }} SCCACHE_GHA_ENABLED: ${{ matrix.USE_SCCACHE && 'true' || '' }} ERROR_ON_WARNINGS: ${{ matrix.ERROR_ON_WARNINGS }} diff --git a/CHANGELOG.md b/CHANGELOG.md index 8c9349c0f..4fa27e456 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## Unreleased + +**Fixes**: + +- An empty proxy (`sentry_options_set_proxy(options, "")`) now means a direct connection with every transport and backend. Before, the WinHTTP transport, the Windows and macOS crashpad handlers and the native backend's daemon fell back to the proxy environment variables or the system proxy settings. ([#2218](https://github.com/getsentry/sentry-native/pull/2218), [crashpad#175](https://github.com/getsentry/crashpad/pull/175)) + ## 0.17.4 **Features**: diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 1b55a68af..37639d00d 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -126,6 +126,9 @@ Some of its behavior is controlled by env-variables: switches to the Ninja generator (`choco install ninja`) and uses `/Z7` debug info format for compatibility. - `TEST_X86`: Passes flags to CMake to enable a 32-bit (cross-)compile. +- `TEST_SYSTEM_PROXY`: Runs the tests that point the user's system proxy + settings at a local test proxy, and restores them afterwards. On macOS this + needs passwordless `sudo`. - `ANDROID_API` / `ANDROID_NDK` / `ANDROID_ARCH`: Instructs the test runner to build using the given Android `NDK` version, targeting the given `API` and `ARCH`. The test runner assumes an already running simulator matching the diff --git a/external/crashpad b/external/crashpad index 493151b22..0bd6ca43a 160000 --- a/external/crashpad +++ b/external/crashpad @@ -1 +1 @@ -Subproject commit 493151b224a6536bcedc3467d042863a063addc4 +Subproject commit 0bd6ca43a0958a86939a37569b61162f92f9a022 diff --git a/include/sentry.h b/include/sentry.h index 6205af91e..29ae701fd 100644 --- a/include/sentry.h +++ b/include/sentry.h @@ -1777,6 +1777,10 @@ SENTRY_API const char *sentry_options_get_dist(const sentry_options_t *opts); * transport on macOS for a SOCKS proxy, but not for an HTTP proxy. * All transports that use libcurl (Linux and the Native SDK transport on macOS) * will honor the proxy settings and not fall back. + * + * An empty string disables the proxy: no transport then reads the + * `http_proxy` and `https_proxy` environment variables or the system proxy + * settings. */ SENTRY_API void sentry_options_set_proxy( sentry_options_t *opts, const char *proxy); diff --git a/src/backends/native/sentry_crash_context.h b/src/backends/native/sentry_crash_context.h index 1f840aa1d..b0d5543ef 100644 --- a/src/backends/native/sentry_crash_context.h +++ b/src/backends/native/sentry_crash_context.h @@ -337,6 +337,7 @@ typedef struct { // Transport configuration (passed from parent options to daemon) char ca_certs[SENTRY_CRASH_MAX_PATH]; // CA certificates file path for SSL char proxy[SENTRY_CRASH_MAX_PATH]; // HTTP proxy URL + bool has_proxy; // Proxy was set, possibly to "" to disable it char user_agent[256]; // User-Agent header for HTTP requests // Minidump output path (filled by daemon) diff --git a/src/backends/native/sentry_crash_daemon.c b/src/backends/native/sentry_crash_daemon.c index f96085529..96f6a4f05 100644 --- a/src/backends/native/sentry_crash_daemon.c +++ b/src/backends/native/sentry_crash_daemon.c @@ -4611,7 +4611,7 @@ sentry__crash_daemon_main(pid_t app_pid, uint64_t app_tid, HANDLE event_handle, sentry_options_set_ca_certs(options, ipc->shmem->ca_certs); } - if (ipc->shmem->proxy[0] != '\0') { + if (ipc->shmem->has_proxy) { SENTRY_DEBUGF("Setting proxy: %s", ipc->shmem->proxy); sentry_options_set_proxy(options, ipc->shmem->proxy); } diff --git a/src/backends/sentry_backend_crashpad.cpp b/src/backends/sentry_backend_crashpad.cpp index 1f7967b0a..08c9a3a3c 100644 --- a/src/backends/sentry_backend_crashpad.cpp +++ b/src/backends/sentry_backend_crashpad.cpp @@ -1132,12 +1132,11 @@ crashpad_backend_startup( const char *proxy_url = options->proxy ? options->proxy : env_proxy ? env_proxy : ""; -#ifdef SENTRY_PLATFORM_LINUX - // explicitly set an empty proxy to avoid reading from env. vars. on Linux + // explicitly set an empty proxy to avoid reading from env. vars. or the + // system proxy settings if (options->proxy && strcmp(options->proxy, "") == 0) { proxy_url = ""; } -#endif bool success = data->client->StartHandler(handler, database, database, minidump_url ? minidump_url : "", proxy_url, annotations, arguments, /* restartable */ true, diff --git a/src/backends/sentry_backend_native.c b/src/backends/sentry_backend_native.c index ff8f97e3d..2cf53feaa 100644 --- a/src/backends/sentry_backend_native.c +++ b/src/backends/sentry_backend_native.c @@ -826,6 +826,7 @@ native_backend_startup( } if (options->proxy) { + ctx->has_proxy = true; #ifdef _WIN32 strncpy_s(ctx->proxy, sizeof(ctx->proxy), options->proxy, _TRUNCATE); #else diff --git a/src/transports/sentry_http_transport_winhttp.c b/src/transports/sentry_http_transport_winhttp.c index cc9db472d..7562585f1 100644 --- a/src/transports/sentry_http_transport_winhttp.c +++ b/src/transports/sentry_http_transport_winhttp.c @@ -122,6 +122,10 @@ winhttp_client_start(void *_client, const sentry_options_t *opts) client->session = WinHttpOpen(user_agent, WINHTTP_ACCESS_TYPE_NAMED_PROXY, client->proxy, WINHTTP_NO_PROXY_BYPASS, 0); + } else if (opts->proxy && opts->proxy[0] == '\0') { + // an explicitly empty proxy disables proxy discovery + client->session = WinHttpOpen(user_agent, WINHTTP_ACCESS_TYPE_NO_PROXY, + WINHTTP_NO_PROXY_NAME, WINHTTP_NO_PROXY_BYPASS, 0); } else { #if _WIN32_WINNT >= 0x0603 client->session diff --git a/tests/conditions.py b/tests/conditions.py index 12dd22f66..04e21514f 100644 --- a/tests/conditions.py +++ b/tests/conditions.py @@ -8,6 +8,8 @@ is_x86 = os.environ.get("TEST_X86") is_arm32 = bool(os.environ.get("TEST_ARM32")) is_qemu = bool(os.environ.get("TEST_QEMU")) +# opt-in because the tests change the user's system proxy settings (with sudo on macOS) +has_system_proxy = bool(os.environ.get("TEST_SYSTEM_PROXY")) is_asan = "asan" in os.environ.get("RUN_ANALYZER", "") is_tsan = "tsan" in os.environ.get("RUN_ANALYZER", "") is_kcov = "kcov" in os.environ.get("RUN_ANALYZER", "") diff --git a/tests/proxy.py b/tests/proxy.py index ac215e719..155e52775 100644 --- a/tests/proxy.py +++ b/tests/proxy.py @@ -31,6 +31,165 @@ def cleanup_proxy_env_vars(): os.environ.pop("https_proxy", None) +@contextlib.contextmanager +def system_proxy(port): + """Set the current user's system HTTP proxy to the test proxy and restore + the previous settings afterwards. WinHTTP proxy discovery and NSURLSession + read these settings, unlike the environment variables.""" + if sys.platform == "win32": + with _wininet_proxy(f"127.0.0.1:{port}"): + yield + elif sys.platform == "darwin": + with _networksetup_proxy(port): + yield + else: + pytest.skip("no system proxy settings on this platform") + + +@contextlib.contextmanager +def _wininet_proxy(server): + import ctypes + from ctypes import wintypes + + INTERNET_OPTION_REFRESH = 37 + INTERNET_OPTION_SETTINGS_CHANGED = 39 + INTERNET_OPTION_PER_CONNECTION_OPTION = 75 + INTERNET_PER_CONN_FLAGS = 1 + INTERNET_PER_CONN_PROXY_SERVER = 2 + PROXY_TYPE_DIRECT = 0x1 + PROXY_TYPE_PROXY = 0x2 + + class Value(ctypes.Union): + _fields_ = [ + ("dwValue", wintypes.DWORD), + ("pszValue", wintypes.LPWSTR), + ("pValue", ctypes.c_void_p), + ("ftValue", wintypes.FILETIME), + ] + + class Option(ctypes.Structure): + _fields_ = [("dwOption", wintypes.DWORD), ("Value", Value)] + + class OptionList(ctypes.Structure): + _fields_ = [ + ("dwSize", wintypes.DWORD), + ("pszConnection", wintypes.LPWSTR), + ("dwOptionCount", wintypes.DWORD), + ("dwOptionError", wintypes.DWORD), + ("pOptions", ctypes.POINTER(Option)), + ] + + wininet = ctypes.WinDLL("wininet", use_last_error=True) + wininet.InternetQueryOptionW.argtypes = [ + ctypes.c_void_p, + wintypes.DWORD, + ctypes.c_void_p, + ctypes.POINTER(wintypes.DWORD), + ] + wininet.InternetSetOptionW.argtypes = [ + ctypes.c_void_p, + wintypes.DWORD, + ctypes.c_void_p, + wintypes.DWORD, + ] + kernel32 = ctypes.WinDLL("kernel32") + kernel32.GlobalFree.argtypes = [ctypes.c_void_p] + + def make_list(flags=None, proxy_server=None): + options = (Option * 2)() + options[0].dwOption = INTERNET_PER_CONN_FLAGS + options[0].Value.dwValue = flags or 0 + options[1].dwOption = INTERNET_PER_CONN_PROXY_SERVER + options[1].Value.pszValue = proxy_server + option_list = OptionList(ctypes.sizeof(OptionList), None, 2, 0, options) + return option_list, options + + def set_options(flags, proxy_server): + option_list, _ = make_list(flags, proxy_server) + if not wininet.InternetSetOptionW( + None, + INTERNET_OPTION_PER_CONNECTION_OPTION, + ctypes.byref(option_list), + ctypes.sizeof(option_list), + ): + raise ctypes.WinError(ctypes.get_last_error()) + wininet.InternetSetOptionW(None, INTERNET_OPTION_SETTINGS_CHANGED, None, 0) + wininet.InternetSetOptionW(None, INTERNET_OPTION_REFRESH, None, 0) + + option_list, options = make_list() + size = wintypes.DWORD(ctypes.sizeof(option_list)) + if not wininet.InternetQueryOptionW( + None, + INTERNET_OPTION_PER_CONNECTION_OPTION, + ctypes.byref(option_list), + ctypes.byref(size), + ): + raise ctypes.WinError(ctypes.get_last_error()) + previous_flags = options[0].Value.dwValue + previous_server = options[1].Value.pszValue + if options[1].Value.pValue: + kernel32.GlobalFree(options[1].Value.pValue) + + set_options(PROXY_TYPE_DIRECT | PROXY_TYPE_PROXY, server) + try: + yield + finally: + set_options(previous_flags, previous_server) + + +@contextlib.contextmanager +def _networksetup_proxy(port): + if subprocess.run(["sudo", "-n", "true"]).returncode != 0: + pytest.skip("setting the system proxy needs sudo") + + services = [ + service + for service in subprocess.check_output( + ["networksetup", "-listallnetworkservices"], text=True + ).splitlines()[1:] + if not service.startswith("*") + ] + previous = {} + for service in services: + output = subprocess.check_output( + ["networksetup", "-getwebproxy", service], text=True + ) + previous[service] = dict( + line.split(": ", 1) for line in output.splitlines() if ": " in line + ) + + try: + for service in services: + subprocess.check_call( + [ + "sudo", + "networksetup", + "-setwebproxy", + service, + "127.0.0.1", + str(port), + ] + ) + yield + finally: + for service, settings in previous.items(): + if settings.get("Server"): + subprocess.check_call( + [ + "sudo", + "networksetup", + "-setwebproxy", + service, + settings["Server"], + settings["Port"], + ] + ) + if settings.get("Enabled") != "Yes": + subprocess.check_call( + ["sudo", "networksetup", "-setwebproxystate", service, "off"] + ) + + def _wait_for_output_file(process, output_file, timeout=10): deadline = time.monotonic() + timeout while time.monotonic() < deadline: diff --git a/tests/test_integration_crashpad.py b/tests/test_integration_crashpad.py index e3b488606..c7e3620f6 100644 --- a/tests/test_integration_crashpad.py +++ b/tests/test_integration_crashpad.py @@ -21,12 +21,13 @@ lib_name, REPLAY_ID, ) -from .conditions import has_crashpad, has_files, has_oom +from .conditions import has_crashpad, has_files, has_oom, has_system_proxy from .proxy import ( setup_proxy_env_vars, cleanup_proxy_env_vars, start_proxy, proxy_test_finally, + system_proxy, ) from .assertions import ( assert_breadcrumb, @@ -305,6 +306,39 @@ def test_crashpad_proxy_set_empty(cmake, httpserver): proxy_test_finally(1, httpserver, proxy_process, expected_proxy_logsize=0) +@pytest.mark.skipif(not has_system_proxy, reason="needs TEST_SYSTEM_PROXY") +@pytest.mark.skipif( + sys.platform not in ["win32", "darwin"] or bool(os.environ.get("TEST_MINGW")), + reason="system proxy discovery requires macOS or non-MinGW Windows builds", +) +@pytest.mark.parametrize("proxy_empty", [False, True]) +def test_crashpad_proxy_system(cmake, httpserver, proxy_empty): + proxy_process = None # store the proxy process to terminate it later + try: + env, proxy_process, tmp_path, port = _setup_crashpad_proxy_test( + cmake, httpserver, "http-proxy" + ) + + with system_proxy(port): + with httpserver.wait(timeout=10) as waiting: + run( + tmp_path, + "sentry_test_integration", + ["log", "crash"] + (["proxy-empty"] if proxy_empty else []), + expect_failure=True, + env=env, + ) + assert waiting.result + + finally: + proxy_test_finally( + 1, + httpserver, + proxy_process, + expected_proxy_logsize=0 if proxy_empty else None, + ) + + def test_crashpad_proxy_https_not_http(cmake, httpserver): proxy_process = None # store the proxy process to terminate it later # we start the proxy but expect it to remain unused (dsn is http, so shouldn't use https proxy) diff --git a/tests/test_integration_native.py b/tests/test_integration_native.py index b97d1c44e..70257cc03 100644 --- a/tests/test_integration_native.py +++ b/tests/test_integration_native.py @@ -44,8 +44,9 @@ wait_for_file, assert_user_feedback, ) -from .conditions import has_files, is_kcov +from .conditions import has_files, has_system_proxy, is_kcov from .conditions import has_native, has_oom, is_asan, is_tsan, is_qemu, is_wine +from .proxy import proxy_test_finally, start_proxy, system_proxy pytestmark = pytest.mark.skipif( not has_native or is_qemu, @@ -1846,3 +1847,67 @@ def test_native_symbolication(cmake, httpserver, crash_mode, symbolize): assert any( "function" in frame for frame in exc["stacktrace"]["frames"] ), "missing symbolicated function names" + + +def test_native_proxy_set_empty(cmake, httpserver): + proxy_process = None # store the proxy process to terminate it later + try: + proxy_process, port = start_proxy("http-proxy") + tmp_path = cmake(["sentry_test_integration"], {"SENTRY_BACKEND": "native"}) + httpserver.expect_oneshot_request("/api/123456/envelope/").respond_with_data( + "OK" + ) + # we start the proxy but expect it to remain unused + env = dict( + os.environ, + SENTRY_DSN=make_dsn(httpserver, proxy_host=True), + http_proxy=f"http://127.0.0.1:{port}", + https_proxy=f"http://127.0.0.1:{port}", + ) + + with httpserver.wait(timeout=10) as waiting: + run_crash( + tmp_path, + "sentry_test_integration", + ["log", "crash", "proxy-empty"], + env=env, + ) + assert waiting.result + + finally: + proxy_test_finally(1, httpserver, proxy_process, expected_proxy_logsize=0) + + +@pytest.mark.skipif(not has_system_proxy, reason="needs TEST_SYSTEM_PROXY") +@pytest.mark.skipif( + sys.platform != "win32" or is_wine, + reason="only the WinHTTP transport reads the system proxy", +) +@pytest.mark.parametrize("proxy_empty", [False, True]) +def test_native_proxy_system(cmake, httpserver, proxy_empty): + proxy_process = None # store the proxy process to terminate it later + try: + proxy_process, port = start_proxy("http-proxy") + tmp_path = cmake(["sentry_test_integration"], {"SENTRY_BACKEND": "native"}) + httpserver.expect_oneshot_request("/api/123456/envelope/").respond_with_data( + "OK" + ) + env = dict(os.environ, SENTRY_DSN=make_dsn(httpserver, proxy_host=True)) + + with system_proxy(port): + with httpserver.wait(timeout=10) as waiting: + run_crash( + tmp_path, + "sentry_test_integration", + ["log", "crash"] + (["proxy-empty"] if proxy_empty else []), + env=env, + ) + assert waiting.result + + finally: + proxy_test_finally( + 1, + httpserver, + proxy_process, + expected_proxy_logsize=0 if proxy_empty else None, + ) diff --git a/tests/test_integration_proxy.py b/tests/test_integration_proxy.py index 72f536e88..c846ee0a3 100644 --- a/tests/test_integration_proxy.py +++ b/tests/test_integration_proxy.py @@ -10,11 +10,12 @@ from .assertions import ( assert_failed_proxy_auth_request, ) -from .conditions import has_http, is_wine +from .conditions import has_http, has_system_proxy, is_wine from .proxy import ( closed_port, start_proxy, proxy_test_finally, + system_proxy, ) pytestmark = pytest.mark.skipif(not has_http, reason="tests need http") @@ -171,6 +172,36 @@ def test_proxy_set_empty(cmake, httpserver): proxy_test_finally(1, httpserver, proxy_process, expected_proxy_logsize=0) +@pytest.mark.skipif(not has_system_proxy, reason="needs TEST_SYSTEM_PROXY") +@pytest.mark.skipif( + sys.platform != "win32" or is_wine, + reason="only the WinHTTP transport reads the system proxy", +) +@pytest.mark.parametrize("proxy_empty", [False, True]) +def test_proxy_system(cmake, httpserver, proxy_empty): + proxy_process = None # store the proxy process to terminate it later + try: + env, proxy_process, tmp_path, port = _setup_http_proxy_test( + cmake, httpserver, "http-proxy" + ) + + with system_proxy(port): + run( + tmp_path, + "sentry_test_integration", + ["log", "capture-event"] + (["proxy-empty"] if proxy_empty else []), + env=env, + ) + + finally: + proxy_test_finally( + 1, + httpserver, + proxy_process, + expected_proxy_logsize=0 if proxy_empty else None, + ) + + def test_proxy_https_not_http(cmake, httpserver): proxy_process = None # store the proxy process to terminate it later try: