From a8152a0e0650f94c3eb9a0cd2939a65df22a2bdd Mon Sep 17 00:00:00 2001 From: Mykhailo Chalyi Date: Tue, 27 Oct 2026 00:36:16 +0000 Subject: [PATCH] fix(render): escape markdown output and allowlist URL schemes Document text could emit raw HTML, links, emphasis, headings and code-fence breakouts, and javascript: links passed through unchanged (threat model T1). - New render/escape.rs: text escaping (inline syntax, line-start block markers), code fences/spans longer than inner backtick runs, link and image scheme allowlists, percent-encoded destinations, escaped titles - Structure cannot leave its container: blocks in inline context flattened, list bodies indented by marker width, blocks always start a new line, hard breaks never make blank lines, nested links render as text - DOCX and spreadsheet parsers no longer pre-escape table pipes; the IR holds raw text and the renderer escapes in table context - output_safety tests with a pulldown-cmark oracle and proptests (S14); cases found by a fuzzing pass against that oracle are regression tests --- AGENTS.md | 2 +- CHANGELOG.md | 13 +- Cargo.lock | 18 + Cargo.toml | 1 + crates/markkit/Cargo.toml | 1 + crates/markkit/src/docx.rs | 6 +- crates/markkit/src/render/escape.rs | 371 ++++++++ crates/markkit/src/render/markdown.rs | 406 +++++---- crates/markkit/src/render/mod.rs | 3 +- crates/markkit/src/spreadsheet.rs | 3 +- crates/markkit/tests/docx_integration.rs | 2 +- .../markkit/tests/fixtures/html/malformed.md | 4 +- crates/markkit/tests/html_integration.rs | 13 +- crates/markkit/tests/html_security.rs | 15 +- crates/markkit/tests/html_urls.rs | 11 +- .../tests/output_safety.proptest-regressions | 7 + crates/markkit/tests/output_safety.rs | 795 ++++++++++++++++++ knowledge/formats/html-extraction.md | 11 +- knowledge/foundations/ir.md | 10 +- knowledge/log.md | 2 +- knowledge/operations/testing.md | 1 + knowledge/security/threat-model.md | 38 +- 22 files changed, 1526 insertions(+), 207 deletions(-) create mode 100644 crates/markkit/src/render/escape.rs create mode 100644 crates/markkit/tests/output_safety.proptest-regressions create mode 100644 crates/markkit/tests/output_safety.rs diff --git a/AGENTS.md b/AGENTS.md index 6579a71..328972f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -30,7 +30,7 @@ Fix root cause. Unsure: read more code; if stuck, ask w/ short options. Unrecogn | formats/format-support | Formats, bytes-in API, detection order, DOCX + spreadsheet mapping | | integrations/tool-contract | `MarkkitTool` LLM tool (`tool` feature), Everruns toolkit contract | | security/security-model | One-page threat/invariant summary | -| security/threat-model | Threats T1-T6, mitigations, test map, resource limits, invariants S1-S12 | +| security/threat-model | Threats T1-T6, mitigations, test map, resource limits, invariants S1-S17 | | runtimes/python-package | PyO3 `markkit` module, maturin, PyPI | | runtimes/wasm-package | wasm-bindgen `@everruns/markkit-wasm`, browser/edge, build + tests | | runtimes/node-package | NAPI-RS `@everruns/markkit`, npm, binding parity table | diff --git a/CHANGELOG.md b/CHANGELOG.md index 8cf3d53..036b34b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,7 +22,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/). - Relative URL resolution: links, images and metadata URLs resolve against the new `MarkkitBuilder::base_url` (CLI `--base-url`) and the page's ``; `extract_url` uses the final URL after redirects. Absolute and fragment-only URLs are left unchanged. -- Math: `$tex$` and `$$` blocks from MathML (`application/x-tex` annotation), KaTeX and MathJax output. +- Math: `` $`tex`$ `` and ` ```math ` blocks from MathML (`application/x-tex` annotation), KaTeX and MathJax output. - IR transforms: `MarkkitBuilder::transform(..)` runs `Transform` passes after parsing and before rendering, for every format. Built-ins `transform::NormalizeHeadings` and `transform::DropImages`; `transform::from_fn` for closures; `Document::visit` / `visit_mut` to walk the tree. @@ -64,6 +64,17 @@ and this project adheres to [Semantic Versioning](https://semver.org/). (`max_redirects`, default 5), the body is bounded by `max_input_size` while streaming, and the whole fetch by `max_processing_secs`. New builder options `allow_private_networks(true)` and `allow_host(..)`; refusals return the new `Error::UrlBlocked`. +- Markdown output is now escaped: text from documents can no longer emit raw HTML (``), + links, emphasis, headings or list markers; code blocks use a fence longer than any backtick run + inside. Links with schemes other than `http`, `https`, `mailto`, `tel`, `ftp` or relative URLs + (e.g. `javascript:`) and images other than `http`/`https`/relative/raster `data:image/*` are + dropped, keeping their text. URLs percent-encode `( ) < > \` and spaces. +- Markdown structure from documents can no longer break out of its container: code blocks in table + cells and headings are flattened to one line, list item bodies are indented by the marker width + (nested content under `1.` items now nests correctly), and nested links render as text. +- The JSON IR of DOCX and spreadsheet tables now holds raw text (pipes were pre-escaped before). +- Math TeX (taken from page markup) renders as code, GitHub's `` $`tex`$ `` and ` ```math ` + forms, instead of `$tex$` / `$$`, so it can no longer emit raw HTML or links in plain GFM. ### Added diff --git a/Cargo.lock b/Cargo.lock index 4b8e4c9..1c3af4c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1197,6 +1197,7 @@ dependencies = [ "futures-core", "html5ever", "proptest", + "pulldown-cmark", "quick-xml", "reqwest", "schemars", @@ -1621,6 +1622,17 @@ dependencies = [ "unarray", ] +[[package]] +name = "pulldown-cmark" +version = "0.13.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "278e9268c93b9e498a2da46c34d90ba93a3255eb99009646398f34bc73ab36eb" +dependencies = [ + "bitflags", + "memchr", + "unicase", +] + [[package]] name = "pyo3" version = "0.30.1" @@ -2622,6 +2634,12 @@ version = "0.1.4" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "eaea85b334db583fe3274d12b4cd1880032beab409c0d774be044d4480ab9a94" +[[package]] +name = "unicase" +version = "2.10.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "357cc3acc6a036009fd6c973ed009037c732d60d0b4f6c673e9041497482a28f" + [[package]] name = "unicode-ident" version = "1.0.27" diff --git a/Cargo.toml b/Cargo.toml index ebd7d17..e0df15c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -73,6 +73,7 @@ napi-derive = "3" tempfile = "3" criterion = { version = "0.8", features = ["html_reports"] } proptest = "1" +pulldown-cmark = { version = "0.13", default-features = false } [profile.release] lto = "thin" diff --git a/crates/markkit/Cargo.toml b/crates/markkit/Cargo.toml index 6e90639..2d179f5 100644 --- a/crates/markkit/Cargo.toml +++ b/crates/markkit/Cargo.toml @@ -70,6 +70,7 @@ anyhow.workspace = true tokio = { workspace = true, features = ["macros", "rt-multi-thread", "net", "io-util", "time"] } tempfile.workspace = true proptest.workspace = true +pulldown-cmark.workspace = true criterion.workspace = true tower = { workspace = true } zip = { workspace = true } diff --git a/crates/markkit/src/docx.rs b/crates/markkit/src/docx.rs index 62f24dc..9d0ee82 100644 --- a/crates/markkit/src/docx.rs +++ b/crates/markkit/src/docx.rs @@ -25,7 +25,6 @@ use crate::config::MarkkitBuilder; use crate::error::Error; use crate::ir::{Document, ListItem, Node, Source, TableCell}; use crate::limits::Budget; -use crate::render::escape_table_cell; use crate::xml::{self, Element, XmlError}; /// Result of parsing a DOCX package. @@ -883,11 +882,10 @@ impl Ctx<'_> { } } -/// Make cell content table-safe: no line breaks, pipes escaped. +/// Make cell content single-line: hard breaks become spaces (the renderer escapes pipes). fn table_cell(nodes: Vec) -> Vec { fn fix(n: Node) -> Node { match n { - Node::Text(t) => Node::Text(escape_table_cell(&t)), Node::LineBreak => Node::Text(" ".into()), Node::Bold { children } => Node::Bold { children: children.into_iter().map(fix).collect(), @@ -903,7 +901,7 @@ fn table_cell(nodes: Vec) -> Vec { title, children, } => Node::Link { - url: url.replace('|', "%7C"), + url, title, children: children.into_iter().map(fix).collect(), }, diff --git a/crates/markkit/src/render/escape.rs b/crates/markkit/src/render/escape.rs new file mode 100644 index 0000000..aca047c --- /dev/null +++ b/crates/markkit/src/render/escape.rs @@ -0,0 +1,371 @@ +//! Output-safety escaping for the Markdown renderer (threat model T1). +//! +//! Decisions: +//! - Escaping happens only here, at render time. The IR always holds raw text, so JSON +//! IR consumers never see Markdown backslashes. +//! - Text is escaped so that a CommonMark/GFM renderer shows it verbatim: no raw HTML, +//! no emphasis, links, code or block structure can come from document text. +//! Characters that are inert in context stay unescaped to keep output readable for +//! people and LLMs (`snake_case`, `C:\dir`, mid-line `#`). +//! - Link and image destinations are filtered by scheme allowlist: `http`, `https`, +//! `mailto`, `tel`, `ftp` and scheme-less (relative, `#fragment`) URLs for links; +//! `http`, `https`, relative and raster `data:image/*` for images. Anything else is +//! dropped and its text (link children, image alt) kept. Scheme detection ignores +//! ASCII whitespace and control characters anywhere, as browsers do. +//! - Destinations are emitted bare with `( ) < > \ space` percent-encoded and entity-like +//! `&` backslash-escaped, so a destination can neither end early nor decode into +//! another scheme (`javascript:`). +//! - Fences are one backtick longer than the longest backtick run in the code. + +/// Where a text run starts, for block-marker escaping. +#[derive(Clone, Copy, PartialEq, Eq)] +pub(crate) enum Position { + LineStart, + Inline, +} + +/// Escape document text for inline Markdown. Returns whether the output still sits at +/// a line start (the run was empty or whitespace only). +pub(crate) fn push_text(buf: &mut String, s: &str, pos: Position) -> bool { + let chars: Vec = s + .chars() + .map(|c| if c == '\n' || c == '\r' { ' ' } else { c }) + .collect(); + let mut line_start = pos == Position::LineStart; + let mut i = 0; + while i < chars.len() { + let c = chars[i]; + if line_start { + if c == ' ' || c == '\t' { + buf.push(c); + i += 1; + continue; + } + line_start = false; + match c { + '#' | '>' | '-' | '+' | '=' => { + buf.push('\\'); + buf.push(c); + i += 1; + continue; + } + '0'..='9' => { + // Ordered list marker: up to 9 digits then `.` or `)`. + let digits = chars[i..].iter().take_while(|d| d.is_ascii_digit()).count(); + if digits <= 9 && matches!(chars.get(i + digits), Some('.') | Some(')')) { + buf.extend(&chars[i..i + digits]); + buf.push('\\'); + buf.push(chars[i + digits]); + i += digits + 1; + continue; + } + } + _ => {} + } + } + let prev = if i == 0 { None } else { Some(chars[i - 1]) }; + let next = chars.get(i + 1).copied(); + let escape = match c { + '`' | '*' | '[' | ']' | '<' | '~' | '|' => true, + // A trailing `\` or `!` could combine with the next node (`\**`, `![`). + '\\' | '!' => next.is_none_or(|n| n.is_ascii_punctuation()), + // Intraword `_` cannot open or close emphasis. + '_' => { + !(prev.is_some_and(char::is_alphanumeric) + && next.is_some_and(char::is_alphanumeric)) + } + // Entity or numeric character reference (or the start of one in the next node). + '&' => next.is_none() || entity_like(&chars[i..]), + _ => false, + }; + if escape { + buf.push('\\'); + } + buf.push(c); + i += 1; + } + line_start +} + +/// Push an inline code span that renders `code` verbatim. +pub(crate) fn push_code_span(buf: &mut String, code: &str, in_table: bool) { + let code: String = code + .chars() + .map(|c| if c == '\n' || c == '\r' { ' ' } else { c }) + .collect(); + if code.is_empty() { + return; + } + let fence = "`".repeat(longest_run(&code, '`') + 1); + // A backtick right before the fence would lengthen it and unbalance the span. + if buf.ends_with('`') { + buf.push(' '); + } + let pad = code.starts_with('`') + || code.ends_with('`') + || (code.starts_with(' ') && code.ends_with(' ') && !code.trim().is_empty()); + buf.push_str(&fence); + if pad { + buf.push(' '); + } + if in_table { + buf.push_str(&code.replace('|', "\\|")); + } else { + buf.push_str(&code); + } + if pad { + buf.push(' '); + } + buf.push_str(&fence); +} + +/// Opening/closing fence for a fenced code block holding `code`. +pub(crate) fn code_fence(code: &str) -> String { + "`".repeat((longest_run(code, '`') + 1).max(3)) +} + +/// Info string for a fenced code block: first word, no backticks or control chars. +pub(crate) fn info_string(lang: &str) -> String { + lang.split(|c: char| c.is_whitespace()) + .find(|w| !w.is_empty()) + .unwrap_or("") + .chars() + .filter(|c| *c != '`' && !c.is_control()) + .collect() +} + +/// `&name;`, `{` or `` at the start of `s` (which begins with `&`). +fn entity_like(s: &[char]) -> bool { + let rest = &s[1..]; + let (body, max): (&[char], usize) = match rest { + ['#', 'x' | 'X', tail @ ..] => (tail, 6), + ['#', tail @ ..] => (tail, 7), + tail => (tail, 32), + }; + let hex = matches!(rest, ['#', 'x' | 'X', ..]); + let numeric = matches!(rest, ['#', ..]); + let n = body + .iter() + .take_while(|c| { + if hex { + c.is_ascii_hexdigit() + } else if numeric { + c.is_ascii_digit() + } else { + c.is_ascii_alphanumeric() + } + }) + .count(); + n >= 1 && n <= max && body.get(n) == Some(&';') +} + +fn longest_run(s: &str, ch: char) -> usize { + let (mut best, mut cur) = (0, 0); + for c in s.chars() { + if c == ch { + cur += 1; + best = best.max(cur); + } else { + cur = 0; + } + } + best +} + +/// Kind of destination, which selects the scheme allowlist. +#[derive(Clone, Copy)] +pub(crate) enum UrlKind { + Link, + Image, +} + +/// Return a Markdown-safe destination for `url`, or `None` when its scheme is not +/// allowed and the link/image must be dropped. +pub(crate) fn safe_destination(url: &str, kind: UrlKind) -> Option { + let cleaned: String = url.chars().filter(|c| !c.is_ascii_control()).collect(); + let cleaned = cleaned.trim_matches(|c: char| c.is_ascii_whitespace()); + if !scheme_allowed(cleaned, kind) { + return None; + } + let mut out = String::with_capacity(cleaned.len()); + let chars: Vec = cleaned.chars().collect(); + for (i, &c) in chars.iter().enumerate() { + match c { + ' ' => out.push_str("%20"), + '(' => out.push_str("%28"), + ')' => out.push_str("%29"), + '<' => out.push_str("%3C"), + '>' => out.push_str("%3E"), + '\\' => out.push_str("%5C"), + '&' if entity_like(&chars[i..]) => out.push_str("\\&"), + c if c.is_whitespace() => { + let mut tmp = [0u8; 4]; + for b in c.encode_utf8(&mut tmp).bytes() { + out.push_str(&format!("%{b:02X}")); + } + } + c => out.push(c), + } + } + if out.is_empty() { + // A bare empty destination would let a following title parse as the URL. + out.push_str("<>"); + } + Some(out) +} + +fn scheme_allowed(url: &str, kind: UrlKind) -> bool { + // Browsers ignore tabs/newlines anywhere in a URL; so does scheme detection. + // The whole scheme is read (no length cap: a capped probe let long schemes + // pass as relative URLs). + let mut chars = url + .chars() + .filter(|c| !c.is_ascii_whitespace()) + .map(|c| c.to_ascii_lowercase()); + let mut scheme = String::new(); + loop { + match chars.next() { + Some(':') => break, + Some('/' | '?' | '#') | None => return true, // relative URL, fragment, or path + Some(c) => scheme.push(c), + } + } + let scheme = scheme.as_str(); + match kind { + UrlKind::Link => matches!(scheme, "http" | "https" | "mailto" | "tel" | "ftp"), + UrlKind::Image => match scheme { + "http" | "https" => true, + "data" => { + let mime: String = chars + .take_while(|c| *c != ';' && *c != ',') + .take(32) + .collect(); + let mime = mime.as_str(); + matches!( + mime, + "image/png" + | "image/jpeg" + | "image/jpg" + | "image/gif" + | "image/webp" + | "image/avif" + | "image/bmp" + ) + } + _ => false, + }, + } +} + +/// Escape a link title for use inside `"..."`. +pub(crate) fn link_title(t: &str) -> String { + let mut out = String::with_capacity(t.len()); + for c in t.chars() { + match c { + // `<` too: if the link ever fails to parse, the title must not + // become raw HTML. + '"' | '\\' | '<' => { + out.push('\\'); + out.push(c); + } + '\n' | '\r' => out.push(' '), + c if c.is_control() => {} + c => out.push(c), + } + } + out +} + +#[cfg(test)] +mod tests { + use super::*; + + fn esc(s: &str, pos: Position) -> String { + let mut b = String::new(); + push_text(&mut b, s, pos); + b + } + + #[test] + fn escapes_inline_syntax() { + assert_eq!( + esc("*a* `b` [c] ~e~ |f|", Position::Inline), + r"\*a\* \`b\` \[c\] \ \~e\~ \|f\|" + ); + assert_eq!(esc("a_b _c_", Position::Inline), r"a_b \_c\_"); + assert_eq!( + esc("& & x < < a&b", Position::Inline), + r"\& & x \< \< a&b" + ); + assert_eq!(esc("tail&", Position::Inline), r"tail\&"); + assert_eq!(esc("C:\\dir\\x", Position::Inline), r"C:\dir\x"); + assert_eq!(esc("end\\", Position::Inline), r"end\\"); + assert_eq!(esc("hi!", Position::Inline), r"hi\!"); + assert_eq!(esc("hi! yes", Position::Inline), "hi! yes"); + } + + #[test] + fn escapes_line_start_markers() { + assert_eq!(esc("# h", Position::LineStart), r"\# h"); + assert_eq!(esc(" - x", Position::LineStart), r" \- x"); + assert_eq!(esc("12. x", Position::LineStart), r"12\. x"); + assert_eq!(esc("1234567890. x", Position::LineStart), "1234567890. x"); + assert_eq!(esc("# h", Position::Inline), "# h"); + } + + #[test] + fn whitespace_only_stays_at_line_start() { + let mut b = String::new(); + assert!(push_text(&mut b, " ", Position::LineStart)); + assert!(!push_text(&mut b, "x", Position::LineStart)); + } + + #[test] + fn fences() { + assert_eq!(code_fence("a"), "```"); + assert_eq!(code_fence("````"), "`````"); + assert_eq!(info_string(" rust extra"), "rust"); + assert_eq!(info_string("js```"), "js"); + let mut b = String::new(); + push_code_span(&mut b, "a`b", false); + assert_eq!(b, "``a`b``"); + b.clear(); + push_code_span(&mut b, "`", false); + assert_eq!(b, "`` ` ``"); + } + + #[test] + fn destinations() { + assert_eq!(safe_destination("javascript:x", UrlKind::Link), None); + assert_eq!(safe_destination("JAVA\tSCRIPT:x", UrlKind::Link), None); + assert_eq!(safe_destination("\u{0}javascript:x", UrlKind::Link), None); + assert_eq!(safe_destination("data:text/html,x", UrlKind::Image), None); + assert_eq!( + safe_destination("data:image/svg+xml,x", UrlKind::Image), + None + ); + assert_eq!(safe_destination("mailto:a@b", UrlKind::Image), None); + assert_eq!(safe_destination("a:b/c", UrlKind::Link), None); + assert_eq!( + safe_destination("./a:b", UrlKind::Link).as_deref(), + Some("./a:b") + ); + assert_eq!( + safe_destination("/x?y=a:b", UrlKind::Link).as_deref(), + Some("/x?y=a:b") + ); + assert_eq!( + safe_destination("http://e/a b(c)", UrlKind::Link).as_deref(), + Some("http://e/a%20b%28c%29") + ); + assert_eq!( + safe_destination("http://e/?a=1&b=2:", UrlKind::Link).as_deref(), + Some("http://e/?a=1&b=2\\:") + ); + } + + #[test] + fn titles() { + assert_eq!(link_title("a \"b\" \\c "), r#"a \"b\" \\c \"#); + } +} diff --git a/crates/markkit/src/render/markdown.rs b/crates/markkit/src/render/markdown.rs index c985c55..b80d4d5 100644 --- a/crates/markkit/src/render/markdown.rs +++ b/crates/markkit/src/render/markdown.rs @@ -9,6 +9,11 @@ //! content goes in its top-left slot, covered slots stay empty. Layout is //! bounded by `MAX_TABLE_GRID_CELLS`; a table whose spans would exceed it is //! laid out ignoring spans (threat model T2, invariant S13). +//! - All text, code and URL escaping lives in [`super::escape`]; see its header +//! for the output-safety decisions (threat model T1). Structure cannot break out +//! of its container: blocks in inline context are flattened to one line, list +//! item bodies are indented by the marker width, every block starts on its own +//! line, and hard breaks never produce a blank line. //! - Output size and the deadline are checked per block, list item and table //! row (not per byte). Tables pad every row to the widest row, so output can //! be quadratic in input; checking per row keeps any buffer within @@ -17,6 +22,7 @@ use std::time::Instant; +use super::escape::{self, Position, UrlKind}; use crate::ir::{Alignment, DefinitionItem, Document, ListItem, Node, TableCell}; /// Grid budget for span layout (rows × columns). @@ -38,6 +44,13 @@ pub struct MarkdownRenderer { max_output: usize, deadline: Option, tripped: Option, + /// Next inline text starts a Markdown line (block markers must be escaped). + line_start: bool, + /// Rendering a table cell (pipes escaped, hard breaks flattened). + in_table: bool, + /// Rendering link text: nested links render as text (CommonMark drops the + /// outer link otherwise). + in_link: bool, } impl MarkdownRenderer { @@ -47,12 +60,18 @@ impl MarkdownRenderer { max_output: usize::MAX, deadline: None, tripped: None, + line_start: false, + in_table: false, + in_link: false, } } /// Render a document to a Markdown string (no limits; the IR is trusted). pub fn render(&mut self, doc: &Document) -> String { self.buf.clear(); + self.line_start = false; + self.in_table = false; + self.in_link = false; self.render_nodes(&doc.nodes, 0); normalize_output(&self.buf) } @@ -120,9 +139,18 @@ impl MarkdownRenderer { if self.over_limit() { return; } - if i > 0 && node.is_block() && nodes[i - 1].is_block() { + let block = node.is_block(); + if i > 0 && (block || nodes[i - 1].is_block()) { + // Every block starts on its own line after a blank line; loose + // inline text after a block starts a new paragraph. + if !self.buf.ends_with('\n') { + self.buf.push('\n'); + } self.buf.push('\n'); } + if !block && (self.buf.is_empty() || self.buf.ends_with('\n')) { + self.line_start = true; + } self.render_node(node, depth); } } @@ -136,6 +164,7 @@ impl MarkdownRenderer { self.buf.push_str(&prefix); self.buf.push(' '); let start = self.buf.len(); + self.line_start = true; self.render_inline(children); trim_trailing_inline(&mut self.buf, start); self.buf.push('\n'); @@ -143,6 +172,7 @@ impl MarkdownRenderer { Node::Paragraph { children, .. } => { let start = self.buf.len(); + self.line_start = true; self.render_inline(children); trim_trailing_inline(&mut self.buf, start); self.buf.push('\n'); @@ -154,7 +184,7 @@ impl MarkdownRenderer { items, .. } => { - self.render_list(*ordered, *start, items, depth); + self.render_list(*ordered, *start, items); } Node::BlockQuote { children, .. } => { @@ -176,16 +206,18 @@ impl MarkdownRenderer { } Node::CodeBlock { language, code, .. } => { - self.buf.push_str("```"); + let fence = escape::code_fence(code); + self.buf.push_str(&fence); if let Some(lang) = language { - self.buf.push_str(lang); + self.buf.push_str(&escape::info_string(lang)); } self.buf.push('\n'); self.buf.push_str(code); if !code.ends_with('\n') { self.buf.push('\n'); } - self.buf.push_str("```\n"); + self.buf.push_str(&fence); + self.buf.push('\n'); } Node::Table { @@ -218,15 +250,20 @@ impl MarkdownRenderer { } } + // TeX is untrusted page content and plain GFM has no math syntax, + // so math renders as code: a ```math fence (GitHub renders it as + // math), inert everywhere else. Node::MathBlock { tex, .. } => { - let tex = tex.trim().replace("$$", "$ $"); + let tex = tex.trim(); if !tex.is_empty() { - self.buf.push_str("$$\n"); - for line in tex.lines().filter(|l| !l.trim().is_empty()) { - self.buf.push_str(line); - self.buf.push('\n'); - } - self.buf.push_str("$$\n"); + self.render_node( + &Node::CodeBlock { + language: Some("math".into()), + code: tex.into(), + source: None, + }, + depth, + ); } } @@ -266,6 +303,7 @@ impl MarkdownRenderer { self.buf.push('\n'); } let start = self.buf.len(); + self.line_start = true; self.render_inline(&item.term); trim_trailing_inline(&mut self.buf, start); self.buf.push('\n'); @@ -277,88 +315,78 @@ impl MarkdownRenderer { } } - /// Render a list. - fn render_list(&mut self, ordered: bool, start: Option, items: &[ListItem], depth: usize) { - let indent = " ".repeat(depth); + /// Render a list. Each item body is rendered on its own and indented by + /// the marker width, so nested blocks stay inside the item for any marker. + fn render_list(&mut self, ordered: bool, start: Option, items: &[ListItem]) { let start_num = start.unwrap_or(1); - for (i, item) in items.iter().enumerate() { if self.over_limit() { return; } let marker = if ordered { - format!("{}{}. ", indent, start_num + i as u64) + format!("{}. ", start_num.saturating_add(i as u64)) } else { - format!("{}- ", indent) + "- ".to_string() + }; + let pad = " ".repeat(marker.len()); + let body = { + let mut sub = self.sub(); + sub.render_item_children(&item.children); + self.absorb(&sub); + normalize_output(&sub.buf) }; + let mut lines = body.lines(); self.buf.push_str(&marker); match item.checked { Some(true) => self.buf.push_str("[x] "), Some(false) => self.buf.push_str("[ ] "), None => {} } + self.buf.push_str(lines.next().unwrap_or("")); + self.buf.push('\n'); + for line in lines { + if !line.is_empty() { + self.buf.push_str(&pad); + } + self.buf.push_str(line); + self.buf.push('\n'); + } + } + } - // Render item children - let has_block = item.children.iter().any(Node::is_block); - if has_block { - // Multi-line item: render sub-blocks indented - let mut first = true; - for child in &item.children { - if first { - first = false; - match child { - Node::Paragraph { children, .. } => { - self.render_inline(children); - self.buf.push('\n'); - } - Node::List { - ordered: sub_ord, - start: sub_start, - items: sub_items, - .. - } => { - self.buf.push('\n'); - self.render_list(*sub_ord, *sub_start, sub_items, depth + 1); - } - _ => { - let mut sub = self.sub(); - sub.render_node(child, depth + 1); - self.absorb(&sub); - let text = sub.buf.trim_end().to_string(); - self.buf.push_str(&text); - self.buf.push('\n'); - } - } - } else { - match child { - Node::List { - ordered: sub_ord, - start: sub_start, - items: sub_items, - .. - } => { - self.render_list(*sub_ord, *sub_start, sub_items, depth + 1); - } - _ => { - let mut sub = self.sub(); - sub.render_node(child, depth + 1); - self.absorb(&sub); - let inner = sub.buf.trim_end().to_string(); - let sub_indent = " ".repeat(depth + 1); - for line in inner.lines() { - self.buf.push_str(&sub_indent); - self.buf.push_str(line); - self.buf.push('\n'); - } - } - } + /// Like [`Self::render_nodes`], but keeps the common "text, then nested list" + /// item tight (no blank line) when the list can interrupt a paragraph. + fn render_item_children(&mut self, nodes: &[Node]) { + for (i, node) in nodes.iter().enumerate() { + if self.over_limit() { + return; + } + let block = node.is_block(); + if i > 0 { + let prev = &nodes[i - 1]; + let tight = matches!(prev, Node::Paragraph { .. } | Node::Heading { .. }) + || prev.is_inline(); + let interrupts = match node { + Node::List { ordered, start, .. } => { + !ordered || matches!(start, None | Some(1)) + } + _ => false, + }; + if tight && interrupts { + if !self.buf.ends_with('\n') { + self.buf.push('\n'); + } + } else if block || prev.is_block() { + if !self.buf.ends_with('\n') { + self.buf.push('\n'); } + self.buf.push('\n'); } - } else { - // Simple inline item - self.render_inline(&item.children); - self.buf.push('\n'); } + if !block && (self.buf.is_empty() || self.buf.ends_with('\n')) { + self.line_start = true; + } + self.render_node(node, 0); } } @@ -372,68 +400,167 @@ impl MarkdownRenderer { /// Render a single inline node. fn render_inline_node(&mut self, node: &Node) { match node { - Node::Text(t) => self.buf.push_str(t), - Node::Bold { children } => { - self.buf.push_str("**"); - self.render_inline(children); - self.buf.push_str("**"); - } - Node::Italic { children } => { - self.buf.push('*'); - self.render_inline(children); - self.buf.push('*'); - } - Node::Strikethrough { children } => { - self.buf.push_str("~~"); - self.render_inline(children); - self.buf.push_str("~~"); + Node::Text(t) => { + let pos = if self.line_start { + Position::LineStart + } else { + Position::Inline + }; + self.line_start = escape::push_text(&mut self.buf, t, pos); } + Node::Bold { children } => self.render_wrapped("**", children), + Node::Italic { children } => self.render_wrapped("*", children), + Node::Strikethrough { children } => self.render_wrapped("~~", children), Node::InlineCode(code) => { - self.buf.push('`'); - self.buf.push_str(code); - self.buf.push('`'); + escape::push_code_span(&mut self.buf, code, self.in_table); + self.line_start = false; } Node::Link { url, title, children, - } => { - self.buf.push('['); - self.render_inline(children); - self.buf.push_str("]("); - self.buf.push_str(url); - if let Some(t) = title { - self.buf.push_str(" \""); - self.buf.push_str(t); - self.buf.push('"'); + } => match escape::safe_destination(url, UrlKind::Link).filter(|_| !self.in_link) { + Some(mut dest) => { + if self.in_table { + dest = dest.replace('|', "%7C"); + } + self.buf.push('['); + self.line_start = false; + self.in_link = true; + self.render_inline(children); + self.in_link = false; + self.buf.push_str("]("); + self.buf.push_str(&dest); + if let Some(t) = title { + self.buf.push_str(" \""); + self.buf.push_str(&escape::link_title(t)); + self.buf.push('"'); + } + self.buf.push(')'); } - self.buf.push(')'); - } - Node::Image { src, alt } => { - self.buf.push_str("!["); - self.buf.push_str(alt); - self.buf.push_str("]("); - self.buf.push_str(src); - self.buf.push(')'); - } + // Disallowed scheme or nested link: keep the text, drop the link. + None => self.render_inline(children), + }, + Node::Image { src, alt } => match escape::safe_destination(src, UrlKind::Image) { + Some(mut dest) => { + if self.in_table { + dest = dest.replace('|', "%7C"); + } + self.buf.push_str("!["); + escape::push_text(&mut self.buf, alt, Position::Inline); + self.buf.push_str("]("); + self.buf.push_str(&dest); + self.buf.push(')'); + self.line_start = false; + } + None => { + let pos = if self.line_start { + Position::LineStart + } else { + Position::Inline + }; + self.line_start = escape::push_text(&mut self.buf, alt, pos); + } + }, + // A break on an empty line would make a blank line and end the + // paragraph (and any open link) early. + Node::LineBreak if self.line_start => {} Node::LineBreak => { - self.buf.push_str(" \n"); + if self.in_table { + self.buf.push(' '); + } else { + self.buf.push_str(" \n"); + self.line_start = true; + } } + // GitHub's $`tex`$ form: math there, a code span elsewhere. Node::Math { tex } => { - let tex = inline_math(tex); + let tex = tex.trim(); if !tex.is_empty() { self.buf.push('$'); - self.buf.push_str(&tex); + escape::push_code_span(&mut self.buf, tex, self.in_table); self.buf.push('$'); + self.line_start = false; } } Node::FootnoteReference { label } => { self.buf.push_str("[^"); self.buf.push_str(&footnote_label(label)); self.buf.push(']'); + self.line_start = false; } - // Block nodes shouldn't appear here but handle gracefully - other => self.render_node(other, 0), + // Block nodes inside inline content (table cells, headings, link + // text): flatten to one line so they cannot break the enclosing block. + other => self.render_block_inline(other), + } + } + + fn render_wrapped(&mut self, delim: &str, children: &[Node]) { + self.buf.push_str(delim); + self.line_start = false; + self.render_inline(children); + self.buf.push_str(delim); + } + + /// Render a block node as single-line inline content. + fn render_block_inline(&mut self, node: &Node) { + let sep = |r: &mut Self| { + // No space right after an opening delimiter (`**`, `~~`, `[`). + if !r.buf.is_empty() && !r.buf.ends_with([' ', '\n', '*', '~', '[']) { + r.buf.push(' '); + } + }; + match node { + Node::Heading { children, .. } + | Node::Paragraph { children, .. } + | Node::BlockQuote { children, .. } + | Node::FootnoteDefinition { children, .. } => { + sep(self); + self.render_inline(children); + } + Node::Figure { + children, caption, .. + } => { + sep(self); + self.render_inline(children); + if let Some(caption) = caption { + sep(self); + self.render_inline(caption); + } + } + Node::List { items, .. } => { + for item in items { + sep(self); + self.render_inline(&item.children); + } + } + Node::DefinitionList { items, .. } => { + for item in items { + sep(self); + self.render_inline(&item.term); + for def in &item.definitions { + sep(self); + self.render_inline(def); + } + } + } + Node::CodeBlock { code, .. } => { + sep(self); + escape::push_code_span(&mut self.buf, code, self.in_table); + self.line_start = false; + } + Node::MathBlock { tex, .. } => { + sep(self); + self.render_inline_node(&Node::Math { tex: tex.clone() }); + } + Node::Table { headers, rows, .. } => { + for cell in headers.iter().chain(rows.iter().flatten()) { + sep(self); + self.render_inline(&cell.children); + } + } + Node::ThematicBreak => sep(self), + inline => self.render_inline_node(inline), } } @@ -454,6 +581,8 @@ impl MarkdownRenderer { if col_count == 0 { return; } + self.in_table = true; + self.line_start = false; for (r, row) in grid.iter().enumerate() { if self.over_limit() { @@ -483,6 +612,7 @@ impl MarkdownRenderer { self.buf.push('\n'); } } + self.in_table = false; } } @@ -563,20 +693,6 @@ fn push_hanging(buf: &mut String, inner: &str, first: &str, rest: &str) { } } -/// Inline TeX for `$...$`: one line, `$` escaped, no edge spaces (which -/// would stop most parsers from treating it as math). -fn inline_math(tex: &str) -> String { - let mut out = String::with_capacity(tex.len()); - for c in tex.trim().chars() { - match c { - '\r' | '\n' => out.push(' '), - '$' if !out.ends_with('\\') => out.push_str("\\$"), - c => out.push(c), - } - } - out -} - /// Footnote labels are emitted inside `[^...]`: keep `[A-Za-z0-9_.-]`, map /// everything else to `-` so a label cannot close the bracket or break the line. fn footnote_label(label: &str) -> String { @@ -620,20 +736,6 @@ fn trim_trailing_inline(buf: &mut String, start: usize) { } } -/// Escape text for a GFM table cell: pipes escaped, line breaks flattened. -#[cfg(any(feature = "docx", feature = "spreadsheet"))] -pub(crate) fn escape_table_cell(s: &str) -> String { - let mut out = String::with_capacity(s.len()); - for c in s.chars() { - match c { - '|' => out.push_str("\\|"), - '\r' | '\n' => out.push(' '), - c => out.push(c), - } - } - out -} - /// Normalize output: collapse multiple blank lines, preserve hard breaks (two trailing spaces). fn normalize_output(s: &str) -> String { let mut result = String::with_capacity(s.len()); @@ -686,12 +788,6 @@ mod tests { assert_eq!(normalize_output("a \nb\n"), "a \nb\n"); } - #[test] - fn test_inline_math() { - assert_eq!(inline_math(" a$b\nc "), "a\\$b c"); - assert_eq!(inline_math("\\$"), "\\$"); - } - #[test] fn math_rendering() { let doc = Document::new(vec![ @@ -708,7 +804,7 @@ mod tests { ]); assert_eq!( MarkdownRenderer::new().render(&doc), - "Euler $e^{i\\pi}$\n\n$$\na\nb $ $ c\n$$\n" + "Euler $`e^{i\\pi}`$\n\n```math\na\n\nb $$ c\n```\n" ); } @@ -718,10 +814,4 @@ mod tests { assert_eq!(footnote_label("a]b\n[c"), "a-b--c"); assert_eq!(footnote_label(" "), "note"); } - - #[cfg(any(feature = "docx", feature = "spreadsheet"))] - #[test] - fn test_escape_table_cell() { - assert_eq!(escape_table_cell("a|b\r\nc"), "a\\|b c"); - } } diff --git a/crates/markkit/src/render/mod.rs b/crates/markkit/src/render/mod.rs index cc262f8..b5bd5ff 100644 --- a/crates/markkit/src/render/mod.rs +++ b/crates/markkit/src/render/mod.rs @@ -2,8 +2,7 @@ //! //! Converts the canonical IR into output formats. +mod escape; mod markdown; pub use markdown::MarkdownRenderer; -#[cfg(any(feature = "docx", feature = "spreadsheet"))] -pub(crate) use markdown::escape_table_cell; diff --git a/crates/markkit/src/spreadsheet.rs b/crates/markkit/src/spreadsheet.rs index 5e4c574..4b0c99d 100644 --- a/crates/markkit/src/spreadsheet.rs +++ b/crates/markkit/src/spreadsheet.rs @@ -30,7 +30,6 @@ use crate::error::Error; use crate::format::Format; use crate::ir::{Document, Node, Source, TableCell}; use crate::limits::Budget; -use crate::render::escape_table_cell; pub(crate) struct SheetOutput { pub doc: Document, @@ -349,7 +348,7 @@ impl Grid { for (c, v) in cells { let idx = (c - min_col) as usize; if idx < width { - row[idx] = escape_table_cell(v.trim()); + row[idx] = v.trim().to_string(); } } row diff --git a/crates/markkit/tests/docx_integration.rs b/crates/markkit/tests/docx_integration.rs index f9bb16a..75db26b 100644 --- a/crates/markkit/tests/docx_integration.rs +++ b/crates/markkit/tests/docx_integration.rs @@ -266,7 +266,7 @@ fn empty_paragraphs_skipped() { #[test] fn xml_entities_decoded() { - assert_eq!(md(¶("a <b> & AB")), "a & AB\n"); + assert_eq!(md(¶("a <b> & AB")), "a \\ & AB\n"); } #[test] diff --git a/crates/markkit/tests/fixtures/html/malformed.md b/crates/markkit/tests/fixtures/html/malformed.md index 69b4a2c..2161df6 100644 --- a/crates/markkit/tests/fixtures/html/malformed.md +++ b/crates/markkit/tests/fixtures/html/malformed.md @@ -4,6 +4,4 @@ Next paragraph Mismatched **bold with no close** -**Entities: & < > " ' - Nested *italic **and bold***** crossed** - ** +**Entities: & \< > " ' Nested *italic **and bold***** crossed** ** diff --git a/crates/markkit/tests/html_integration.rs b/crates/markkit/tests/html_integration.rs index a10e233..483f9d0 100644 --- a/crates/markkit/tests/html_integration.rs +++ b/crates/markkit/tests/html_integration.rs @@ -571,13 +571,14 @@ fn ordered_list_default_start() { #[test] fn link_empty_url() { let md = convert(r#"

