diff --git a/crates/socket-patch-cli/tests/common/cache_env.rs b/crates/socket-patch-cli/tests/common/cache_env.rs index 8ba1f5879..d01fd7e4b 100644 --- a/crates/socket-patch-cli/tests/common/cache_env.rs +++ b/crates/socket-patch-cli/tests/common/cache_env.rs @@ -59,6 +59,7 @@ #![allow(dead_code)] +use std::ffi::OsString; use std::path::PathBuf; use std::process::Command; @@ -285,8 +286,22 @@ pub fn is_ambient_bun_var(name: &str) -> bool { /// rule in the module docs: the scrub iterates the parent env and would /// otherwise remove what those seed). pub fn scrub_ambient_bun_env(cmd: &mut Command) { - for (k, _) in std::env::vars_os() { - if is_ambient_bun_var(&k.to_string_lossy()) { + scrub_parent_vars(cmd, parent_var_names(), is_ambient_bun_var); +} + +/// The names of the PARENT environment, read once. +fn parent_var_names() -> Vec { + std::env::vars_os().map(|(k, _)| k).collect() +} + +/// Remove every name in `parent` that `ambient` matches from `cmd`. The +/// scrubs and their self-tests share it so a self-test can check the +/// result against the same snapshot of the parent env the scrub saw: +/// in-process tests in the same binary mirror flags into the env +/// (`--offline` sets `SOCKET_OFFLINE`), so two separate reads race. +fn scrub_parent_vars(cmd: &mut Command, parent: Vec, ambient: fn(&str) -> bool) { + for k in parent { + if ambient(&k.to_string_lossy()) { cmd.env_remove(&k); } } @@ -316,11 +331,7 @@ pub fn is_ambient_vlt_var(name: &str) -> bool { /// Remove every [`is_ambient_vlt_var`] variable of the PARENT environment /// from `cmd` (call it before [`isolate`], see the module docs). pub fn scrub_ambient_vlt_env(cmd: &mut Command) { - for (k, _) in std::env::vars_os() { - if is_ambient_vlt_var(&k.to_string_lossy()) { - cmd.env_remove(&k); - } - } + scrub_parent_vars(cmd, parent_var_names(), is_ambient_vlt_var); } // ── Self-tests ──────────────────────────────────────────────────────── @@ -373,8 +384,9 @@ mod cache_env_selftests { /// nothing else is touched, and no matching parent variable is missed. #[test] fn ambient_bun_scrub_removes_only_matching_parent_vars() { + let parent = parent_var_names(); let mut cmd = Command::new("true"); - scrub_ambient_bun_env(&mut cmd); + scrub_parent_vars(&mut cmd, parent.clone(), is_ambient_bun_var); let removed: Vec = cmd .get_envs() .filter(|(_, value)| value.is_none()) @@ -386,7 +398,7 @@ mod cache_env_selftests { "{name} was removed but is not an ambient bun var" ); } - for (name, _) in std::env::vars_os() { + for name in &parent { let name = name.to_string_lossy(); if is_ambient_bun_var(&name) { assert!( @@ -435,8 +447,9 @@ mod cache_env_selftests { #[test] fn ambient_vlt_scrub_removes_only_matching_parent_vars() { + let parent = parent_var_names(); let mut cmd = Command::new("true"); - scrub_ambient_vlt_env(&mut cmd); + scrub_parent_vars(&mut cmd, parent.clone(), is_ambient_vlt_var); let removed: Vec = cmd .get_envs() .filter(|(_, value)| value.is_none()) @@ -445,7 +458,7 @@ mod cache_env_selftests { for name in &removed { assert!(is_ambient_vlt_var(name), "{name} was removed"); } - for (name, _) in std::env::vars_os() { + for name in &parent { let name = name.to_string_lossy(); if is_ambient_vlt_var(&name) { assert!(removed.iter().any(|r| *r == name), "{name} kept"); diff --git a/crates/socket-patch-cli/tests/mode_migration_pypi.rs b/crates/socket-patch-cli/tests/mode_migration_pypi.rs index 02197d8f0..95f773f11 100644 --- a/crates/socket-patch-cli/tests/mode_migration_pypi.rs +++ b/crates/socket-patch-cli/tests/mode_migration_pypi.rs @@ -3096,3 +3096,79 @@ async fn pipenv_residual_export_takeover_refusal_names_the_export() { "the takeover reclaims the vendored wheel: {env:#}" ); } + +/// The checkout directory names #547 reproduced. Hatch expands `{root:uri}` +/// with hatchling's `path_to_uri`, which escapes only spaces, so pip reads +/// `%XX` as an escape, `#` as a fragment, `?` as a query, and the PEP 508 +/// parser rejects `;`. (`?` cannot be a Windows file name.) +fn unsafe_hatch_roots() -> Vec<&'static str> { + let mut names = vec!["app_feature%2Fsix-patch", "p%20q", "a#b", "semi;colon"]; + if cfg!(unix) { + names.push("q?x"); + } + names +} + +/// #547: vendoring a Hatch project whose checkout path contains a character +/// Hatch's `{root:uri}` leaves unescaped is refused before anything is +/// written, so the scan no longer reports success for wiring no Hatch env +/// can install. Paths Hatch does expand correctly (`@`, `'`, a space) still +/// vendor. +#[tokio::test] +async fn hatch_vendored_refuses_a_root_hatch_cannot_expand() { + for name in unsafe_hatch_roots() { + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path().join(name); + std::fs::create_dir_all(&root).unwrap(); + let files = stage_hatch(&root); + let original = std::fs::read_to_string(root.join(files[0])).unwrap(); + stage_manifest(&root); + let (code, env) = run_cli(&root, &["vendor"], &[]); + assert_eq!(code, 1, "{name}: {env:#}"); + let rendered = env.to_string(); + assert!( + rendered.contains("pypi_hatch_unsupported") && rendered.contains("{root:uri}"), + "{name}: the root-path guard is the reported refusal: {env:#}" + ); + assert_eq!( + std::fs::read_to_string(root.join(files[0])).unwrap(), + original, + "{name}: pyproject.toml untouched" + ); + assert!( + !root.join(".socket/vendor/state.json").exists(), + "{name}: nothing recorded" + ); + } + for name in ["at@sign", "quo'te", "sp ace"] { + let tmp = tempfile::tempdir().unwrap(); + let root = tmp.path().join(name); + std::fs::create_dir_all(&root).unwrap(); + let files = stage_hatch(&root); + vendor_project(&root, files); + } +} + +/// #547: a vendored Hatch checkout moved to a path Hatch cannot expand +/// (a Jenkins multibranch workspace such as `app_feature%2Fsix-patch`) +/// fails the next `vendor` run instead of passing it as in sync, and the +/// committed wiring is left as it was. +#[tokio::test] +async fn hatch_vendored_rerun_refuses_after_relocation_to_unsafe_root() { + let (tmp, root) = project(); + let files = stage_hatch(&root); + vendor_project(&root, files); + let wired = std::fs::read_to_string(root.join(files[0])).unwrap(); + let moved = tmp.path().join("app_feature%2Fsix-patch"); + std::fs::rename(&root, &moved).unwrap(); + let (code, env) = run_cli(&moved, &["vendor"], &[]); + assert_eq!(code, 1, "{env:#}"); + assert!( + env.to_string().contains("pypi_hatch_unsupported"), + "{env:#}" + ); + assert_eq!( + std::fs::read_to_string(moved.join(files[0])).unwrap(), + wired + ); +} diff --git a/crates/socket-patch-core/src/utils/hatch.rs b/crates/socket-patch-core/src/utils/hatch.rs index 6ba38b4c2..b1cfea9e0 100644 --- a/crates/socket-patch-core/src/utils/hatch.rs +++ b/crates/socket-patch-core/src/utils/hatch.rs @@ -11,6 +11,39 @@ use crate::vendor::common::{pep508_name, pyproject_dependency_specs, DeclTable}; /// exactly these from the candidate set, so the lists cannot drift. pub const HATCH_FILES: [&str; 2] = ["pyproject.toml", "hatch.toml"]; +/// The first character of `root`'s path that Hatch's `{root:uri}` expands +/// into a broken file URL (#547). Hatchling's `path_to_uri` escapes only +/// the space, so pip percent-decodes a `%XX` escape, cuts the path at a +/// `#` fragment or `?` query, and the PEP 508 parser stops at a `;`, a tab +/// or any other whitespace or control character. Hatch builds the URL from +/// the real path (symlinks resolved), so that is the path checked; only its +/// named components are, so a Windows `\\?\` prefix never counts. `None` +/// when the expansion is a usable URL. +pub fn root_uri_unsafe_char(root: &std::path::Path) -> Option { + let real = std::fs::canonicalize(root) + .or_else(|_| std::path::absolute(root)) + .unwrap_or_else(|_| root.to_path_buf()); + real.components() + .filter_map(|component| match component { + std::path::Component::Normal(name) => Some(name.to_string_lossy()), + _ => None, + }) + .find_map(|name| { + let chars: Vec = name.chars().collect(); + chars.iter().enumerate().find_map(|(i, &c)| { + let escape = c == '%' + && chars + .get(i + 1..i + 3) + .is_some_and(|hex| hex.iter().all(char::is_ascii_hexdigit)); + (escape + || matches!(c, '#' | '?' | ';') + || c.is_control() + || (c.is_whitespace() && c != ' ')) + .then_some(c) + }) + }) +} + pub fn is_hatch(files: &BTreeMap) -> bool { files.contains_key("hatch.toml") || files.get("pyproject.toml").is_some_and(|text| { diff --git a/crates/socket-patch-core/src/vendor/pypi_hatch.rs b/crates/socket-patch-core/src/vendor/pypi_hatch.rs index 36517ac44..ec7f35b47 100644 --- a/crates/socket-patch-core/src/vendor/pypi_hatch.rs +++ b/crates/socket-patch-core/src/vendor/pypi_hatch.rs @@ -41,6 +41,7 @@ pub(super) async fn load( ) -> Result { let files = read_files(root).await?; require_pip_installer()?; + require_expandable_root(root)?; let prefix = format!("{{root:uri}}/.socket/vendor/pypi/{uuid}/"); let state = super::state::load_state_shared(root) .await @@ -107,13 +108,15 @@ pub(super) async fn load( } /// The guards vendoring applies whatever the project's wiring: the pip -/// installer (environment variable and environment settings), and -/// Hatch >=1.2 when `name` is an environment dependency. Checked before a +/// installer (environment variable and environment settings), a project +/// path `{root:uri}` can expand, and Hatch >=1.2 when `name` is an +/// environment dependency. Checked before a /// superseded patch's wiring is unwound, so a refusal unrelated to that /// wiring never touches the project's files. pub(super) async fn preflight(root: &Path, name: &str) -> Result<(), Failure> { let files = read_files(root).await?; require_pip_installer()?; + require_expandable_root(root)?; hatch::require_pip_installer(&files).map_err(|error| ("pypi_hatch_unsupported", error))?; if hatch::has_environment_dependency(&files, name) { require_environment_context_support(root).await?; @@ -121,6 +124,20 @@ pub(super) async fn preflight(root: &Path, name: &str) -> Result<(), Failure> { Ok(()) } +/// Refuse a project root Hatch's `{root:uri}` cannot expand into a usable +/// file URL (#547): every vendored reference would fail to install there. +fn require_expandable_root(root: &Path) -> Result<(), Failure> { + match hatch::root_uri_unsafe_char(root) { + Some(c) => Err(( + "pypi_hatch_unsupported", + format!( + "the project path contains {c:?}, which Hatch's {{root:uri}} does not escape, so vendored references would not install from this checkout; move the checkout to a path without it, or use hosted or agent mode" + ), + )), + None => Ok(()), + } +} + fn require_pip_installer() -> Result<(), Failure> { if std::env::var("HATCH_ENV_TYPE_VIRTUAL_UV_PATH").is_ok_and(|path| !path.is_empty()) { return Err(("pypi_hatch_unsupported", "vendored Hatch wheels require the pip installer: uv does not enforce local wheel fragment hashes".into())); @@ -439,6 +456,66 @@ mod tests { assert!(held(format!("\u{feff}\u{feff}{live}"))); } + /// A symlink in `dir` to `target` (Unix), else `target` itself. + fn linked(dir: &Path, target: &Path) -> std::path::PathBuf { + #[cfg(unix)] + { + let link = dir.join("link"); + std::os::unix::fs::symlink(target, &link).unwrap(); + link + } + #[cfg(not(unix))] + { + let _ = dir; + target.to_path_buf() + } + } + + /// #547: `load` and `preflight` refuse a project root Hatch's + /// `{root:uri}` expands into a broken URL, and accept the characters it + /// handles (`@`, `'`, a space). + #[tokio::test] + async fn root_hatch_cannot_expand_is_refused() { + let mut unsafe_names = vec!["feature%2Fx", "p%20q", "a#b", "semi;colon"]; + if cfg!(unix) { + unsafe_names.extend(["q?x", "tab\tbed"]); + } + for name in unsafe_names { + let temp = tempfile::tempdir().unwrap(); + let root = temp.path().join(name); + tokio::fs::create_dir_all(&root).await.unwrap(); + tokio::fs::write(root.join("pyproject.toml"), ORIGINAL) + .await + .unwrap(); + let Err((code, detail)) = load(&root, "one", "1", UUID).await else { + panic!("{name}: load must refuse"); + }; + assert_eq!(code, "pypi_hatch_unsupported", "{name}"); + assert!(detail.contains("{root:uri}"), "{name}: {detail}"); + let Err((_, through_link)) = load(&linked(temp.path(), &root), "one", "1", UUID).await + else { + panic!("{name}: a symlink to the checkout must not hide its real path"); + }; + assert_eq!(through_link, detail, "{name}"); + assert_eq!( + preflight(&root, "one").await.map_err(|(code, _)| code), + Err("pypi_hatch_unsupported"), + "{name}" + ); + } + // pip leaves a `%` that starts no `%XX` escape as it is. + for name in ["at@sign", "quo'te", "sp ace", "100%", "50%off"] { + let temp = tempfile::tempdir().unwrap(); + let root = temp.path().join(name); + tokio::fs::create_dir_all(&root).await.unwrap(); + tokio::fs::write(root.join("pyproject.toml"), ORIGINAL) + .await + .unwrap(); + assert!(load(&root, "one", "1", UUID).await.is_ok(), "{name}"); + assert!(preflight(&root, "one").await.is_ok(), "{name}"); + } + } + #[tokio::test] async fn recorded_pin_drift_missing_artifact_and_ledgerless_source() { let temp = tempfile::tempdir().unwrap(); diff --git a/docs/testing/hatch.md b/docs/testing/hatch.md index 0dd27b61e..248089223 100644 --- a/docs/testing/hatch.md +++ b/docs/testing/hatch.md @@ -5,7 +5,11 @@ PEP 508 declarations in `project.dependencies`, optional dependencies, and Hatch environment `dependencies` / `extra-dependencies`. External `hatch.toml` tables override the corresponding top-level `tool.hatch` keys. Project references enable Hatchling's `allow-direct-references` setting. -Vendored references use `{root:uri}` so checkouts remain relocatable. +Vendored references use `{root:uri}` so checkouts remain relocatable, +except to a path containing a `%XX` escape, `#`, `?`, `;`, or whitespace +other than a space: Hatch's `{root:uri}` escapes only spaces, so pip reads +the expanded URL as an escape, fragment or query, or rejects it (#547). A vendored scan or `vendor` run from such a path +is refused before writing; hosted and agent mode are unaffected. Vendored environment references require Hatch >=1.2 on PATH; preflight verifies the installed version because Hatch 1.0 and 1.1 do not expand that context. Vendored Hatch requires the pip installer; uv currently ignores hash