Fix yarn copies with invalid manifest names (#1598)

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

@@ -1885,10 +1885,29 @@ async fn issue_939_berry_renamed_copy_is_named() {
 /// plus `extra` files, and return its `redirect_copy_unpatched` details
 /// (after checking the registry entry is still pinned).
 async fn berry_linked_copy_details(uuid: &str, spec: &str, extra: &[(&str, &str)]) -> Vec<String> {
-    let ws = "app%40workspace%3A.";
-    let key = format!("lp2@{spec}::locator={ws}");
     let server = MockServer::start().await;
     mount_left_pad_grant(&server, uuid).await;
+    let mut files = berry_linked_copy_files(spec);
+    for (rel, text) in extra {
+        files.insert(rel.to_string(), text.as_bytes().to_vec());
+    }
+    let output = run_engine(&server, build_input(&files, &[], options(false))).await;
+    let project = &output.projects[0];
+    assert!(project.error.is_none(), "{spec}: {:?}", project.error);
+    assert_eq!(
+        project.redirected.len(),
+        1,
+        "{spec}: {:?} {:#}",
+        project.skipped,
+        project.redirect
+    );
+    copy_unpatched_details(project)
+}
+
+/// A berry project depending on registry left-pad and on `lp2` at `spec`.
+fn berry_linked_copy_files(spec: &str) -> BTreeMap<String, Vec<u8>> {
+    let ws = "app%40workspace%3A.";
+    let key = format!("lp2@{spec}::locator={ws}");
     let mut files = BTreeMap::new();
     files.insert(
         "package.json".to_string(),
@@ -1914,20 +1933,7 @@ async fn berry_linked_copy_details(uuid: &str, spec: &str, extra: &[(&str, &str)
         )
         .into_bytes(),
     );
-    for (rel, text) in extra {
-        files.insert(rel.to_string(), text.as_bytes().to_vec());
-    }
-    let output = run_engine(&server, build_input(&files, &[], options(false))).await;
-    let project = &output.projects[0];
-    assert!(project.error.is_none(), "{spec}: {:?}", project.error);
-    assert_eq!(
-        project.redirected.len(),
-        1,
-        "{spec}: {:?} {:#}",
-        project.skipped,
-        project.redirect
-    );
-    copy_unpatched_details(project)
+    files
 }
 
 /// #1540: a berry `portal:` / `link:` copy with an ABSOLUTE path is never
@@ -2017,3 +2023,65 @@ async fn issue_1579_berry_portal_placeholder_and_versionless_copies_are_named()
         }
     }
 }
