Fix yarn classic copy identity read two ways (#1951)

crates/socket-patch-cli/tests/e2e_redirect_yarn_classic_build.rs

@@ -1542,9 +1542,10 @@ 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.
+/// it stays unpatched. `scan --mode hosted` must name the copy exactly as
+/// it names an in-root one (`redirect_yarn_classic_directory_skipped`,
+/// read through the reader vendored and VEX use, #1951), once, 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() {
@@ -1635,11 +1636,22 @@ async fn classic_outside_root_copy_under_another_name_is_named_and_not_attested(
         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}"
+    let naming: Vec<&str> = env["warnings"]
+        .as_array()
+        .into_iter()
+        .flatten()
+        .filter(|w| {
+            w["detail"]
+                .as_str()
+                .is_some_and(|d| d.contains("lp2@file:../ext/lpdir"))
+        })
+        .filter_map(|w| w["code"].as_str())
+        .collect();
+    assert_eq!(
+        naming,
+        ["redirect_yarn_classic_directory_skipped"],
+        "the outside-root copy must be named once, as an in-root one is: {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"),

crates/socket-patch-core/src/formats/yarn/copy.rs

@@ -11,7 +11,7 @@
 //! [`CopyRef`], and so cannot disagree on which package it is.
 
 use super::patterns::split_classic_pattern;
-use super::source::{is_tarball_path, registry_tarball_name};
+use super::source::{classic_copy_source, is_tarball_path, registry_tarball_name, CopySource};
 
 /// Where a yarn lock entry's copy comes from, read for the package it holds.
 #[derive(Debug, Clone, PartialEq, Eq)]
@@ -165,6 +165,36 @@ pub(crate) fn classic_copy_ref(patterns: &[String], resolved: Option<&str>) -> O
     }
 }
 
+/// The package a classic `file:` directory or url copy really installs,
+/// whatever dependency name its key carries (#1236), with where yarn 1
+/// installs it from: read through [`classic_copy_ref`] and
+/// [`CopyRef::identity`], the reader vendored and VEX use, so the hosted
+/// rewriter makes the same decision for every copy (#1951). `read_text`
+/// gets [`CopyRef::read_path`] (root-relative, possibly climbing above the
+/// root; never absolute). `None` for any other block, a `file:` tarball
+/// (whose bytes a text reader can't open), a url naming no registry
+/// package, or an unreadable or nameless manifest.
+pub(crate) fn classic_copy_name(
+    patterns: &[String],
+    resolved: Option<&str>,
+    version: &str,
+    read_text: impl Fn(&str) -> Option<String>,
+) -> Option<(String, CopySource)> {
+    let source = classic_copy_source(patterns, resolved);
+    let copy = classic_copy_ref(patterns, resolved)?;
+    match (source, &copy) {
+        (CopySource::Directory, CopyRef::Directory(_))
+        | (CopySource::RemoteTarball, CopyRef::Url(_)) => {}
+        _ => return None,
+    }
+    let text = match copy.read_path() {
+        Some(rel) => Some(read_text(&rel)?),
+        None => None,
+    };
+    let (name, _) = copy.identity(version, text.as_deref().map(str::as_bytes))?;
+    Some((name, source))
+}
+
 /// The copy a berry locator `reference` installs from: a `file:` tarball or
 /// directory, a `portal:` / `link:` directory, an `exec:` script (each
 /// relative to the workspace its `locator=` binding names), an http(s)

crates/socket-patch-core/src/formats/yarn/source.rs

@@ -1,7 +1,6 @@
 //! Where yarn 1 installs a classic lock block's copy from, decided once for
 //! every mode that reads or rewrites the block.
 
-use super::copy::manifest_name;
 use super::patterns::split_classic_pattern;
 use crate::vendor::npm_origin::npm_spec_is_registry;
 
@@ -75,59 +74,6 @@ pub(crate) fn classic_copy_source(patterns: &[String], resolved: Option<&str>) -
     }
 }
 
