diff --git a/crates/socket-patch-cli/tests/e2e_redirect_yarn_classic_build.rs b/crates/socket-patch-cli/tests/e2e_redirect_yarn_classic_build.rs index ae4599e04..4dc56fffa 100644 --- a/crates/socket-patch-cli/tests/e2e_redirect_yarn_classic_build.rs +++ b/crates/socket-patch-cli/tests/e2e_redirect_yarn_classic_build.rs @@ -1481,6 +1481,136 @@ async fn classic_file_directory_copy_under_another_name_is_named_and_not_atteste ); } +/// #1516: the #1236 copy, but from a directory OUTSIDE the project root +/// (`"lp2": "file:../ext/lpdir"`, shared code in a non-workspace +/// monorepo). yarn 1 installs it into `node_modules/lp2` all the same, so +/// it stays unpatched. `scan --mode hosted` must name the copy +/// (`redirect_copy_unpatched`), and neither its in-run VEX nor a lock-only +/// `vex` may attest left-pad not_affected. +#[tokio::test(flavor = "multi_thread")] +#[serial_test::serial] +async fn classic_outside_root_copy_under_another_name_is_named_and_not_attested() { + if !require_yarn_classic( + "e2e_redirect_yarn_classic_build (outside-root other-name file:)", + |c| { + cache_env::isolate(c); + }, + ) { + return; + } + let tmp = tempfile::tempdir().unwrap(); + let proj = tmp.path().join("proj"); + let copy = tmp.path().join("ext").join("lpdir"); + std::fs::create_dir_all(©).unwrap(); + std::fs::create_dir_all(&proj).unwrap(); + std::fs::write( + copy.join("package.json"), + format!(r#"{{"name":"{DEP}","version":"{DEP_VERSION}","main":"index.js"}}"#), + ) + .unwrap(); + let orig: &[u8] = b"module.exports = function leftPad(s) { return s; };\n"; + std::fs::write(copy.join("index.js"), orig).unwrap(); + std::fs::write( + proj.join("package.json"), + format!( + r#"{{"name":"outside-root-classic","version":"0.0.0","private":true,"dependencies":{{"{DEP}":"{DEP_VERSION}","lp2":"file:../ext/lpdir"}}}}"# + ), + ) + .unwrap(); + let cache = tmp.path().join("yarn-cache"); + let install = corepack( + &proj, + &yarn_classic(), + &["install", "--no-progress"], + &[("YARN_CACHE_FOLDER", cache.to_str().unwrap())], + ); + if !install.status.success() { + skip!( + "(outside-root other-name file:): fixture `yarn install` failed:\n{}", + String::from_utf8_lossy(&install.stderr) + ); + return; + } + let lock_pristine = std::fs::read_to_string(proj.join("yarn.lock")).unwrap(); + assert!( + lock_pristine.contains("lp2@file:../ext/lpdir"), + "fixture must lock the copy under its dependency name:\n{lock_pristine}" + ); + + let installed_dir = proj.join("node_modules").join(DEP); + let installed_orig = std::fs::read(installed_dir.join("index.js")).unwrap(); + let patched: Vec = [MARKER.as_bytes(), &installed_orig].concat(); + let tgz_path = tmp.path().join("patched.tgz"); + build_patched_tgz(&installed_dir, &patched, &tgz_path); + let tgz = std::fs::read(&tgz_path).unwrap(); + let server = mock_hosted_grant(&tgz, &installed_orig, &patched, "other-name fixture").await; + + let api_url = server.uri(); + let api = [ + "--api-url", + api_url.as_str(), + "--org", + ORG, + "--api-token", + "fake", + ]; + let mut args = vec![ + "scan", + "--mode", + "hosted", + "--json", + "--yes", + "--cwd", + proj.to_str().unwrap(), + "--vex", + "out.vex.json", + "--vex-product", + PRODUCT, + ]; + args.extend(api); + let (code, stdout, stderr) = run_socket(&proj, &args); + println!("scan exit {code}\nstdout:\n{stdout}\nstderr:\n{stderr}"); + let env: serde_json::Value = serde_json::from_str(&stdout) + .unwrap_or_else(|e| panic!("scan --json output is not JSON: {e}\n{stdout}\n{stderr}")); + let lock = std::fs::read_to_string(proj.join("yarn.lock")).unwrap(); + assert!( + lock.contains(&format!("{UUID}/{DEP}-{DEP_VERSION}.tgz")), + "the registry left-pad block must still be pinned:\n{lock}" + ); + assert!( + env.to_string().contains("redirect_copy_unpatched"), + "the outside-root copy must be named: {env}" + ); + assert!(env.to_string().contains("lp2@file:../ext/lpdir"), "{env}"); + let vex = std::fs::read_to_string(proj.join("out.vex.json")).unwrap_or_default(); + assert!( + !vex.contains("not_affected"), + "the in-run VEX must not attest left-pad:\n{vex}\n{env}" + ); + + // Lock-only: with node_modules gone, `vex` reads only the lock. + std::fs::remove_dir_all(proj.join("node_modules")).unwrap(); + let mut args = vec![ + "vex", + "--cwd", + proj.to_str().unwrap(), + "--output", + "lock-only.vex.json", + "--product", + PRODUCT, + "--patch-server-url", + api_url.as_str(), + ]; + args.extend(api); + let (code, stdout, stderr) = run_socket(&proj, &args); + println!("vex exit {code}\nstdout:\n{stdout}\nstderr:\n{stderr}"); + let vex = std::fs::read_to_string(proj.join("lock-only.vex.json")).unwrap_or_default(); + assert!( + !vex.contains("not_affected"), + "lock-only vex must not attest left-pad:\n{vex}\n{stdout}\n{stderr}" + ); +} + /// A mock patch API granting one hosted patch of `DEP@DEP_VERSION` whose /// tarball is `tgz` (`index.js` from `orig` to `patched`). async fn mock_hosted_grant(tgz: &[u8], orig: &[u8], patched: &[u8], title: &str) -> MockServer { diff --git a/crates/socket-patch-core/src/formats/yarn/copy.rs b/crates/socket-patch-core/src/formats/yarn/copy.rs index ec1ea9cd1..b00e09378 100644 --- a/crates/socket-patch-core/src/formats/yarn/copy.rs +++ b/crates/socket-patch-core/src/formats/yarn/copy.rs @@ -36,7 +36,17 @@ impl CopyRef { /// The root-relative file to read for [`CopyRef::identity`]: the tarball /// itself, or the directory's `package.json`. `None` for a url, which /// names its package in its path. + /// + /// `None` too for an absolute path: a lock is untrusted input, and an + /// absolute spelling can name any file on the machine, or (a Windows + /// UNC path) a network share. Such a copy is never read, so its package + /// stays unknown (#1516). pub(crate) fn read_path(&self) -> Option { + if let CopyRef::Tarball(rel) | CopyRef::Directory(rel) | CopyRef::Linked(rel) = self { + if crate::utils::relpath::is_anchored(rel) { + return None; + } + } match self { CopyRef::Tarball(rel) => Some(rel.clone()), CopyRef::Directory(dir) | CopyRef::Linked(dir) => Some(if dir.is_empty() { @@ -93,9 +103,10 @@ impl CopyRef { /// The copy a classic block installs from its key patterns and `resolved`: /// a `file:` path in a key pattern (relative to the root, where yarn 1 -/// writes it), else an http(s) `resolved`. `None` for anything else, or a -/// `file:` path that leaves the root. The caller decides which blocks are -/// copies at all ([`super::source::classic_copy_source`]). +/// writes it, and read there even when it leaves the root: yarn installs +/// `file:../shared/left-pad` all the same, #1516), else an http(s) +/// `resolved`. `None` for anything else. The caller decides which blocks +/// are copies at all ([`super::source::classic_copy_source`]). pub(crate) fn classic_copy_ref(patterns: &[String], resolved: Option<&str>) -> Option { let file = patterns.iter().find_map(|p| { let (_, range) = split_classic_pattern(p)?; @@ -104,7 +115,7 @@ pub(crate) fn classic_copy_ref(patterns: &[String], resolved: Option<&str>) -> O match file { Some(path) => { let path = path.split('#').next().unwrap_or_default(); - let rel = crate::utils::cargo_workspace::normalize_rel("", path)?; + let rel = copy_path("", path)?; Some(if is_tarball_path(path) { CopyRef::Tarball(rel) } else { @@ -122,8 +133,8 @@ pub(crate) fn classic_copy_ref(patterns: &[String], resolved: Option<&str>) -> O /// The copy a berry locator `reference` installs from: a `file:` tarball or /// directory, a `portal:` / `link:` directory (each relative to the /// workspace its `locator=` binding names), or an http(s) url. `None` for -/// any other protocol (`npm:`, `workspace:`, `patch:`, `exec:`, git), or a -/// path that leaves the root. +/// any other protocol (`npm:`, `workspace:`, `patch:`, `exec:`, git). A +/// path that leaves the root is still read there ([`copy_path`]). pub(crate) fn berry_copy_ref(reference: &str) -> Option { if reference.starts_with("http") { return Some(CopyRef::Url( @@ -153,8 +164,8 @@ pub(crate) fn berry_copy_ref(reference: &str) -> Option { /// The root-relative path a berry `file:` / `portal:` / `link:` reference /// names: relative to the workspace in its `locator=` binding -/// (`b@workspace:packages/b`), or to the root when it has none. `None` when -/// it leaves the root. +/// (`b@workspace:packages/b`), or to the root when it has none +/// ([`copy_path`]). fn berry_copy_path(reference: &str, path: &str) -> Option { let workspace = reference .split_once("::") @@ -171,7 +182,31 @@ fn berry_copy_path(reference: &str, path: &str) -> Option { } else { workspace }; - crate::utils::cargo_workspace::normalize_rel(&workspace, path) + copy_path(&workspace, path) +} + +/// Where a copy's `path`, relative to the root-relative directory `base`, +/// is read from: normalized and root-relative, keeping one leading `../` +/// per level it climbs above the root (`../shared/left-pad`), or as +/// spelled when it is absolute. yarn installs a copy from outside the +/// project as readily as one inside it, so dropping such a path would +/// leave the copy unread and its package unattributed (#1516). `None` +/// only for an empty path. +fn copy_path(base: &str, path: &str) -> Option { + if path.is_empty() { + return None; + } + if crate::utils::relpath::is_anchored(path) { + return Some(path.to_string()); + } + let joined = if base.is_empty() { + path.to_string() + } else { + format!("{base}/{path}") + }; + Some(crate::utils::relpath::normalize_rel_keeping_escapes( + &joined, + )) } fn parse_manifest(bytes: &[u8]) -> Option { @@ -181,7 +216,7 @@ fn parse_manifest(bytes: &[u8]) -> Option { /// `name` of the `package.json` inside an npm tarball. fn tarball_manifest_name(bytes: &[u8]) -> Option { - let map = crate::patch::package::read_archive_bytes_to_map(bytes).ok()?; + let map = crate::patch::package::read_archive_bytes_to_map_any_root(bytes).ok()?; manifest_name(map.get("package.json")?) } @@ -215,7 +250,42 @@ mod tests { "https://registry.npmjs.org/left-pad/-/left-pad-1.3.0.tgz".into() )) ); - assert_eq!(classic_copy_ref(&pats(&["lp2@file:../out"]), None), None); + // #1516: yarn installs a copy from outside the root all the same, + // so it is read there, not dropped. + assert_eq!( + classic_copy_ref(&pats(&["lp2@file:../ext/lpdir"]), None), + Some(CopyRef::Directory("../ext/lpdir".into())) + ); + assert_eq!( + classic_copy_ref( + &pats(&["lp2@file:../shared/lp.tgz"]), + Some("file:../shared/lp.tgz#abc") + ), + Some(CopyRef::Tarball("../shared/lp.tgz".into())) + ); + let abs = classic_copy_ref(&pats(&["lp2@file:/abs/lpdir"]), None); + assert_eq!(abs, Some(CopyRef::Directory("/abs/lpdir".into()))); + assert_eq!( + abs.unwrap().read_path(), + None, + "an absolute path is never read" + ); + } + + /// #1516: yarn strips a `file:` tarball's first path segment whatever + /// its name, so a `git archive --prefix=left-pad-1.3.0/` tarball names + /// its package like a `package/` one. + #[test] + fn tarball_identity_reads_any_root_directory() { + let tarball = CopyRef::Tarball("lp.tgz".into()); + for root in ["package", "left-pad-1.3.0"] { + let bytes = crate::vex::discover::testing::npm_tgz_rooted(root, "left-pad", "1.3.0"); + assert_eq!( + tarball.identity("1.3.0", Some(&bytes)), + Some(("left-pad".into(), "1.3.0".into())), + "{root}/" + ); + } } #[test] @@ -227,8 +297,8 @@ mod tests { ); assert_eq!( berry_copy_ref(&format!("file:../../../lp.tgz#../../../lp.tgz{ws}")), - None, - "three levels up from packages/b leaves the root" + Some(CopyRef::Tarball("../lp.tgz".into())), + "three levels up from packages/b leaves the root, and is read there (#1516)" ); assert_eq!( berry_copy_ref(&format!("file:../forks/lp.tgz#../forks/lp.tgz{ws}")), diff --git a/crates/socket-patch-core/src/patch/package.rs b/crates/socket-patch-core/src/patch/package.rs index 39f1183d9..d2f2b9acf 100644 --- a/crates/socket-patch-core/src/patch/package.rs +++ b/crates/socket-patch-core/src/patch/package.rs @@ -143,6 +143,44 @@ pub fn read_archive_bytes_to_map_strict( read_archive_from_reader(bytes, true, true) } +/// [`read_archive_bytes_to_map`] keyed the way npm-family installers +/// extract a tarball: the first path segment stripped whatever its name +/// (`package/`, `left-pad-1.3.0/` from `git archive --prefix`, a GitHub +/// release's `-/`), where the lenient decoder strips only a +/// literal `package/` (#1516). A member that two different roots both +/// spell is dropped: which one the installer keeps depends on archive +/// order, which the map does not record. Members directly at the archive +/// root (no first segment to strip) are dropped too. +pub(crate) fn read_archive_bytes_to_map_any_root( + bytes: &[u8], +) -> Result>, ArchiveError> { + let raw = read_archive_from_reader(bytes, false, false)?; + let mut out: HashMap> = HashMap::new(); + let mut ambiguous: std::collections::HashSet = std::collections::HashSet::new(); + let mut roots: HashMap = HashMap::new(); + for (path, data) in raw { + let path = path.trim_start_matches("./"); + let Some((root, rest)) = path.split_once('/') else { + continue; + }; + let rest = rest.trim_start_matches('/'); + if rest.is_empty() || ambiguous.contains(rest) { + continue; + } + match roots.get(rest) { + Some(seen) if seen != root => { + out.remove(rest); + ambiguous.insert(rest.to_string()); + } + _ => { + roots.insert(rest.to_string(), root.to_string()); + out.insert(rest.to_string(), data); + } + } + } + Ok(out) +} + /// Read a source distribution without npm's `package/` prefix convention. /// Uses the same strict member checks and decompression limits. pub(crate) fn read_sdist_tar_bytes_to_map_strict( diff --git a/crates/socket-patch-core/src/vendor/yarn_berry_lock.rs b/crates/socket-patch-core/src/vendor/yarn_berry_lock.rs index 167698af4..84571be78 100644 --- a/crates/socket-patch-core/src/vendor/yarn_berry_lock.rs +++ b/crates/socket-patch-core/src/vendor/yarn_berry_lock.rs @@ -244,8 +244,14 @@ impl NpmLockBackend for YarnBerryBackend { super::yarn_copies::other_name_copies(project_root, &lock_text, name, version).await; // The package IS installed, only under other names, and `yarn // install` can't re-lock those copies: not "not found" (#939). - if scan.target.is_none() && scan.alias_keys.is_empty() && !other_name.is_empty() { - let keys: Vec<&str> = other_name.iter().map(|(key, _)| key.as_str()).collect(); + // Only a copy that says it is `name@version` proves that; an + // unreadable one may be another package (#1516). + let keys: Vec<&str> = other_name + .iter() + .filter(|c| c.identified) + .map(|c| c.key.as_str()) + .collect(); + if scan.target.is_none() && scan.alias_keys.is_empty() && !keys.is_empty() { return Err(Box::new(refused( "vendor_lock_entry_not_rewritable", format!( @@ -257,15 +263,25 @@ impl NpmLockBackend for YarnBerryBackend { ), ))); } - for (key, source) in other_name { + for copy in other_name { + let key = ©.key; + let origin = super::yarn_copies::copy_origin(©.source); warnings.push(VendorWarning::new( "vendor_copy_unpatched", - format!( - "{YARN_LOCK} entry `{key}` installs {name}@{version} under another \ - dependency name, from {}; the name-keyed resolutions entry vendoring \ - writes never reaches it, so that copy stays UNPATCHED", - super::yarn_copies::copy_origin(&source) - ), + if copy.identified { + format!( + "{YARN_LOCK} entry `{key}` installs {name}@{version} under another \ + dependency name, from {origin}; the name-keyed resolutions entry \ + vendoring writes never reaches it, so that copy stays UNPATCHED" + ) + } else { + format!( + "{YARN_LOCK} entry `{key}` installs version {version} under another \ + dependency name, from {origin}, whose package can't be read, so it \ + may be {name}@{version}; the name-keyed resolutions entry vendoring \ + writes never reaches it, so if it is, that copy stays UNPATCHED" + ) + }, )); } let (target_idx, target_is_ours) = target_gate(&scan, name, version)?; @@ -730,14 +746,15 @@ pub(crate) async fn preflight_packages( let aliased = scan_berry_target(&blocks, &coords.name, &coords.version) .is_ok_and(|scan| !scan.alias_keys.is_empty()); if !aliased - && !super::yarn_copies::other_name_copies( + && super::yarn_copies::other_name_copies( project_root, &lock_text, &coords.name, &coords.version, ) .await - .is_empty() + .iter() + .any(|c| c.identified) { *gate = Err("vendor_lock_entry_not_rewritable"); } diff --git a/crates/socket-patch-core/src/vendor/yarn_classic_lock.rs b/crates/socket-patch-core/src/vendor/yarn_classic_lock.rs index 523e68978..276bf4a39 100644 --- a/crates/socket-patch-core/src/vendor/yarn_classic_lock.rs +++ b/crates/socket-patch-core/src/vendor/yarn_classic_lock.rs @@ -127,7 +127,7 @@ impl NpmLockBackend for YarnClassicBackend { only_other_name_copies(o, &blocks, &coords.name, &coords.version, &other_name) })?; warnings.extend(skipped); - warnings.extend(other_name); + warnings.extend(other_name.into_iter().map(|(w, _)| w)); Ok(YarnClassicPlan { text, candidate_keys, @@ -384,29 +384,39 @@ fn rewritable_candidates( /// (`"lp2@file:./lpdir"`), so which package it is comes from the copy /// itself ([`super::yarn_copies::other_name_copies`]). No re-lock of `name` /// reaches that copy, so it stays unpatched; the registry blocks are still -/// wired. +/// wired. Each warning comes with whether the copy was identified as +/// `name@version`; one whose package cannot be read is named as a copy it +/// may be (#1516). async fn other_name_copy_warnings( project_root: &Path, lock_text: &str, name: &str, version: &str, -) -> Vec { +) -> Vec<(VendorWarning, bool)> { super::yarn_copies::other_name_copies(project_root, lock_text, name, version) .await .into_iter() - .map(|(key, source)| { - let code = match source { + .map(|copy| { + let code = match copy.source { crate::formats::yarn::copy::CopyRef::Directory(_) => "vendor_link_entry_skipped", _ => "vendor_yarn_classic_non_registry_entry_skipped", }; - VendorWarning::new( - code, + let key = ©.key; + let origin = super::yarn_copies::copy_origin(©.source); + let detail = if copy.identified { format!( "lock entry `{key}` installs {name}@{version} under another dependency \ - name, from {}; vendoring can't rewire it, so this copy stays UNPATCHED", - super::yarn_copies::copy_origin(&source) - ), - ) + name, from {origin}; vendoring can't rewire it, so this copy stays UNPATCHED" + ) + } else { + format!( + "lock entry `{key}` installs version {version} under another dependency \ + name, from {origin}, whose package can't be read, so it may be \ + {name}@{version}; vendoring can't rewire it, so if it is, this copy stays \ + UNPATCHED" + ) + }; + (VendorWarning::new(code, detail), copy.identified) }) .collect() } @@ -423,15 +433,21 @@ fn only_other_name_copies( blocks: &[LockBlock], name: &str, version: &str, - other_name: &[VendorWarning], + other_name: &[(VendorWarning, bool)], ) -> Box { - if other_name.is_empty() + // Only a copy that says it is `name@version` proves the package is + // installed; an unreadable one keeps the "not found" remedy. + let details: Vec<&str> = other_name + .iter() + .filter(|(_, identified)| *identified) + .map(|(w, _)| w.detail.as_str()) + .collect(); + if details.is_empty() || has_own_name_block(blocks, name, version) || super::npm_common::refusal_code(&refusal) != "vendor_lock_entry_not_found" { return refusal; } - let details: Vec<&str> = other_name.iter().map(|w| w.detail.as_str()).collect(); Box::new(refused( "vendor_lock_entry_not_rewritable", format!( @@ -505,9 +521,10 @@ pub(crate) async fn preflight_packages( continue; }; if !has_own_name_block(&blocks, &coords.name, &coords.version) - && !other_name_copy_warnings(project_root, &text, &coords.name, &coords.version) + && other_name_copy_warnings(project_root, &text, &coords.name, &coords.version) .await - .is_empty() + .iter() + .any(|(_, identified)| *identified) { *gate = Err("vendor_lock_entry_not_rewritable"); } @@ -1946,6 +1963,96 @@ wow@^1.0.0: } } + /// #1516: renamed copies #1236's reader could not read were taken for + /// "not left-pad" and vendoring reported success without naming them, + /// while `node_modules/lp2` installed left-pad@1.3.0 unpatched: a + /// `file:` directory outside the root, a tarball whose root directory + /// is not `package/`, and a non-registry url. Each is named now (the + /// url one as a copy it may be). Control: the outside-root directory + /// holding another package is not named. + #[tokio::test] + async fn issue_1516_unread_other_name_copies_are_named() { + // A sibling directory of the project, spelled `..//lpdir` + // the way yarn records it. + let ext = tempfile::tempdir().unwrap(); + let ext_name = ext + .path() + .file_name() + .unwrap() + .to_str() + .unwrap() + .to_string(); + let extra = format!( + "\n\"lp2@file:../{ext_name}/lpdir\":\n version \"1.3.0\"\n\n\ + \"lp3@file:./lp.tgz\":\n version \"1.3.0\"\n resolved \"file:./lp.tgz#cccc\"\n\n\ + \"lp4@http://127.0.0.1:8799/lp.tgz\":\n version \"1.3.0\"\n \ + resolved \"http://127.0.0.1:8799/lp.tgz#dddd\"\n" + ); + let lock = format!("{Y2_BEFORE}{extra}"); + for (fork, named) in [("left-pad", true), ("other-pkg", false)] { + let fx = fixture_with_lock(&lock).await; + // Same-parent tempdirs: `../` from the root is `ext`. + assert_eq!(fx.root().parent(), ext.path().parent()); + tokio::fs::create_dir_all(ext.path().join("lpdir")) + .await + .unwrap(); + tokio::fs::write( + ext.path().join("lpdir/package.json"), + format!(r#"{{"name":"{fork}","version":"1.3.0"}}"#), + ) + .await + .unwrap(); + tokio::fs::write( + fx.root().join("lp.tgz"), + crate::vex::discover::testing::npm_tgz_rooted("left-pad-1.3.0", fork, "1.3.0"), + ) + .await + .unwrap(); + let (result, entry, warnings) = expect_done(fx.vendor(false).await); + assert!(result.success, "{:?}", result.error); + assert_eq!(entry.unwrap().wiring.len(), 1, "the registry block"); + let count = |code: &str, key: &str| { + warnings + .iter() + .filter(|w| w.code == code && w.detail.contains(key)) + .count() + }; + assert_eq!( + count("vendor_link_entry_skipped", "lp2@"), + usize::from(named), + "{fork} outside-root dir: {warnings:?}" + ); + assert_eq!( + count("vendor_yarn_classic_non_registry_entry_skipped", "lp3@"), + usize::from(named), + "{fork} left-pad-1.3.0/ tarball: {warnings:?}" + ); + // The url names no package: named as a copy it may be, either way. + assert!( + warnings.iter().any(|w| { + w.code == "vendor_yarn_classic_non_registry_entry_skipped" + && w.detail.contains("lp4@") + && w.detail.contains("may be left-pad@1.3.0") + }), + "{fork} url: {warnings:?}" + ); + assert!(fx.lock_text().await.contains(extra.trim_start())); + } + } + + /// #1516: a copy whose package can't be read is never proof that the + /// package is installed, so a lock whose only other-name entry is an + /// unreadable url copy keeps the "not found" refusal and its remedy. + #[tokio::test] + async fn issue_1516_unread_copy_alone_is_not_found() { + let lock = "# yarn lockfile v1\n\n\n\"lp4@http://127.0.0.1:8799/lp.tgz\":\n \ + version \"1.3.0\"\n resolved \"http://127.0.0.1:8799/lp.tgz#dddd\"\n"; + let fx = fixture_with_lock(lock).await; + expect_refused(fx.vendor(false).await, "vendor_lock_entry_not_found"); + let pre = preflight_packages(fx.root(), &[("pkg:npm/left-pad@1.3.0", &fx.record)]).await; + assert_eq!(pre, vec![Err("vendor_lock_entry_not_found")]); + } + /// #1236 (review): when left-pad@1.3.0 is locked ONLY under another /// dependency name, vendoring refuses `vendor_lock_entry_not_rewritable` /// naming that copy, in the loop and in the download plan alike, not diff --git a/crates/socket-patch-core/src/vendor/yarn_copies.rs b/crates/socket-patch-core/src/vendor/yarn_copies.rs index 383c5985c..038d45969 100644 --- a/crates/socket-patch-core/src/vendor/yarn_copies.rs +++ b/crates/socket-patch-core/src/vendor/yarn_copies.rs @@ -21,16 +21,34 @@ use crate::utils::fs::read_regular_to_bytes; use super::lock_inventory::yarn::{berry_entries, classic_entries}; -/// `(lock key, copy)` of every live entry of `lock_text` (either grammar) -/// that installs `name@version` under another dependency name, in lock -/// order. A copy whose package cannot be read (a missing directory, an -/// unreadable tarball, a non-registry url) is left out. +/// A lock entry under another dependency name that installs, or may +/// install, `name@version` ([`other_name_copies`]). +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct OtherNameCopy { + /// The lock entry's key, as the lock spells it. + pub(crate) key: String, + pub(crate) source: CopyRef, + /// The copy itself says it is `name@version`. `false` for a copy of + /// that version whose package cannot be read (a missing directory, a + /// tarball with no readable `package.json`, a non-registry url): it may + /// be `name`, so it is named, but never taken as proof that `name` is + /// installed (#1516). + pub(crate) identified: bool, +} + +/// Every live entry of `lock_text` (either grammar) that installs +/// `name@version` under another dependency name, in lock order. A copy of +/// that version whose package cannot be read is kept, unidentified: it +/// may hold `name`, and leaving it out would let the scan and `vex` vouch +/// for a lock that installs it unpatched (#1516). A `portal:` / `link:` +/// copy whose manifest cannot be read is left out: its version is that +/// manifest's, so nothing ties it to `version`. pub(crate) async fn other_name_copies( project_root: &Path, lock_text: &str, name: &str, version: &str, -) -> Vec<(String, CopyRef)> { +) -> Vec { let candidates: Vec<(String, CopyRef, String)> = if is_berry_lock(lock_text) { berry_entries(lock_text) .entries @@ -87,12 +105,17 @@ pub(crate) async fn other_name_copies( Some(rel) => read_regular_to_bytes(&project_root.join(rel)).await.ok(), None => None, }; - if copy_identity(&source, &lock_version, bytes) - .await - .is_some_and(|(n, v)| n == name && v == version) - { - out.push((key, source)); - } + let identified = match copy_identity(&source, &lock_version, bytes).await { + Some((n, v)) if n == name && v == version => true, + Some(_) => continue, + None if matches!(source, CopyRef::Linked(_)) => continue, + None => false, + }; + out.push(OtherNameCopy { + key, + source, + identified, + }); } out } diff --git a/crates/socket-patch-core/src/vex/discover/mod.rs b/crates/socket-patch-core/src/vex/discover/mod.rs index 63cb35d7a..fafc90b54 100644 --- a/crates/socket-patch-core/src/vex/discover/mod.rs +++ b/crates/socket-patch-core/src/vex/discover/mod.rs @@ -435,6 +435,21 @@ pub struct UnpatchedCopy { pub how: String, } +/// A lock entry that installs SOME package at `version` under another +/// dependency name, from a copy whose package can't be read +/// ([`Discovery::unidentified_copy`], #1516). +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct UnidentifiedCopy { + /// Root-relative lock file. + pub file: PathBuf, + /// The lock entry's version. + pub version: String, + /// The lock entry's key, as the lock spells it. + pub key: String, + /// How that entry installs, completing "lock entry `` …". + pub how: String, +} + /// A ref another lock contests ([`Discovery::contest_across_locks`]): it /// was dropped from `refs` and diagnosed [`DIAG_REF_UNATTRIBUTABLE`]. Kept /// so a ledger reader can name the contesting lock instead of reporting the @@ -563,6 +578,10 @@ pub struct Discovery { /// Same-lock copies that contest that lock's own refs /// ([`Discovery::unpatched_copy`]). pub unpatched_copies: Vec, + /// Same-lock copies whose package can't be read, pending + /// [`Discovery::contest_within_locks`] ([`Discovery::unidentified_copy`]); + /// always empty once discovery returns (folded into `unpatched_copies`). + pub unidentified_copies: Vec, /// Refs in `refs` whose wiring a build bypasses ([`Unattested`]). pub unattested: Vec, /// Copies no wiring reaches, pending [`Discovery::unattest_unwired_copies`]; @@ -868,12 +887,63 @@ impl Discovery { }); } + /// A lock entry of `file` that installs a copy of some package at + /// `version` whose package can't be read (a non-registry url, a missing + /// directory, a tarball with no readable `package.json`). It may be any + /// package at that version, so [`Discovery::contest_within_locks`] + /// takes it as an unpatched copy of every package `file` wires at + /// `version`: vouching for one would rest on a copy nobody read + /// (#1516). Matched there, once every extractor has pushed its refs. + pub(crate) fn unidentified_copy(&mut self, file: &str, version: &str, key: &str, how: &str) { + self.unidentified_copies.push(UnidentifiedCopy { + file: PathBuf::from(file), + version: version.to_string(), + key: key.to_string(), + how: how.to_string(), + }); + } + + /// Fold [`Discovery::unidentified_copies`] into `unpatched_copies`: one + /// copy per ref of the same file at the same version. + fn expand_unidentified_copies(&mut self) { + if self.unidentified_copies.is_empty() { + return; + } + let mut wired: BTreeMap<(&Path, &str), BTreeSet<&str>> = BTreeMap::new(); + for r in &self.refs { + if let Some((_, version)) = r.purl.rsplit_once('@') { + wired + .entry((r.source_file.as_path(), version)) + .or_default() + .insert(r.purl.as_str()); + } + } + let mut expanded = Vec::new(); + for u in &self.unidentified_copies { + for purl in wired + .get(&(u.file.as_path(), u.version.as_str())) + .into_iter() + .flatten() + { + expanded.push(UnpatchedCopy { + purl: purl.to_string(), + file: u.file.clone(), + key: u.key.clone(), + how: u.how.clone(), + }); + } + } + self.unidentified_copies.clear(); + self.unpatched_copies.extend(expanded); + } + /// Drop every ref whose OWN lock also installs an unpatched copy of the /// same `name@version` ([`Discovery::unpatched_copy`]): the build ships /// that copy whatever the wiring does, so the ref is diagnosed /// ([`DIAG_REF_UNATTRIBUTABLE`], naming the entry) and not emitted, but /// [`Discovery::shadowed`]. Its uuid stays recognized (rule 11). fn contest_within_locks(&mut self) { + self.expand_unidentified_copies(); if self.unpatched_copies.is_empty() { return; } @@ -2976,6 +3046,12 @@ pub(crate) mod testing { /// A minimal npm tarball (`package/package.json` naming /// `name@version`) — a user's `file:` tarball copy. pub(crate) fn npm_tgz(name: &str, version: &str) -> Vec { + npm_tgz_rooted("package", name, version) + } + + /// [`npm_tgz`] with the manifest under `root/` instead of `package/` + /// (a `git archive --prefix` or GitHub release tarball, #1516). + pub(crate) fn npm_tgz_rooted(root: &str, name: &str, version: &str) -> Vec { let manifest = format!(r#"{{"name":"{name}","version":"{version}"}}"#); let mut tar = tar::Builder::new(flate2::write::GzEncoder::new( Vec::new(), @@ -2985,8 +3061,12 @@ pub(crate) mod testing { header.set_size(manifest.len() as u64); header.set_mode(0o644); header.set_cksum(); - tar.append_data(&mut header, "package/package.json", manifest.as_bytes()) - .unwrap(); + tar.append_data( + &mut header, + format!("{root}/package.json"), + manifest.as_bytes(), + ) + .unwrap(); tar.into_inner().unwrap().finish().unwrap() } @@ -3018,14 +3098,26 @@ pub(crate) mod testing { /// A throwaway project root. pub(crate) struct Project { - dir: tempfile::TempDir, + _dir: tempfile::TempDir, + root: PathBuf, origins: Vec, } impl Project { pub(crate) fn new() -> Self { + Self::nested("") + } + + /// A project rooted at `sub/` inside the throwaway directory, so a + /// fixture can write files beside it (`../ext/…`) without leaving + /// the tempdir (#1516). + pub(crate) fn nested(sub: &str) -> Self { + let dir = tempfile::tempdir().expect("create tempdir"); + let root = dir.path().join(sub); + std::fs::create_dir_all(&root).expect("create project root"); Project { - dir: tempfile::tempdir().expect("create tempdir"), + _dir: dir, + root, origins: Vec::new(), } } @@ -3038,7 +3130,7 @@ pub(crate) mod testing { } pub(crate) fn root(&self) -> &Path { - self.dir.path() + &self.root } /// Write `content` at root-relative `rel`, creating parents. diff --git a/crates/socket-patch-core/src/vex/discover/testing/golden.rs b/crates/socket-patch-core/src/vex/discover/testing/golden.rs index 4e62d89ca..d08299347 100644 --- a/crates/socket-patch-core/src/vex/discover/testing/golden.rs +++ b/crates/socket-patch-core/src/vex/discover/testing/golden.rs @@ -135,6 +135,8 @@ fn render(out: &Discovery, root: &Path) -> Value { stale_selectors, // Already folded into `unattested` by the time a run returns. unwired_copies: _, + // Already folded into `unpatched_copies` by the time a run returns. + unidentified_copies: _, } = out; let render_ref = |r: &PatchedRef| { let PatchedRef { diff --git a/crates/socket-patch-core/src/vex/discover/yarn.rs b/crates/socket-patch-core/src/vex/discover/yarn.rs index 649aeb0eb..7fedd9ae9 100644 --- a/crates/socket-patch-core/src/vex/discover/yarn.rs +++ b/crates/socket-patch-core/src/vex/discover/yarn.rs @@ -153,8 +153,10 @@ async fn extract_classic(ctx: &DiscoverCtx<'_>, entries: Vec, out: &m /// The copy a classic `file:` / url block installs, to be read for the /// package it really holds ([`record_copies`], #1236): yarn 1 keys it by the /// DEPENDENCY name (`"lp2@file:./lpdir"`), like berry. The `file:` path is -/// the key pattern's, relative to the root (a path yarn resolved against a -/// workspace member reads nothing there, and is left alone). +/// the key pattern's: yarn 1 writes it relative to the root and normalized, +/// whichever `package.json` declared it (a member's `file:./lpdir` locks as +/// `file:./packages/a/lpdir`), so one that leaves the root is read there too +/// (#1516). fn classic_copy(entry: &YarnEntry, resolved: Option<&str>) -> Option { Some(LockCopy { key: entry.block.key.clone(), @@ -857,8 +859,13 @@ struct LockCopy { /// the name the depender gave it, so `"lp2": "file:left-pad-1.3.0.tgz"` /// locks as `lp2@…` while `node_modules/lp2` IS left-pad@1.3.0, and no pin /// of `left-pad` reaches it. The real package comes from the copy itself -/// ([`CopyRef::identity`]); a copy whose package cannot be read is left -/// alone. +/// ([`CopyRef::identity`]). A copy whose package cannot be read (a +/// missing directory, a tarball with no readable `package.json`, a +/// non-registry url) may be any package at its lock version, so it is +/// recorded as a copy of every package this lock wires at that version: +/// vouching for those would rest on a copy nobody read (#1516). A +/// `portal:` / `link:` copy with no readable manifest has no version, and +/// is left alone. async fn record_copies(ctx: &DiscoverCtx<'_>, copies: Vec, out: &mut Discovery) { for copy in copies { let bytes = match copy.source.read_path() { @@ -868,6 +875,18 @@ async fn record_copies(ctx: &DiscoverCtx<'_>, copies: Vec, out: &mut D let Some((name, version)) = crate::vendor::yarn_copies::copy_identity(©.source, ©.version, bytes).await else { + if !matches!(copy.source, CopyRef::Linked(_)) { + out.unidentified_copy( + YARN_LOCK, + ©.version, + ©.key, + &format!( + "{}, whose package can't be read, so it may be this version of any \ + package (yarn keys it by the dependency name)", + copy.source.how() + ), + ); + } continue; }; out.unpatched_copy( @@ -1396,6 +1415,184 @@ mod tests { } } + /// #1516: renamed classic copies #1236's reader could not read, so + /// each was taken for "not left-pad" and the wiring was attested while + /// `node_modules/lpN` installed left-pad@1.3.0 unpatched: a root + /// `file:` directory or tarball outside the root (yarn installs it all + /// the same), a tarball whose root directory is not `package/` (yarn + /// strips the first segment whatever it is), a non-registry url, and an + /// absolute path (never read: an untrusted lock could name any file or + /// share). Each now contests the ref. Controls: an outside-root copy + /// holding another package; an unreadable url copy of another version. + /// A key the root `package.json` doesn't declare (a workspace member's, + /// which yarn 1 also locks root-relative) contests the same way. + #[tokio::test] + async fn issue_1516_unread_classic_copies_contest_the_ref() { + let hosted = hosted_url("npm", "left-pad", "1.3.0", UUID_A, "left-pad-1.3.0.tgz"); + let wirings = [ + ( + "hosted", + classic_block("left-pad@1.3.0", "1.3.0", &hosted, Some(SRI)), + ), + ( + "vendored", + classic_block( + "left-pad@1.3.0", + "1.3.0", + &format!("file:./.socket/vendor/npm/{UUID_A}/left-pad-1.3.0.tgz#{SHA1}"), + Some(SRI), + ), + ), + ]; + let url_copy = |version: &str| { + classic_block( + "\"lp4@http://127.0.0.1:8799/lp.tgz\"", + version, + &format!("http://127.0.0.1:8799/lp.tgz#{SHA1}"), + None, + ) + }; + let copies = [ + ( + "outside-root file: directory", + "lp1@", + "\"lp1@file:../ext/lpdir\":\n version \"1.3.0\"\n\n".to_string(), + ), + ( + "outside-root file: tarball", + "lp2@", + classic_block( + "\"lp2@file:../ext/left-pad-1.3.0.tgz\"", + "1.3.0", + &format!("file:../ext/left-pad-1.3.0.tgz#{SHA1}"), + None, + ), + ), + ( + "tarball rooted at left-pad-1.3.0/", + "lp3@", + classic_block( + "\"lp3@file:./lp.tgz\"", + "1.3.0", + &format!("file:./lp.tgz#{SHA1}"), + None, + ), + ), + ("non-registry url", "lp4@", url_copy("1.3.0")), + ( + "absolute file: directory", + "lp5@", + "\"lp5@file:/srv/ext/lpdir\":\n version \"1.3.0\"\n\n".to_string(), + ), + ]; + let project = |blocks: &[String], fork: &str| { + let p = Project::nested("p"); + p.write("yarn.lock", classic(blocks)); + p.write( + "package.json", + serde_json::json!({ + "name": "p", + "version": "1.0.0", + "dependencies": { + "left-pad": "1.3.0", + "lp1": "file:../ext/lpdir", + "lp3": "file:./lp.tgz", + "lp4": "http://127.0.0.1:8799/lp.tgz", + "lp5": "file:/srv/ext/lpdir", + }, + "devDependencies": { "lp2": "file:../ext/left-pad-1.3.0.tgz" }, + }) + .to_string(), + ); + p.write( + "../ext/lpdir/package.json", + format!(r#"{{"name":"{fork}","version":"1.3.0"}}"#), + ); + p.write("../ext/left-pad-1.3.0.tgz", npm_tgz(fork, "1.3.0")); + p.write("lp.tgz", npm_tgz_rooted("left-pad-1.3.0", fork, "1.3.0")); + p + }; + for (mode, wired) in &wirings { + for (case, dep, copy) in &copies { + let out = run(&project(&[wired.clone(), copy.clone()], "left-pad")).await; + assert!(out.refs.is_empty(), "{mode} {case}: {:#?}", out.refs); + assert!( + out.diagnostics + .iter() + .any(|d| d.code == DIAG_REF_UNATTRIBUTABLE + && d.detail.contains(*dep) + && d.detail.contains("UNPATCHED")), + "{mode} {case}: {:#?}", + out.diagnostics + ); + } + // The readable copies hold another package: not left-pad. + for (case, _, copy) in &copies[..3] { + let out = run(&project(&[wired.clone(), copy.clone()], "other-pkg")).await; + assert_eq!(out.refs.len(), 1, "{mode} {case}: {:#?}", out.diagnostics); + } + // An unreadable copy of another version can't be left-pad@1.3.0. + let out = run(&project(&[wired.clone(), url_copy("2.0.0")], "left-pad")).await; + assert_eq!( + out.refs.len(), + 1, + "{mode} url 2.0.0: {:#?}", + out.diagnostics + ); + // yarn 1 locks a `file:` key relative to the root and normalized, + // whoever declared it (a member's `file:../../../ext/lpdir`, the + // root's `file:../ext/lpdir/`): a key the root `package.json` + // doesn't spell is read all the same. + let p = project(&[wired.clone(), copies[0].2.clone()], "left-pad"); + p.write( + "package.json", + r#"{"name":"p","version":"1.0.0","workspaces":["packages/*"],"dependencies":{"left-pad":"1.3.0"}}"#, + ); + let out = run(&p).await; + assert!(out.refs.is_empty(), "{mode} undeclared: {:#?}", out.refs); + } + } + + /// #1516: an unidentified berry copy contests a ref the berry extractor + /// confirms only after reading the copies (a vendored entry, confirmed + /// by `resolutions`): the contest is matched once every ref is in. + #[tokio::test] + async fn issue_1516_unread_berry_copy_contests_a_vendored_ref() { + let wired = berry_vendored_block("left-pad", "1.3.0", UUID_A); + let url = |version: &str| { + berry_block( + "lp2@http://127.0.0.1:8799/lp.tgz", + version, + "lp2@http://127.0.0.1:8799/lp.tgz", + None, + ) + }; + let project = |blocks: &[String]| { + let p = Project::new(); + p.write("yarn.lock", berry(blocks)); + p.write( + "package.json", + package_json_with_resolutions(serde_json::json!({ + "left-pad": format!("file:./.socket/vendor/npm/{UUID_A}/left-pad-1.3.0.tgz"), + })), + ); + p + }; + let out = run(&project(&[wired.clone(), url("1.3.0")])).await; + assert!(out.refs.is_empty(), "{:#?}", out.refs); + assert!( + out.diagnostics + .iter() + .any(|d| d.code == DIAG_REF_UNATTRIBUTABLE && d.detail.contains("lp2@")), + "{:#?}", + out.diagnostics + ); + assert_refs( + &run(&project(&[wired, url("2.0.0")])).await, + &[("pkg:npm/left-pad@1.3.0", UUID_A, WiringMode::Vendored)], + ); + } + /// Socket-shaped blocks that fail validation are DIAGNOSED, never refs: /// no version, mixed-package key, unsafe name, a leaf naming another /// package, a traversal leaf, a non-canonical vendored uuid. diff --git a/docs/ecosystems.md b/docs/ecosystems.md index adfc35a62..5f9a6a86e 100644 --- a/docs/ecosystems.md +++ b/docs/ecosystems.md @@ -285,7 +285,13 @@ Each Legacy format has an upgrade path and an undo path. Both work in v5: reaches that copy, so it stays unpatched. Which package such a copy is comes from the copy itself (the tarball's or directory's `package.json`, or the registry package a URL's path names; a berry `portal:` / `link:` target's `package.json` also gives the - version, since the lock records `0.0.0-use.local`). Hosted mode still pins the + version, since the lock records `0.0.0-use.local`). The copy is read wherever yarn + installs it from, including a `file:` path outside the project (`file:../shared/lp`), + and a tarball's `package.json` is found under whatever top-level directory it has, as + yarn strips the first path segment. A copy whose package can't be read (a non-registry + URL, a missing directory) is treated as possibly any package at its lock version: it + is named as a copy it may be, and `vex` attests nothing that lock wires at that + version. Hosted mode still pins the registry entry and names the copy (`redirect_copy_unpatched`, unless a yarn classic skip warning already names it); vendored mode still wires the registry entry and names it (`vendor_copy_unpatched` for berry; the yarn classic codes below for classic); and