Skip to content

Commit 515f68c

Browse files
committed
Fix npm crawler FIFO-safe .yarnrc read and normalize modules-folder path
- Replace bare std::fs::read_to_string with read_regular_to_string_sync for .yarnrc to prevent blocking on FIFOs - Add normalize_modules_folder function to handle path normalization for --modules-folder config values - Apply path_safety::is_safe_multi_segment check to prevent path traversal and escaping project root - Add comprehensive tests for normalize_modules_folder covering edge cases
1 parent 549f8c0 commit 515f68c

1 file changed

Lines changed: 62 additions & 5 deletions

File tree

‎crates/socket-patch-core/src/crawlers/npm_crawler.rs‎

Lines changed: 62 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,7 @@ use serde::Deserialize;
88
use super::types::{CrawledPackage, CrawlerOptions};
99
use super::walk_pool::{par_map, run_walk};
1010
use crate::patch::path_safety;
11-
use crate::utils::fs::{is_dir, is_dir_sync, read_dir_entries_sync};
11+
use crate::utils::fs::{is_dir, is_dir_sync, read_dir_entries_sync, read_regular_to_string_sync};
1212
use crate::utils::purl::{percent_decode_purl_component, strip_purl_qualifiers};
1313
use crate::vendor::vlt_lock_text::decode_vlt_dep_id;
1414

@@ -46,7 +46,11 @@ const SKIP_DIRS: &[&str] = &[
4646
pub(super) fn configured_install_roots(start_path: &Path) -> Vec<PathBuf> {
4747
let mut roots = Vec::new();
4848
if let Some(folder) = yarnrc_modules_folder(start_path) {
49-
roots.push(start_path.join(folder));
49+
if let Some(normalized) = normalize_modules_folder(&folder) {
50+
if path_safety::is_safe_multi_segment(&normalized) {
51+
roots.push(start_path.join(normalized));
52+
}
53+
}
5054
}
5155
if start_path.join("rush.json").is_file() {
5256
roots.push(start_path.join("common").join("temp").join("node_modules"));
@@ -78,10 +82,12 @@ pub(super) fn merge_configured_install_roots(
7882
}
7983

8084
/// The `--modules-folder` value from the nearest `.yarnrc` at or above
81-
/// `start_path`, if any `.yarnrc` sets it.
85+
/// `start_path`, if any `.yarnrc` sets it. Read `.yarnrc` via
86+
/// `read_regular_to_string_sync` for the same reason `package.json` is:
87+
/// a FIFO planted at that workspace path would wedge a plain read forever.
8288
fn yarnrc_modules_folder(start_path: &Path) -> Option<String> {
8389
start_path.ancestors().find_map(|dir| {
84-
let rc = std::fs::read_to_string(dir.join(".yarnrc")).ok()?;
90+
let rc = read_regular_to_string_sync(&dir.join(".yarnrc")).ok()?;
8591
parse_yarnrc_modules_folder(&rc)
8692
})
8793
}
@@ -137,6 +143,29 @@ fn split_yarnrc_token(s: &str, is_key: bool) -> Option<(String, &str)> {
137143
(end > 0).then(|| (s[..end].to_string(), &s[end..]))
138144
}
139145

146+
/// Reduce a `--modules-folder` value to plain `a/b` segments before the
147+
/// safety gate. Yarn accepts `./`-prefixed and `.`-interleaved values
148+
/// (`./deps`, `lib/./deps`) and either separator, so those shapes must
149+
/// resolve rather than be refused. `..` is resolved lexically the same way
150+
/// Composer's config.vendor-dir normalization does; a value that climbs
151+
/// above the project root (or reduces to it) fails closed as `None`.
152+
fn normalize_modules_folder(raw: &str) -> Option<String> {
153+
if raw.starts_with(['/', '\\']) {
154+
return None;
155+
}
156+
let mut segments: Vec<&str> = Vec::new();
157+
for segment in raw.split(['/', '\\']) {
158+
match segment {
159+
"" | "." => {}
160+
".." => {
161+
segments.pop()?;
162+
}
163+
other => segments.push(other),
164+
}
165+
}
166+
(!segments.is_empty()).then(|| segments.join("/"))
167+
}
168+
140169
// ---------------------------------------------------------------------------
141170
// Helper: read and parse package.json
142171
// ---------------------------------------------------------------------------
@@ -1237,7 +1266,11 @@ impl NpmCrawler {
12371266
/// Inside a store entry (`store_entry`) a link is a dependency edge into
12381267
/// a sibling entry, whose own visit records that copy, so only a real
12391268
/// directory there matches.
1240-
fn visit_resolver_dir(nm_path: PathBuf, store_entry: bool, pending: &[Target]) -> ResolverVisit {
1269+
fn visit_resolver_dir(
1270+
nm_path: PathBuf,
1271+
store_entry: bool,
1272+
pending: &[Target],
1273+
) -> ResolverVisit {
12411274
let listing = list_dir_sync(&nm_path);
12421275
let probe_filter = ProbeFilter::new(&listing);
12431276
let matched = pending
@@ -4329,6 +4362,30 @@ mod tests {
43294362
}
43304363
}
43314364

4365+
#[test]
4366+
fn test_normalize_modules_folder() {
4367+
let n = normalize_modules_folder;
4368+
// Yarn-legal `./` prefixes and `.` segments reduce to the
4369+
// plain path; either separator is accepted.
4370+
assert_eq!(n("./deps").as_deref(), Some("deps"));
4371+
assert_eq!(n("./lib/deps").as_deref(), Some("lib/deps"));
4372+
assert_eq!(n("lib/./deps").as_deref(), Some("lib/deps"));
4373+
assert_eq!(n("lib\\deps").as_deref(), Some("lib/deps"));
4374+
assert_eq!(n("lib/../deps").as_deref(), Some("deps"));
4375+
assert_eq!(n("deps").as_deref(), Some("deps"));
4376+
// Escaping the project, reducing to it, or absolute — fail closed.
4377+
assert_eq!(n(".."), None);
4378+
assert_eq!(n("../elsewhere"), None);
4379+
assert_eq!(n("lib/../.."), None);
4380+
assert_eq!(n("."), None);
4381+
assert_eq!(n("a/.."), None);
4382+
assert_eq!(n("/etc/deps"), None);
4383+
assert_eq!(n("\\\\share\\deps"), None);
4384+
// A drive-letter segment survives normalization; the
4385+
// `is_safe_multi_segment` gate downstream rejects the colon.
4386+
assert_eq!(n("C:\\deps").as_deref(), Some("C:/deps"));
4387+
}
4388+
43324389
fn local_options(cwd: &Path) -> CrawlerOptions {
43334390
CrawlerOptions {
43344391
cwd: cwd.to_path_buf(),

0 commit comments

Comments
 (0)