fix(gix-transport): honor proxy configuration with reqwest - #3017
Draft
Sebastian Thiel (Byron) wants to merge 2 commits into
Draft
Sebastian Thiel (Byron) wants to merge 2 commits into
Sebastian Thiel (Byron) wants to merge 2 commits into
Conversation
Member
Author
|
Supersedes #3016. Codex |
The blocking `reqwest` backend received `http::Options` but constructed its client before seeing them. A configured `http.proxy` was silently ignored, allowing direct requests even when only the proxy could reach the destination. The regression fails with a direct DNS error against `git.invalid` before this change. Configure the client from the effective proxy and bypass list, preserving connection reuse until that configuration or its credentials change. Explicit configuration overrides the environment, an empty proxy disables proxying, and `no_proxy=*` also bypasses numeric addresses. Preserve the configuration when restarting a failed worker so invalid proxy settings cannot turn into a direct connection on retry. Select environment proxies for each actual request URL, after request hooks and accepted redirects. Explicit proxy configuration still applies to both schemes. Use Git/libcurl's documented port 1080 for HTTP proxies without a port, preserving explicit ports and the usual HTTPS default. For `reqwest`, leave imported environment proxy values out of explicit transport overrides so redirects can select the destination scheme. Keep `curl`'s existing setup. Prepare credential helpers for the imported proxy candidates so username-only environment URLs still obtain the matching password, including after a redirect. Preserve explicit empty settings while treating empty environment values as absent during fallback. Validate environment proxies only when their destination scheme is actually requested, including redirects and URL-changing request hooks. An unused scheme's proxy must not break a valid request, and a configured global bypass must ignore disabled proxies entirely. Reuse `reqwest` for proxy routing, Basic credentials and SOCKS, and use the existing credential helper callback to obtain, approve or reject proxy credentials. Reject unsupported schemes, Unix socket paths, SOCKS4 user IDs (which reqwest otherwise silently drops), and explicit authentication methods that `reqwest` cannot honor. Document its preemptive Basic handling of `anyauth` and its current requirement for a TLS feature when using SOCKS. `reqwest` strips proxy authentication on authority-changing redirects without reapplying it. Execute each accepted hop separately, retaining the existing redirect safety policy and reapplying proxy authentication for that destination. Preserve POST-to-GET conversion, replay buffered 307/308 bodies, and stop when streamed bodies cannot be replayed. Do not carry origin credentials across authorities or proxy headers to bypassed origins. Reuse helper credentials while the selected proxy stays the same. Preserve explicit `Authorization` headers over credentials embedded in the origin URL, matching the previous request-builder precedence. Use the bypass matcher from `reqwest`'s existing `hyper-util` dependency before validating or authenticating a proxy. A bypassed destination must not need proxy credentials or store unvalidated ones. This introduces a direct dependency but no new packages. Close streamed upload bodies before reporting proxy setup errors, so the writer and header reader cannot block waiting for each other. Approve proxy credentials even when the origin returns an error, reject them only for a proxy authentication response (407), and leave them alone when a connection fails without a response. Regression tests cover the redirect, default-port and credential rejection findings from review. Include HTTPS CONNECT rejections: `reqwest` hides their status and hyper-util does not export its error type, so match the specific source message until a typed API becomes available. Test 407, other tunnel failures and dropped connections to protect that distinction. Git reference: `http.c` and `t/t5564-http-proxy.sh` at `d38352cd43ab9745686d697872408bc3249a153f`. The recording-proxy regression also runs `git ls-remote` with Git 2.54.0 (Apple Git-157) and verifies the same absolute request target. Routing, bypass, environment precedence, inline authentication and SOCKS tests are shared with the curl backend; the credential-helper regression is specific to reqwest because `curl` currently loses the helper password when its proxy URL has a username. Validation: - Reproduced the issue with the failing recording-proxy test first. - Regressions cover unused proxy settings, global bypass of invalid settings, SOCKS4 user IDs, request-hook URL changes, environment helper selection, authenticated redirects, buffered/streamed POST redirects, explicit authorization precedence, and upload setup failures. - Transport tests with plain reqwest, Rust TLS and native TLS. - The complete curl HTTP transport integration suite, including HTTPS proxy selection after redirects. - Repository transport-option tests with both reqwest and curl, including the imported-environment regression from review. - `cargo fmt --all -- --check` and transport documentation generation. - Library Clippy with `--no-deps -- -D warnings`; test Clippy completes with existing warnings in the older transport tests.
Libcurl prioritizes user information in `http.proxy` over `CURLOPT_PROXYUSERNAME` and `CURLOPT_PROXYPASSWORD`. A URL such as `http://user@proxy` therefore discarded the helper's password and sent `user:` instead. Like Git, remove URL credentials before passing the proxy to libcurl when a helper is configured, and keep using the dedicated credential options. Preserve explicit empty proxies and use `gix-url` to retain the proxy scheme, host, port and path. Run the existing recording-proxy credential check against both backends. It reproduced the empty password with curl before the fix; reqwest's helper credentials already worked for HTTP, HTTPS CONNECT, and environment-selected proxies. Validation: 39 curl and 46 reqwest transport tests passed with Rust TLS, along with formatting and strict library Clippy. Clippy reports the existing removed-lint warning.
Sebastian Thiel (Byron)
force-pushed
the
http-proxy-reqwest
branch
from
September 25, 2026 02:34
638e64c to
be62dd5
Compare
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.
Tasks
This section is for Byron only. Models continuing this PR must not add, remove, check, uncheck, rename, or reorder checkboxes here.
Everything below this line was generated by
Codex GPT-6.Created by Codex on behalf of Byron. Byron will review before this is ready to merge.
Fixes #3015.
The blocking
reqwestbackend acceptedhttp.proxybut could send the request directly. Build and reuse its client from the selected proxy and bypass settings, including explicit empty overrides and environment fallback.Use reqwest's existing HTTP/HTTPS and SOCKS proxy support and the existing credential callback; no new packages or public options are needed. A direct dependency on reqwest's existing
hyper-utilprovides its bypass matcher, so bypassed destinations skip proxy validation and credential lookup. Each accepted redirect selects its destination's proxy and reapplies proxy authentication. Origin credentials stay within their authority, bypassed origins receive no proxy credentials, and POST redirects retain the existing body rules. Environment-selected proxies keep their URL-specific credential helpers. Explicit origin authorization headers retain precedence over URL credentials, and setup errors release streamed uploads. Curl's configuration path is preserved.Unsupported proxy schemes, Unix socket paths, SOCKS4 user IDs, and explicit Digest/Negotiate/NTLM authentication fail instead of silently ignoring the setting. The documented reqwest limits remain:
AnyAuthandBasicuse preemptive Basic authentication, and SOCKS currently requires a TLS-enabled build. Unused environment proxy errors are reported only if a request selects that scheme. Origin errors do not reject valid proxy credentials; CONNECT 407 handling checks the preserved source message because reqwest does not expose a typed status.Recording-proxy tests run against both backends and compare the request target with
git ls-remoteusing Git 2.54.0 (Apple Git-157). Git behavior was checked againsthttp.candt/t5564-http-proxy.shatd38352cd43ab9745686d697872408bc3249a153f; omitted HTTP proxy ports use the documented libcurl default of 1080. The wire-level helper test is reqwest-specific because curl currently loses the helper password when its proxy URL contains a username; repository helper-configuration tests cover both backends.Validation:
--no-deps -- -D warnings. Existing repository/test lint warnings remain unchanged.ef61e17was attempted once but interrupted by the account usage limit; the final revision has the local validation listed above.