fix(torad): bypass system proxy env vars when fetching .torrent sources
reqwest honours HTTP_PROXY/ALL_PROXY from the environment by default. A self-hosted Jackett is normally reached over loopback, and routing loopback through an inherited proxy breaks it — which matters once torad runs in an environment where those vars are set for VPN reasons. Extracts the builder into `build_http_client` so the behaviour is directly testable, and adds a test that serves one response from a loopback listener while ALL_PROXY points at a dead port. The test is falsifiable: removing .no_proxy() makes it fail with ConnectionRefused against the proxy address, which was verified before committing. The proxy URL is deliberately http:// rather than socks5:// — reqwest is built here without its `socks` feature, so a SOCKS proxy would be ignored and the test would pass either way. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
@@ -48,15 +48,29 @@ pub(crate) struct SourceResolver {
|
||||
handlers: Vec<Box<dyn SourceHandler>>,
|
||||
}
|
||||
|
||||
/// Builds the HTTP client shared by the Jackett and generic-`.torrent` handlers.
|
||||
///
|
||||
/// Two non-default settings, both load-bearing:
|
||||
///
|
||||
/// - **Manual redirect policy.** Jackett 302-redirects to `magnet:` URIs that
|
||||
/// reqwest refuses to follow; we inspect `Location` ourselves so the magnet
|
||||
/// handler can take over.
|
||||
/// - **No system proxy.** reqwest otherwise honours `HTTP_PROXY`/`ALL_PROXY`
|
||||
/// from the environment. A self-hosted Jackett is typically reached over
|
||||
/// loopback, and routing loopback through an inherited proxy breaks it. Note
|
||||
/// this client also serves arbitrary remote `.torrent` URLs via
|
||||
/// [`http_torrent`], so the setting applies to those too.
|
||||
fn build_http_client() -> Result<reqwest::Client> {
|
||||
reqwest::Client::builder()
|
||||
.redirect(reqwest::redirect::Policy::none())
|
||||
.no_proxy()
|
||||
.build()
|
||||
.context("failed to build HTTP client")
|
||||
}
|
||||
|
||||
impl SourceResolver {
|
||||
pub(crate) fn new() -> Result<Self> {
|
||||
// Manual redirect policy: Jackett 302-redirects to `magnet:` URIs that
|
||||
// reqwest would refuse to follow; we want to inspect `Location`
|
||||
// ourselves so the magnet handler can take over.
|
||||
let http_client = reqwest::Client::builder()
|
||||
.redirect(reqwest::redirect::Policy::none())
|
||||
.build()
|
||||
.context("failed to build HTTP client")?;
|
||||
let http_client = build_http_client()?;
|
||||
Ok(Self {
|
||||
handlers: vec![
|
||||
Box::new(magnet::MagnetHandler),
|
||||
@@ -76,3 +90,59 @@ impl SourceResolver {
|
||||
anyhow::bail!("no handler matched source: {source}")
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
use tokio::io::{AsyncReadExt, AsyncWriteExt};
|
||||
use tokio::net::TcpListener;
|
||||
|
||||
/// Serves exactly one minimal HTTP/1.1 response, then stops. Returns the
|
||||
/// bound address so the test can build a URL for it.
|
||||
async fn spawn_oneshot_server() -> std::net::SocketAddr {
|
||||
let listener = TcpListener::bind("127.0.0.1:0").await.unwrap();
|
||||
let addr = listener.local_addr().unwrap();
|
||||
tokio::spawn(async move {
|
||||
if let Ok((mut sock, _)) = listener.accept().await {
|
||||
let mut buf = [0u8; 1024];
|
||||
let _ = sock.read(&mut buf).await;
|
||||
let _ = sock
|
||||
.write_all(b"HTTP/1.1 200 OK\r\nContent-Length: 2\r\n\r\nhi")
|
||||
.await;
|
||||
let _ = sock.flush().await;
|
||||
}
|
||||
});
|
||||
addr
|
||||
}
|
||||
|
||||
/// Regression for [`build_http_client`]'s `.no_proxy()`: reqwest reads
|
||||
/// `ALL_PROXY` at build time, so without it this request would be sent to a
|
||||
/// dead proxy instead of the loopback server and fail.
|
||||
///
|
||||
/// The proxy URL is deliberately `http://`, not `socks5://` — reqwest is
|
||||
/// built here without its `socks` feature, so a SOCKS proxy would simply be
|
||||
/// ignored and the test would pass with or without `.no_proxy()`.
|
||||
///
|
||||
/// Env vars are process-global and `set_var` is `unsafe` under the 2024
|
||||
/// edition. This is the only test in the crate that touches the
|
||||
/// environment, and it restores it before yielding, so nothing else here
|
||||
/// can observe the change — but it is not safe to add a second such test
|
||||
/// without serialising them.
|
||||
#[tokio::test]
|
||||
async fn http_client_ignores_system_proxy() {
|
||||
let addr = spawn_oneshot_server().await;
|
||||
|
||||
unsafe { std::env::set_var("ALL_PROXY", "http://127.0.0.1:1") };
|
||||
let client = build_http_client();
|
||||
unsafe { std::env::remove_var("ALL_PROXY") };
|
||||
|
||||
let response = client
|
||||
.expect("client should build")
|
||||
.get(format!("http://{addr}/"))
|
||||
.send()
|
||||
.await
|
||||
.expect("request must reach the loopback server, not the dead proxy");
|
||||
assert_eq!(response.status(), reqwest::StatusCode::OK);
|
||||
assert_eq!(response.text().await.unwrap(), "hi");
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user