From 0f609aa4ccf4959d575f8f6679250bcbc2003a35 Mon Sep 17 00:00:00 2001 From: Breadway Date: Mon, 3 Aug 2026 08:43:29 +0800 Subject: [PATCH] Fix bugs found by an Opus 5 audit: races, parsing, and CDATA gaps - Atomic claim on review-queue approval, closing a double-approve race that could grab the same release twice (scheduler.rs) - Constant-time comparison for the daemon API token, closing a timing side channel - RSS items wrapped in CDATA (common for titles with '&') were silently dropped - only Event::Text was ever handled - Reject malformed apibay info_hash values before building a magnet link that extract_btih can't parse back out - Parse sizes with no space before the unit ("38.1GiB") - Fix "Season N - NN" episode parsing and stop misreading a YYYY-MM-DD date as a bare episode range - Query embeddings are no longer cached, fixing unbounded cache growth over the daemon's lifetime (only library-side candidates need caching) --- breadarrd/src/api/mod.rs | 41 ++++++++++- breadarrd/src/api/routes/review.rs | 19 ++++- breadarrd/src/matcher/mod.rs | 21 +++++- breadarrd/src/parser/mod.rs | 44 +++++++++++ breadarrd/src/parser/tokens.rs | 68 +++++++++++++++-- breadarrd/src/scheduler.rs | 100 +++++++++++++++++++++++-- breadarrd/src/sources/mod.rs | 22 +++++- breadarrd/src/sources/rss.rs | 113 ++++++++++++++++++++++------- breadarrd/src/sources/tpb.rs | 47 +++++++++++- 9 files changed, 427 insertions(+), 48 deletions(-) diff --git a/breadarrd/src/api/mod.rs b/breadarrd/src/api/mod.rs index 6fa0ebe..92f82f0 100644 --- a/breadarrd/src/api/mod.rs +++ b/breadarrd/src/api/mod.rs @@ -109,13 +109,27 @@ async fn require_api_token(State(state): State, req: Request, next: Ne .get(axum::http::header::AUTHORIZATION) .and_then(|v| v.to_str().ok()) .and_then(|v| v.strip_prefix("Bearer ")) - .is_some_and(|token| token == state.config.daemon.api_token); + .is_some_and(|token| constant_time_eq(token, &state.config.daemon.api_token)); if !authorized { return (StatusCode::UNAUTHORIZED, "missing or invalid API token").into_response(); } next.run(req).await } +/// Byte-wise `==` short-circuits on the first mismatching byte, making +/// comparison time a (weak, but real) signal of how many leading bytes of a +/// guessed token were correct — a classic timing oracle. This always +/// touches every byte of the shorter input regardless of where they first +/// differ. Real-world exposure here is low (loopback-bound by default, a +/// personal single-user daemon), but it costs nothing to close. +fn constant_time_eq(a: &str, b: &str) -> bool { + let (a, b) = (a.as_bytes(), b.as_bytes()); + if a.len() != b.len() { + return false; + } + a.iter().zip(b.iter()).fold(0u8, |acc, (x, y)| acc | (x ^ y)) == 0 +} + pub fn router(state: AppState) -> Router { Router::new() .route("/health", get(routes::health::health)) @@ -179,3 +193,28 @@ pub fn router(state: AppState) -> Router { )) .with_state(state) } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn constant_time_eq_matches_identical_strings() { + assert!(constant_time_eq("secret-token", "secret-token")); + } + + #[test] + fn constant_time_eq_rejects_different_strings_of_the_same_length() { + assert!(!constant_time_eq("secret-token", "secret-toke1")); + } + + #[test] + fn constant_time_eq_rejects_different_lengths() { + assert!(!constant_time_eq("short", "a-much-longer-token")); + } + + #[test] + fn constant_time_eq_treats_empty_strings_as_equal() { + assert!(constant_time_eq("", "")); + } +} diff --git a/breadarrd/src/api/routes/review.rs b/breadarrd/src/api/routes/review.rs index f16fbeb..d00f480 100644 --- a/breadarrd/src/api/routes/review.rs +++ b/breadarrd/src/api/routes/review.rs @@ -75,15 +75,26 @@ pub async fn approve( } }; - let torrent_hash = scheduler::grab_prepared_approval(qbit, &state.qbit_category, &prepared) - .await - .map_err(internal)?; + let torrent_hash = match scheduler::grab_prepared_approval(qbit, &state.qbit_category, &prepared).await { + Ok(hash) => hash, + Err(e) => { + // The grab errored outright (not just "added but no hash + // captured" — `finalize_review_approval` below handles that + // case and still runs to completion). `prepare_review_approval` + // already claimed this row into `approved` before we got here; + // without releasing it back to `pending`, a transient + // qBittorrent error would strand the review permanently + // unapprovable with nothing ever recorded for it. + let conn = state.conn.lock().await; + let _ = scheduler::release_review_claim(&conn, id); + return Err(internal(e)); + } + }; { let conn = state.conn.lock().await; scheduler::finalize_review_approval( &conn, - id, &prepared, &state.qbit_category, torrent_hash.as_deref(), diff --git a/breadarrd/src/matcher/mod.rs b/breadarrd/src/matcher/mod.rs index fc82c08..88a8b15 100644 --- a/breadarrd/src/matcher/mod.rs +++ b/breadarrd/src/matcher/mod.rs @@ -96,6 +96,17 @@ impl TitleMatcher { }) } + /// Caches by text — appropriate for candidate-side text only (library + /// `media_item` titles/aliases, or a metadata provider's small, + /// bounded result list), which the same daemon process legitimately + /// re-embeds across many calls. Deliberately never used for the query + /// side (see `embed_query`): `TitleMatcher` lives for the whole life of + /// `background_loop`, which never returns, and the query is a + /// freshly-parsed release title from every RSS item and search result + /// the daemon ever sees — almost never repeated verbatim. Caching those + /// too grew this `HashMap` without bound for the process's entire + /// (months-long) lifetime, a slow but real leak on a box also running + /// several GB of concurrent GPU/CPU transcode work. fn embed_cached(&mut self, text: &str) -> Result> { if let Some(v) = self.cache.get(text) { return Ok(v.clone()); @@ -105,6 +116,12 @@ impl TitleMatcher { Ok(v) } + /// The query-side counterpart to `embed_cached` — same embedding, never + /// stored in `self.cache`. See `embed_cached`'s doc comment for why. + fn embed_query(&mut self, text: &str) -> Result> { + self.embedder.embed(text) + } + /// Given a flat list of candidate texts (e.g. every search result's /// name plus its aliases, flattened with an index back to which result /// each one belongs to), returns the index of whichever candidate text @@ -121,7 +138,7 @@ impl TitleMatcher { query: &str, candidates: &[(usize, String)], ) -> Result> { - let query_emb = self.embed_cached(query)?; + let query_emb = self.embed_query(query)?; let mut best: Option<(usize, f32)> = None; for (owner_index, text) in candidates { let emb = self.embed_cached(text)?; @@ -137,7 +154,7 @@ impl TitleMatcher { /// aliases, returning the single best match and whether it clears the /// auto-match bar or needs a human to confirm it in the review queue. pub fn match_title(&mut self, conn: &Connection, query: &str) -> Result { - let query_emb = self.embed_cached(query)?; + let query_emb = self.embed_query(query)?; let mut stmt = conn.prepare( "SELECT id, title FROM media_item WHERE monitored = 1 diff --git a/breadarrd/src/parser/mod.rs b/breadarrd/src/parser/mod.rs index 97780e1..f24fcaa 100644 --- a/breadarrd/src/parser/mod.rs +++ b/breadarrd/src/parser/mod.rs @@ -209,6 +209,50 @@ mod tests { ); } + // Regression test for a real gap found in review: "Season 2 - 25" was + // first swallowed whole by `looks_like_episode_range` (its own + // `BARE_EPISODE_RANGE_RE` skips over the un-matchable "Season" word and + // finds its first real match at "2 - 25", mistaking the season marker's + // own number for a range start), and even with that fixed, + // `extract_episode_info`'s `SEASON_PACK_RE` branch used to return + // season-only and never look for a trailing episode number at all. + // Either bug alone drops episode 25 silently. + #[test] + fn parses_a_season_marker_followed_by_a_dash_episode() { + let p = parse("[Erai-raws] Some Show Season 2 - 25 [1080p]"); + assert_eq!(p.season, Some(2)); + assert_eq!(p.episode, Some(25)); + } + + // Companion case: a genuine season-only pack (no trailing dash-episode + // anywhere) must still resolve to season-only, not spuriously pick up + // an unrelated number as an episode. + #[test] + fn a_genuine_season_only_pack_with_no_dash_episode_still_has_no_episode() { + let p = parse("Some Show Season 2 Complete [1080p]"); + assert_eq!(p.season, Some(2)); + assert_eq!(p.episode, None); + } + + // Regression test for a real gap found in review: a date-named release + // ("2024-01-15") got misread by `BARE_EPISODE_RANGE_RE` as an episode + // range — the 4-digit year is too many digits for `\d{1,3}` to match + // whole, so its first real match starts at the month/day pair + // ("01-15") instead, and `looks_like_episode_range` treats that as a + // real range. Asserted directly against the tokenizer rather than + // `parse()`, since a false-positive range and a genuine "no episode + // marker at all" both surface identically as `None`/`None` on + // `ParsedRelease` — `looks_like_episode_range` returning `false` is the + // actual fix being tested here. + #[test] + fn does_not_mistake_a_yyyy_mm_dd_date_for_an_episode_range() { + assert!(!tokens::looks_like_episode_range("Some Daily Show 2024-01-15 1080p WEB-DL")); + + let p = parse("Some Daily Show 2024-01-15 1080p WEB-DL"); + assert_eq!(p.season, None); + assert_eq!(p.episode, None); + } + #[test] fn parses_yameii_dash_sxxexx_with_english_dub_tag() { let p = parse("[Yameii] Ascendance of a Bookworm - S04E11 [English Dub] [CR WEB-DL 1080p H264 AAC] [8ACE7B72] (Honzuki no Gekokujou)"); diff --git a/breadarrd/src/parser/tokens.rs b/breadarrd/src/parser/tokens.rs index 1215930..c48f6a8 100644 --- a/breadarrd/src/parser/tokens.rs +++ b/breadarrd/src/parser/tokens.rs @@ -90,15 +90,46 @@ static SXX_EPISODE_RANGE_RE: LazyLock = // elsewhere in the title with a smaller trailing number. static BARE_EPISODE_RANGE_RE: LazyLock = LazyLock::new(|| Regex::new(r"\b(\d{1,3})\s*[-~]\s*(\d{1,3})\b").unwrap()); +// A `YYYY-MM-DD` date ("2024-01-15"): the year's 4 digits are too many for +// `BARE_EPISODE_RANGE_RE`'s/`DASH_EPISODE_RE`'s `\d{1,3}` to match as a +// whole, so those regexes' *first real match* on a date-named release ends +// up starting at the month ("01-15", or "01" alone) instead — a bare +// month/day pair, not a real episode range or episode number. Matched as a +// whole date span (not just a "year-" prefix check) so both the month *and* +// the day segment are covered — checking only the text immediately before a +// candidate match would still let "15" in "2024-01-15" slip through as a +// false "episode 15" once "01" alone was correctly rejected. +static DATE_RE: LazyLock = + LazyLock::new(|| Regex::new(r"\b(?:19|20)\d{2}-\d{1,2}-\d{1,2}\b").unwrap()); + +fn overlaps_a_date(s: &str, start: usize, end: usize) -> bool { + DATE_RE.find_iter(s).any(|d| d.start() <= start && end <= d.end()) +} pub(super) fn looks_like_episode_range(s: &str) -> bool { if BATCH_WORD_RE.is_match(s) || SXX_EPISODE_RANGE_RE.is_match(s) { return true; } - BARE_EPISODE_RANGE_RE.captures(s).is_some_and(|c| { - let a: u32 = c[1].parse().unwrap_or(0); - let b: u32 = c[2].parse().unwrap_or(0); - b > a + BARE_EPISODE_RANGE_RE.captures_iter(s).any(|c| { + let first = c.get(1).unwrap(); + let second = c.get(2).unwrap(); + let a: u32 = first.as_str().parse().unwrap_or(0); + let b: u32 = second.as_str().parse().unwrap_or(0); + if b <= a { + return false; + } + if overlaps_a_date(s, first.start(), second.end()) { + return false; + } + // "Season 2 - 25": the range's first number is really a season + // marker's own number (checked by comparing spans, not just text, + // so this only fires when the two genuinely overlap), not a range + // start — "2 - 25" isn't a real episode range, it's "season 2, + // episode 25", resolved separately in `extract_episode_info`. + let is_season_marker_number = SEASON_PACK_RE.captures(s).is_some_and(|sc| { + sc.get(1).unwrap().range() == first.range() + }); + !is_season_marker_number }) } @@ -165,6 +196,23 @@ pub(super) fn extract_year(s: &str) -> Option { s[m.start()..m.end()].parse().ok() } +/// `DASH_EPISODE_RE`'s first match that isn't actually the month of a +/// `YYYY-MM-DD` date. A bare `-\s*\d{1,3}\b` alone can't tell "Show - 25 +/// [1080p]" (a real episode number) apart from "...2024-01-15..." (the "01" +/// is just a month, matched for the same reason `BARE_EPISODE_RANGE_RE` +/// does in `looks_like_episode_range` — the 4-digit year is too many digits +/// to match as a whole, so the regex's first real match starts one segment +/// later). Reused by every `extract_episode_info` branch that falls back to +/// `DASH_EPISODE_RE`, not just the range-detection path, since the date +/// misread happens independently of whether `looks_like_episode_range` +/// fires. +fn find_real_dash_episode(s: &str) -> Option> { + DASH_EPISODE_RE.captures_iter(s).find(|c| { + let m = c.get(0).unwrap(); + !overlaps_a_date(s, m.start(), m.end()) + }) +} + /// Returns (season, episode, absolute_episode, title_span_end) — the last /// element is the byte offset in `s` where the episode/season token (or, /// failing that, the first quality marker) begins, used to slice out the @@ -194,9 +242,19 @@ pub(super) fn extract_episode_info(s: &str) -> (Option, Option, Option } if let Some(c) = SEASON_PACK_RE.captures(s) { let season = c[1].parse().ok(); + // A season marker immediately followed elsewhere by a dash-number + // ("Season 2 - 25") names one episode within that season, not a + // season-only pack — checked here rather than reordering the checks + // above `SXX_DASH_EP_RE`/`SXXEXX_RE` still get first crack at more + // specific shapes, and a genuine season-only pack (no trailing + // dash-number anywhere) is unaffected. + if let Some(ep) = find_real_dash_episode(s) { + let episode: Option = ep[1].parse().ok(); + return (season, episode, None, c.get(0).unwrap().start()); + } return (season, None, None, c.get(0).unwrap().start()); } - if let Some(c) = DASH_EPISODE_RE.captures(s) { + if let Some(c) = find_real_dash_episode(s) { let episode: Option = c[1].parse().ok(); return (None, episode, episode, c.get(0).unwrap().start()); } diff --git a/breadarrd/src/scheduler.rs b/breadarrd/src/scheduler.rs index b6bba16..37deb68 100644 --- a/breadarrd/src/scheduler.rs +++ b/breadarrd/src/scheduler.rs @@ -2112,6 +2112,30 @@ pub fn prepare_review_approval(conn: &Connection, review_id: i64) -> Result Result Result<()> { + conn.execute( + "UPDATE review_queue SET status = 'pending' WHERE id = ?1 AND status = 'approved'", + params![review_id], + )?; + Ok(()) +} + /// The actual grab — network/qBittorrent I/O only, no `Connection` involved, /// safe to `.await` from anywhere. pub async fn grab_prepared_approval( @@ -2133,11 +2181,12 @@ pub async fn grab_prepared_approval( grab_and_capture_hash(qbit, &prepared.link, qbit_category).await } -/// Sync-only: records the grab and marks the review approved. Call after -/// [`grab_prepared_approval`] completes. +/// Sync-only: records the grab. Call after [`grab_prepared_approval`] +/// completes. Doesn't touch `review_queue.status` — `prepare_review_approval` +/// already claimed it into `approved` before the grab ran (see its doc +/// comment). pub fn finalize_review_approval( conn: &Connection, - review_id: i64, prepared: &PreparedApproval, qbit_category: &str, torrent_hash: Option<&str>, @@ -2168,10 +2217,6 @@ pub fn finalize_review_approval( torrent_hash, status, )?; - conn.execute( - "UPDATE review_queue SET status = 'approved' WHERE id = ?1", - params![review_id], - )?; Ok(()) } @@ -2545,6 +2590,47 @@ mod tests { } } + // Regression test for a real gap found in review: `prepare_review_approval` + // used to only *read* `status == "pending"`, never claim it — two + // concurrent `approve()` calls (a double-click, or an HTTP client retry) + // could both pass that check, both grab the same release, and both + // write their own `release`/`torrent_fetch` rows. A second call for the + // same review must now see it's already claimed. + #[test] + fn a_second_prepare_call_on_an_already_claimed_review_sees_not_pending() { + let conn = seeded_movie_conn(); + let review_id = insert_pending_review(&conn, "Some Movie 2016 1080p BluRay x264"); + + assert!(matches!( + prepare_review_approval(&conn, review_id).unwrap(), + ApprovalPrep::Ready(_) + )); + // Same review_id, called again before any grab or finalize ran — + // simulates the double-click/retry race. + assert!(matches!( + prepare_review_approval(&conn, review_id).unwrap(), + ApprovalPrep::NotPending + )); + } + + #[test] + fn release_review_claim_reverts_a_claimed_row_back_to_pending() { + let conn = seeded_movie_conn(); + let review_id = insert_pending_review(&conn, "Some Movie 2016 1080p BluRay x264"); + + assert!(matches!( + prepare_review_approval(&conn, review_id).unwrap(), + ApprovalPrep::Ready(_) + )); + release_review_claim(&conn, review_id).unwrap(); + // The claim was released (e.g. because the grab itself then errored + // outright) — a fresh approval attempt must be possible again. + assert!(matches!( + prepare_review_approval(&conn, review_id).unwrap(), + ApprovalPrep::Ready(_) + )); + } + #[test] fn rejects_a_movie_review_whose_title_looks_like_an_episode() { let conn = seeded_movie_conn(); diff --git a/breadarrd/src/sources/mod.rs b/breadarrd/src/sources/mod.rs index f6676ca..646d82f 100644 --- a/breadarrd/src/sources/mod.rs +++ b/breadarrd/src/sources/mod.rs @@ -45,7 +45,17 @@ pub(crate) fn urlencode(s: &str) -> String { pub(crate) fn parse_human_size(s: &str) -> Option { let s = s.trim(); - let (num_part, unit) = s.split_once(' ')?; + // Split on the first character that isn't part of the number, rather + // than requiring a literal space — some sources render this without one + // ("38.1GiB"). Requiring a space made `split_once(' ')` return `None` + // for those, silently leaving `size_bytes` unset rather than failing + // outright: the gate's size sanity check (`gate.rs`) treats a missing + // size as "nothing to check" and skips it entirely instead of rejecting + // the release, so a value this parser simply couldn't read bypassed + // size validation altogether rather than being caught by it. + let split_at = s.find(|c: char| !(c.is_ascii_digit() || c == '.'))?; + let (num_part, unit) = s.split_at(split_at); + let unit = unit.trim(); let num: f64 = num_part.parse().ok()?; let mult = match unit { "B" => 1.0, @@ -80,4 +90,14 @@ mod tests { fn rejects_unknown_unit() { assert_eq!(parse_human_size("5 XiB"), None); } + + // Regression test for a real gap found in review: some sources render + // this with no space between the number and the unit — the old + // `split_once(' ')` returned `None` for those, silently leaving + // `size_bytes` unset (which skips the gate's size sanity check entirely) + // rather than rejecting a value this parser genuinely couldn't read. + #[test] + fn parses_a_size_with_no_space_before_the_unit() { + assert_eq!(parse_human_size("38.1GiB"), Some(40909563494)); + } } diff --git a/breadarrd/src/sources/rss.rs b/breadarrd/src/sources/rss.rs index 34adf96..5425646 100644 --- a/breadarrd/src/sources/rss.rs +++ b/breadarrd/src/sources/rss.rs @@ -52,6 +52,39 @@ fn build_search_url(feed_url: &str, query: Option<&str>) -> String { } } +/// Every field accumulated across one ``'s `Text`/`CData` events, +/// bundled into one struct (rather than six separate `&mut Option<_>` +/// parameters) purely to keep `accumulate_field` under clippy's +/// too-many-arguments threshold. +#[derive(Default)] +struct ItemFields { + title: Option, + link: Option, + guid: Option, + seeders: Option, + leechers: Option, + size_bytes: Option, +} + +/// Appends rather than overwrites title/link/guid: a real feed can split a +/// single logical value across more than one `Text`/`CData` event for the +/// same tag (mixed content, or just a parser buffer boundary) — a plain +/// assignment would silently keep only the *last* fragment, truncating the +/// value. `nyaa:seeders`/`nyaa:leechers`/`nyaa:size` stay parse-and-overwrite +/// since they're short numeric/size tokens, not free text expected to span +/// multiple events. +fn accumulate_field(tag: &str, text: &str, fields: &mut ItemFields) { + match tag { + "title" => fields.title.get_or_insert_with(String::new).push_str(text), + "link" => fields.link.get_or_insert_with(String::new).push_str(text), + "guid" => fields.guid.get_or_insert_with(String::new).push_str(text), + "nyaa:seeders" => fields.seeders = text.parse().ok(), + "nyaa:leechers" => fields.leechers = text.parse().ok(), + "nyaa:size" => fields.size_bytes = parse_human_size(text), + _ => {} + } +} + fn parse_nyaa_rss(bytes: &[u8]) -> Result> { let mut reader = Reader::from_reader(bytes); reader.config_mut().trim_text(true); @@ -61,12 +94,7 @@ fn parse_nyaa_rss(bytes: &[u8]) -> Result> { let mut in_item = false; let mut cur_tag = String::new(); - let mut title = None; - let mut link = None; - let mut guid = None; - let mut seeders = None; - let mut leechers = None; - let mut size_bytes = None; + let mut fields = ItemFields::default(); loop { match reader.read_event_into(&mut buf)? { @@ -75,41 +103,42 @@ fn parse_nyaa_rss(bytes: &[u8]) -> Result> { let name = String::from_utf8_lossy(e.name().as_ref()).into_owned(); if name == "item" { in_item = true; - title = None; - link = None; - guid = None; - seeders = None; - leechers = None; - size_bytes = None; + fields = ItemFields::default(); } cur_tag = name; } Event::Text(t) if in_item => { let raw = t.decode()?; let text = unescape(&raw)?.into_owned(); - match cur_tag.as_str() { - "title" => title = Some(text), - "link" => link = Some(text), - "guid" => guid = Some(text), - "nyaa:seeders" => seeders = text.parse().ok(), - "nyaa:leechers" => leechers = text.parse().ok(), - "nyaa:size" => size_bytes = parse_human_size(&text), - _ => {} - } + accumulate_field(&cur_tag, &text, &mut fields); + } + // CDATA content is raw text by definition — XML entity escaping + // doesn't apply inside a CDATA section (running `unescape()` on + // it would misinterpret a literal "&" as an escaped + // ampersand), so this decodes without it. Many real-world feeds + // wrap ``/`<link>` in CDATA; previously only + // `Event::Text` was handled at all, so those items silently + // came back with `title = None` and were dropped at the + // `item`-close check below with zero error — pointing + // `nyaa_rss_url` at a CDATA-heavy feed yielded zero items, not + // a visible failure. + Event::CData(t) if in_item => { + let text = t.decode()?.into_owned(); + accumulate_field(&cur_tag, &text, &mut fields); } Event::End(e) => { let name = String::from_utf8_lossy(e.name().as_ref()).into_owned(); if name == "item" { if let (Some(title), Some(link), Some(guid)) = - (title.take(), link.take(), guid.take()) + (fields.title.take(), fields.link.take(), fields.guid.take()) { items.push(RawReleaseItem { title, link, guid, - size_bytes, - seeders, - leechers, + size_bytes: fields.size_bytes, + seeders: fields.seeders, + leechers: fields.leechers, }); } in_item = false; @@ -156,6 +185,40 @@ mod tests { assert_eq!(item.size_bytes, Some(373817344)); } + // Regression test for a real gap found in review: many real-world feeds + // wrap `<title>`/`<link>`/`<guid>` in CDATA rather than plain text + // content (often to avoid having to XML-escape ampersands/brackets + // common in release titles). Previously only `Event::Text` was + // handled — `Event::CData` was silently ignored — so every field + // wrapped this way came back `None` and the whole item was dropped at + // the `item`-close check with no error surfaced at all. + #[test] + fn parses_cdata_wrapped_fields() { + const CDATA_SAMPLE: &str = r#"<?xml version="1.0" encoding="UTF-8"?> +<rss xmlns:nyaa="https://nyaa.si/xmlns/nyaa" version="2.0"> +<channel> +<item> + <title><![CDATA[[Group] Some Show & Friends - 05 [1080p]]]> + + + 12 + 3 + 356.5 MiB + + +"#; + + let items = parse_nyaa_rss(CDATA_SAMPLE.as_bytes()).unwrap(); + assert_eq!(items.len(), 1, "a CDATA-wrapped item must not be silently dropped"); + let item = &items[0]; + // A literal "&" survives verbatim — CDATA content isn't + // XML-entity-escaped, so this must NOT come back as "&". + assert_eq!(item.title, "[Group] Some Show & Friends - 05 [1080p]"); + assert_eq!(item.link, "https://nyaa.si/download/2130903.torrent"); + assert_eq!(item.guid, "https://nyaa.si/view/2130903"); + assert_eq!(item.seeders, Some(12)); + } + #[test] fn build_search_url_appends_query_param() { assert_eq!( diff --git a/breadarrd/src/sources/tpb.rs b/breadarrd/src/sources/tpb.rs index b6730d6..e72d01f 100644 --- a/breadarrd/src/sources/tpb.rs +++ b/breadarrd/src/sources/tpb.rs @@ -4,6 +4,17 @@ use serde::Deserialize; use super::{urlencode, RawReleaseItem, ReleaseSource}; +/// A valid BitTorrent v1 info_hash: 40 hex chars or 32 base32 chars — same +/// shape `qbit::extract_btih` accepts out of a magnet URI. Checked before +/// building a magnet from `info_hash` at all: apibay is normally reliable, +/// but a malformed value would otherwise silently produce a magnet +/// `extract_btih` can't parse back out, downgrading that grab to the slow +/// ~30s `torrents/info` polling path with no visible error anywhere. +fn is_valid_info_hash(hash: &str) -> bool { + (hash.len() == 40 && hash.bytes().all(|b| b.is_ascii_hexdigit())) + || (hash.len() == 32 && hash.bytes().all(|b| matches!(b, b'2'..=b'7' | b'a'..=b'z' | b'A'..=b'Z'))) +} + /// A community-run JSON API mirror of The Pirate Bay's search — unlike /// 1337x, this is a genuine machine-readable API (not HTML scraping), and /// unlike 1337x's HTML results table, `info_hash` is enough to build a @@ -85,8 +96,15 @@ impl ReleaseSource for TpbSource { // A query with no matches returns a single sentinel row // (id="0", an all-zero info_hash) rather than an empty array — // has to be filtered out explicitly or it'd be treated as one - // real (and completely bogus) result. - .filter(|r| r.id != "0" && !r.info_hash.chars().all(|c| c == '0')) + // real (and completely bogus) result. The all-zero hash is + // itself 40 valid hex characters, so `is_valid_info_hash` alone + // wouldn't catch it — both checks are needed, not one replacing + // the other. + .filter(|r| { + r.id != "0" + && !r.info_hash.chars().all(|c| c == '0') + && is_valid_info_hash(&r.info_hash) + }) .map(|r| RawReleaseItem { title: r.name.clone(), link: build_magnet(&r.info_hash, &r.name), @@ -125,8 +143,31 @@ mod tests { let results: Vec = serde_json::from_str(body).unwrap(); let filtered: Vec<_> = results .into_iter() - .filter(|r| r.id != "0" && !r.info_hash.chars().all(|c| c == '0')) + .filter(|r| { + r.id != "0" + && !r.info_hash.chars().all(|c| c == '0') + && is_valid_info_hash(&r.info_hash) + }) .collect(); assert!(filtered.is_empty()); } + + #[test] + fn is_valid_info_hash_accepts_both_real_shapes() { + assert!(is_valid_info_hash("8F87C7C186172F17E35F4512BB1A3E93B614ADED")); // 40 hex + assert!(is_valid_info_hash("abcdefghijklmnopqrstuvwxyz234567")); // 32 base32 + } + + // Regression test for a real gap found in review: a malformed + // `info_hash` from apibay used to flow straight into `build_magnet` + // with no validation, silently producing a magnet `qbit::extract_btih` + // can't parse back out — downgrading that grab to the slow polling path + // with no error surfaced anywhere. + #[test] + fn is_valid_info_hash_rejects_malformed_values() { + assert!(!is_valid_info_hash("")); + assert!(!is_valid_info_hash("too-short")); + assert!(!is_valid_info_hash("not-a-hex-string-at-all-nope!!!!!!!!!!!!")); // 40 chars, non-hex + assert!(!is_valid_info_hash("8F87C7C186172F17E35F4512BB1A3E93B614ADE")); // 39 hex chars + } }