HTML import: pictures referenced by a relative path are embedded (#98) (#113)

* HTML import: pictures referenced by a relative path are embedded (#98)

An img whose src was a path next to the HTML file was dropped, leaving
only its alt text; only data: URIs loaded. The HTML importer now takes
an optional image loader (the formats crate stays free of file access
and wasm-safe), and opening an HTML file from disk loads relative
sources from the file's folder. URLs and absolute paths are not
followed, and files that aren't pictures are ignored.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* HTML import: relative pictures stay in the folder, read only regular files, within a budget (#98)

Review follow-up for #113. Pictures named by a relative path are now
loaded by LocalImages:

- Only regular files are read. The path is checked before opening (a
  named pipe would block the open), the opened handle is checked again,
  and the read goes through take(limit + 1), so the per-picture limit
  holds even for files that report a length of 0, like /dev/zero.
- The path is canonicalized and must stay inside the HTML file's
  folder, so `..` and symlinks can't reach other files.
- Each file is read once and shared between references, and every
  reference counts against a 1 GB budget for the document, so one large
  picture referenced many times can't make the import read or copy
  without bound. Bytes read from oversized files count too.

The image loader now returns Arc<Vec<u8>>, and make_img takes either a
Vec or an Arc.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
Co-authored-by: lloyd <lloyd@stackworx.io>
Co-authored-by: Brandon Thomas <echelon@gmail.com>

crates/engine/src/io.rs

@@ -53,7 +53,118 @@ pub fn open_path(path: &std::path::Path) -> Result<Document, String> {
         return Err(format!("{}: file is larger than 2 GB", path.display()));
     }
     let bytes = std::fs::read(path).map_err(|e| format!("{}: {e}", path.display()))?;
-    open_bytes(&path.to_string_lossy(), &bytes)
+    let name = path.to_string_lossy();
+    if matches!(ext_of(&name).as_str(), "html" | "htm" | "xhtml") {
+        // Pictures referenced by a relative path load from beside the HTML file.
+        let images = LocalImages::new(path.parent().unwrap_or(std::path::Path::new("")));
+        let mut doc = wordcraft_formats::html::import_with(&wordcraft_formats::html::decode(&bytes), &|src| images.load(src));
+        doc.ensure_nonempty();
+        return Ok(doc);
+    }
+    open_bytes(&name, &bytes)
+}
+
+/// The pictures an HTML file names by a path relative to its folder.
+///
+/// Only regular files inside the folder (after resolving `..` and symlinks) are read. URLs
+/// (`http:`, `file:`…) and absolute paths are not followed. Each file is read at most once and
+/// at most `max_file` bytes of it; every reference, cached or not, counts against a budget for
+/// the whole document, so one large picture can't be multiplied by referencing it many times.
+#[cfg(not(target_arch = "wasm32"))]
+pub(crate) struct LocalImages {
+    /// The folder, canonical; `None` when it can't be resolved (nothing loads then).
+    dir: Option<std::path::PathBuf>,
+    max_file: u64,
+    /// Bytes the document may still load.
+    left: std::cell::Cell<u64>,
+    cache: std::cell::RefCell<Vec<(std::path::PathBuf, std::sync::Arc<Vec<u8>>)>>,
+}
+
+#[cfg(not(target_arch = "wasm32"))]
+impl LocalImages {
+    /// Largest picture file read.
+    const MAX_FILE: u64 = 256 << 20;
+    /// Picture bytes one document may load, counting every reference.
+    const BUDGET: u64 = 1 << 30;
+    /// Files whose bytes are kept for further references.
+    const CACHE: usize = 64;
+
+    pub(crate) fn new(dir: &std::path::Path) -> LocalImages {
+        LocalImages::with_limits(dir, LocalImages::MAX_FILE, LocalImages::BUDGET)
+    }
+
+    pub(crate) fn with_limits(dir: &std::path::Path, max_file: u64, budget: u64) -> LocalImages {
+        let dir = if dir.as_os_str().is_empty() { std::path::Path::new(".") } else { dir };
+        LocalImages { dir: std::fs::canonicalize(dir).ok(), max_file, left: std::cell::Cell::new(budget), cache: Default::default() }
+    }
+
+    /// Bytes of the file `src` names, or `None` when it isn't a regular file inside the folder,
+    /// is too large, or the budget is spent.
+    pub(crate) fn load(&self, src: &str) -> Option<std::sync::Arc<Vec<u8>>> {
+        let path = self.resolve(src)?;
+        let cached = self.cache.borrow().iter().find(|(p, _)| *p == path).map(|(_, d)| d.clone());
+        if let Some(data) = cached {
+            self.charge(data.len())?;
+            return Some(data);
+        }
+        let limit = self.max_file.min(self.left.get());
+        let bytes = read_regular_file(&path, limit)?;
+        // What was read is spent even when it's over the limit.
+        self.charge(bytes.len())?;
+        if bytes.len() as u64 > limit {
+            return None;
+        }
+        let data = std::sync::Arc::new(bytes);
+        let mut cache = self.cache.borrow_mut();
+        if cache.len() < LocalImages::CACHE {
+            cache.push((path, data.clone()));
+        }
+        Some(data)
+    }
+
+    /// Take `n` bytes from the budget; `None` (and the budget emptied) when there aren't enough.
+    fn charge(&self, n: usize) -> Option<()> {
+        let left = self.left.get().checked_sub(n as u64);
+        self.left.set(left.unwrap_or(0));
+        left.map(|_| ())
+    }
+
+    /// The canonical path `src` names, if it's inside the folder.
+    fn resolve(&self, src: &str) -> Option<std::path::PathBuf> {
+        let src = src.split(['#', '?']).next().unwrap_or("");
+        if src.is_empty() || src.starts_with("//") || src.split_once(':').is_some_and(|(scheme, _)| !scheme.contains('/')) {
+            return None;
+        }
+        let rel = String::from_utf8(wordcraft_formats::model::percent_decode(src)).ok()?;
+        let rel = std::path::Path::new(&rel);
+        if rel.components().any(|c| matches!(c, std::path::Component::Prefix(_) | std::path::Component::RootDir)) {
+            return None;
+        }
+        let dir = self.dir.as_ref()?;
+        // Canonical paths have `..` and symlinks resolved, so this keeps both inside the folder.
+        let path = std::fs::canonicalize(dir.join(rel)).ok()?;
+        path.starts_with(dir).then_some(path)
+    }
+}
+
+/// Up to `limit + 1` bytes of the regular file at `path` (more than `limit` means too large), or
+/// `None` for anything else. A named pipe blocks when opened and a device reports a length of 0
+/// and may never end, so the path is checked before opening, the opened handle is checked again,
+/// and the read is bounded whatever length the file reports.
+#[cfg(not(target_arch = "wasm32"))]
+fn read_regular_file(path: &std::path::Path, limit: u64) -> Option<Vec<u8>> {
+    use std::io::Read;
+    if !std::fs::metadata(path).ok()?.is_file() {
+        return None;
+    }
+    let file = std::fs::File::open(path).ok()?;
+    let meta = file.metadata().ok()?;
+    if !meta.is_file() {
+        return None;
+    }
+    let mut out = Vec::with_capacity(usize::try_from(meta.len().min(limit)).unwrap_or(0));
+    file.take(limit.saturating_add(1)).read_to_end(&mut out).ok()?;
+    Some(out)
 }
 
 #[cfg(target_arch = "wasm32")]

crates/engine/src/tests.rs

@@ -632,6 +632,161 @@ fn caret_navigation() {
     assert_eq!(s.sel.focus.off, 10);
 }
 
+#[cfg(not(target_arch = "wasm32"))]
+#[test]
+fn html_pictures_load_relative_to_the_file() {
+    // Issue #98: `<img src="logo.png">` beside an HTML file is embedded when it is opened.
+    let dir = std::env::temp_dir().join(format!("wordcraft-html-img-{}", std::process::id()));
+    std::fs::create_dir_all(dir.join("img")).unwrap();
+    let mut png = Vec::new();
+    image::RgbaImage::from_pixel(4, 2, image::Rgba([10, 20, 30, 255]))
+        .write_to(&mut std::io::Cursor::new(&mut png), image::ImageFormat::Png)
+        .unwrap();
+    std::fs::write(dir.join("img/my logo.png"), &png).unwrap();
+    std::fs::write(dir.join("secret.txt"), b"not a picture").unwrap();
+    let html = r#"<p><img src="img/my%20logo.png" alt="Logo"></p><p><img src="secret.txt" alt="T"></p><p><img src="http://example.com/x.png" alt="Web"></p>"#;
+    std::fs::write(dir.join("page.html"), html).unwrap();
+    let doc = crate::io::open_path(&dir.join("page.html"));
+    let _ = std::fs::remove_dir_all(&dir);
+    let doc = doc.unwrap();
+    assert_eq!(doc.media.len(), 1);
+    let text = doc.plain_text(StoryRef::Body);
+    assert!(!text.contains("Logo") && text.contains('T') && text.contains("Web"), "{text:?}");
+}
+
+/// A fresh, empty scratch folder for one test.
+#[cfg(not(target_arch = "wasm32"))]
+fn scratch_dir(name: &str) -> std::path::PathBuf {
+    let dir = std::env::temp_dir().join(format!("wordcraft-{name}-{}", std::process::id()));
+    let _ = std::fs::remove_dir_all(&dir);
+    std::fs::create_dir_all(&dir).unwrap();
+    dir
+}
+
+#[cfg(not(target_arch = "wasm32"))]
+fn tiny_png() -> Vec<u8> {
+    let mut png = Vec::new();
+    image::RgbaImage::from_pixel(2, 2, image::Rgba([200, 30, 30, 255]))
+        .write_to(&mut std::io::Cursor::new(&mut png), image::ImageFormat::Png)
+        .unwrap();
+    png
+}
+
+#[cfg(not(target_arch = "wasm32"))]
+#[test]
+fn html_pictures_outside_the_folder_are_not_loaded() {
+    // #98 review: `..` must not reach files outside the HTML file's folder.
+    let root = scratch_dir("html-img-escape");
+    std::fs::create_dir_all(root.join("site/img")).unwrap();
+    std::fs::write(root.join("outside.png"), tiny_png()).unwrap();
+    std::fs::write(root.join("site/inside.png"), tiny_png()).unwrap();
+    let html = r#"<p><img src="../outside.png" alt="Up"></p><p><img src="img/../../outside.png" alt="Sneak"></p><p><img src="img/../inside.png" alt="In"></p>"#;
+    std::fs::write(root.join("site/page.html"), html).unwrap();
+    let doc = crate::io::open_path(&root.join("site/page.html"));
+    let _ = std::fs::remove_dir_all(&root);
+    let doc = doc.unwrap();
+    assert_eq!(doc.media.len(), 1, "only the picture inside the folder loads");
+    let text = doc.plain_text(StoryRef::Body);
+    assert!(text.contains("Up") && text.contains("Sneak") && !text.contains("In"), "{text:?}");
+}
+
+#[cfg(unix)]
+#[test]
+fn html_pictures_through_a_symlink_out_of_the_folder_are_not_loaded() {
+    // #98 review: a symlink inside the folder must not lead outside it.
+    let root = scratch_dir("html-img-symlink");
+    std::fs::create_dir_all(root.join("site")).unwrap();
+    std::fs::write(root.join("outside.png"), tiny_png()).unwrap();
+    std::os::unix::fs::symlink(root.join("outside.png"), root.join("site/link.png")).unwrap();
+    std::fs::write(root.join("site/page.html"), r#"<p><img src="link.png" alt="Link"></p>"#).unwrap();
+    let doc = crate::io::open_path(&root.join("site/page.html"));
+    let _ = std::fs::remove_dir_all(&root);
+    let doc = doc.unwrap();
+    assert!(doc.media.is_empty());
+    assert!(doc.plain_text(StoryRef::Body).contains("Link"));
+}
+
+#[cfg(unix)]
+#[test]
+fn html_picture_that_is_a_named_pipe_is_skipped_without_blocking() {
+    // #98 review: opening a FIFO blocks until a writer appears; it must never be opened.
+    let dir = scratch_dir("html-img-fifo");
+    let fifo = dir.join("pipe.png");
+    let made = std::process::Command::new("mkfifo").arg(&fifo).status();
+    if !made.is_ok_and(|s| s.success()) {
+        let _ = std::fs::remove_dir_all(&dir);
+        eprintln!("mkfifo unavailable; skipping");
+        return;
+    }
+    std::fs::write(dir.join("page.html"), r#"<p><img src="pipe.png" alt="Pipe"></p>"#).unwrap();
+    let (tx, rx) = std::sync::mpsc::channel();
+    let page = dir.join("page.html");
+    std::thread::spawn(move || {
+        let _ = tx.send(crate::io::open_path(&page));
+    });
+    let got = rx.recv_timeout(std::time::Duration::from_secs(10));
+    if got.is_err() {
+        // Unblock the stuck reader so the thread ends, then fail.
+        let _ = std::fs::OpenOptions::new().write(true).open(&fifo);
+    }
+    let _ = std::fs::remove_dir_all(&dir);
+    let doc = got.expect("opening the HTML file blocked on a named pipe").unwrap();
+    assert!(doc.media.is_empty());
+    assert!(doc.plain_text(StoryRef::Body).contains("Pipe"));
+}
+
+#[cfg(not(target_arch = "wasm32"))]
+#[test]
+fn html_picture_size_limit_holds_whatever_length_the_file_reports() {
+    use crate::io::LocalImages;
+    let dir = scratch_dir("html-img-limit");
+    std::fs::write(dir.join("ten.bin"), [7u8; 10]).unwrap();
+    std::fs::write(dir.join("eleven.bin"), [7u8; 11]).unwrap();
+    let images = LocalImages::with_limits(&dir, 10, 1000);
+    let ten = images.load("ten.bin").map(|d| d.len());
+    let eleven = images.load("eleven.bin");
+    let _ = std::fs::remove_dir_all(&dir);
+    assert_eq!(ten, Some(10));
+    assert!(eleven.is_none());
+    #[cfg(unix)]
+    {
+        // A device reports a length of 0 and never ends: it is not a regular file, and the read is
+        // bounded anyway.
+        let dev = LocalImages::with_limits(std::path::Path::new("/dev"), 1 << 20, 1 << 20);
+        assert!(dev.load("zero").is_none());
+        assert!(dev.load("null").is_none());
+    }
+}
+
+#[cfg(not(target_arch = "wasm32"))]
+#[test]
+fn html_pictures_share_one_read_and_a_per_document_budget() {
+    // #98 review: a picture referenced many times is read once, and every reference counts
+    // against the document's budget so a large picture can't be multiplied without bound.
+    use crate::io::LocalImages;
+    let dir = scratch_dir("html-img-budget");
+    let png = tiny_png();
+    let n = png.len() as u64;
+    std::fs::write(dir.join("a.png"), &png).unwrap();
+    let images = LocalImages::with_limits(&dir, 1 << 20, 3 * n);
+    let first = images.load("a.png").unwrap();
+    std::fs::write(dir.join("a.png"), b"changed on disk").unwrap();
+    let second = images.load("./a.png").unwrap();
+    assert!(std::sync::Arc::ptr_eq(&first, &second), "the second reference reuses the first read");
+    assert_eq!(*second, png);
+    assert!(images.load("a.png").is_some());
+    assert!(images.load("a.png").is_none(), "budget spent");
+    // Files over the per-picture limit still cost what was read.
+    std::fs::write(dir.join("big.bin"), vec![1u8; 100]).unwrap();
+    std::fs::write(dir.join("small.bin"), [1u8; 5]).unwrap();
+    let images = LocalImages::with_limits(&dir, 10, 25);
+    assert!(images.load("big.bin").is_none());
+    assert!(images.load("big.bin").is_none());
+    let small = images.load("small.bin");
+    let _ = std::fs::remove_dir_all(&dir);
+    assert!(small.is_none(), "two oversized reads used up the budget");
+}
+
 fn para_style(s: &Session, i: usize) -> Option<String> {
     s.doc.para_at(&Pos::body(i, 0)).and_then(|p| p.props.style.clone())
 }

crates/formats/src/html.rs

@@ -4,13 +4,15 @@
 //! HTML's implicit closes for `p`, `li`, `tr`, `td`/`th` and headings. It understands `p`, `div`,
 //! `h1`–`h6`, `b`/`strong`, `i`/`em`, `u`, `s`/`strike`/`del`, `sup`/`sub`, `code`/`kbd`, `a`
 //! (links and `id`/`name` anchors), `br`, `hr`, `ul`/`ol`/`li` (nested), `table`/`tr`/`td`/`th`
-//! (colspan/rowspan), `img` with `data:` URIs, `blockquote`, `pre`, `font`, and the CSS
+//! (colspan/rowspan), `img` (`data:` URIs, other sources through a caller's [`ImageLoader`]), `blockquote`, `pre`, `font`, and the CSS
 //! properties `color`, `background-color`, `font-weight`, `font-style`, `font-size`,
 //! `font-family`, `text-decoration`, `vertical-align`, `text-align`, `white-space` and page
 //! breaks. Scripts and styles are skipped; nesting is capped.
 //!
 //! Export writes clean semantic HTML with inline CSS and pictures as `data:` URIs.
 
+use std::sync::Arc;
+
 use wordcraft_doc::{Align, Document};
 
 use crate::model::{
@@ -362,7 +364,13 @@ fn declares_border(attrs: &[(String, String)], is_table: bool) -> bool {
     })
 }
 
-struct Builder {
+/// Loads the bytes of an image an `img` names by a source other than a `data:` URI (a path
+/// relative to the HTML file). `None` when it can't be found. A picture used more than once can
+/// share one buffer.
+pub type ImageLoader<'a> = &'a dyn Fn(&str) -> Option<Arc<Vec<u8>>>;
+
+struct Builder<'r> {
+    images: ImageLoader<'r>,
     containers: Vec<Vec<FBlock>>,
     cells: Vec<Cell>,
     tables: Vec<TableB>,
@@ -552,9 +560,10 @@ fn attr<'a>(attrs: &'a [(String, String)], name: &str) -> Option<&'a str> {
     attrs.iter().find(|(k, _)| k == name).map(|(_, v)| v.as_str())
 }
 
-impl Builder {
-    fn new() -> Builder {
+impl<'r> Builder<'r> {
+    fn new(images: ImageLoader<'r>) -> Builder<'r> {
         Builder {
+            images,
             containers: vec![Vec::new()],
             cells: Vec::new(),
             tables: Vec::new(),
@@ -906,7 +915,9 @@ impl Builder {
                         }
                     }
                 }
-                let img = attr(attrs, "src").and_then(data_uri).and_then(|d| make_img(d, w, h, &alt));
+                let img = attr(attrs, "src")
+                    .and_then(|src| if src.trim_start().starts_with("data:") { data_uri(src).map(Arc::new) } else { (self.images)(src.trim()) })
+                    .and_then(|d| make_img(d, w, h, &alt));
                 let f = self.fmt();
                 match img {
                     Some(im) => self.start_para().inlines.push(Inline::Image(im)),
@@ -1068,8 +1079,13 @@ impl Builder {
 
 /// Parse HTML text into the flow model.
 pub fn parse(s: &str) -> Flow {
+    parse_with(s, &|_| None)
+}
+
+/// [`parse`], loading pictures that aren't `data:` URIs through `images`.
+pub fn parse_with(s: &str, images: ImageLoader) -> Flow {
     let mut lx = Lexer { s, pos: 0 };
-    let mut b = Builder::new();
+    let mut b = Builder::new(images);
     let mut guard = 0usize;
     while let Some(t) = lx.next() {
         guard += 1;
@@ -1098,6 +1114,11 @@ pub fn import(s: &str) -> Document {
     model::to_doc(&parse(s))
 }
 
+/// [`import`], loading pictures that aren't `data:` URIs through `images`.
+pub fn import_with(s: &str, images: ImageLoader) -> Document {
+    model::to_doc(&parse_with(s, images))
+}
+
 // ---------------------------------------------------------------------------------------------
 // Export
 

crates/formats/src/model.rs

@@ -795,7 +795,8 @@ pub fn image_px(data: &[u8]) -> Option<(u32, u32)> {
 
 /// A picture from bytes with an optional display size (points); the natural size (96 dpi) is
 /// used otherwise, shrunk to fit a 6.5" column.
-pub fn make_img(data: Vec<u8>, w: Option<f32>, h: Option<f32>, alt: &str) -> Option<Img> {
+pub fn make_img(data: impl Into<Arc<Vec<u8>>>, w: Option<f32>, h: Option<f32>, alt: &str) -> Option<Img> {
+    let data = data.into();
     let ext = sniff_image(&data)?;
     let (pw, ph) = image_px(&data).map(|(a, b)| (a as f32 * 0.75, b as f32 * 0.75)).unwrap_or((72.0, 72.0));
     let (pw, ph) = (pw.max(1.0), ph.max(1.0));
@@ -809,7 +810,7 @@ pub fn make_img(data: Vec<u8>, w: Option<f32>, h: Option<f32>, alt: &str) -> Opt
         h *= 468.0 / w;
         w = 468.0;
     }
-    Some(Img { data: Arc::new(data), ext: ext.to_string(), w: w.clamp(1.0, 1584.0), h: h.clamp(1.0, 1584.0), alt: alt.to_string() })
+    Some(Img { data, ext: ext.to_string(), w: w.clamp(1.0, 1584.0), h: h.clamp(1.0, 1584.0), alt: alt.to_string() })
 }
 
 const B64: &[u8; 64] = b"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/";

crates/formats/src/tests.rs

@@ -520,3 +520,21 @@ proptest::proptest! {
         proptest::prop_assert_eq!(got.trim(), text.trim(), "md: {}", String::from_utf8_lossy(&md));
     }
 }
+
+#[test]
+fn html_img_sources_go_through_the_loader() {
+    // Issue #98: an `img` named by a relative path was dropped, leaving only its alt text.
+    let html = r#"<p><img src="logo.png" alt="Sample Logo" width="120" height="40"></p><p><img src="gone.png" alt="Missing"></p><p>Text after.</p>"#;
+    let pic = png();
+    let seen = std::cell::RefCell::new(Vec::new());
+    let d = crate::html::import_with(html, &|src| {
+        seen.borrow_mut().push(src.to_string());
+        (src == "logo.png").then(|| std::sync::Arc::new(pic.clone()))
+    });
+    assert_eq!(*seen.borrow(), ["logo.png", "gone.png"]);
+    assert_eq!(d.media.len(), 1);
+    let text = d.plain_text(wordcraft_doc::StoryRef::Body);
+    assert!(text.contains("Missing") && !text.contains("Sample Logo"), "{text:?}");
+    // Without a loader only data: URIs load.
+    assert!(import("html", html.as_bytes()).unwrap().unwrap().media.is_empty());
+}