Skip to content

Commit bd2db4b

Browse files
Fix yarn classic copy identity read two ways (#1951) (#1984)
* Start fix for #1951 Assisted-by: Claude Code:claude-opus-5-5 * Test hosted names yarn copies as vendored does Hosted scans name a yarn classic file: directory copy through their own reader, which drops a copy outside the project root (file:../x) that vendored and VEX read and name. Add regression tests for the rewriter and the hosted manifest read, plus the shared reader they will use. Refs #1951 Assisted-by: Claude Code:claude-opus-5-5 * Read yarn classic copies through one reader A hosted scan named a yarn classic file: directory or url copy through its own reader in formats/yarn/source.rs, which dropped a directory outside the project root (file:../ext/x). Vendored and VEX read that copy and named it, so hosted reported a clean pin while yarn kept installing the unpatched out-of-root copy, without the directory-skipped warning vendored gives. Hosted now reads the copy's manifest and identity through formats::yarn::copy (classic_copy_ref, CopyRef::read_path and identity), the same reader vendored and VEX use: out-of-root copies are read, absolute paths never are. The second reader and its third dir/package.json join are deleted. Fixes #1951 Assisted-by: Claude Code:claude-opus-5-5 * Expect one yarn reader's warning outside the root A yarn classic file: directory copy outside the project root is now named by the yarn rewriter, with the same redirect_yarn_classic_directory_skipped warning an in-root copy gets, instead of the engine's generic redirect_copy_unpatched fallback. The e2e now asserts that code, and that the copy is named exactly once. The rewriter test's doc says what it compares: which package the copy holds. Refs #1951 Assisted-by: Claude Code:claude-opus-5-5 --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent da41350 commit bd2db4b

5 files changed

Lines changed: 196 additions & 74 deletions

File tree

‎crates/socket-patch-cli/tests/e2e_redirect_yarn_classic_build.rs‎

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1542,9 +1542,10 @@ async fn classic_file_directory_copy_under_another_name_is_named_and_not_atteste
15421542
/// #1516: the #1236 copy, but from a directory OUTSIDE the project root
15431543
/// (`"lp2": "file:../ext/lpdir"`, shared code in a non-workspace
15441544
/// monorepo). yarn 1 installs it into `node_modules/lp2` all the same, so
1545-
/// it stays unpatched. `scan --mode hosted` must name the copy
1546-
/// (`redirect_copy_unpatched`), and neither its in-run VEX nor a lock-only
1547-
/// `vex` may attest left-pad not_affected.
1545+
/// it stays unpatched. `scan --mode hosted` must name the copy exactly as
1546+
/// it names an in-root one (`redirect_yarn_classic_directory_skipped`,
1547+
/// read through the reader vendored and VEX use, #1951), once, and neither
1548+
/// its in-run VEX nor a lock-only `vex` may attest left-pad not_affected.
15481549
#[tokio::test(flavor = "multi_thread")]
15491550
#[serial_test::serial]
15501551
async fn classic_outside_root_copy_under_another_name_is_named_and_not_attested() {
@@ -1635,11 +1636,22 @@ async fn classic_outside_root_copy_under_another_name_is_named_and_not_attested(
16351636
lock.contains(&format!("{UUID}/{DEP}-{DEP_VERSION}.tgz")),
16361637
"the registry left-pad block must still be pinned:\n{lock}"
16371638
);
1638-
assert!(
1639-
env.to_string().contains("redirect_copy_unpatched"),
1640-
"the outside-root copy must be named: {env}"
1639+
let naming: Vec<&str> = env["warnings"]
1640+
.as_array()
1641+
.into_iter()
1642+
.flatten()
1643+
.filter(|w| {
1644+
w["detail"]
1645+
.as_str()
1646+
.is_some_and(|d| d.contains("lp2@file:../ext/lpdir"))
1647+
})
1648+
.filter_map(|w| w["code"].as_str())
1649+
.collect();
1650+
assert_eq!(
1651+
naming,
1652+
["redirect_yarn_classic_directory_skipped"],
1653+
"the outside-root copy must be named once, as an in-root one is: {env}"
16411654
);
1642-
assert!(env.to_string().contains("lp2@file:../ext/lpdir"), "{env}");
16431655
let vex = std::fs::read_to_string(proj.join("out.vex.json")).unwrap_or_default();
16441656
assert!(
16451657
!vex.contains("not_affected"),

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

Lines changed: 31 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111
//! [`CopyRef`], and so cannot disagree on which package it is.
1212
1313
use super::patterns::split_classic_pattern;
14-
use super::source::{is_tarball_path, registry_tarball_name};
14+
use super::source::{classic_copy_source, is_tarball_path, registry_tarball_name, CopySource};
1515

1616
/// Where a yarn lock entry's copy comes from, read for the package it holds.
1717
#[derive(Debug, Clone, PartialEq, Eq)]
@@ -165,6 +165,36 @@ pub(crate) fn classic_copy_ref(patterns: &[String], resolved: Option<&str>) -> O
165165
}
166166
}
167167

168+
/// The package a classic `file:` directory or url copy really installs,
169+
/// whatever dependency name its key carries (#1236), with where yarn 1
170+
/// installs it from: read through [`classic_copy_ref`] and
171+
/// [`CopyRef::identity`], the reader vendored and VEX use, so the hosted
172+
/// rewriter makes the same decision for every copy (#1951). `read_text`
173+
/// gets [`CopyRef::read_path`] (root-relative, possibly climbing above the
174+
/// root; never absolute). `None` for any other block, a `file:` tarball
175+
/// (whose bytes a text reader can't open), a url naming no registry
176+
/// package, or an unreadable or nameless manifest.
177+
pub(crate) fn classic_copy_name(
178+
patterns: &[String],
179+
resolved: Option<&str>,
180+
version: &str,
181+
read_text: impl Fn(&str) -> Option<String>,
182+
) -> Option<(String, CopySource)> {
183+
let source = classic_copy_source(patterns, resolved);
184+
let copy = classic_copy_ref(patterns, resolved)?;
185+
match (source, &copy) {
186+
(CopySource::Directory, CopyRef::Directory(_))
187+
| (CopySource::RemoteTarball, CopyRef::Url(_)) => {}
188+
_ => return None,
189+
}
190+
let text = match copy.read_path() {
191+
Some(rel) => Some(read_text(&rel)?),
192+
None => None,
193+
};
194+
let (name, _) = copy.identity(version, text.as_deref().map(str::as_bytes))?;
195+
Some((name, source))
196+
}
197+
168198
/// The copy a berry locator `reference` installs from: a `file:` tarball or
169199
/// directory, a `portal:` / `link:` directory, an `exec:` script (each
170200
/// relative to the workspace its `locator=` binding names), an http(s)

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

Lines changed: 0 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
11
//! Where yarn 1 installs a classic lock block's copy from, decided once for
22
//! every mode that reads or rewrites the block.
33
4-
use super::copy::manifest_name;
54
use super::patterns::split_classic_pattern;
65
use crate::vendor::npm_origin::npm_spec_is_registry;
76

@@ -75,59 +74,6 @@ pub(crate) fn classic_copy_source(patterns: &[String], resolved: Option<&str>) -
7574
}
7675
}
7776

78-
/// The root-relative `file:` directory a classic block's key names, if
79-
/// any: yarn 1 COPIES it into node_modules under the DEPENDENCY name
80-
/// (`"lp2@file:./lpdir"`), so which package that copy is comes from the
81-
/// directory's own `package.json` (#1236). `None` for a `file:` tarball or
82-
/// a path that leaves the root.
83-
pub(crate) fn classic_file_directory(patterns: &[String]) -> Option<String> {
84-
patterns.iter().find_map(|p| {
85-
let path = split_classic_pattern(p)?.1.strip_prefix("file:")?;
86-
let path = path.split('#').next().unwrap_or_default();
87-
if is_tarball_path(path) {
88-
return None;
89-
}
90-
crate::utils::cargo_workspace::normalize_rel("", path)
91-
})
92-
}
93-
94-
/// The package a classic `file:` directory or url copy really installs,
95-
/// whatever dependency name its key carries (#1236): the directory's
96-
/// `package.json` `name` (read through `read_text`, given the
97-
/// root-relative manifest path), or the registry package a url tarball's
98-
/// path names. `None` when neither says (a `file:` tarball, a
99-
/// non-registry url, an unreadable manifest).
100-
pub(crate) fn classic_copy_real_name(
101-
patterns: &[String],
102-
resolved: Option<&str>,
103-
version: &str,
104-
read_text: impl Fn(&str) -> Option<String>,
105-
) -> Option<(String, CopySource)> {
106-
match classic_copy_source(patterns, resolved) {
107-
CopySource::Directory => {
108-
let dir = classic_file_directory(patterns)?;
109-
let manifest = if dir.is_empty() {
110-
"package.json".to_string()
111-
} else {
112-
format!("{dir}/package.json")
113-
};
114-
let name = manifest_name(read_text(&manifest)?.as_bytes())?;
115-
Some((name, CopySource::Directory))
116-
}
117-
CopySource::RemoteTarball => {
118-
let url = resolved?.split('#').next().unwrap_or_default();
119-
if !url.starts_with("http") {
120-
return None;
121-
}
122-
Some((
123-
registry_tarball_name(url, version)?,
124-
CopySource::RemoteTarball,
125-
))
126-
}
127-
_ => None,
128-
}
129-
}
130-
13177
/// The package an npm registry tarball url serves, from its
13278
/// `/<name>/-/<leaf>-<version>.tgz` path (`<name>` may be `@scope/leaf`,
13379
/// its `@` / `/` possibly percent-encoded); `None` for any other shape.

‎crates/socket-patch-core/src/hosted/engine.rs‎

Lines changed: 61 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -693,26 +693,25 @@ pub async fn read_candidate_files(
693693
out.read(view, unreadable, NPMRC_REL).await;
694694
// A `file:` directory copy is locked under the DEPENDENCY name, so
695695
// the classic rewriter reads which package it is from the
696-
// directory's `package.json` (#1236). Advisory: never rewritten,
697-
// and an unreadable one only leaves that copy unnamed.
698-
let dirs: BTreeSet<String> = out
696+
// directory's `package.json` (#1236), at the path vendored and VEX
697+
// read it from (#1951): outside the root too, never at an absolute
698+
// path. Advisory: never rewritten, and an unreadable one only
699+
// leaves that copy unnamed.
700+
let manifests: BTreeSet<String> = out
699701
.files
700702
.get("yarn.lock")
701703
.map(|lock| {
704+
use crate::formats::yarn::copy::{classic_copy_ref, CopyRef};
702705
crate::vendor::lock_inventory::yarn::classic_entries(lock)
703706
.iter()
704-
.filter_map(|e| {
705-
crate::formats::yarn::source::classic_file_directory(&e.patterns)
707+
.filter_map(|e| match classic_copy_ref(&e.patterns, None)? {
708+
copy @ CopyRef::Directory(_) => copy.read_path(),
709+
_ => None,
706710
})
707711
.collect()
708712
})
709713
.unwrap_or_default();
710-
for dir in dirs {
711-
let rel = if dir.is_empty() {
712-
"package.json".to_string()
713-
} else {
714-
format!("{dir}/package.json")
715-
};
714+
for rel in manifests {
716715
if out.files.contains_key(&rel) {
717716
continue;
718717
}
@@ -4329,6 +4328,57 @@ mod tests {
43294328
(read, done)
43304329
}
43314330

4331+
/// #1951: the hosted run reads a yarn classic `file:` directory copy's
4332+
/// `package.json` where vendored and VEX read it
4333+
/// ([`crate::formats::yarn::copy::CopyRef::read_path`]): from outside
4334+
/// the root too, since yarn 1 installs `file:../ext/lpdir` all the same,
4335+
/// but never at an absolute path a lock names (#1516).
4336+
#[tokio::test]
4337+
async fn issue_1951_yarn_classic_copy_manifests_read_as_vendored_reads_them() {
4338+
let tmp = tempfile::tempdir().unwrap();
4339+
let root = tmp.path().join("app");
4340+
let manifest = r#"{"name":"left-pad","version":"1.3.0"}"#;
4341+
for dir in ["app/lpdir", "ext/lpdir", "abs/lpdir"] {
4342+
let dir = tmp.path().join(dir);
4343+
std::fs::create_dir_all(&dir).unwrap();
4344+
std::fs::write(dir.join("package.json"), manifest).unwrap();
4345+
}
4346+
let abs = tmp.path().join("abs/lpdir");
4347+
let abs = abs.to_str().unwrap().replace('\\', "/");
4348+
std::fs::write(
4349+
root.join("yarn.lock"),
4350+
format!(
4351+
"# yarn lockfile v1\n\n\n\
4352+
left-pad@1.3.0:\n version \"1.3.0\"\n \
4353+
resolved \"https://registry.yarnpkg.com/left-pad/-/left-pad-1.3.0.tgz#bbbb\"\n\n\
4354+
\"lp2@file:./lpdir\":\n version \"1.3.0\"\n\n\
4355+
\"lp3@file:../ext/lpdir\":\n version \"1.3.0\"\n\n\
4356+
\"lp4@file:{abs}\":\n version \"1.3.0\"\n"
4357+
),
4358+
)
4359+
.unwrap();
4360+
let candidates = vec![left_pad_candidate()];
4361+
let read =
4362+
read_candidate_files(&ProjectView::Disk(&root), &BTreeSet::new(), &candidates).await;
4363+
assert_eq!(
4364+
read.files.get("lpdir/package.json").map(String::as_str),
4365+
Some(manifest)
4366+
);
4367+
assert_eq!(
4368+
read.files
4369+
.get("../ext/lpdir/package.json")
4370+
.map(String::as_str),
4371+
Some(manifest),
4372+
"{:?}",
4373+
read.files.keys()
4374+
);
4375+
assert!(
4376+
!read.files.keys().any(|k| k.contains("abs/lpdir")),
4377+
"{:?}",
4378+
read.files.keys()
4379+
);
4380+
}
4381+
43324382
#[tokio::test]
43334383
async fn issue_490_the_root_manifest_overrides_reach_the_npm_rewriter() {
43344384
let redirected = |done: &Rewritten| {

‎crates/socket-patch-core/src/patch/redirect/mod.rs‎

Lines changed: 85 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4591,7 +4591,7 @@ fn rewrite_yarn_classic_with(
45914591
.zip(&heads)
45924592
.map(|(b, (patterns, _))| {
45934593
let version = classic_field(&b.lines, "version")?;
4594-
let (name, source) = crate::formats::yarn::source::classic_copy_real_name(
4594+
let (name, source) = crate::formats::yarn::copy::classic_copy_name(
45954595
patterns,
45964596
classic_field(&b.lines, "resolved"),
45974597
version,
@@ -13772,6 +13772,90 @@ mod tests {
1377213772
}
1377313773
}
1377413774

13775+
/// #1951: the hosted rewriter reads which package a yarn classic
13776+
/// `file:` / url copy holds through the same reader vendored and VEX use
13777+
/// ([`crate::formats::yarn::copy::classic_copy_ref`] and
13778+
/// [`crate::formats::yarn::copy::CopyRef::identity`]), so the modes
13779+
/// identify the same copies. (A copy no reader identifies is a separate
13780+
/// rule: vendored keeps it as a possible copy, #1516.) yarn 1 installs
13781+
/// `file:../ext/lpdir` from outside the root as readily as from inside
13782+
/// it, so that copy is read and named; an absolute `file:` path is never
13783+
/// read (a lock is untrusted input, #1516); a codeload tarball names no
13784+
/// registry package.
13785+
#[test]
13786+
fn issue_1951_yarn_classic_copy_named_as_vendored_reads_it() {
13787+
use crate::formats::yarn::copy::classic_copy_ref;
13788+
let ovr = npm_override(
13789+
"left-pad",
13790+
"1.3.0",
13791+
"http://p.test/lp.tgz",
13792+
"sha512-PATCHED==",
13793+
);
13794+
let registry_block = "left-pad@1.3.0:\n version \"1.3.0\"\n \
13795+
resolved \"https://registry.yarnpkg.com/left-pad/-/left-pad-1.3.0.tgz#bbbb\"\n \
13796+
integrity sha512-UPSTREAMupstream==\n";
13797+
let left_pad = r#"{"name":"left-pad","version":"1.3.0"}"#;
13798+
let manifests = BTreeMap::from([
13799+
(
13800+
"../ext/lpdir/package.json".to_string(),
13801+
left_pad.to_string(),
13802+
),
13803+
("/abs/lpdir/package.json".to_string(), left_pad.to_string()),
13804+
("lpdir/package.json".to_string(), left_pad.to_string()),
13805+
]);
13806+
let codeload = "https://codeload.github.com/stevemao/left-pad/tar.gz/abc";
13807+
for (case, patterns, resolved, named) in [
13808+
("in-root directory", "lp2@file:./lpdir", None, true),
13809+
("out-of-root directory", "lp2@file:../ext/lpdir", None, true),
13810+
("absolute directory", "lp2@file:/abs/lpdir", None, false),
13811+
(
13812+
"codeload tarball",
13813+
"lp2@stevemao/left-pad#v1.3.0",
13814+
Some(codeload),
13815+
false,
13816+
),
13817+
] {
13818+
// The package vendored and VEX identify this copy as.
13819+
let pats = vec![patterns.to_string()];
13820+
let vendored = classic_copy_ref(&pats, resolved).and_then(|copy| {
13821+
let bytes = copy
13822+
.read_path()
13823+
.and_then(|rel| manifests.get(&rel))
13824+
.map(|t| t.as_bytes());
13825+
copy.identity("1.3.0", bytes)
13826+
});
13827+
assert_eq!(
13828+
vendored.is_some(),
13829+
named,
13830+
"{case}: vendored reads {vendored:?}"
13831+
);
13832+
13833+
let mut block = format!("\"{patterns}\":\n version \"1.3.0\"\n");
13834+
if let Some(url) = resolved {
13835+
block.push_str(&format!(" resolved \"{url}\"\n"));
13836+
}
13837+
let mut files = manifests.clone();
13838+
files.insert(
13839+
"yarn.lock".to_string(),
13840+
format!("# yarn lockfile v1\n\n\n{registry_block}\n{block}"),
13841+
);
13842+
let mut r = RewriteResult::default();
13843+
rewrite_yarn_classic(&files, std::slice::from_ref(&ovr), &mut r);
13844+
assert!(
13845+
r.files["yarn.lock"].contains("resolved \"http://p.test/lp.tgz#5ha1\""),
13846+
"{case}: the registry block is still pinned: {:?}",
13847+
r.warnings
13848+
);
13849+
let hosted = r.warnings.iter().any(|w| w.detail.contains("lp2@"));
13850+
assert_eq!(hosted, named, "{case}: {:?}", r.warnings);
13851+
assert_eq!(
13852+
r.bundled_skipped_uuids.contains(&ovr.patch_uuid),
13853+
named,
13854+
"{case}"
13855+
);
13856+
}
13857+
}
13858+
1377513859
/// #363: yarn 1 fetches a git-pattern block with its git fetcher from
1377613860
/// the block's `resolved`, so a hosted tarball there makes every later
1377713861
/// install fail. The block stays byte-identical with a named warning,

0 commit comments

Comments
 (0)