diff --git a/README.md b/README.md index 03b5971..1bed58e 100644 --- a/README.md +++ b/README.md @@ -404,6 +404,23 @@ comments,world,epub,publish,server}` implement them; `src/auth.rs` owns the rati token formula used by both the EPUB writer and the server; `src/types.rs` is the contract between stages. +### Auditing images against real articles + +Image handling fails in ways no synthetic fixture predicts, because every +publisher invents its own lazy-loading scheme. `examples/image_audit.rs` replays +the extraction and image pipeline over the articles of issues already published +and counts what actually reaches the page: + +```sh +cargo run --release --example image_audit -- \ + --cache /tmp/pagecache ~/bookorbit/books/daily-epub/*.epub +``` + +It prints per-article `refs / embedded / shown / placeholders`, a tally of loss +reasons, and totals. Pages are cached on first run, so a change can be measured +against byte-identical input; `--dump ` lists the URLs one +article resolved to. It needs the network and is not part of `cargo test`. + --- ## Known limitations @@ -425,6 +442,10 @@ From spec §7, plus what implementation turned up: falls back to matching the feed title. - **Images are downloaded once per edition** (the two editions need different resolutions and colour profiles), so an image-heavy issue makes two passes. +- **An image that cannot be embedded is dropped, not announced,** unless its alt + text is a real description — decorative rules, spacers and dead links would + otherwise litter the page with `[image: …]` lines. Verify image handling with + `examples/image_audit.rs` after touching extraction. - **`dc:date` rides inside a `dcterms:date` metadata fragment** because `epub-builder` neither exposes `dc:date` nor accepts a non-`chrono` date. The OPF output is correct; the mechanism is a workaround. diff --git a/examples/image_audit.rs b/examples/image_audit.rs new file mode 100644 index 0000000..aa73217 --- /dev/null +++ b/examples/image_audit.rs @@ -0,0 +1,446 @@ +//! Real-world image audit: replay the extraction + image pipeline over the +//! articles of already-published issues and report what reaches the page. +//! +//! Unit tests can only prove the code does what we think on markup we wrote. +//! This runs the same code over the pages the issues were actually built from, +//! so a regression in lazy-image handling, URL matching or re-encoding shows up +//! as a number rather than a hunch. +//! +//! ```text +//! cargo run --release --example image_audit -- \ +//! --cache /tmp/pagecache ~/bookorbit/books/daily-epub/*.epub +//! ``` +//! +//! Pages are cached on disk after the first run, so before/after comparisons +//! see byte-identical input. Articles are re-fetched and re-extracted through +//! readability even when the original issue took its body from Miniflux, so the +//! counts describe the fetch path, not that specific issue's history. + +use std::collections::BTreeMap; +use std::io::Read; +use std::path::{Path, PathBuf}; + +use daily_epub::epub::{build, images}; +use daily_epub::extract::{self, Extractor}; +use daily_epub::types::{Article, EntryId, ExtractMethod, ImageAsset, Pick, SourceKind, SourceRef}; + +/// One article we are auditing, pulled back out of a published EPUB. +#[derive(Debug, Clone)] +struct Target { + issue: String, + entry_id: EntryId, + title: String, + url: String, +} + +/// What the pipeline did with one article's images. +#[derive(Debug, Default, Clone)] +struct Outcome { + /// `<img>` elements in the extracted body. + refs: usize, + /// Images that downloaded and re-encoded successfully. + embedded: usize, + /// `<img src="images/…">` in the rendered chapter. + rendered: usize, + /// `[image: …]` paragraphs in the rendered chapter. + placeholders: usize, + /// Reasons individual images did not make it, most specific first. + losses: Vec<String>, +} + +#[tokio::main] +async fn main() { + tracing_subscriber::fmt() + .with_writer(std::io::stderr) + .with_env_filter( + tracing_subscriber::EnvFilter::try_from_default_env() + .unwrap_or_else(|_| "error".into()), + ) + .init(); + + let mut cache = PathBuf::from("/tmp/daily-epub-audit-cache"); + let mut dump: Option<String> = None; + let mut epubs: Vec<PathBuf> = Vec::new(); + let mut args = std::env::args().skip(1); + while let Some(arg) = args.next() { + match arg.as_str() { + "--cache" => cache = PathBuf::from(args.next().expect("--cache needs a path")), + "--dump" => dump = Some(args.next().expect("--dump needs a title substring")), + other => epubs.push(PathBuf::from(other)), + } + } + if epubs.is_empty() { + eprintln!("usage: image_audit [--cache DIR] <issue.epub>..."); + std::process::exit(2); + } + std::fs::create_dir_all(&cache).expect("cache dir"); + + // One entry per article: passing `*.epub` picks up both editions of every + // issue, and the same article twice would collide on its asset ids. + let mut seen: std::collections::HashSet<EntryId> = std::collections::HashSet::new(); + let targets: Vec<Target> = epubs + .iter() + .flat_map(|p| targets_from_epub(p)) + .filter(|t| seen.insert(t.entry_id)) + .collect(); + eprintln!( + "auditing {} articles from {} files", + targets.len(), + epubs.len() + ); + + let http = daily_epub::http::build_client(std::time::Duration::from_secs(20)).unwrap(); + let extractor = Extractor::new(http.clone(), vec![]); + + // Extract every article first, then run one issue-wide image pass, exactly + // as `epub::build_edition` does. + let mut picks: Vec<Pick> = Vec::new(); + let mut skipped: Vec<(Target, String)> = Vec::new(); + for target in &targets { + match body_for(&cache, &http, &extractor, target).await { + Ok(html) => picks.push(pick_for(target, html)), + Err(e) => skipped.push((target.clone(), e)), + } + } + + let assets = + images::collect_for_issue(&http, &picks, daily_epub::types::Edition::Standard).await; + eprintln!("embedded {} images", assets.len()); + + let by_entry: BTreeMap<EntryId, Vec<&ImageAsset>> = assets.iter().fold( + BTreeMap::new(), + |mut acc: BTreeMap<EntryId, Vec<&ImageAsset>>, a| { + if let Some(id) = entry_of(&a.id) { + acc.entry(id).or_default().push(a); + } + acc + }, + ); + + let mut totals = Outcome::default(); + let mut rows: Vec<(Target, Outcome)> = Vec::new(); + for (target, pick) in targets_of(&targets, &picks) { + let refs = images::extract_img_refs(&pick.article.content_html); + let embedded = by_entry.get(&target.entry_id).map_or(0, |v| v.len()); + let body = build::prepare_body(&pick.article.content_html, &assets); + let rendered = body.matches("<img src=\"images/").count(); + let placeholders = body.matches("image-placeholder").count(); + + let embedded_urls: Vec<&str> = by_entry + .get(&target.entry_id) + .map(|v| v.iter().map(|a| a.source_url.as_str()).collect()) + .unwrap_or_default(); + let losses = refs + .iter() + .filter(|r| !embedded_urls.contains(&r.src.as_str())) + .map(|r| classify(&r.src)) + .collect::<Vec<_>>(); + + if dump.as_ref().is_some_and(|d| target.title.contains(d)) { + println!("\n== {} [{}]", target.title, target.url); + for r in &refs { + let state = if embedded_urls.contains(&r.src.as_str()) { + "ok " + } else { + "MISS" + }; + println!(" {state} {}", r.src); + } + println!(" --- rendered body images:"); + for cap in body.split("<img src=\"").skip(1) { + println!(" {}", cap.split('"').next().unwrap_or_default()); + } + } + + let outcome = Outcome { + refs: refs.len(), + embedded, + rendered, + placeholders, + losses, + }; + totals.refs += outcome.refs; + totals.embedded += outcome.embedded; + totals.rendered += outcome.rendered; + totals.placeholders += outcome.placeholders; + totals.losses.extend(outcome.losses.iter().cloned()); + rows.push((target.clone(), outcome)); + } + + println!( + "\n{:<12} {:>5} {:>5} {:>5} {:>5} article", + "issue", "refs", "emb", "shown", "ph" + ); + println!("{}", "-".repeat(100)); + for (t, o) in &rows { + let flag = if o.placeholders > 0 || o.rendered < o.refs { + "!" + } else { + " " + }; + println!( + "{:<12} {:>5} {:>5} {:>5} {:>5} {} {} [{}]", + t.issue, + o.refs, + o.embedded, + o.rendered, + o.placeholders, + flag, + truncate(&t.title, 44), + host(&t.url), + ); + for loss in &o.losses { + println!("{:>36} lost: {loss}", ""); + } + } + + println!("\n== totals"); + println!(" articles {}", rows.len()); + println!(" <img> in bodies {}", totals.refs); + println!(" embedded {}", totals.embedded); + println!(" shown in chapters {}", totals.rendered); + println!(" placeholders {}", totals.placeholders); + println!( + " orphaned assets {}", + totals.embedded.saturating_sub(totals.rendered) + ); + let mut kinds: BTreeMap<String, usize> = BTreeMap::new(); + for loss in &totals.losses { + *kinds.entry(loss.clone()).or_default() += 1; + } + println!("\n== loss reasons"); + for (kind, n) in &kinds { + println!(" {n:>4} {kind}"); + } + if !skipped.is_empty() { + println!("\n== unfetchable ({})", skipped.len()); + for (t, e) in &skipped { + println!(" {} — {e}", truncate(&t.title, 50)); + } + } +} + +/// Pair each target with the pick built from it (targets that failed to fetch +/// have no pick and are skipped). +fn targets_of<'a>(targets: &'a [Target], picks: &'a [Pick]) -> Vec<(&'a Target, &'a Pick)> { + picks + .iter() + .filter_map(|p| { + targets + .iter() + .find(|t| t.entry_id == p.article.best_entry_id) + .map(|t| (t, p)) + }) + .collect() +} + +fn entry_of(asset_id: &str) -> Option<EntryId> { + asset_id + .strip_prefix("img-")? + .split('-') + .next()? + .parse() + .ok() +} + +/// Why one `<img>` never became an embedded asset — a guess from its URL, good +/// enough to group the failures. +fn classify(src: &str) -> String { + let lower = src.to_ascii_lowercase(); + if src.contains('{') || src.contains('}') { + format!("unresolved URL template — {}", truncate(src, 90)) + } else if src.contains(' ') || src.contains("%20") { + format!("srcset blob in src — {}", truncate(src, 90)) + } else if lower.contains(".svg") { + format!("svg — {}", truncate(src, 90)) + } else if src.starts_with("data:") { + "data: URI (lazy placeholder)".to_string() + } else { + format!("download/decode failed — {}", truncate(src, 90)) + } +} + +/// Fetch + extract one article, using the on-disk page cache. +async fn body_for( + cache: &Path, + _http: &reqwest::Client, + extractor: &Extractor, + target: &Target, +) -> Result<String, String> { + let key = cache.join(format!("{:x}.html", seahash(&target.url))); + let url_key = key.with_extension("url"); + if !key.exists() { + // `fetch_readable` does the fetching we want but returns readability's + // output; cache the raw page instead so extraction changes are visible. + let (raw, final_url) = raw_fetch(&target.url).await?; + std::fs::write(&key, raw).map_err(|e| e.to_string())?; + std::fs::write(&url_key, final_url).map_err(|e| e.to_string())?; + } + let bytes = std::fs::read(&key).map_err(|e| e.to_string())?; + if bytes.is_empty() { + return Err("empty page".into()); + } + // Relative URLs belong to the page we landed on, not the one we asked for. + let base = std::fs::read_to_string(&url_key).unwrap_or_else(|_| target.url.clone()); + let html = String::from_utf8_lossy(&bytes).into_owned(); + let _ = extractor; + let readable = extract::readability(&html, &base).map_err(|e| e.to_string())?; + Ok(extract::sanitize_with_base( + &extract::normalize_img_tags(&readable), + &base, + )) +} + +/// A plain page fetch with the extractor's desktop UA and gzip handling, +/// returning the body and the URL the fetch landed on. +async fn raw_fetch(url: &str) -> Result<(Vec<u8>, String), String> { + let client = reqwest::Client::builder() + .user_agent(extract::DESKTOP_UA) + .gzip(true) + .timeout(std::time::Duration::from_secs(30)) + .build() + .map_err(|e| e.to_string())?; + let resp = client + .get(url) + .header( + reqwest::header::ACCEPT, + "text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8", + ) + .send() + .await + .map_err(|e| e.to_string())?; + if !resp.status().is_success() { + return Err(format!("HTTP {}", resp.status())); + } + let final_url = resp.url().to_string(); + resp.bytes() + .await + .map(|b| (b.to_vec(), final_url)) + .map_err(|e| e.to_string()) +} + +fn pick_for(target: &Target, content_html: String) -> Pick { + let word_count = extract::word_count(&content_html); + let image_urls = extract::collect_image_urls(&content_html, &target.url); + Pick { + article: Article { + id: target.entry_id, + canonical_url: target.url.clone(), + title: target.title.clone(), + best_entry_id: target.entry_id, + content_html, + word_count, + excerpt_only: false, + image_count: image_urls.len() as i64, + image_urls, + sources: vec![SourceRef { + entry_id: target.entry_id, + feed_id: 1, + feed_title: "Feed".into(), + category: None, + kind: SourceKind::Feed, + }], + first_seen: "2026-08-15T05:30:00Z".parse().unwrap(), + url: target.url.clone(), + author: None, + feed_id: 1, + feed_title: "Feed".into(), + category: None, + published_at: None, + comments_url: None, + social: vec![], + extract_method: ExtractMethod::Readability, + }, + section: "Audit".into(), + position: 0, + is_lead: false, + summary: None, + llm: None, + discussion: None, + } +} + +/// Pull `(entry id, title, url)` out of every article chapter in an EPUB. +fn targets_from_epub(path: &Path) -> Vec<Target> { + let issue = path + .file_stem() + .map(|s| s.to_string_lossy().replace("The Daily EPUB - ", "")) + .unwrap_or_default(); + let file = std::fs::File::open(path).expect("open epub"); + let mut zip = zip::ZipArchive::new(file).expect("read epub"); + let names: Vec<String> = (0..zip.len()) + .filter_map(|i| zip.by_index(i).ok().map(|f| f.name().to_string())) + .filter(|n| n.contains("art-") && n.ends_with(".xhtml")) + .collect(); + + let mut out = Vec::new(); + for name in names { + let mut xhtml = String::new(); + if zip + .by_name(&name) + .and_then(|mut f| f.read_to_string(&mut xhtml).map_err(Into::into)) + .is_err() + { + continue; + } + let Some(entry_id) = name + .rsplit('/') + .next() + .and_then(|f| f.strip_prefix("art-")) + .and_then(|f| f.strip_suffix(".xhtml")) + .and_then(|f| f.parse::<EntryId>().ok()) + else { + continue; + }; + let Some(url) = between(&xhtml, r#"class="read-online"><a href=""#, '"') else { + continue; + }; + let title = between(&xhtml, "<title>", '<').unwrap_or_else(|| "?".into()); + out.push(Target { + issue: issue.clone(), + entry_id, + title: unescape(&title), + url: unescape(&url), + }); + } + out +} + +fn between(haystack: &str, prefix: &str, end: char) -> Option<String> { + let start = haystack.find(prefix)? + prefix.len(); + let rest = &haystack[start..]; + let stop = rest.find(end)?; + Some(rest[..stop].to_string()) +} + +fn unescape(s: &str) -> String { + s.replace("&", "&") + .replace("'", "'") + .replace(""", "\"") + .replace("<", "<") + .replace(">", ">") +} + +fn host(url: &str) -> String { + url::Url::parse(url) + .ok() + .and_then(|u| u.host_str().map(str::to_string)) + .unwrap_or_default() +} + +fn truncate(s: &str, n: usize) -> String { + if s.chars().count() <= n { + return s.to_string(); + } + s.chars().take(n.saturating_sub(1)).collect::<String>() + "…" +} + +/// Tiny stable hash for cache filenames. +fn seahash(s: &str) -> u64 { + let mut h: u64 = 0xcbf2_9ce4_8422_2325; + for b in s.as_bytes() { + h ^= u64::from(*b); + h = h.wrapping_mul(0x0000_0100_0000_01b3); + } + h +} diff --git a/src/epub/build.rs b/src/epub/build.rs index 9793808..2bb1e82 100644 --- a/src/epub/build.rs +++ b/src/epub/build.rs @@ -451,7 +451,7 @@ fn encode_cover(pixmap: tiny_skia::Pixmap, edition: Edition) -> Result<Vec<u8>, // --------------------------------------------------------------------------- /// Prepare article markup for XHTML: rewrite images, sanitize, self-close voids. -fn prepare_body(html: &str, images_: &[ImageAsset]) -> String { +pub fn prepare_body(html: &str, images_: &[ImageAsset]) -> String { // Sanitize first: image rewriting emits our own trusted markup (including the // `image-placeholder` class, which ammonia would otherwise strip). let cleaned = ammonia::clean(html); @@ -1013,7 +1013,7 @@ pub mod fixtures { title: title.to_string(), best_entry_id: entry_id, content_html: format!( - "<p>Body of <em>{title}</em> with an image.</p><img src=\"https://img.example/{entry_id}.png\" alt=\"A chart\"><p>More words & things.</p>" + "<p>Body of <em>{title}</em> with an image.</p><img src=\"https://img.example/{entry_id}.png\" alt=\"A chart of the daily figures\"><p>More words & things.</p>" ), word_count: 1200, excerpt_only: false, @@ -1307,7 +1307,11 @@ mod tests { assert!(chapter.xhtml.contains("Read online")); assert!(chapter.xhtml.contains("href=\"disc-1001.xhtml\"")); // The un-downloaded image degrades to a placeholder. - assert!(chapter.xhtml.contains("[image: A chart]")); + assert!( + chapter + .xhtml + .contains("[image: A chart of the daily figures]") + ); assert_xml_ok(&chapter.xhtml); } diff --git a/src/epub/images.rs b/src/epub/images.rs index a002155..e6a4c11 100644 --- a/src/epub/images.rs +++ b/src/epub/images.rs @@ -22,8 +22,8 @@ pub const CONCURRENCY: usize = 8; pub const ISSUE_ASSET_BUDGET_BYTES: usize = 25 * 1024 * 1024; /// Images smaller than this in either dimension are decorative — skipped (§3.10). pub const MIN_DIMENSION_PX: u32 = 24; -/// Images referenced per article are already capped at 12 by extraction (§3.3). -pub const MAX_IMAGES_PER_ARTICLE: usize = 12; +/// Width/height an SVG is rasterized to when it declares no intrinsic size. +const SVG_FALLBACK_SIZE: u32 = 1000; /// HTML void elements: XHTML requires them self-closed (§3.10 "valid XHTML"). pub const VOID_ELEMENTS: &[&str] = &[ @@ -136,6 +136,8 @@ pub fn extract_img_refs(html: &str) -> Vec<ImgRef> { pub async fn download(http: &reqwest::Client, url: &str) -> Option<Vec<u8>> { let resp = http .get(url) + // Some CDNs answer `Accept: */*` with an HTML interstitial (§3.10). + .header(reqwest::header::ACCEPT, "image/*,*/*;q=0.8") .timeout(Duration::from_secs(DOWNLOAD_TIMEOUT_SECS)) .send() .await @@ -175,8 +177,14 @@ pub async fn download(http: &reqwest::Client, url: &str) -> Option<Vec<u8>> { /// Decode, resize/grayscale, flatten transparency to white and re-encode (§3.10). /// /// Line art with transparency is kept as PNG after flattening; everything else -/// becomes JPEG. Returns `None` for undecodable sources (SVG/WebP without support). +/// becomes JPEG. SVG is rasterized first — charts and diagrams are frequently +/// vector-only, and dropping them loses the point of the article. Returns `None` +/// for sources no decoder handles. pub fn reencode(bytes: &[u8], profile: ImageProfile) -> Option<(Vec<u8>, &'static str)> { + if looks_like_svg(bytes) { + let raster = rasterize_svg(bytes, profile)?; + return reencode(&raster, profile); + } let format = image::guess_format(bytes).ok(); let decoded = image::load_from_memory(bytes) .map_err(|e| tracing::debug!("undecodable image: {e}")) @@ -239,6 +247,74 @@ pub fn reencode(bytes: &[u8], profile: ImageProfile) -> Option<(Vec<u8>, &'stati Some((out.into_inner(), "image/jpeg")) } +/// True when `bytes` are an SVG document (possibly behind an XML prolog or BOM). +fn looks_like_svg(bytes: &[u8]) -> bool { + let head = &bytes[..bytes.len().min(1024)]; + let text = String::from_utf8_lossy(head); + let text = text.trim_start_matches('\u{feff}').trim_start(); + text.starts_with("<svg") + || (text.starts_with("<?xml") || text.starts_with("<!DOCTYPE svg")) && text.contains("<svg") +} + +/// Rasterize an SVG to a PNG at the profile's target width (§3.10). +/// +/// The profile's own resize pass then handles the height cap, so this only has +/// to land in the right ballpark. +fn rasterize_svg(bytes: &[u8], profile: ImageProfile) -> Option<Vec<u8>> { + let mut options = resvg::usvg::Options::default(); + options.fontdb_mut().load_system_fonts(); + let tree = resvg::usvg::Tree::from_data(bytes, &options) + .map_err(|e| tracing::debug!("svg did not parse: {e}")) + .ok()?; + + // An `<svg>` that parses but draws nothing is not an image, it is a stray + // tag: rasterizing it would embed a blank rectangle. + if tree.root().children().is_empty() { + tracing::debug!("svg has nothing to draw"); + return None; + } + let size = tree.size(); + let (sw, sh) = (size.width(), size.height()); + if !(sw.is_finite() && sh.is_finite()) || sw <= 0.0 || sh <= 0.0 { + return None; + } + // Judge "decorative" by the declared size, before scaling: a 16×16 icon is + // an icon however large we choose to draw it. + if sw < MIN_DIMENSION_PX as f32 || sh < MIN_DIMENSION_PX as f32 { + tracing::debug!(sw, sh, "skipping decorative svg"); + return None; + } + // Vector art has no native resolution, so render straight at the edition's + // target width — upscaling a rasterized copy afterwards would only blur it. + let target_w = profile + .max_width + .max(SVG_FALLBACK_SIZE.min(profile.max_width)); + let scale = (target_w as f32 / sw).min(profile.max_height as f32 / sh); + let scale = if scale.is_finite() && scale > 0.0 { + scale + } else { + 1.0 + }; + let (w, h) = ( + (sw * scale).round().max(1.0) as u32, + (sh * scale).round().max(1.0) as u32, + ); + let mut pixmap = tiny_skia::Pixmap::new(w, h)?; + // E-ink has no transparency; render onto white so alpha never becomes black. + pixmap.fill(tiny_skia::Color::WHITE); + resvg::render( + &tree, + tiny_skia::Transform::from_scale(scale, scale), + &mut pixmap.as_mut(), + ); + let rgba = image::RgbaImage::from_raw(w, h, pixmap.take_demultiplied())?; + let mut png = Cursor::new(Vec::new()); + DynamicImage::ImageRgba8(rgba) + .write_to(&mut png, ImageFormat::Png) + .ok()?; + Some(png.into_inner()) +} + /// Composite over an opaque white page — e-ink has no transparency (§3.10). fn flatten_to_white(img: &DynamicImage) -> DynamicImage { let rgba = img.to_rgba8(); @@ -302,7 +378,6 @@ fn pending_for_pick(pick: &Pick) -> Vec<PendingImage> { } refs.into_iter() .filter(|r| r.src.starts_with("http://") || r.src.starts_with("https://")) - .take(MAX_IMAGES_PER_ARTICLE) .enumerate() .map(|(i, r)| PendingImage { id: format!("img-{entry_id}-{i}"), @@ -409,8 +484,12 @@ pub(crate) fn tag_name(inner: &str) -> String { .to_ascii_lowercase() } -/// Parse `name="value"` pairs out of a tag body. -fn parse_attrs(inner: &str) -> Vec<(String, String)> { +/// Parse `name="value"` pairs out of a tag body, with entity-decoded values. +/// +/// Decoding matters: this scanner reads markup that ammonia has serialized, and +/// ammonia writes `&` in a URL as `&`. A raw comparison against a URL that +/// came out of a real HTML parser would never match (§3.10). +pub(crate) fn parse_attrs(inner: &str) -> Vec<(String, String)> { let mut attrs = Vec::new(); let bytes: Vec<char> = inner.chars().collect(); let mut i = 0; @@ -457,11 +536,60 @@ fn parse_attrs(inner: &str) -> Vec<(String, String)> { } } } - attrs.push((name, value)); + attrs.push((name, decode_entities(&value))); } attrs } +/// Decode the handful of entities an HTML serializer emits inside attributes. +/// +/// Numeric forms are included because feeds and WordPress write `&` for +/// `&`; anything else is left alone rather than guessed at. +pub(crate) fn decode_entities(s: &str) -> String { + if !s.contains('&') { + return s.to_string(); + } + let mut out = String::with_capacity(s.len()); + let mut rest = s; + while let Some(i) = rest.find('&') { + out.push_str(&rest[..i]); + let tail = &rest[i..]; + let Some(end) = tail[..tail.len().min(12)].find(';') else { + out.push('&'); + rest = &tail[1..]; + continue; + }; + let entity = &tail[1..end]; + let decoded = match entity { + "amp" => Some('&'), + "lt" => Some('<'), + "gt" => Some('>'), + "quot" => Some('"'), + "apos" | "#39" => Some('\''), + "nbsp" => Some('\u{a0}'), + _ => entity + .strip_prefix('#') + .and_then(|n| match n.strip_prefix(['x', 'X']) { + Some(hex) => u32::from_str_radix(hex, 16).ok(), + None => n.parse::<u32>().ok(), + }) + .and_then(char::from_u32), + }; + match decoded { + Some(c) => { + out.push(c); + rest = &tail[end + 1..]; + } + None => { + out.push('&'); + rest = &tail[1..]; + } + } + } + out.push_str(rest); + out +} + /// Escape a string for use inside a double-quoted XML attribute. fn attr_escape(s: &str) -> String { let mut out = String::with_capacity(s.len()); @@ -491,6 +619,40 @@ pub fn text_escape(s: &str) -> String { out } +/// Whether an image we could not embed is worth telling the reader about. +/// +/// Only descriptive alt text qualifies. A filename, a bare label like `red line` +/// on a divider rule, or no alt at all carries nothing the reader loses by not +/// seeing the picture — announcing those turns every decorative graphic and +/// dead link into a line of clutter, which is how the placeholders got out of +/// hand in the first place. +fn alt_is_worth_announcing(alt: &str) -> bool { + const MIN_DESCRIPTIVE_WORDS: usize = 4; + !alt.is_empty() + && !is_filename_alt(alt) + && alt.split_whitespace().count() >= MIN_DESCRIPTIVE_WORDS +} + +/// True for alt text that is really just the uploaded filename — `IMG_0808.JPG`, +/// `cut pieces v01.JPG`, `chart-final-2.png`. +fn is_filename_alt(alt: &str) -> bool { + let alt = alt.trim(); + if alt.contains(' ') && alt.split_whitespace().count() > 4 { + return false; + } + let Some((stem, ext)) = alt.rsplit_once('.') else { + return false; + }; + let ext = ext.to_ascii_lowercase(); + matches!( + ext.as_str(), + "jpg" | "jpeg" | "png" | "gif" | "webp" | "svg" | "avif" | "bmp" | "heic" + ) && !stem.is_empty() + && stem + .chars() + .all(|c| c.is_alphanumeric() || matches!(c, ' ' | '_' | '-' | '.')) +} + /// Rewrite `<img src>` to the embedded hrefs, replacing misses with the /// `[image: alt]` placeholder paragraph (§3.10). pub fn rewrite_img_srcs(html: &str, assets: &[ImageAsset]) -> String { @@ -531,15 +693,15 @@ pub fn rewrite_img_srcs(html: &str, assets: &[ImageAsset]) -> String { attr_escape(alt) )); } + // An image we could not embed is only worth announcing when its + // alt text tells the reader something; otherwise the `<img>` + // just goes away. That covers the decorative graphics the + // re-encoder deliberately skips as well as genuine misses (§3.10). + None if !alt_is_worth_announcing(&alt) => {} None => { - let label = if alt.is_empty() { - "image unavailable" - } else { - &alt - }; out.push_str(&format!( "<p class=\"image-placeholder\">[image: {}]</p>", - text_escape(label) + text_escape(&alt) )); } } @@ -618,20 +780,76 @@ mod tests { #[test] fn rewrites_hits_and_placeholders_misses() { let assets = vec![asset("https://e.g/a.png", "images/img-1-0.jpg")]; - let html = r#"<p>x</p><img src="https://e.g/a.png" alt="Alt & more"><img src="https://e.g/gone.png" alt="Missing">"#; + let html = r#"<p>x</p><img src="https://e.g/a.png" alt="Alt & more"><img src="https://e.g/gone.png" alt="A chart of missing things">"#; let out = rewrite_img_srcs(html, &assets); - assert!(out.contains(r#"<img src="images/img-1-0.jpg" alt="Alt &amp; more"/>"#)); - assert!(out.contains(r#"<p class="image-placeholder">[image: Missing]</p>"#)); + // The alt round-trips through one level of escaping, not two. + assert!(out.contains(r#"<img src="images/img-1-0.jpg" alt="Alt & more"/>"#)); + assert!( + out.contains(r#"<p class="image-placeholder">[image: A chart of missing things]</p>"#) + ); assert!(!out.contains("gone.png")); } + /// The whole point of the fix: ammonia writes `&` into the markup, and + /// the asset was keyed on the URL a real parser produced. #[test] - fn placeholder_falls_back_when_alt_is_missing() { - let out = rewrite_img_srcs(r#"<img src="https://e.g/x.png">"#, &[]); + fn entity_encoded_urls_still_match_their_asset() { + let assets = vec![asset( + "https://e.g/a.jpg?id=1&width=980", + "images/img-1-0.jpg", + )]; + let html = r#"<img src="https://e.g/a.jpg?id=1&width=980" alt="Chart"/>"#; + assert!(rewrite_img_srcs(html, &assets).contains(r#"src="images/img-1-0.jpg""#)); + // The numeric spelling WordPress emits works too. + let html = r#"<img src="https://e.g/a.jpg?id=1&width=980" alt="Chart"/>"#; + assert!(rewrite_img_srcs(html, &assets).contains(r#"src="images/img-1-0.jpg""#)); + } + + #[test] + fn unembeddable_images_only_speak_up_when_the_alt_says_something() { + // No alt at all: the image simply disappears. assert_eq!( - out, - r#"<p class="image-placeholder">[image: image unavailable]</p>"# + rewrite_img_srcs(r#"<img src="https://e.g/x.png">"#, &[]), + "" ); + // A filename is not a description. + assert_eq!( + rewrite_img_srcs(r#"<img src="https://e.g/x.png" alt="IMG_0808.JPG">"#, &[]), + "" + ); + // Neither is the label on a decorative divider rule. + assert_eq!( + rewrite_img_srcs(r#"<img src="https://e.g/rule.png" alt="red line">"#, &[]), + "" + ); + // A real description is worth keeping. + assert!( + rewrite_img_srcs( + r#"<img src="https://e.g/x.png" alt="A man in a hard hat stands over a well hole">"#, + &[] + ) + .contains("[image: A man in a hard hat stands over a well hole]") + ); + } + + #[test] + fn filename_alt_detection() { + for yes in [ + "IMG_0808.JPG", + "cut pieces v01.JPG", + "chart-final-2.png", + "diagram.svg", + ] { + assert!(is_filename_alt(yes), "{yes} should read as a filename"); + } + for no in [ + "A hydrogen well head", + "", + "Fig. 3", + "The lion-man of Hohlenstein-Stadel, carved from mammoth ivory.", + ] { + assert!(!is_filename_alt(no), "{no} should read as a description"); + } } #[test] @@ -685,7 +903,37 @@ mod tests { .write_to(&mut png, ImageFormat::Png) .unwrap(); assert!(reencode(&png.into_inner(), ImageProfile::STANDARD).is_none()); + // A bare `<svg>` tag with nothing to draw is markup, not a picture. assert!(reencode(b"<svg>not an image</svg>", ImageProfile::STANDARD).is_none()); + assert!(reencode(b"not an image at all", ImageProfile::STANDARD).is_none()); + } + + #[test] + fn svg_charts_are_rasterized_rather_than_dropped() { + let svg = br##"<svg xmlns="http://www.w3.org/2000/svg" width="400" height="300"> + <rect x="10" y="10" width="380" height="280" fill="#3355bb"/> + <circle cx="200" cy="150" r="60" fill="#ffcc00"/> + </svg>"##; + let (bytes, mime) = reencode(svg, ImageProfile::STANDARD).expect("svg rasterizes"); + assert_eq!(mime, "image/png", "flat colour art stays lossless"); + let decoded = image::load_from_memory(&bytes).expect("decodable output"); + // Drawn at the edition's target width, not at the SVG's nominal size. + assert_eq!(decoded.dimensions(), (1200, 900)); + assert!(!decoded.color().has_alpha(), "rendered onto white"); + + // An XML prolog and a leading BOM must not hide the format. + let with_prolog = format!( + "\u{feff}<?xml version=\"1.0\"?>{}", + String::from_utf8_lossy(svg) + ); + let (x4, _) = reencode(with_prolog.as_bytes(), ImageProfile::X4).expect("x4 rasterizes"); + let x4 = image::load_from_memory(&x4).unwrap(); + assert!(x4.width() <= 480 && x4.height() <= 800); + + // A 16×16 icon is decorative however large we could draw it. + let icon = br#"<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16"> + <rect width="16" height="16"/></svg>"#; + assert!(reencode(icon, ImageProfile::STANDARD).is_none()); } #[test] diff --git a/src/extract.rs b/src/extract.rs index 5dfde4b..894398a 100644 --- a/src/extract.rs +++ b/src/extract.rs @@ -11,14 +11,13 @@ use futures::StreamExt; use scraper::{Html, Node, Selector}; use url::Url; +use crate::epub::images::{tag_end, tag_name}; use crate::types::{Article, ExtractMethod, Extracted}; /// Word count at or above which Miniflux content is treated as full text (§3.3). pub const FULL_TEXT_MIN_WORDS: i64 = 250; /// Maximum bytes downloaded when fetching an article page (§3.3). pub const MAX_FETCH_BYTES: usize = 3 * 1024 * 1024; -/// Maximum images collected per article (§3.3). -pub const MAX_IMAGES_PER_ARTICLE: usize = 12; /// Note appended to bodies we could only excerpt (§3.3). pub const EXCERPT_NOTE: &str = "(excerpt only — read online)"; @@ -130,20 +129,37 @@ impl Extractor { let _guard = span.enter(); // 1. Miniflux content, when it already looks like full text. - let feed_html = sanitize_with_base(&article.content_html, &article.url); + let feed_html = + sanitize_with_base(&normalize_img_tags(&article.content_html), &article.url); let feed_words = word_count(&feed_html); if feed_words >= FULL_TEXT_MIN_WORDS { - return self.finish(article, feed_html, feed_words, ExtractMethod::Miniflux); + return self.finish( + article, + feed_html, + feed_words, + ExtractMethod::Miniflux, + &article.url, + ); } // 2. Fetch the page and run readability over it. if self.can_fetch() { match self.fetch_readable(&article.url).await { - Ok(html) => { - let clean = sanitize_with_base(&html, &article.url); + Ok(page) => { + // Relative URLs in the markup belong to the page we ended up + // on, not the one we asked for: shortener and syndication + // links land on another host entirely. + let clean = + sanitize_with_base(&normalize_img_tags(&page.html), &page.final_url); let words = word_count(&clean); if words > feed_words && words > 0 { - return self.finish(article, clean, words, ExtractMethod::Readability); + return self.finish( + article, + clean, + words, + ExtractMethod::Readability, + &page.final_url, + ); } tracing::debug!(words, feed_words, "readability was not an improvement"); } @@ -160,20 +176,24 @@ impl Extractor { format!("{feed_html}<p>{EXCERPT_NOTE}</p>") }; let words = word_count(&body); - let mut out = self.finish(article, body, words, ExtractMethod::Excerpt); + let mut out = self.finish(article, body, words, ExtractMethod::Excerpt, &article.url); out.excerpt_only = true; out } /// Assemble the [`Extracted`] value once a body has been chosen. + /// + /// `base_url` is what the body's relative URLs resolve against — the page we + /// landed on for a fetched article, the article URL otherwise. fn finish( &self, article: &Article, content_html: String, words: i64, method: ExtractMethod, + base_url: &str, ) -> Extracted { - let image_urls = collect_image_urls(&content_html, &article.url); + let image_urls = collect_image_urls(&content_html, base_url); let excerpt_only = method == ExtractMethod::Excerpt || looks_paywalled(&article.url, words, &self.paywall_domains); Extracted { @@ -220,7 +240,10 @@ impl Extractor { /// Fetch `url` (10s timeout, desktop UA, [`MAX_FETCH_BYTES`] cap) and run /// `dom_smoothie` readability over it (§3.3). - pub async fn fetch_readable(&self, url: &str) -> Result<String, ExtractError> { + /// + /// Returns the URL the fetch actually landed on alongside the markup, so + /// callers resolve relative links against the right origin. + pub async fn fetch_readable(&self, url: &str) -> Result<Page, ExtractError> { let Some(http) = &self.http else { return Err(ExtractError::FetchDisabled); }; @@ -237,6 +260,7 @@ impl Extractor { if !response.status().is_success() { return Err(ExtractError::Status(response.status().as_u16())); } + let final_url = response.url().to_string(); if let Some(ct) = response .headers() .get(reqwest::header::CONTENT_TYPE) @@ -256,17 +280,36 @@ impl Extractor { body.extend_from_slice(&chunk); } let html = String::from_utf8_lossy(&body).into_owned(); - readability(&html, url) + Ok(Page { + html: readability(&html, &final_url)?, + final_url, + }) } } +/// An article page after fetching and readability (§3.3). +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Page { + /// Readability's main-content markup. + pub html: String, + /// Where the fetch ended up, after any redirects — the base for relative URLs. + pub final_url: String, +} + /// Run `dom_smoothie` over a fetched page and return its main-content HTML (§3.3). +/// +/// The page is normalized first ([`prepare_for_readability`]) so that readability +/// sees plain, already-resolved `<img src>` elements. Left to itself it damages +/// them in two ways: it deletes whole subtrees (lightbox `<button>` wrappers take +/// their image with them) and its lazy-image heuristic overwrites a perfectly +/// good `src` with whatever other attribute happens to contain `.jpg`. pub fn readability(html: &str, url: &str) -> Result<String, ExtractError> { + let html = prepare_for_readability(html); let config = dom_smoothie::Config { max_elements_to_parse: 60_000, ..Default::default() }; - let mut readability = dom_smoothie::Readability::new(html, Some(url), Some(config)) + let mut readability = dom_smoothie::Readability::new(html.as_str(), Some(url), Some(config)) .map_err(|_| ExtractError::NoContent)?; let parsed = readability.parse().map_err(|_| ExtractError::NoContent)?; let content = parsed.content.to_string(); @@ -300,6 +343,303 @@ fn merge_paywall_domains(configured: Vec<String>) -> Vec<String> { domains } +// --------------------------------------------------------------------------- +// Image normalization (§3.3) +// --------------------------------------------------------------------------- + +/// Elements that readability deletes outright, and which a page may nevertheless +/// have wrapped around an image (lightbox triggers, mostly). +const IMAGE_WRAPPER_TAGS: &[&str] = &["button", "form", "fieldset", "object"]; + +/// Attributes lazy-loading libraries use for the real image URL. +/// +/// These outrank `src`, because a page only sets them when `src` is a stand-in: +/// a transparent GIF, a blurred thumbnail, an inline SVG spacer. Attributes that +/// merely *look* image-ish (`data-template`, `data-attrs`, `data-orig-file`) are +/// deliberately absent — those hold templates and metadata, and preferring them +/// is exactly the mistake readability's own heuristic makes. +const LAZY_SRC_ATTRS: &[&str] = &[ + "data-src", + "data-lazy-src", + "data-original", + "data-runner-src", + "data-full-src", + "data-hi-res-src", + "data-image-src", +]; + +/// Widest `srcset` candidate we will pick; above this we are downloading pixels +/// the re-encoder immediately throws away. +const MAX_SRCSET_WIDTH: u32 = 2000; + +/// Make a fetched page safe to hand to readability (§3.3). +/// +/// Two passes, both about images: unwrap the elements that would take an image +/// with them when readability deletes them, then reduce every `<img>` to a plain +/// `src`/`alt`/`title` triple. The second pass is what stops readability's own +/// lazy-image heuristic from replacing a working `src` — with no `srcset`, +/// `loading` or `data-*` attributes left on the element, it has nothing to +/// substitute and leaves the image alone. +pub fn prepare_for_readability(html: &str) -> String { + normalize_img_tags(&unwrap_image_wrappers(html)) +} + +/// Replace image-only `<button>`/`<form>`/`<fieldset>`/`<object>` wrappers with +/// their contents (§3.3). +/// +/// A `<button>` holding nothing but an image is a lightbox trigger, not a +/// control: the image is the content. Wrappers that also carry text are left +/// alone, because those really are interface. +pub fn unwrap_image_wrappers(html: &str) -> String { + let mut out = String::with_capacity(html.len()); + let mut cursor = 0usize; + while let Some(rel) = html[cursor..].find('<') { + let start = cursor + rel; + out.push_str(&html[cursor..start]); + let Some(end) = tag_end(html, start) else { + out.push_str(&html[start..]); + return out; + }; + let raw = &html[start..end]; + let inner = raw.trim_start_matches('<').trim_end_matches('>'); + let name = tag_name(inner); + + if IMAGE_WRAPPER_TAGS.contains(&name.as_str()) + && !inner.trim_end().ends_with('/') + && let Some((content, after)) = element_content(html, end, &name) + && content.contains("<img") + && html_to_text(content).trim().is_empty() + { + out.push_str(&unwrap_image_wrappers(content)); + cursor = after; + continue; + } + + out.push_str(raw); + cursor = end; + } + out.push_str(&html[cursor..]); + out +} + +/// The content of an element whose open tag ended at `body_start`, plus the +/// offset just past its close tag. `None` when the element is never closed. +fn element_content<'a>(html: &'a str, body_start: usize, name: &str) -> Option<(&'a str, usize)> { + let open = format!("<{name}"); + let close = format!("</{name}"); + let mut depth = 1usize; + let mut cursor = body_start; + while let Some(rel) = html[cursor..].find('<') { + let start = cursor + rel; + let end = tag_end(html, start)?; + let tag = &html[start..end]; + let lower = tag.to_ascii_lowercase(); + if lower.starts_with(&close) { + depth -= 1; + if depth == 0 { + return Some((&html[body_start..start], end)); + } + } else if lower.starts_with(&open) + && !lower[open.len()..].starts_with(|c: char| c.is_alphanumeric() || c == '-') + && !tag.trim_end().ends_with("/>") + { + depth += 1; + } + cursor = end; + } + None +} + +/// Reduce every `<img>` to `<img src alt title>` with a usable URL (§3.3). +/// +/// The `src` a page ships is not automatically the one to use: it can be a lazy +/// placeholder (`data:image/svg+xml,…`), an unresolved template +/// (`…/resize/{width}/…`), a JSON blob a framework parked there, or an entire +/// `srcset` string. Candidates are tried in order and the first plausible one +/// wins; an image with no plausible candidate is dropped, because a broken +/// `<img>` only becomes clutter further down the pipeline. +pub fn normalize_img_tags(html: &str) -> String { + let mut out = String::with_capacity(html.len()); + let mut cursor = 0usize; + // `<picture>` puts the real candidates on sibling `<source>` elements. + let mut picture_srcset: Option<String> = None; + + while let Some(rel) = html[cursor..].find('<') { + let start = cursor + rel; + out.push_str(&html[cursor..start]); + let Some(end) = tag_end(html, start) else { + out.push_str(&html[start..]); + return out; + }; + let raw = &html[start..end]; + let inner = raw + .trim_start_matches('<') + .trim_end_matches('>') + .trim_end_matches('/'); + let name = tag_name(inner); + + match name.as_str() { + "picture" => { + picture_srcset = None; + out.push_str(raw); + } + "source" => { + let attrs = crate::epub::images::parse_attrs(inner); + if picture_srcset.is_none() + && let Some(set) = + attr(&attrs, "srcset").or_else(|| attr(&attrs, "data-srcset")) + { + picture_srcset = Some(set.to_string()); + } + out.push_str(raw); + } + "img" => { + let attrs = crate::epub::images::parse_attrs(inner); + if let Some(src) = best_img_src(&attrs, picture_srcset.as_deref()) { + out.push_str("<img src=\""); + out.push_str(&escape_attr(&src)); + out.push('"'); + for key in ["alt", "title"] { + if let Some(v) = attr(&attrs, key) { + out.push(' '); + out.push_str(key); + out.push_str("=\""); + out.push_str(&escape_attr(v)); + out.push('"'); + } + } + out.push_str("/>"); + } else { + tracing::debug!(tag = %&raw[..raw.len().min(120)], "dropping unusable img"); + } + } + _ => out.push_str(raw), + } + if name == "picture" && inner.starts_with('/') { + picture_srcset = None; + } + cursor = end; + } + out.push_str(&html[cursor..]); + out +} + +fn attr<'a>(attrs: &'a [(String, String)], name: &str) -> Option<&'a str> { + attrs + .iter() + .find(|(k, _)| k == name) + .map(|(_, v)| v.trim()) + .filter(|v| !v.is_empty()) +} + +/// Pick the best URL for one `<img>` from everything the element carries. +fn best_img_src(attrs: &[(String, String)], picture_srcset: Option<&str>) -> Option<String> { + for key in LAZY_SRC_ATTRS { + if let Some(v) = attr(attrs, key).filter(|s| plausible_url(s)) { + return Some(v.to_string()); + } + } + if let Some(src) = attr(attrs, "src").filter(|s| plausible_url(s)) { + return Some(src.to_string()); + } + if let Some(from_set) = attr(attrs, "srcset").and_then(best_from_srcset) { + return Some(from_set); + } + if let Some(from_set) = attr(attrs, "data-srcset").and_then(best_from_srcset) { + return Some(from_set); + } + picture_srcset.and_then(best_from_srcset) +} + +/// The widest candidate in a `srcset` that is still worth downloading. +/// +/// Parsed by whitespace rather than by comma: the URLs of several image CDNs +/// contain commas of their own, and splitting on those shreds them. +fn best_from_srcset(srcset: &str) -> Option<String> { + let mut best: Option<(u32, String)> = None; + let mut smallest: Option<(u32, String)> = None; + let mut pending: Option<String> = None; + + let mut consider = |url: String, width: u32| { + if width <= MAX_SRCSET_WIDTH && best.as_ref().is_none_or(|(w, _)| width > *w) { + best = Some((width, url.clone())); + } + if smallest.as_ref().is_none_or(|(w, _)| width < *w) { + smallest = Some((width, url)); + } + }; + + for token in srcset.split_whitespace() { + let token = token.trim_end_matches(','); + if token.is_empty() { + continue; + } + match parse_descriptor(token) { + Some(width) => { + if let Some(url) = pending.take() { + consider(url, width); + } + } + None => { + // A URL with no descriptor of its own still counts, at width 1x. + if let Some(url) = pending.replace(token.to_string()) { + consider(url, 1); + } + } + } + } + if let Some(url) = pending.take() { + consider(url, 1); + } + + best.or(smallest) + .map(|(_, url)| url) + .filter(|u| plausible_url(u)) +} + +/// `800w` → 800, `2x` → a synthetic width so density candidates sort sensibly. +fn parse_descriptor(token: &str) -> Option<u32> { + let (value, unit) = token.split_at(token.len().checked_sub(1)?); + match unit { + "w" => value.parse::<u32>().ok(), + "x" => value + .parse::<f32>() + .ok() + .map(|d| (d * 1000.0).round().clamp(1.0, f32::from(u16::MAX)) as u32), + _ => None, + } +} + +/// Whether a string can serve as an image URL at all. +/// +/// This is deliberately about shape, not about the host: whitespace, braces and +/// quotes mean we are looking at a `srcset` blob, an unfilled URL template or a +/// serialized object, none of which will ever resolve. +fn plausible_url(candidate: &str) -> bool { + let candidate = candidate.trim(); + if candidate.is_empty() || candidate.len() > 2048 { + return false; + } + if candidate.starts_with("data:") || candidate.starts_with("about:") { + return false; + } + if candidate + .chars() + .any(|c| c.is_whitespace() || matches!(c, '{' | '}' | '"' | '\'' | '<' | '>' | '\\')) + { + return false; + } + // `%20` is a space that survived encoding — same blob, different spelling. + !candidate.contains("%20") +} + +fn escape_attr(s: &str) -> String { + s.replace('&', "&") + .replace('<', "<") + .replace('>', ">") + .replace('"', """) +} + // --------------------------------------------------------------------------- // Sanitization (§3.3) // --------------------------------------------------------------------------- @@ -404,8 +744,10 @@ pub fn word_count(html: &str) -> i64 { .count() as i64 } -/// Absolute image URLs referenced by `html`, resolved against `base_url`, -/// capped at [`MAX_IMAGES_PER_ARTICLE`] (§3.3). +/// Absolute image URLs referenced by `html`, resolved against `base_url` (§3.3). +/// +/// Every image an article carries is kept: a photo essay with thirty pictures is +/// a photo essay, and the issue-wide byte budget is the real backstop. pub fn collect_image_urls(html: &str, base_url: &str) -> Vec<String> { let Ok(selector) = Selector::parse("img") else { return Vec::new(); @@ -433,9 +775,6 @@ pub fn collect_image_urls(html: &str, base_url: &str) -> Vec<String> { if seen.insert(url.clone()) { out.push(url); } - if out.len() >= MAX_IMAGES_PER_ARTICLE { - break; - } } out } @@ -598,14 +937,15 @@ mod tests { } #[test] - fn image_collection_resolves_and_caps() { + fn image_collection_resolves_and_keeps_every_image() { let mut html = String::from(r#"<img src="/a.png"><img src="https://cdn.dev/b.png">"#); html.push_str(r#"<img data-src="c.png"><img src="/a.png"><img src="data:image/png;x">"#); for i in 0..20 { html.push_str(&format!(r#"<img src="/n{i}.png">"#)); } let urls = collect_image_urls(&html, "https://blog.dev/posts/one"); - assert_eq!(urls.len(), MAX_IMAGES_PER_ARTICLE); + // Two named images, the data-src one, and all twenty of the rest: no cap. + assert_eq!(urls.len(), 23); assert_eq!(urls[0], "https://blog.dev/a.png"); assert_eq!(urls[1], "https://cdn.dev/b.png"); assert_eq!(urls[2], "https://blog.dev/posts/c.png"); @@ -615,6 +955,221 @@ mod tests { assert!(collect_image_urls("<p>none</p>", "https://blog.dev").is_empty()); } + // ----------------------------------------------------------------------- + // Image normalization — each case is a page shape seen in a real issue. + // ----------------------------------------------------------------------- + + fn src_of(html: &str) -> Vec<String> { + Html::parse_fragment(html) + .select(&Selector::parse("img").unwrap()) + .filter_map(|e| e.value().attr("src").map(str::to_string)) + .collect() + } + + #[test] + fn lazy_placeholder_src_gives_way_to_the_real_url() { + // IEEE Spectrum: an inline SVG spacer with the URL parked on data-runner-src. + let html = r#"<img alt="A well head" lazy-loadable="true" + src="data:image/svg+xml,%3Csvg%20xmlns=%27http://www.w3.org/2000/svg%27%3E%3C/svg%3E" + data-runner-src="https://spectrum.ieee.org/media-library/well.jpg?id=675&width=980"/>"#; + assert_eq!( + src_of(&normalize_img_tags(html)), + ["https://spectrum.ieee.org/media-library/well.jpg?id=675&width=980"] + ); + } + + #[test] + fn unresolved_url_templates_fall_through_to_a_real_candidate() { + // NPR: readability copies data-template over a perfectly good src. + let html = r#"<img alt="Meghan Cliffel" + src="https://npr.brightspotcdn.com/resize/{width}/quality/{quality}/x.jpg" + srcset="https://npr.brightspotcdn.com/resize/1100/quality/50/x.jpg 1100w"/>"#; + assert_eq!( + src_of(&normalize_img_tags(html)), + ["https://npr.brightspotcdn.com/resize/1100/quality/50/x.jpg"] + ); + // With nothing usable anywhere, the image goes rather than becoming a + // request for a picture that says "Image". + let only_template = + r#"<img alt="x" src="https://cdn.dev/resize/{width}/quality/{quality}/x.jpg"/>"#; + assert!(src_of(&normalize_img_tags(only_template)).is_empty()); + } + + #[test] + fn a_srcset_blob_parked_in_src_is_rejected_and_reparsed() { + // dfarq: readability copies the entire srcset string into src. + let blob = "https://i0.wp.com/x.jpg?resize=300%2C158&ssl=1 300w, \ + https://i0.wp.com/x.jpg?resize=1024%2C540&ssl=1 1024w, \ + https://i0.wp.com/x.jpg?w=3000&ssl=1 3000w"; + let html = format!(r#"<img alt="printer" src="{blob}" srcset="{blob}"/>"#); + // The widest candidate under the download ceiling wins; 3000w does not. + assert_eq!( + src_of(&normalize_img_tags(&html)), + ["https://i0.wp.com/x.jpg?resize=1024%2C540&ssl=1"] + ); + } + + #[test] + fn a_json_blob_parked_in_src_is_rejected() { + // Substack: readability copies data-attrs (JSON) over src. + let html = r#"<img alt="" src="{"src":"https://s3.dev/a.jpeg","width":1000}" + srcset="https://substackcdn.com/image/fetch/$s_!y1,w_1456,c_limit/a.jpeg 1456w"/>"#; + assert_eq!( + src_of(&normalize_img_tags(html)), + ["https://substackcdn.com/image/fetch/$s_!y1,w_1456,c_limit/a.jpeg"] + ); + } + + #[test] + fn picture_sources_back_up_an_empty_img() { + let html = r#"<picture> + <source srcset="https://cdn.dev/a.avif 800w" type="image/avif"/> + <source srcset="https://cdn.dev/a.webp 800w" type="image/webp"/> + <img alt="A photo"/> + </picture>"#; + assert_eq!( + src_of(&normalize_img_tags(html)), + ["https://cdn.dev/a.avif"] + ); + } + + #[test] + fn a_good_src_is_never_traded_for_a_metadata_attribute() { + // `data-template`, `data-attrs` and friends hold templates and JSON, not + // URLs — trading a working src for one of those is the original sin. + let html = r#"<img alt="A photo" loading="lazy" class="lazyload" + src="https://cdn.dev/real.jpg" + data-template="https://cdn.dev/{width}/real.jpg" + data-orig-file="https://cdn.dev/orig.jpg"/>"#; + assert_eq!( + src_of(&normalize_img_tags(html)), + ["https://cdn.dev/real.jpg"] + ); + } + + #[test] + fn a_lazy_loader_placeholder_loses_to_its_data_src() { + // iRunFar (a3-lazy-load): src is a shared 1×1 spacer that would download + // and re-encode perfectly happily, and be the wrong picture. + let html = r#"<img class="lazy lazy-hidden" alt="Brooks Cascadia 20" + src="//www.irunfar.com/wp-content/plugins/a3-lazy-load/assets/images/lazy_placeholder.gif" + data-src="https://s3.amazonaws.com/www.irunfar.com/uploads/Brooks-Cascadia-20.jpg"/>"#; + assert_eq!( + src_of(&normalize_img_tags(html)), + ["https://s3.amazonaws.com/www.irunfar.com/uploads/Brooks-Cascadia-20.jpg"] + ); + } + + #[test] + fn normalization_keeps_alt_and_title_and_drops_the_rest() { + let html = r#"<img src="/a.png" alt="An & alt" title="T" class="x" width="900" + onerror="evil()" srcset="/b.png 2x"/>"#; + let out = normalize_img_tags(html); + assert!(out.contains(r#"alt="An & alt""#), "{out}"); + assert!(out.contains(r#"title="T""#), "{out}"); + for gone in ["class=", "width=", "onerror", "srcset="] { + assert!( + !out.contains(gone), + "expected {gone} to be dropped from {out}" + ); + } + // Relative URLs survive: sanitize_with_base absolutizes them later. + assert_eq!(src_of(&out), ["/a.png"]); + } + + #[test] + fn lightbox_buttons_no_longer_take_their_image_with_them() { + // Nautilus: readability deletes <button> and everything inside it. + let html = r#"<figure class="wp-block-image"> + <button type="button" aria-label="Enlarge image"> + <img class="wp-image-1" src="https://cdn.dev/flower.png?w=710" alt=""/> + </button> + <figcaption>BEAUTIFUL DANGER: a belladonna flower.</figcaption> + </figure>"#; + let out = unwrap_image_wrappers(html); + assert!(!out.contains("<button"), "{out}"); + assert!(!out.contains("</button>"), "{out}"); + assert!(out.contains("flower.png"), "{out}"); + assert!(out.contains("BEAUTIFUL DANGER"), "caption survives: {out}"); + } + + #[test] + fn buttons_that_are_really_buttons_are_left_alone() { + let html = r#"<button class="subscribe">Subscribe <img src="/icon.png" alt=""/></button>"#; + assert_eq!(unwrap_image_wrappers(html), html); + // And an image-free control is untouched too. + let plain = r#"<button>Share</button>"#; + assert_eq!(unwrap_image_wrappers(plain), plain); + } + + #[test] + fn nested_and_unclosed_wrappers_do_not_derail_the_scan() { + let nested = r#"<button><button><img src="/a.png"/></button></button><p>after</p>"#; + let out = unwrap_image_wrappers(nested); + assert!(!out.contains("button"), "{out}"); + assert!(out.contains("/a.png") && out.contains("after"), "{out}"); + // An open tag that never closes is passed through rather than eating + // the rest of the document. + let unclosed = r#"<button><img src="/a.png"/><p>rest</p>"#; + assert!(unwrap_image_wrappers(unclosed).contains("rest")); + } + + #[test] + fn srcset_descriptors_pick_the_widest_usable_candidate() { + assert_eq!( + best_from_srcset("/a.png 150w, /b.png 800w, /c.png 4000w").as_deref(), + Some("/b.png") + ); + // Density descriptors work as an ordering too. + assert_eq!( + best_from_srcset("/a.png 1x, /b.png 2x").as_deref(), + Some("/b.png") + ); + // A bare URL with no descriptor is still a candidate. + assert_eq!(best_from_srcset("/only.png").as_deref(), Some("/only.png")); + // Every candidate too wide: take the narrowest rather than nothing. + assert_eq!( + best_from_srcset("/big.png 3000w, /huge.png 5000w").as_deref(), + Some("/big.png") + ); + assert_eq!(best_from_srcset(" ").as_deref(), None); + } + + #[test] + fn plausibility_is_about_shape_not_host() { + for good in [ + "https://cdn.dev/a.jpg?id=1&width=980", + "/_next/image?url=https%3A%2F%2Fx.dev%2Fa.png&w=3840&q=75", + "https://substackcdn.com/image/fetch/$s_!y1,w_1456/a.jpeg", + ] { + assert!(plausible_url(good), "{good} should be usable"); + } + for bad in [ + "", + "data:image/svg+xml,%3Csvg%3E%3C/svg%3E", + "https://cdn.dev/resize/{width}/a.jpg", + r#"{"src":"https://cdn.dev/a.jpg"}"#, + "https://cdn.dev/a.jpg 300w, https://cdn.dev/b.jpg 600w", + "https://cdn.dev/a.jpg%20300w", + ] { + assert!(!plausible_url(bad), "{bad} should be rejected"); + } + } + + #[tokio::test] + async fn feed_content_is_normalized_too() { + // Feeds carry the same lazy markup pages do. + let extractor = Extractor::offline(vec![]); + let body = format!( + r#"<p>{}</p><img src="data:image/gif;base64,zz" data-src="https://cdn.dev/real.jpg"/>"#, + "lorem ".repeat(400) + ); + let article = article("https://blog.dev/p", &body); + let out = extractor.extract(&article).await; + assert_eq!(out.method, ExtractMethod::Miniflux); + assert_eq!(out.image_urls, ["https://cdn.dev/real.jpg"]); + } + #[test] fn paywall_heuristic() { let domains = merge_paywall_domains(vec!["paywalled.dev".into()]); @@ -685,6 +1240,41 @@ mod tests { assert!(articles[1].excerpt_only); } + /// A redirected article's relative image URLs belong to where it landed. + /// + /// `postgr.es/p/9sl` redirects to `boringsql.com`; resolving its `/images/…` + /// against the shortener produced two 404s and two lost charts. + #[tokio::test] + async fn relative_urls_resolve_against_the_url_we_landed_on() { + use axum::response::{Html, Redirect}; + use axum::routing::get; + + let page = format!( + "<html><body><article><h1>Post</h1>{}\ + <img src=\"images/chart.svg\" alt=\"A chart\"/></article></body></html>", + "<p>Body copy that readability will happily keep. </p>".repeat(40) + ); + let app = axum::Router::new() + .route("/p/9sl", get(|| async { Redirect::to("/posts/real/") })) + .route("/posts/real/", get(move || async move { Html(page) })); + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap(); + let server = tokio::spawn(async move { axum::serve(listener, app).await }); + + let http = crate::http::build_client(Duration::from_secs(5)).unwrap(); + let extractor = Extractor::new(http, vec![]); + let article = article(&format!("http://{addr}/p/9sl"), "<p>stub</p>"); + let out = extractor.extract(&article).await; + server.abort(); + + assert_eq!(out.method, ExtractMethod::Readability); + assert_eq!( + out.image_urls, + [format!("http://{addr}/posts/real/images/chart.svg")], + "the shortener path must not be the base" + ); + } + #[tokio::test] async fn offline_extractor_never_fetches() { let extractor = Extractor::offline(vec![]);