-/// The root-relative `file:` directory a classic block's key names, if
-/// any: yarn 1 COPIES it into node_modules under the DEPENDENCY name
-/// (`"lp2@file:./lpdir"`), so which package that copy is comes from the
-/// directory's own `package.json` (#1236). `None` for a `file:` tarball or
-/// a path that leaves the root.
-pub(crate) fn classic_file_directory(patterns: &[String]) -> Option<String> {
-    patterns.iter().find_map(|p| {
-        let path = split_classic_pattern(p)?.1.strip_prefix("file:")?;
-        let path = path.split('#').next().unwrap_or_default();
-        if is_tarball_path(path) {
-            return None;
-        }
-        crate::utils::cargo_workspace::normalize_rel("", path)
-    })
-}
-
-/// The package a classic `file:` directory or url copy really installs,
-/// whatever dependency name its key carries (#1236): the directory's
-/// `package.json` `name` (read through `read_text`, given the
-/// root-relative manifest path), or the registry package a url tarball's
-/// path names. `None` when neither says (a `file:` tarball, a
-/// non-registry url, an unreadable manifest).
-pub(crate) fn classic_copy_real_name(
-    patterns: &[String],
-    resolved: Option<&str>,
-    version: &str,
-    read_text: impl Fn(&str) -> Option<String>,
-) -> Option<(String, CopySource)> {
-    match classic_copy_source(patterns, resolved) {
-        CopySource::Directory => {
-            let dir = classic_file_directory(patterns)?;
-            let manifest = if dir.is_empty() {
-                "package.json".to_string()
-            } else {
-                format!("{dir}/package.json")
-            };
-            let name = manifest_name(read_text(&manifest)?.as_bytes())?;
-            Some((name, CopySource::Directory))
-        }
-        CopySource::RemoteTarball => {
-            let url = resolved?.split('#').next().unwrap_or_default();
-            if !url.starts_with("http") {
-                return None;
-            }
-            Some((
-                registry_tarball_name(url, version)?,
-                CopySource::RemoteTarball,
-            ))
-        }
-        _ => None,
-    }
-}
-
 /// The package an npm registry tarball url serves, from its
 /// `/<name>/-/<leaf>-<version>.tgz` path (`<name>` may be `@scope/leaf`,
 /// its `@` / `/` possibly percent-encoded); `None` for any other shape.

crates/socket-patch-core/src/hosted/engine.rs

