From 487907adfb1369cfe0796a8836a235adf4c64081 Mon Sep 17 00:00:00 2001 From: aiongg <67543387+aiongg@users.noreply.github.com> Date: Fri, 23 Oct 2026 13:02:41 -0400 Subject: [PATCH] fix(links): resolve remote document paths with craft-docpath's resolve_absolute (#3730) * fix(links): use resolve_absolute for remote document paths * fix(ui): refuse output file names that end in a dot or a space * ci: check that git dependencies come from storytold * ci: release only with git dependencies from storytold * ci: check git dependency sources on a locked, parsed Cargo.lock The git-sources step in ci.yml and release.yml fails when Cargo.lock is missing, when .cargo/config.toml replaces or patches a source, when Cargo.lock is out of date with the manifests (cargo metadata --locked), and when any package source parsed from Cargo.lock as TOML is a git source outside github.com/storytold/. A grep on Cargo's usual spelling missed single-quoted strings and stale lockfiles. * fix(links): a Windows path on Unix is refused, not reported as needing a source resolve_remote_document_path reported MissingSourcePath for any path that resolved but not to an absolute one. With an absolute source, such a path is one this platform cannot open (C:\x.pdf on Unix): it is now NotLocal(Invalid). A relative target from a missing or relative source still gives MissingSourcePath. * ci: check the legacy .cargo/config for source replacement too Cargo reads .cargo/config as well as .cargo/config.toml, so the git-sources step greps both; grep -s keeps a missing file from failing the step. * ci: parse cargo config for source replacement, patches and path overrides --- .github/workflows/ci.yml | 33 +++++++++++++++++++++++++++++ .github/workflows/release.yml | 40 +++++++++++++++++++++++++++++++++++ crates/engine/src/links.rs | 20 ++++++++++++------ crates/ui-egui/src/files.rs | 21 +++++++++++++----- 4 files changed, 102 insertions(+), 12 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index f94d7dbe40..53ed10344b 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -36,6 +36,39 @@ jobs: steps: - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + - name: Git dependencies come from storytold + if: runner.os == 'Linux' + run: | + set -euo pipefail + test -f Cargo.lock || { echo "Cargo.lock is missing"; exit 1; } + # Source replacement, patches and path overrides in cargo config never show up + # in Cargo.lock. Parsed rather than grepped: TOML has many spellings of one key. + # Cargo still reads the legacy .cargo/config. + python3 - <<'PY' + import os, sys, tomllib + for path in (".cargo/config", ".cargo/config.toml"): + if not os.path.exists(path): + continue + config = tomllib.load(open(path, "rb")) + replaced = [name for name, source in config.get("source", {}).items() + if isinstance(source, dict) and "replace-with" in source] + if replaced or "patch" in config or "paths" in config: + print(f"{path} replaces, patches or overrides a source") + sys.exit(1) + PY + # Fails when Cargo.lock is out of date with the manifests. + cargo metadata --locked --format-version 1 > /dev/null + python3 - <<'PY' + import sys, tomllib + lock = tomllib.load(open("Cargo.lock", "rb")) + bad = sorted({p["source"] for p in lock.get("package", []) + if p.get("source", "").startswith("git+") + and not p["source"].startswith("git+https://github.com/storytold/")}) + if bad: + print("Git dependencies outside github.com/storytold:", *bad, sep="\n") + sys.exit(1) + PY + # The optional craft-fonts build input (Japanese fonts), so the Japanese font tests run on # Linux; Windows tests the build without it. See craftrules standards/fonts.md. - name: Check out craft-fonts (Linux) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index c6ee0e1c31..6c9994ae36 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -18,7 +18,47 @@ concurrency: cancel-in-progress: false jobs: + git-sources: + name: Git dependencies come from storytold + runs-on: ubuntu-latest + permissions: + contents: read + steps: + - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 + - name: Git dependencies come from storytold + run: | + set -euo pipefail + test -f Cargo.lock || { echo "Cargo.lock is missing"; exit 1; } + # Source replacement, patches and path overrides in cargo config never show up + # in Cargo.lock. Parsed rather than grepped: TOML has many spellings of one key. + # Cargo still reads the legacy .cargo/config. + python3 - <<'PY' + import os, sys, tomllib + for path in (".cargo/config", ".cargo/config.toml"): + if not os.path.exists(path): + continue + config = tomllib.load(open(path, "rb")) + replaced = [name for name, source in config.get("source", {}).items() + if isinstance(source, dict) and "replace-with" in source] + if replaced or "patch" in config or "paths" in config: + print(f"{path} replaces, patches or overrides a source") + sys.exit(1) + PY + # Fails when Cargo.lock is out of date with the manifests. + cargo metadata --locked --format-version 1 > /dev/null + python3 - <<'PY' + import sys, tomllib + lock = tomllib.load(open("Cargo.lock", "rb")) + bad = sorted({p["source"] for p in lock.get("package", []) + if p.get("source", "").startswith("git+") + and not p["source"].startswith("git+https://github.com/storytold/")}) + if bad: + print("Git dependencies outside github.com/storytold:", *bad, sep="\n") + sys.exit(1) + PY + release: + needs: git-sources # Intentionally tracked at @main (not SHA-pinned) so org-wide CI improvements, security fixes, and build optimizations propagate without PR churn across all craftapp repos. uses: storytold/craft-common-actions/.github/workflows/release.yml@main with: diff --git a/crates/engine/src/links.rs b/crates/engine/src/links.rs index 4902d72f53..9b1c49729f 100644 --- a/crates/engine/src/links.rs +++ b/crates/engine/src/links.rs @@ -176,7 +176,7 @@ pub fn remote_document_path_is_absolute(raw: &str) -> bool { /// Resolve a PDF remote-Go-To file specification to a local path without opening it. /// Relative paths are based on the source document, which should be an absolute path; an /// in-memory source has no fallback to CWD. URLs (including `file:`), network and device paths are -/// refused ([`craft_docpath::resolve`]). +/// refused ([`craft_docpath::resolve_absolute`]). pub fn resolve_remote_document_path(source: Option<&Path>, raw: &str) -> Result { if raw.is_empty() || raw.trim().is_empty() { return Err(RemoteDocumentPathError::Empty); @@ -197,12 +197,14 @@ pub fn resolve_remote_document_path(source: Option<&Path>, raw: &str) -> Result< return Err(RemoteDocumentPathError::NotLocal(craft_docpath::Refused::Url)); } } - let path = craft_docpath::resolve(raw, source).map_err(RemoteDocumentPathError::NotLocal)?; - // A relative result would be read against the working directory. - if !path.is_absolute() { - return Err(RemoteDocumentPathError::MissingSourcePath); - } - Ok(path) + craft_docpath::resolve_absolute(raw, source).map_err(|r| match r { + // A path that resolves but not to an absolute one needed a source with an absolute path. With + // such a source, it is a path this platform cannot open (`C:\x.pdf` on Unix). + craft_docpath::Refused::Invalid if !source.is_some_and(Path::is_absolute) && craft_docpath::resolve(raw, source).is_ok() => { + RemoteDocumentPathError::MissingSourcePath + } + r => RemoteDocumentPathError::NotLocal(r), + }) } /// Largest linked PDF a GoToR link may open (1 GiB, the largest size the large-file work measures). @@ -512,6 +514,8 @@ mod tests { assert_eq!(resolve(Some(source), "/manual/target.pdf").as_deref(), Ok(Path::new("/manual/target.pdf"))); assert_eq!(resolve(None, "target.pdf"), Err(E::MissingSourcePath)); assert_eq!(resolve(Some(Path::new("source.pdf")), "target.pdf"), Err(E::MissingSourcePath), "a relative source is not used"); + #[cfg(unix)] + assert_eq!(resolve(Some(source), "C:\\x.pdf"), Err(E::NotLocal(craft_docpath::Refused::Invalid)), "a Windows path on Unix needs no source"); assert_eq!(resolve(Some(source), ""), Err(E::Empty)); assert_eq!(resolve(Some(source), " "), Err(E::Empty)); for url in @@ -520,6 +524,8 @@ mod tests { assert!(matches!(resolve(Some(source), url), Err(E::NotLocal(_))), "{url}"); } assert_eq!(resolve(Some(source), "folder/hidden\u{200B}.pdf"), Err(E::Hidden)); + #[cfg(windows)] + assert_eq!(resolve(Some(source), "folder/a:b.pdf"), Err(E::NotLocal(craft_docpath::Refused::Invalid))); assert_eq!(resolve(Some(source), &"a".repeat(super::MAX_REMOTE_DOCUMENT_PATH_BYTES + 1)), Err(E::TooLong)); } diff --git a/crates/ui-egui/src/files.rs b/crates/ui-egui/src/files.rs index d9334d84c8..4a4f5333a8 100644 --- a/crates/ui-egui/src/files.rs +++ b/crates/ui-egui/src/files.rs @@ -217,12 +217,13 @@ pub(crate) fn attachment_file_name(name: &str) -> String { if reserved { format!("_{name}") } else { name.to_string() } } -/// Whether `name` names a file directly inside a folder: not empty, `.` or `..`, with no `/` or -/// `\\`, and no drive (`C:`). On Windows any `:` is refused, which also rules out alternate data -/// streams. +/// Whether `name` names a file directly inside a folder: not empty, with no `/` or `\\`, no drive +/// (`C:`), and not ending in `.` or a space (which also rules out `.` and `..`): Windows drops a +/// trailing dot or space, so `report.` would write `report`. On Windows any `:` is refused, which +/// also rules out alternate data streams. pub(crate) fn is_plain_file_name(name: &str) -> bool { let drive = matches!(name.as_bytes(), [letter, b':', ..] if letter.is_ascii_alphabetic()); - !(name.is_empty() || name == "." || name == ".." || name.contains(['/', '\\']) || drive || (cfg!(windows) && name.contains(':'))) + !(name.is_empty() || name.ends_with(['.', ' ']) || name.contains(['/', '\\']) || drive || (cfg!(windows) && name.contains(':'))) } /// Pages ▸ Rotate Pages. @@ -1084,6 +1085,16 @@ mod stem_tests { } } + #[test] + fn plain_file_names_do_not_end_in_a_dot_or_a_space() { + for good in ["report.pdf", "report", ".hidden", "a b.pdf", "資料.pdf"] { + assert!(is_plain_file_name(good), "{good}"); + } + for bad in ["report.", "report.pdf.", "report.pdf ", "report. . ", " ", "...", ". "] { + assert!(!is_plain_file_name(bad), "{bad:?}"); + } + } + #[test] fn write_files_refuses_names_that_leave_the_folder() { let dir = std::env::temp_dir().join(format!("pdfcraft-write-files-{}", std::process::id())); @@ -1094,7 +1105,7 @@ mod stem_tests { let mut app = crate::PdfCraftApp::new(); app.set_option("language", "en").unwrap(); app.export_dir_override = Some(out.to_string_lossy().into_owned()); - for bad in ["../x.pdf", "..", ".", "", "sub/x.pdf", "sub\\x.pdf", "C:x.pdf", absolute.as_str()] { + for bad in ["../x.pdf", "..", ".", "", "sub/x.pdf", "sub\\x.pdf", "C:x.pdf", "x.pdf.", absolute.as_str()] { assert!(!is_plain_file_name(bad), "{bad}"); // A plain name first: nothing is written when any name in the batch is refused. app.write_files(&[("ok.pdf".into(), std::sync::Arc::new(b"%PDF".to_vec())), (bad.into(), std::sync::Arc::new(b"%PDF".to_vec()))], "t");