Fix yarn portal copies read as no package (#1579)

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

@@ -1984,3 +1984,36 @@ async fn issue_1561_berry_missing_relative_portal_and_link_copies_are_named() {
         assert!(details.is_empty(), "{protocol}: {details:?}");
     }
 }
+
+/// #1579: a `portal:` target holding only a `.gitkeep` placeholder (yarn
+/// refuses a manifest-less portal, so it is not what gets installed), and a
+/// linked manifest with no `name`, may be any package; one naming left-pad
+/// with no version may be left-pad. The hosted scan pins the registry entry
+/// and names each as possibly unpatched; it reported a clean success.
+/// Controls: a `link:` to the placeholder (plain assets) and a versionless
+/// manifest naming another package are no copy of left-pad.
+#[tokio::test]
+async fn issue_1579_berry_portal_placeholder_and_versionless_copies_are_named() {
+    const UUID: &str = "15791579-1579-4579-8579-157915791579";
+    for protocol in ["portal", "link"] {
+        let spec = format!("{protocol}:./vend/lp");
+        for (file, text, named) in [
+            ("vend/lp/.gitkeep", "", protocol == "portal"),
+            ("vend/lp/package.json", r#"{"version":"1.3.0"}"#, true),
+            ("vend/lp/package.json", r#"{"name":"left-pad"}"#, true),
+            ("vend/lp/package.json", r#"{"name":"other-pkg"}"#, false),
+        ] {
+            let details = berry_linked_copy_details(UUID, &spec, &[(file, text)]).await;
+            if !named {
+                assert!(details.is_empty(), "{protocol} {text:?}: {details:?}");
+                continue;
+            }
+            assert_eq!(details.len(), 1, "{protocol} {text:?}: {details:?}");
+            assert!(
+                details[0].contains(&format!("lp2@{spec}::")) && details[0].contains("UNPATCHED"),
+                "{protocol} {text:?}: {}",
+                details[0]
+            );
+        }
+    }
+}

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

@@ -26,12 +26,21 @@ pub(crate) enum CopyRef {
     /// `node_modules`: the lock records the placeholder version
     /// `0.0.0-use.local`, so the package's name AND version are its
     /// `package.json`'s.
-    Linked(String),
+    Linked(String, LinkKind),
     /// A url tarball (`#` fragment dropped): the package is the registry
     /// package its `/<name>/-/<leaf>-<version>.tgz` path names.
     Url(String),
 }
 
+/// Which berry protocol a [`CopyRef::Linked`] copy came from. yarn installs
+/// a `link:` to any directory, a package or plain assets, but refuses a
+/// `portal:` whose target has no `package.json` (#1579).
+#[derive(Debug, Clone, Copy, PartialEq, Eq)]
+pub(crate) enum LinkKind {
+    Portal,
+    Link,
+}
+
 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
@@ -42,14 +51,14 @@ impl CopyRef {
     /// 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 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() {
+            CopyRef::Directory(dir) | CopyRef::Linked(dir, _) => Some(if dir.is_empty() {
                 "package.json".to_string()
             } else {
                 format!("{dir}/package.json")
@@ -70,7 +79,7 @@ impl CopyRef {
         let name = match self {
             CopyRef::Tarball(_) => tarball_manifest_name(bytes?)?,
             CopyRef::Directory(_) => manifest_name(bytes?)?,
-            CopyRef::Linked(_) => {
+            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();
@@ -81,6 +90,20 @@ impl CopyRef {
         Some((name, lock_version.to_string()))
     }
 
+    /// The package a [`CopyRef::Linked`] copy's manifest names when it
+    /// names no version, so [`CopyRef::identity`] can't say: the copy may
+    /// be any version of that package, and of no other (#1579). `None` for
+    /// any other copy, or a manifest that names no package (an empty or
+    /// 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))
+            }
+            _ => None,
+        }
+    }
+
     /// Where the copy installs from, for a warning or diagnostic: "installs
     /// it from …" / "links it to …". A url is redacted, since the phrase
     /// may be published (a VEX document, a JSON envelope).
@@ -89,7 +112,7 @@ impl CopyRef {
             CopyRef::Tarball(rel) | CopyRef::Directory(rel) => {
                 format!("installs it from the user's file:{rel}")
             }
-            CopyRef::Linked(rel) => {
+            CopyRef::Linked(rel, _) => {
                 let rel = if rel.is_empty() { "." } else { rel };
                 format!("links it to the user's directory {rel} (portal:/link:)")
             }
@@ -142,12 +165,11 @@ pub(crate) fn berry_copy_ref(reference: &str) -> Option<CopyRef> {
         ));
     }
     let (path, linked) = if let Some(path) = reference.strip_prefix("file:") {
-        (path, false)
-    } else if let Some(path) = reference
-        .strip_prefix("portal:")
-        .or_else(|| reference.strip_prefix("link:"))
-    {
-        (path, true)
+        (path, None)
+    } else if let Some(path) = reference.strip_prefix("portal:") {
+        (path, Some(LinkKind::Portal))
+    } else if let Some(path) = reference.strip_prefix("link:") {
+        (path, Some(LinkKind::Link))
     } else {
         return None;
     };
@@ -156,8 +178,8 @@ pub(crate) fn berry_copy_ref(reference: &str) -> Option<CopyRef> {
     let path = path.split("::").next().unwrap_or_default();
     let path = path.split('#').next().unwrap_or_default();
     let rel = berry_copy_path(reference, path)?;
-    Some(if linked {
-        CopyRef::Linked(rel)
+    Some(if let Some(kind) = linked {
+        CopyRef::Linked(rel, kind)
     } else if is_tarball_path(path) {
         CopyRef::Tarball(rel)
     } else {
@@ -212,13 +234,6 @@ fn copy_path(base: &str, path: &str) -> Option<String> {
     ))
 }
 
-/// Whether `bytes` parse as a JSON `package.json` (any content): a copy
-/// whose manifest doesn't (a Git LFS pointer, an encrypted blob) can't be
-/// read for its package (#1561).
-pub(crate) fn is_json_manifest(bytes: &[u8]) -> bool {
-    parse_manifest(bytes).is_some()
-}
-
 fn parse_manifest(bytes: &[u8]) -> Option<serde_json::Value> {
     let bytes = crate::formats::text::strip_bom_bytes(bytes);
     serde_json::from_slice::<serde_json::Value>(bytes).ok()
@@ -320,11 +335,11 @@ mod tests {
         );
         assert_eq!(
             berry_copy_ref(&format!("portal:../lp{ws}")),
-            Some(CopyRef::Linked("packages/lp".into()))
+            Some(CopyRef::Linked("packages/lp".into(), LinkKind::Portal))
         );
         assert_eq!(
             berry_copy_ref("link:./vend/lp::locator=app%40workspace%3A."),
-            Some(CopyRef::Linked("vend/lp".into()))
+            Some(CopyRef::Linked("vend/lp".into(), LinkKind::Link))
         );
         assert_eq!(berry_copy_ref("npm:1.3.0"), None);
         assert_eq!(berry_copy_ref("workspace:packages/b"), None);
@@ -335,27 +350,43 @@ mod tests {
     #[test]
     fn absolute_linked_refs_keep_the_path_and_are_unread() {
         let ws = "::locator=app%40workspace%3A.";
-        for (reference, path) in [
-            (format!("portal:/home/me/lp{ws}"), "/home/me/lp"),
-            (format!("link:/home/me/lp{ws}"), "/home/me/lp"),
-            (format!("portal:C:/Users/me/lp{ws}"), "C:/Users/me/lp"),
+        for (reference, path, kind) in [
+            (
+                format!("portal:/home/me/lp{ws}"),
+                "/home/me/lp",
+                LinkKind::Portal,
+            ),
+            (
+                format!("link:/home/me/lp{ws}"),
+                "/home/me/lp",
+                LinkKind::Link,
+            ),
+            (
+                format!("portal:C:/Users/me/lp{ws}"),
+                "C:/Users/me/lp",
+                LinkKind::Portal,
+            ),
         ] {
             assert_eq!(
                 berry_copy_ref(&reference),
-                Some(CopyRef::Linked(path.into())),
+                Some(CopyRef::Linked(path.into(), kind)),
                 "{reference}"
             );
         }
-        assert!(CopyRef::Linked("/home/me/lp".into()).read_path().is_none());
+        assert!(CopyRef::Linked("/home/me/lp".into(), LinkKind::Portal)
+            .read_path()
+            .is_none());
         assert_eq!(
-            CopyRef::Linked("vend/lp".into()).read_path().as_deref(),
+            CopyRef::Linked("vend/lp".into(), LinkKind::Link)
+                .read_path()
+                .as_deref(),
             Some("vend/lp/package.json")
         );
     }
 
     #[test]
     fn linked_identity_reads_name_and_version_from_the_manifest() {
-        let linked = CopyRef::Linked("vend/lp".into());
+        let linked = CopyRef::Linked("vend/lp".into(), LinkKind::Portal);
         assert_eq!(linked.read_path().as_deref(), Some("vend/lp/package.json"));
         let manifest = br#"{"name":"left-pad","version":"1.3.0"}"#;
         assert_eq!(
@@ -373,4 +404,24 @@ mod tests {
         );
         assert_eq!(dir.identity("1.3.0", None), None);
     }
+
+    /// #1579: a linked manifest naming a package but no version names that
+    /// package; an empty or invalid name names none.
+    #[test]
+    fn linked_name_reads_a_valid_name_only() {
+        let linked = CopyRef::Linked("vend/lp".into(), LinkKind::Portal);
+        assert_eq!(
+            linked
+                .linked_name(Some(br#"{"name":"left-pad"}"#))
+                .as_deref(),
+            Some("left-pad")
+        );
+        for manifest in [&br#"{"name":""}"#[..], br#"{"name":"../x"}"#, br#"{}"#] {
+            assert_eq!(linked.linked_name(Some(manifest)), None);
+        }
+        assert_eq!(
+            CopyRef::Directory("lp".into()).linked_name(Some(br#"{"name":"left-pad"}"#)),
+            None
+        );
+    }
 }

crates/socket-patch-core/src/vendor/yarn_berry_lock.rs

@@ -275,11 +275,18 @@ impl NpmLockBackend for YarnBerryBackend {
                          dependency name, from {origin}; the name-keyed resolutions entry \
                          vendoring writes never reaches it, so that copy stays UNPATCHED"
                     )
+                } else if copy.versionless {
+                    format!(
+                        "{YARN_LOCK} entry `{key}` installs {name} under another dependency \
+                         name, from {origin}, whose package.json names no version, so it may \
+                         be {name}@{version}; the name-keyed resolutions entry vendoring \
+                         writes never reaches it, so if it is, that copy stays UNPATCHED"
+                    )
                 } else {
                     // An unidentified `portal:` / `link:` copy has no
                     // version to name (#1540, #1561).
                     let what =
-                        if matches!(copy.source, crate::formats::yarn::copy::CopyRef::Linked(_)) {
+                        if matches!(copy.source, crate::formats::yarn::copy::CopyRef::Linked(..)) {
                             "a package".to_string()
                         } else {
                             format!("version {version}")
@@ -2569,12 +2576,13 @@ __metadata:
     /// absolute one (#1540): named as a possible unpatched copy, while the
     /// registry entry still vendors. It was dropped silently. Control: a
     /// `link:` to an existing plain directory with no `package.json` is no
-    /// package copy and stays unnamed.
+    /// package copy and stays unnamed; a `portal:` to one is named, since
+    /// yarn refuses a manifest-less portal (#1579).
     #[tokio::test]
     async fn issue_1561_missing_relative_portal_and_link_targets_are_named() {
         for protocol in ["portal", "link"] {
             for target in ["missing", "empty", "lfs-pointer", "plain-dir"] {
-                let plain_dir = target == "plain-dir";
+                let plain_dir = target == "plain-dir" && protocol == "link";
                 let key = format!("lp2@{protocol}:./vend/lp::locator=vendor-spike%40workspace%3A.");
                 let block = format!(
                     "\n\"{key}\":\n  version: 0.0.0-use.local\n  resolution: \"{key}\"\n  \
@@ -2626,6 +2634,67 @@ __metadata:
         }
     }
 
+    /// #1579: a `portal:` target with no `package.json` (a `.gitkeep`
+    /// placeholder; yarn refuses a manifest-less portal, so it is not what
+    /// gets installed) and a linked manifest with no `name` are unknown, and
+    /// one naming left-pad with no version may be left-pad: each is named as
+    /// a possible unpatched copy while the registry entry still vendors.
+    /// Controls: a `link:` to a placeholder directory (plain assets) and a
+    /// versionless manifest naming another package stay unnamed.
+    #[tokio::test]
+    async fn issue_1579_portal_placeholder_and_versionless_linked_manifests_are_named() {
+        for protocol in ["portal", "link"] {
+            for (target, named) in [
+                ("gitkeep", protocol == "portal"),
+                ("no-name", true),
+                ("no-version-left-pad", true),
+                ("no-version-other", false),
+            ] {
+                let key = format!("lp2@{protocol}:./vend/lp::locator=vendor-spike%40workspace%3A.");
+                let block = format!(
+                    "\n\"{key}\":\n  version: 0.0.0-use.local\n  resolution: \"{key}\"\n  \
+                     languageName: node\n  linkType: soft\n"
+                );
+                let lock = format!("{B3_BEFORE_LOCK}{block}");
+                let fx = fixture_with(B3_BEFORE_PKG, &lock).await;
+                let dir = fx.tmp.path().join("vend/lp");
+                tokio::fs::create_dir_all(&dir).await.unwrap();
+                let (file, text) = match target {
+                    "gitkeep" => (".gitkeep", ""),
+                    "no-name" => ("package.json", r#"{"version":"1.3.0"}"#),
+                    "no-version-left-pad" => ("package.json", r#"{"name":"left-pad"}"#),
+                    _ => ("package.json", r#"{"name":"other-pkg"}"#),
+                };
+                tokio::fs::write(dir.join(file), text).await.unwrap();
+
+                let (result, entry, warnings) = expect_done(fx.vendor(false).await);
+                assert!(result.success, "{protocol}/{target}: {:?}", result.error);
+                assert!(entry.is_some(), "{protocol}/{target}: registry vendors");
+                let copies: Vec<&VendorWarning> = warnings
+                    .iter()
+                    .filter(|w| w.code == "vendor_copy_unpatched")
+                    .collect();
+                if !named {
+                    assert!(copies.is_empty(), "{protocol}/{target}: {warnings:?}");
+                    continue;
+                }
+                assert_eq!(copies.len(), 1, "{protocol}/{target}: {warnings:?}");
+                let (what, why) = if target == "no-version-left-pad" {
+                    ("installs left-pad under", "names no version")
+                } else {
+                    ("installs a package under", "can't be read")
+                };
+                assert!(
+                    copies[0].detail.contains(&format!("`\"{key}\"`"))
+                        && copies[0].detail.contains(what)
+                        && copies[0].detail.contains(why),
+                    "{protocol}/{target}: {}",
+                    copies[0].detail
+                );
+            }
+        }
+    }
+
     /// #939 (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