fix: bound document nesting and escape generated urls

core/src/error.rs

@@ -26,6 +26,11 @@ pub enum Error {
     /// name is a typo or a missing cargo feature.
     UnknownFormat { listener: String, format: String, available: Vec<String> },
 
+    /// A document nests blocks or inlines deeper than the parser allows. Refused
+    /// rather than parsed, because every walk over the result recurses and a
+    /// stack overflow aborts the process rather than unwinding.
+    TooDeep { limit: usize },
+
     /// An include or ASCII-art target cannot be used. `path` is where the
     /// directive actually pointed, resolved, which is the thing an author needs
     /// to see when a relative target is wrong.
@@ -68,6 +73,9 @@ impl fmt::Display for Error {
             Error::DuplicateHost { host, first, second } => {
                 write!(f, "host '{host}' is claimed by both site '{first}' and site '{second}'")
             }
+            Error::TooDeep { limit } => {
+                write!(f, "document nests more than {limit} levels deep")
+            }
             Error::Include { path, reason } => {
                 write!(f, "include target {} {reason}", path.display())
             }

core/src/parse.rs

@@ -25,19 +25,38 @@ use crate::preprocess;
 /// `source` is expected to be canonical, as [`crate::site::Site`] passes it.
 pub fn document(source: &Path, root: &Path) -> Result<Doc, Error> {
     let expanded = preprocess::expand(source, root)?;
-    let mut doc = markdown(&expanded);
+    let mut doc = markdown(&expanded)?;
     directives::apply(&mut doc, source.parent().unwrap_or(root), root)?;
     Ok(doc)
 }
 
+/// Hard limit on how deeply blocks and inlines may nest.
+///
+/// Everything downstream of the parser walks [`Doc`] recursively: the directive
+/// pass, every renderer, and the derived `Drop` for [`Block`]. Unbounded nesting
+/// therefore overflows the stack, and a stack overflow aborts the process
+/// instead of unwinding, so the handler's `catch_unwind` cannot contain it and
+/// one pathological document would take every virtual host down with it.
+/// Capping once, here, is what keeps all of those walks safe. Real prose nests a
+/// handful of levels; ten is already unusual.
+const MAX_NESTING: usize = 100;
+
 /// Parse one run of Markdown.
-pub fn markdown(text: &str) -> Doc {
+///
+/// Fails only when the source nests deeper than [`MAX_NESTING`]; a document is
+/// refused whole rather than silently truncated to the limit.
+pub fn markdown(text: &str) -> Result<Doc, Error> {
     let options = Options::ENABLE_TABLES | Options::ENABLE_STRIKETHROUGH;
     let mut builder = Builder::default();
     for event in Parser::new_ext(text, options) {
         builder.handle(event);
+        // Checked per event, so the half-built document is never deeper than the
+        // limit and dropping it cannot overflow either.
+        if builder.too_deep {
+            return Err(Error::TooDeep { limit: MAX_NESTING });
+        }
     }
-    builder.finish()
+    Ok(builder.finish())
 }
 
 /// Data a tag needs when it closes, for the tags whose `TagEnd` carries none.
@@ -82,6 +101,9 @@ struct Builder {
     /// list item holds its text with no `Paragraph` around it, so inline events
     /// arrive with nothing open; without this they would be dropped.
     implicit: bool,
+    /// Set when a container would have nested past [`MAX_NESTING`]; the parse is
+    /// abandoned rather than the container being opened.
+    too_deep: bool,
     first_h1: Option<String>,
 }
 
@@ -96,6 +118,7 @@ impl Default for Builder {
             code: None,
             html: None,
             implicit: false,
+            too_deep: false,
             first_h1: None,
         }
     }
@@ -131,7 +154,17 @@ impl Builder {
         }
     }
 
+    /// Live nesting depth. Every open container holds a frame on one of these
+    /// stacks, so their total is what has to stay bounded.
+    fn depth(&self) -> usize {
+        self.blocks.len() + self.inlines.len() + self.lists.len() + self.tables.len()
+    }
+
     fn start(&mut self, tag: Tag<'_>) {
+        if self.depth() > MAX_NESTING {
+            self.too_deep = true;
+            return;
+        }
         // An implicit paragraph ends where the next block begins, so it has to
         // be closed before that block is added or the order would invert.
         if matches!(
@@ -374,7 +407,7 @@ mod tests {
     use super::*;
 
     fn blocks(text: &str) -> Vec<Block> {
-        markdown(text).blocks
+        markdown(text).unwrap().blocks
     }
 
     fn text(value: &str) -> Vec<Inline> {
@@ -404,10 +437,10 @@ mod tests {
 
     #[test]
     fn records_the_first_level_one_heading() {
-        let doc = markdown("## Second\n\n# First\n\n# Another\n");
+        let doc = markdown("## Second\n\n# First\n\n# Another\n").unwrap();
         assert_eq!(doc.first_h1.as_deref(), Some("First"));
         // A document with no h1 has nothing to derive a title from.
-        assert_eq!(markdown("## Only\n").first_h1, None);
+        assert_eq!(markdown("## Only\n").unwrap().first_h1, None);
     }
 
     #[test]
@@ -573,4 +606,57 @@ mod tests {
         };
         assert_eq!(inline[1], Inline::Html("<b>".into()));
     }
+
+    #[test]
+    fn nesting_past_the_cap_is_refused_rather_than_overflowing_the_stack() {
+        // Everything downstream of the parser walks the tree recursively, and a
+        // stack overflow aborts the process instead of unwinding, so this is the
+        // one place the depth can be bounded. Both of these used to abort.
+        let over = MAX_NESTING + 50;
+        let deep = [
+            ("bare quote markers", "> ".repeat(over)),
+            ("quoted text", format!("{}deep\n", "> ".repeat(over))),
+            ("nested lists", (0..over).map(|i| format!("{}- x\n", "  ".repeat(i))).collect()),
+        ];
+        for (what, source) in deep {
+            let err = markdown(&source).expect_err(what);
+            assert!(
+                matches!(err, Error::TooDeep { limit } if limit == MAX_NESTING),
+                "{what}: {err:?}"
+            );
+            assert_eq!(
+                err.to_string(),
+                format!("document nests more than {MAX_NESTING} levels deep")
+            );
+        }
+    }
+
+    #[test]
+    fn deeply_repeated_inline_markers_are_bounded_by_the_parser_itself() {
+        // Emphasis and link nesting cannot run away the way block containers can:
+        // pulldown-cmark pairs delimiters and refuses to nest links at all, so
+        // 150 of each yields 75 levels and 1. The cap still counts inline frames,
+        // but these sources are not what it is there for.
+        let n = 150;
+        assert!(markdown(&format!("{}x{}", "*".repeat(n), "*".repeat(n))).is_ok());
+        assert!(markdown(&format!("{}x{}", "[".repeat(n), "](/a)".repeat(n))).is_ok());
+    }
+
+    #[test]
+    fn nesting_within_the_cap_still_parses() {
+        // The limit has to be far above anything real prose does, or valid pages
+        // would start failing; ten levels is already unusual.
+        let doc = markdown(&format!("{}deep\n", "> ".repeat(20))).unwrap();
+        assert_eq!(doc.blocks.len(), 1);
+        let nested =
+            markdown(&(0..20).map(|i| format!("{}- x\n", "  ".repeat(i))).collect::<String>());
+        assert!(nested.is_ok());
+    }
+
+    #[test]
+    fn a_deeply_nested_document_is_refused_whole_not_truncated() {
+        // Silently cutting the document at the limit would serve a page missing
+        // most of its content with no indication anything was dropped.
+        assert!(markdown(&format!("# Title\n\n{}deep\n", "> ".repeat(MAX_NESTING + 1))).is_err());
+    }
 }

core/src/path.rs

@@ -48,9 +48,35 @@ pub fn url_for(root: &Path, source: &Path) -> Option<String> {
     let rel = source.strip_prefix(root).ok()?;
     if rel.file_name()? == "index.md" {
         let parent = rel.parent()?.to_str()?;
-        return Some(if parent.is_empty() { "/".to_string() } else { format!("/{parent}/") });
+        return Some(if parent.is_empty() {
+            "/".to_string()
+        } else {
+            format!("/{}/", encode_path(parent))
+        });
     }
-    Some(format!("/{}", rel.with_extension("").to_str()?))
+    Some(format!("/{}", encode_path(rel.with_extension("").to_str()?)))
+}
+
+/// Percent-encode a path for use in a URL, leaving `/` as the separator.
+///
+/// A file name may contain bytes that mean something else in a URL or in a
+/// response header, and this URL is emitted as a redirect target: `?` or `#`
+/// would truncate the path, and a CR LF pair would end the header, so a file
+/// named `x\r\nLocation: elsewhere.md` could inject a header of its own into
+/// every protocol's redirect. Escaping everything outside RFC 3986's unreserved
+/// set closes that off and incidentally makes spaces and non-ASCII names work.
+/// `clean_path` decodes on the way back in, so the URL still resolves.
+fn encode_path(path: &str) -> String {
+    let mut out = String::with_capacity(path.len());
+    for byte in path.bytes() {
+        match byte {
+            b'A'..=b'Z' | b'a'..=b'z' | b'0'..=b'9' | b'-' | b'.' | b'_' | b'~' | b'/' => {
+                out.push(byte as char)
+            }
+            other => out.push_str(&format!("%{other:02X}")),
+        }
+    }
+    out
 }
 
 /// Decode `%XX` escapes, leaving an invalid escape as the literal text it is.
@@ -194,4 +220,47 @@ mod tests {
     fn url_for_refuses_a_path_outside_the_root() {
         assert_eq!(url_for(Path::new("/srv/content"), Path::new("/etc/passwd")), None);
     }
+
+    #[test]
+    fn url_for_escapes_bytes_that_would_end_a_response_header() {
+        // A file really can be named this. The URL is emitted as a redirect
+        // target, so an unescaped CR LF here injects a header into the response
+        // of every protocol: `Location: /x` followed by an attacker's own line.
+        let root = PathBuf::from("/srv/content");
+        let url = url_for(&root, &root.join("x\r\nX-Injected: yes.md")).unwrap();
+        assert_eq!(url, "/x%0D%0AX-Injected%3A%20yes");
+        assert!(!url.contains('\r') && !url.contains('\n'));
+    }
+
+    #[test]
+    fn url_for_escapes_characters_that_would_truncate_the_path() {
+        let root = PathBuf::from("/srv/content");
+        // `?` and `#` would start a query or a fragment, losing the rest.
+        assert_eq!(url_for(&root, &root.join("a?b.md")).unwrap(), "/a%3Fb");
+        assert_eq!(url_for(&root, &root.join("a#b.md")).unwrap(), "/a%23b");
+        // A literal percent must not read as an escape on the way back in.
+        assert_eq!(url_for(&root, &root.join("100%.md")).unwrap(), "/100%25");
+        // A tab would forge a field separator in a Gopher menu line.
+        assert_eq!(url_for(&root, &root.join("a\tb.md")).unwrap(), "/a%09b");
+    }
+
+    #[test]
+    fn url_for_escapes_spaces_and_non_ascii_names() {
+        let root = PathBuf::from("/srv/content");
+        assert_eq!(url_for(&root, &root.join("my notes.md")).unwrap(), "/my%20notes");
+        assert_eq!(url_for(&root, &root.join("caf\u{e9}.md")).unwrap(), "/caf%C3%A9");
+        assert_eq!(url_for(&root, &root.join("d i r/index.md")).unwrap(), "/d%20i%20r/");
+    }
+
+    #[test]
+    fn an_encoded_url_still_resolves_back_to_the_same_path() {
+        // The round trip is what makes escaping safe: the redirect target a
+        // client is sent must clean back to the file it came from.
+        let root = PathBuf::from("/srv/content");
+        for name in ["my notes.md", "caf\u{e9}.md", "100%.md", "a?b.md", "a#b.md"] {
+            let url = url_for(&root, &root.join(name)).unwrap();
+            let stem = name.strip_suffix(".md").unwrap();
+            assert_eq!(clean_path(&url).as_deref(), Some(stem), "{name} via {url}");
+        }
+    }
 }

core/src/preprocess.rs

@@ -14,6 +14,7 @@
 
 use std::collections::BTreeSet;
 use std::fs;
+use std::io::Read;
 use std::path::{Path, PathBuf};
 
 use crate::error::{Error, IncludeReason};
@@ -65,8 +66,7 @@ fn expand_file(
     depth: usize,
     out: &mut Output,
 ) -> Result<(), Error> {
-    let text =
-        fs::read_to_string(path).map_err(|cause| Error::Io { path: path.to_path_buf(), cause })?;
+    let text = read_capped(path)?;
     let base = path.parent().unwrap_or(root);
 
     for line in text.lines() {
@@ -90,6 +90,36 @@ fn expand_file(
     Ok(())
 }
 
+/// Read a file, refusing one larger than the whole expansion may be.
+///
+/// `fs::read_to_string` allocates the entire file before anything can reject it,
+/// so one oversized document in the tree would cost its full size on every
+/// concurrent request -- with the default connection cap, 256 times over -- even
+/// though [`MAX_BYTES`] bounds what can actually be used. Nothing past that cap
+/// can reach the output, so reading past it is waste an attacker can amplify.
+fn read_capped(path: &Path) -> Result<String, Error> {
+    let too_large = || Error::Include { path: path.to_path_buf(), reason: IncludeReason::TooLarge };
+    let length =
+        fs::metadata(path).map_err(|cause| Error::Io { path: path.to_path_buf(), cause })?.len();
+    if length > MAX_BYTES as u64 {
+        return Err(too_large());
+    }
+
+    let file =
+        fs::File::open(path).map_err(|cause| Error::Io { path: path.to_path_buf(), cause })?;
+    let mut text = String::with_capacity(length as usize);
+    // Capped again while reading, because the file may have grown since the stat;
+    // this is the bound that actually holds. One byte past, so a file of exactly
+    // the cap still reads.
+    file.take(MAX_BYTES as u64 + 1)
+        .read_to_string(&mut text)
+        .map_err(|cause| Error::Io { path: path.to_path_buf(), cause })?;
+    if text.len() > MAX_BYTES {
+        return Err(too_large());
+    }
+    Ok(text)
+}
+
 /// Canonicalise a path and require it to be a regular file inside `root`.
 pub(crate) fn canonical_within(path: &Path, root: &Path) -> Result<PathBuf, Error> {
     let canonical = path
@@ -256,4 +286,24 @@ mod tests {
         tree.write("page.md", "![[part.md]]\n![[part.md]]\n");
         assert_eq!(tree.expand("page.md").unwrap(), "shared\nshared\n");
     }
+
+    #[test]
+    fn an_oversized_file_is_refused_without_being_read_into_memory() {
+        // A single document larger than the expansion cap used to be allocated in
+        // full before anything could reject it, once per concurrent request.
+        let dir = tempfile::tempdir().unwrap();
+        let big = dir.path().join("big.md");
+        fs::write(&big, "x".repeat(9 * 1024 * 1024)).unwrap();
+        let err = expand(&big, dir.path()).unwrap_err();
+        assert!(matches!(err, Error::Include { reason: IncludeReason::TooLarge, .. }), "{err:?}");
+    }
+
+    #[test]
+    fn a_file_at_exactly_the_cap_still_reads() {
+        let dir = tempfile::tempdir().unwrap();
+        let edge = dir.path().join("edge.md");
+        // The trailing newline each line gains puts the output at the cap exactly.
+        fs::write(&edge, "x".repeat(8 * 1024 * 1024 - 1)).unwrap();
+        assert!(expand(&edge, dir.path()).is_ok());
+    }
 }