+
+/// #1598: a linked manifest whose `name` is empty or no valid npm name
+/// names no package, and one whose `version` is empty or no semver names
+/// no version, exactly like a missing key (#1579). The hosted scan read
+/// them literally ("a package called `""`", "another version"), reported a
+/// clean success, and its in-run VEX attested `not_affected` while yarn
+/// linked unpatched left-pad as `node_modules/lp2`. Now each is named.
+#[tokio::test]
+async fn issue_1598_berry_invalid_manifest_name_or_version_is_named() {
+    const UUID: &str = "15981598-1598-4598-8598-159815981598";
+    for protocol in ["portal", "link"] {
+        let spec = format!("{protocol}:./vend/lp");
+        for text in [
+            r#"{"name":"","version":"1.3.0"}"#,
+            r#"{"name":"../x","version":"1.3.0"}"#,
+            r#"{"name":" left-pad","version":"1.3.0"}"#,
+            r#"{"name":"left-pad","version":""}"#,
+            r#"{"name":"left-pad","version":"v1.3.0"}"#,
+        ] {
+            let details =
+                berry_linked_copy_details(UUID, &spec, &[("vend/lp/package.json", text)]).await;
+            assert_eq!(details.len(), 1, "{protocol} {text}: {details:?}");
+            assert!(
+                details[0].contains(&format!("lp2@{spec}::")) && details[0].contains("UNPATCHED"),
+                "{protocol} {text}: {}",
+                details[0]
+            );
+        }
+    }
+}
+
+/// #1598 on disk: the real binary's `scan --mode hosted` over the same
+/// project names the copy too, not only the in-memory engine.
+#[tokio::test]
+async fn issue_1598_berry_invalid_manifest_name_or_version_is_named_on_disk() {
+    const UUID: &str = "15981598-1598-4598-8598-159815981598";
+    for text in [
+        r#"{"name":"","version":"1.3.0"}"#,
+        r#"{"name":"../x","version":"1.3.0"}"#,
+        r#"{"name":"left-pad","version":""}"#,
+    ] {
+        let server = MockServer::start().await;
+        mount_left_pad_grant(&server, UUID).await;
+        let mut files = berry_linked_copy_files("portal:./vend/lp");
+        files.insert("vend/lp/package.json".to_string(), text.as_bytes().to_vec());
+        let disk = run_disk(&server, &files, false);
+        let warnings = disk.redirect["warnings"]
+            .as_array()
+            .cloned()
+            .unwrap_or_default();
+        assert!(
+            warnings
+                .iter()
+                .any(|w| w["code"] == "redirect_copy_unpatched"
+                    && w["detail"]
+                        .as_str()
+                        .is_some_and(|d| d.contains("UNPATCHED"))),
+            "{text}: {:#}",
+            disk.envelope
+        );
+    }
+}

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, manifest_name, registry_tarball_name};
+use super::source::{is_tarball_path, registry_tarball_name};
 
 /// Where a yarn lock entry's copy comes from, read for the package it holds.
 #[derive(Debug, Clone, PartialEq, Eq)]
@@ -81,9 +81,7 @@ impl CopyRef {
             CopyRef::Directory(_) => manifest_name(bytes?)?,
             CopyRef::Linked(..) => {
                 let manifest = parse_manifest(bytes?)?;
-                let name = manifest.get("name")?.as_str()?.to_string();
-                let version = manifest.get("version")?.as_str()?.to_string();
-                return Some((name, version));
+                return Some((package_name(&manifest)?, package_version(&manifest)?));
             }
             CopyRef::Url(url) => registry_tarball_name(url, lock_version)?,
         };