@@ -693,26 +693,25 @@ pub async fn read_candidate_files(
         out.read(view, unreadable, NPMRC_REL).await;
         // A `file:` directory copy is locked under the DEPENDENCY name, so
         // the classic rewriter reads which package it is from the
-        // directory's `package.json` (#1236). Advisory: never rewritten,
-        // and an unreadable one only leaves that copy unnamed.
-        let dirs: BTreeSet<String> = out
+        // directory's `package.json` (#1236), at the path vendored and VEX
+        // read it from (#1951): outside the root too, never at an absolute
+        // path. Advisory: never rewritten, and an unreadable one only
+        // leaves that copy unnamed.
+        let manifests: BTreeSet<String> = out
             .files
             .get("yarn.lock")
             .map(|lock| {
+                use crate::formats::yarn::copy::{classic_copy_ref, CopyRef};
                 crate::vendor::lock_inventory::yarn::classic_entries(lock)
                     .iter()
-                    .filter_map(|e| {
-                        crate::formats::yarn::source::classic_file_directory(&e.patterns)
+                    .filter_map(|e| match classic_copy_ref(&e.patterns, None)? {
+                        copy @ CopyRef::Directory(_) => copy.read_path(),
+                        _ => None,
                     })
                     .collect()
             })
             .unwrap_or_default();
-        for dir in dirs {
-            let rel = if dir.is_empty() {
-                "package.json".to_string()
-            } else {
-                format!("{dir}/package.json")
-            };
+        for rel in manifests {
             if out.files.contains_key(&rel) {
                 continue;
             }
@@ -4329,6 +4328,57 @@ mod tests {
         (read, done)
     }
 
+    /// #1951: the hosted run reads a yarn classic `file:` directory copy's
+    /// `package.json` where vendored and VEX read it
+    /// ([`crate::formats::yarn::copy::CopyRef::read_path`]): from outside
+    /// the root too, since yarn 1 installs `file:../ext/lpdir` all the same,
+    /// but never at an absolute path a lock names (#1516).
+    #[tokio::test]
+    async fn issue_1951_yarn_classic_copy_manifests_read_as_vendored_reads_them() {
+        let tmp = tempfile::tempdir().unwrap();
+        let root = tmp.path().join("app");
+        let manifest = r#"{"name":"left-pad","version":"1.3.0"}"#;
+        for dir in ["app/lpdir", "ext/lpdir", "abs/lpdir"] {
+            let dir = tmp.path().join(dir);
+            std::fs::create_dir_all(&dir).unwrap();
+            std::fs::write(dir.join("package.json"), manifest).unwrap();
+        }
+        let abs = tmp.path().join("abs/lpdir");
+        let abs = abs.to_str().unwrap().replace('\\', "/");
+        std::fs::write(
+            root.join("yarn.lock"),
+            format!(
+                "# yarn lockfile v1\n\n\n\
+                 left-pad@1.3.0:\n  version \"1.3.0\"\n  \
+                 resolved \"https://registry.yarnpkg.com/left-pad/-/left-pad-1.3.0.tgz#bbbb\"\n\n\
+                 \"lp2@file:./lpdir\":\n  version \"1.3.0\"\n\n\
+                 \"lp3@file:../ext/lpdir\":\n  version \"1.3.0\"\n\n\
+                 \"lp4@file:{abs}\":\n  version \"1.3.0\"\n"
+            ),
+        )
+        .unwrap();
+        let candidates = vec![left_pad_candidate()];
+        let read =
+            read_candidate_files(&ProjectView::Disk(&root), &BTreeSet::new(), &candidates).await;
+        assert_eq!(
+            read.files.get("lpdir/package.json").map(String::as_str),
+            Some(manifest)
+        );
+        assert_eq!(
+            read.files
+                .get("../ext/lpdir/package.json")
+                .map(String::as_str),
+            Some(manifest),
+            "{:?}",
+            read.files.keys()
+        );
+        assert!(
+            !read.files.keys().any(|k| k.contains("abs/lpdir")),
+            "{:?}",
+            read.files.keys()
+        );
+    }
+
     #[tokio::test]
     async fn issue_490_the_root_manifest_overrides_reach_the_npm_rewriter() {
         let redirected = |done: &Rewritten| {

crates/socket-patch-core/src/patch/redirect/mod.rs

@@ -4590,7 +4590,7 @@ fn rewrite_yarn_classic_with(
         .zip(&heads)
         .map(|(b, (patterns, _))| {
             let version = classic_field(&b.lines, "version")?;
-            let (name, source) = crate::formats::yarn::source::classic_copy_real_name(
+            let (name, source) = crate::formats::yarn::copy::classic_copy_name(
                 patterns,
                 classic_field(&b.lines, "resolved"),
                 version,
@@ -13751,6 +13751,90 @@ mod tests {
         }
     }
 
+    /// #1951: the hosted rewriter reads which package a yarn classic
+    /// `file:` / url copy holds through the same reader vendored and VEX use
+    /// ([`crate::formats::yarn::copy::classic_copy_ref`] and
+    /// [`crate::formats::yarn::copy::CopyRef::identity`]), so the modes
+    /// identify the same copies. (A copy no reader identifies is a separate
+    /// rule: vendored keeps it as a possible copy, #1516.) yarn 1 installs
+    /// `file:../ext/lpdir` from outside the root as readily as from inside
+    /// it, so that copy is read and named; an absolute `file:` path is never
+    /// read (a lock is untrusted input, #1516); a codeload tarball names no
+    /// registry package.
+    #[test]
+    fn issue_1951_yarn_classic_copy_named_as_vendored_reads_it() {
+        use crate::formats::yarn::copy::classic_copy_ref;
+        let ovr = npm_override(
+            "left-pad",
+            "1.3.0",
+            "http://p.test/lp.tgz",
+            "sha512-PATCHED==",
+        );
+        let registry_block = "left-pad@1.3.0:\n  version \"1.3.0\"\n  \
+             resolved \"https://registry.yarnpkg.com/left-pad/-/left-pad-1.3.0.tgz#bbbb\"\n  \
+             integrity sha512-UPSTREAMupstream==\n";
+        let left_pad = r#"{"name":"left-pad","version":"1.3.0"}"#;
+        let manifests = BTreeMap::from([
+            (
+                "../ext/lpdir/package.json".to_string(),
+                left_pad.to_string(),
+            ),
+            ("/abs/lpdir/package.json".to_string(), left_pad.to_string()),
+            ("lpdir/package.json".to_string(), left_pad.to_string()),
+        ]);
+        let codeload = "https://codeload.github.com/stevemao/left-pad/tar.gz/abc";
+        for (case, patterns, resolved, named) in [
+            ("in-root directory", "lp2@file:./lpdir", None, true),
+            ("out-of-root directory", "lp2@file:../ext/lpdir", None, true),
+            ("absolute directory", "lp2@file:/abs/lpdir", None, false),
+            (
+                "codeload tarball",
+                "lp2@stevemao/left-pad#v1.3.0",
+                Some(codeload),
+                false,
+            ),
+        ] {
+            // The package vendored and VEX identify this copy as.
+            let pats = vec![patterns.to_string()];
+            let vendored = classic_copy_ref(&pats, resolved).and_then(|copy| {
+                let bytes = copy
+                    .read_path()
+                    .and_then(|rel| manifests.get(&rel))
+                    .map(|t| t.as_bytes());
+                copy.identity("1.3.0", bytes)
+            });
+            assert_eq!(
+                vendored.is_some(),
+                named,
+                "{case}: vendored reads {vendored:?}"
+            );
+
+            let mut block = format!("\"{patterns}\":\n  version \"1.3.0\"\n");
+            if let Some(url) = resolved {
+                block.push_str(&format!("  resolved \"{url}\"\n"));
+            }
+            let mut files = manifests.clone();
+            files.insert(
+                "yarn.lock".to_string(),
+                format!("# yarn lockfile v1\n\n\n{registry_block}\n{block}"),
+            );
+            let mut r = RewriteResult::default();
+            rewrite_yarn_classic(&files, std::slice::from_ref(&ovr), &mut r);
+            assert!(
+                r.files["yarn.lock"].contains("resolved \"http://p.test/lp.tgz#5ha1\""),
+                "{case}: the registry block is still pinned: {:?}",
+                r.warnings
+            );
+            let hosted = r.warnings.iter().any(|w| w.detail.contains("lp2@"));
+            assert_eq!(hosted, named, "{case}: {:?}", r.warnings);
+            assert_eq!(
+                r.bundled_skipped_uuids.contains(&ovr.patch_uuid),
+                named,
+                "{case}"
+            );
+        }
+    }
+
     /// #363: yarn 1 fetches a git-pattern block with its git fetcher from
     /// the block's `resolved`, so a hosted tarball there makes every later
     /// install fail. The block stays byte-identical with a named warning,