Fix berry unwind under a user yarn patch (#962)

crates/socket-patch-cli/CLI_CONTRACT.md

@@ -187,7 +187,14 @@ A missing referenced diff does not remove that protection.
   reversal (`rollback`, `remove`, `vendor --revert`) keep their existing behavior.
   `repair` may restore Socket-owned artifacts; any re-vendoring uses the same
   consent gate. Existing Socket pins are not automatically unwound when a user
-  adds a patch: inspect them and explicitly roll back if necessary.
+  adds a patch: inspect them and explicitly roll back if necessary. One
+  exception: a Yarn Berry hosted pin or vendored tarball that a user `patch:`
+  descriptor wraps (`yarn patch` run over the Socket pin) cannot be unwound
+  under it, since the wrapper would name a descriptor no lock entry resolves.
+  `rollback`, `remove` and `vendor --revert` refuse that package before
+  writing (hosted: a refused pin, exit 1; vendored:
+  `vendor_lock_still_wired_revert_blocked`, dry runs included) and ask for the
+  `patch:` descriptor to be replaced by the registry range first.
 
 Detection covers root and declared npm workspace manifests, ancestor workspace
 settings, and the following evidence:

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

@@ -1139,6 +1139,103 @@ async fn yarn_berry_catalog_dependency_is_pinned_and_rolled_back() {
     );
 }
 
+/// #962: the user ran `yarn patch` / `yarn patch-commit -s` over a hosted
+/// pin, so a `patch:` lock entry and a `resolutions` descriptor wrap the
+/// hosted tarball URL. Restoring the base entry to its `npm:` descriptor
+/// would leave that wrapper naming a URL no entry resolves any more: every
+/// `yarn install --immutable` then fails (YN0028), and a plain install
+/// re-pins the Socket tarball. `rollback` and `remove` must refuse the pin
+/// and write nothing.
+#[tokio::test]
+#[serial]
+async fn yarn_berry_rollback_refuses_a_pin_wrapped_by_a_user_yarn_patch() {
+    for command in ["rollback", "remove"] {
+        let server = MockServer::start().await;
+        mock_discovery(&server).await;
+        let hosted_url = HOSTED_URL.replace("http://patch.test", &server.uri());
+        mock_reference_with_berry_url(&server, &hosted_url).await;
+        mock_view(&server).await;
+        let tarball = upstream_tarball();
+        mock_npm_registry(
+            &server,
+            &vlt_hosted_common::sha512_sri(&tarball),
+            Some(tarball),
+        )
+        .await;
+
+        let tmp = tempfile::tempdir().unwrap();
+        write_berry_project(tmp.path());
+        let env = run_redirect_subprocess_with(
+            tmp.path(),
+            &server.uri(),
+            &["--patch-server-url", &server.uri()],
+        );
+        assert_eq!(vlt_hosted_common::redirected(&env), 1, "{env:#}");
+
+        // What yarn 4.18 `yarn patch-commit -s` writes over the hosted pin:
+        // the dependency becomes a `patch:` descriptor wrapping the tarball
+        // URL (with yarn's `%3A` escapes), locked by a `patch:` entry next to
+        // the hosted one, which stays (the `resolutions` selector is kept).
+        let wrapped = format!(
+            "patch:{NAME}@{}#~/.yarn/patches/{NAME}-http-735c2bb24a.patch",
+            hosted_url.replace(':', "%3A")
+        );
+        let pkg_path = tmp.path().join("package.json");
+        let mut pkg: serde_json::Value =
+            serde_json::from_str(&std::fs::read_to_string(&pkg_path).unwrap()).unwrap();
+        pkg["dependencies"][NAME] = serde_json::json!(wrapped);
+        std::fs::write(
+            &pkg_path,
+            serde_json::to_string_pretty(&pkg).unwrap() + "\n",
+        )
+        .unwrap();
+        let lock_path = tmp.path().join("yarn.lock");
+        let pinned = std::fs::read_to_string(&lock_path).unwrap();
+        let patched_entry = format!(
+            "\"{NAME}@{wrapped}\":\n  version: {VERSION}\n  \
+             resolution: \"{NAME}@{wrapped}::version={VERSION}&hash=e516cc\"\n  \
+             checksum: 10c0/{}\n  languageName: node\n  linkType: hard\n\n",
+            "4".repeat(128)
+        );
+        let workspace = "\"consumer@workspace:.\":";
+        assert!(pinned.contains(workspace), "{pinned}");
+        let lock = pinned
+            .replace(
+                &format!("    {NAME}: \"npm:^{VERSION}\"\n"),
+                &format!("    {NAME}: \"{wrapped}\"\n"),
+            )
+            .replace(workspace, &format!("{patched_entry}{workspace}"));
+        std::fs::write(&lock_path, &lock).unwrap();
+        let pkg_before = std::fs::read_to_string(&pkg_path).unwrap();
+
+        let (code, env) = hosted_unwind_json(tmp.path(), &server, &server.uri(), &[command, PURL]);
+        assert_eq!(
+            code,
+            Some(1),
+            "{command} must refuse the wrapped pin: {env:#}"
+        );
+        assert_eq!(
+            rollback_json::hosted_reverted(&env),
+            serde_json::json!([]),
+            "{command}: {env:#}"
+        );
+        assert!(
+            env.to_string().contains("yarn patch"),
+            "{command}: the refusal names the user's `yarn patch`: {env:#}"
+        );
+        assert_eq!(
+            std::fs::read_to_string(&lock_path).unwrap(),
+            lock,
+            "{command} leaves yarn.lock untouched"
+        );
+        assert_eq!(
+            std::fs::read_to_string(&pkg_path).unwrap(),
+            pkg_before,
+            "{command} leaves package.json untouched"
+        );
+    }
+}
+
 /// #404 upgrade path: a lock pinned by an earlier release carries the old
 /// `npm:<v>::__archiveUrl=<url>` resolution, which makes yarn's npm fetcher
 /// send registry auth to the patch host. `rollback` must still recognize and

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

