QoL updates (#65)

* One-indexed positions in CLI and vulnerable node parse validation

* Address lint and format

* Clarify errors

* Update README

* Bump MSRV

README.md

@@ -44,7 +44,7 @@ let package_resolver = PackageResolver::builder()
 // Build a project from the package resolver.
 let project = Project::new(package_resolver);
 
-// Define a target node.
+// Define a target node (rows/columns are zero-indexed).
 let vulnerable_node = VulnerableNode::new("lru-cache", "index.js", 1017, 17, 1017, 24);
 
 // Compute the reachability graph.

vuln-reach-cli/README.md

@@ -30,7 +30,7 @@ packages = [
   { name = "generic-pool", version = "3.8.2" },
 ]
 vuln = [
-  { package = "generic-pool", module = "lib/Pool.js", start_row = 743, start_column = 17, end_row = 743, end_column = 21 }
+  { package = "generic-pool", module = "lib/Pool.js", start_row = 744, start_column = 18, end_row = 744, end_column = 22 }
 ]
 
 [[projects]]
@@ -42,16 +42,22 @@ packages = [
   { name = "minipass", version = "4.0.2" },
 ]
 vuln = [
-  { package = "lru-cache", module = "index.js", start_row = 1017, start_column = 17, end_row = 1017, end_column = 24 }
+  { package = "lru-cache", module = "index.js", start_row = 1018, start_column = 18, end_row = 1018, end_column = 25 }
 ]
 ```
 
 In each of those objects: 
-- `packages` is the list of packages, and their version, available in the project.
-- `vuln` states that there is a vulnerable node
-  in the specified `package`, in the file `module` at position `(row, column)`. Note that
-  the position is 0-indexed so what would look like `(10, 8)` in a text editor should be
-  specified as `(9, 7)`.
+- `packages` is the list of packages, and their version, available in the project. It must contain
+  the elements of the entire _transitive_ dependency tree, not just the first-level dependencies.
+  You can find the transitive dependency tree via the [Phylum CLI](https://github.com/phylum-dev/cli):
+  ```shell
+  $ npm init -y && npm i --save <top level packages>  # for a new project
+  $ phylum parse package-lock.json
+  ```
+- `vuln` indicates a vulnerable node in the specified `package`, in the file
+  `module` at position `(row, column)`. Note that, differently from the library,
+  the position is 1-indexed so specify these values in the same way they are
+  displayed in a text editor.
 
 The term "vulnerable node" is used loosely here -- any identifier can be chosen as the
 target node that will be searched for.

vuln-reach-cli/sample.toml

@@ -8,7 +8,7 @@ packages = [
   { name = "generic-pool", version = "3.8.2" },
 ]
 vuln = [
-  { package = "generic-pool", module = "lib/Pool.js", start_row = 743, start_column = 17, end_row = 743, end_column = 21 }
+  { package = "generic-pool", module = "lib/Pool.js", start_row = 744, start_column = 18, end_row = 744, end_column = 22 }
 ]
 
 [[projects]]
@@ -20,5 +20,5 @@ packages = [
   { name = "minipass", version = "4.0.2" },
 ]
 vuln = [
-  { package = "lru-cache", module = "index.js", start_row = 1017, start_column = 17, end_row = 1017, end_column = 24 }
+  { package = "lru-cache", module = "index.js", start_row = 1018, start_column = 18, end_row = 1018, end_column = 25 }
 ]

vuln-reach-cli/src/main.rs

@@ -1,16 +1,20 @@
 use std::collections::HashMap;
+use std::fmt::Display;
 use std::path::{Path, PathBuf};
 
 use anyhow::{anyhow, Result};
 use clap::Parser;
 use futures::future;
-use serde::Deserialize;
+use serde::de::Error;
+use serde::{Deserialize, Deserializer};
 use tokio::fs;
 use vuln_reach::javascript::package::reachability::{NodePath, VulnerableNode};
 use vuln_reach::javascript::package::resolver::PackageResolver;
 use vuln_reach::javascript::package::Package;
 use vuln_reach::javascript::project::Project;
 
+type StdResult<T, E> = std::result::Result<T, E>;
+
 #[derive(Deserialize)]
 struct NpmRegistry {
     versions: HashMap<String, Version>,
@@ -77,9 +81,93 @@ struct ProjectDef {
     name: String,
     tarballs: PathBuf,
     packages: Vec<PackageDef>,
+    #[serde(deserialize_with = "deserialize_vulnerable_node")]
     vuln: Vec<VulnerableNode>,
 }
 
+#[derive(Default)]
+struct NodeValidation {
+    start_after_end: Option<((usize, usize), (usize, usize))>,
+    zero_value: bool,
+}
+
+impl NodeValidation {
+    fn new(node: &VulnerableNode) -> Self {
+        let start_row = node.start_row();
+        let start_column = node.start_column();
+        let end_row = node.end_row();
+        let end_column = node.end_column();
+
+        let start_after_end =
+            if start_row > end_row || (start_row == end_row && start_column > end_column) {
+                Some(((start_row, start_column), (end_row, end_column)))
+            } else {
+                None
+            };
+
+        Self {
+            start_after_end,
+            zero_value: node.start_row() == 0
+                || node.end_row() == 0
+                || node.start_column() == 0
+                || node.end_column() == 0,
+        }
+    }
+
+    fn is_error(&self) -> bool {
+        self.start_after_end.is_some() || self.zero_value
+    }
+}
+
+impl Display for NodeValidation {
+    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        write!(f, "Invalid node representation: ")?;
+
+        if let Some(((start_row, start_column), (end_row, end_column))) = self.start_after_end {
+            write!(
+                f,
+                "Start position ({start_row}, {start_column}) is after end position ({end_row}, \
+                 {end_column}); "
+            )?;
+        }
+
+        if self.zero_value {
+            write!(f, "Zero values are not allowed")?;
+        }
+
+        Ok(())
+    }
+}
+
+fn deserialize_vulnerable_node<'de, D: Deserializer<'de>>(
+    deserializer: D,
+) -> StdResult<Vec<VulnerableNode>, D::Error> {
+    let nodes = Vec::<VulnerableNode>::deserialize(deserializer)?;
+    nodes
+        .into_iter()
+        .map(|node| {
+            let validation = NodeValidation::new(&node);
+            if validation.is_error() {
+                return Err(D::Error::custom(validation.to_string()));
+            }
+
+            let start_row = node.start_row();
+            let start_column = node.start_column();
+            let end_row = node.end_row();
+            let end_column = node.end_column();
+
+            Ok(VulnerableNode::new(
+                node.package(),
+                node.module(),
+                start_row.saturating_sub(1),
+                start_column.saturating_sub(1),
+                end_row.saturating_sub(1),
+                end_column.saturating_sub(1),
+            ))
+        })
+        .collect::<StdResult<Vec<VulnerableNode>, D::Error>>()
+}
+
 impl ProjectDef {
     async fn sync(&self) -> Result<()> {
         let results = future::join_all(

vuln-reach-cli/tests/fixtures/jest-environment-jsdom-28.1.3.toml

@@ -0,0 +1,123 @@
+[[projects]]
+name = "jest-environment-jsdom:28.1.3"
+tarballs = "./tarballs"
+packages = [
+  { "name" = "@babel/code-frame", "version" = "7.22.13" },
+  { "name" = "ansi-styles", "version" = "3.2.1" },
+  { "name" = "chalk", "version" = "2.4.2" },
+  { "name" = "color-convert", "version" = "1.9.3" },
+  { "name" = "color-name", "version" = "1.1.3" },
+  { "name" = "escape-string-regexp", "version" = "1.0.5" },
+  { "name" = "has-flag", "version" = "3.0.0" },
+  { "name" = "supports-color", "version" = "5.5.0" },
+  { "name" = "@babel/helper-validator-identifier", "version" = "7.22.20" },
+  { "name" = "@babel/highlight", "version" = "7.22.20" },
+  { "name" = "ansi-styles", "version" = "3.2.1" },
+  { "name" = "chalk", "version" = "2.4.2" },
+  { "name" = "color-convert", "version" = "1.9.3" },
+  { "name" = "color-name", "version" = "1.1.3" },
+  { "name" = "escape-string-regexp", "version" = "1.0.5" },
+  { "name" = "has-flag", "version" = "3.0.0" },
+  { "name" = "supports-color", "version" = "5.5.0" },
+  { "name" = "@jest/environment", "version" = "28.1.3" },
+  { "name" = "@jest/fake-timers", "version" = "28.1.3" },
+  { "name" = "@jest/schemas", "version" = "28.1.3" },
+  { "name" = "@jest/types", "version" = "28.1.3" },
+  { "name" = "@sinclair/typebox", "version" = "0.24.51" },
+  { "name" = "@sinonjs/commons", "version" = "1.8.6" },
+  { "name" = "@sinonjs/fake-timers", "version" = "9.1.2" },
+  { "name" = "@tootallnate/once", "version" = "2.0.0" },
+  { "name" = "@types/istanbul-lib-coverage", "version" = "2.0.6" },
+  { "name" = "@types/istanbul-lib-report", "version" = "3.0.3" },
+  { "name" = "@types/istanbul-reports", "version" = "3.0.4" },
+  { "name" = "@types/jsdom", "version" = "16.2.15" },
+  { "name" = "@types/node", "version" = "20.9.1" },
+  { "name" = "@types/parse5", "version" = "6.0.3" },
+  { "name" = "@types/stack-utils", "version" = "2.0.3" },
+  { "name" = "@types/tough-cookie", "version" = "4.0.5" },
+  { "name" = "@types/yargs", "version" = "17.0.31" },
+  { "name" = "@types/yargs-parser", "version" = "21.0.3" },
+  { "name" = "abab", "version" = "2.0.6" },
+  { "name" = "acorn", "version" = "8.11.2" },
+  { "name" = "acorn-globals", "version" = "6.0.0" },
+  { "name" = "acorn", "version" = "7.4.1" },
+  { "name" = "acorn-walk", "version" = "7.2.0" },
+  { "name" = "agent-base", "version" = "6.0.2" },
+  { "name" = "ansi-regex", "version" = "5.0.1" },
+  { "name" = "ansi-styles", "version" = "4.3.0" },
+  { "name" = "asynckit", "version" = "0.4.0" },
+  { "name" = "braces", "version" = "3.0.2" },
+  { "name" = "browser-process-hrtime", "version" = "1.0.0" },
+  { "name" = "chalk", "version" = "4.1.2" },
+  { "name" = "ci-info", "version" = "3.9.0" },
+  { "name" = "color-convert", "version" = "2.0.1" },
+  { "name" = "color-name", "version" = "1.1.4" },
+  { "name" = "combined-stream", "version" = "1.0.8" },
+  { "name" = "cssom", "version" = "0.5.0" },
+  { "name" = "cssstyle", "version" = "2.3.0" },
+  { "name" = "cssom", "version" = "0.3.8" },
+  { "name" = "data-urls", "version" = "3.0.2" },
+  { "name" = "whatwg-url", "version" = "11.0.0" },
+  { "name" = "debug", "version" = "4.3.4" },
+  { "name" = "decimal.js", "version" = "10.4.3" },
+  { "name" = "delayed-stream", "version" = "1.0.0" },
+  { "name" = "domexception", "version" = "4.0.0" },
+  { "name" = "escape-string-regexp", "version" = "2.0.0" },
+  { "name" = "escodegen", "version" = "2.1.0" },
+  { "name" = "esprima", "version" = "4.0.1" },
+  { "name" = "estraverse", "version" = "5.3.0" },
+  { "name" = "esutils", "version" = "2.0.3" },
+  { "name" = "fill-range", "version" = "7.0.1" },
+  { "name" = "form-data", "version" = "4.0.0" },
+  { "name" = "graceful-fs", "version" = "4.2.11" },
+  { "name" = "has-flag", "version" = "4.0.0" },
+  { "name" = "html-encoding-sniffer", "version" = "3.0.0" },
+  { "name" = "http-proxy-agent", "version" = "5.0.0" },
+  { "name" = "https-proxy-agent", "version" = "5.0.1" },
+  { "name" = "iconv-lite", "version" = "0.6.3" },
+  { "name" = "is-number", "version" = "7.0.0" },
+  { "name" = "is-potential-custom-element-name", "version" = "1.0.1" },
+  { "name" = "jest-environment-jsdom", "version" = "28.1.3" },
+  { "name" = "jest-message-util", "version" = "28.1.3" },
+  { "name" = "jest-mock", "version" = "28.1.3" },
+  { "name" = "jest-util", "version" = "28.1.3" },
+  { "name" = "js-tokens", "version" = "4.0.0" },
+  { "name" = "jsdom", "version" = "19.0.0" },
+  { "name" = "micromatch", "version" = "4.0.5" },
+  { "name" = "mime-db", "version" = "1.52.0" },
+  { "name" = "mime-types", "version" = "2.1.35" },
+  { "name" = "ms", "version" = "2.1.2" },
+  { "name" = "nwsapi", "version" = "2.2.7" },
+  { "name" = "parse5", "version" = "6.0.1" },
+  { "name" = "picomatch", "version" = "2.3.1" },
+  { "name" = "pretty-format", "version" = "28.1.3" },
+  { "name" = "ansi-styles", "version" = "5.2.0" },
+  { "name" = "psl", "version" = "1.9.0" },
+  { "name" = "punycode", "version" = "2.3.1" },
+  { "name" = "react-is", "version" = "18.2.0" },
+  { "name" = "safer-buffer", "version" = "2.1.2" },
+  { "name" = "saxes", "version" = "5.0.1" },
+  { "name" = "slash", "version" = "3.0.0" },
+  { "name" = "source-map", "version" = "0.6.1" },
+  { "name" = "stack-utils", "version" = "2.0.6" },
+  { "name" = "supports-color", "version" = "7.2.0" },
+  { "name" = "symbol-tree", "version" = "3.2.4" },
+  { "name" = "to-regex-range", "version" = "5.0.1" },
+  { "name" = "tough-cookie", "version" = "4.0.0" },
+  { "name" = "tr46", "version" = "3.0.0" },
+  { "name" = "type-detect", "version" = "4.0.8" },
+  { "name" = "undici-types", "version" = "5.26.5" },
+  { "name" = "universalify", "version" = "0.1.2" },
+  { "name" = "w3c-hr-time", "version" = "1.0.2" },
+  { "name" = "w3c-xmlserializer", "version" = "3.0.0" },
+  { "name" = "webidl-conversions", "version" = "7.0.0" },
+  { "name" = "whatwg-encoding", "version" = "2.0.0" },
+  { "name" = "whatwg-mimetype", "version" = "3.0.0" },
+  { "name" = "whatwg-url", "version" = "10.0.0" },
+  { "name" = "ws", "version" = "8.14.2" },
+  { "name" = "xml-name-validator", "version" = "4.0.0" },
+  { "name" = "xmlchars", "version" = "2.2.0" }
+]
+vuln = [
+  { package = "tough-cookie", module = "lib/memstore.js", start_row = 111, start_column = 32, end_row = 111, end_column = 34 }
+]

vuln-reach/Cargo.toml

@@ -7,7 +7,7 @@ authors = ["Phylum, Inc. <engineering@phylum.io>"]
 repository = "https://github.com/phylum-dev/vuln-reach"
 documentation = "https://docs.rs/vuln-reach"
 license-file = "../LICENSE"
-rust-version = "1.68"
+rust-version = "1.70"
 readme = "../README.md"
 
 [dependencies]

vuln-reach/src/javascript/lang/accesses.rs

@@ -260,6 +260,14 @@ impl<'a> AccessGraph<'a> {
             }
             visited_nodes.insert(node);
 
+            // Ensure that the node is of kind identifier.
+            if node.kind() != "identifier" {
+                return Err(Error::Generic(format!(
+                    "Node {node:?} of kind {} is not an identifier",
+                    node.kind()
+                )));
+            }
+
             // Retrieve the access scope of the current node, which must exist for all
             // identifiers.
             let access = access_scopes.get(&node).copied().ok_or_else(|| {

vuln-reach/src/javascript/module/mod.rs

@@ -444,9 +444,10 @@ mod tests {
         assert!(paths.len() == 1, "Wrong number of paths found");
 
         let Some(PathToExport::SideEffect { name, access_path, effect_node }) =
-            paths.into_iter().next() else {
-                panic!("Path found is not to a side effect")
-            };
+            paths.into_iter().next()
+        else {
+            panic!("Path found is not to a side effect")
+        };
 
         assert_eq!(name, "foo", "Wrong side effect name {name}");
         assert_eq!(

vuln-reach/src/javascript/package/reachability.rs

@@ -37,11 +37,11 @@ fn identify_import_nodes<'a, R: ModuleResolver>(
 
             esm.into_iter()
                 .filter(|esm_import| {
-                    let Ok(resolved_spec) = package
-                        .resolver()
-                        .resolve_relative(esm_import.source(), dependent_spec) else {
-                            return false;
-                        };
+                    let Ok(resolved_spec) =
+                        package.resolver().resolve_relative(esm_import.source(), dependent_spec)
+                    else {
+                        return false;
+                    };
 
                     resolved_spec == imported_spec_abs
                 })
@@ -57,11 +57,11 @@ fn identify_import_nodes<'a, R: ModuleResolver>(
         Imports::CommonJs(cjs) => cjs
             .into_iter()
             .filter(|&cjs_import| {
-                let Ok(resolved_spec) = package
-                    .resolver()
-                    .resolve_relative(cjs_import.source(), dependent_spec) else {
-                        return false;
-                    };
+                let Ok(resolved_spec) =
+                    package.resolver().resolve_relative(cjs_import.source(), dependent_spec)
+                else {
+                    return false;
+                };
 
                 resolve_path(resolved_spec, |spec| spec == imported_spec_abs).is_some()
             })
@@ -142,15 +142,12 @@ fn compute_paths_from_export_to_modules<'a, R: ModuleResolver>(
 
         // Find paths from the imported dependency nodes to the dependent export
         // nodes.
-        let dependent_paths_from_imports_to_exports = dependent_imports.into_iter().fold(
-            Ok::<_, Error>(Vec::new()),
-            |out, import_node| {
-                let mut out = out?;
+        let dependent_paths_from_imports_to_exports =
+            dependent_imports.into_iter().try_fold(Vec::new(), |mut out, import_node| {
                 out.extend(dependent_module.paths_to_exports(import_node)?);
 
-                Ok(out)
-            },
-        )?;
+                Ok::<_, Error>(out)
+            })?;
 
         let dependent_dependents = package.cache().dependents_of(dependent).collect::<Vec<_>>();