From f42ae56bb4e07d9377f327eace4a82599adf88ab Mon Sep 17 00:00:00 2001 From: Elijah Voigt Date: Tue, 21 Jul 2026 09:31:00 -0700 Subject: [PATCH] fix(pages): normalize '.' path segments so ./-prefixed tar entries upload MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GNU tar prefixes every entry with ./ when archiving a directory (tar czf out.tgz . yields ./index.html, ./bin_bg.wasm, …). The path validator rejected any '.' segment as traversal, so such tarballs failed with "archive entry has an unsafe path: './...'" and their ./index.html never satisfied the root-index.html requirement — making ./-prefixed archives unusable. (.wasm was never the issue: extensions aren't checked and mime.rs maps .wasm to application/wasm.) check_entry_path now strips '.' segments (leading and interior) and returns the normalized path used as the R2 key, while still rejecting '..', absolute paths, backslashes, NUL, drive letters, empty (//) segments, and names that normalize to nothing; depth is measured on the normalized path. Both the zip and tar manifest builders store the normalized name. +10 tests. Bean: pages-aalu. Co-Authored-By: Claude Opus 4.8 --- ...ath-segments-so-prefixed-tar-entries-ar.md | 33 +++ pages/src/core/upload.rs | 188 ++++++++++++++++-- 2 files changed, 199 insertions(+), 22 deletions(-) create mode 100644 pages/.beans/pages-aalu--normalize-path-segments-so-prefixed-tar-entries-ar.md diff --git a/pages/.beans/pages-aalu--normalize-path-segments-so-prefixed-tar-entries-ar.md b/pages/.beans/pages-aalu--normalize-path-segments-so-prefixed-tar-entries-ar.md new file mode 100644 index 0000000..6fe6783 --- /dev/null +++ b/pages/.beans/pages-aalu--normalize-path-segments-so-prefixed-tar-entries-ar.md @@ -0,0 +1,33 @@ +--- +# pages-aalu +title: Normalize '.' path segments so ./-prefixed tar entries are accepted +status: completed +type: bug +priority: high +created_at: 2026-07-21T16:25:26Z +updated_at: 2026-07-21T16:30:49Z +--- + +## Symptom +Uploading a GNU-tar-style tarball fails with `archive entry has an unsafe path: './bin_bg.wasm'` (a `PathTraversal` 400). Not wasm-specific — `.wasm` is fully allowed (mime.rs maps it to application/wasm; extensions are not checked on upload). + +## Root cause +`check_entry_path` (core/upload.rs ~664-670) splits the entry name on `/` and rejects any segment equal to `.` (alongside `..` and empty). GNU `tar czf out.tgz .` prefixes every entry with `./` (e.g. `./index.html`, `./bin_bg.wasm`), so the leading `.` segment trips the traversal guard. This also breaks the root-`index.html` requirement for such tarballs (`./index.html` != `index.html`), making `./`-prefixed archives unusable. Zip tools don't emit `./`, so only the new compressed-tar path exposed this. + +## Fix +Treat `.` segments as the no-ops they are: normalize them out (strip leading `./`, drop interior `.` segments) and use the normalized `/`-joined path as the stored R2 key. Keep rejecting `..`, absolute paths, backslashes, NUL, drive letters, over-length/over-depth, and empty-after-normalization. Apply to both zip and tar paths for consistency (share via `check_entry_path`). Depth is measured on the normalized path. + +## Tasks +- [x] Normalize `.` segments in check_entry_path; return the normalized path for use as the R2 key +- [x] Update zip + tar manifest builders to store the normalized path (so index.html detection sees the normalized name) +- [x] Tests: `./`-prefixed tar entry accepted (incl. ./index.html satisfies root requirement and ./bin_bg.wasm accepted), interior `./` normalized, `..` still rejected, `.`-only / empty-after-normalization rejected +- [x] All six validation commands pass (native + wasm) +- [x] Commit, push, deploy + +## Summary of Changes + +`check_entry_path` now returns the normalized path (`Result`): `.` current-directory segments are stripped (leading `./` and interior `/./`) instead of rejected, while `..`, absolute paths, backslashes, NUL, drive letters, empty (`//`) segments, and names that normalize to nothing are still rejected. Depth is measured on the normalized segment count. Both `build_zip_manifest` and `read_tar_entries` store the normalized path, so a GNU-tar `./index.html` satisfies the root-level requirement and `./bin_bg.wasm` is accepted. + +`.wasm` was never blocked — the upload validator does not check extensions, and `mime.rs` maps `.wasm` to `application/wasm`. + ++10 tests: 3 tar `./`-prefix wrappers (gzip/zstd/xz) plus 7 direct `check_entry_path` cases (leading/interior `.` normalization, `..` still rejected, `.`-only rejected, empty segment rejected, depth measured post-normalization). All six validation commands pass: 196 unit + 20 doctests. diff --git a/pages/src/core/upload.rs b/pages/src/core/upload.rs index 9b38bf4..538fc90 100644 --- a/pages/src/core/upload.rs +++ b/pages/src/core/upload.rs @@ -370,8 +370,9 @@ fn build_zip_manifest( continue; } - let name = entry.name().to_string(); - check_entry_path(&name, limits)?; + // `check_entry_path` returns the normalized path (`.` segments + // stripped) — that normalized form is what we store and dedup on. + let name = check_entry_path(entry.name(), limits)?; if !seen.insert(name.clone()) { return Err(UploadError::DuplicateEntry { path: name }); @@ -509,15 +510,18 @@ fn read_tar_entries( // OS-specific separator rewriting) — `check_entry_path` must see the // exact stored name to make its traversal decision. let name_bytes = entry.path_bytes(); - let name = match std::str::from_utf8(&name_bytes) { - Ok(s) => s.to_string(), + let raw_name = match std::str::from_utf8(&name_bytes) { + Ok(s) => s, Err(_) => { return Err(UploadError::PathTraversal { path: String::from_utf8_lossy(&name_bytes).into_owned(), }) } }; - check_entry_path(&name, limits)?; + // `check_entry_path` returns the normalized path (`.` segments + // stripped) — GNU tar's `./`-prefixed names normalize to their plain + // form, which is what we store and dedup on. + let name = check_entry_path(raw_name, limits)?; if !seen.insert(name.clone()) { return Err(UploadError::DuplicateEntry { path: name }); @@ -617,14 +621,25 @@ impl std::io::Write for LimitedWriter { } } -/// Validates a raw zip entry name against the path-traversal and -/// length/depth rules from design §2.2. +/// Validates a raw archive entry name against the path-traversal and +/// length/depth rules from design §2.2, returning the **normalized** path to +/// store as the R2 object key suffix. /// -/// Rejects: empty names, names over `max_path_len`, names containing a -/// backslash or NUL byte, names starting with `/` (absolute), names -/// starting with a drive letter (`C:...`), and any `.` or `..` or empty -/// path segment. Also enforces `max_depth` on the segment count. -fn check_entry_path(name: &str, limits: &UploadLimits) -> Result<(), UploadError> { +/// A `.` (current-directory) path segment is a no-op, so it is *normalized +/// away* rather than rejected: `"./index.html"` becomes `"index.html"` and +/// `"assets/./app.js"` becomes `"assets/app.js"`. This matters because GNU +/// `tar` conventionally prefixes every entry with `./` when archiving a +/// directory (`tar czf out.tgz .` yields `./index.html`, `./bin_bg.wasm`, +/// …) — without this, such tarballs would be rejected outright and their +/// `./index.html` would never satisfy the root-`index.html` requirement. +/// Stripping `.` is safe: unlike `..`, it never escapes the page root. +/// +/// Rejects: empty names, names over `max_path_len` (checked on the raw name), +/// names containing a backslash or NUL byte, names starting with `/` +/// (absolute), names starting with a drive letter (`C:...`), any `..` or +/// empty (`//`) path segment, and a name that normalizes to nothing (e.g. +/// `"."` or `"./"`). Enforces `max_depth` on the *normalized* segment count. +fn check_entry_path(name: &str, limits: &UploadLimits) -> Result { if name.is_empty() { return Err(UploadError::PathTraversal { path: name.to_string(), @@ -654,22 +669,35 @@ fn check_entry_path(name: &str, limits: &UploadLimits) -> Result<(), UploadError }); } - let segments: Vec<&str> = name.split('/').collect(); - if segments.len() > limits.max_depth { - return Err(UploadError::PathTooDeep { - path: name.to_string(), - max: limits.max_depth, - }); - } - for seg in &segments { - if seg.is_empty() || *seg == "." || *seg == ".." { + // Normalize: drop `.` segments (harmless current-directory no-ops); reject + // `..` (traversal) and empty (`//`) segments. + let mut normalized: Vec<&str> = Vec::new(); + for seg in name.split('/') { + if seg == "." { + continue; + } + if seg.is_empty() || seg == ".." { return Err(UploadError::PathTraversal { path: name.to_string(), }); } + normalized.push(seg); + } + + // A name consisting only of `.`/`./` segments carries no real path. + if normalized.is_empty() { + return Err(UploadError::PathTraversal { + path: name.to_string(), + }); + } + if normalized.len() > limits.max_depth { + return Err(UploadError::PathTooDeep { + path: name.to_string(), + max: limits.max_depth, + }); } - Ok(()) + Ok(normalized.join("/")) } /// Chunk size used by [`read_entry_limited`]'s streaming read loop. @@ -1381,6 +1409,27 @@ mod tests { assert!(matches!(err, UploadError::TotalTooLarge { .. })); } + /// GNU-tar-style `./`-prefixed entries (as produced by `tar czf out.tgz + /// .`) are accepted and stored under their normalized, `./`-stripped + /// paths — including a `.wasm` file, which is fully supported. The + /// `./index.html` entry must satisfy the root-level `index.html` + /// requirement, and an interior `assets/./app.js` normalizes too. + fn check_tar_dot_slash_prefix_normalized(comp: Compression) { + let tar = make_tar( + comp, + &[ + ("./", b""), + ("./index.html", b""), + ("./bin_bg.wasm", b"\0asm"), + ("assets/./app.js", b"x"), + ], + ); + let manifest = build_manifest(UploadKind::Tar(comp), tar, &limits()).unwrap(); + let mut paths: Vec<&str> = manifest.iter().map(|e| e.path.as_str()).collect(); + paths.sort(); + assert_eq!(paths, ["assets/app.js", "bin_bg.wasm", "index.html"]); + } + // ── gzip ───────────────────────────────────────────────────────────── #[test] @@ -1413,6 +1462,11 @@ mod tests { check_tar_rejects_total_over_limit(Compression::Gzip); } + #[test] + fn targz_dot_slash_prefix_normalized() { + check_tar_dot_slash_prefix_normalized(Compression::Gzip); + } + /// A gzip "bomb": a single highly-compressible entry whose declared /// (and real) uncompressed size dwarfs a tight per-file limit. Because /// gzip decodes lazily as the tar entry is read, the streaming guard @@ -1469,6 +1523,11 @@ mod tests { check_tar_rejects_total_over_limit(Compression::Zstd); } + #[test] + fn tarzst_dot_slash_prefix_normalized() { + check_tar_dot_slash_prefix_normalized(Compression::Zstd); + } + /// A zstd "bomb": as with gzip, `ruzstd`'s `StreamingDecoder` decodes /// lazily, so the streaming guard aborts the read long before the whole /// entry is decompressed. @@ -1518,6 +1577,11 @@ mod tests { check_tar_rejects_total_over_limit(Compression::Xz); } + #[test] + fn tarxz_dot_slash_prefix_normalized() { + check_tar_dot_slash_prefix_normalized(Compression::Xz); + } + /// The xz "bomb" test, exercising the one-shot path's [`LimitedWriter`] /// specifically: xz can only decode the whole stream at once, so the /// guarantee is upheld by the writer aborting decompression once its @@ -1701,4 +1765,84 @@ mod tests { let zip = make_zip(&[("index.html", b"hi")]); assert_eq!(detect_kind(&zip, ""), UploadKind::Zip); } + + // ── path normalization (check_entry_path) ─────────────────────────── + + #[test] + fn check_entry_path_strips_leading_dot_slash() { + assert_eq!( + check_entry_path("./bin_bg.wasm", &limits()).unwrap(), + "bin_bg.wasm" + ); + assert_eq!( + check_entry_path("./index.html", &limits()).unwrap(), + "index.html" + ); + } + + #[test] + fn check_entry_path_strips_interior_dot_segments() { + assert_eq!( + check_entry_path("assets/./app.js", &limits()).unwrap(), + "assets/app.js" + ); + assert_eq!( + check_entry_path("./a/./b/./c.txt", &limits()).unwrap(), + "a/b/c.txt" + ); + } + + #[test] + fn check_entry_path_leaves_plain_paths_unchanged() { + assert_eq!( + check_entry_path("assets/app.js", &limits()).unwrap(), + "assets/app.js" + ); + } + + #[test] + fn check_entry_path_still_rejects_dotdot() { + // `..` must never be normalized away — it escapes the page root. + assert!(matches!( + check_entry_path("../evil", &limits()), + Err(UploadError::PathTraversal { .. }) + )); + assert!(matches!( + check_entry_path("a/../b", &limits()), + Err(UploadError::PathTraversal { .. }) + )); + } + + #[test] + fn check_entry_path_rejects_names_that_normalize_to_nothing() { + // "." / "./" carry no real path once the `.` segments are stripped. + for name in [".", "./", "././"] { + assert!( + matches!( + check_entry_path(name, &limits()), + Err(UploadError::PathTraversal { .. }) + ), + "expected {name:?} to be rejected" + ); + } + } + + #[test] + fn check_entry_path_still_rejects_empty_segments() { + assert!(matches!( + check_entry_path("assets//evil.js", &limits()), + Err(UploadError::PathTraversal { .. }) + )); + } + + #[test] + fn check_entry_path_depth_measured_after_normalization() { + // Leading `./` no-ops must not count against max_depth: a path whose + // real depth is within the limit is accepted despite extra `.` + // segments padding the raw split. + let max = limits().max_depth; + let real = format!("{}x.txt", "a/".repeat(max - 1)); // exactly max segments + let padded = format!("./{real}"); + assert_eq!(check_entry_path(&padded, &limits()).unwrap(), real); + } }