text

"#); - assert!(md.contains("[text]()")); + // Empty destination is written as `<>` so a title cannot parse as the URL. + assert!(md.contains("[text](<>)"), "{md}"); } #[test] fn image_empty_src() { let md = convert(r#"x"#); - assert!(md.contains("![x]()")); + assert!(md.contains("![x](<>)"), "{md}"); } #[test] @@ -929,20 +930,20 @@ fn sources_do_not_change_markdown() { #[test] fn mathml_with_tex_annotation() { let html = r#"

Area π\pi r^2 here.

"#; - assert_eq!(convert(html), "Area $\\pi r^2$ here.\n"); + assert_eq!(convert(html), "Area $`\\pi r^2`$ here.\n"); } #[test] fn katex_inline_and_display() { let html = r#"

Inline xx^2.

E = mc^2E=mc2
"#; - assert_eq!(convert(html), "Inline $x^2$.\n\n$$\nE = mc^2\n$$\n"); + assert_eq!(convert(html), "Inline $`x^2`$.\n\n```math\nE = mc^2\n```\n"); } #[test] fn mathjax_container() { let html = r#"visual\int f"#; - assert_eq!(convert(html), "$$\n\\int f\n$$\n"); + assert_eq!(convert(html), "```math\n\\int f\n```\n"); } #[test] @@ -955,7 +956,7 @@ fn mathml_without_tex_keeps_text() { fn math_dollars_cannot_break_out() { let html = r#"