@@ -97,9 +95,7 @@ impl CopyRef {
     /// invalid npm name names none, so the copy may be any package).
     pub(crate) fn linked_name(&self, bytes: Option<&[u8]>) -> Option<String> {
         match self {
-            CopyRef::Linked(..) => {
-                manifest_name(bytes?).filter(|n| crate::vendor::is_safe_npm_name(n))
-            }
+            CopyRef::Linked(..) => manifest_name(bytes?),
             _ => None,
         }
     }
@@ -239,6 +235,32 @@ fn parse_manifest(bytes: &[u8]) -> Option<serde_json::Value> {
     serde_json::from_slice::<serde_json::Value>(bytes).ok()
 }
 
+/// `name` of a `package.json`, when it names a package ([`package_name`]).
+pub(crate) fn manifest_name(bytes: &[u8]) -> Option<String> {
+    package_name(&parse_manifest(bytes)?)
+}
+
+/// A parsed manifest's `name`, when it names a package at all. An empty
+/// name, or one npm could never have published (a path such as `../x`,
+/// whitespace, any character outside npm's url-safe set), names none, so a
+/// copy holding it may be any package (#1579, #1598): reading it literally
+/// would call the copy "some other package" and let it escape every
+/// contest.
+fn package_name(manifest: &serde_json::Value) -> Option<String> {
+    let name = manifest.get("name")?.as_str()?;
+    crate::vendor::vlt_lock_text::is_registry_package_name(name).then(|| name.to_string())
+}
+
+/// A parsed manifest's `version`, when it names one: npm's semver grammar,
+/// with build metadata dropped as npm drops it (`1.3.0+build.5` IS 1.3.0).
+/// An empty or non-semver version (`""`, `v1.3.0`, `latest`) names none,
+/// as a missing one does (#1579, #1598).
+fn package_version(manifest: &serde_json::Value) -> Option<String> {
+    let version = manifest.get("version")?.as_str()?;
+    crate::vendor::vlt_lock_text::is_npm_semver(version)
+        .then(|| version.split('+').next().unwrap_or(version).to_string())
+}
+
 /// `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_any_root(bytes).ok()?;
@@ -405,6 +427,88 @@ mod tests {
         assert_eq!(dir.identity("1.3.0", None), None);
     }
 
+    /// #1598: an empty or invalid npm name names no package, and an empty
+    /// or non-semver version names no version, as a missing key does
+    /// (#1579), for every copy whose manifest is read.
+    #[test]
+    fn invalid_manifest_name_or_version_identifies_nothing() {
+        let linked = CopyRef::Linked("vend/lp".into(), LinkKind::Portal);
+        let dir = CopyRef::Directory("lpdir".into());
+        let tarball = CopyRef::Tarball("lp.tgz".into());
+        for name in [
+            "",
+            "../x",
+            " left-pad",
+            "left-pad ",
+            "left pad",
+            "a/b/c",
+            "left-pad\u{200b}",
+            "left-pad?",
+            "@/left-pad",
+            ".left-pad",
+        ] {
+            let manifest = serde_json::json!({ "name": name, "version": "1.3.0" }).to_string();
+            assert_eq!(
+                linked.identity("0.0.0-use.local", Some(manifest.as_bytes())),
+                None,
+                "linked {name:?}"
+            );
+            assert_eq!(linked.linked_name(Some(manifest.as_bytes())), None);
+            assert_eq!(
+                dir.identity("1.3.0", Some(manifest.as_bytes())),
+                None,
+                "directory {name:?}"
+            );
+            let bytes = crate::vex::discover::testing::npm_tgz_rooted("package", name, "1.3.0");
+            assert_eq!(
+                tarball.identity("1.3.0", Some(&bytes)),
+                None,
+                "tarball {name:?}"
+            );
+        }
+        for version in [
+            "",
+            " 1.3.0",
+            "v1.3.0",
+            "latest",
+            "1.3",
+            "9007199254740993.0.0",
+        ] {
+            let manifest =
+                serde_json::json!({ "name": "left-pad", "version": version }).to_string();
+            assert_eq!(
+                linked.identity("0.0.0-use.local", Some(manifest.as_bytes())),
+                None,
+                "linked version {version:?}"
+            );
+            assert_eq!(
+                linked.linked_name(Some(manifest.as_bytes())).as_deref(),
+                Some("left-pad"),
+                "a usable name still says which package (#1579)"
+            );
+        }
+        // Valid spellings keep their identity.
+        for (name, version) in [
+            ("left-pad", "1.3.0"),
+            ("@scope/left-pad", "1.3.0-beta.1"),
+            ("Left_Pad.js", "1.3.0"),
+        ] {
+            let manifest = serde_json::json!({ "name": name, "version": version }).to_string();
+            assert_eq!(
+                linked.identity("0.0.0-use.local", Some(manifest.as_bytes())),
+                Some((name.into(), version.into()))
+            );
+        }
+        // Build metadata is no part of npm's version: the copy IS 1.3.0.
+        assert_eq!(
+            linked.identity(
+                "0.0.0-use.local",
+                Some(br#"{"name":"left-pad","version":"1.3.0+build.5"}"#)
+            ),
+            Some(("left-pad".into(), "1.3.0".into()))
+        );
+    }
+
     /// #1579: a linked manifest naming a package but no version names that
     /// package; an empty or invalid name names none.
     #[test]

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

@@ -1,6 +1,7 @@
 //! 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;
 
@@ -127,16 +128,6 @@ pub(crate) fn classic_copy_real_name(
     }
 }
 
-/// `name` of a `package.json`.
-pub(crate) fn manifest_name(bytes: &[u8]) -> Option<String> {
-    let bytes = crate::formats::text::strip_bom_bytes(bytes);
-    serde_json::from_slice::<serde_json::Value>(bytes)
-        .ok()?
-        .get("name")?
-        .as_str()
-        .map(str::to_string)
-}
-
 /// 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.