Fix yarn renamed-copy identity fail-open (#1516)

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(&copy).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<u8> = [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 {

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<String> {
+        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<CopyRef> {
     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<CopyRef> {
     if reference.starts_with("http") {
         return Some(CopyRef::Url(
@@ -153,8 +164,8 @@ pub(crate) fn berry_copy_ref(reference: &str) -> Option<CopyRef> {
 
 /// 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<String> {
     let workspace = reference
         .split_once("::")
@@ -171,7 +182,31 @@ fn berry_copy_path(reference: &str, path: &str) -> Option<String> {
     } 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<String> {
+    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<serde_json::Value> {
@@ -181,7 +216,7 @@ fn parse_manifest(bytes: &[u8]) -> Option<serde_json::Value> {
 
 /// `name` of the `package.json` inside an npm tarball.
 fn tarball_manifest_name(bytes: &[u8]) -> Option<String> {
-    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}")),

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 `<repo>-<tag>/`), 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<HashMap<String, Vec<u8>>, ArchiveError> {
+    let raw = read_archive_from_reader(bytes, false, false)?;
+    let mut out: HashMap<String, Vec<u8>> = HashMap::new();
+    let mut ambiguous: std::collections::HashSet<String> = std::collections::HashSet::new();
+    let mut roots: HashMap<String, String> = 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(