a$ **b** $c

"#; - assert_eq!(convert(html), "$a\\$ **b** \\$c$\n"); + assert_eq!(convert(html), "$`a$ **b** $c`$\n"); } // ─── Transforms ────────────────────────────────────────────────────── diff --git a/crates/markkit/tests/html_security.rs b/crates/markkit/tests/html_security.rs index af22b21..a156c3d 100644 --- a/crates/markkit/tests/html_security.rs +++ b/crates/markkit/tests/html_security.rs @@ -29,19 +29,15 @@ fn xss_event_handler() { #[test] fn xss_javascript_url() { let md = convert(r#"Click"#); - // Link is rendered but the URL is preserved as-is (consumer's responsibility to sanitize URLs) - // The key is no script execution context in markdown - assert!(md.contains("Click")); + // Script-scheme link dropped, text kept. + assert_eq!(md, "Click\n"); } #[test] fn xss_data_uri_img() { let md = convert(r#"xss"#); - // Image is rendered as markdown image — the data URI is in the URL portion - // which is safe in markdown context (no script execution) - assert!(md.contains("![xss]")); - // The output should not contain raw HTML script tags outside of a URL - assert!(!md.starts_with("", + "file:///etc/passwd", + ] { + let md = render(vec![para(vec![link(url)])]); + assert_safe(&md); + assert!(md.contains("click"), "{url:?} -> {md:?}"); + assert!( + !md.to_ascii_lowercase().contains("script:"), + "{url:?} -> {md:?}" + ); + } +} + +#[test] +fn long_scheme_is_not_treated_as_relative() { + // Found by fuzzing: scheme detection used to stop after 64 characters. + let url = format!("{}javascript:alert(1)", "a".repeat(80)); + let md = render(vec![para(vec![link(&url)])]); + assert_safe(&md); + assert_eq!(md, "click\n"); +} + +#[test] +fn entity_encoded_scheme_does_not_decode_into_link() { + let md = render(vec![para(vec![link("javascript:alert(1)")])]); + assert_safe(&md); +} + +#[test] +fn safe_links_are_kept() { + for url in [ + "https://e.com/a?b=1&c=2#f", + "http://e.com", + "mailto:a@e.com", + "/rel/path", + "../x", + "#anchor", + "page.html", + ] { + let md = render(vec![para(vec![link(url)])]); + let dest = events(&md).into_iter().find_map(|e| match e { + Event::Start(Tag::Link { dest_url, .. }) => Some(dest_url.to_string()), + _ => None, + }); + assert_eq!(dest.as_deref(), Some(url), "{md}"); + } +} + +#[test] +fn url_with_parens_spaces_and_brackets_stays_one_link() { + let md = render(vec![para(vec![link("http://e.com/a)b (c)\\e")])]); + let dests: Vec = events(&md) + .into_iter() + .filter_map(|e| match e { + Event::Start(Tag::Link { dest_url, .. }) => Some(dest_url.to_string()), + _ => None, + }) + .collect(); + assert_eq!( + dests, + vec!["http://e.com/a%29b%20%28c%29%3Cd%3E%5Ce"], + "{md}" + ); + assert_eq!(visible_text(&md), "click"); +} + +#[test] +fn link_text_with_bracket_does_not_break_link() { + let md = render(vec![para(vec![Node::Link { + url: "https://e.com".into(), + title: Some("say \"hi\"".into()), + children: vec![text("a]b")], + }])]); + let titles: Vec = events(&md) + .into_iter() + .filter_map(|e| match e { + Event::Start(Tag::Link { title, .. }) => Some(title.to_string()), + _ => None, + }) + .collect(); + assert_eq!(titles, vec!["say \"hi\""], "{md}"); + assert_eq!(visible_text(&md), "a]b"); +} + +#[test] +fn unsafe_image_becomes_alt_text() { + let md = render(vec![para(vec![Node::Image { + src: "javascript:alert(1)".into(), + alt: "pic".into(), + }])]); + assert_safe(&md); + assert_eq!(visible_text(&md), "pic"); + let md = render(vec![para(vec![Node::Image { + src: "data:image/svg+xml,".into(), + alt: "svg".into(), + }])]); + assert_safe(&md); + assert!(!md.contains("onload"), "{md}"); +} + +#[test] +fn raster_data_and_http_images_are_kept() { + for src in ["https://e.com/a.png", "data:image/png;base64,iVBORw0KGgo="] { + let md = render(vec![para(vec![Node::Image { + src: src.into(), + alt: "a]b".into(), + }])]); + let got = events(&md).into_iter().find_map(|e| match e { + Event::Start(Tag::Image { dest_url, .. }) => Some(dest_url.to_string()), + _ => None, + }); + assert_eq!(got.as_deref(), Some(src), "{md}"); + assert_eq!(visible_text(&md), "a]b"); + } +} + +// ─── Code ──────────────────────────────────────────────────────────── + +#[test] +fn code_block_cannot_close_its_fence_early() { + let code = "before\n```\n# escaped\n\n````\nafter"; + let md = render(vec![ + Node::CodeBlock { + source: None, + language: Some("js".into()), + code: code.into(), + }, + para(vec![text("tail")]), + ]); + assert_eq!( + block_tags(&md), + vec!["CodeBlock(Fenced(Borrowed(\"js\")))", "Paragraph"], + "{md}" + ); + assert_safe(&md); + let body: String = events(&md) + .into_iter() + .skip_while(|e| !matches!(e, Event::Start(Tag::CodeBlock(_)))) + .take_while(|e| !matches!(e, Event::End(TagEnd::CodeBlock))) + .filter_map(|e| match e { + Event::Text(t) => Some(t.to_string()), + _ => None, + }) + .collect(); + assert_eq!(body, format!("{code}\n")); +} + +#[test] +fn code_block_language_cannot_inject() { + let md = render(vec![Node::CodeBlock { + source: None, + language: Some("js```\n# x".into()), + code: "a".into(), + }]); + assert_eq!(block_tags(&md).len(), 1, "{md}"); +} + +#[test] +fn inline_code_with_backticks_round_trips() { + for code in ["a`b", "``x``", "`", "a ``` b"] { + let md = render(vec![para(vec![Node::InlineCode(code.into())])]); + assert_eq!(visible_text(&md), code, "{code:?} -> {md:?}"); + } +} + +#[test] +fn adjacent_code_spans_do_not_merge_fences() { + // Found by fuzzing: the closing fence of one span ran into the opening + // fence of the next, so neither closed and `` leaked as HTML. + let md = render(vec![para(vec![ + Node::InlineCode("a``b".into()), + Node::InlineCode("`x".into()), + ])]); + assert_safe(&md); + let codes: Vec<String> = events(&md) + .into_iter() + .filter_map(|e| match e { + Event::Code(c) => Some(c.to_string()), + _ => None, + }) + .collect(); + assert_eq!(codes, vec!["a``b", "<title>`x"], "{md}"); +} + +// ─── Tables ────────────────────────────────────────────────────────── + +#[test] +fn table_cell_pipes_and_breaks_do_not_split_cells() { + let md = render(vec![Node::Table { + source: None, + alignments: vec![], + headers: vec![ + TableCell::new(vec![text("h|1")]), + TableCell::new(vec![text("h2")]), + ], + rows: vec![vec![ + TableCell::new(vec![text("a|b"), Node::LineBreak, text("c")]), + TableCell::new(vec![Node::InlineCode("x|y".into())]), + ]], + }]); + let cells: Vec<String> = { + let mut out = Vec::new(); + let mut cur: Option<String> = None; + for e in events(&md) { + match e { + Event::Start(Tag::TableCell) => cur = Some(String::new()), + Event::End(TagEnd::TableCell) => out.push(cur.take().unwrap()), + Event::Text(t) | Event::Code(t) => { + if let Some(c) = cur.as_mut() { + c.push_str(&t) + } + } + _ => {} + } + } + out + }; + assert_eq!(cells, vec!["h|1", "h2", "a|b c", "x|y"], "{md}"); +} + +// ─── Block nodes in inline context (found by fuzzing) ─────────────── + +#[test] +fn code_block_in_table_cell_stays_in_cell() { + let md = render(vec![Node::Table { + source: None, + alignments: vec![], + headers: vec![TableCell::new(vec![text("h")])], + rows: vec![vec![TableCell::new(vec![Node::CodeBlock { + source: None, + language: None, + code: "a\n<script>\n```".into(), + }])]], + }]); + assert_safe(&md); + assert_eq!(block_tags(&md), vec!["Table([None])"], "{md}"); +} + +#[test] +fn blocks_inside_heading_stay_on_one_line() { + let md = render(vec![Node::Heading { + source: None, + level: 2, + children: vec![ + text("a"), + para(vec![text("b")]), + Node::List { + source: None, + ordered: false, + start: None, + items: vec![ListItem::new(vec![text("c")])], + }, + Node::CodeBlock { + source: None, + language: None, + code: "<x>".into(), + }, + ], + }]); + assert_safe(&md); + assert_eq!( + block_tags(&md), + vec!["Heading { level: H2, id: None, classes: [], attrs: [] }"], + "{md}" + ); + assert_eq!(visible_text(&md), "a b c <x>", "{md}"); +} + +#[test] +fn empty_destination_cannot_swallow_title() { + let md = render(vec![para(vec![Node::Link { + url: String::new(), + title: Some("javascript:alert(1)".into()), + children: vec![text("x")], + }])]); + assert_safe(&md); + let md = render(vec![para(vec![Node::Image { + src: " ".into(), + alt: "a".into(), + }])]); + assert_safe(&md); +} + +#[test] +fn inline_run_next_to_block_at_top_level() { + // Found by fuzzing: a block right after loose inline text started mid-line, + // so its fence did not open and the code became raw HTML. + let md = render(vec![ + text("lead"), + Node::CodeBlock { + source: None, + language: None, + code: "<?x\n<script>".into(), + }, + text("# tail"), + Node::BlockQuote { + source: None, + children: vec![para(vec![text("q")])], + }, + text("after"), + ]); + assert_safe(&md); + assert_eq!( + block_tags(&md), + vec![ + "Paragraph", + "CodeBlock(Fenced(Borrowed(\"\")))", + "Paragraph", + "BlockQuote(None)", + "Paragraph", + "Paragraph" + ], + "{md}" + ); + assert!(visible_text(&md).contains("# tail"), "{md}"); +} + +#[test] +fn code_block_in_nested_ordered_list_stays_fenced() { + // Found by fuzzing: sub-blocks were indented 2 spaces per level, less than + // an ordered marker's width, so the fence fell out of the item. + let inner = Node::List { + source: None, + ordered: true, + start: Some(10), + items: vec![ListItem::new(vec![ + para(vec![text("item")]), + Node::CodeBlock { + source: None, + language: None, + code: "<title>x".into(), + }, + ])], + }; + let md = render(vec![Node::List { + source: None, + ordered: true, + start: None, + items: vec![ListItem::new(vec![para(vec![text("outer")]), inner])], + }]); + assert_safe(&md); + let tags = block_tags(&md); + assert_eq!( + tags.iter().filter(|t| t.starts_with("List")).count(), + 2, + "{md}" + ); + assert_eq!( + tags.iter().filter(|t| t.starts_with("CodeBlock")).count(), + 1, + "{md}" + ); + // The code block is inside the inner item: nothing follows the outer list. + assert!(md.trim_end().ends_with("```"), "{md}"); +} + +#[test] +fn nested_list_under_ordered_item_is_nested() { + let md = render(vec![Node::List { + source: None, + ordered: true, + start: None, + items: vec![ListItem::new(vec![ + para(vec![text("a")]), + Node::List { + source: None, + ordered: false, + start: None, + items: vec![ListItem::new(vec![text("b")])], + }, + ])], + }]); + assert_eq!(md, "1. a\n - b\n"); + let starts = block_tags(&md) + .iter() + .filter(|t| t.starts_with("List")) + .count(); + assert_eq!(starts, 2, "{md}"); +} + +#[test] +fn repeated_hard_breaks_do_not_end_link_or_paragraph() { + // Found by fuzzing: two hard breaks made a blank line, ending the link early + // and leaking its title as raw text. + let md = render(vec![para(vec![Node::Link { + url: "https://e.com".into(), + title: Some("".into()), + children: vec![ + text("a"), + Node::LineBreak, + Node::LineBreak, + text(" "), + Node::LineBreak, + text("b"), + ], + }])]); + assert_safe(&md); + assert_eq!(block_tags(&md), vec!["Paragraph"], "{md}"); + assert!( + events(&md) + .iter() + .any(|e| matches!(e, Event::Start(Tag::Link { .. }))), + "{md}" + ); +} + +#[test] +fn nested_links_keep_outer_link_intact() { + // Found by fuzzing: a link inside link text cancels the outer link in + // CommonMark, leaking the outer title (``) as raw text. + let inner = Node::Link { + url: "https://in.example".into(), + title: None, + children: vec![text("in")], + }; + let md = render(vec![para(vec![Node::Link { + url: "https://out.example".into(), + title: Some(" \"q\"".into()), + children: vec![text("a "), inner], + }])]); + assert_safe(&md); + let links: Vec = events(&md) + .into_iter() + .filter_map(|e| match e { + Event::Start(Tag::Link { + dest_url, title, .. + }) => Some(format!("{dest_url} {title}")), + _ => None, + }) + .collect(); + assert_eq!(links, vec!["https://out.example \"q\""], "{md}"); + assert_eq!(visible_text(&md), "a in"); +} + +// ─── End to end through the HTML parser ────────────────────────────── + +#[cfg(feature = "html")] +#[test] +fn html_payload_page_is_safe() { + let html = r#"

