Fix npm crawler missing configured install roots (#493, #518) - #520
Conversation
Assisted-by: Claude Code:claude-opus-5-5
The npm crawler found a project's installed packages only in dirs named node_modules, so it never looked where yarn classic installs with `--modules-folder` or where Rush installs (common/temp). Agent mode reported those packages as not installed and left them unpatched, and hosted vex treated the unpatched copy as absent and attested the patch from the lockfile pin alone. The crawler now also reads the nearest .yarnrc --modules-folder and adds common/temp/node_modules in a Rush repo (rush.json at the root). Fixes #493, #518 Assisted-by: Claude Code:claude-opus-5-5
|
BugBot review Generated by Claude Code |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 2 potential issues.
Autofix Details
Bugbot Autofix prepared fixes for both issues found in the latest run.
- ✅ Fixed: Bare read of workspace
.yarnrc- Replaced bare std::fs::read_to_string with read_regular_to_string_sync to prevent blocking on FIFO files at the .yarnrc path.
- ✅ Fixed: Unguarded yarn modules-folder path
- Added normalize_modules_folder function to normalize paths and applied is_safe_multi_segment check to prevent path traversal and escaping project root.
Or push these changes by commenting:
@cursor push 515f68cd5e
Preview (515f68cd5e)
diff --git a/crates/socket-patch-core/src/crawlers/npm_crawler.rs b/crates/socket-patch-core/src/crawlers/npm_crawler.rs
--- a/crates/socket-patch-core/src/crawlers/npm_crawler.rs
+++ b/crates/socket-patch-core/src/crawlers/npm_crawler.rs
@@ -8,7 +8,7 @@
use super::types::{CrawledPackage, CrawlerOptions};
use super::walk_pool::{par_map, run_walk};
use crate::patch::path_safety;
-use crate::utils::fs::{is_dir, is_dir_sync, read_dir_entries_sync};
+use crate::utils::fs::{is_dir, is_dir_sync, read_dir_entries_sync, read_regular_to_string_sync};
use crate::utils::purl::{percent_decode_purl_component, strip_purl_qualifiers};
use crate::vendor::vlt_lock_text::decode_vlt_dep_id;
@@ -46,7 +46,11 @@
pub(super) fn configured_install_roots(start_path: &Path) -> Vec<PathBuf> {
let mut roots = Vec::new();
if let Some(folder) = yarnrc_modules_folder(start_path) {
- roots.push(start_path.join(folder));
+ if let Some(normalized) = normalize_modules_folder(&folder) {
+ if path_safety::is_safe_multi_segment(&normalized) {
+ roots.push(start_path.join(normalized));
+ }
+ }
}
if start_path.join("rush.json").is_file() {
roots.push(start_path.join("common").join("temp").join("node_modules"));
@@ -78,10 +82,12 @@
}
/// The `--modules-folder` value from the nearest `.yarnrc` at or above
-/// `start_path`, if any `.yarnrc` sets it.
+/// `start_path`, if any `.yarnrc` sets it. Read `.yarnrc` via
+/// `read_regular_to_string_sync` for the same reason `package.json` is:
+/// a FIFO planted at that workspace path would wedge a plain read forever.
fn yarnrc_modules_folder(start_path: &Path) -> Option<String> {
start_path.ancestors().find_map(|dir| {
- let rc = std::fs::read_to_string(dir.join(".yarnrc")).ok()?;
+ let rc = read_regular_to_string_sync(&dir.join(".yarnrc")).ok()?;
parse_yarnrc_modules_folder(&rc)
})
}
@@ -137,6 +143,29 @@
(end > 0).then(|| (s[..end].to_string(), &s[end..]))
}
+/// Reduce a `--modules-folder` value to plain `a/b` segments before the
+/// safety gate. Yarn accepts `./`-prefixed and `.`-interleaved values
+/// (`./deps`, `lib/./deps`) and either separator, so those shapes must
+/// resolve rather than be refused. `..` is resolved lexically the same way
+/// Composer's config.vendor-dir normalization does; a value that climbs
+/// above the project root (or reduces to it) fails closed as `None`.
+fn normalize_modules_folder(raw: &str) -> Option<String> {
+ if raw.starts_with(['/', '\\']) {
+ return None;
+ }
+ let mut segments: Vec<&str> = Vec::new();
+ for segment in raw.split(['/', '\\']) {
+ match segment {
+ "" | "." => {}
+ ".." => {
+ segments.pop()?;
+ }
+ other => segments.push(other),
+ }
+ }
+ (!segments.is_empty()).then(|| segments.join("/"))
+}
+
// ---------------------------------------------------------------------------
// Helper: read and parse package.json
// ---------------------------------------------------------------------------
@@ -1237,7 +1266,11 @@
/// Inside a store entry (`store_entry`) a link is a dependency edge into
/// a sibling entry, whose own visit records that copy, so only a real
/// directory there matches.
- fn visit_resolver_dir(nm_path: PathBuf, store_entry: bool, pending: &[Target]) -> ResolverVisit {
+ fn visit_resolver_dir(
+ nm_path: PathBuf,
+ store_entry: bool,
+ pending: &[Target],
+ ) -> ResolverVisit {
let listing = list_dir_sync(&nm_path);
let probe_filter = ProbeFilter::new(&listing);
let matched = pending
@@ -4329,6 +4362,30 @@
}
}
+ #[test]
+ fn test_normalize_modules_folder() {
+ let n = normalize_modules_folder;
+ // Yarn-legal `./` prefixes and `.` segments reduce to the
+ // plain path; either separator is accepted.
+ assert_eq!(n("./deps").as_deref(), Some("deps"));
+ assert_eq!(n("./lib/deps").as_deref(), Some("lib/deps"));
+ assert_eq!(n("lib/./deps").as_deref(), Some("lib/deps"));
+ assert_eq!(n("lib\\deps").as_deref(), Some("lib/deps"));
+ assert_eq!(n("lib/../deps").as_deref(), Some("deps"));
+ assert_eq!(n("deps").as_deref(), Some("deps"));
+ // Escaping the project, reducing to it, or absolute — fail closed.
+ assert_eq!(n(".."), None);
+ assert_eq!(n("../elsewhere"), None);
+ assert_eq!(n("lib/../.."), None);
+ assert_eq!(n("."), None);
+ assert_eq!(n("a/.."), None);
+ assert_eq!(n("/etc/deps"), None);
+ assert_eq!(n("\\\\share\\deps"), None);
+ // A drive-letter segment survives normalization; the
+ // `is_safe_multi_segment` gate downstream rejects the colon.
+ assert_eq!(n("C:\\deps").as_deref(), Some("C:/deps"));
+ }
+
fn local_options(cwd: &Path) -> CrawlerOptions {
CrawlerOptions {
cwd: cwd.to_path_buf(),You can send follow-ups to the cloud agent here.
The .yarnrc --modules-folder value comes from the project being scanned and names a tree apply writes patches into. An absolute or escaping value (/etc, ../elsewhere) is now ignored, like composer's vendor-dir, so the crawl and apply stay inside the project. The .yarnrc is also read with the regular-file guard, so a FIFO planted at that path can no longer hang scan, apply or vex. Assisted-by: Claude Code:claude-opus-5-5
|
BugBot review Generated by Claude Code |
|
[agent]
No fix to port exists, since nothing in the diff reaches this path. I'll re-run the failed job once when the workflow run finishes. If it fails again I'll treat it as real and root-cause it. Generated by Claude Code |
|
[agent] Generated by Claude Code |
Mikola Lysenko (mikolalysenko)
added
the
Ready for review
|
[agent] Ready for review at
Generated by Claude Code |
|
Reviewed The default modules-folder and Rush paths are covered, but two valid Yarn Classic configurations still resolve to the wrong install root.
Validation: |
Two valid yarn classic setups still pointed the crawler at the wrong install root, leaving the real tree unpatched and letting vex fall back to lockfile-only evidence: - A modules folder set in an ancestor .yarnrc is relative to that file's directory, not the project. /repo/.yarnrc with "--modules-folder project/deps" installs /repo/project into /repo/project/deps; the crawler looked in project/project/deps. - --install.modules-folder wins over --modules-folder regardless of line order or which .yarnrc defines each, because yarn merges each key through the rc files separately and applies install-scoped args last. The crawler took whichever line came last. The resolved folder must still lie strictly inside the project. Verified against yarn 1.22.22 installs for all three layouts. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012pqLNDF3U1KUabGfPsxoZ8
|
[agent] Thanks, both findings are confirmed and fixed in ec95949. I checked yarn 1.22.22's own
Tests:
Both new crawl tests fail on 113ae51 ( Generated by Claude Code |
|
BugBot review Generated by Claude Code |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
Bugbot Autofix is ON. A cloud agent has been kicked off to fix the reported issue.
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit ec95949. Configure here.
|
Follow-up reviewed Inherited Validation on this head: both original reviewer regression tests pass (2 passed), and |
3 participants
LLM Description written by Claude Code:claude-opus-5-5
Fixes #493
Fixes #518
Summary
The npm crawler now also looks where the package manager is configured to install, not just in directories named
node_modules:--modules-folder(Agent mode ignores yarn classic's--modules-folder: packages installed there are reported "not installed" andscan --mode agentexits 0 leaving them unpatched #493): the effective modules folder from the.yarnrcfiles at or above the project, resolved the way yarn 1.x resolves it. Each key comes from the nearest.yarnrcthat sets it, and its value is resolved against that file's directory.--install.modules-folderwins over--modules-folder. Only a result strictly inside the project is honored.vexattests a transitive dep not_affected while its installed copy is unpatched, and agent apply reports it package_not_installed #518):common/temp/node_moduleswhenrush.jsonis at the root. Rush runs pnpm there, so every transitive dependency lives in that.pnpmstore, and the projects'node_modulesonly link their direct deps.For users this means agent-mode
apply/scannow patches packages in these layouts instead of reporting thempackage_not_installed. It also meansvexno longer issues a falsenot_affected: it used to read the uncrawled, unpatched installed copy as "nothing installed" and attest from the hosted lock pin. Now installed evidence wins (hash_mismatch), as on a default layout. The vendoredvendored_tree_out_of_syncdisclosure also fires for a modules-folder tree.Root cause
NpmCrawler::find_local_node_modules_dirs(crates/socket-patch-core/src/crawlers/npm_crawler.rs) built the install roots only from a walk that collects directories literally namednode_modulesand prunestemp(SKIP_DIRS). It never consulted the package manager's own install-location config. Every consumer goes throughget_node_modules_paths: apply, scan, vex verification,vex_consumed's alias walk, and the vendored out-of-sync check. So that one function caused all the reported symptoms.Change
configured_install_roots(start_path): the configured roots listed above.merge_configured_install_roots: appends those roots to the walk's roots. Walked roots inside a configured root are dropped (the walk descends into a non-node_modulesmodules folder and would otherwise reportdeps/<pkg>/node_modulesas a workspace), and duplicates are skipped. Walk order is otherwise unchanged, so theseendedup winners on existing layouts don't change..yarnrc(yarn lockfile syntax) reader for--modules-folderand--install.modules-folder, kept apart: bare or quoted key and value, optional:,#comments, BOM/CRLF, last setting wins per key. The file is read through the regular-file guard.resolve_modules_folderresolves the value against the defining.yarnrc's directory and keeps it only if it is strictly inside the project, like composer'sconfig.vendor-dirgate. Absolute, drive-qualified or escaping values are ignored, so crawl and apply stay inside the project.npm_crawler/oracle.rs) uses the same helper, so the randomized walk-equivalence tests still compare like with like.Out of scope (not reported in either issue): Rush subspaces' per-subspace temp folders, and a
.yarnrcin the home or/etcdirectory that isn't above the project. Wrappers undernpm/,pypi/,gem/only dispatch to the binary and need no change.Tests (red without the fix → green with it)
npm_crawler::tests::test_yarnrc_modules_folder_is_a_crawl_root[deps/outer/node_modules], not[deps]npm_crawler::tests::test_parse_yarnrc_modules_foldere2e_vex_redirect::yarn_modules_folder_install_is_hash_verified_not_lockfile_attestedverified/not_affectedover the unpatcheddeps/copye2e_vex_vendor::vendored_modules_folder_tree_out_of_sync_warnsvendored_tree_out_of_syncwarningin_process_alternate_installers::yarn_modules_folder_install_is_patchedin_process_alternate_installers::yarn_classic_modules_folder_install_then_apply_patches_file(realyarn install1.22.22)npm_crawler::tests::test_rush_common_temp_is_a_crawl_rootcommon/temp/node_modulese2e_vex_redirect::rush_common_temp_install_is_hash_verified_not_lockfile_attestedverified/not_affectedover the unpatched store copyin_process_alternate_installers::rush_transitive_dep_in_common_temp_store_is_patchede2e_redirect_rush_simtier 1 (realpnpm@9 install --frozen-lockfile): new stale-store legrush, unpatched store copy: exit Some(0)npm_crawler::tests::test_merge_configured_install_rootstest_resolve_modules_folder,test_escaping_modules_folder_is_not_a_crawl_root../outside/ absolute dir crawledtest_fifo_yarnrc_does_not_block_the_crawlopen(2))test_inherited_yarnrc_modules_folder_resolves_against_its_dir[](looked inproject/project/deps)test_install_scoped_modules_folder_takes_precedence[general], not[specific]Commands run locally (Linux):
cargo clippy --workspace --all-features -- -D warnings(the CI command): clean.cargo test -p socket-patch-core --lib: all 71 crawler tests pass. Four tests elsewhere fail only because this container runs as root (read-only-directory tests can't fail a write as uid 0):copy_tree::relax_loop_must_not_traverse_symlinked_root,vlt_heal::an_unremovable_hidden_lock_keeps_every_store_entry,pypi_poetry::wire_write_failure_…,pypi_requirements::wire_failure_rolls_back_…. This PR doesn't touch them, and they pass in CI.cargo test -p socket-patch-cli --all-features --test e2e_vex_redirect(27/27),--test e2e_vex_vendor(23/23),--test in_process_alternate_installers(21/21),--test e2e_redirect_rush_sim -- --ignored(3/3, real pnpm).yarn install --offline1.22.22 confirms the inherited and install-scoped layouts: installs land inproject/deps,project/specificandproject/specific.cargo test --workspace --all-featuresbuild exceeds this container's disk allowance, so the full matrix runs in CI.cargo fmt --all -- --checkisn't a CI gate and isn't clean onmain. Only this PR's hunks were formatted; no unrelated reformatting is included.Review and CI
.yarnrcFIFO could block the crawl. It's now read through the regular-file guard.--modules-folderpath could escape the project. It's now normalized and gated like composervendor-dir..yarnrcmodules folder must resolve against its own directory.--install.modules-foldertakes precedence over--modules-folder.native (ubuntu-latest, 0.0.0-16)(vlt) andnative (ubuntu-latest, 1.3.10)(Bun) failed once during a production patch-API outage (Connection reset by peer, 02:28–02:35 UTC). Each passed on its single re-run.🤖 Generated with Claude Code
https://claude.ai/code/session_012pqLNDF3U1KUabGfPsxoZ8