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)
This commit is contained in:
Breadway
parent
66d323b7f7
commit
0f609aa4cc
9 changed files with 427 additions and 48 deletions
|
|
@ -109,13 +109,27 @@ async fn require_api_token(State(state): State<AppState>, 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("", ""));
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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(),
|
||||
|
|
|
|||
|
|
@ -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<Vec<f32>> {
|
||||
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<Vec<f32>> {
|
||||
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<Option<(usize, f32)>> {
|
||||
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<MatchOutcome> {
|
||||
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
|
||||
|
|
|
|||
|
|
@ -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)");
|
||||
|
|
|
|||
|
|
@ -90,15 +90,46 @@ static SXX_EPISODE_RANGE_RE: LazyLock<Regex> =
|
|||
// elsewhere in the title with a smaller trailing number.
|
||||
static BARE_EPISODE_RANGE_RE: LazyLock<Regex> =
|
||||
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<Regex> =
|
||||
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<u32> {
|
|||
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<regex::Captures<'_>> {
|
||||
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<u32>, Option<u32>, 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<u32> = 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<u32> = c[1].parse().ok();
|
||||
return (None, episode, episode, c.get(0).unwrap().start());
|
||||
}
|
||||
|
|
|
|||
|
|
@ -2112,6 +2112,30 @@ pub fn prepare_review_approval(conn: &Connection, review_id: i64) -> Result<Appr
|
|||
let profile = load_quality_profile(conn, media_item.quality_profile_id, profile_kind)?;
|
||||
let score = scoring::score(&parsed, 0, false, &profile);
|
||||
|
||||
// Claimed atomically here, still under the caller's DB lock — a real
|
||||
// TOCTOU otherwise: the caller only checked `status == "pending"` above
|
||||
// (a plain read, not a claim), then drops the lock and does the actual
|
||||
// grab (~30s of network/qBittorrent I/O) before `finalize_review_approval`
|
||||
// ever writes anything. A double-click, or an HTTP client retrying after
|
||||
// an apparent timeout, lands two concurrent `approve()` calls that both
|
||||
// pass the read above, both grab the same release, and both write their
|
||||
// own `release`/`torrent_fetch` rows for it. Marking `approved` here
|
||||
// rather than waiting for `finalize_review_approval` closes that window;
|
||||
// `approved` at this point means "no longer available for a second
|
||||
// approval attempt," not "successfully grabbed" — same distinction
|
||||
// `release.status` already draws between `grabbed` and `failed`, and if
|
||||
// the grab itself then errors outright, `release_review_claim` reverts
|
||||
// this back to `pending` (see its own doc comment).
|
||||
let claimed = conn.execute(
|
||||
"UPDATE review_queue SET status = 'approved' WHERE id = ?1 AND status = 'pending'",
|
||||
params![review_id],
|
||||
)?;
|
||||
if claimed == 0 {
|
||||
// Lost the race to a concurrent approve() between the read above
|
||||
// and this claim.
|
||||
return Ok(ApprovalPrep::NotPending);
|
||||
}
|
||||
|
||||
Ok(ApprovalPrep::Ready(PreparedApproval {
|
||||
media_item_id: media_item.id,
|
||||
episode_id,
|
||||
|
|
@ -2123,6 +2147,30 @@ pub fn prepare_review_approval(conn: &Connection, review_id: i64) -> Result<Appr
|
|||
}))
|
||||
}
|
||||
|
||||
/// Releases a claim `prepare_review_approval` took when the grab itself
|
||||
/// then fails outright (a network/qBittorrent error — `Err` from
|
||||
/// `grab_prepared_approval`, not just a missing hash on an otherwise-ok add;
|
||||
/// see `finalize_review_approval`'s own handling for that case, which still
|
||||
/// runs to completion and records a `failed` release). Without this, an
|
||||
/// approval claimed just before a transient qBittorrent outage would be
|
||||
/// stuck `approved` forever with no `release`/`torrent_fetch` row to show
|
||||
/// for it — worse than the un-atomic version this replaced, which at least
|
||||
/// left the row `pending` and retryable. Guarded on `WHERE status =
|
||||
/// 'approved'` mainly to no-op if the row somehow moved on already (e.g. a
|
||||
/// racing `reject`), not because `approved` distinguishes "merely claimed"
|
||||
/// from "successfully finalized" — it doesn't, `review_queue.status` has no
|
||||
/// separate state for that. Safe in practice because a single request's
|
||||
/// grab either errors (this runs, nothing was ever finalized) or succeeds
|
||||
/// (`finalize_review_approval` runs instead, this never gets called) — the
|
||||
/// two are mutually exclusive within one `approve()` call.
|
||||
pub fn release_review_claim(conn: &Connection, review_id: i64) -> 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();
|
||||
|
|
|
|||
|
|
@ -45,7 +45,17 @@ pub(crate) fn urlencode(s: &str) -> String {
|
|||
|
||||
pub(crate) fn parse_human_size(s: &str) -> Option<u64> {
|
||||
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));
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -52,6 +52,39 @@ fn build_search_url(feed_url: &str, query: Option<&str>) -> String {
|
|||
}
|
||||
}
|
||||
|
||||
/// Every field accumulated across one `<item>`'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<String>,
|
||||
link: Option<String>,
|
||||
guid: Option<String>,
|
||||
seeders: Option<u32>,
|
||||
leechers: Option<u32>,
|
||||
size_bytes: Option<u64>,
|
||||
}
|
||||
|
||||
/// 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<Vec<RawReleaseItem>> {
|
||||
let mut reader = Reader::from_reader(bytes);
|
||||
reader.config_mut().trim_text(true);
|
||||
|
|
@ -61,12 +94,7 @@ fn parse_nyaa_rss(bytes: &[u8]) -> Result<Vec<RawReleaseItem>> {
|
|||
|
||||
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<Vec<RawReleaseItem>> {
|
|||
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 `<title>`/`<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]]]></title>
|
||||
<link><![CDATA[https://nyaa.si/download/2130903.torrent]]></link>
|
||||
<guid isPermaLink="true"><![CDATA[https://nyaa.si/view/2130903]]></guid>
|
||||
<nyaa:seeders>12</nyaa:seeders>
|
||||
<nyaa:leechers>3</nyaa:leechers>
|
||||
<nyaa:size>356.5 MiB</nyaa:size>
|
||||
</item>
|
||||
</channel>
|
||||
</rss>"#;
|
||||
|
||||
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!(
|
||||
|
|
|
|||
|
|
@ -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<TpbResult> = 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
|
||||
}
|
||||
}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue