diff --git a/crates/socket-patch-cli/tests/hosted_memory_engine.rs b/crates/socket-patch-cli/tests/hosted_memory_engine.rs index ab354a335..b41d35188 100644 --- a/crates/socket-patch-cli/tests/hosted_memory_engine.rs +++ b/crates/socket-patch-cli/tests/hosted_memory_engine.rs @@ -2085,3 +2085,70 @@ async fn issue_1598_berry_invalid_manifest_name_or_version_is_named_on_disk() { ); } } + +/// #1630: a berry `exec:` script, or a git remote that isn't http(s), +/// under another dependency name installs `node_modules/lp2` from a +/// package the lock doesn't name, at its lock version. It may be the +/// pinned left-pad@1.3.0, so the hosted scan names it as possibly +/// unpatched (`redirect_copy_unpatched`); it used to report a clean +/// success while yarn installed unpatched left-pad as `node_modules/lp2`. +/// Control: the same copy at another version is no copy of 1.3.0. +#[tokio::test] +async fn issue_1630_berry_exec_and_git_copies_are_named() { + const UUID: &str = "16301630-1630-4630-8630-163016301630"; + let ws = "app%40workspace%3A."; + for (spec, key, resolution) in [ + ( + "exec:./gen.js", + format!("lp2@exec:./gen.js::locator={ws}"), + format!("lp2@exec:./gen.js#./gen.js::hash=db115a&locator={ws}"), + ), + ( + "git+ssh://git@github.com/stevemao/left-pad.git#v1.3.0", + "lp2@git+ssh://git@github.com/stevemao/left-pad.git#v1.3.0".to_string(), + "lp2@git+ssh://git@github.com/stevemao/left-pad.git#commit=5d3c4f2".to_string(), + ), + ] { + for (version, named) in [("1.3.0", true), ("2.0.0", false)] { + let server = MockServer::start().await; + mount_left_pad_grant(&server, UUID).await; + let mut files = berry_linked_copy_files(spec); + let lock = String::from_utf8(files["yarn.lock"].clone()).unwrap(); + let linked = format!( + "\"lp2@{spec}::locator={ws}\":\n version: 0.0.0-use.local\n \ + resolution: \"lp2@{spec}::locator={ws}\"\n \ + languageName: node\n linkType: soft\n" + ); + assert!(lock.contains(&linked), "{lock}"); + let lock = lock.replace( + &linked, + &format!( + "\"{key}\":\n version: {version}\n resolution: \"{resolution}\"\n \ + languageName: node\n linkType: hard\n" + ), + ); + files.insert("yarn.lock".to_string(), lock.into_bytes()); + 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 + ); + let details = copy_unpatched_details(project); + if !named { + assert!(details.is_empty(), "{spec}@{version}: {details:?}"); + continue; + } + assert_eq!(details.len(), 1, "{spec}@{version}: {details:?}"); + assert!( + details[0].contains(&key) && details[0].contains("UNPATCHED"), + "{spec}: {}", + details[0] + ); + } + } +} diff --git a/crates/socket-patch-core/src/formats/yarn/copy.rs b/crates/socket-patch-core/src/formats/yarn/copy.rs index 33e5229fa..fa502cd97 100644 --- a/crates/socket-patch-core/src/formats/yarn/copy.rs +++ b/crates/socket-patch-core/src/formats/yarn/copy.rs @@ -2,8 +2,8 @@ //! dependency name the entry is keyed by (#939, #1236). //! //! Yarn (classic and berry alike) keys a `file:` tarball or directory, a -//! url tarball and a berry `portal:` / `link:` dependency by the name the -//! depender gave it: `"lp2": "file:./left-pad-1.3.0.tgz"` locks as +//! url tarball and a berry `portal:` / `link:` / `exec:` / git dependency +//! by the name the depender gave it: `"lp2": "file:./left-pad-1.3.0.tgz"` locks as //! `lp2@file:…`, while `node_modules/lp2` IS left-pad@1.3.0. No pin of //! `left-pad` reaches that copy, so every reader that asks "does this lock //! also install an unpatched copy of the patched `name@version`?" (lockfile @@ -30,6 +30,15 @@ pub(crate) enum CopyRef { /// A url tarball (`#` fragment dropped): the package is the registry /// package its `//-/-.tgz` path names. Url(String), + /// A root-relative berry `exec:` script that builds the package at + /// install time: the lock names only its version, so the package is + /// never known (#1630). + Exec(String), + /// A berry git remote that isn't http(s) (`git+ssh:`, `ssh:`, `git:`; + /// `#` fragment dropped): the lock names only its version, so the + /// package is never known (#1630). An http(s) git remote is a + /// [`CopyRef::Url`], whose path names no registry package either. + Git(String), } /// Which berry protocol a [`CopyRef::Linked`] copy came from. yarn installs @@ -44,7 +53,8 @@ pub(crate) enum LinkKind { 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. + /// names its package in its path, and for an `exec:` script or a git + /// remote, which hold no package until yarn builds or clones them. /// /// `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 @@ -63,14 +73,14 @@ impl CopyRef { } else { format!("{dir}/package.json") }), - CopyRef::Url(_) => None, + CopyRef::Url(_) | CopyRef::Exec(_) | CopyRef::Git(_) => None, } } /// The `(name, version)` this copy installs, given the lock entry's /// `version` and the bytes read at [`CopyRef::read_path`]. `None` when /// the copy does not say (an unreadable or nameless manifest, a - /// non-registry url). + /// non-registry url, an `exec:` script, a git remote). pub(crate) fn identity( &self, lock_version: &str, @@ -84,6 +94,7 @@ impl CopyRef { return Some((package_name(&manifest)?, package_version(&manifest)?)); } CopyRef::Url(url) => registry_tarball_name(url, lock_version)?, + CopyRef::Exec(_) | CopyRef::Git(_) => return None, }; Some((name, lock_version.to_string())) } @@ -116,6 +127,11 @@ impl CopyRef { "installs it from {:?}", crate::utils::redact::redact_url(url) ), + CopyRef::Exec(rel) => format!("builds it with the user's exec: script {rel}"), + CopyRef::Git(url) => format!( + "installs it from git {:?}", + crate::utils::redact::redact_url(url) + ), } } } @@ -150,22 +166,34 @@ 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). A -/// path that leaves the root is still read there ([`copy_path`]). +/// directory, a `portal:` / `link:` directory, an `exec:` script (each +/// relative to the workspace its `locator=` binding names), an http(s) +/// url, or another git remote. `None` for any other protocol (`npm:`, +/// `workspace:`, `patch:`). A path that leaves the root is still read +/// there ([`copy_path`]). pub(crate) fn berry_copy_ref(reference: &str) -> Option { + let url = || reference.split('#').next().unwrap_or_default().to_string(); if reference.starts_with("http") { - return Some(CopyRef::Url( - reference.split('#').next().unwrap_or_default().to_string(), - )); + return Some(CopyRef::Url(url())); } - let (path, linked) = if let Some(path) = reference.strip_prefix("file:") { + // yarn's git resolver takes these besides http(s) (#1630); `git@host:` + // is the scp-like ssh spelling. + if ["git:", "git+", "ssh:", "git@"] + .iter() + .any(|p| reference.starts_with(p)) + { + return Some(CopyRef::Git(url())); + } + // `None` for `file:`, which is a tarball or a directory by its path. + type Kind = Option CopyRef>; + let (path, kind): (_, Kind) = if let Some(path) = reference.strip_prefix("file:") { (path, None) } else if let Some(path) = reference.strip_prefix("portal:") { - (path, Some(LinkKind::Portal)) + (path, Some(|rel| CopyRef::Linked(rel, LinkKind::Portal))) } else if let Some(path) = reference.strip_prefix("link:") { - (path, Some(LinkKind::Link)) + (path, Some(|rel| CopyRef::Linked(rel, LinkKind::Link))) + } else if let Some(path) = reference.strip_prefix("exec:") { + (path, Some(CopyRef::Exec)) } else { return None; }; @@ -174,8 +202,8 @@ pub(crate) fn berry_copy_ref(reference: &str) -> Option { 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 let Some(kind) = linked { - CopyRef::Linked(rel, kind) + Some(if let Some(kind) = kind { + kind(rel) } else if is_tarball_path(path) { CopyRef::Tarball(rel) } else { @@ -183,8 +211,8 @@ pub(crate) fn berry_copy_ref(reference: &str) -> Option { }) } -/// The root-relative path a berry `file:` / `portal:` / `link:` reference -/// names: relative to the workspace in its `locator=` binding +/// The root-relative path a berry `file:` / `portal:` / `link:` / `exec:` +/// reference names: relative to the workspace in its `locator=` binding /// (`b@workspace:packages/b`), or to the root when it has none /// ([`copy_path`]). fn berry_copy_path(reference: &str, path: &str) -> Option { @@ -367,6 +395,43 @@ mod tests { assert_eq!(berry_copy_ref("workspace:packages/b"), None); } + /// #1630: an `exec:` generator, or a git remote that isn't http(s), + /// installs a copy under the dependency name whose package the lock + /// doesn't say. It is a copy all the same, never read, so it may be + /// any package at its lock version. + #[test] + fn issue_1630_exec_and_git_refs_are_unread_copies() { + let ws = "::locator=b%40workspace%3Apackages%2Fb"; + let exec = berry_copy_ref(&format!("exec:./gen.js#./gen.js::hash=db115a&{}", &ws[2..])); + assert_eq!(exec, Some(CopyRef::Exec("packages/b/gen.js".into()))); + for git in [ + "git+ssh://git@github.com/stevemao/left-pad.git#commit=5d3c4f2", + "ssh://git@github.com/stevemao/left-pad.git#commit=5d3c4f2", + "git://github.com/stevemao/left-pad.git#commit=5d3c4f2", + "git@github.com:stevemao/left-pad.git#commit=5d3c4f2", + ] { + let source = berry_copy_ref(git); + assert_eq!( + source, + Some(CopyRef::Git(git.split('#').next().unwrap().into())), + "{git}" + ); + } + for source in [exec.unwrap(), CopyRef::Git("ssh://git@h/x.git".into())] { + assert_eq!(source.read_path(), None, "{source:?}"); + assert_eq!(source.identity("1.3.0", None), None, "{source:?}"); + assert_eq!(source.linked_name(None), None, "{source:?}"); + } + assert_eq!( + CopyRef::Git("git+ssh://tok:secret@h/x.git".into()).how(), + "installs it from git \"git+ssh://@h/x.git\"" + ); + assert_eq!( + CopyRef::Exec("gen.js".into()).how(), + "builds it with the user's exec: script gen.js" + ); + } + /// #1540: an absolute `portal:` / `link:` path keeps its whole spelling, /// a Windows drive's `:` included, and is never read. #[test] 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 e26e33064..271afdee2 100644 --- a/crates/socket-patch-core/src/vendor/yarn_berry_lock.rs +++ b/crates/socket-patch-core/src/vendor/yarn_berry_lock.rs @@ -237,10 +237,11 @@ impl NpmLockBackend for YarnBerryBackend { ), )); } - // A `file:` / url / `portal:` / `link:` copy of the package under - // ANOTHER dependency name (`lp2@file:…`, #939): the name-keyed - // resolutions entry never moves it, so it keeps installing the - // unpatched bytes. Named, like the alias entries above. + // A `file:` / url / `portal:` / `link:` / `exec:` / git copy of the + // package under ANOTHER dependency name (`lp2@file:…`, #939, + // #1630): the name-keyed resolutions entry never moves it, so it + // keeps installing the unpatched bytes. Named, like the alias + // entries above. let other_name = super::yarn_copies::other_name_copies(project_root, &lock_text, name, version).await; // The package IS installed, only under other names, and `yarn @@ -2431,7 +2432,8 @@ __metadata: /// (`lp2@…`), so a copy of left-pad@1.3.0 under another name escapes the /// name-keyed resolutions entry and installs unpatched. Vendoring still /// wires the registry entry, names that copy (`vendor_copy_unpatched`), - /// and leaves it byte-untouched. Control: a `file:` / `portal:` copy + /// and leaves it byte-untouched. So does an `exec:` script or a + /// non-http git remote (#1630). Control: a `file:` / `portal:` copy /// holding another package is not named. #[tokio::test] async fn issue_939_other_name_copies_are_named() { @@ -2470,10 +2472,27 @@ __metadata: "0.0.0-use.local", format!("lp2@link:./forks/left-pad::locator={ws}"), ), + // #1630: the lock doesn't say which package these build or + // clone, so they are named whatever is on disk. + ( + "exec: script", + format!("lp2@exec:./gen.js::locator={ws}"), + "1.3.0", + format!("lp2@exec:./gen.js#./gen.js::hash=db115a&locator={ws}"), + ), + ( + "git+ssh: remote", + "lp2@git+ssh://git@github.com/stevemao/left-pad.git#v1.3.0".to_string(), + "1.3.0", + "lp2@git+ssh://git@github.com/stevemao/left-pad.git#commit=5d3c4f2".to_string(), + ), ]; for (case, key, version, resolution) in &cases { for fork in ["left-pad", "other-pkg"] { - let named = fork == "left-pad" || case.contains("url"); + let named = fork == "left-pad" + || case.contains("url") + || case.contains("exec") + || case.contains("git"); let block = format!( "\n\"{key}\":\n version: {version}\n resolution: \"{resolution}\"\n \ languageName: node\n linkType: hard\n" diff --git a/crates/socket-patch-core/src/vendor/yarn_copies.rs b/crates/socket-patch-core/src/vendor/yarn_copies.rs index acb323bbf..026cede00 100644 --- a/crates/socket-patch-core/src/vendor/yarn_copies.rs +++ b/crates/socket-patch-core/src/vendor/yarn_copies.rs @@ -3,7 +3,8 @@ //! #1236), shared by the classic and berry backends. //! //! Yarn keys a `file:` tarball or directory, a url tarball and a berry -//! `portal:` / `link:` dependency by the name the depender gave it +//! `portal:` / `link:` / `exec:` / git dependency by the name the depender +//! gave it //! (`lp2@file:./left-pad-1.3.0.tgz`), so a re-lock or `resolutions` entry //! for `left-pad` never moves it, and `node_modules/lp2` keeps the //! unpatched left-pad bytes. Which package such a copy is comes from the @@ -31,8 +32,9 @@ pub(crate) struct OtherNameCopy { pub(crate) source: CopyRef, /// The copy itself says it is `name@version`. `false` for a copy of /// that version whose package cannot be read (a missing directory, a - /// tarball with no readable `package.json`, a non-registry url, or an - /// unread `portal:` / `link:` directory of any version): it may be + /// tarball with no readable `package.json`, a non-registry url, an + /// `exec:` script, a git remote, or an unread `portal:` / `link:` + /// directory of any version): it may be /// `name`, so it is named, but never taken as proof that `name` is /// installed (#1516, #1540). pub(crate) identified: bool, @@ -189,5 +191,7 @@ pub(crate) fn copy_origin(source: &CopyRef) -> &'static str { CopyRef::Directory(_) => "a file: directory, which yarn copies into node_modules", CopyRef::Linked(..) => "a portal:/link: directory, which yarn links into node_modules", CopyRef::Url(_) => "a URL tarball", + CopyRef::Exec(_) => "an exec: script, which yarn runs to build it", + CopyRef::Git(_) => "a git repository", } } diff --git a/crates/socket-patch-core/src/vex/discover/mod.rs b/crates/socket-patch-core/src/vex/discover/mod.rs index 0c02a4ee5..9e0ec74b1 100644 --- a/crates/socket-patch-core/src/vex/discover/mod.rs +++ b/crates/socket-patch-core/src/vex/discover/mod.rs @@ -978,7 +978,8 @@ impl Discovery { /// A lock entry of `file` that installs a copy of some package at /// `version` whose package can't be read (a non-registry url, a missing - /// directory, a tarball with no readable `package.json`). It may be any + /// directory, a tarball with no readable `package.json`, a berry + /// `exec:` script or git remote). It may be any /// package at that version, so [`Discovery::contest_within_locks`] /// takes it as an unpatched copy of every package `file` wires at /// `version` (at any version when `version` is `None`, #1540; of diff --git a/crates/socket-patch-core/src/vex/discover/yarn.rs b/crates/socket-patch-core/src/vex/discover/yarn.rs index 95d24de68..c1292f35d 100644 --- a/crates/socket-patch-core/src/vex/discover/yarn.rs +++ b/crates/socket-patch-core/src/vex/discover/yarn.rs @@ -42,7 +42,10 @@ //! names the package the entry installs (for an `npm:` alias key the //! resolution carries the REAL ident); the version is the block's unquoted //! `version:` line. `workspace:`, `patch:`, `portal:`, `link:`, `exec:`, -//! `git` and plain registry locators are not ours and are skipped silently. +//! `git` and plain registry locators are not ours. A plain registry locator +//! and a copy under another dependency name (`file:`, url, `portal:`, +//! `link:`, `exec:`, git; #939, #1630) contest a wiring of the package they +//! install, or may install; the rest are skipped silently. //! //! * **Hosted** (`patch::redirect::rewrite_yarn_berry`): the direct //! tarball-URL locator `resolution: "name@https://patch.socket.dev/…"`, @@ -538,17 +541,17 @@ fn berry_block( // A `portal:` / `link:` directory under another dependency name // links `node_modules/` to the package its `package.json` // names, which no `resolutions` pin of that package reaches (#939). - if reference.starts_with("portal:") || reference.starts_with("link:") { - if let (Some(source), Some(version)) = ( - berry_copy_ref(reference), - berry_field(&block.lines, "version"), - ) { - copies.push(LockCopy { - key: block.key.clone(), - source, - version: version.to_string(), - }); - } + // An `exec:` script or a non-http git remote installs a package + // the lock doesn't name at its `version:` (#1630). + if let (Some(source), Some(version)) = ( + berry_copy_ref(reference), + berry_field(&block.lines, "version"), + ) { + copies.push(LockCopy { + key: block.key.clone(), + source, + version: version.to_string(), + }); } return; // workspace: / patch: / portal: / link: / exec: / git }; @@ -860,8 +863,9 @@ fn emit( } /// A yarn lock entry (berry or classic) that installs a user's `file:` -/// tarball / directory, url tarball or berry `portal:` / `link:` directory -/// (not a Socket wiring), awaiting [`record_copies`]. +/// tarball / directory, url tarball, berry `portal:` / `link:` directory, +/// berry `exec:` script or berry git remote (not a Socket wiring), +/// awaiting [`record_copies`]. struct LockCopy { key: String, source: CopyRef, @@ -875,7 +879,8 @@ struct LockCopy { /// of `left-pad` reaches it. The real package comes from the copy itself /// ([`CopyRef::identity`]). A copy whose package cannot be read (a /// missing directory, a tarball with no readable `package.json`, a -/// non-registry url) may be any package at its lock version, so it is +/// non-registry url, an `exec:` script, a git remote; #1630) may be any +/// package at its lock version, so it is /// recorded as a copy of every package this lock wires at that version: /// vouching for those would rest on a copy nobody read (#1516). A /// `portal:` / `link:` copy has no lock version: one whose package is @@ -1630,6 +1635,56 @@ mod tests { ); } + /// #1630: an `exec:` generator or a non-http git remote under another + /// dependency name installs `node_modules/lp2` from a package the lock + /// doesn't name, at its lock version. It may be the wired + /// left-pad@1.3.0, so it contests the ref like an unread url copy. + /// Control: the same copy at another version is no copy of 1.3.0. + #[tokio::test] + async fn issue_1630_exec_and_git_copies_contest_a_vendored_ref() { + let wired = berry_vendored_block("left-pad", "1.3.0", UUID_A); + let ws = "app%40workspace%3A."; + let copies = [ + ( + "exec:", + format!("lp2@exec:./gen.js::locator={ws}"), + format!("lp2@exec:./gen.js#./gen.js::hash=db115a&locator={ws}"), + ), + ( + "git+ssh:", + "lp2@git+ssh://git@github.com/stevemao/left-pad.git#v1.3.0".to_string(), + "lp2@git+ssh://git@github.com/stevemao/left-pad.git#commit=5d3c4f2".to_string(), + ), + ]; + let project = |blocks: &[String]| { + let p = Project::new(); + p.write("yarn.lock", berry(blocks)); + p.write( + "package.json", + package_json_with_resolutions(serde_json::json!({ + "left-pad": format!("file:./.socket/vendor/npm/{UUID_A}/left-pad-1.3.0.tgz"), + })), + ); + p + }; + for (case, key, resolution) in &copies { + let copy = |version: &str| berry_block(key, version, resolution, None); + let out = run(&project(&[wired.clone(), copy("1.3.0")])).await; + assert!(out.refs.is_empty(), "{case}: {:#?}", out.refs); + assert!( + out.diagnostics + .iter() + .any(|d| d.code == DIAG_REF_UNATTRIBUTABLE && d.detail.contains("lp2@")), + "{case}: {:#?}", + out.diagnostics + ); + assert_refs( + &run(&project(&[wired.clone(), copy("2.0.0")])).await, + &[("pkg:npm/left-pad@1.3.0", UUID_A, WiringMode::Vendored)], + ); + } + } + /// Socket-shaped blocks that fail validation are DIAGNOSED, never refs: /// no version, mixed-package key, unsafe name, a leaf naming another /// package, a traversal leaf, a non-canonical vendored uuid. diff --git a/docs/ecosystems.md b/docs/ecosystems.md index deda930d5..692d148d8 100644 --- a/docs/ecosystems.md +++ b/docs/ecosystems.md @@ -289,8 +289,8 @@ Each Legacy format has an upgrade path and an undo path. Both work in v5: different package (`"left-pad@npm:some-fork@^1.3.0"`, the fork-substitution idiom) — is never rewritten: it resolves a different package. - **yarn copies under another dependency name (classic & berry)** — yarn keys a `file:` - tarball or directory, a URL tarball and a berry `portal:` / `link:` dependency by the - name the depender gave it: `"lp2": "file:./left-pad-1.3.0.tgz"` locks as `lp2@file:…`, + tarball or directory, a URL tarball and a berry `portal:` / `link:` / `exec:` / git + dependency by the name the depender gave it: `"lp2": "file:./left-pad-1.3.0.tgz"` locks as `lp2@file:…`, while `node_modules/lp2` is left-pad@1.3.0. No pin or vendored wiring of `left-pad` reaches that copy, so it stays unpatched. Which package such a copy is comes from the copy itself (the tarball's or directory's `package.json`, or the registry package a @@ -299,7 +299,9 @@ Each Legacy format has an upgrade path and an undo path. Both work in v5: installs it from, including a `file:` path outside the project (`file:../shared/lp`), and a tarball's `package.json` is found under whatever top-level directory it has, as yarn strips the first path segment. A copy whose package can't be read (a non-registry - URL, a missing directory) is treated as possibly any package at its lock version: it + URL, a missing directory, a berry `exec:` script or a berry git remote such as + `git+ssh://…`, which hold no package until yarn builds or clones them) is treated as + possibly any package at its lock version: it is named as a copy it may be, and `vex` attests nothing that lock wires at that version. An absolute path (`portal:/home/me/lp`, `file:/abs/lp.tgz`) is never read, as the lock is untrusted input; a `portal:` / `link:` copy whose `package.json` isn't