diff --git a/crates/socket-patch-cli/tests/scan_requirements_lock_only.rs b/crates/socket-patch-cli/tests/scan_requirements_lock_only.rs index 9ab939607..5feeb532b 100644 --- a/crates/socket-patch-cli/tests/scan_requirements_lock_only.rs +++ b/crates/socket-patch-cli/tests/scan_requirements_lock_only.rs @@ -5,7 +5,10 @@ //! release: //! //! * #523: whitespace around `==` and the legacy `name (==X)` form; -//! * #412: pins reached through in-root `-r` includes. +//! * #412: pins reached through in-root `-r` includes; +//! * #994: include targets pip unquotes (`-r "dev reqs.txt"`, +//! `--requirement="dev.txt"`, `-r dev\ reqs.txt`) or expands +//! (`-r ${REQDIR}/dev.txt`). //! //! Driven through the built binary against a mock patch API; the //! assertion is what discovery sends to the batch endpoint and the @@ -32,7 +35,12 @@ async fn mount_empty_batch(mock: &MockServer) { .await; } -fn run_scan(root: &Path, mock_uri: &str, extra: &[&str]) -> (i32, serde_json::Value) { +fn run_scan( + root: &Path, + mock_uri: &str, + extra: &[&str], + envs: &[(&str, &str)], +) -> (i32, serde_json::Value) { let mut argv = vec![ "scan", "--json", @@ -51,6 +59,7 @@ fn run_scan(root: &Path, mock_uri: &str, extra: &[&str]) -> (i32, serde_json::Va .env("SOCKET_TELEMETRY_DISABLED", "1") .env_remove("VIRTUAL_ENV") .env_remove("CONDA_PREFIX") + .envs(envs.iter().copied()) .output() .expect("run socket-patch"); let stdout = String::from_utf8_lossy(&out.stdout); @@ -87,6 +96,14 @@ async fn batch_purls(mock: &MockServer) -> Vec { } async fn assert_lock_only_discovers(files: &[(&str, &str)], expected: &[&str]) { + assert_lock_only_discovers_with_env(files, &[], expected).await; +} + +async fn assert_lock_only_discovers_with_env( + files: &[(&str, &str)], + envs: &[(&str, &str)], + expected: &[&str], +) { for mode in [&[][..], &["--vendor"][..]] { let mock = MockServer::start().await; mount_empty_batch(&mock).await; @@ -96,7 +113,7 @@ async fn assert_lock_only_discovers(files: &[(&str, &str)], expected: &[&str]) { std::fs::create_dir_all(p.parent().unwrap()).unwrap(); std::fs::write(p, content).unwrap(); } - let (code, v) = run_scan(tmp.path(), &mock.uri(), mode); + let (code, v) = run_scan(tmp.path(), &mock.uri(), mode, envs); assert_eq!(code, 0, "mode={mode:?}: {v}"); assert_eq!( v["lockfileOnlyPackages"].as_u64(), @@ -148,3 +165,45 @@ async fn lock_only_scan_discovers_included_pins() { ) .await; } + +/// #994: pip `shlex`-splits an include line's options, so a quoted or +/// backslash-escaped target names the file without its quotes, and a +/// target with a space is one path, not two words. +#[tokio::test] +async fn lock_only_scan_discovers_quoted_include_targets() { + let cases: &[(&str, &str, &str)] = &[ + ("dq", "-r \"dev reqs.txt\"\n", "dev reqs.txt"), + ("sq", "-r 'dev reqs.txt'\n", "dev reqs.txt"), + ("dq_nospace", "-r \"dev.txt\"\n", "dev.txt"), + ("bs", "-r dev\\ reqs.txt\n", "dev reqs.txt"), + ("longq", "--requirement \"dev.txt\"\n", "dev.txt"), + ("eqq", "--requirement=\"dev.txt\"\n", "dev.txt"), + ("attached", "-r\"dev reqs.txt\"\n", "dev reqs.txt"), + ]; + for (case, root, include) in cases { + eprintln!("case {case}"); + assert_lock_only_discovers( + &[ + ("requirements.txt", root), + (include, "sp-fixture-quoted==1.0.0\n"), + ], + &["pkg:pypi/sp-fixture-quoted@1.0.0"], + ) + .await; + } +} + +/// #994: pip expands `${NAME}` from the environment before it parses the +/// line, so `-r ${REQDIR}/dev.txt` follows `$REQDIR`. +#[tokio::test] +async fn lock_only_scan_discovers_env_var_include_target() { + assert_lock_only_discovers_with_env( + &[ + ("requirements.txt", "-r ${SP_TEST_REQDIR}/dev.txt\n"), + ("sub/dev.txt", "sp-fixture-env==1.0.0\n"), + ], + &[("SP_TEST_REQDIR", "sub")], + &["pkg:pypi/sp-fixture-env@1.0.0"], + ) + .await; +} diff --git a/crates/socket-patch-core/src/crawlers/gradle_cache.rs b/crates/socket-patch-core/src/crawlers/gradle_cache.rs index ef295ee27..afd7c4fba 100644 --- a/crates/socket-patch-core/src/crawlers/gradle_cache.rs +++ b/crates/socket-patch-core/src/crawlers/gradle_cache.rs @@ -70,8 +70,7 @@ pub fn hash_eq(dir_name: &str, sha1_hex: &str) -> bool { /// Whether `bytes` are the pristine download Gradle stored in the hash /// directory `dir_name` (their sha1 names it). pub fn pristine(dir_name: &str, bytes: &[u8]) -> bool { - use sha1::{Digest, Sha1}; - hash_eq(dir_name, &hex::encode(Sha1::digest(bytes))) + hash_eq(dir_name, &crate::utils::digest::sha1_hex_of(bytes)) } /// Whether `path` is a version directory of a `files-2.1` tree @@ -432,8 +431,6 @@ impl DerivedIndex { /// The [`DerivedCopies`] of the jar `jar_leaf` whose pristine bytes /// hash to `pristine_sha1`. pub fn query(&self, jar_leaf: &str, pristine_sha1: &str) -> DerivedCopies { - use sha1::{Digest, Sha1}; - let instrumented = format!("instrumented-{jar_leaf}"); let mut out = DerivedCopies { incomplete: self.incomplete, @@ -460,7 +457,9 @@ impl DerivedIndex { out.stale.push(path.clone()); } else if name == jar_leaf || name == instrumented { match crate::utils::fs::read_regular_to_bytes_sync(path) { - Ok(bytes) if hash_eq(&hex::encode(Sha1::digest(&bytes)), pristine_sha1) => { + Ok(bytes) + if hash_eq(&crate::utils::digest::sha1_hex_of(&bytes), pristine_sha1) => + { out.stale.push(path.clone()) } Ok(_) => out.unknown.push(path.clone()), diff --git a/crates/socket-patch-core/src/patch/jvm_jar.rs b/crates/socket-patch-core/src/patch/jvm_jar.rs index 82d679406..f38a84403 100644 --- a/crates/socket-patch-core/src/patch/jvm_jar.rs +++ b/crates/socket-patch-core/src/patch/jvm_jar.rs @@ -25,8 +25,6 @@ use std::collections::HashMap; use std::path::{Path, PathBuf}; -use sha1::Digest as _; - use crate::crawlers::gradle_cache; use crate::hash::git_sha256::compute_git_sha256_from_bytes; use crate::manifest::schema::PatchFileInfo; @@ -353,12 +351,11 @@ fn unpatched_members( } fn sha256_hex(bytes: &[u8]) -> String { - use sha2::Digest as _; - hex::encode(sha2::Sha256::digest(bytes)) + crate::utils::digest::sha256_hex_of(bytes) } fn sha1_hex(bytes: &[u8]) -> String { - hex::encode(sha1::Sha1::digest(bytes)) + crate::utils::digest::sha1_hex_of(bytes) } /// `/jvm-originals/.jar`. diff --git a/crates/socket-patch-core/src/patch/sidecars/maven.rs b/crates/socket-patch-core/src/patch/sidecars/maven.rs index f2f5a2466..8798bfce6 100644 --- a/crates/socket-patch-core/src/patch/sidecars/maven.rs +++ b/crates/socket-patch-core/src/patch/sidecars/maven.rs @@ -17,8 +17,6 @@ use std::path::{Path, PathBuf}; -use sha1::Digest as _; - use super::{ SidecarAdvisory, SidecarAdvisoryCode, SidecarError, SidecarFile, SidecarFileAction, SidecarPayload, SidecarSeverity, @@ -44,7 +42,7 @@ impl Algo { fn digest(self, bytes: &[u8]) -> String { match self { - Algo::Sha1 => hex::encode(sha1::Sha1::digest(bytes)), + Algo::Sha1 => crate::utils::digest::sha1_hex_of(bytes), Algo::Md5 => hex::encode(md5(bytes)), } } diff --git a/crates/socket-patch-core/src/utils/requirements.rs b/crates/socket-patch-core/src/utils/requirements.rs index b13f634d3..650442c48 100644 --- a/crates/socket-patch-core/src/utils/requirements.rs +++ b/crates/socket-patch-core/src/utils/requirements.rs @@ -93,6 +93,99 @@ pub(crate) fn strip_comment(text: &str) -> &str { split_comment(text).0 } +/// pip's `expand_env_variables`: each `${NAME}` whose `NAME` is +/// `[A-Z0-9_]+` is replaced by `lookup(NAME)`; an unset or empty variable +/// leaves the reference as written. pip expands after stripping comments +/// and before it splits the options, so a variable can carry quotes or +/// spaces that the split then reads. +pub(crate) fn expand_env_vars(code: &str, lookup: impl Fn(&str) -> Option) -> String { + let mut out = String::with_capacity(code.len()); + let mut rest = code; + while let Some(start) = rest.find("${") { + out.push_str(&rest[..start]); + let after = &rest[start + 2..]; + let name_len = after + .find(|c: char| !(c.is_ascii_uppercase() || c.is_ascii_digit() || c == '_')) + .unwrap_or(after.len()); + let value = (name_len > 0 && after[name_len..].starts_with('}')) + .then(|| lookup(&after[..name_len])) + .flatten() + .filter(|v| !v.is_empty()); + match value { + Some(v) => { + out.push_str(&v); + rest = &after[name_len + 1..]; + } + None => { + out.push_str("${"); + rest = after; + } + } + } + out.push_str(rest); + out +} + +/// Python's `shlex.split` (POSIX mode), which pip runs over a line's +/// options: whitespace separates words; `'…'` is literal; inside `"…"` a +/// backslash escapes only `"` and `\`; elsewhere a backslash escapes any +/// character; adjacent quoted and bare parts join into one word, and `""` +/// is an empty word. `None` for an unclosed quote or a trailing lone +/// backslash, which pip refuses ("Could not split options"). +pub(crate) fn shlex_split(text: &str) -> Option> { + let mut words = Vec::new(); + let mut word = String::new(); + let mut in_word = false; + let mut chars = text.chars(); + while let Some(c) = chars.next() { + match c { + ' ' | '\t' | '\r' | '\n' => { + if in_word { + words.push(std::mem::take(&mut word)); + in_word = false; + } + } + '\\' => { + word.push(chars.next()?); + in_word = true; + } + '\'' => { + in_word = true; + loop { + match chars.next()? { + '\'' => break, + ch => word.push(ch), + } + } + } + '"' => { + in_word = true; + loop { + match chars.next()? { + '"' => break, + '\\' => { + let next = chars.next()?; + if next != '"' && next != '\\' { + word.push('\\'); + } + word.push(next); + } + ch => word.push(ch), + } + } + } + _ => { + word.push(c); + in_word = true; + } + } + } + if in_word { + words.push(word); + } + Some(words) +} + /// The `(name as spelled, version)` of an exact `name[extras]==X` registry /// requirement (a logical line's code part; an optional `; marker` and /// options may follow), `None` for anything else — ranges, `===`, wildcards @@ -364,4 +457,78 @@ mod tests { assert_eq!(archive_filename_coords("six.tar.gz"), None, "no version"); assert_eq!(archive_filename_coords("six-1.0.egg"), None); } + + #[test] + fn expand_env_vars_follows_pips_name_grammar() { + let lookup = |name: &str| match name { + "REQDIR" => Some("sub".to_string()), + "SPACED" => Some("\"dev reqs.txt\"".to_string()), + "EMPTY" => Some(String::new()), + _ => None, + }; + assert_eq!( + expand_env_vars("-r ${REQDIR}/dev.txt", lookup), + "-r sub/dev.txt" + ); + assert_eq!( + expand_env_vars("-r ${SPACED}", lookup), + "-r \"dev reqs.txt\"" + ); + assert_eq!( + expand_env_vars("${REQDIR}/${REQDIR}", lookup), + "sub/sub", + "every reference is expanded" + ); + for kept in [ + "-r ${UNSET}/dev.txt", + "-r ${EMPTY}/dev.txt", + "-r ${reqdir}/dev.txt", + "-r $REQDIR/dev.txt", + "-r ${}/dev.txt", + "-r ${REQDIR", + "-r ${REQ-DIR}/dev.txt", + ] { + assert_eq!(expand_env_vars(kept, lookup), kept, "{kept:?}"); + } + } + + #[test] + fn shlex_split_matches_python_posix_mode() { + let split = |s: &str| shlex_split(s).map(|w| w.join("|")); + assert_eq!(split("-r dev.txt").as_deref(), Some("-r|dev.txt")); + assert_eq!(split("-r\t dev.txt ").as_deref(), Some("-r|dev.txt")); + assert_eq!( + split("-r \"dev reqs.txt\"").as_deref(), + Some("-r|dev reqs.txt") + ); + assert_eq!( + split("-r 'dev reqs.txt'").as_deref(), + Some("-r|dev reqs.txt") + ); + assert_eq!( + split("-r dev\\ reqs.txt").as_deref(), + Some("-r|dev reqs.txt") + ); + assert_eq!( + split("--requirement=\"dev.txt\"").as_deref(), + Some("--requirement=dev.txt") + ); + assert_eq!(split("a\"b c\"'d e'f").as_deref(), Some("ab cd ef")); + assert_eq!( + split("'a\\b'").as_deref(), + Some("a\\b"), + "no escapes in '…'" + ); + assert_eq!( + split("\"a\\\"b\\\\c\\d\"").as_deref(), + Some("a\"b\\c\\d"), + "in \"…\" only \\\" and \\\\ are escapes" + ); + assert_eq!(split("sub\\dev.txt").as_deref(), Some("subdev.txt")); + assert_eq!(shlex_split("-r \"\"").unwrap(), vec!["-r", ""]); + assert_eq!(shlex_split("").unwrap(), Vec::::new()); + for unbalanced in ["-r \"dev.txt", "-r 'dev.txt", "-r dev.txt\\"] { + assert_eq!(shlex_split(unbalanced), None, "{unbalanced:?}"); + } + } } diff --git a/crates/socket-patch-core/src/vendor/pypi_requirements.rs b/crates/socket-patch-core/src/vendor/pypi_requirements.rs index be8602984..c828cc04d 100644 --- a/crates/socket-patch-core/src/vendor/pypi_requirements.rs +++ b/crates/socket-patch-core/src/vendor/pypi_requirements.rs @@ -26,7 +26,8 @@ use std::path::{Path, PathBuf}; use crate::crawlers::python_crawler::canonicalize_pypi_name; use crate::utils::fs::{atomic_write_bytes_preserving_mode, read_regular_to_string}; use crate::utils::requirements::{ - hash_options, logical_lines, requires_hashes, split_comment, strip_comment, vendor_tag, + expand_env_vars, hash_options, logical_lines, requires_hashes, shlex_split, split_comment, + strip_comment, vendor_tag, }; use super::common::{detect_eol, refuse_symlinked}; @@ -966,7 +967,7 @@ pub(crate) fn requirements_includes(rel: &str, content: &str) -> Vec { .filter_map(|ll| include_target(&ll.text)) .map(|target| { let joined = if include_dir.is_empty() { - target.to_string() + target } else { format!("{include_dir}/{target}") }; @@ -975,21 +976,41 @@ pub(crate) fn requirements_includes(rel: &str, content: &str) -> Vec { .collect() } -/// The `-r`/`--requirement` include target of a logical line, if any. -fn include_target(text: &str) -> Option<&str> { - let code = strip_comment(text).trim(); - if let Some(rest) = code.strip_prefix("--requirement=") { - return Some(rest.trim()).filter(|s| !s.is_empty()); - } - let mut tokens = code.split_whitespace(); - match tokens.next() { - Some("-r") | Some("--requirement") => tokens.next(), - // pip's optparse also accepts the attached short form (`-rdev.txt`); - // the bare `-r` was consumed by the arm above, so the value here is - // never empty. (No other requirements-file option starts with `-r`.) - Some(t) if t.starts_with("-r") && !t.starts_with("--") => Some(&t[2..]), - _ => None, - } +/// The `-r`/`--requirement` include target of a logical line, if any, +/// read the way pip's `req_file.py` reads it: comment stripped, `${NAME}` +/// expanded from the environment, then the options `shlex`-split, so +/// `-r "dev reqs.txt"`, `-r dev\\ reqs.txt`, `--requirement="dev.txt"` and +/// `-r ${REQDIR}/dev.txt` name the file pip opens (#994). +fn include_target(text: &str) -> Option { + include_target_with(text, |name| std::env::var(name).ok()) +} + +/// [`include_target`] with the environment lookup injected (tests). +fn include_target_with(text: &str, env: impl Fn(&str) -> Option) -> Option { + let code = expand_env_vars(strip_comment(text), env); + // An unbalanced quote is pip's "Could not split options" error: there + // is no file to follow. + let mut words = shlex_split(&code)?.into_iter(); + let first = words.next()?; + let target = if let Some(rest) = first.strip_prefix("--requirement=") { + // `--requirement= dev.txt` (a space after the `=`) is read as + // the next word, as before the shlex split. + if rest.is_empty() { + words.next() + } else { + Some(rest.to_string()) + } + } else { + match first.as_str() { + "-r" | "--requirement" => words.next(), + // pip's optparse also accepts the attached short form + // (`-rdev.txt`, `-r"dev reqs.txt"`). No other + // requirements-file option starts with `-r`. + t if t.starts_with("-r") && !t.starts_with("--") => Some(t[2..].to_string()), + _ => None, + } + }; + target.filter(|t| !t.is_empty()) } /// Lexically normalize a relative path (`a/../b` → `b`); escapes above the @@ -1571,7 +1592,9 @@ mod tests { /// root; a pin there must refuse exactly like a `../` include. Mangling it /// into an in-root relative path silently skips the file pip *can* read, /// and the transitive line appended at the root EOF gives pip a "double - /// requirement" error. + /// requirement" error. The path is quoted, as pip needs it on Windows: + /// pip `shlex`-splits the line, so an unquoted `C:\…` loses its + /// backslashes (#994). #[tokio::test] async fn pin_in_absolute_include_refuses() { let outer = tempfile::tempdir().unwrap(); @@ -1579,7 +1602,7 @@ mod tests { tokio::fs::create_dir_all(&root).await.unwrap(); let shared = outer.path().join("shared.txt"); tokio::fs::write(&shared, "six==1.16.0\n").await.unwrap(); - let root_content = format!("-r {}\n", shared.display()); + let root_content = format!("-r \"{}\"\n", shared.display()); tokio::fs::write(root.join("requirements.txt"), &root_content) .await .unwrap(); @@ -2332,14 +2355,17 @@ mod tests { #[tokio::test] async fn requirement_equals_long_form_include_is_followed() { // Unit shape checks for the `=` arm. - assert_eq!(include_target("--requirement=dev.txt"), Some("dev.txt")); assert_eq!( - include_target("--requirement= dev.txt "), + include_target("--requirement=dev.txt").as_deref(), + Some("dev.txt") + ); + assert_eq!( + include_target("--requirement= dev.txt ").as_deref(), Some("dev.txt"), "the attached value is trimmed" ); assert_eq!( - include_target("--requirement="), + include_target("--requirement=").as_deref(), None, "an empty attached value is not an include" ); @@ -2366,6 +2392,59 @@ mod tests { ); } + /// #994: an include target is read the way pip reads it — `${NAME}` + /// expanded, then the options `shlex`-split — so quotes, backslash + /// escapes and env references name the file pip opens. + #[test] + fn include_target_unquotes_and_expands_like_pip() { + let env = |name: &str| (name == "REQDIR").then(|| "sub".to_string()); + let cases: &[(&str, Option<&str>)] = &[ + ("-r dev.txt", Some("dev.txt")), + ("-r\tdev.txt", Some("dev.txt")), + ("-r \"dev reqs.txt\"", Some("dev reqs.txt")), + ("-r 'dev reqs.txt'", Some("dev reqs.txt")), + ("-r \"dev.txt\"", Some("dev.txt")), + ("-r dev\\ reqs.txt", Some("dev reqs.txt")), + ("-r ${REQDIR}/dev.txt", Some("sub/dev.txt")), + ("-r ${UNSET}/dev.txt", Some("${UNSET}/dev.txt")), + ("--requirement \"dev.txt\"", Some("dev.txt")), + ("--requirement=\"dev.txt\"", Some("dev.txt")), + ("--requirement='dev reqs.txt'", Some("dev reqs.txt")), + ("-r\"dev reqs.txt\"", Some("dev reqs.txt")), + ("-rdev.txt", Some("dev.txt")), + ("-r \"dev reqs.txt\" # comment", Some("dev reqs.txt")), + ("-r \"dev.txt", None), + ("-r \"\"", None), + ("-r", None), + ("-c constraints.txt", None), + ("six==1.16.0", None), + ("--require-hashes", None), + ]; + for (line, want) in cases { + assert_eq!(include_target_with(line, env).as_deref(), *want, "{line:?}"); + } + } + + /// #994: the planner follows a quoted include with a space in its name + /// and wires the pin there instead of appending a duplicate at the root. + #[tokio::test] + async fn quoted_include_with_space_is_followed() { + let tmp = write_root("-r \"dev reqs.txt\"\n").await; + tokio::fs::write(tmp.path().join("dev reqs.txt"), "six==1.16.0\n") + .await + .unwrap(); + let wiring = wire_requirements(tmp.path(), "six", "1.16.0", REL_WHEEL, SHA) + .await + .unwrap(); + assert_eq!(wiring.len(), 1); + assert_eq!(wiring[0].file, "dev reqs.txt"); + assert_eq!(read_root(tmp.path()).await, "-r \"dev reqs.txt\"\n"); + assert_eq!( + requirements_include_names(tmp.path()).await.unwrap(), + vec!["requirements.txt".to_string(), "dev reqs.txt".to_string()] + ); + } + // ── pure-function matrices ─────────────────────────────────────────── /// Lexical normalization: interior `..` pops the stack (which decides