diff --git a/codex-rs/core/src/tools/handlers/dependency_check.rs b/codex-rs/core/src/tools/handlers/dependency_check.rs index 1921498d9d..ac13598508 100644 --- a/codex-rs/core/src/tools/handlers/dependency_check.rs +++ b/codex-rs/core/src/tools/handlers/dependency_check.rs @@ -35,7 +35,6 @@ use codex_dependency_check::detect_dependency_install_command; use codex_dependency_check::npm_ci_command; use codex_dependency_check::npm_install_command; use codex_dependency_check::npm_query_installed_command; -use codex_dependency_check::npm_query_lock_command; use codex_dependency_check::npm_rebuild_command; use codex_dependency_check::validate_npm_manifest; use codex_protocol::exec_output::ExecToolCallOutput; @@ -172,13 +171,7 @@ impl DependencyCheckHandler { &resolve, )); } - let checked_graph = match query_graph( - &runner, - resolution_dir.path(), - WorkingDirectoryAccess::PreapprovedScratch, - ) - .await? - { + let checked_graph = match read_lock_graph(resolution_dir.path()).await { Ok(graph) => graph, Err(message) => return Ok(blocked_output(message)), }; @@ -214,11 +207,10 @@ impl DependencyCheckHandler { if lock_update.exit_code != 0 { return Ok(command_failure_output("project lock update", &lock_update)); } - let locked_graph = - match query_graph(&runner, &workdir, WorkingDirectoryAccess::Default).await? { - Ok(graph) => graph, - Err(message) => return Ok(blocked_output(message)), - }; + let locked_graph = match read_lock_graph(&workdir).await { + Ok(graph) => graph, + Err(message) => return Ok(blocked_output(message)), + }; if let Err(mismatch) = checked_graph.compare(&locked_graph) { return Ok(graph_mismatch_output("project lock update", &mismatch)); } @@ -278,25 +270,19 @@ pub(crate) fn dependency_manifest_edit_message() -> String { .to_string() } -async fn query_graph( - runner: &DependencyCommandRunner, - cwd: &Path, - working_directory_access: WorkingDirectoryAccess, -) -> Result, FunctionCallError> { - let query = runner - .run( - npm_query_lock_command(), - cwd, - ScriptPolicy::Disabled, - working_directory_access, - "Read the exact npm lock graph for dependency verification.", +async fn read_lock_graph(cwd: &Path) -> Result { + let lockfile_path = cwd.join("package-lock.json"); + let lockfile = read_regular_file(&lockfile_path).await.map_err(|err| { + format!( + "Dependency Check stopped before lifecycle scripts because it could not read {}: {err}", + lockfile_path.display() ) - .await?; - if query.exit_code != 0 { - return Ok(Err(command_failure_message("npm graph query", &query))); - } - Ok(NpmGraph::from_query_json(&query.stdout.text) - .map_err(|err| format!("Dependency Check stopped before lifecycle scripts because npm returned an unverifiable graph: {err}"))) + })?; + NpmGraph::from_package_lock_json(&lockfile).map_err(|err| { + format!( + "Dependency Check stopped before lifecycle scripts because npm produced an unverifiable package-lock.json graph: {err}" + ) + }) } async fn query_installed_graph( diff --git a/codex-rs/dependency-check/src/lib.rs b/codex-rs/dependency-check/src/lib.rs index f5360d1b1e..2038e53b14 100644 --- a/codex-rs/dependency-check/src/lib.rs +++ b/codex-rs/dependency-check/src/lib.rs @@ -17,7 +17,6 @@ pub use npm::NpmManifestError; pub use npm::npm_ci_command; pub use npm::npm_install_command; pub use npm::npm_query_installed_command; -pub use npm::npm_query_lock_command; pub use npm::npm_rebuild_command; pub use npm::validate_npm_manifest; pub use osv::DependencyPolicyAction; diff --git a/codex-rs/dependency-check/src/npm.rs b/codex-rs/dependency-check/src/npm.rs index b69c0701d5..fbf8458793 100644 --- a/codex-rs/dependency-check/src/npm.rs +++ b/codex-rs/dependency-check/src/npm.rs @@ -1,6 +1,7 @@ use crate::DependencyCheckRequest; use serde::Deserialize; use serde_json::Value; +use std::collections::BTreeMap; use std::collections::BTreeSet; use thiserror::Error; use url::Url; @@ -31,18 +32,28 @@ pub struct NpmInstalledGraph { } impl NpmGraph { - pub fn from_query_json(json: &str) -> Result { - let nodes: Vec = serde_json::from_str(json)?; + pub fn from_package_lock_json(json: &str) -> Result { + let lockfile: NpmPackageLock = serde_json::from_str(json)?; + if !matches!(lockfile.lockfile_version, 2 | 3) { + return Err(NpmGraphError::UnsupportedLockfileVersion( + lockfile.lockfile_version, + )); + } let mut packages = BTreeSet::new(); - for node in nodes { - if node.location.as_deref() == Some("") { + for (location, package) in lockfile.packages { + if location.is_empty() { continue; } - let name = node.name.ok_or(NpmGraphError::MissingField("name"))?; - let version = node.version.ok_or(NpmGraphError::MissingField("version"))?; - let resolved = node + let name = package + .name + .or_else(|| package_name_from_lockfile_location(&location)) + .ok_or(NpmGraphError::MissingField("name"))?; + let version = package + .version + .ok_or(NpmGraphError::MissingField("version"))?; + let resolved = package .resolved .ok_or_else(|| NpmGraphError::UnsupportedSource { name: name.clone(), @@ -61,7 +72,7 @@ impl NpmGraph { resolved: Some(resolved), }); } - let integrity = node + let integrity = package .integrity .filter(|integrity| !integrity.is_empty()) .ok_or_else(|| NpmGraphError::MissingIntegrity { @@ -202,10 +213,12 @@ impl std::error::Error for NpmInstalledGraphMismatch {} #[derive(Debug, Error)] pub enum NpmGraphError { - #[error("npm query returned invalid JSON: {0}")] + #[error("npm graph input is invalid JSON: {0}")] InvalidJson(#[from] serde_json::Error), - #[error("npm query result is missing required field `{0}`")] + #[error("npm graph entry is missing required field `{0}`")] MissingField(&'static str), + #[error("npm package-lock version {0} is unsupported; expected version 2 or 3")] + UnsupportedLockfileVersion(u64), #[error("npm package `{name}@{version}` has an unsupported resolved source: {resolved:?}")] UnsupportedSource { name: String, @@ -278,16 +291,6 @@ pub fn npm_install_command( command } -pub fn npm_query_lock_command() -> Vec { - vec![ - "npm".to_string(), - "query".to_string(), - "*".to_string(), - "--json".to_string(), - "--package-lock-only".to_string(), - ] -} - pub fn npm_ci_command() -> Vec { vec![ "npm".to_string(), @@ -316,10 +319,45 @@ struct NpmQueryNode { name: Option, version: Option, resolved: Option, - integrity: Option, location: Option, } +#[derive(Debug, Deserialize)] +#[serde(rename_all = "camelCase")] +struct NpmPackageLock { + lockfile_version: u64, + #[serde(default)] + packages: BTreeMap, +} + +#[derive(Debug, Deserialize)] +struct NpmLockPackage { + name: Option, + version: Option, + resolved: Option, + integrity: Option, +} + +fn package_name_from_lockfile_location(location: &str) -> Option { + let (_, package_path) = location.rsplit_once("node_modules/")?; + let mut components = package_path.split('/'); + let first = components.next()?; + if first.is_empty() { + return None; + } + if first.starts_with('@') { + let second = components.next()?; + if second.is_empty() || components.next().is_some() { + return None; + } + Some(format!("{first}/{second}")) + } else if components.next().is_none() { + Some(first.to_string()) + } else { + None + } +} + struct ParsedNpmQueryNode { name: String, version: String, @@ -405,11 +443,14 @@ mod tests { #[test] fn parses_and_compares_lock_graphs() { - let graph = NpmGraph::from_query_json( - r#"[ - {"name":"example","version":"1.0.0","location":"","resolved":null}, - {"name":"zod","version":"3.23.8","location":"node_modules/zod","resolved":"https://registry.npmjs.org/zod/-/zod-3.23.8.tgz","integrity":"sha512-example"} - ]"#, + let graph = NpmGraph::from_package_lock_json( + r#"{ + "lockfileVersion": 3, + "packages": { + "": {"name":"example","version":"1.0.0"}, + "node_modules/zod": {"version":"3.23.8","resolved":"https://registry.npmjs.org/zod/-/zod-3.23.8.tgz","integrity":"sha512-example"} + } + }"#, ) .expect("parse graph"); assert_eq!( @@ -430,12 +471,12 @@ mod tests { #[test] fn reports_lock_and_installed_graph_mismatches() { - let checked = NpmGraph::from_query_json( - r#"[{"name":"zod","version":"3.23.8","location":"node_modules/zod","resolved":"https://registry.npmjs.org/zod/-/zod-3.23.8.tgz","integrity":"sha512-checked"}]"#, + let checked = NpmGraph::from_package_lock_json( + r#"{"lockfileVersion":3,"packages":{"node_modules/zod":{"version":"3.23.8","resolved":"https://registry.npmjs.org/zod/-/zod-3.23.8.tgz","integrity":"sha512-checked"}}}"#, ) .expect("checked graph"); - let changed = NpmGraph::from_query_json( - r#"[{"name":"zod","version":"3.24.0","location":"node_modules/zod","resolved":"https://registry.npmjs.org/zod/-/zod-3.24.0.tgz","integrity":"sha512-changed"}]"#, + let changed = NpmGraph::from_package_lock_json( + r#"{"lockfileVersion":3,"packages":{"node_modules/zod":{"version":"3.24.0","resolved":"https://registry.npmjs.org/zod/-/zod-3.24.0.tgz","integrity":"sha512-changed"}}}"#, ) .expect("changed graph"); let installed = NpmInstalledGraph::from_query_json( @@ -449,13 +490,45 @@ mod tests { #[test] fn rejects_unverifiable_sources() { - let err = NpmGraph::from_query_json( - r#"[{"name":"local","version":"1.0.0","location":"node_modules/local","resolved":"file:../local"}]"#, + let err = NpmGraph::from_package_lock_json( + r#"{"lockfileVersion":3,"packages":{"node_modules/local":{"version":"1.0.0","resolved":"file:../local"}}}"#, ) .expect_err("local source should fail"); assert!(matches!(err, NpmGraphError::UnsupportedSource { .. })); } + #[test] + fn derives_scoped_and_nested_package_names_from_lockfile_locations() { + let graph = NpmGraph::from_package_lock_json( + r#"{ + "lockfileVersion": 2, + "packages": { + "node_modules/@scope/top": {"version":"1.0.0","resolved":"https://registry.npmjs.org/@scope/top/-/top-1.0.0.tgz","integrity":"sha512-top"}, + "node_modules/parent/node_modules/@scope/nested": {"version":"2.0.0","resolved":"https://registry.npmjs.org/@scope/nested/-/nested-2.0.0.tgz","integrity":"sha512-nested"} + } + }"#, + ) + .expect("parse graph"); + + assert_eq!( + graph.coordinates(), + BTreeSet::from([ + ("@scope/nested".to_string(), "2.0.0".to_string()), + ("@scope/top".to_string(), "1.0.0".to_string()), + ]) + ); + } + + #[test] + fn rejects_legacy_lockfile_without_packages_graph() { + let err = NpmGraph::from_package_lock_json( + r#"{"lockfileVersion":1,"dependencies":{"zod":{"version":"3.23.8"}}}"#, + ) + .expect_err("legacy lockfile should fail"); + + assert!(matches!(err, NpmGraphError::UnsupportedLockfileVersion(1))); + } + #[test] fn rejects_workspaces_and_non_npm_projects() { assert!(matches!( diff --git a/codex-rs/dependency-check/src/osv.rs b/codex-rs/dependency-check/src/osv.rs index a191834afa..e9c7429bbd 100644 --- a/codex-rs/dependency-check/src/osv.rs +++ b/codex-rs/dependency-check/src/osv.rs @@ -198,8 +198,8 @@ mod tests { use wiremock::matchers::path; fn graph() -> NpmGraph { - NpmGraph::from_query_json( - r#"[{"name":"example","version":"1.0.0","location":"node_modules/example","resolved":"https://registry.npmjs.org/example/-/example-1.0.0.tgz","integrity":"sha512-example"}]"#, + NpmGraph::from_package_lock_json( + r#"{"lockfileVersion":3,"packages":{"node_modules/example":{"version":"1.0.0","resolved":"https://registry.npmjs.org/example/-/example-1.0.0.tgz","integrity":"sha512-example"}}}"#, ) .expect("parse graph") } diff --git a/docs/dependency-check.md b/docs/dependency-check.md index 931721a906..7f5f4f7a16 100644 --- a/docs/dependency-check.md +++ b/docs/dependency-check.md @@ -29,7 +29,7 @@ direct dependencies. into a temporary directory. 2. Run `npm install --package-lock-only --ignore-scripts` there to resolve the complete graph without running package lifecycle code. -3. Run `npm query '*' --json --package-lock-only` and require every package to +3. Parse the npm v2/v3 `package-lock.json` graph and require every package to have an HTTPS artifact URL and integrity value. 4. Query the OSV batch API for every unique exact npm package coordinate. 5. Update the real project lock graph through Codex's normal sandbox and @@ -73,7 +73,7 @@ description = "Workspace write access with npm manifests read-only." ":tmpdir" = "write" "/path/to/node/runtime" = "read" -[permissions.dependency-check.filesystem.":project_roots"] +[permissions.dependency-check.filesystem.":workspace_roots"] "." = "write" "package.json" = "read" "package-lock.json" = "read"