1 From 1c7055f0cf0b3f183e65373112f23425c14871ae Mon Sep 17 00:00:00 20012 From: hu13-test <[email protected]>3 Date: Tue, 27 Oct 2026 15:33:24 +00004 Subject: [PATCH] refuse forge frontmatter paths that escape out/ (#60)5 6 Issue/PR id and release version become path segments7 (issues/<id>.html, releases/<v>.html, releases/<v>/), so a8 '../../tmp/x' value wrote outside the build output. Validate ids9 ([A-Za-z0-9_-]) and versions (single safe segment) at load time,10 guard write_page() with a lexical relative-path check, and refuse11 unsafe release versions in render too. Refused entries warn and are12 skipped; the build still succeeds.13 ---14  src/forge.rs         |  13 +++++15  src/main.rs          |   1 +16  src/paths.rs         |  74 ++++++++++++++++++++++++17  src/releases.rs      |  43 ++++++++------18  tests/path_safety.rs | 134 +++++++++++++++++++++++++++++++++++++++++++19  5 files changed, 248 insertions(+), 17 deletions(-)20  create mode 100644 src/paths.rs21  create mode 100644 tests/path_safety.rs22 23 diff --git a/src/forge.rs b/src/forge.rs24 index 6044b16..dda2cf3 10064425 --- a/src/forge.rs26 +++ b/src/forge.rs27 @@ -579,6 +579,10 @@ fn load_issues(root: &Path) -> Vec<Issue> {28          let Some(fm): Option<IssueFm> = parse_frontmatter(yaml, &label) else {29              continue;30          };31 +        if !crate::paths::is_safe_id(&fm.id) {32 +            eprintln!("hu13: forge: skipping {label}: unsafe id {:?}", fm.id);33 +            continue;34 +        }35          let comments = load_comments(36              &dir.join(&uuid).join("comments"),37              &format!("issues/{uuid}/comments"),38 @@ -620,6 +624,10 @@ fn load_prs(root: &Path) -> Vec<Pr> {39          let Some(fm): Option<PrFm> = parse_frontmatter(yaml, &label) else {40              continue;41          };42 +        if !crate::paths::is_safe_id(&fm.id) {43 +            eprintln!("hu13: forge: skipping {label}: unsafe id {:?}", fm.id);44 +            continue;45 +        }46          let patch = std::fs::read(dir.join(&uuid).join("patch.diff"))47              .ok()48              .map(|bytes| String::from_utf8_lossy(&bytes).into_owned());49 @@ -1278,6 +1286,11 @@ fn render_ci_index(50  }51  52  fn write_page(out: &Path, rel: &str, html: &str) -> Result<(), String> {53 +    if !crate::paths::is_safe_relative(rel) {54 +        return Err(format!(55 +            "hu13: refusing to write unsafe output path `{rel}`"56 +        ));57 +    }58      let dest = out.join(rel);59      if let Some(parent) = dest.parent() {60          std::fs::create_dir_all(parent).map_err(|e| format!("hu13: create {rel} parent: {e}"))?;61 diff --git a/src/main.rs b/src/main.rs62 index 148c678..d2ee742 10064463 --- a/src/main.rs64 +++ b/src/main.rs65 @@ -8,6 +8,7 @@ mod gitx;66  mod inbox;67  mod layout;68  mod logo;69 +mod paths;70  mod releases;71  mod serve;72  mod wiki;73 diff --git a/src/paths.rs b/src/paths.rs74 new file mode 10064475 index 0000000..437caaa76 --- /dev/null77 +++ b/src/paths.rs78 @@ -0,0 +1,74 @@79 +//! Output-path safety (f0rge #60).80 +//!81 +//! Every path that becomes a file under the build output dir must be82 +//! checked here first: `.forge/` frontmatter (`id`, release `version`) is83 +//! untrusted input, and `out.join("issues/{id}.html")` with84 +//! `id = "../../tmp/x"` writes outside `out/`.85 +//!86 +//! Rules: no absolute paths, no `\` or NUL, no empty/`.`/`..` segments.87 +//! The checks are lexical on purpose — they run before anything is88 +//! created, so there is nothing to canonicalize yet.89 +90 +/// `true` when `segment` is a single safe path component.91 +#[must_use]92 +pub fn is_safe_segment(segment: &str) -> bool {93 +    !segment.is_empty() && segment != "." && segment != ".." && !segment.contains(['/', '\\', '\0'])94 +}95 +96 +/// `true` when `rel` is a relative path made only of safe segments.97 +#[must_use]98 +pub fn is_safe_relative(rel: &str) -> bool {99 +    !rel.is_empty()100 +        && !rel.starts_with('/')101 +        && !rel.contains(['\\', '\0'])102 +        && rel.split('/').all(is_safe_segment)103 +}104 +105 +/// `true` when `id` is safe both as a path segment and as a displayed106 +/// forge id: ASCII alphanumerics, `-`, `_` only.107 +#[must_use]108 +pub fn is_safe_id(id: &str) -> bool {109 +    is_safe_segment(id)110 +        && id111 +            .bytes()112 +            .all(|b| b.is_ascii_alphanumeric() || b == b'-' || b == b'_')113 +}114 +115 +#[cfg(test)]116 +mod tests {117 +    use super::*;118 +119 +    #[test]120 +    fn rejects_traversal_and_absolute() {121 +        for bad in [122 +            "..",123 +            "../x",124 +            "a/../../b",125 +            "/etc/passwd",126 +            "a\\b",127 +            "a\0b",128 +            "",129 +            "a//b",130 +            "./a",131 +            "a/",132 +        ] {133 +            assert!(!is_safe_relative(bad), "{bad:?} must be rejected");134 +        }135 +    }136 +137 +    #[test]138 +    fn accepts_normal_relative_paths() {139 +        for ok in ["a", "a/b", "a-b_c.html", "issues/1.html"] {140 +            assert!(is_safe_relative(ok), "{ok:?} must be accepted");141 +        }142 +    }143 +144 +    #[test]145 +    fn ids_are_strict() {146 +        assert!(is_safe_id("06b5602f-4327-4377-99f6-3dca67b5bd75"));147 +        assert!(is_safe_id("PWNED2"));148 +        for bad in ["../../x", "a/b", "a b", "a.b", "..", "", "a\\b"] {149 +            assert!(!is_safe_id(bad), "{bad:?} must be rejected");150 +        }151 +    }152 +}153 diff --git a/src/releases.rs b/src/releases.rs154 index 03ca8e5..12fbf5a 100644155 --- a/src/releases.rs156 +++ b/src/releases.rs157 @@ -261,20 +261,23 @@ pub fn load(repo: &Path) -> Vec<Release> {158                  continue;159              }160          };161 +        // `version` becomes a path segment (`releases/<v>.html`,162 +        // `releases/<v>/`): untrusted frontmatter, so reject traversal163 +        // loudly instead of writing outside the output dir (f0rge #60).164 +        if !crate::paths::is_safe_segment(&fm.version) {165 +            eprintln!(166 +                "hu13: releases: skipping {label}: unsafe version {:?}",167 +                fm.version168 +            );169 +            continue;170 +        }171          // Artifacts: present only when `out/<version>/` exists at build.172          // Absent -> notes-only, no warning.173 -        let artifacts = if fm.version.contains('/')174 -            || fm.version.contains('\\')175 -            || fm.version.split('/').any(|p| p == "..")176 -        {177 -            Vec::new()178 +        let out_dir = root.join("out").join(&fm.version);179 +        let artifacts = if out_dir.is_dir() {180 +            collect_artifacts(&out_dir)181          } else {182 -            let out_dir = root.join("out").join(&fm.version);183 -            if out_dir.is_dir() {184 -                collect_artifacts(&out_dir)185 -            } else {186 -                Vec::new()187 -            }188 +            Vec::new()189          };190          out.push(Release {191              version: fm.version,192 @@ -424,13 +427,19 @@ pub fn render_loaded(193      std::fs::write(&dest, index).map_err(|e| format!("hu13: write releases index: {e}"))?;194  195      for rel in releases {196 +        // `load` already rejected unsafe versions; keep the guard here197 +        // too so a future caller cannot bypass it (defense in depth).198 +        if !crate::paths::is_safe_segment(&rel.version) {199 +            return Err(format!(200 +                "hu13: refusing to write release with unsafe version `{}`",201 +                rel.version202 +            ));203 +        }204          // Copy the whole out dir when present (no size cap).205 -        if !rel.version.contains('/') && !rel.version.contains('\\') {206 -            let src_dir = root.join("out").join(&rel.version);207 -            if src_dir.is_dir() {208 -                let dest_dir = out.join("releases").join(&rel.version);209 -                copy_tree(&src_dir, &dest_dir)?;210 -            }211 +        let src_dir = root.join("out").join(&rel.version);212 +        if src_dir.is_dir() {213 +            let dest_dir = out.join("releases").join(&rel.version);214 +            copy_tree(&src_dir, &dest_dir)?;215          }216          let html = render_release_page(rel, site_name, nav, branch, short_sha, description);217          let page_dest = out.join("releases").join(format!("{}.html", rel.version));218 diff --git a/tests/path_safety.rs b/tests/path_safety.rs219 new file mode 100644220 index 0000000..d56023f221 --- /dev/null222 +++ b/tests/path_safety.rs223 @@ -0,0 +1,134 @@224 +//! f0rge #60: `.forge` frontmatter is untrusted and must never escape the225 +//! output dir. Issue/PR `id` and release `version` become path segments226 +//! (`issues/<id>.html`, `releases/<version>.html`, `releases/<version>/`),227 +//! so a `../../..` value used to write outside `-o`.228 +229 +use std::path::{Path, PathBuf};230 +use std::process::Command;231 +232 +fn git(repo: &Path, args: &[&str]) {233 +    let st = Command::new("git")234 +        .arg("-C")235 +        .arg(repo)236 +        .args(args)237 +        .status()238 +        .expect("spawn git");239 +    assert!(st.success(), "git {args:?} failed");240 +}241 +242 +fn fresh_dir(prefix: &str) -> PathBuf {243 +    let dir = std::env::temp_dir().join(format!(244 +        "hu13-paths-{prefix}-{}-{}",245 +        std::process::id(),246 +        std::time::SystemTime::now()247 +            .duration_since(std::time::UNIX_EPOCH)248 +            .map(|d| d.as_nanos())249 +            .unwrap_or(0)250 +    ));251 +    std::fs::create_dir_all(&dir).unwrap();252 +    dir253 +}254 +255 +fn write(repo: &Path, rel: &str, content: &str) {256 +    let dest = repo.join(rel);257 +    std::fs::create_dir_all(dest.parent().expect("parent")).unwrap();258 +    std::fs::write(&dest, content).unwrap();259 +}260 +261 +#[test]262 +fn hostile_frontmatter_cannot_write_outside_out() {263 +    let repo = fresh_dir("repo");264 +    let out = fresh_dir("out");265 +    let marker = format!(266 +        "hu13-pwn-{}-{}",267 +        std::process::id(),268 +        std::time::SystemTime::now()269 +            .duration_since(std::time::UNIX_EPOCH)270 +            .map(|d| d.as_nanos())271 +            .unwrap_or(0)272 +    );273 +    let escaped = |name: &str| PathBuf::from(format!("/tmp/{marker}-{name}.html"));274 +275 +    git(&repo, &["init", "-b", "main"]);276 +    git(&repo, &["config", "user.name", "hu13-test"]);277 +    git(&repo, &["config", "user.email", "[email protected]"]);278 +    git(&repo, &["config", "commit.gpgsign", "false"]);279 +    write(&repo, "README.md", "# path safety\n");280 +281 +    // Issue whose id escapes 6 levels (out lives in /tmp, so /../../.. > /).282 +    write(283 +        &repo,284 +        ".forge/issues/aaaaaaaa-1111-4111-8111-111111111111/issue.md",285 +        &format!(286 +            "---\nid: ../../../../../../tmp/{marker}-issue\ntitle: pwn\n\287 +             state: open\ncreated: 2026-09-10T03:00:00Z\n---\nbody\n"288 +        ),289 +    );290 +    // PR whose id escapes.291 +    write(292 +        &repo,293 +        ".forge/prs/bbbbbbbb-2222-4222-8222-222222222222/pr.md",294 +        &format!(295 +            "---\nid: ../../../../../../tmp/{marker}-pr\ntitle: pwn\n\296 +             state: open\nbase: main\nhead: x\ncreated: 2026-09-10T03:00:00Z\n---\nd\n"297 +        ),298 +    );299 +    // Release whose version is absolute / traversal.300 +    write(301 +        &repo,302 +        ".forge/releases/r1.md",303 +        &format!(304 +            "---\nversion: /tmp/{marker}-release\ntitle: r1\n\305 +             created: 2026-09-10T03:00:00Z\n---\nnotes\n"306 +        ),307 +    );308 +    write(309 +        &repo,310 +        ".forge/releases/r2.md",311 +        "---\nversion: ../../../../../../tmp/{marker}-dotdot\ntitle: r2\n\312 +         created: 2026-09-10T03:00:00Z\n---\nnotes\n",313 +    );314 +    // A valid release still has to render.315 +    write(316 +        &repo,317 +        ".forge/releases/r3.md",318 +        "---\nversion: v1.0.0\ntitle: good\ncreated: 2026-09-10T04:00:00Z\n---\nnotes\n",319 +    );320 +321 +    git(&repo, &["add", "-A"]);322 +    git(&repo, &["commit", "-m", "hostile fixture"]);323 +324 +    let produced = Command::new(env!("CARGO_BIN_EXE_hu13"))325 +        .arg("build")326 +        .arg(&repo)327 +        .arg("-o")328 +        .arg(&out)329 +        .output()330 +        .expect("spawn hu13 build");331 +    let stderr = String::from_utf8_lossy(&produced.stderr);332 +    assert!(333 +        produced.status.success(),334 +        "hostile frontmatter must not fail the build: {stderr}"335 +    );336 +337 +    for name in ["issue", "pr", "release", "dotdot"] {338 +        let p = escaped(name);339 +        assert!(340 +            !p.exists(),341 +            "build wrote outside out/: {} exists",342 +            p.display()343 +        );344 +    }345 +    // Nothing landed at /tmp directly either (traversal is blocked, not346 +    // merely clamped).347 +    assert!(!Path::new(&format!("/tmp/{marker}-issue.html")).exists());348 +349 +    // Refused entries are warned about, naming the bad value.350 +    assert!(stderr.contains("unsafe id"), "stderr: {stderr}");351 +    assert!(stderr.contains("unsafe version"), "stderr: {stderr}");352 +353 +    // Valid content survives.354 +    assert!(out.join("releases/v1.0.0.html").is_file());355 +    // Build still completes with an output tree.356 +    assert!(out.join("index.html").is_file());357 +}358 -- 359 2.56.0360