diff --git a/crates/socket-patch-cli/CLI_CONTRACT.md b/crates/socket-patch-cli/CLI_CONTRACT.md index f210b750e..aefb4b8f2 100644 --- a/crates/socket-patch-cli/CLI_CONTRACT.md +++ b/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: diff --git a/crates/socket-patch-cli/tests/in_process_redirect.rs b/crates/socket-patch-cli/tests/in_process_redirect.rs index 36244519f..152fefb22 100644 --- a/crates/socket-patch-cli/tests/in_process_redirect.rs +++ b/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:::__archiveUrl=` resolution, which makes yarn's npm fetcher /// send registry auth to the patch host. `rollback` must still recognize and diff --git a/crates/socket-patch-core/src/formats/yarn/patterns.rs b/crates/socket-patch-core/src/formats/yarn/patterns.rs index 85a9ef3b4..71bf24327 100644 --- a/crates/socket-patch-core/src/formats/yarn/patterns.rs +++ b/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, &'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') + }); + 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 { + 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\":"; + 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] diff --git a/crates/socket-patch-core/src/patch/redirect/upstream/npm.rs b/crates/socket-patch-core/src/patch/redirect/upstream/npm.rs index 2bd805f4f..3f4b47d3f 100644 --- a/crates/socket-patch-core/src/patch/redirect/upstream/npm.rs +++ b/crates/socket-patch-core/src/patch/redirect/upstream/npm.rs @@ -727,6 +727,14 @@ async fn restore_berry( let mut hits: Vec = Vec::new(); // What the lock still declares, read once for the URL-keyed hits. let mut declared: Option> = 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 = [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 { + 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 diff --git a/crates/socket-patch-core/src/patch/user_patches.rs b/crates/socket-patch-core/src/patch/user_patches.rs index 955ec67e6..0ee7c9d2d 100644 --- a/crates/socket-patch-core/src/patch/user_patches.rs +++ b/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') - }); - 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) { diff --git a/crates/socket-patch-core/src/vendor/yarn_berry_lock.rs b/crates/socket-patch-core/src/vendor/yarn_berry_lock.rs index 2d8633693..f2ae19f3b 100644 --- a/crates/socket-patch-core/src/vendor/yarn_berry_lock.rs +++ b/crates/socket-patch-core/src/vendor/yarn_berry_lock.rs @@ -841,6 +841,29 @@ pub async fn revert_yarn_berry_opts( } } } + // A user `yarn patch` over the vendored tarball (#962): restoring the + // recorded entry under its `patch:` wrapper leaves a lock every + // `yarn install --immutable` rejects, so refuse before either file is + // written (and before the dry-run return, so a preview agrees). + if let Some(wired) = user_patch_wrapping(project_root, &entry.uuid).await { + let detail = format!( + "refusing to revert {uuid_dir_rel}: a `patch:` entry from `yarn patch` in {wired} \ + still wraps the vendored tarball, 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`, re-run `vendor --revert`, then re-run `yarn patch` on the \ + registry version" + ); + return RevertOutcome { + success: false, + warnings: vec![VendorWarning::new( + "vendor_lock_still_wired_revert_blocked", + detail.clone(), + )], + error: Some(detail), + kept_artifact: false, + }; + } if dry_run { return RevertOutcome::ok(); } @@ -1102,6 +1125,26 @@ pub async fn revert_yarn_berry_opts( // ───────────────────────────── revert internals ───────────────────────────── +/// The first wired file (yarn.lock, then package.json) in which a user +/// `patch:` reference wraps the vendored artifact of `uuid` (#962). An +/// unreadable or missing file wraps nothing here: the wired revert's own +/// reads report it. +async fn user_patch_wrapping(project_root: &Path, uuid: &str) -> Option<&'static str> { + let needle = uuid.to_ascii_lowercase(); + for wired in [YARN_LOCK, PACKAGE_JSON] { + let Ok(text) = read_regular_to_string(&project_root.join(wired)).await else { + continue; + }; + if crate::formats::yarn::patterns::user_patch_sources(&text) + .iter() + .any(|source| source.to_ascii_lowercase().contains(&needle)) + { + return Some(wired); + } + } + None +} + /// Put back an entry vendoring pruned (see [`KIND_LOCK_ENTRY_PRUNED`]): /// the rewritten shared entry returns to its original lines, a removed one /// is inserted where yarn sorts it. Already restored is silent; anything @@ -3944,6 +3987,71 @@ __metadata: ); } + /// #962: the user ran `yarn patch` / `yarn patch-commit -s` over the + /// vendored package, so a `patch:` lock entry and dependency descriptor + /// wrap the vendored tarball. Restoring the recorded entry would leave + /// that wrapper naming a descriptor no entry resolves: every + /// `yarn install --immutable` rejects the lock (YN0028). The revert must + /// refuse BEFORE writing either file, on a dry run too. + #[tokio::test] + async fn revert_refuses_before_writing_when_a_user_yarn_patch_wraps_the_artifact() { + let fx = fixture().await; + let (_, entry, _) = expect_done(fx.vendor(false).await); + let entry = entry.unwrap(); + + let text = tokio::fs::read_to_string(fx.lock_path()).await.unwrap(); + let resolution = text + .lines() + .find_map(|l| l.strip_prefix(" resolution: \"left-pad@")) + .and_then(|r| r.strip_suffix('"')) + .expect("the vendored lock must carry our resolution line") + .to_string(); + let wrapped = format!( + "patch:left-pad@{}#~/.yarn/patches/left-pad-file-0d6d28.patch", + resolution.replace(':', "%3A") + ); + let lock = format!( + "{text}\n\"left-pad@{wrapped}\":\n version: 1.3.0\n \ + resolution: \"left-pad@{wrapped}::version=1.3.0&hash=0d6d28\"\n \ + languageName: node\n linkType: hard\n" + ); + tokio::fs::write(fx.lock_path(), &lock).await.unwrap(); + let pkg_text = tokio::fs::read_to_string(fx.pkg_path()).await.unwrap(); + let mut pkg: Value = serde_json::from_str(&pkg_text).unwrap(); + // yarn 4.18 `patch-commit -s` rewrites the dependency itself. + pkg["dependencies"]["left-pad"] = Value::String(wrapped); + let pkg_text = serde_json::to_string_pretty(&pkg).unwrap() + "\n"; + tokio::fs::write(fx.pkg_path(), &pkg_text).await.unwrap(); + + for dry_run in [true, false] { + let outcome = revert_yarn_berry(&entry, fx.root(), dry_run).await; + assert!( + !outcome.success, + "dry_run={dry_run}: must refuse under a user `yarn patch`: {:?}", + outcome.warnings + ); + assert!( + outcome.warnings.iter().any(|w| { + w.code == "vendor_lock_still_wired_revert_blocked" + && w.detail.contains("yarn patch") + }), + "dry_run={dry_run}: {:?}", + outcome.warnings + ); + assert_eq!( + tokio::fs::read_to_string(fx.lock_path()).await.unwrap(), + lock, + "dry_run={dry_run}: yarn.lock untouched" + ); + assert_eq!( + tokio::fs::read_to_string(fx.pkg_path()).await.unwrap(), + pkg_text, + "dry_run={dry_run}: package.json untouched" + ); + assert!(fx.tgz_path().exists(), "the artifact must survive"); + } + } + /// The user re-keys our resolutions entry (`"left-pad"` → /// `"left-pad@npm:1.3.0"`) without changing its `file:` value. The /// recorded key then reads as already-converged (Added record, key