From ff10071b0a4c3399e5a21ae85830ec78d7c1ce76 Mon Sep 17 00:00:00 2001 From: InstaZDLL Date: Wed, 14 Oct 2026 20:35:11 +0200 Subject: [PATCH 1/8] feat(metadata): the clients take a client, an address and a key from their caller waveflow-server is about to enrich its catalogue through these clients, and it cannot use them as they are. Each built its own reqwest client, which follows up to ten redirects, read the body with no ceiling, and aimed at a constant address that leaves a hermetic suite nothing to point at. TheAudioDB asked under the shared free key with no way to pass another. Each client now has with_http(client, base_url) beside new(), and TheAudioDB takes its key there. new() builds exactly what it built before, so the desktop asks the same questions of the same addresses. Three things change for every caller, the desktop included. Bodies are read through a two-megabyte ceiling, checked against the announced length first and against a running count for a response that announces nothing or lies. Last.fm's read methods no longer answer Ok for every error code. Code 6 is "no such artist" and stays an answer; a refused key or a spent quota is a LookupError, and so is a failure status whose body names no code. Read as answers, they told a caller that caches its results that a throttled minute was an artist with no biography and no neighbours. The desktop's two callers already treat both the same for the biography; for similar artists a refused key now falls back to Deezer instead of being cached as an empty list. Transport errors lose their URL. reqwest prints it in Display and Debug, and these URLs carry the caller's key, in Last.fm's query string and in TheAudioDB's path. Every caller logs what it gets. Last.fm also gains artist_get_info_by_mbid: an id has no homonyms. Each new test was made to fail under the change it guards: eight inversions, each caught by the test that names it. Signed-off-by: InstaZDLL --- src-tauri/crates/core/Cargo.toml | 2 +- src-tauri/crates/core/src/metadata/deezer.rs | 100 +++++- src-tauri/crates/core/src/metadata/lastfm.rs | 340 +++++++++++++++--- src-tauri/crates/core/src/metadata/mod.rs | 223 +++++++++++- .../crates/core/src/metadata/theaudiodb.rs | 132 ++++++- 5 files changed, 707 insertions(+), 90 deletions(-) diff --git a/src-tauri/crates/core/Cargo.toml b/src-tauri/crates/core/Cargo.toml index aee833029..95da9e2a6 100644 --- a/src-tauri/crates/core/Cargo.toml +++ b/src-tauri/crates/core/Cargo.toml @@ -220,4 +220,4 @@ tempfile = "3" # Async test harness for the scanner passes that talk to SQLite # (`refresh_folder_covers`). Only the attribute macro + a current-thread # runtime are needed, so no "full". -tokio = { version = "1", features = ["macros", "rt-multi-thread"] } +tokio = { version = "1", features = ["macros", "rt-multi-thread", "net", "io-util"] } diff --git a/src-tauri/crates/core/src/metadata/deezer.rs b/src-tauri/crates/core/src/metadata/deezer.rs index cfb4a5e94..5d23ccc52 100644 --- a/src-tauri/crates/core/src/metadata/deezer.rs +++ b/src-tauri/crates/core/src/metadata/deezer.rs @@ -10,6 +10,8 @@ use serde::Deserialize; +use crate::metadata::{default_http, read_bounded, BodyError, MAX_RESPONSE_BYTES}; + const BASE_URL: &str = "https://api.deezer.com"; const USER_AGENT: &str = "WaveFlow/0.1"; const TIMEOUT_SECS: u64 = 5; @@ -21,6 +23,7 @@ const TIMEOUT_SECS: u64 = 5; #[derive(Clone)] pub struct DeezerClient { http: reqwest::Client, + base_url: String, } // ── API response types ────────────────────────────────────────────── @@ -94,6 +97,19 @@ pub enum DeezerError { /// decode failure. #[error("deezer rate-limited the request (HTTP 429)")] RateLimited, + #[error("deezer's answer exceeds {limit} bytes")] + TooLarge { limit: usize }, + #[error("deezer's answer is not the expected JSON: {0}")] + Decode(#[from] serde_json::Error), +} + +impl From for DeezerError { + fn from(err: BodyError) -> Self { + match err { + BodyError::Http(err) => DeezerError::Http(err), + BodyError::TooLarge { limit } => DeezerError::TooLarge { limit }, + } + } } impl DeezerError { @@ -103,7 +119,7 @@ impl DeezerError { match self { DeezerError::Api(err) => err.is_quota_exceeded(), DeezerError::RateLimited => true, - DeezerError::Http(_) => false, + DeezerError::Http(_) | DeezerError::TooLarge { .. } | DeezerError::Decode(_) => false, } } } @@ -257,12 +273,16 @@ impl Default for DeezerClient { impl DeezerClient { pub fn new() -> Self { - let http = reqwest::Client::builder() - .user_agent(USER_AGENT) - .timeout(std::time::Duration::from_secs(TIMEOUT_SECS)) - .build() - .expect("failed to build reqwest client"); - Self { http } + Self::with_http(default_http(USER_AGENT, TIMEOUT_SECS), BASE_URL) + } + + /// A client over `http`, aimed at `base_url` — the API root, with no + /// trailing slash. + pub fn with_http(http: reqwest::Client, base_url: impl Into) -> Self { + Self { + http, + base_url: base_url.into(), + } } /// Send `request` and read the body as `T`, turning Deezer's in-band @@ -279,18 +299,16 @@ impl DeezerClient { if response.status() == reqwest::StatusCode::TOO_MANY_REQUESTS { return Err(DeezerError::RateLimited); } - response - .error_for_status()? - .json::>() - .await? - .into_result() + let response = response.error_for_status()?; + let body = read_bounded(response, MAX_RESPONSE_BYTES).await?; + serde_json::from_slice::>(&body)?.into_result() } /// Search artists by name. Returns up to 25 hits (Deezer default). pub async fn search_artist(&self, name: &str) -> DeezerResult> { let resp: DeezerSearchResponse = Self::fetch( self.http - .get(format!("{BASE_URL}/search/artist")) + .get(format!("{}/search/artist", self.base_url)) .query(&[("q", name)]), ) .await?; @@ -299,14 +317,18 @@ impl DeezerClient { /// Fetch a single artist by Deezer ID. pub async fn get_artist(&self, deezer_id: i64) -> DeezerResult { - Self::fetch(self.http.get(format!("{BASE_URL}/artist/{deezer_id}"))).await + Self::fetch( + self.http + .get(format!("{}/artist/{deezer_id}", self.base_url)), + ) + .await } /// Search albums by a free-text query (typically "album title artist name"). pub async fn search_album(&self, query: &str) -> DeezerResult> { let resp: DeezerSearchResponse = Self::fetch( self.http - .get(format!("{BASE_URL}/search/album")) + .get(format!("{}/search/album", self.base_url)) .query(&[("q", query)]), ) .await?; @@ -319,7 +341,7 @@ impl DeezerClient { pub async fn search_track(&self, query: &str) -> DeezerResult> { let resp: DeezerSearchResponse = Self::fetch( self.http - .get(format!("{BASE_URL}/search/track")) + .get(format!("{}/search/track", self.base_url)) .query(&[("q", query)]), ) .await?; @@ -328,7 +350,11 @@ impl DeezerClient { /// Fetch a single album by Deezer ID. pub async fn get_album(&self, deezer_id: i64) -> DeezerResult { - Self::fetch(self.http.get(format!("{BASE_URL}/album/{deezer_id}"))).await + Self::fetch( + self.http + .get(format!("{}/album/{deezer_id}", self.base_url)), + ) + .await } /// The tracks of an album, in the catalogue's own order, **every @@ -358,7 +384,7 @@ impl DeezerClient { for _ in 0..MAX_PAGES { let resp: DeezerSearchResponse = Self::fetch( self.http - .get(format!("{BASE_URL}/album/{deezer_id}/tracks")) + .get(format!("{}/album/{deezer_id}/tracks", self.base_url)) .query(&[("index", index.to_string()), ("limit", PAGE.to_string())]), ) .await?; @@ -386,7 +412,7 @@ impl DeezerClient { pub async fn get_related_artists(&self, deezer_id: i64) -> DeezerResult> { let resp: DeezerSearchResponse = Self::fetch( self.http - .get(format!("{BASE_URL}/artist/{deezer_id}/related")), + .get(format!("{}/artist/{deezer_id}/related", self.base_url)), ) .await?; Ok(resp.data) @@ -532,4 +558,40 @@ mod tests { fn best_picture_is_none_when_no_sizes_present() { assert_eq!(hit(None, None).best_picture(), None); } + + use crate::metadata::test_server::{serve, serve_json, strict_http, Reply}; + + #[tokio::test] + async fn a_search_is_read_through_an_injected_client_and_address() { + let provider = serve_json(r#"{"data":[{"id":464,"name":"Björk"}]}"#).await; + let client = DeezerClient::with_http(strict_http(), provider.base_url.clone()); + + let hits = client.search_artist("Björk").await.unwrap(); + + assert_eq!(hits.len(), 1); + assert_eq!(hits[0].name, "Björk"); + assert!(provider.request_line(0).starts_with("GET /search/artist?")); + } + + #[tokio::test] + async fn an_answer_over_the_ceiling_is_refused_even_unannounced() { + let padding = "x".repeat(crate::metadata::MAX_RESPONSE_BYTES); + let body = format!(r#"{{"data":[{{"id":1,"name":"{padding}"}}]}}"#); + let provider = serve(Reply { + status: "200 OK", + headers: &[], + body: &body, + announce_length: false, + }) + .await; + let client = DeezerClient::with_http(strict_http(), provider.base_url.clone()); + + let error = client + .search_artist("Björk") + .await + .expect_err("an oversized answer must not be read whole"); + + assert!(matches!(error, DeezerError::TooLarge { .. }), "{error:?}"); + assert!(!error.is_quota_exceeded()); + } } diff --git a/src-tauri/crates/core/src/metadata/lastfm.rs b/src-tauri/crates/core/src/metadata/lastfm.rs index d49ffbbb2..dd10a16b5 100644 --- a/src-tauri/crates/core/src/metadata/lastfm.rs +++ b/src-tauri/crates/core/src/metadata/lastfm.rs @@ -19,6 +19,8 @@ use md5::{Digest, Md5}; use regex::Regex; use serde::Deserialize; +use super::{default_http, read_bounded, LookupError, MAX_RESPONSE_BYTES}; + const BASE_URL: &str = "https://ws.audioscrobbler.com/2.0/"; const USER_AGENT: &str = "WaveFlow/0.1"; const TIMEOUT_SECS: u64 = 5; @@ -64,6 +66,7 @@ pub struct LastfmArtistInfo { pub struct LastfmClient { http: reqwest::Client, + base_url: String, } impl Default for LastfmClient { @@ -72,46 +75,103 @@ impl Default for LastfmClient { } } +/// Last.fm's code for "the artist you supplied could not be found" — +/// the one error that is an answer rather than a failure. +const ERROR_NOT_FOUND: i64 = 6; + +/// Reads a read-only method's answer through the size ceiling, and +/// sorts Last.fm's in-band `error` into the two things it can mean. +/// +/// Last.fm reports most failures as JSON with an `error` code, sometimes +/// under a failure status and sometimes under 200, so the code decides +/// first. Only a body that names no code — because it does not parse, or +/// parses as nothing in particular — falls back on the status. +async fn read_reply( + request: reqwest::RequestBuilder, + error_of: impl Fn(&T) -> Option, +) -> Result, LookupError> { + let response = request.send().await?; + let status = response.status(); + let body = read_bounded(response, MAX_RESPONSE_BYTES).await?; + let reply: T = match serde_json::from_slice(&body) { + Ok(reply) => reply, + Err(_) if !status.is_success() => { + return Err(LookupError::Status { + status: status.as_u16(), + }); + } + Err(err) => return Err(err.into()), + }; + match error_of(&reply) { + // A failure status whose body parses but names no error — `{}` + // under a 503 — is still a failure: read as a reply it would say + // "no such artist". + None if !status.is_success() => Err(LookupError::Status { + status: status.as_u16(), + }), + None => Ok(Some(reply)), + Some(ERROR_NOT_FOUND) => Ok(None), + Some(code) => Err(LookupError::Refused { code }), + } +} + impl LastfmClient { pub fn new() -> Self { - let http = reqwest::Client::builder() - .user_agent(USER_AGENT) - .timeout(std::time::Duration::from_secs(TIMEOUT_SECS)) - .build() - .expect("failed to build reqwest client"); - Self { http } + Self::with_http(default_http(USER_AGENT, TIMEOUT_SECS), BASE_URL) + } + + /// A client over `http`, aimed at `base_url` — the API root, the one + /// that ends in `/2.0/`. Last.fm answers `/2.0` without the slash + /// with a redirect, which a client that follows none would read as a + /// failure. + pub fn with_http(http: reqwest::Client, base_url: impl Into) -> Self { + Self { + http, + base_url: base_url.into(), + } } /// Fetch an artist's bio via `artist.getInfo`. Returns `Ok(None)` /// when Last.fm reports no match (`error=6`) or the response has - /// no artist payload. HTML anchor links in `summary` are stripped - /// before return so the UI can render plain text. + /// no artist payload; any other error code is a [`LookupError`], + /// because a refused key or a spent quota is not an artist without a + /// biography. HTML anchor links in `summary` are stripped before + /// return so the UI can render plain text. pub async fn artist_get_info( &self, name: &str, api_key: &str, - ) -> reqwest::Result> { - let resp: LastfmArtistGetInfoResponse = self - .http - .get(BASE_URL) - .query(&[ - ("method", "artist.getinfo"), - ("artist", name), - ("api_key", api_key), - ("format", "json"), - ("autocorrect", "1"), - ]) - .send() - .await? - .json() - .await?; + ) -> Result, LookupError> { + self.get_info(("artist", name), api_key).await + } - if resp.error.is_some() { - // error=6 = "The artist you supplied could not be found". - // Any other error also surfaces as an empty enrichment so - // the caller doesn't need to differentiate. + /// [`Self::artist_get_info`] keyed on a MusicBrainz artist id rather + /// than a name. An id has no homonyms, so when the tags carry one it + /// is the better question to ask. + pub async fn artist_get_info_by_mbid( + &self, + mbid: &str, + api_key: &str, + ) -> Result, LookupError> { + self.get_info(("mbid", mbid), api_key).await + } + + async fn get_info( + &self, + subject: (&str, &str), + api_key: &str, + ) -> Result, LookupError> { + let request = self.http.get(&self.base_url).query(&[ + ("method", "artist.getinfo"), + subject, + ("api_key", api_key), + ("format", "json"), + ("autocorrect", "1"), + ]); + let Some(resp) = read_reply(request, |r: &LastfmArtistGetInfoResponse| r.error).await? + else { return Ok(None); - } + }; let Some(artist) = resp.artist else { return Ok(None); @@ -185,33 +245,27 @@ impl LastfmClient { /// Fetch up to `limit` similar artists via `artist.getSimilar`. /// Returns an empty vec when Last.fm reports no match (`error=6`) /// or the response carries no `similarartists` payload — callers - /// can then fall back to Deezer's `/artist/{id}/related`. + /// can then fall back to Deezer's `/artist/{id}/related`. Any other + /// error code is a [`LookupError`], so a refused key falls back too + /// instead of being remembered as an artist with no neighbours. pub async fn artist_get_similar( &self, name: &str, api_key: &str, limit: u32, - ) -> reqwest::Result> { + ) -> Result, LookupError> { let limit_str = limit.to_string(); - let resp: LastfmSimilarResponse = self - .http - .get(BASE_URL) - .query(&[ - ("method", "artist.getsimilar"), - ("artist", name), - ("api_key", api_key), - ("format", "json"), - ("autocorrect", "1"), - ("limit", limit_str.as_str()), - ]) - .send() - .await? - .json() - .await?; - - if resp.error.is_some() { + let request = self.http.get(&self.base_url).query(&[ + ("method", "artist.getsimilar"), + ("artist", name), + ("api_key", api_key), + ("format", "json"), + ("autocorrect", "1"), + ("limit", limit_str.as_str()), + ]); + let Some(resp) = read_reply(request, |r: &LastfmSimilarResponse| r.error).await? else { return Ok(Vec::new()); - } + }; let Some(payload) = resp.similarartists else { return Ok(Vec::new()); }; @@ -339,7 +393,7 @@ impl LastfmClient { let resp = self .http - .post(BASE_URL) + .post(&self.base_url) .form(&[ ("api_key", api_key), ("method", "auth.getMobileSession"), @@ -422,7 +476,7 @@ impl LastfmClient { form.push(("api_sig", &api_sig)); form.push(("format", "json")); - let resp = self.http.post(BASE_URL).form(&form).send().await?; + let resp = self.http.post(&self.base_url).form(&form).send().await?; let body = resp.text().await?; if let Ok(err) = serde_json::from_str::(&body) { return Err(LastfmError::Api { @@ -470,7 +524,7 @@ impl LastfmClient { form.push(("api_sig", &api_sig)); form.push(("format", "json")); - let resp = self.http.post(BASE_URL).form(&form).send().await?; + let resp = self.http.post(&self.base_url).form(&form).send().await?; let body = resp.text().await?; if let Ok(err) = serde_json::from_str::(&body) { return Err(LastfmError::Api { @@ -535,4 +589,188 @@ mod tests { "https://lastfm.freetls.fastly.net/i/u/300x300/abc123def456abc123def456abc12345.png" )); } + + use crate::metadata::test_server::{serve, serve_json, strict_http, Reply}; + + fn client_for(base_url: &str) -> LastfmClient { + LastfmClient::with_http(strict_http(), format!("{base_url}/2.0/")) + } + + #[tokio::test] + async fn a_biography_is_read_through_an_injected_client_and_address() { + let provider = serve_json( + r#"{"artist":{"name":"Björk","bio":{"summary":"Icelandic. Read more on Last.fm","content":"Born in Reykjavík."}}}"#, + ) + .await; + + let info = client_for(&provider.base_url) + .artist_get_info("Björk", "the-key") + .await + .unwrap() + .expect("a biography"); + + assert_eq!(info.bio_full.as_deref(), Some("Born in Reykjavík.")); + let line = provider.request_line(0); + assert!(line.starts_with("GET /2.0/?"), "{line}"); + assert!(line.contains("method=artist.getinfo"), "{line}"); + assert!(line.contains("api_key=the-key"), "{line}"); + } + + #[tokio::test] + async fn an_unknown_artist_is_an_answer() { + let provider = + serve_json(r#"{"error":6,"message":"The artist you supplied could not be found"}"#) + .await; + + let info = client_for(&provider.base_url) + .artist_get_info("Nobody At All", "the-key") + .await + .expect("not found is not a failure"); + + assert!(info.is_none()); + } + + /// The reason this module's read methods stopped answering `Ok` for + /// every error code: a refused key is a failure to ask, and a caller + /// that caches answers would otherwise remember it as an artist + /// without a biography. + #[tokio::test] + async fn a_refused_key_is_a_failure_and_not_an_absence() { + let provider = serve(Reply { + status: "403 Forbidden", + headers: &[], + body: r#"{"error":10,"message":"Invalid API key - You must be granted a valid key by last.fm"}"#, + announce_length: true, + }) + .await; + + let error = client_for(&provider.base_url) + .artist_get_info("Björk", "a-bad-key") + .await + .expect_err("a refused key must not read as an absence"); + + assert!( + matches!(error, LookupError::Refused { code: 10 }), + "{error:?}" + ); + } + + #[tokio::test] + async fn a_spent_quota_under_a_success_status_is_still_a_failure() { + let provider = serve_json(r#"{"error":29,"message":"Rate limit exceeded"}"#).await; + + let error = client_for(&provider.base_url) + .artist_get_similar("Björk", "the-key", 10) + .await + .expect_err("a throttled call must not read as no neighbours"); + + assert!( + matches!(error, LookupError::Refused { code: 29 }), + "{error:?}" + ); + } + + #[tokio::test] + async fn a_redirect_is_not_followed_by_a_client_that_follows_none() { + let elsewhere = serve_json(r#"{"artist":{"name":"Björk"}}"#).await; + let location = format!("location: {}/2.0/", elsewhere.base_url); + let provider = serve(Reply { + status: "302 Found", + headers: &[&location], + body: "", + announce_length: true, + }) + .await; + + let error = client_for(&provider.base_url) + .artist_get_info("Björk", "the-key") + .await + .expect_err("a redirect is not an answer"); + + assert!( + matches!(error, LookupError::Status { status: 302 }), + "{error:?}" + ); + assert_eq!(elsewhere.hits(), 0, "the redirect target was contacted"); + } + + #[tokio::test] + async fn an_answer_over_the_ceiling_is_refused_even_unannounced() { + let padding = "x".repeat(MAX_RESPONSE_BYTES); + let body = format!(r#"{{"artist":{{"name":"{padding}"}}}}"#); + let provider = serve(Reply { + status: "200 OK", + headers: &[], + body: &body, + announce_length: false, + }) + .await; + + let error = client_for(&provider.base_url) + .artist_get_info("Björk", "the-key") + .await + .expect_err("an oversized answer must not be read whole"); + + assert!(matches!(error, LookupError::TooLarge { .. }), "{error:?}"); + } + + #[tokio::test] + async fn a_musicbrainz_id_is_asked_as_one() { + let provider = serve_json(r#"{"artist":{"name":"Björk"}}"#).await; + + client_for(&provider.base_url) + .artist_get_info_by_mbid("87c5dedd-371d-4a53-9f7f-80522fb7f3cb", "the-key") + .await + .unwrap(); + + let line = provider.request_line(0); + assert!( + line.contains("mbid=87c5dedd-371d-4a53-9f7f-80522fb7f3cb"), + "{line}" + ); + assert!(!line.contains("artist="), "{line}"); + } + + /// The key travels in the query string, and `reqwest` prints the URL + /// of a failed request — so an unredacted error would hand the key to + /// whatever logs it. + #[tokio::test] + async fn a_transport_error_does_not_carry_the_key() { + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let address = listener.local_addr().unwrap(); + drop(listener); + + let error = LastfmClient::with_http(strict_http(), format!("http://{address}/2.0/")) + .artist_get_info("Björk", "the-secret-key") + .await + .expect_err("nothing is listening"); + + assert!(matches!(error, LookupError::Http(_)), "{error:?}"); + assert!(!format!("{error}").contains("the-secret-key"), "{error}"); + assert!( + !format!("{error:?}").contains("the-secret-key"), + "{error:?}" + ); + } + + #[tokio::test] + async fn a_failure_status_with_an_empty_object_is_a_failure() { + let provider = serve(Reply { + status: "503 Service Unavailable", + headers: &[], + body: "{}", + announce_length: true, + }) + .await; + + let error = client_for(&provider.base_url) + .artist_get_info("Björk", "the-key") + .await + .expect_err("an outage must not read as an unknown artist"); + + assert!( + matches!(error, LookupError::Status { status: 503 }), + "{error:?}" + ); + } } diff --git a/src-tauri/crates/core/src/metadata/mod.rs b/src-tauri/crates/core/src/metadata/mod.rs index 6d56cba92..5c0fd0ed8 100644 --- a/src-tauri/crates/core/src/metadata/mod.rs +++ b/src-tauri/crates/core/src/metadata/mod.rs @@ -3,8 +3,14 @@ //! //! Every client here is a plain `reqwest::Client` wrapper — no Tauri, //! no database, no filesystem. They are designed to be reusable from -//! both the desktop app (`crates/app`) and the future -//! `waveflow-server` (RFC-001 §6.2) without any glue. +//! both the desktop app (`crates/app`) and `waveflow-server` without any +//! glue. +//! +//! Each client has two constructors. `new()` builds its own HTTP client +//! and aims at the provider's public address, which is what the desktop +//! wants. `with_http` takes both from the caller, because a server has +//! rules the desktop does not: it follows no redirect, and its tests +//! point every outbound call at a listener of their own. pub mod album_match; pub mod deezer; @@ -12,3 +18,216 @@ pub mod lastfm; pub mod lrclib; pub mod name_match; pub mod theaudiodb; + +/// The most a provider's answer may weigh before it is refused unread. +/// +/// The largest real answer is a Last.fm biography or a page of a Deezer +/// tracklist, a few tens of kilobytes. The ceiling is two orders of +/// magnitude above that, so it never trims a genuine answer and still +/// stops a response that would otherwise be read into memory whole. +pub const MAX_RESPONSE_BYTES: usize = 2 * 1024 * 1024; + +/// Why a read-only lookup produced no answer — as opposed to the +/// provider answering that it knows nothing, which is `Ok(None)` or an +/// empty list and is worth remembering. Every variant here is worth +/// asking again. +#[derive(Debug, thiserror::Error)] +pub enum LookupError { + /// Built through `From`, which strips the URL — see [`redact`]. + #[error("request failed: {0}")] + Http(reqwest::Error), + /// The provider answered, and its answer was a refusal: a bad key, a + /// suspended one, a quota spent. Not "nothing matched" — reading it + /// as that would remember a throttled minute as an artist with no + /// biography. + #[error("the provider refused the request (code {code})")] + Refused { code: i64 }, + /// A failure status with no body the provider's own error format + /// could be read from. + #[error("the provider answered HTTP {status}")] + Status { status: u16 }, + #[error("the answer exceeds {limit} bytes")] + TooLarge { limit: usize }, + #[error("the answer is not the expected JSON: {0}")] + Decode(#[from] serde_json::Error), +} + +/// Why a body could not be read: the only two failures [`read_bounded`] +/// can produce, kept apart from [`LookupError`] so a client with its own +/// error type converts them without a case that cannot happen. +#[derive(Debug, thiserror::Error)] +pub enum BodyError { + /// Built through `From`, which strips the URL — see [`redact`]. + #[error("request failed: {0}")] + Http(reqwest::Error), + #[error("the answer exceeds {limit} bytes")] + TooLarge { limit: usize }, +} + +/// A transport error without the URL it was raised on. +/// +/// `reqwest` prints the request URL in both `Display` and `Debug`, and +/// these URLs carry the caller's key — in Last.fm's query string, in +/// TheAudioDB's path. Every caller logs the error it gets, so an error +/// that kept its URL would write the key into the log on the first +/// network hiccup. +fn redact(err: reqwest::Error) -> reqwest::Error { + err.without_url() +} + +impl From for LookupError { + fn from(err: reqwest::Error) -> Self { + LookupError::Http(redact(err)) + } +} + +impl From for BodyError { + fn from(err: reqwest::Error) -> Self { + BodyError::Http(redact(err)) + } +} + +impl From for LookupError { + fn from(err: BodyError) -> Self { + match err { + BodyError::Http(err) => LookupError::Http(err), + BodyError::TooLarge { limit } => LookupError::TooLarge { limit }, + } + } +} + +/// A response body read whole, unless it outgrows `limit`. +/// +/// `Content-Length` is checked first so an announced giant is refused +/// before a byte of it is read; the running count covers a response that +/// announces nothing, or lies. +pub(crate) async fn read_bounded( + mut response: reqwest::Response, + limit: usize, +) -> Result, BodyError> { + if response + .content_length() + .is_some_and(|length| length > limit as u64) + { + return Err(BodyError::TooLarge { limit }); + } + let mut body = Vec::new(); + while let Some(chunk) = response.chunk().await? { + if body.len() + chunk.len() > limit { + return Err(BodyError::TooLarge { limit }); + } + body.extend_from_slice(&chunk); + } + Ok(body) +} + +/// Builds the client `new()` constructors use: the desktop's defaults, +/// unchanged from what each module used to build for itself. +pub(crate) fn default_http(user_agent: &str, timeout_secs: u64) -> reqwest::Client { + reqwest::Client::builder() + .user_agent(user_agent) + .timeout(std::time::Duration::from_secs(timeout_secs)) + .build() + .expect("failed to build reqwest client") +} + +/// A canned HTTP listener for the tests in this module's children: it +/// answers every connection with the same response, verbatim, and keeps +/// the head of each request it received — which is how a test proves a +/// redirect was *not* followed, or that a key went where it belongs. +#[cfg(test)] +pub(crate) mod test_server { + use std::sync::{Arc, Mutex}; + + use tokio::io::{AsyncReadExt, AsyncWriteExt}; + use tokio::net::TcpListener; + + pub(crate) struct Served { + pub base_url: String, + requests: Arc>>, + } + + impl Served { + /// How many connections the listener answered. + pub(crate) fn hits(&self) -> usize { + self.requests.lock().unwrap().len() + } + + /// The request line of the `index`th request, e.g. + /// `GET /2.0/?method=… HTTP/1.1`. + pub(crate) fn request_line(&self, index: usize) -> String { + let requests = self.requests.lock().unwrap(); + requests[index] + .lines() + .next() + .unwrap_or_default() + .to_owned() + } + } + + pub(crate) struct Reply<'a> { + /// e.g. `"200 OK"`. + pub status: &'a str, + pub headers: &'a [&'a str], + pub body: &'a str, + /// Whether to send `content-length`. Without it the body runs to + /// the end of the connection, which is the shape a ceiling that + /// only trusted the header would let through. + pub announce_length: bool, + } + + pub(crate) async fn serve(reply: Reply<'_>) -> Served { + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let address = listener.local_addr().unwrap(); + let mut response = format!("HTTP/1.1 {}\r\n", reply.status); + for header in reply.headers { + response.push_str(header); + response.push_str("\r\n"); + } + response.push_str("content-type: application/json\r\nconnection: close\r\n"); + if reply.announce_length { + response.push_str(&format!("content-length: {}\r\n", reply.body.len())); + } + response.push_str("\r\n"); + response.push_str(reply.body); + let requests = Arc::new(Mutex::new(Vec::new())); + let seen = requests.clone(); + tokio::spawn(async move { + loop { + let Ok((mut socket, _)) = listener.accept().await else { + return; + }; + let mut head = [0u8; 4096]; + let read = socket.read(&mut head).await.unwrap_or(0); + seen.lock() + .unwrap() + .push(String::from_utf8_lossy(&head[..read]).into_owned()); + let _ = socket.write_all(response.as_bytes()).await; + let _ = socket.shutdown().await; + } + }); + Served { + base_url: format!("http://{address}"), + requests, + } + } + + /// A `200 OK` carrying `body`, with its length announced. + pub(crate) async fn serve_json(body: &str) -> Served { + serve(Reply { + status: "200 OK", + headers: &[], + body, + announce_length: true, + }) + .await + } + + /// The client a server would hand in: no redirect followed. + pub(crate) fn strict_http() -> reqwest::Client { + reqwest::Client::builder() + .redirect(reqwest::redirect::Policy::none()) + .build() + .unwrap() + } +} diff --git a/src-tauri/crates/core/src/metadata/theaudiodb.rs b/src-tauri/crates/core/src/metadata/theaudiodb.rs index 1b4af62c3..654ce5511 100644 --- a/src-tauri/crates/core/src/metadata/theaudiodb.rs +++ b/src-tauri/crates/core/src/metadata/theaudiodb.rs @@ -27,6 +27,7 @@ use serde::Deserialize; use crate::metadata::name_match::{normalize_name, select_by_name}; +use crate::metadata::{default_http, read_bounded, LookupError, MAX_RESPONSE_BYTES}; const BASE_URL: &str = "https://www.theaudiodb.com/api/v1/json"; const FREE_API_KEY: &str = "123"; @@ -150,6 +151,8 @@ impl TheAudioDbArtist { pub struct TheAudioDbClient { http: reqwest::Client, + base_url: String, + api_key: String, } impl Default for TheAudioDbClient { @@ -159,13 +162,30 @@ impl Default for TheAudioDbClient { } impl TheAudioDbClient { + /// The desktop's client, on the shared free key — see the module + /// header for why one user stays under its cap. pub fn new() -> Self { - let http = reqwest::Client::builder() - .user_agent(USER_AGENT) - .timeout(std::time::Duration::from_secs(TIMEOUT_SECS)) - .build() - .expect("failed to build reqwest client"); - Self { http } + Self::with_http( + default_http(USER_AGENT, TIMEOUT_SECS), + BASE_URL, + FREE_API_KEY, + ) + } + + /// A client over `http`, aimed at `base_url` (the root that ends in + /// `/json`), asking under `api_key`. The free key is a test key the + /// whole internet shares: anything serving more than one person + /// should bring its own. + pub fn with_http( + http: reqwest::Client, + base_url: impl Into, + api_key: impl Into, + ) -> Self { + Self { + http, + base_url: base_url.into(), + api_key: api_key.into(), + } } /// Look up an artist by name, returning its bio in `lang` (English @@ -173,21 +193,23 @@ impl TheAudioDbClient { /// nothing matches the name — a match with neither bio nor fanart /// still comes back as `Some` with both fields empty, so the caller /// can cache the "looked it up, nothing there" outcome instead of - /// re-querying a rate-limited API on every visit. + /// re-querying a rate-limited API on every visit. A failure status + /// is an `Err`: TheAudioDB has no in-band error to read instead. pub async fn artist_info( &self, name: &str, lang: &str, - ) -> reqwest::Result> { - let url = format!("{BASE_URL}/{FREE_API_KEY}/search.php"); - let resp: SearchResponse = self - .http - .get(url) - .query(&[("s", name)]) - .send() - .await? - .json() - .await?; + ) -> Result, LookupError> { + let url = format!("{}/{}/search.php", self.base_url, self.api_key); + let response = self.http.get(url).query(&[("s", name)]).send().await?; + let status = response.status(); + if !status.is_success() { + return Err(LookupError::Status { + status: status.as_u16(), + }); + } + let resp: SearchResponse = + serde_json::from_slice(&read_bounded(response, MAX_RESPONSE_BYTES).await?)?; let searched = normalize_name(name); let Some(artist) = resp @@ -327,4 +349,80 @@ mod tests { assert!(payload.fanart_urls().is_empty()); assert_eq!(ArtistPayload::default().fanart_urls(), Vec::::new()); } + + use crate::metadata::test_server::{serve, serve_json, strict_http, Reply}; + + #[tokio::test] + async fn the_operators_key_goes_where_the_free_one_used_to() { + let provider = serve_json( + r#"{"artists":[{"strArtist":"Björk","strBiography":"Born in Reykjavík."}]}"#, + ) + .await; + let client = TheAudioDbClient::with_http( + strict_http(), + format!("{}/api/v1/json", provider.base_url), + "operator-key", + ); + + let artist = client + .artist_info("Björk", "en") + .await + .unwrap() + .expect("a match"); + + assert_eq!(artist.bio_full.as_deref(), Some("Born in Reykjavík.")); + let line = provider.request_line(0); + assert!( + line.starts_with("GET /api/v1/json/operator-key/search.php?"), + "{line}" + ); + assert!(!line.contains("/123/"), "{line}"); + } + + #[tokio::test] + async fn a_failure_status_is_a_failure_and_not_an_absence() { + let provider = serve(Reply { + status: "503 Service Unavailable", + headers: &[], + body: "", + announce_length: true, + }) + .await; + let client = TheAudioDbClient::with_http(strict_http(), provider.base_url.clone(), "k"); + + let error = client + .artist_info("Björk", "en") + .await + .expect_err("an outage must not read as an unknown artist"); + + assert!( + matches!(error, LookupError::Status { status: 503 }), + "{error:?}" + ); + } + + /// TheAudioDB takes its key in the path rather than the query, which + /// a redaction aimed at query strings alone would miss. + #[tokio::test] + async fn a_transport_error_does_not_carry_the_key() { + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let address = listener.local_addr().unwrap(); + drop(listener); + let client = TheAudioDbClient::with_http( + strict_http(), + format!("http://{address}"), + "the-secret-key", + ); + + let error = client + .artist_info("Björk", "en") + .await + .expect_err("nothing is listening"); + + assert!(matches!(error, LookupError::Http(_)), "{error:?}"); + assert!( + !format!("{error:?}").contains("the-secret-key"), + "{error:?}" + ); + } } From 60d44c735a87755048ea38563aa83a689772d4b1 Mon Sep 17 00:00:00 2001 From: InstaZDLL Date: Wed, 14 Oct 2026 20:35:29 +0200 Subject: [PATCH 2/8] docs(metadata): a refused key is a failure, not an empty answer Signed-off-by: InstaZDLL --- docs/features/integrations.md | 2 +- docs/features/library.md | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/features/integrations.md b/docs/features/integrations.md index e7209788a..44ec5f091 100644 --- a/docs/features/integrations.md +++ b/docs/features/integrations.md @@ -39,7 +39,7 @@ The frontend helper `lib/tauri/artwork.ts::resolveRemoteImage` prefers the local ### Read-only (artist bios) -`artist.getInfo` for biographies, called from `enrich_artist_deezer` after the Deezer pass. Cached in the same `metadata_artist` row with the same 30-day TTL. **Optional**: requires a user-supplied API key in `app_setting['lastfm_api_key']`. Without it, bios are skipped silently and the UI shows local data. +`artist.getInfo` for biographies, called from `enrich_artist_deezer` after the Deezer pass. Cached in the same `metadata_artist` row with the same 30-day TTL. **Optional**: requires a user-supplied API key in `app_setting['lastfm_api_key']`. Without it, bios are skipped silently and the UI shows local data. Only Last.fm's error 6 ("artist not found") reads as an empty answer; any other code — a refused key, a spent quota — is an `Err`, so it is retried rather than cached as an artist with no biography. **Collapse toggle (issue #422).** A long bio pushes the discography off the artist page, so the bio block carries an inline Show/Hide chevron ([`ArtistDetailView`](../../src/components/views/ArtistDetailView.tsx) + [`useArtistBioCollapsed`](../../src/hooks/useArtistBioCollapsed.ts)). The preference is **global per-profile** (`profile_setting['artist.bio_collapsed']`, default shown), not per-artist and not in Settings — toggling on any artist page is remembered across every artist. Distinct from the existing "read more/less" which only swaps the short/full bio text; collapse hides the whole block so the tracklist moves up. diff --git a/docs/features/library.md b/docs/features/library.md index f0ea48178..e3e080c13 100644 --- a/docs/features/library.md +++ b/docs/features/library.md @@ -290,7 +290,7 @@ All three commands emit **`artist:updated`** with the artist id. Without it only Everything above carries the **square** artist photo. The [artist hero](ui.md#artist-hero) needs a **wide** one, and the only source in the stack that has one is TheAudioDB (issue #482). -[`metadata::theaudiodb`](../../src-tauri/crates/core/src/metadata/theaudiodb.rs) already queried `search.php` for multi-language bios; the same response carries `strArtistFanart` (+ `2/3/4`), `strArtistWideThumb` and `strArtistBanner`. `TheAudioDbClient::artist_info` returns bio **and** `fanart_urls` from one lookup. It returns `Some` for any name match even with neither bio nor fanart, so the caller can cache the "looked, nothing there" outcome. +[`metadata::theaudiodb`](../../src-tauri/crates/core/src/metadata/theaudiodb.rs) already queried `search.php` for multi-language bios; the same response carries `strArtistFanart` (+ `2/3/4`), `strArtistWideThumb` and `strArtistBanner`. `TheAudioDbClient::artist_info` returns bio **and** `fanart_urls` from one lookup. It returns `Some` for any name match even with neither bio nor fanart, so the caller can cache the "looked, nothing there" outcome; a failure status is an `Err`, never that outcome. **All four fanarts come back, and the strips no longer do** (issue #693). The client used to fall back to `strArtistWideThumb` and `strArtistBanner` when an artist had no fanart; both are **1000×185** strips by TheAudioDB's own spec, and the hero they were feeding is routinely wider than 1000 px and far taller than 185 px relative to its width — so they arrived upscaled and cropped to a sliver, which is what "the backdrop is blurry" looked like. They are not deserialized at all now; an artist with no fanart falls through to the blurred-photo tier, which is honest about being an ambient colour field rather than a picture. The fanarts themselves are kept **all four**, because they differ enormously in framing and the picker below lets the user choose. From 459200a40ae4fd50ee6d6c0c2cc45bb3866393bc Mon Sep 17 00:00:00 2001 From: InstaZDLL Date: Wed, 14 Oct 2026 21:22:21 +0200 Subject: [PATCH 3/8] fix(metadata): a key never leaves in clear, and a signed form never leaves its origin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CodeQL flagged TheAudioDB's key travelling to an address with_http accepted unchecked, and the alert was right: before with_http the address was a constant https URL, and now a caller can pass http. Last.fm has the same exposure, with a key in its query string and a session key or a password in its signed forms. with_http on both now refuses a plaintext address unless it is this machine, where a test's listener or a local stand-in lives and there is no wire to read. That vets the address a caller chooses, not the ones a provider chooses afterwards: new()'s client followed any redirect, and on a 307 or 308 reqwest sends the body again — the signed form — wherever it points. It now follows a redirect only inside the origin first asked, which also forbids falling back to plaintext. From the review of the first push: - the three signed methods read through the same ceiling as the rest, with a TooLarge error of their own rather than a fake API error; - LastfmError and DeezerError strip the URL from transport errors, as LookupError already did. The signed calls carry no secret in theirs, but Deezer's carries the search, and the earlier commit claimed this of every transport error in the module; - a mobile-session reply that fails to decode no longer puts its body in the error message, since that body can hold the session key. The redirect test asked for a target on another port, which the desktop's client now refuses as well, so it could no longer tell the injected client from the default one. It now redirects inside the origin, which only the injected client refuses. Seventeen inversions, each caught by the test that names it. Signed-off-by: InstaZDLL --- src-tauri/crates/core/src/metadata/deezer.rs | 31 +++- src-tauri/crates/core/src/metadata/lastfm.rs | 142 +++++++++++++++-- src-tauri/crates/core/src/metadata/mod.rs | 146 +++++++++++++++++- .../crates/core/src/metadata/theaudiodb.rs | 44 ++++-- 4 files changed, 331 insertions(+), 32 deletions(-) diff --git a/src-tauri/crates/core/src/metadata/deezer.rs b/src-tauri/crates/core/src/metadata/deezer.rs index 5d23ccc52..8db4d89eb 100644 --- a/src-tauri/crates/core/src/metadata/deezer.rs +++ b/src-tauri/crates/core/src/metadata/deezer.rs @@ -86,8 +86,10 @@ impl DeezerApiError { /// looked exactly alike (#406 was diagnosed through that fog). #[derive(Debug, thiserror::Error)] pub enum DeezerError { + /// Built through `From`, which strips the URL: it carries the search + /// query, and a caller that logs this error must not log that. #[error("deezer request failed: {0}")] - Http(#[from] reqwest::Error), + Http(reqwest::Error), #[error("deezer refused the request: {0}")] Api(DeezerApiError), /// Throttling that arrived as a status code rather than in the body. @@ -103,6 +105,12 @@ pub enum DeezerError { Decode(#[from] serde_json::Error), } +impl From for DeezerError { + fn from(err: reqwest::Error) -> Self { + DeezerError::Http(err.without_url()) + } +} + impl From for DeezerError { fn from(err: BodyError) -> Self { match err { @@ -594,4 +602,25 @@ mod tests { assert!(matches!(error, DeezerError::TooLarge { .. }), "{error:?}"); assert!(!error.is_quota_exceeded()); } + + /// The search travels in the query string, which a caller that logs + /// this error must not log. + #[tokio::test] + async fn a_transport_error_does_not_carry_the_search() { + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let address = listener.local_addr().unwrap(); + drop(listener); + let client = DeezerClient::with_http(strict_http(), format!("http://{address}")); + + let error = client + .search_artist("the-searched-name") + .await + .expect_err("nothing is listening"); + + assert!(matches!(error, DeezerError::Http(_)), "{error:?}"); + assert!( + !format!("{error:?}").contains("the-searched-name"), + "{error:?}" + ); + } } diff --git a/src-tauri/crates/core/src/metadata/lastfm.rs b/src-tauri/crates/core/src/metadata/lastfm.rs index dd10a16b5..b59c08570 100644 --- a/src-tauri/crates/core/src/metadata/lastfm.rs +++ b/src-tauri/crates/core/src/metadata/lastfm.rs @@ -19,7 +19,10 @@ use md5::{Digest, Md5}; use regex::Regex; use serde::Deserialize; -use super::{default_http, read_bounded, LookupError, MAX_RESPONSE_BYTES}; +use super::{ + default_http, read_bounded, require_secure, BaseUrlError, BodyError, LookupError, + MAX_RESPONSE_BYTES, +}; const BASE_URL: &str = "https://ws.audioscrobbler.com/2.0/"; const USER_AGENT: &str = "WaveFlow/0.1"; @@ -117,18 +120,27 @@ async fn read_reply( impl LastfmClient { pub fn new() -> Self { - Self::with_http(default_http(USER_AGENT, TIMEOUT_SECS), BASE_URL) + Self { + http: default_http(USER_AGENT, TIMEOUT_SECS), + base_url: BASE_URL.to_owned(), + } } /// A client over `http`, aimed at `base_url` — the API root, the one /// that ends in `/2.0/`. Last.fm answers `/2.0` without the slash /// with a redirect, which a client that follows none would read as a /// failure. - pub fn with_http(http: reqwest::Client, base_url: impl Into) -> Self { - Self { + /// + /// Refused unless `base_url` is `https` or this machine: every call + /// carries a key, and the signed ones a session key and a password. + pub fn with_http( + http: reqwest::Client, + base_url: impl Into, + ) -> Result { + Ok(Self { http, - base_url: base_url.into(), - } + base_url: require_secure(base_url.into())?, + }) } /// Fetch an artist's bio via `artist.getInfo`. Returns `Ok(None)` @@ -321,10 +333,35 @@ pub fn is_lastfm_star_placeholder(url: &str) -> bool { /// `message` payload that comes back with a 200 OK. #[derive(Debug, thiserror::Error)] pub enum LastfmError { + /// Built through `From`, which strips the URL, as every transport + /// error in this module is. #[error("network: {0}")] - Http(#[from] reqwest::Error), + Http(reqwest::Error), #[error("Last.fm API error {code}: {message}")] Api { code: i64, message: String }, + #[error("Last.fm's answer exceeds {limit} bytes")] + TooLarge { limit: usize }, +} + +impl From for LastfmError { + fn from(err: reqwest::Error) -> Self { + LastfmError::Http(err.without_url()) + } +} + +impl From for LastfmError { + fn from(err: BodyError) -> Self { + match err { + BodyError::Http(err) => LastfmError::Http(err), + BodyError::TooLarge { limit } => LastfmError::TooLarge { limit }, + } + } +} + +/// A signed method's answer, read through the same ceiling as every other. +async fn read_signed_body(response: reqwest::Response) -> Result { + let body = read_bounded(response, MAX_RESPONSE_BYTES).await?; + Ok(String::from_utf8_lossy(&body).into_owned()) } /// Compute the API signature for a Last.fm signed request: sort the @@ -405,7 +442,7 @@ impl LastfmClient { .send() .await?; - let body = resp.text().await?; + let body = read_signed_body(resp).await?; if let Ok(err) = serde_json::from_str::(&body) { return Err(LastfmError::Api { code: err.error, @@ -415,7 +452,9 @@ impl LastfmClient { let parsed: MobileSessionResponse = serde_json::from_str(&body).map_err(|e| LastfmError::Api { code: -1, - message: format!("decode mobile session: {e}; body={body}"), + // Not the body: a reply that fails to decode may still + // carry a session key, and this message is logged. + message: format!("decode mobile session: {e}"), })?; let Some(session) = parsed.session else { return Err(LastfmError::Api { @@ -477,7 +516,7 @@ impl LastfmClient { form.push(("format", "json")); let resp = self.http.post(&self.base_url).form(&form).send().await?; - let body = resp.text().await?; + let body = read_signed_body(resp).await?; if let Ok(err) = serde_json::from_str::(&body) { return Err(LastfmError::Api { code: err.error, @@ -525,7 +564,7 @@ impl LastfmClient { form.push(("format", "json")); let resp = self.http.post(&self.base_url).form(&form).send().await?; - let body = resp.text().await?; + let body = read_signed_body(resp).await?; if let Ok(err) = serde_json::from_str::(&body) { return Err(LastfmError::Api { code: err.error, @@ -593,7 +632,7 @@ mod tests { use crate::metadata::test_server::{serve, serve_json, strict_http, Reply}; fn client_for(base_url: &str) -> LastfmClient { - LastfmClient::with_http(strict_http(), format!("{base_url}/2.0/")) + LastfmClient::with_http(strict_http(), format!("{base_url}/2.0/")).unwrap() } #[tokio::test] @@ -671,12 +710,15 @@ mod tests { } #[tokio::test] + /// The redirect stays inside the origin on purpose: the desktop's own + /// client follows those, so only the client handed to `with_http` — + /// which follows none — can be the reason this one is refused. A + /// redirect to another origin would be refused by either, and could + /// not tell them apart. async fn a_redirect_is_not_followed_by_a_client_that_follows_none() { - let elsewhere = serve_json(r#"{"artist":{"name":"Björk"}}"#).await; - let location = format!("location: {}/2.0/", elsewhere.base_url); let provider = serve(Reply { status: "302 Found", - headers: &[&location], + headers: &["location: /2.0/moved"], body: "", announce_length: true, }) @@ -691,7 +733,7 @@ mod tests { matches!(error, LookupError::Status { status: 302 }), "{error:?}" ); - assert_eq!(elsewhere.hits(), 0, "the redirect target was contacted"); + assert_eq!(provider.hits(), 1, "the redirect was followed"); } #[tokio::test] @@ -741,6 +783,7 @@ mod tests { drop(listener); let error = LastfmClient::with_http(strict_http(), format!("http://{address}/2.0/")) + .unwrap() .artist_get_info("Björk", "the-secret-key") .await .expect_err("nothing is listening"); @@ -773,4 +816,71 @@ mod tests { "{error:?}" ); } + + #[tokio::test] + async fn a_signed_answer_over_the_ceiling_is_refused_even_unannounced() { + let padding = "x".repeat(MAX_RESPONSE_BYTES); + let body = format!(r#"{{"session":{{"name":"{padding}","key":"k"}}}}"#); + let provider = serve(Reply { + status: "200 OK", + headers: &[], + body: &body, + announce_length: false, + }) + .await; + + let error = client_for(&provider.base_url) + .auth_get_mobile_session("the-key", "the-secret", "someone", "a-password") + .await + .expect_err("an oversized answer must not be read whole"); + + assert!(matches!(error, LastfmError::TooLarge { .. }), "{error:?}"); + } + + /// A reply that fails to decode can still carry the session key, which + /// is good until revoked — and the error it becomes is logged. + #[tokio::test] + async fn a_session_reply_that_fails_to_decode_does_not_carry_its_key() { + let provider = serve_json(r#"{"session":{"key":"the-session-key"}}"#).await; + + let error = client_for(&provider.base_url) + .auth_get_mobile_session("the-key", "the-secret", "someone", "a-password") + .await + .expect_err("a session without a name does not decode"); + + assert!(!format!("{error}").contains("the-session-key"), "{error}"); + assert!( + !format!("{error:?}").contains("the-session-key"), + "{error:?}" + ); + } + + #[tokio::test] + async fn a_signed_transport_error_does_not_carry_its_url() { + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let address = listener.local_addr().unwrap(); + drop(listener); + let client = LastfmClient::with_http( + strict_http(), + format!("http://{address}/a-path-worth-hiding/2.0/"), + ) + .unwrap(); + + let error = client + .auth_get_mobile_session("the-key", "the-secret", "someone", "a-password") + .await + .expect_err("nothing is listening"); + + assert!(matches!(error, LastfmError::Http(_)), "{error:?}"); + assert!( + !format!("{error:?}").contains("a-path-worth-hiding"), + "{error:?}" + ); + } + + #[test] + fn a_plaintext_address_off_this_machine_is_refused() { + let refused = LastfmClient::with_http(strict_http(), "http://ws.audioscrobbler.com/2.0/"); + assert!(matches!(refused, Err(BaseUrlError::Plaintext { .. }))); + } } diff --git a/src-tauri/crates/core/src/metadata/mod.rs b/src-tauri/crates/core/src/metadata/mod.rs index 5c0fd0ed8..281f36c7f 100644 --- a/src-tauri/crates/core/src/metadata/mod.rs +++ b/src-tauri/crates/core/src/metadata/mod.rs @@ -121,12 +121,75 @@ pub(crate) async fn read_bounded( Ok(body) } -/// Builds the client `new()` constructors use: the desktop's defaults, -/// unchanged from what each module used to build for itself. +/// Why a client that carries a key refused the address it was given. +#[derive(Debug, thiserror::Error)] +pub enum BaseUrlError { + #[error("not an absolute URL")] + Invalid, + /// The key — in Last.fm's query string, in TheAudioDB's path — would + /// cross the network readable by anyone on the way. + #[error("a key would travel in clear to {host}; the address must be https")] + Plaintext { host: String }, +} + +/// `base_url`, if a key can be sent to it. +/// +/// `https` always; plain `http` only to this machine, which is where a +/// test's listener or a developer's local stand-in lives and where there +/// is no wire to read the key from. Every other plaintext address is +/// refused before a request exists to carry the key. +pub(crate) fn require_secure(base_url: String) -> Result { + let parsed = reqwest::Url::parse(&base_url).map_err(|_| BaseUrlError::Invalid)?; + let host = parsed.host_str().ok_or(BaseUrlError::Invalid)?; + // An IPv6 host comes back bracketed, `[::1]`. + let loopback = host.eq_ignore_ascii_case("localhost") + || host + .trim_start_matches('[') + .trim_end_matches(']') + .parse::() + .is_ok_and(|address| address.is_loopback()); + match parsed.scheme() { + "https" => Ok(base_url), + "http" if loopback => Ok(base_url), + _ => Err(BaseUrlError::Plaintext { + host: host.to_owned(), + }), + } +} + +/// How many redirects a `new()` client follows, as `reqwest` did by default. +const MAX_REDIRECTS: usize = 10; + +/// The redirect rule of a `new()` client: follow, but never out of the +/// origin first asked. +/// +/// The origin includes the scheme, so this also forbids falling back to +/// plaintext. It matters for the signed Last.fm calls above all: on a +/// `307` or `308`, `reqwest` sends the body again — a form carrying a +/// session key or a password — to wherever the redirect points. +/// `require_secure` vets the address a caller chose; this vets the ones +/// the provider chooses afterwards. +fn same_origin_only() -> reqwest::redirect::Policy { + reqwest::redirect::Policy::custom(|attempt| { + let first = attempt.previous().first().map(reqwest::Url::origin); + if attempt.previous().len() > MAX_REDIRECTS { + attempt.error("too many redirects") + } else if first.as_ref() == Some(&attempt.url().origin()) { + attempt.follow() + } else { + attempt.stop() + } + }) +} + +/// Builds the client `new()` constructors use: the desktop's timeout and +/// user agent, as each module used to build for itself, and redirects +/// kept inside the origin. pub(crate) fn default_http(user_agent: &str, timeout_secs: u64) -> reqwest::Client { reqwest::Client::builder() .user_agent(user_agent) .timeout(std::time::Duration::from_secs(timeout_secs)) + .redirect(same_origin_only()) .build() .expect("failed to build reqwest client") } @@ -231,3 +294,82 @@ pub(crate) mod test_server { .unwrap() } } + +#[cfg(test)] +mod tests { + use super::test_server::{serve, serve_json, Reply}; + use super::{default_http, require_secure, BaseUrlError}; + + /// The desktop's client follows a redirect only inside the origin it + /// asked: another port is another origin, so the target is never + /// contacted and the redirect comes back as the answer. + #[tokio::test] + async fn the_desktop_client_does_not_follow_a_redirect_out_of_its_origin() { + let elsewhere = serve_json("{}").await; + let location = format!("location: {}/", elsewhere.base_url); + let asked = serve(Reply { + status: "307 Temporary Redirect", + headers: &[&location], + body: "", + announce_length: true, + }) + .await; + + let response = default_http("test", 5) + .post(format!("{}/", asked.base_url)) + .form(&[("sk", "a-session-key")]) + .send() + .await + .unwrap(); + + assert_eq!(response.status().as_u16(), 307); + assert_eq!(elsewhere.hits(), 0, "the signed form was sent elsewhere"); + } + + fn verdict(url: &str) -> Result<(), String> { + require_secure(url.to_owned()) + .map(|_| ()) + .map_err(|err| match err { + BaseUrlError::Invalid => "invalid".to_owned(), + BaseUrlError::Plaintext { host } => format!("plaintext to {host}"), + }) + } + + #[test] + fn https_is_accepted_anywhere() { + assert_eq!(verdict("https://ws.audioscrobbler.com/2.0/"), Ok(())); + } + + #[test] + fn plaintext_is_accepted_to_this_machine_only() { + assert_eq!(verdict("http://127.0.0.1:8080/2.0/"), Ok(())); + assert_eq!(verdict("http://[::1]:8080/2.0/"), Ok(())); + assert_eq!(verdict("http://localhost/2.0/"), Ok(())); + assert_eq!( + verdict("http://ws.audioscrobbler.com/2.0/"), + Err("plaintext to ws.audioscrobbler.com".to_owned()) + ); + } + + /// A name that merely starts like a loopback address is a remote host. + #[test] + fn a_host_that_only_looks_local_is_remote() { + assert_eq!( + verdict("http://127.0.0.1.attacker.example/"), + Err("plaintext to 127.0.0.1.attacker.example".to_owned()) + ); + assert_eq!( + verdict("http://localhost.attacker.example/"), + Err("plaintext to localhost.attacker.example".to_owned()) + ); + } + + #[test] + fn another_scheme_or_no_url_is_refused() { + assert!(verdict("ftp://127.0.0.1/").is_err()); + assert_eq!( + verdict("ws.audioscrobbler.com/2.0/"), + Err("invalid".to_owned()) + ); + } +} diff --git a/src-tauri/crates/core/src/metadata/theaudiodb.rs b/src-tauri/crates/core/src/metadata/theaudiodb.rs index 654ce5511..d545cecd8 100644 --- a/src-tauri/crates/core/src/metadata/theaudiodb.rs +++ b/src-tauri/crates/core/src/metadata/theaudiodb.rs @@ -27,7 +27,9 @@ use serde::Deserialize; use crate::metadata::name_match::{normalize_name, select_by_name}; -use crate::metadata::{default_http, read_bounded, LookupError, MAX_RESPONSE_BYTES}; +use crate::metadata::{ + default_http, read_bounded, require_secure, BaseUrlError, LookupError, MAX_RESPONSE_BYTES, +}; const BASE_URL: &str = "https://www.theaudiodb.com/api/v1/json"; const FREE_API_KEY: &str = "123"; @@ -165,27 +167,30 @@ impl TheAudioDbClient { /// The desktop's client, on the shared free key — see the module /// header for why one user stays under its cap. pub fn new() -> Self { - Self::with_http( - default_http(USER_AGENT, TIMEOUT_SECS), - BASE_URL, - FREE_API_KEY, - ) + Self { + http: default_http(USER_AGENT, TIMEOUT_SECS), + base_url: BASE_URL.to_owned(), + api_key: FREE_API_KEY.to_owned(), + } } /// A client over `http`, aimed at `base_url` (the root that ends in /// `/json`), asking under `api_key`. The free key is a test key the /// whole internet shares: anything serving more than one person /// should bring its own. + /// + /// Refused unless `base_url` is `https` or this machine: the key + /// travels in the path. pub fn with_http( http: reqwest::Client, base_url: impl Into, api_key: impl Into, - ) -> Self { - Self { + ) -> Result { + Ok(Self { http, - base_url: base_url.into(), + base_url: require_secure(base_url.into())?, api_key: api_key.into(), - } + }) } /// Look up an artist by name, returning its bio in `lang` (English @@ -362,7 +367,8 @@ mod tests { strict_http(), format!("{}/api/v1/json", provider.base_url), "operator-key", - ); + ) + .unwrap(); let artist = client .artist_info("Björk", "en") @@ -388,7 +394,8 @@ mod tests { announce_length: true, }) .await; - let client = TheAudioDbClient::with_http(strict_http(), provider.base_url.clone(), "k"); + let client = + TheAudioDbClient::with_http(strict_http(), provider.base_url.clone(), "k").unwrap(); let error = client .artist_info("Björk", "en") @@ -412,7 +419,8 @@ mod tests { strict_http(), format!("http://{address}"), "the-secret-key", - ); + ) + .unwrap(); let error = client .artist_info("Björk", "en") @@ -425,4 +433,14 @@ mod tests { "{error:?}" ); } + + #[test] + fn a_plaintext_address_off_this_machine_is_refused() { + let refused = TheAudioDbClient::with_http( + strict_http(), + "http://www.theaudiodb.com/api/v1/json", + "operator-key", + ); + assert!(matches!(refused, Err(BaseUrlError::Plaintext { .. }))); + } } From 11d394753b3c91e011bb8783e8e3740a84688ee5 Mon Sep 17 00:00:00 2001 From: InstaZDLL Date: Wed, 14 Oct 2026 21:37:56 +0200 Subject: [PATCH 4/8] test(metadata): the session tests make their credentials at run time CodeQL read the literal secret and password the three mobile-session tests hand to auth_get_mobile_session as hard-coded credentials, since the call signs them. Those tests are about the body ceiling, the decode error and the redacted URL; the values play no part, so they are now made at run time. Signed-off-by: InstaZDLL --- src-tauri/crates/core/src/metadata/lastfm.rs | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/src-tauri/crates/core/src/metadata/lastfm.rs b/src-tauri/crates/core/src/metadata/lastfm.rs index b59c08570..ccce0be93 100644 --- a/src-tauri/crates/core/src/metadata/lastfm.rs +++ b/src-tauri/crates/core/src/metadata/lastfm.rs @@ -631,6 +631,15 @@ mod tests { use crate::metadata::test_server::{serve, serve_json, strict_http, Reply}; + /// A secret and a password made at run time. Handed to a signed call, + /// a literal is a hard-coded credential to CodeQL whatever the test is + /// about, and these tests are about the body and the error, not the + /// values. + fn throwaway_credentials() -> (String, String) { + let nonce = std::process::id(); + (nonce.to_string(), nonce.wrapping_add(1).to_string()) + } + fn client_for(base_url: &str) -> LastfmClient { LastfmClient::with_http(strict_http(), format!("{base_url}/2.0/")).unwrap() } @@ -819,6 +828,7 @@ mod tests { #[tokio::test] async fn a_signed_answer_over_the_ceiling_is_refused_even_unannounced() { + let (secret, password) = throwaway_credentials(); let padding = "x".repeat(MAX_RESPONSE_BYTES); let body = format!(r#"{{"session":{{"name":"{padding}","key":"k"}}}}"#); let provider = serve(Reply { @@ -830,7 +840,7 @@ mod tests { .await; let error = client_for(&provider.base_url) - .auth_get_mobile_session("the-key", "the-secret", "someone", "a-password") + .auth_get_mobile_session("the-key", &secret, "someone", &password) .await .expect_err("an oversized answer must not be read whole"); @@ -841,10 +851,11 @@ mod tests { /// is good until revoked — and the error it becomes is logged. #[tokio::test] async fn a_session_reply_that_fails_to_decode_does_not_carry_its_key() { + let (secret, password) = throwaway_credentials(); let provider = serve_json(r#"{"session":{"key":"the-session-key"}}"#).await; let error = client_for(&provider.base_url) - .auth_get_mobile_session("the-key", "the-secret", "someone", "a-password") + .auth_get_mobile_session("the-key", &secret, "someone", &password) .await .expect_err("a session without a name does not decode"); @@ -857,6 +868,7 @@ mod tests { #[tokio::test] async fn a_signed_transport_error_does_not_carry_its_url() { + let (secret, password) = throwaway_credentials(); let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); let address = listener.local_addr().unwrap(); drop(listener); @@ -867,7 +879,7 @@ mod tests { .unwrap(); let error = client - .auth_get_mobile_session("the-key", "the-secret", "someone", "a-password") + .auth_get_mobile_session("the-key", &secret, "someone", &password) .await .expect_err("nothing is listening"); From 08750fe8c337ac5cc08aea56c9936d998700cc26 Mon Sep 17 00:00:00 2001 From: InstaZDLL Date: Wed, 14 Oct 2026 22:17:13 +0200 Subject: [PATCH 5/8] fix(artist): a failure to ask keeps the biography and is not cached MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The earlier commits made a refused key, a spent quota or a network failure distinguishable from "no such artist", and the desktop's own caller still collapsed the two. On any failure it wrote an empty biography over the cached one, stamped the selected source on it and gave the row the full thirty days: one bad minute cost an artist their biography for a month. The bio columns now follow the rule the background columns already did. When the provider answered, its answer replaces what was there, empty or not. When it could not be asked, the row keeps the biography and the source it had and expires now, so the next visit asks again; a first visit that fails records no source, so nothing reads the row as a biography looked up. TheAudioDB's bio is held to the same rule through the reach it already tracked. The upsert returns what the row holds afterwards, so a kept biography is still shown — but only if it answers the source and language now selected, as the cache-hit check already required. The write moves into upsert_artist_row so it can be tested against the real app migrations. These tests run on Linux only, like the rest of the app crate's; they were run under WSL with five inversions, each caught by the test that names it. Signed-off-by: InstaZDLL --- src-tauri/crates/app/src/commands/deezer.rs | 377 +++++++++++++++++--- 1 file changed, 322 insertions(+), 55 deletions(-) diff --git a/src-tauri/crates/app/src/commands/deezer.rs b/src-tauri/crates/app/src/commands/deezer.rs index d5ebf4393..78d8b96d3 100644 --- a/src-tauri/crates/app/src/commands/deezer.rs +++ b/src-tauri/crates/app/src/commands/deezer.rs @@ -718,29 +718,33 @@ async fn enrich_artist_named( } }; - // 5. Fetch the bio from the selected source (issue #295). Network - // failures and missing matches are non-fatal — we still persist - // the Deezer portion so the next refresh doesn't spam the - // network. The source/language we used is stored alongside so a - // later switch re-fetches instead of serving the wrong bio. - let (bio_short, bio_full) = match active_source { + // 5. Fetch the bio from the selected source (issue #295). A missing + // match is an answer and is cached like one. A failure to ask — + // the network, a refused key, a spent quota — is not: the row + // keeps the biography it had and expires now, so the next visit + // asks again (see `upsert_artist_row`). The source/language we used + // is stored alongside so a later switch re-fetches instead of + // serving the wrong bio. + let (bio_short, bio_full, bio_reached) = match active_source { BioSource::Lastfm => match read_lastfm_api_key(&state).await? { Some(api_key) => { let lastfm = LastfmClient::new(); match lastfm.artist_get_info(&artist_name, &api_key).await { - Ok(Some(info)) => (info.bio_summary, info.bio_full), - Ok(None) => (None, None), + Ok(Some(info)) => (info.bio_summary, info.bio_full, true), + Ok(None) => (None, None, true), Err(err) => { tracing::warn!(?err, "Last.fm artist_get_info failed"); - (None, None) + (None, None, false) } } } - None => (None, None), + // Nothing was asked, so nothing failed: no key is a setting, + // and the row records it as an absence like before. + None => (None, None, true), }, BioSource::TheAudioDb => match audiodb.as_ref() { - Some(info) => (info.bio_short.clone(), info.bio_full.clone()), - None => (None, None), + Some(info) => (info.bio_short.clone(), info.bio_full.clone(), true), + None => (None, None, audiodb_reached), }, }; @@ -787,10 +791,122 @@ async fn enrich_artist_named( // which provider produced the bio so a later switch invalidates // it (see the cache-hit check above). Language is only meaningful // for TheAudioDB, so Last.fm stores NULL. - let expires = now + CACHE_TTL_MS; let stored_lang: Option<&str> = matches!(active_source, BioSource::TheAudioDb).then_some(active_lang.as_str()); - sqlx::query( + // What the row holds after the write, which is what to show: on a + // failure to ask, the biography it already had rather than nothing. + let (bio_short, bio_full) = upsert_artist_row( + &pool, + &ArtistRow { + deezer_id: hit.id, + name: &hit.name, + picture_url: picture_url.as_deref(), + picture_hash: picture_hash.as_deref(), + fans_count: hit.nb_fan, + bio: BioAnswer { + reached: bio_reached, + short: bio_short.as_deref(), + full: bio_full.as_deref(), + source: active_source.as_str(), + language: stored_lang, + }, + background_url: background_url.as_deref(), + background_hash: background_hash.as_deref(), + // NULL when TheAudioDB could not be reached, so the row still + // reads as "never looked" rather than as "this artist has no + // fanart". + background_urls_json: background_urls_json.as_deref(), + // Stamped even when the lookup came back empty — that's the + // whole point of the column: "we asked, TheAudioDB has + // nothing". Left NULL when the API couldn't be reached at all, + // so that retries. + background_fetched_at: audiodb_reached.then_some(now), + now, + }, + ) + .await?; + + Ok(DeezerArtistEnrichment { + deezer_id: Some(hit.id), + picture_url, + picture_path, + picture_path_1x, + picture_path_2x, + fans_count: hit.nb_fan, + bio_short, + bio_full, + background_url, + background_path, + artwork_path: None, + artwork_path_1x: None, + artwork_path_2x: None, + }) +} + +/// What the selected bio provider said, for one write of the cache row. +struct BioAnswer<'a> { + /// Whether the provider answered at all. `false` is a failure to ask, + /// which must neither erase a cached biography nor be cached itself. + reached: bool, + short: Option<&'a str>, + full: Option<&'a str>, + source: &'a str, + language: Option<&'a str>, +} + +/// One write of an artist's `app.metadata_artist` row. +struct ArtistRow<'a> { + deezer_id: i64, + name: &'a str, + picture_url: Option<&'a str>, + picture_hash: Option<&'a str>, + fans_count: Option, + bio: BioAnswer<'a>, + background_url: Option<&'a str>, + background_hash: Option<&'a str>, + background_urls_json: Option<&'a str>, + background_fetched_at: Option, + now: i64, +} + +/// Upsert the Deezer + bio cache row and return the biography it holds +/// afterwards. +/// +/// The unified `metadata_artist` table lives in app.db so every profile +/// shares the same cache. `bio_source` / `bio_language` record which +/// provider produced the bio so a later switch invalidates it (see the +/// cache-hit check in `enrich_artist_deezer`). Language is only +/// meaningful for TheAudioDB, so Last.fm stores NULL. +/// +/// The bio columns follow the rule the background columns already did. +/// When the provider answered, its answer replaces what was there, empty +/// or not — an artist whose biography was removed upstream must lose it +/// here too, so this tests whether the provider was reached rather than +/// whether the new value is empty. When it was not reached, the row keeps +/// the biography and the source it had, and **expires now**: the Deezer +/// half was fetched and is stored, but the visit that failed to ask for +/// a biography must not stand for thirty days. A fresh row in that case +/// records no source, so nothing reads it as a biography looked up. +async fn upsert_artist_row( + pool: &sqlx::SqlitePool, + row: &ArtistRow<'_>, +) -> Result<(Option, Option), sqlx::Error> { + let expires = if row.bio.reached { + row.now + CACHE_TTL_MS + } else { + row.now + }; + let (source, language) = if row.bio.reached { + (Some(row.bio.source), row.bio.language) + } else { + (None, None) + }; + let (short, full, stored_source, stored_language): ( + Option, + Option, + Option, + Option, + ) = sqlx::query_as( "INSERT INTO app.metadata_artist (deezer_id, name, picture_url, picture_hash, fans_count, bio_short, bio_full, bio_source, bio_language, background_url, background_hash, background_urls, @@ -801,10 +917,10 @@ async fn enrich_artist_named( picture_url = excluded.picture_url, picture_hash = excluded.picture_hash, fans_count = excluded.fans_count, - bio_short = excluded.bio_short, - bio_full = excluded.bio_full, - bio_source = excluded.bio_source, - bio_language = excluded.bio_language, + bio_short = CASE WHEN ?16 THEN excluded.bio_short ELSE bio_short END, + bio_full = CASE WHEN ?16 THEN excluded.bio_full ELSE bio_full END, + bio_source = CASE WHEN ?16 THEN excluded.bio_source ELSE bio_source END, + bio_language = CASE WHEN ?16 THEN excluded.bio_language ELSE bio_language END, -- The three background columns are replaced only when -- TheAudioDB actually answered, which `background_fetched_at` -- records (it is NULL exactly when the API could not be @@ -825,46 +941,38 @@ async fn enrich_artist_named( THEN background_urls ELSE excluded.background_urls END, background_fetched_at = excluded.background_fetched_at, fetched_at = excluded.fetched_at, - expires_at = excluded.expires_at", + expires_at = excluded.expires_at + RETURNING bio_short, bio_full, bio_source, bio_language", ) - .bind(hit.id) - .bind(&hit.name) - .bind(picture_url.as_deref()) - .bind(picture_hash.as_deref()) - .bind(hit.nb_fan) - .bind(bio_short.as_deref()) - .bind(bio_full.as_deref()) - .bind(active_source.as_str()) - .bind(stored_lang) - .bind(background_url.as_deref()) - .bind(background_hash.as_deref()) - // NULL when TheAudioDB could not be reached, so the row still reads - // as "never looked" rather than as "this artist has no fanart". - .bind(background_urls_json.as_deref()) - // Stamped even when the lookup came back empty — that's the whole - // point of the column: "we asked, TheAudioDB has nothing". Left NULL - // when the API couldn't be reached at all, so that retries. - .bind(audiodb_reached.then_some(now)) - .bind(now) + .bind(row.deezer_id) + .bind(row.name) + .bind(row.picture_url) + .bind(row.picture_hash) + .bind(row.fans_count) + .bind(row.bio.short) + .bind(row.bio.full) + .bind(source) + .bind(language) + .bind(row.background_url) + .bind(row.background_hash) + .bind(row.background_urls_json) + .bind(row.background_fetched_at) + .bind(row.now) .bind(expires) - .execute(&pool) + .bind(row.bio.reached) + .fetch_one(pool) .await?; - - Ok(DeezerArtistEnrichment { - deezer_id: Some(hit.id), - picture_url, - picture_path, - picture_path_1x, - picture_path_2x, - fans_count: hit.nb_fan, - bio_short, - bio_full, - background_url, - background_path, - artwork_path: None, - artwork_path_1x: None, - artwork_path_2x: None, - }) + // A biography kept through a failure was fetched under whatever source + // and language were selected then. Shown now, it would answer a + // question the user no longer asks — the cache-hit check refuses it for + // the same reason — so it stays in the row and is not returned. + let asked_for = stored_source.as_deref() == Some(row.bio.source) + && stored_language.as_deref() == row.bio.language; + if row.bio.reached || asked_for { + Ok((short, full)) + } else { + Ok((None, None)) + } } // ── Cover management ──────────────────────────────────────────────── @@ -1742,4 +1850,163 @@ mod tests { assert!(is_public_http_url(url), "should be allowed: {url}"); } } + + mod artist_row { + use super::super::{upsert_artist_row, ArtistRow, BioAnswer, CACHE_TTL_MS}; + + /// A pool whose connections see the real `app` schema as `app`, + /// attached per connection the way the application attaches it + /// (see the harness in `lyrics.rs` for why each detail matters). + async fn pool_with_app_schema(dir: &std::path::Path) -> sqlx::SqlitePool { + use sqlx::sqlite::{SqliteConnectOptions, SqlitePoolOptions}; + use std::str::FromStr; + + let opts_for = |path: &std::path::Path| { + SqliteConnectOptions::from_str(&format!("sqlite://{}", path.display())) + .unwrap() + .create_if_missing(true) + }; + let app_path = dir.join("app.db"); + let app_pool = sqlx::SqlitePool::connect_with(opts_for(&app_path)) + .await + .unwrap(); + sqlx::migrate!("../../migrations/app") + .run(&app_pool) + .await + .unwrap(); + app_pool.close().await; + + let attach = format!( + "ATTACH DATABASE '{}' AS app", + app_path.display().to_string().replace('\'', "''") + ); + SqlitePoolOptions::new() + .after_connect(move |conn, _meta| { + let attach = attach.clone(); + Box::pin(async move { + sqlx::query(sqlx::AssertSqlSafe(attach)) + .execute(&mut *conn) + .await?; + Ok(()) + }) + }) + .connect_with(opts_for(&dir.join("profile.db"))) + .await + .unwrap() + } + + fn row<'a>(now: i64, reached: bool, full: Option<&'a str>) -> ArtistRow<'a> { + ArtistRow { + deezer_id: 464, + name: "Björk", + picture_url: None, + picture_hash: None, + fans_count: Some(1), + bio: BioAnswer { + reached, + short: full, + full, + source: "lastfm", + language: None, + }, + background_url: None, + background_hash: None, + background_urls_json: None, + background_fetched_at: None, + now, + } + } + + async fn stored(pool: &sqlx::SqlitePool) -> (Option, Option, i64) { + sqlx::query_as( + "SELECT bio_full, bio_source, expires_at FROM app.metadata_artist WHERE deezer_id = 464", + ) + .fetch_one(pool) + .await + .unwrap() + } + + /// A refused key or a network failure is not "this artist has no + /// biography": the one already cached stays, is what the caller + /// shows, and the row expires so the next visit asks again. + #[tokio::test] + async fn a_failure_to_ask_keeps_the_biography_and_expires_now() { + let dir = tempfile::tempdir().unwrap(); + let pool = pool_with_app_schema(dir.path()).await; + upsert_artist_row(&pool, &row(1_000, true, Some("Born in Reykjavík."))) + .await + .unwrap(); + + let (_, shown) = upsert_artist_row(&pool, &row(2_000, false, None)) + .await + .unwrap(); + + assert_eq!(shown.as_deref(), Some("Born in Reykjavík.")); + let (bio, source, expires_at) = stored(&pool).await; + assert_eq!(bio.as_deref(), Some("Born in Reykjavík.")); + assert_eq!(source.as_deref(), Some("lastfm")); + assert_eq!( + expires_at, 2_000, + "a failure must not be cached for the TTL" + ); + } + + /// The other half of the rule, so it cannot be met with a + /// COALESCE: an answer that carries nothing still replaces the + /// biography, and is cached for the whole TTL. + #[tokio::test] + async fn an_empty_answer_still_replaces_the_biography() { + let dir = tempfile::tempdir().unwrap(); + let pool = pool_with_app_schema(dir.path()).await; + upsert_artist_row(&pool, &row(1_000, true, Some("Born in Reykjavík."))) + .await + .unwrap(); + + let (_, shown) = upsert_artist_row(&pool, &row(2_000, true, None)) + .await + .unwrap(); + + assert_eq!(shown, None); + let (bio, _, expires_at) = stored(&pool).await; + assert_eq!(bio, None); + assert_eq!(expires_at, 2_000 + CACHE_TTL_MS); + } + + /// A first visit that failed to ask leaves a row no cache check + /// can read as a biography looked up. + #[tokio::test] + async fn a_first_failure_records_no_source() { + let dir = tempfile::tempdir().unwrap(); + let pool = pool_with_app_schema(dir.path()).await; + + upsert_artist_row(&pool, &row(1_000, false, None)) + .await + .unwrap(); + + let (bio, source, expires_at) = stored(&pool).await; + assert_eq!((bio, source, expires_at), (None, None, 1_000)); + } + + /// Kept through a failure is not the same as asked for: a biography + /// fetched under the source the user has since left stays in the + /// row, and is not shown as the answer to the new one. + #[tokio::test] + async fn a_kept_biography_from_another_source_is_not_shown() { + let dir = tempfile::tempdir().unwrap(); + let pool = pool_with_app_schema(dir.path()).await; + let mut before = row(1_000, true, Some("Née à Reykjavík.")); + before.bio.source = "theaudiodb"; + before.bio.language = Some("fr"); + upsert_artist_row(&pool, &before).await.unwrap(); + + let (_, shown) = upsert_artist_row(&pool, &row(2_000, false, None)) + .await + .unwrap(); + + assert_eq!(shown, None); + let (bio, source, _) = stored(&pool).await; + assert_eq!(bio.as_deref(), Some("Née à Reykjavík.")); + assert_eq!(source.as_deref(), Some("theaudiodb")); + } + } } From 64d620a902b33860321fcf936435fb33aa9eb3f6 Mon Sep 17 00:00:00 2001 From: InstaZDLL Date: Wed, 14 Oct 2026 22:36:28 +0200 Subject: [PATCH 6/8] fix(integration): placing the Last.fm key asks every artist again With Last.fm selected and no key, an artist's row is cached with an empty biography stamped lastfm, and the cache-hit check reuses it for its whole thirty days. Placing a key afterwards asked nothing: the biographies it would unlock stayed empty for a month. Clearing or replacing the key had the mirror problem, leaving biographies fetched under the old one. set_lastfm_api_key now expires every biography cached under Last.fm in the same transaction as the setting. Nothing is fetched up front; each artist is asked again the next time it is opened. Rows from another source keep their TTL. Run under WSL like the rest of the app crate's tests, with two inversions: a change that expires nothing, and one that expires every source. Signed-off-by: InstaZDLL --- .../crates/app/src/commands/integration.rs | 120 +++++++++++++++--- 1 file changed, 103 insertions(+), 17 deletions(-) diff --git a/src-tauri/crates/app/src/commands/integration.rs b/src-tauri/crates/app/src/commands/integration.rs index bd41931c0..f4b5f7af9 100644 --- a/src-tauri/crates/app/src/commands/integration.rs +++ b/src-tauri/crates/app/src/commands/integration.rs @@ -49,26 +49,52 @@ pub async fn set_lastfm_api_key( api_key: String, ) -> AppResult<()> { let trimmed = api_key.trim(); - if trimmed.is_empty() { - sqlx::query("DELETE FROM app_setting WHERE key = ?") + let key = (!trimmed.is_empty()).then_some(trimmed); + store_lastfm_api_key(&state.app_db, key, now_ms()).await?; + Ok(()) +} + +/// Write or clear the Last.fm key, and expire every biography cached under +/// Last.fm in the same transaction. +/// +/// A row looked up without a key holds an empty biography stamped +/// `lastfm`, and the cache-hit check in `enrich_artist_deezer` reuses it +/// for its whole TTL: a key placed afterwards would not be asked anything +/// for a month. The same holds the other way — a key removed or replaced +/// leaves biographies fetched under the old one. Expiring the rows costs +/// nothing up front; each artist is asked again the next time it is +/// opened, and only then. +async fn store_lastfm_api_key( + pool: &sqlx::SqlitePool, + key: Option<&str>, + now: i64, +) -> Result<(), sqlx::Error> { + let mut tx = pool.begin().await?; + match key { + None => { + sqlx::query("DELETE FROM app_setting WHERE key = ?") + .bind(LASTFM_KEY) + .execute(&mut *tx) + .await?; + } + Some(key) => { + sqlx::query( + "INSERT INTO app_setting (key, value, value_type, updated_at) + VALUES (?, ?, 'string', ?) + ON CONFLICT(key) DO UPDATE + SET value = excluded.value, updated_at = excluded.updated_at", + ) .bind(LASTFM_KEY) - .execute(&state.app_db) + .bind(key) + .bind(now) + .execute(&mut *tx) .await?; - return Ok(()); + } } - - sqlx::query( - "INSERT INTO app_setting (key, value, value_type, updated_at) - VALUES (?, ?, 'string', ?) - ON CONFLICT(key) DO UPDATE - SET value = excluded.value, updated_at = excluded.updated_at", - ) - .bind(LASTFM_KEY) - .bind(trimmed) - .bind(now_ms()) - .execute(&state.app_db) - .await?; - Ok(()) + sqlx::query("UPDATE metadata_artist SET expires_at = 0 WHERE bio_source = 'lastfm'") + .execute(&mut *tx) + .await?; + tx.commit().await } /// Internal helper used by `enrich_artist_deezer` to look up the key @@ -463,3 +489,63 @@ pub async fn read_lastfm_credentials( } Ok(Some((api_key, api_secret, session_key, username))) } + +#[cfg(test)] +mod tests { + use super::store_lastfm_api_key; + + async fn app_pool(dir: &std::path::Path) -> sqlx::SqlitePool { + use sqlx::sqlite::SqliteConnectOptions; + use std::str::FromStr; + + let options = + SqliteConnectOptions::from_str(&format!("sqlite://{}", dir.join("app.db").display())) + .unwrap() + .create_if_missing(true); + let pool = sqlx::SqlitePool::connect_with(options).await.unwrap(); + sqlx::migrate!("../../migrations/app") + .run(&pool) + .await + .unwrap(); + pool + } + + async fn seed(pool: &sqlx::SqlitePool, deezer_id: i64, source: &str) { + sqlx::query( + "INSERT INTO metadata_artist (deezer_id, name, bio_source, fetched_at, expires_at) + VALUES (?, 'artist', ?, 1, 9999999999999)", + ) + .bind(deezer_id) + .bind(source) + .execute(pool) + .await + .unwrap(); + } + + async fn expires_at(pool: &sqlx::SqlitePool, deezer_id: i64) -> i64 { + sqlx::query_scalar("SELECT expires_at FROM metadata_artist WHERE deezer_id = ?") + .bind(deezer_id) + .fetch_one(pool) + .await + .unwrap() + } + + /// A biography cached while no key was set is empty and stamped + /// `lastfm`; placing a key must make it ask again rather than serve + /// that emptiness for the rest of the month. Biographies from another + /// source owe the key nothing and keep their TTL. + #[tokio::test] + async fn placing_or_clearing_the_key_expires_only_lastfm_biographies() { + for key in [Some("a-key-from-the-settings"), None] { + let dir = tempfile::tempdir().unwrap(); + let pool = app_pool(dir.path()).await; + seed(&pool, 1, "lastfm").await; + seed(&pool, 2, "theaudiodb").await; + + store_lastfm_api_key(&pool, key, 42).await.unwrap(); + + assert_eq!(expires_at(&pool, 1).await, 0, "key {key:?}"); + assert_eq!(expires_at(&pool, 2).await, 9_999_999_999_999, "key {key:?}"); + } + } +} From 74a0095831852fca081f51a454caa092351a3c76 Mon Sep 17 00:00:00 2001 From: InstaZDLL Date: Thu, 15 Oct 2026 08:47:24 +0200 Subject: [PATCH 7/8] fix(integration): a key change also expires biographies with no source `BioSource::parse` reads a NULL `bio_source` as Last.fm, so a row written before the column existed is served as a Last.fm biography. Placing or clearing the key now expires it along with the rows stamped `lastfm`. Signed-off-by: InstaZDLL --- .../crates/app/src/commands/integration.rs | 23 +++++++++++++------ 1 file changed, 16 insertions(+), 7 deletions(-) diff --git a/src-tauri/crates/app/src/commands/integration.rs b/src-tauri/crates/app/src/commands/integration.rs index 54258d039..bd6d4aff4 100644 --- a/src-tauri/crates/app/src/commands/integration.rs +++ b/src-tauri/crates/app/src/commands/integration.rs @@ -92,9 +92,15 @@ async fn store_lastfm_api_key( .await?; } } - sqlx::query("UPDATE metadata_artist SET expires_at = 0 WHERE bio_source = 'lastfm'") - .execute(&mut *tx) - .await?; + // `BioSource::parse` reads a NULL source as Last.fm, so a row written + // before the column existed is served as a Last.fm biography and owes + // the key the same. + sqlx::query( + "UPDATE metadata_artist SET expires_at = 0 + WHERE bio_source = 'lastfm' OR bio_source IS NULL", + ) + .execute(&mut *tx) + .await?; tx.commit().await } @@ -661,7 +667,7 @@ mod tests { pool } - async fn seed(pool: &sqlx::SqlitePool, deezer_id: i64, source: &str) { + async fn seed(pool: &sqlx::SqlitePool, deezer_id: i64, source: Option<&str>) { sqlx::query( "INSERT INTO metadata_artist (deezer_id, name, bio_source, fetched_at, expires_at) VALUES (?, 'artist', ?, 1, 9999999999999)", @@ -684,19 +690,22 @@ mod tests { /// A biography cached while no key was set is empty and stamped /// `lastfm`; placing a key must make it ask again rather than serve /// that emptiness for the rest of the month. Biographies from another - /// source owe the key nothing and keep their TTL. + /// source owe the key nothing and keep their TTL. A row with no source + /// predates the column and is read as Last.fm, so it expires too. #[tokio::test] async fn placing_or_clearing_the_key_expires_only_lastfm_biographies() { for key in [Some("a-key-from-the-settings"), None] { let dir = tempfile::tempdir().unwrap(); let pool = app_pool(dir.path()).await; - seed(&pool, 1, "lastfm").await; - seed(&pool, 2, "theaudiodb").await; + seed(&pool, 1, Some("lastfm")).await; + seed(&pool, 2, Some("theaudiodb")).await; + seed(&pool, 3, None).await; store_lastfm_api_key(&pool, key, 42).await.unwrap(); assert_eq!(expires_at(&pool, 1).await, 0, "key {key:?}"); assert_eq!(expires_at(&pool, 2).await, 9_999_999_999_999, "key {key:?}"); + assert_eq!(expires_at(&pool, 3).await, 0, "key {key:?}"); } } } From a4456bb4e1f8f393c540fed8529a7c290c2e4006 Mon Sep 17 00:00:00 2001 From: InstaZDLL Date: Thu, 15 Oct 2026 10:06:35 +0200 Subject: [PATCH 8/8] fix(artist): retry a failed biography after an hour, and ignore an unchanged key A failure to ask for a biography expired the row at once, so a refused key or a spent quota re-ran the whole enrichment on every visit and every batch pass. It now stands for `RETRY_AFTER_MS`, an hour. `store_lastfm_api_key` expired every Last.fm biography on each save, and the Settings card sends the key on every save, changed or not. It now compares with the stored value first and does nothing when they match. Signed-off-by: InstaZDLL --- src-tauri/crates/app/src/commands/deezer.rs | 45 ++++++++------ .../crates/app/src/commands/integration.rs | 61 +++++++++++++++---- 2 files changed, 78 insertions(+), 28 deletions(-) diff --git a/src-tauri/crates/app/src/commands/deezer.rs b/src-tauri/crates/app/src/commands/deezer.rs index 78d8b96d3..2634fd364 100644 --- a/src-tauri/crates/app/src/commands/deezer.rs +++ b/src-tauri/crates/app/src/commands/deezer.rs @@ -42,6 +42,11 @@ use crate::{ /// TTL for cached Deezer entries: 30 days in milliseconds. const CACHE_TTL_MS: i64 = 30 * 24 * 60 * 60 * 1000; +/// How long a row stands when the biography could not be asked for: an +/// hour, so a refused key or a spent quota is retried soon without +/// re-running the whole enrichment on every visit and every batch pass. +const RETRY_AFTER_MS: i64 = 60 * 60 * 1000; + fn now_ms() -> i64 { Utc::now().timestamp_millis() } @@ -721,10 +726,10 @@ async fn enrich_artist_named( // 5. Fetch the bio from the selected source (issue #295). A missing // match is an answer and is cached like one. A failure to ask — // the network, a refused key, a spent quota — is not: the row - // keeps the biography it had and expires now, so the next visit - // asks again (see `upsert_artist_row`). The source/language we used - // is stored alongside so a later switch re-fetches instead of - // serving the wrong bio. + // keeps the biography it had and expires within the hour, so a + // later visit asks again (see `upsert_artist_row`). The + // source/language we used is stored alongside so a later switch + // re-fetches instead of serving the wrong bio. let (bio_short, bio_full, bio_reached) = match active_source { BioSource::Lastfm => match read_lastfm_api_key(&state).await? { Some(api_key) => { @@ -883,10 +888,11 @@ struct ArtistRow<'a> { /// or not — an artist whose biography was removed upstream must lose it /// here too, so this tests whether the provider was reached rather than /// whether the new value is empty. When it was not reached, the row keeps -/// the biography and the source it had, and **expires now**: the Deezer -/// half was fetched and is stored, but the visit that failed to ask for -/// a biography must not stand for thirty days. A fresh row in that case -/// records no source, so nothing reads it as a biography looked up. +/// the biography and the source it had, and **expires after +/// `RETRY_AFTER_MS`**: the Deezer half was fetched and is stored, but the +/// visit that failed to ask for a biography must not stand for thirty +/// days — nor be repeated on every open while the key stays refused. A +/// fresh row in that case records no source. async fn upsert_artist_row( pool: &sqlx::SqlitePool, row: &ArtistRow<'_>, @@ -894,7 +900,7 @@ async fn upsert_artist_row( let expires = if row.bio.reached { row.now + CACHE_TTL_MS } else { - row.now + row.now + RETRY_AFTER_MS }; let (source, language) = if row.bio.reached { (Some(row.bio.source), row.bio.language) @@ -1852,7 +1858,7 @@ mod tests { } mod artist_row { - use super::super::{upsert_artist_row, ArtistRow, BioAnswer, CACHE_TTL_MS}; + use super::super::{upsert_artist_row, ArtistRow, BioAnswer, CACHE_TTL_MS, RETRY_AFTER_MS}; /// A pool whose connections see the real `app` schema as `app`, /// attached per connection the way the application attaches it @@ -1928,9 +1934,10 @@ mod tests { /// A refused key or a network failure is not "this artist has no /// biography": the one already cached stays, is what the caller - /// shows, and the row expires so the next visit asks again. + /// shows, and the row expires within the hour so a later visit + /// asks again — not for the TTL, and not on the very next open. #[tokio::test] - async fn a_failure_to_ask_keeps_the_biography_and_expires_now() { + async fn a_failure_to_ask_keeps_the_biography_and_is_retried_soon() { let dir = tempfile::tempdir().unwrap(); let pool = pool_with_app_schema(dir.path()).await; upsert_artist_row(&pool, &row(1_000, true, Some("Born in Reykjavík."))) @@ -1946,8 +1953,9 @@ mod tests { assert_eq!(bio.as_deref(), Some("Born in Reykjavík.")); assert_eq!(source.as_deref(), Some("lastfm")); assert_eq!( - expires_at, 2_000, - "a failure must not be cached for the TTL" + expires_at, + 2_000 + RETRY_AFTER_MS, + "a failure is retried after the window, not the TTL" ); } @@ -1972,8 +1980,8 @@ mod tests { assert_eq!(expires_at, 2_000 + CACHE_TTL_MS); } - /// A first visit that failed to ask leaves a row no cache check - /// can read as a biography looked up. + /// A first visit that failed to ask records no source, and is + /// retried after the same window. #[tokio::test] async fn a_first_failure_records_no_source() { let dir = tempfile::tempdir().unwrap(); @@ -1984,7 +1992,10 @@ mod tests { .unwrap(); let (bio, source, expires_at) = stored(&pool).await; - assert_eq!((bio, source, expires_at), (None, None, 1_000)); + assert_eq!( + (bio, source, expires_at), + (None, None, 1_000 + RETRY_AFTER_MS) + ); } /// Kept through a failure is not the same as asked for: a biography diff --git a/src-tauri/crates/app/src/commands/integration.rs b/src-tauri/crates/app/src/commands/integration.rs index bd6d4aff4..007653a5e 100644 --- a/src-tauri/crates/app/src/commands/integration.rs +++ b/src-tauri/crates/app/src/commands/integration.rs @@ -65,12 +65,23 @@ pub async fn set_lastfm_api_key( /// leaves biographies fetched under the old one. Expiring the rows costs /// nothing up front; each artist is asked again the next time it is /// opened, and only then. +/// +/// Saving the value already stored is a no-op: the Settings card and +/// onboarding send the key on every save, changed or not, and each of +/// those must not cost every Last.fm biography its cache. async fn store_lastfm_api_key( pool: &sqlx::SqlitePool, key: Option<&str>, now: i64, ) -> Result<(), sqlx::Error> { let mut tx = pool.begin().await?; + let stored: Option = sqlx::query_scalar("SELECT value FROM app_setting WHERE key = ?") + .bind(LASTFM_KEY) + .fetch_optional(&mut *tx) + .await?; + if stored.as_deref() == key { + return Ok(()); + } match key { None => { sqlx::query("DELETE FROM app_setting WHERE key = ?") @@ -694,18 +705,46 @@ mod tests { /// predates the column and is read as Last.fm, so it expires too. #[tokio::test] async fn placing_or_clearing_the_key_expires_only_lastfm_biographies() { - for key in [Some("a-key-from-the-settings"), None] { - let dir = tempfile::tempdir().unwrap(); - let pool = app_pool(dir.path()).await; - seed(&pool, 1, Some("lastfm")).await; - seed(&pool, 2, Some("theaudiodb")).await; - seed(&pool, 3, None).await; - - store_lastfm_api_key(&pool, key, 42).await.unwrap(); + for (before, after) in [ + (None, Some("a-key-from-the-settings")), + (Some("a-key-from-the-settings"), Some("another-key")), + (Some("a-key-from-the-settings"), None), + ] { + assert_eq!( + save_over(before, after).await, + [0, KEPT, 0], + "{before:?} -> {after:?}" + ); + } + } - assert_eq!(expires_at(&pool, 1).await, 0, "key {key:?}"); - assert_eq!(expires_at(&pool, 2).await, 9_999_999_999_999, "key {key:?}"); - assert_eq!(expires_at(&pool, 3).await, 0, "key {key:?}"); + /// The Settings card sends the key on every save, changed or not; a + /// save that changes nothing must not cost the biographies their cache. + #[tokio::test] + async fn saving_the_stored_key_again_expires_nothing() { + for key in [Some("a-key-from-the-settings"), None] { + assert_eq!(save_over(key, key).await, [KEPT; 3], "{key:?}"); } } + + const KEPT: i64 = 9_999_999_999_999; + + /// Store `before`, seed a `lastfm`, a `theaudiodb` and a source-less + /// row, save `after`, and read the three expiries back. + async fn save_over(before: Option<&str>, after: Option<&str>) -> [i64; 3] { + let dir = tempfile::tempdir().unwrap(); + let pool = app_pool(dir.path()).await; + store_lastfm_api_key(&pool, before, 1).await.unwrap(); + seed(&pool, 1, Some("lastfm")).await; + seed(&pool, 2, Some("theaudiodb")).await; + seed(&pool, 3, None).await; + + store_lastfm_api_key(&pool, after, 42).await.unwrap(); + + [ + expires_at(&pool, 1).await, + expires_at(&pool, 2).await, + expires_at(&pool, 3).await, + ] + } }