@@ -114,6 +114,84 @@ pub(crate) fn classic_key_real_name(patterns: &[String]) -> Option<&str> {
     names.all(|n| n == Some(first)).then_some(first)
 }
 
+/// What a berry `patch:` reference wraps, every layer peeled.
+#[derive(Debug, PartialEq, Eq)]
+pub(crate) struct PatchLayers {
+    /// The innermost source, percent-decoded (`left-pad@npm:1.3.0`,
+    /// `left-pad@http://host/x.tgz`).
+    pub source: String,
+    /// Some layer applies a patch outside yarn's builtin compatibility
+    /// namespace (`yarn patch`, or a plugin's own builtin).
+    pub user_patch: bool,
+}
+
+/// Peel a berry `patch:` reference (`patch:…` or `name@patch:…`) layer by
+/// layer: a compatibility patch can wrap a user patch, or vice versa, and
+/// both still belong to the original package. `Ok(None)` when `spec` is no
+/// patch reference; `Err` names a reference too odd to read.
+pub(crate) fn patch_layers(spec: &str) -> Result<Option<PatchLayers>, &'static str> {
+    use crate::utils::purl::percent_decode_purl_component;
+
+    if !spec.starts_with("patch:") && !spec.contains("@patch:") {
+        return Ok(None);
+    }
+    let mut source = spec.to_owned();
+    let mut user_patch = false;
+    for _ in 0..16 {
+        let Some(inner) = source
+            .strip_prefix("patch:")
+            .or_else(|| source.split_once("@patch:").map(|(_, rest)| rest))
+        else {
+            return Ok(Some(PatchLayers { source, user_patch }));
+        };
+        let Some((original, patches)) = inner.split_once('#') else {
+            return Err("unrecognized patch: reference");
+        };
+        // Only Yarn's compatibility namespace is exempt. Plugins can
+        // supply their own builtin patches through getBuiltinPatch.
+        let patches = percent_decode_purl_component(patches);
+        user_patch |= !patches
+            .split("::")
+            .next()
+            .unwrap_or(&patches)
+            .split('&')
+            .all(|p| {
+                let p = p.rsplit('!').next().unwrap_or(p).trim_start_matches('~');
+                p.starts_with("builtin<compat/") && p.ends_with('>')
+            });
+        source = percent_decode_purl_component(original).into_owned();
+    }
+    Err("patch locator nesting exceeds inspection limit")
+}
+
+/// The sources every user `patch:` reference in `text` (a berry lock or a
+/// `package.json`) wraps, innermost layer first decoded
+/// ([`patch_layers`]): `patch:left-pad@http%3A//h/x.tgz#~/.yarn/patches/
+/// x.patch` wraps `left-pad@http://h/x.tgz`. Yarn's builtin compatibility
+/// wrappers are not the user's and are left out.
+pub(crate) fn user_patch_sources(text: &str) -> Vec<String> {
+    text.match_indices("patch:")
+        // The protocol opens a value (`"patch:`) or a range (`name@patch:`),
+        // never the middle of a word: `x.patch::version=…` is no reference.
+        .filter(|&(at, _)| {
+            text[..at]
+                .chars()
+                .next_back()
+                .is_none_or(|c| matches!(c, '@' | '"' | '\'') || c.is_whitespace())
+        })
+        .filter_map(|(at, _)| {
+            let rest = &text[at..];
+            let end = rest
+                .find(|c: char| matches!(c, '"' | '\'' | ',') || c.is_whitespace())
+                .unwrap_or(rest.len());
+            match patch_layers(&rest[..end]) {
+                Ok(Some(layers)) if layers.user_patch => Some(layers.source),
+                _ => None,
+            }
+        })
+        .collect()
+}
+
 /// A classic `resolved` value split at its first `#`: the url before it,
 /// and the fragment as a lowercase sha1 when it is 40 hex digits (either
 /// case) — the legacy tarball verifier yarn v1 enforces when no
