From 9ecbfde656d5be0ef001137eedd7e3c268af3a7d Mon Sep 17 00:00:00 2001 From: Miguel Palhas Date: Sat, 29 Aug 2026 18:42:43 +0100 Subject: [PATCH] fix(dl): only log in when qBittorrent asks Logging in before the first call turned a stale or wrong password into a hard failure on calls that would have succeeded: an instance that bypasses authentication for arr's address answers everything, and never needed the credentials at all. Authentication is now established on the first 403. The health probe was reading a 403 from `app/version` as healthy, on the reasoning that a live daemon is all the lamp claims. That is exactly the state arr cannot use, and it left the lamp green while every grab and every reaper tick failed on rejected credentials. It now lists torrents through the configured client, so the lamp fails when arr's own calls do. Co-Authored-By: Claude Opus 5 (1M context) --- crates/arr-api/src/health.rs | 28 +++++++++---------- crates/arr-api/src/lib.rs | 17 +++++++----- crates/arr-dl/src/lib.rs | 52 +++++++++++++++++++++++++++++++----- 3 files changed, 70 insertions(+), 27 deletions(-) diff --git a/crates/arr-api/src/health.rs b/crates/arr-api/src/health.rs index 05ddc65..f0fec51 100644 --- a/crates/arr-api/src/health.rs +++ b/crates/arr-api/src/health.rs @@ -181,21 +181,21 @@ async fn probe_prowlarr(state: &AppState) -> Check { } } -/// A live `WebUI` answers `/api/v2/app/version`, and answers `403` when the -/// probe carries no session. Both are a running daemon, which is all this -/// lamp claims; whether arr's credentials work shows up on the first grab. +/// The probe is a real torrent list through the configured client, not a bare +/// reachability check. +/// +/// A `WebUI` that wants credentials answers `403` to anything unauthenticated, +/// so "the port is open" is true of an instance arr cannot use at all. Reading +/// that as healthy once left the lamp green while every grab and every reaper +/// tick failed on a rejected password, which is the one thing this lamp exists +/// to catch. async fn probe_qbit(state: &AppState) -> Check { - let base = state.upstreams().qbittorrent_url.trim_end_matches('/'); - let request = state.http().get(format!("{base}/api/v2/app/version")); - match request.send().await { - Ok(response) - if response.status().is_success() - || response.status() == reqwest::StatusCode::FORBIDDEN => - { - Check::ok() - } - Ok(response) => Check::unreachable(format!("http {}", response.status().as_u16())), - Err(err) => Check::unreachable(describe(err)), + let Some(qbit) = state.qbit() else { + return Check::unconfigured("no qBittorrent client is configured"); + }; + match qbit.list_torrents().await { + Ok(_) => Check::ok(), + Err(error) => Check::unreachable(error.to_string()), } } diff --git a/crates/arr-api/src/lib.rs b/crates/arr-api/src/lib.rs index 36dff92..ad787b0 100644 --- a/crates/arr-api/src/lib.rs +++ b/crates/arr-api/src/lib.rs @@ -191,8 +191,8 @@ mod tests { let qbit = MockServer::start().await; Mock::given(method("GET")) - .and(path("/api/v2/app/version")) - .respond_with(ResponseTemplate::new(403).set_body_string("Forbidden")) + .and(path("/api/v2/torrents/info")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!([]))) .mount(&qbit) .await; @@ -237,6 +237,7 @@ mod tests { .with_tmdb_api_key(Some("key".into())), ) .expect("state") + .with_qbit(arr_dl::QbitClient::new(&qbit.uri()).expect("client")) // The subtitle binaries default to `PATH`; pin them to something // every machine has so this test stays about the classic upstreams. .with_syncer(arr_subs::Syncer::new().with_binary("sh")) @@ -253,7 +254,9 @@ mod tests { #[tokio::test] async fn a_missing_tmdb_key_is_unconfigured_not_an_outage() { let (prowlarr, qbit) = upstreams_up().await; - let state = AppState::new(Upstreams::new(prowlarr.uri(), qbit.uri())).expect("state"); + let state = AppState::new(Upstreams::new(prowlarr.uri(), qbit.uri())) + .expect("state") + .with_qbit(arr_dl::QbitClient::new(&qbit.uri()).expect("client")); let body = report(state).await; assert_eq!(body["tmdb"]["status"], "unconfigured"); @@ -266,8 +269,9 @@ mod tests { // Port 1 is privileged and nothing binds it, so the probe gets a // refused connection immediately instead of waiting out the timeout. - let state = - AppState::new(Upstreams::new("http://127.0.0.1:1".into(), qbit.uri())).expect("state"); + let state = AppState::new(Upstreams::new("http://127.0.0.1:1".into(), qbit.uri())) + .expect("state") + .with_qbit(arr_dl::QbitClient::new(&qbit.uri()).expect("client")); let body = report(state).await; assert_eq!(body["status"], "degraded"); @@ -307,7 +311,8 @@ mod tests { .with_tmdb_url(tmdb.uri()) .with_tmdb_api_key(Some("super-secret".into())), ) - .expect("state"); + .expect("state") + .with_qbit(arr_dl::QbitClient::new(&qbit.uri()).expect("client")); let body = report(state).await; assert_eq!(body["tmdb"]["status"], "unreachable"); diff --git a/crates/arr-dl/src/lib.rs b/crates/arr-dl/src/lib.rs index 5688dd1..9d555fe 100644 --- a/crates/arr-dl/src/lib.rs +++ b/crates/arr-dl/src/lib.rs @@ -479,10 +479,11 @@ impl QbitClient { path: &str, build: impl Fn() -> reqwest::RequestBuilder, ) -> Result { - if self.credentials.is_some() && self.session.read().await.is_none() { - self.login().await?; - } - + // Deliberately no login before the first call. An instance that + // bypasses authentication for arr's address answers everything, and + // logging in first would turn a stale or wrong password into a hard + // failure on a call that would have succeeded. Authentication is + // established only when qBittorrent actually asks for it, below. for attempt in 0..2 { let session = self.session.read().await.clone(); let mut request = build().header(REFERER, &self.origin); @@ -496,7 +497,7 @@ impl QbitClient { if self.credentials.is_none() { return Err(Error::Unauthenticated); } - // The session expired or the client restarted under us. + // No session yet, or the one held expired. self.login().await?; continue; } @@ -668,7 +669,7 @@ mod tests { use std::path::PathBuf; use serde_json::json; - use wiremock::matchers::{body_string_contains, method, path, query_param}; + use wiremock::matchers::{body_string_contains, header, method, path, query_param}; use wiremock::{Mock, MockServer, ResponseTemplate}; use super::{AddTorrent, Error, QbitClient, TorrentSource, TorrentState}; @@ -688,7 +689,7 @@ mod tests { } #[tokio::test] - async fn logs_in_once_and_reuses_the_session() { + async fn logs_in_once_on_the_first_403_and_reuses_the_session() { let server = MockServer::start().await; Mock::given(method("POST")) .and(path("/api/v2/auth/login")) @@ -702,8 +703,17 @@ mod tests { .await; Mock::given(method("GET")) .and(path("/api/v2/torrents/info")) + .and(header("cookie", "SID=abc123")) .respond_with(ResponseTemplate::new(200).set_body_json(json!([]))) .expect(2) + .with_priority(1) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path("/api/v2/torrents/info")) + .respond_with(ResponseTemplate::new(403).set_body_string("Forbidden")) + .expect(1) + .with_priority(2) .mount(&server) .await; @@ -713,9 +723,37 @@ mod tests { assert!(client.list_torrents().await.unwrap().is_empty()); } + /// An instance that bypasses authentication for arr's address answers + /// every call, so a stale password configured alongside it must not turn + /// a working call into a failure. Nothing here ever reaches `auth/login`. + #[tokio::test] + async fn a_bypassed_instance_is_never_logged_into() { + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/api/v2/auth/login")) + .respond_with(ResponseTemplate::new(200).set_body_string("Fails.")) + .expect(0) + .mount(&server) + .await; + Mock::given(method("GET")) + .and(path("/api/v2/torrents/info")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!([]))) + .mount(&server) + .await; + + let client = + QbitClient::with_credentials(&server.uri(), "user".into(), "stale".into()).unwrap(); + assert!(client.list_torrents().await.unwrap().is_empty()); + } + #[tokio::test] async fn a_wrong_password_is_a_login_error_not_a_retry_loop() { let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/api/v2/torrents/info")) + .respond_with(ResponseTemplate::new(403).set_body_string("Forbidden")) + .mount(&server) + .await; Mock::given(method("POST")) .and(path("/api/v2/auth/login")) .respond_with(ResponseTemplate::new(200).set_body_string("Fails."))