proxy: unify GUI-saved proxy as the single source of truth for HTTP#149
Merged
Conversation
Restore the matrix-sdk security defaults (TLS 1.2 minimum, matrix-rust-sdk user agent) that were lost when build_client switched from .proxy(url) to .http_client(custom). Make the same policy reach every HTTP call site — session restore, media download, homeserver discovery, SSO pre-build, the TSP worker, and the updater check — so GUI state can't drift from what sliding sync actually uses. Plumb a CLI --proxy override stash through proxy_config::CLI_PROXY_OVERRIDE so restore_session and DownloadAndSaveFile honor it the same way build_client does, instead of silently dropping it. Invalidate the DEFAULT_SSO_CLIENT cache when its baked-in proxy no longer matches the current effective proxy — previously, clearing the proxy after boot left the cached client routing SSO through the deleted proxy. Drop socks5/socks5h from validate_proxy_url: reqwest is not compiled with the socks feature, so accepting them in the GUI just promises connectivity the runtime can't deliver. http/https only until the feature is enabled. Harden save_proxy_url: hold the env lock across the file write so two concurrent saves cannot leave proxy_state.json and the process env disagreeing, and recover the lock automatically when a prior panic poisoned it instead of permanently disabling proxy management. Match build_client's 60s reqwest timeout in restore_session so restored sessions don't hang indefinitely on a slow CONNECT/TLS phase. Spec specs/task-proxy-policy.spec.md captures the contract: proxy_state.json wins over inherited env, null means forced direct, and one helper builds the reqwest client for every Robrix HTTP path.
# Conflicts: # src/persistence/matrix_state.rs # src/sliding_sync.rs
…ource of truth
Replace the env-var-mutating proxy approach with explicit reqwest builder
configuration so every HTTP construction site routes through one helper
(`build_policy_reqwest_client`) with predictable, testable behavior.
Why:
- The previous design wrote/cleared `http_proxy` / `https_proxy` etc. on the
process, which is fragile (shell-launch vs IDE-launch behavior diverges)
and leaks into uncontrolled subprocesses and third-party crates.
- Worse, env-var proxy was applied indiscriminately to ALL homeservers
including LAN addresses, which was the root cause of restore_session
failures for users running a self-hosted homeserver while a system proxy
was active.
What changed:
- `proxy_config.rs`: remove `PROXY_ENV_LOCK`, `apply_proxy_to_process_env`,
`build_env_proxy_config`, and the `robius_proxy` dependency. The new
helpers (`build_policy_reqwest_client`, `apply_policy_to_reqwest_builder`,
`build_reqwest_proxy`) call `.proxy(...)` explicitly when the GUI has a
proxy and `.no_proxy()` when it doesn't — env vars are never consulted.
- `sliding_sync.rs`, `persistence/matrix_state.rs`: build_client and
restore_session now go through `build_policy_reqwest_client` and pass the
result via `.http_client(http_client)` on the Matrix SDK builder.
- `tsp/mod.rs`: TSP uses a different reqwest version than matrix-sdk, so it
mirrors the same policy via `build_tsp_reqwest_proxy` /
`apply_tsp_proxy_policy` plus TLS 1.2 minimum.
- `updater.rs`: wraps the shared helper as `build_updater_http_client` with
a 10s timeout.
- `Cargo.toml`/`Cargo.lock`: drop `robius-proxy` (now dead code).
- `validate_proxy_url`: socks5/socks5h schemes are no longer accepted
(reqwest is not compiled with the `socks` feature).
- Spec updated to reflect "GUI is the single source of truth; no env mutation".
Tests:
- proxy_config: 6 unit tests cover policy UA, scheme rejection, save/load
round-trip, no-proxy when GUI empty, loopback bypass on explicit proxy.
- tsp: 2 unit tests cover mirrored policy.
- sliding_sync: discovery_http_client_{accepts_valid,rejects_invalid}_proxy_override.
- updater: updater_http_client_disables_system_proxy_when_proxy_is_none.
…y direct When a user has Clash (or any other public HTTP proxy) saved in Robrix and also connects to a self-hosted homeserver on the LAN (e.g. Palpo at 192.168.1.58), the previous loopback-only bypass routed the LAN request through the public proxy, which almost always fails to forward to RFC 1918 addresses. The user had to choose between "proxy on for matrix.org" OR "proxy off for LAN" — not both. Expand `DEFAULT_NO_PROXY_BYPASS` to include the standard private-network ranges, so a single GUI proxy configuration handles both cases: - IPv4 RFC 1918: 10.0.0.0/8, 172.16.0.0/12, 192.168.0.0/16 - IPv4 link-local: 169.254.0.0/16 - IPv6 ULA: fc00::/7 - IPv6 link-local: fe80::/10 reqwest's `NoProxy::from_string` accepts CIDR notation, so this is a pure string-list change with no new parser logic. Trade-off (documented in spec): if a corporate VPN resolves a public matrix.* hostname to an RFC 1918 address via split DNS, that traffic will also be treated as direct. This is the right default 99% of the time (VPN already provides encrypted transport), and a future GUI exception list can override it if needed. Test updates: - `build_policy_reqwest_client_attaches_no_proxy_bypass_for_local_addresses` and the TSP mirror now assert every entry in `DEFAULT_NO_PROXY_BYPASS` appears in the proxy debug representation. The "unexpected" guard list switches from "private ranges shouldn't be there" to "specific LAN IPs shouldn't be hardcoded" — CIDR ranges cover those addresses, but the verbatim IPs must not appear.
A previous version of `validate_proxy_url` accepted `socks5://` and `socks5h://` URLs. After this branch dropped that support (reqwest is not compiled with the `socks` feature), any user who had saved a socks5 URL via the old GUI would, on next launch, see all reqwest client builds fail with an opaque "Unsupported proxy URL scheme" error originating deep inside build_policy_reqwest_client — no proxy-related message in the UI. Add a forward-compatible guard in `load_saved_proxy_url_from_path`: after normalizing the saved URL, run it through `validate_proxy_url` and, on rejection, log a `warning!` and return `None`. The user can then re-save a supported scheme via Settings; meanwhile the rest of the app behaves as if no proxy were configured (which is correct — the saved one isn't usable). This guard also auto-handles any future scheme tightening: no second list of "deprecated schemes" to maintain. Test: `load_saved_proxy_url_ignores_legacy_socks_scheme` writes a real `proxy_state.json` with a `socks5://` entry and asserts the load returns None, catching regressions in the validate→load wiring.
The post-save success popup in Settings → Preferences → Proxy previously told users to "re-login to apply to the current Matrix session". This was technically functional but semantically wrong: in Matrix terminology "re-login" means logout + supply credentials again, whereas all the user actually needs is to restart the app so a fresh Matrix Client is built with the new proxy (PR #140's non-destructive restore preserves the session across restarts). Fix the misleading verb in both en.json and zh-CN.json. No code or UI behavior changes — the popup still fires from the same save-success path.
# Conflicts: # src/sliding_sync.rs
This was referenced May 27, 2026
Open
Author
Post-merge audit: what "inconsistency" actually meantRecording for future reference — this is the concrete picture of what was inconsistent before this PR and what got aligned after. Useful when revisiting proxy decisions or debugging proxy-adjacent issues. Pre-PR state (at commit
|
| # | Entry point | Honored GUI proxy? | Honored CLI --proxy? |
Had LAN bypass? | Mechanism |
|---|---|---|---|---|---|
| 1 | build_client (main Matrix Client) |
✅ | ✅ | ❌ | env mutation + .proxy() (triple coverage) |
| 2 | restore_session |
✅ (saved only) | ❌ | ❌ | load_saved_proxy_url (skipped CLI override resolver) |
| 3 | URL preview download | ❌ | ❌ | N/A | reqwest::Client::new() — zero proxy awareness |
| 4 | build_discovery_http_client |
✅ | ✅ | ❌ | .proxy(), no bypass |
| 5 | DEFAULT_SSO_CLIENT prebuild |
✅ via fallback | ❌ | ❌ | build_client(&Cli::default(), …) — lost CLI args |
| 6 | Updater | ❌ | ❌ | N/A | matrix_sdk::reqwest::get() — zero proxy awareness |
| 7 | TSP | ❌ | ❌ | N/A | ClientBuilder::new() — zero proxy, no TLS min, no bypass |
Headline stats:
- 3 of 7 entries had zero proxy awareness (URL preview, Updater, TSP) — filling proxy in GUI did nothing for them
- 1 of 7 dropped CLI override (
restore_session) —--proxyworked on first login, was ignored on next launch's restore - 6 of 7 had no LAN bypass — filling a public proxy made LAN homeservers unreachable
- 3 of 7 depended on process-env mutation — cross-platform fragility (shell-launch vs IDE-launch divergence)
User-visible bugs this caused
- Fill GUI proxy → main Matrix works ✓, but
Check for updatesfails silently ✗ - Fill GUI proxy → main Matrix works ✓, but URL previews / image downloads in rooms fail ✗
- Launch with
--proxy http://…→ first login works ✓, next launch's session restore fails ✗ - Fill GUI proxy and want to connect to LAN Palpo → fails ✗ (everything routed through public proxy)
Post-PR state
All 7 entries route through one helper:
build_policy_reqwest_client(
resolve_effective_proxy_url(proxy_override).as_deref(),
Some(timeout),
)which automatically applies:
.proxy(...)withNoProxybypass (RFC 1918 + loopback + link-local + IPv6 ULA).no_proxy()when no proxy is set- Shared
POLICY_USER_AGENT - Shared TLS 1.2 minimum
TSP uses a different reqwest version, so it mirrors via apply_tsp_proxy_policy with equivalent semantics.
Result: all 7 entries agree on "does this go through proxy", "which proxy", and "is LAN bypassed" — 100% consistency on the three dimensions that matter.
Known accepted limitations (not bugs)
- No hot-switch — matrix-sdk Client caches the reqwest Client, so GUI proxy changes require a restart. Documented in the "restart Robrix" save popup.
- No socks5 support — reqwest is not compiled with the
socksfeature. 99% of users use Clash mixed-port (HTTP+SOCKS on same port) sohttp://127.0.0.1:7890works. - System proxy (Wi-Fi settings on macOS/Windows, system VPN config) is not auto-detected — only TUN-mode VPN tools (OS-layer) work transparently without configuration; everything else requires explicit GUI fill.
Unrelated bugs surfaced during testing (filed separately)
- Switch account between different homeservers triggers infinite 401 UnknownToken retry storm #157 — Switch account between different homeservers triggers infinite 401 UnknownToken retry storm
- Proxy save-error popup hidden behind input modal (Z-order conflict) #158 — Proxy save-error popup hidden behind input modal (Z-order conflict)
Both are orthogonal to this PR and pre-existed it — discovered during manual testing.
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
1 participant
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.
Problems this PR fixes
The current proxy implementation has six concrete defects that let
proxy_state.json(the GUI's "saved proxy" file) drift out of sync with what the running process and the live Matrix client actually use. Each one is verified by a unit test insrc/proxy_config.rsor bycargo build --features tsp.1. TLS minimum and User-Agent regression (security)
When
build_clientandrestore_sessionswitched fromClient::builder().proxy(url)to.http_client(custom), they lost matrix-sdk'sHttpSettingsdefaults —min_tls_version(TLS_1_2)(BCP 195) anduser_agent("matrix-rust-sdk"). TheHttpConfig::Custombranch in matrix-sdk passes the supplied reqwest client through verbatim and never layers these defaults on top. Robrix could negotiate sub-1.2 TLS to homeservers on systems whose TLS backend allows it.2. CLI
--proxyoverride silently dropped on session restore and media downloadbuild_clientcorrectly resolvedcli.proxy→ CLI override or saved.restore_sessionandMatrixRequest::DownloadAndSaveFileinstead calledload_saved_proxy_url()directly, ignoring--proxy. Users launching withrobrix --proxy http://Awhose saved value was different (orNone) had their main client route through A while downloads bypassed it — or vice versa.3. SSO pre-built client cached with stale proxy
DEFAULT_SSO_CLIENTis built proactively at startup with whatever proxy was saved at that moment.spawn_sso_serverthen reused the cached client whenevereffective_proxy.is_some() == false. If the user cleared the proxy in Settings after boot, the rebuild condition was false and SSO routed through the deleted proxy — exactly the failure mode the spec is meant to eliminate.4.
socks5/socks5haccepted in validation but unreachable at runtimevalidate_proxy_urlaccepted four schemes, butCargo.lockconfirms reqwest 0.12.28 is compiled withouttokio-socks.Proxy::all("socks5://...")fails at first request with an opaque connector error, hidden behind a generic anyhow message. The PR drops socks5 from the allow-list rather than ship a promise the runtime can't keep.5.
PROXY_ENV_LOCKpoisoning permanently disabled proxy managementA
Mutex<()>poisoned by a panic-while-held caused every subsequentapply_proxy_to_process_envcall to fail with "Failed to lock proxy env: poisoned ...". The user could not change or clear their proxy until restart.6.
save_proxy_urlfile write outside the env lockThe file write happened before the env apply and outside the mutex. Two concurrent saves (UI thread vs. OIDC worker thread) could interleave write(A) → write(B) → apply(B) → apply(A), leaving file=B and env=A. The spec's "single source of truth" contract was violable.
Additional cleanups in scope
restore_sessionwas missing the 60s reqwest timeout thatbuild_clientsets; restored sessions could hang indefinitely on a slow CONNECT/TLS phase.create_reqwest_client(src/tsp/mod.rs, gated on--features tsp) was missingmin_tls_version(TLS_1_2)and the loopback NoProxy bypass. Mirrored frombuild_policy_reqwest_client.src/updater.rs) had already been routed throughbuild_policy_reqwest_clientin an earlier change but the spec did not list it in Allowed; spec extended to legitimize.Design
A single helper,
proxy_config::build_policy_reqwest_client, now builds every reqwest client used by Matrix / TSP / updater code with consistent TLS, UA, NoProxy bypass, and proxy resolution. A startup-timeOnceLock<Option<String>>namedCLI_PROXY_OVERRIDEholds the parsed--proxyvalue so any later resolver picks it up without re-parsing argv or threading the value through deep call chains.DEFAULT_SSO_CLIENTnow carries the proxy URL it was built with as a third tuple element.spawn_sso_serverreuses the cached client only when the homeserver is default and the cached proxy still matches the current effective proxy; otherwise it rebuilds.save_proxy_urlholdsPROXY_ENV_LOCKacross both the file write and the env apply. The lock helper auto-recovers from a poisoned state viaclear_poison()+into_inner().Spec coverage
All seven scenarios in
specs/task-proxy-policy.spec.mdmap to tests or tocargo build:proxy_state_none_clears_inherited_env_proxy_varssave_proxy_url_none_clears_env_proxy_varssave_proxy_url_some_sets_proxy_env_and_bypass_rulesbuild_policy_reqwest_client_disables_system_proxy_when_proxy_is_nonebuild_policy_reqwest_client_attaches_no_proxy_bypass_for_local_addressesvalidate_proxy_url_rejects_socks_schemes+discovery_http_client_rejects_invalid_proxy_overrideThree additional tests cover behaviors added in this PR:
policy_user_agent_carries_robrix_identity_and_sdk_familyvalidate_proxy_url_rejects_socks_schemesapply_proxy_to_process_env_recovers_from_poisoned_lockWhat this PR deliberately does not do
settings.preferences.proxy.popup.savedalready tells the user "Re-login to apply to the current Matrix session." Implementing in-place rebuild would require dropping CLIENT, SYNC_SERVICE, and the worker task — out of scope here.std::env::set_varUB on Rust 2024. The env-mutation surface is reduced and serialized viaPROXY_ENV_LOCK, but the underlying race with third-party crates reading env (reqwest, hyper) cannot be eliminated while the spec mandates env vars be set. Documented as a known limit.Test plan
cargo build— passes locallycargo build --features tsp— passes locallycargo test --lib proxy_config::— 8/8 passes locallyHTTPS_PROXYset — confirm Robrix routes direct (spec-mandated)robrix --proxy http://localhost:8080with a different saved value — confirm CLI override wins for both login and downloads