@@ -195,6 +273,34 @@ pub(crate) fn berry_npm_alias_target(range: &str) -> Option<&str> {
 mod tests {
     use super::*;
 
+    /// #962: the source a user `patch:` wraps, as `yarn patch-commit`
+    /// spells it in a lock key, a `resolution:` and a `package.json` value;
+    /// builtin compatibility wrappers are not the user's.
+    #[test]
+    fn user_patch_sources_peel_what_yarn_patch_wraps() {
+        let lock = "\"left-pad@patch:left-pad@http%3A//h%3A1/x.tgz#~/.yarn/patches/a.patch\":\n  \
+                    resolution: \"left-pad@patch:left-pad@npm%3A1.3.0#~/.yarn/patches/a.patch\
+                    ::version=1.3.0&hash=0d6d28\"\n";
+        assert_eq!(
+            user_patch_sources(lock),
+            vec!["left-pad@http://h:1/x.tgz", "left-pad@npm:1.3.0"]
+        );
+        let pkg = r#"{ "dependencies": { "a": "patch:a@file%3A./v/a.tgz#./p.patch" } }"#;
+        assert_eq!(user_patch_sources(pkg), vec!["a@file:./v/a.tgz"]);
+        // A user patch over yarn's own compat wrapper: both layers peeled.
+        let nested = "\"typescript@patch:typescript@patch%3Atypescript@http%253A//h/t.tgz\
+                      %23optional!builtin%3Ccompat/typescript%3E#~/.yarn/patches/t.patch\":";
+        assert_eq!(
+            user_patch_sources(nested),
+            vec!["typescript@http://h/t.tgz"]
+        );
+        // The compat wrapper alone is not a user patch.
+        let builtin =
+            "\"resolve@patch:resolve@http%3A//h/r.tgz#optional!builtin<compat/resolve>\":";
+        assert!(user_patch_sources(builtin).is_empty());
+        assert!(user_patch_sources("\"left-pad@npm:1.3.0\":\n").is_empty());
+    }
+
     /// #1271: the classic grammar keeps an empty range (`left-pad@` from
     /// `"left-pad": ""`); the strict berry grammar still rejects it.
     #[test]

crates/socket-patch-core/src/patch/redirect/upstream/npm.rs

@@ -727,6 +727,14 @@ async fn restore_berry(
     let mut hits: Vec<Hit> = Vec::new();
     // What the lock still declares, read once for the URL-keyed hits.
     let mut declared: Option<BTreeSet<String>> = None;
+    // Hosted pins a user `yarn patch` wraps (#962): restoring the base
+    // entry would leave the `patch:` descriptor naming a URL no entry
+    // resolves, which every `yarn install --immutable` rejects (YN0028).
+    let wrapped: BTreeSet<String> = [Some(raw), pkg_text.as_deref()]
+        .into_iter()
+        .flatten()
+        .flat_map(|text| yarn_patch_wrapped_uuids(text, ctx))
+        .collect();
     for (i, block) in blocks.iter().enumerate() {
         let lines = stanza_lines(block);
         let Some(resolution) = berry_field(&lines, "resolution").map(str::to_string) else {
@@ -739,11 +747,7 @@ async fn restore_berry(
             continue;
         };
         let url_pin = reference.starts_with("https://") || reference.starts_with("http://");
-        let archive = if url_pin {
-            reference
-        } else if let Some((_, binding)) = reference.split_once("::__archiveUrl=") {
-            binding.split('&').next().unwrap_or(binding)
-        } else {
+        let Some(archive) = berry_hosted_archive(reference) else {
             continue;
         };
         let Some(uuid) = ctx.hosted_uuid(archive) else {
@@ -766,6 +770,20 @@ async fn restore_berry(
             );
             continue;
         };
+        if wrapped.contains(&uuid) {
+            result.refuse(
+                &uuid,
+                format!(
+                    "{name}@{version}: a `patch:` entry from `yarn patch` still wraps the \
+                     hosted tarball in {rel}, and restoring the registry entry under it would \
+                     leave a lock `yarn install --immutable` rejects — put the registry range \
+                     back in place of the `patch:` descriptor in the package.json that declares \
+                     it, run `yarn install`, roll back, then re-run `yarn patch` on the registry \
+                     version"
+                ),
+            );
+            continue;
+        }
         // An entry keyed by the tarball descriptor itself takes back the
         // descriptors its `resolutions` selectors route to that URL.
         let keyed_by_url = matches!(
@@ -1041,6 +1059,32 @@ async fn restore_berry(
     view.write(rel, doc.render(&moved));
 }
 
+/// The tarball URL a berry hosted pin's locator reference names: the
+/// tarball-URL locator itself, or the percent-encoded `::__archiveUrl=`
+/// binding of an `npm:` locator pinned by releases up to 5.0 (#404).
+fn berry_hosted_archive(reference: &str) -> Option<&str> {
+    if reference.starts_with("https://") || reference.starts_with("http://") {
+        Some(reference)
+    } else {
+        let (_, binding) = reference.split_once("::__archiveUrl=")?;
+        Some(binding.split('&').next().unwrap_or(binding))
+    }
+}
+
+/// The hosted patch uuids a user `patch:` reference in `text` (a berry
+/// lock or a root `package.json`) wraps (#962).
+fn yarn_patch_wrapped_uuids(text: &str, ctx: &Ctx<'_>) -> Vec<String> {
+    use crate::formats::yarn::patterns::{split_pattern, user_patch_sources};
+
+    user_patch_sources(text)
+        .iter()
+        .filter_map(|source| {
+            let (_, reference) = split_pattern(source)?;
+            ctx.hosted_uuid(berry_hosted_archive(reference)?)
+        })
+        .collect()
+}
+
 /// Retire the leftover hosted `resolutions` selectors of a yarn berry root
 /// `package.json` (#1203): `yarn remove` / `yarn up` deleted the lock entry
 /// a hosted pin keyed by its tarball URL but left its selector, which now

crates/socket-patch-core/src/patch/user_patches.rs

@@ -14,7 +14,7 @@ use serde_json::Value;
 use crate::formats::text::strip_bom;
 use crate::utils::fs::read_regular_to_string;
 use crate::utils::npm_range::npm_range_admits;
-use crate::utils::purl::{normalize_purl, percent_decode_purl_component, strip_purl_qualifiers};
+use crate::utils::purl::{normalize_purl, strip_purl_qualifiers};
 use crate::utils::workspace_globs::{workspaces_include_as, WorkspacesDialect};
 
 #[derive(Default, Debug)]
@@ -474,45 +474,14 @@ impl UserPatchIndex {
         if !spec.starts_with("patch:") {
             return;
         }
-        let mut source = spec.to_owned();
-        let mut user_patch = false;
-        // Inspect every layer: a compatibility patch can wrap a user patch,
-        // or vice versa. Both still belong to the original package.
-        for _ in 0..16 {
-            if let Some(inner) = source
-                .strip_prefix("patch:")
-                .or_else(|| source.split_once("@patch:").map(|(_, rest)| rest))
-            {
-                let Some((original, patches)) = inner.split_once('#') else {
-                    self.unknown("npm", path, "unrecognized patch: reference");
-                    return;
-                };
-                // Only Yarn's compatibility namespace is exempt. Plugins can
-                // supply their own builtin patches through getBuiltinPatch.
-                let patches = percent_decode_purl_component(patches);
-                user_patch |= !patches
-                    .split("::")
-                    .next()
-                    .unwrap_or(&patches)
-                    .split('&')
-                    .all(|p| {
-                        let p = p.rsplit('!').next().unwrap_or(p).trim_start_matches('~');
-                        p.starts_with("builtin<compat/") && p.ends_with('>')
-                    });
-                source = percent_decode_purl_component(original).into_owned();
-                continue;
-            }
-            if user_patch {
-                let (name, version) = locator_target(&source);
+        match crate::formats::yarn::patterns::patch_layers(spec) {
+            Ok(Some(layers)) if layers.user_patch => {
+                let (name, version) = locator_target(&layers.source);
                 self.add("npm", name, version, path);
             }
-            return;
+            Ok(_) => {}
+            Err(reason) => self.unknown("npm", path, reason),
         }
-        self.unknown(
-            "npm",
-            path,
-            "patch locator nesting exceeds inspection limit",
-        );
     }
 
     async fn patch_package(&mut self, dir: &Path, value: &Value) {