# not heading *x* <img src=x onerror=1>

+click p +
```
+evil
+```

after

"#; + let md = markkit::Markkit::builder() + .build() + .extract_html_sync(html) + .unwrap(); + assert_safe(&md); + assert!(md.contains("\\>().join(" "); + prop_assume!(!collapsed.is_empty()); + assert_safe(&md); + prop_assert_eq!(block_tags(&md), vec!["Paragraph".to_string()], "{:?}", md); + prop_assert_eq!(visible_text(&md).split_whitespace().collect::>().join(" "), collapsed, "{:?}", md); + } + + /// Arbitrary link URLs never yield an unsafe destination or raw HTML. + #[test] + fn arbitrary_link_urls_are_safe(url in "\\PC{0,40}", t in "[a-z]{1,5}") { + let md = render(vec![para(vec![Node::Link { url, title: None, children: vec![text(&t)] }])]); + assert_safe(&md); + prop_assert!(visible_text(&md).contains(&t)); + } + + /// Arbitrary code never escapes its fenced block. + #[test] + fn arbitrary_code_stays_fenced(code in "[`~a \n]{0,40}") { + let md = render(vec![Node::CodeBlock { source: None, language: None, code }, para(vec![text("tail")])]); + let tags = block_tags(&md); + prop_assert_eq!(tags.len(), 2, "{:?}", md); + prop_assert!(tags[0].starts_with("CodeBlock"), "{:?}", md); + } +} + +// ─── Math ──────────────────────────────────────────────────────────── + +/// TeX comes straight from page markup (``), so it is as untrusted +/// as text. Plain GFM has no math syntax: `$...$` content is parsed as +/// Markdown, so math must be inert there. +#[test] +fn inline_math_cannot_emit_html_or_links() { + for tex in [ + "", + "[x](javascript:alert(1))", + "a ** b ** c", + "`` x ``", + ] { + let md = render(vec![para(vec![ + text("see "), + Node::Math { tex: tex.into() }, + ])]); + assert_safe(&md); + assert!( + visible_text(&md).contains(tex.trim_matches('`').trim()), + "{tex}: {md}" + ); + assert_eq!(block_tags(&md), ["Paragraph"], "{tex}: {md}"); + } +} + +#[test] +fn display_math_stays_in_its_block() { + let tex = "a\n```\n\n# h\n$$\n- b"; + let md = render(vec![ + Node::MathBlock { + tex: tex.into(), + source: None, + }, + para(vec![text("after")]), + ]); + assert_safe(&md); + let tags = block_tags(&md); + assert_eq!(tags.len(), 2, "{md}"); + assert!( + tags[0].starts_with("CodeBlock") && tags[1] == "Paragraph", + "{md}" + ); +} + +#[test] +fn math_in_table_cell_stays_in_cell() { + let md = render(vec![Node::Table { + alignments: vec![], + headers: vec![TableCell::new(vec![Node::Math { + tex: "a | b".into(), + }])], + rows: vec![], + source: None, + }]); + assert_safe(&md); + assert!(md.starts_with("| $`a \\| b`$ |"), "{md}"); +} + +#[test] +fn display_math_in_inline_context_is_flattened() { + let md = render(vec![Node::Table { + alignments: vec![], + headers: vec![TableCell::new(vec![Node::MathBlock { + tex: "x\n= 1".into(), + source: None, + }])], + rows: vec![], + source: None, + }]); + assert_safe(&md); + assert!(md.starts_with("| $`x = 1`$ |"), "{md}"); +} diff --git a/knowledge/formats/html-extraction.md b/knowledge/formats/html-extraction.md index 3788639..c0157a9 100644 --- a/knowledge/formats/html-extraction.md +++ b/knowledge/formats/html-extraction.md @@ -39,7 +39,7 @@ HTML from the web: malformed markup, complex layouts, embedded scripts, etc. | `
  • ` | `- [x] item` | | `
    ` + `
    ` | content + `*caption*` | | footnote refs / sections | `[^1]` / `[^1]: body` | -| ``, KaTeX, MathJax | `$tex$` / `$$` block | +| ``, KaTeX, MathJax | `` $`tex`$ `` / ` ```math ` block | | `/` | `*text*` | | `/` | `**text**` | | `/` | `~~text~~` | @@ -221,7 +221,10 @@ serde_json's 128-level recursion limit bounds nesting; invalid JSON is ignored. - Nested lists: supported via recursive `collect_list_items` with depth tracking - Tables within tables: outer table parsed; inner table content may appear as text - Inline SVG: stripped entirely (non-content element) -- Data URIs: preserved as-is in `src`/`href` attributes (no execution in markdown) +- URL schemes: links keep `http`, `https`, `mailto`, `tel`, `ftp` and relative URLs; + images keep `http`, `https`, relative and raster `data:image/*`. Other schemes drop the + link or image and keep its text (see [Threat Model](../security/threat-model.md) T1) +- Markdown-significant characters in text are escaped at render time (`<`, `*`, `[`, ...) - Relative URLs: resolved only when a base is known, see Relative URL Resolution - Character encoding: `String::from_utf8_lossy` for byte input; html5ever handles encoding within HTML - Null bytes: replaced with U+FFFD by html5ever @@ -229,8 +232,8 @@ serde_json's 128-level recursion limit bounds nesting; invalid JSON is ignored. - Empty containers: all block-level containers with no content are elided - HTML entities: decoded by html5ever before reaching parser - Blocks inside inline formatting: after an unclosed ``, html5ever's adoption - agency wraps following paragraphs in the formatting element; they render inside the - emphasis markers (known limitation, see `malformed` fixture) + agency wraps following paragraphs in the formatting element; they render flattened + onto one line inside the emphasis markers (known limitation, see `malformed` fixture) ## See also diff --git a/knowledge/foundations/ir.md b/knowledge/foundations/ir.md index 28b00bb..d76784a 100644 --- a/knowledge/foundations/ir.md +++ b/knowledge/foundations/ir.md @@ -68,7 +68,7 @@ All block variants except `ThematicBreak` also carry `source` (see Provenance). | `DefinitionList` | `items: Vec` | Terms with one or more definitions. | | `Figure` | `children: Vec`, `caption: Option>` | Block content with inline caption. | | `FootnoteDefinition` | `label: String`, `children: Vec` | Footnote body (blocks). | -| `MathBlock` | `tex: String` | Display math, TeX source. Renders `$$` fenced; `$$` inside is split. | +| `MathBlock` | `tex: String` | Display math, TeX source. Renders as a ` ```math ` code fence (math on GitHub, inert code elsewhere; TeX is untrusted, S17). | #### Inline Variants @@ -83,7 +83,7 @@ All block variants except `ThematicBreak` also carry `source` (see Provenance). | `Image` | `src: String`, `alt: String` | Image reference. Leaf node (no children). | | `LineBreak` | (unit) | Hard line break. Renders as two trailing spaces + newline. | | `FootnoteReference` | `label: String` | Marker pointing at the `FootnoteDefinition` with the same label. | -| `Math` | `tex: String` | Inline math, TeX source. Renders `$tex$`: one line, trimmed, unescaped `$` escaped. | +| `Math` | `tex: String` | Inline math, TeX source. Renders GitHub's `` $`tex`$ `` form: one line, trimmed, inside a code span so plain-GFM readers never parse it as Markdown or HTML. | ### Provenance (`Source`) @@ -141,7 +141,8 @@ At render time, the `normalize_output` function: - Block nodes separated by blank lines - Inline content concatenated directly -- Lists indented with 2-space increments per depth level +- List item bodies (continuation lines, nested lists, sub-blocks) indented by the marker + width (`- ` = 2, `1. ` = 3) - BlockQuote lines prefixed with `> ` - Tables rendered as GFM pipe tables; separator carries alignment (`:---`, `:---:`, `---:`). Spans are laid out on a grid: content in the @@ -153,6 +154,9 @@ At render time, the `normalize_output` function: - Definition lists: term line, then `: definition` per definition (continuation indented 4), items separated by blank lines - Figures: children, then the caption as an italic paragraph +- Text, code and URLs escaped by `render/escape.rs` so IR text renders verbatim and + unsafe link/image schemes are dropped ([Threat Model](../security/threat-model.md) T1). + The IR itself always holds unescaped text. ## IR Invariants diff --git a/knowledge/log.md b/knowledge/log.md index 293615e..886c1ff 100644 --- a/knowledge/log.md +++ b/knowledge/log.md @@ -6,8 +6,8 @@ ## 2026-10-26 +* **Output safety**: the renderer escaped nothing, so document text could emit raw HTML, links, headings and fence breakouts, and `javascript:` links passed through. Escaping and a URL scheme allowlist now live in `render/escape.rs`; DOCX and spreadsheet parsers stop pre-escaping table pipes, so the IR holds raw text. New invariant S17 with a pulldown-cmark oracle. A first fuzzing pass against that oracle found eight more container breakouts (list indentation by marker width, blocks in inline context, adjacent code spans, nested links, ...), all fixed. Math now renders as code (`` $`tex`$ ``, ` ```math `) because TeX from page markup reached plain-GFM output unescaped. See [Threat Model](security/threat-model.md) T1. * **Main content**: opt-in Readability-style detection (`main_content`, CLI `--main-content`) keeps the article body and drops sidebars, comments, share bars; falls back to the full body when nothing scores. Algorithm in [HTML Extraction](formats/html-extraction.md); bounds in [Threat Model](security/threat-model.md) T2g. - * **Relative URLs**: links, images and metadata URLs resolve against `base_url` (or the final `extract_url` response URL) plus ``. Only scheme-less references change; page bases are limited to `http(s)`. New `extract_url_document`. See [HTML Extraction](formats/html-extraction.md) and [Threat Model](security/threat-model.md) T1. * **Transforms**: `transform::Transform` passes between parse and render (`MarkkitBuilder::transform`), built-ins `NormalizeHeadings` / `DropImages`, `from_fn`, IR walkers. `Error` is now `#[non_exhaustive]` with `Error::Transform`. See [Markkit Architecture](foundations/architecture.md), [Examples](operations/examples.md). diff --git a/knowledge/operations/testing.md b/knowledge/operations/testing.md index f83a318..2be08e1 100644 --- a/knowledge/operations/testing.md +++ b/knowledge/operations/testing.md @@ -17,6 +17,7 @@ tags: | `crates/markkit/tests/html_integration.rs` | End-to-end HTML to markdown | | `crates/markkit/tests/html_fixtures.rs` | Golden files: `tests/fixtures/html/.html` vs `.md` | | `crates/markkit/tests/html_security.rs` | XSS payloads, nesting, oversized input | +| `crates/markkit/tests/output_safety.rs` | Renderer escaping, URL scheme allowlist, fences; pulldown-cmark oracle + proptests | | `crates/markkit/tests/html_proptest.rs` | Property tests: no panic, no script/style, determinism, limits | | `crates/markkit/tests/docx_integration.rs`, `spreadsheet_integration.rs` | Office mapping | | `crates/markkit/tests/document_detection.rs` | Format detection order and explicit unsupported errors | diff --git a/knowledge/security/threat-model.md b/knowledge/security/threat-model.md index 6113b8f..18a7367 100644 --- a/knowledge/security/threat-model.md +++ b/knowledge/security/threat-model.md @@ -26,8 +26,9 @@ tags: ### T1: Injection via Extracted Content -**Description**: Attacker crafts HTML so extracted markdown contains executable content -(script tags, event handlers, style expressions). +**Description**: Attacker crafts a document so extracted markdown contains executable +content (script tags, event handlers, script-scheme links) or spoofed structure (text +that renders as headings, links, code fences or tables). **Impact**: High -- downstream consumers may render markdown as HTML, re-introducing XSS. @@ -36,7 +37,26 @@ tags: **Mitigations**: - Parser strips all `` | Stripped (non-content element) | | Event handlers | `

    ` | Attribute not read | -| JavaScript URLs | `` | URL preserved as-is in markdown (no execution context); never rewritten by base resolution | -| Data URI payloads | `` | Rendered as markdown image (no execution) | +| JavaScript URLs | `` | Link dropped, text kept (scheme allowlist); never rewritten by base resolution | +| Data URI payloads | `` | Image dropped, alt text kept (only raster `data:image/*` allowed) | +| Markdown in text | `

    # x *y* <img onerror>

    ` | Escaped; renders as the literal text | +| Fence breakout | ```` ``` ```` inside `
    ` | Fence longer than any backtick run in the code |
 | Style expressions | `style="background:url(javascript:...)"` | Attribute not read |
 | Deep nesting | 1000+ nested `
    ` | Flattened to text at `max_nesting_depth` (256) |
 | Huge text nodes | 100KB+ single text node | Passed through (bounded by input limit) |
@@ -346,6 +371,7 @@ process, or smuggling content past a human reviewer.
 | S14 | Returned Markdown never exceeds `max_output_size` | `limits::*output_cap*` |
 | S15 | No configuration of `max_nesting_depth` can overflow the stack | `limits::*hard_ceiling_survives_small_stack` |
 | S16 | Extraction stops with `ResourceLimit` once `timeout` passes (checked at least after each non-interruptible third-party step) | `limits::*zero_timeout_rejects`, `limits::tests::*` |
+| S17 | Rendered markdown has no raw HTML, no non-allowlisted link/image scheme, and document text cannot create Markdown structure | `output_safety::*` (pulldown-cmark oracle + proptests) |
 
 ## See also