diff --git a/src/srd/fetch.rs b/src/srd/fetch.rs index 19f37bc..17a5d82 100644 --- a/src/srd/fetch.rs +++ b/src/srd/fetch.rs @@ -275,8 +275,15 @@ fn unpack_stripped(tarball: &[u8], destination: &Path) -> Result<(), FetchError> if !should_vendor(&relative_path) { continue; } + let entry_type = entry.header().entry_type(); + if !is_safe_entry_type(entry_type) { + return Err(FetchError::UnsafeEntry(format!( + "{} is a {entry_type:?} entry, not a file or directory", + entry_path.display() + ))); + } let target = destination.join(&relative_path); - if entry.header().entry_type().is_dir() { + if entry_type.is_dir() { fs::create_dir_all(&target)?; } else { let file_parent = target.parent().unwrap_or(Path::new("")); @@ -297,6 +304,16 @@ fn is_safe_entry_path(path: &Path) -> bool { .any(|component| matches!(component, std::path::Component::ParentDir)) } +/// Reports whether a tar entry's type is safe to unpack: a regular file or +/// a directory, the only two shapes this vendored repo actually contains. +/// A symlink entry unpacked through `Entry::unpack` has its target used +/// unvalidated, so a later entry whose path walks through that symlink can +/// write outside `destination`; rejecting the symlink itself closes that +/// off before any such entry is ever written. +fn is_safe_entry_type(entry_type: tar::EntryType) -> bool { + entry_type.is_file() || entry_type.is_dir() +} + /// The upstream repo's top-level files that document the source and its /// licensing, rather than being part of the entry corpus itself. const VENDORED_TOP_LEVEL_FILES: [&str; 3] = ["Legal.md", "README.md", "structure.md"]; @@ -546,6 +563,39 @@ mod tests { encoder.finish().unwrap() } + /// Builds a tarball with a symlink entry (`07_Spells/x`, targeting + /// `/tmp`) followed by a regular file entry whose path walks through + /// that symlink (`07_Spells/x/evil.md`), the way a malicious upstream + /// commit could try to write outside the unpack destination via a + /// symlinked directory rather than a `..` path component. + fn tar_gz_with_a_symlink_escape(top_dir: &str) -> Vec { + let mut builder = tar::Builder::new(Vec::new()); + append_entry(&mut builder, &format!("{top_dir}/"), None); + append_entry(&mut builder, &format!("{top_dir}/07_Spells/"), None); + + let mut symlink_header = tar::Header::new_gnu(); + symlink_header + .set_path(format!("{top_dir}/07_Spells/x")) + .unwrap(); + symlink_header.set_entry_type(tar::EntryType::Symlink); + symlink_header.set_link_name("/tmp").unwrap(); + symlink_header.set_mode(0o777); + symlink_header.set_size(0); + symlink_header.set_cksum(); + builder.append(&symlink_header, io::empty()).unwrap(); + + append_entry( + &mut builder, + &format!("{top_dir}/07_Spells/x/evil.md"), + Some(b"evil"), + ); + let tar_bytes = builder.into_inner().unwrap(); + + let mut encoder = flate2::write::GzEncoder::new(Vec::new(), flate2::Compression::fast()); + encoder.write_all(&tar_bytes).unwrap(); + encoder.finish().unwrap() + } + fn fixture_meta() -> SrdMeta { SrdMeta { version: "5.2.1".to_string(), @@ -995,6 +1045,28 @@ mod tests { server.join().unwrap(); } + #[test] + fn fetch_text_rejects_a_symlink_entry() { + let body = tar_gz_with_a_symlink_escape("dnd.srd.5.2.1-deadbeef"); + let expected = sha256_hex(&body); + let (url, server) = serve_once(body); + let config = TextFetchConfig { + url, + destination: unique_temp_dir().join("dnd.srd.5.2.1"), + expected_tarball_sha256: expected, + commit: "deadbeef".to_string(), + meta_path: unique_temp_dir().join("meta.yaml"), + }; + + let error = fetch_text(&config, &fixture_meta()).unwrap_err(); + + assert!(matches!(error, FetchError::UnsafeEntry(_))); + assert!(error.to_string().contains("07_Spells/x")); + assert!(error.to_string().contains("Symlink")); + assert!(!Path::new("/tmp/evil.md").exists()); + server.join().unwrap(); + } + #[test] fn fetch_text_rejects_a_tarball_hash_mismatch() { let body = tiny_tar_gz("dnd.srd.5.2.1-deadbeef"); diff --git a/src/srd/verify/diff.rs b/src/srd/verify/diff.rs index ccc188a..6d8cef0 100644 --- a/src/srd/verify/diff.rs +++ b/src/srd/verify/diff.rs @@ -35,18 +35,57 @@ fn template(op: &Op) -> Option { /// Describes the first divergence between `source` and `corpus`, or `None` /// if the two word streams are identical. +/// +/// Equal streams return immediately, without allocating the alignment +/// matrix. On a mismatch, the common prefix and common suffix are trimmed +/// off both streams first, so the matrix only covers the words that +/// actually differ; the trimmed words still supply context when the +/// divergence sits too close to either end of what remains. pub fn first_divergence(source: &[String], corpus: &[String]) -> Option { - let ops = edit_script(source, corpus); + if source == corpus { + return None; + } + + let prefix_len = common_prefix_len(source, corpus); + let suffix_len = common_suffix_len(&source[prefix_len..], &corpus[prefix_len..]); + let prefix = &source[..prefix_len]; + let suffix = &source[source.len() - suffix_len..]; + let source_middle = &source[prefix_len..source.len() - suffix_len]; + let corpus_middle = &corpus[prefix_len..corpus.len() - suffix_len]; + + let ops = edit_script(source_middle, corpus_middle); let (index, message) = ops .iter() .enumerate() .find_map(|(i, op)| template(op).map(|message| (i, message)))?; - let before = context_before(&ops, index); - let after = context_after(&ops, index); + let before = context_before(&ops, index, prefix); + let after = context_after(&ops, index, suffix); let context = format!("{before} ... {after}").trim().to_string(); Some(format!("{message} (context: {context})")) } +/// How many leading words `source` and `corpus` share. +fn common_prefix_len(source: &[String], corpus: &[String]) -> usize { + source + .iter() + .zip(corpus.iter()) + .take_while(|(a, b)| a == b) + .count() +} + +/// How many trailing words `source` and `corpus` share. Comparing from the +/// end and stopping at the first zipped mismatch naturally caps the count +/// at the shorter stream's length, so a stream that is a prefix of the +/// other never double-counts words already claimed by the common prefix. +fn common_suffix_len(source: &[String], corpus: &[String]) -> usize { + source + .iter() + .rev() + .zip(corpus.iter().rev()) + .take_while(|(a, b)| a == b) + .count() +} + /// Aligns `source` against `corpus` word by word, minimizing the number of /// single-word inserts, deletes, and substitutions. fn edit_script(source: &[String], corpus: &[String]) -> Vec { @@ -93,26 +132,38 @@ fn edit_script(source: &[String], corpus: &[String]) -> Vec { ops } -/// The last few equal words before `ops[index]`, in reading order. -fn context_before(ops: &[Op], index: usize) -> String { +/// The last few equal words before `ops[index]`, in reading order. When +/// `ops` itself does not hold enough equal words, the words trimmed off +/// the common prefix before the alignment ran fill in the rest. +fn context_before(ops: &[Op], index: usize, prefix: &[String]) -> String { let mut words: Vec<&str> = ops[..index] .iter() .rev() .filter_map(equal_word) .take(CONTEXT_WORDS) .collect(); + if words.len() < CONTEXT_WORDS { + let remaining = CONTEXT_WORDS - words.len(); + words.extend(prefix.iter().rev().take(remaining).map(String::as_str)); + } words.reverse(); words.join(" ") } -/// The next few equal words after `ops[index]`, in reading order. -fn context_after(ops: &[Op], index: usize) -> String { - ops[index + 1..] +/// The next few equal words after `ops[index]`, in reading order. When +/// `ops` itself does not hold enough equal words, the words trimmed off +/// the common suffix before the alignment ran fill in the rest. +fn context_after(ops: &[Op], index: usize, suffix: &[String]) -> String { + let mut words: Vec<&str> = ops[index + 1..] .iter() .filter_map(equal_word) .take(CONTEXT_WORDS) - .collect::>() - .join(" ") + .collect(); + if words.len() < CONTEXT_WORDS { + let remaining = CONTEXT_WORDS - words.len(); + words.extend(suffix.iter().take(remaining).map(String::as_str)); + } + words.join(" ") } fn equal_word(op: &Op) -> Option<&str> { @@ -130,6 +181,11 @@ mod tests { text.split_whitespace().map(str::to_string).collect() } + #[test] + fn template_is_none_for_an_equal_op() { + assert_eq!(template(&Op::Equal("a".to_string())), None); + } + #[test] fn context_skips_a_second_nearby_divergence() { let message = first_divergence(&words("a b c d e f g"), &words("a b X d Y f g")).unwrap(); @@ -141,6 +197,54 @@ mod tests { assert_eq!(first_divergence(&words("a b c"), &words("a b c")), None); } + #[test] + fn common_prefix_len_counts_leading_shared_words() { + assert_eq!(common_prefix_len(&words("a b c"), &words("a b x")), 2); + } + + #[test] + fn common_prefix_len_is_zero_without_a_shared_first_word() { + assert_eq!(common_prefix_len(&words("a b c"), &words("x b c")), 0); + } + + #[test] + fn common_suffix_len_counts_trailing_shared_words() { + assert_eq!(common_suffix_len(&words("a b c"), &words("x b c")), 2); + } + + #[test] + fn common_suffix_len_caps_at_the_shorter_streams_length() { + assert_eq!(common_suffix_len(&words("a b c"), &words("c")), 1); + } + + #[test] + fn reports_an_extra_trailing_word_when_source_is_a_strict_prefix() { + let message = first_divergence(&words("a b"), &words("a b b")).unwrap(); + assert!(message.contains("adds")); + assert!(message.contains("\"b\"")); + } + + #[test] + fn reports_a_missing_trailing_word_when_corpus_is_a_strict_prefix() { + let message = first_divergence(&words("a b b"), &words("a b")).unwrap(); + assert!(message.contains("drops it")); + assert!(message.contains("\"b\"")); + } + + #[test] + fn a_single_word_change_deep_in_a_long_stream_stays_isolated() { + let mut source_text = "filler ".repeat(500); + source_text.push_str("roar"); + let mut corpus_text = "filler ".repeat(500); + corpus_text.push_str("roars"); + + let message = first_divergence(&words(&source_text), &words(&corpus_text)).unwrap(); + + assert!(message.contains("\"roar\"")); + assert!(message.contains("\"roars\"")); + assert!(message.contains("context: filler filler filler ...")); + } + #[test] fn reports_a_dropped_word() { let message = first_divergence( diff --git a/src/srd/verify/monster.rs b/src/srd/verify/monster.rs index 92a1310..5814cbc 100644 --- a/src/srd/verify/monster.rs +++ b/src/srd/verify/monster.rs @@ -29,11 +29,9 @@ pub struct Ability { /// renders to the same comma-joined text for the stat-block word count. /// /// `size`, `creature_type`, and `alignment` are validated here (present, -/// non-empty) but not read again afterward: their words live in the -/// source's italic type line, outside the stat-block region, so the -/// ordinary body word-stream check is what holds them to the source, not -/// this struct. -#[allow(dead_code)] +/// non-empty), then read again by `subtitle::check_monster`, which checks +/// them against the source's italic type line, outside the stat-block +/// region that the ordinary body word-stream check covers. pub struct MonsterFields { pub size: String, pub creature_type: String, diff --git a/src/srd/verify/spell.rs b/src/srd/verify/spell.rs index a2d1684..d099be5 100644 --- a/src/srd/verify/spell.rs +++ b/src/srd/verify/spell.rs @@ -18,11 +18,11 @@ const SCHOOLS: [&str; 8] = [ ]; /// A spell's mechanical frontmatter, validated. `level`, `school`, and -/// `classes` are validated here (range, enum, lowercase) but not read -/// again afterward; the field-line drift check only re-checks the fields +/// `classes` are validated here (range, enum, lowercase), then read again +/// by `subtitle::check_spell`, which checks them against the source's +/// subtitle line; the field-line drift check only re-checks the fields /// that duplicate a bold line in the body (`casting_time`, `range`, /// `components`, `duration`). -#[allow(dead_code)] pub struct SpellFields { pub level: i64, pub school: String,