From 6c77fa4c566bda475775ad11cf77d4049b5666ab Mon Sep 17 00:00:00 2001 From: Cody De Arkland Date: Wed, 5 Aug 2026 15:23:21 +0000 Subject: [PATCH 1/2] fix(up): skip unresolvable paths instead of failing the whole upload MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A dangling symlink anywhere in the tree aborted `railway up` during indexing with "No such file or directory", creating no deployment. An exact-path `.railwayignore` rule could not exclude it: `follow_links(true)` makes the walker stat the symlink target before the ignore rules are consulted for that entry, so the error is raised before matching happens. Treat a walk entry whose error is `NotFound` as skippable — a broken link, or a file removed mid-walk, has no contents to upload — and report it through `reporter::warn` so it is structured under `--json`. Any other error still fails the deploy, since that would mean a silently incomplete tarball. Validate the root path up front: it reaches the walker as the same `NotFound`, and no caller checks it, so skipping it would turn a typo'd path into an empty but successful deploy. --- src/controllers/upload.rs | 224 +++++++++++++++++++++++++++++++++++++- 1 file changed, 222 insertions(+), 2 deletions(-) diff --git a/src/controllers/upload.rs b/src/controllers/upload.rs index 907ed65a5..5acc00ad1 100644 --- a/src/controllers/upload.rs +++ b/src/controllers/upload.rs @@ -1,7 +1,7 @@ use std::path::Path; use std::sync::{Arc, Mutex}; -use anyhow::Result; +use anyhow::{Context, Result}; use gzp::{ZBuilder, deflate::Gzip}; use ignore::WalkBuilder; use reqwest::Client; @@ -26,6 +26,22 @@ pub struct UpErrorResponse { pub message: String, } +/// A walk error that means "this entry has no contents to upload", rather than a +/// condition that should stop the upload. +/// +/// `follow_links(true)` makes the walker stat a symlink's target before the ignore +/// rules are consulted for that entry, so a dangling link surfaces here as +/// `NotFound` — the reason an exact-path `.railwayignore` rule cannot exclude one. +/// A file deleted mid-walk by a concurrent build lands here too; both are +/// legitimately skippable. +/// +/// Only `NotFound` qualifies. A permission or genuine I/O error on a real entry +/// would mean a silently incomplete tarball, which is worth failing the deploy over. +fn is_missing_path(err: &ignore::Error) -> bool { + err.io_error() + .is_some_and(|io| io.kind() == std::io::ErrorKind::NotFound) +} + /// Create a gzipped tarball from a project directory, respecting .railwayignore and .gitignore. /// /// `on_progress` is called with `(current, total)` after each entry is processed. @@ -36,6 +52,17 @@ pub fn create_deploy_tarball( no_gitignore: bool, mut on_progress: impl FnMut(usize, usize), ) -> Result> { + // The root reaches the walker as the same `NotFound` that `is_missing_path` + // skips, and no caller validates it — so a typo'd path would upload an empty + // but *successful* deploy. `metadata` follows symlinks, rejecting a dangling + // root link too. Existence is all that is checked: `up ` is supported. + std::fs::metadata(project_path).with_context(|| { + format!( + "Failed to read `{}` for upload: no such path", + project_path.display() + ) + })?; + let bytes = Vec::::new(); let arc = Arc::new(Mutex::new(bytes)); let mut parz = ZBuilder::::new() @@ -60,7 +87,26 @@ pub fn create_deploy_tarball( on_progress(0, total); for (i, entry) in walked.into_iter().enumerate() { - let entry = entry?; + let entry = match entry { + Ok(entry) => entry, + // Skipping beats aborting the whole upload: see `is_missing_path`. + Err(err) if is_missing_path(&err) => { + // Prefer the bare path: the error's own Display already embeds + // it inside an io description, so interpolating the error would + // print the path twice. + let what = match &err { + ignore::Error::WithPath { path, .. } => path.display().to_string(), + other => other.to_string(), + }; + crate::util::reporter::warn( + "UNREADABLE_PATH_SKIPPED", + format!("skipping unreadable path: {what}"), + Some("a broken symlink, or a file removed while indexing"), + ); + continue; + } + Err(err) => return Err(err.into()), + }; let path = entry.path(); if path .components() @@ -148,3 +194,177 @@ pub async fn upload_deploy_tarball( let response = res.json::().await?; Ok(response) } + +#[cfg(test)] +mod tests { + use super::*; + use std::io::Read; + + /// Sorted archive member names, so tests assert on what was actually + /// captured rather than merely on success. + fn entries_of(root: &Path) -> Vec { + let tarball = tarball_of(root).expect("tarball"); + let mut decoder = flate2::read::GzDecoder::new(tarball.as_slice()); + let mut decompressed = Vec::new(); + decoder.read_to_end(&mut decompressed).unwrap(); + let mut archive = tar::Archive::new(decompressed.as_slice()); + let mut names: Vec = archive + .entries() + .unwrap() + .map(|entry| entry.unwrap().path().unwrap().display().to_string()) + .collect(); + names.sort(); + names + } + + /// The fallible form, for the tests that assert the upload is refused. + fn tarball_of(root: &Path) -> Result> { + create_deploy_tarball(root, root, false, |_, _| {}) + } + + #[cfg(unix)] + fn dangling(path: &Path) { + std::os::unix::fs::symlink("../does-not-exist", path).unwrap(); + } + + /// A dangling symlink anywhere in the tree used to abort the whole upload with + /// `No such file or directory`. Links are skipped; every real file still ships. + #[cfg(unix)] + #[test] + fn dangling_symlinks_are_skipped_instead_of_failing_the_upload() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + std::fs::write(root.join("main.py"), b"print('hi')").unwrap(); + for sub in ["backend", "worker"] { + std::fs::create_dir(root.join(sub)).unwrap(); + std::fs::write(root.join(sub).join("real.txt"), b"x").unwrap(); + dangling(&root.join(sub).join("AGENTS.md")); + } + + let entries = entries_of(root); + assert!( + entries.iter().any(|name| name.ends_with("main.py")), + "real files must still be uploaded, got {entries:?}" + ); + assert_eq!( + entries.iter().filter(|n| n.ends_with("real.txt")).count(), + 2, + "files beside a broken link must survive, got {entries:?}" + ); + assert!( + !entries.iter().any(|name| name.ends_with("AGENTS.md")), + "unresolvable symlinks must not be in the tarball, got {entries:?}" + ); + } + + /// The reported CI shape: an exact-path `.railwayignore` rule alongside the + /// dangling link. Asserts the rule is honored for a real file too, so this + /// covers the ignore path and not just the skip branch. + #[cfg(unix)] + #[test] + fn railwayignore_rules_apply_when_a_dangling_symlink_is_present() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + std::fs::create_dir(root.join("backend")).unwrap(); + std::fs::write(root.join("main.py"), b"print('hi')").unwrap(); + std::fs::write(root.join("backend/secret.txt"), b"nope").unwrap(); + dangling(&root.join("backend/AGENTS.md")); + std::fs::write( + root.join(".railwayignore"), + b"/backend/AGENTS.md\n/backend/secret.txt\n", + ) + .unwrap(); + + let entries = entries_of(root); + assert!(entries.iter().any(|name| name.ends_with("main.py"))); + assert!( + !entries.iter().any(|name| name.ends_with("secret.txt")), + "an ignored real file must be excluded, got {entries:?}" + ); + assert!(!entries.iter().any(|name| name.ends_with("AGENTS.md"))); + } + + /// A symlink to a real file must still be followed and uploaded, and a + /// symlinked directory still traversed — `follow_links(false)` would have been + /// the tempting fix and would regress both (see #296). + #[cfg(unix)] + #[test] + fn resolvable_symlinks_are_still_followed() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + std::fs::write(root.join("real.txt"), b"content").unwrap(); + std::os::unix::fs::symlink("real.txt", root.join("link.txt")).unwrap(); + std::fs::create_dir(root.join("realdir")).unwrap(); + std::fs::write(root.join("realdir/inner.txt"), b"x").unwrap(); + std::os::unix::fs::symlink("realdir", root.join("linkdir")).unwrap(); + + let entries = entries_of(root); + assert!( + entries.iter().any(|name| name.ends_with("link.txt")), + "symlink to a real file must be uploaded, got {entries:?}" + ); + assert!( + entries + .iter() + .any(|n| n.contains("linkdir") && n.ends_with("inner.txt")), + "symlinked directory must be traversed, got {entries:?}" + ); + } + + /// A symlink loop reports ELOOP, not `NotFound`, so the skip must not swallow it. + #[cfg(unix)] + #[test] + fn symlink_loop_stays_fatal() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path(); + std::fs::write(root.join("keep.txt"), b"x").unwrap(); + std::os::unix::fs::symlink(root.join("loop_a"), root.join("loop_b")).unwrap(); + std::os::unix::fs::symlink(root.join("loop_b"), root.join("loop_a")).unwrap(); + assert!( + tarball_of(root).is_err(), + "a symlink loop is not a broken link and must stay fatal" + ); + } + + /// A missing root reaches the walker as the same `NotFound` the skip branch + /// swallows, so without an explicit check `up ./typo` would deploy nothing and + /// report success. + #[test] + fn missing_root_must_be_an_error() { + let dir = tempfile::tempdir().unwrap(); + let err = tarball_of(&dir.path().join("does-not-exist")) + .expect_err("a missing root must not deploy successfully") + .to_string(); + assert!( + err.contains("no such path"), + "error should name the missing path, got: {err}" + ); + } + + /// Same trap via a root that exists as a link but does not resolve — this is + /// what breaks if the root check is switched to `symlink_metadata`. + #[cfg(unix)] + #[test] + fn dangling_symlink_as_the_root_must_be_an_error() { + let dir = tempfile::tempdir().unwrap(); + let root = dir.path().join("rootlink"); + std::os::unix::fs::symlink("nowhere", &root).unwrap(); + assert!( + tarball_of(&root).is_err(), + "a dangling root symlink must not produce a successful empty deploy" + ); + } + + /// The root check must not over-correct: an empty directory is legitimately + /// empty, and `up ` is a supported shape. + #[test] + fn empty_directory_and_single_file_roots_are_accepted() { + let dir = tempfile::tempdir().unwrap(); + tarball_of(dir.path()).expect("an empty project directory is not an error"); + + let file = dir.path().join("solo.txt"); + std::fs::write(&file, b"x").unwrap(); + create_deploy_tarball(&file, dir.path(), false, |_, _| {}) + .expect("a file root must still be uploadable"); + } +} From 250d9e90b53a2a046ac5d5141fe721d05b135dd9 Mon Sep 17 00:00:00 2001 From: Cody De Arkland Date: Wed, 5 Aug 2026 15:28:10 +0000 Subject: [PATCH 2/2] test(up): gate the Unix-only tarball helper so Windows builds stay warning-free --- src/controllers/upload.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/controllers/upload.rs b/src/controllers/upload.rs index 5acc00ad1..be9ca017b 100644 --- a/src/controllers/upload.rs +++ b/src/controllers/upload.rs @@ -198,11 +198,14 @@ pub async fn upload_deploy_tarball( #[cfg(test)] mod tests { use super::*; - use std::io::Read; /// Sorted archive member names, so tests assert on what was actually - /// captured rather than merely on success. + /// captured rather than merely on success. Only the symlink cases inspect + /// contents, and those are Unix-only. + #[cfg(unix)] fn entries_of(root: &Path) -> Vec { + use std::io::Read; + let tarball = tarball_of(root).expect("tarball"); let mut decoder = flate2::read::GzDecoder::new(tarball.as_slice()); let mut decompressed = Vec::new();