Merge pull request #738 from DataDog/jf/K9VULN-1999

[K9VULN-1999] Fix and improve usefulness of JavaScript stack trace

Author: jasonforal

crates/static-analysis-kernel/src/analysis/ddsa_lib/common.rs

@@ -45,36 +45,70 @@ pub enum DDSAJsRuntimeError {
 #[derive(Debug, Clone)]
 pub struct JsError {
     pub message: String,
-    pub stack_trace: Vec<String>,
+    pub stack_trace_frames: Vec<JsCallSite>,
+}
+
+#[derive(Debug, Clone, Eq, PartialEq)]
+pub struct JsCallSite {
+    pub type_name: Option<String>,
+    pub function_name: Option<String>,
+    pub method_name: Option<String>,
+    pub file_name: Option<String>,
+    pub line_number: usize,
+    pub column_number: usize,
+    /// The string representation of this call site, as formatted by v8 (see [stack trace format] documentation).
+    ///
+    /// [stack trace format]: https://v8.dev/docs/stack-trace-api#appendix%3A-stack-trace-format
+    v8_formatted_string: String,
+}
+
+impl Display for JsCallSite {
+    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+        write!(f, "{}", self.v8_formatted_string)
+    }
 }
 
 impl From<deno_core::error::JsError> for JsError {
     fn from(value: deno_core::error::JsError) -> Self {
-        // `deno_core` formats the `deno_core::error::JsError::stack` such that the first line contains
-        // the error message, and all subsequent lines are a human-friendly stack trace. For example:
-        // ```
-        // TypeError: Cannot read properties of undefined (reading 'someProp')
-        //   at SomeClass.someOtherFunction (ext:ddsa_lib/someModule:572:29)
-        //   at SomeClass.someFunction (ext:ddsa_lib/someModule:157:22)
-        //   at <anonymous>:1:13
-        // ```
-        let stack_trace = value.stack.map_or(vec![], |str| {
+        // v8 formats an exception error message with a stable format described at
+        // https://v8.dev/docs/stack-trace-api#appendix%3A-stack-trace-format. `deno_core` splits
+        // this error message by line and assigns the resulting `Vec<String>` to the `stack` field.
+        // At the same time, it constructs individual JsStackFrame structs with a typed breakdown
+        // of v8's string format.
+        //
+        // The first line is the exception message (this is implicitly tested in the
+        // `v8_exception_stack_trace_parsing` test, however it's listed here to communicate the expectation)
+        // This uses a fuzzy check because there might be qualifiers like "Uncaught" before the message.
+        debug_assert!(value
+            .exception_message
+            .contains(value.stack.as_ref().unwrap().lines().next().unwrap()),);
+        // ...so it is skipped to only collect the call sites.
+        let stack_trace_frames = value.stack.map_or(vec![], |str| {
             str.lines()
                 .skip(1)
-                .map(ToString::to_string)
+                .zip(value.frames)
+                .map(|(display_str, frame)| JsCallSite {
+                    type_name: frame.type_name,
+                    function_name: frame.function_name,
+                    method_name: frame.method_name,
+                    file_name: frame.file_name,
+                    line_number: frame.line_number.unwrap_or_default() as usize,
+                    column_number: frame.column_number.unwrap_or_default() as usize,
+                    v8_formatted_string: display_str.to_string(),
+                })
                 .collect::<Vec<_>>()
         });
         Self {
             message: value.exception_message,
-            stack_trace,
+            stack_trace_frames,
         }
     }
 }
 
 impl Display for JsError {
     fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
         writeln!(f, "{}", self.message)?;
-        for line in &self.stack_trace {
+        for line in &self.stack_trace_frames {
             writeln!(f, "{}", line)?;
         }
         Ok(())
@@ -158,10 +192,8 @@ pub fn v8_interned<'s>(scope: &mut HandleScope<'s>, str: &str) -> v8::Local<'s,
 }
 
 /// Creates a [`Normal`](v8::string::NewStringType::Normal) v8 string, which always allocates memory
-/// to create the string, even if it has been seen by the runtime before.
-///
-/// # Panics
-/// Panics if `str` is longer than the v8 string length limit.
+/// to create the string, even if it has been seen by the runtime before. An empty string is
+/// returned if the string is larger than the v8 string length limit.
 #[inline(always)]
 pub fn v8_string<'s>(scope: &mut HandleScope<'s>, str: &str) -> v8::Local<'s, v8::String> {
     v8::String::new_from_utf8(scope, str.as_bytes(), v8::NewStringType::Normal)
@@ -298,14 +330,15 @@ pub fn set_key_value<G>(
     v8_object.set(scope, v8_key.into(), v8_value);
 }
 
-/// Creates a `v8::Global` [`UnboundScript`](v8::UnboundScript) from the given code.
+/// Creates a `v8::Global` [`UnboundScript`](v8::UnboundScript) from the given code and origin.
 pub fn compile_script(
     scope: &mut HandleScope,
     code: &str,
+    origin: Option<&v8::ScriptOrigin>,
 ) -> Result<v8::Global<v8::UnboundScript>, DDSAJsRuntimeError> {
     let code_str = v8_string(scope, code);
     let tc_scope = &mut v8::TryCatch::new(scope);
-    let script_result = v8::Script::compile(tc_scope, code_str, None);
+    let script_result = v8::Script::compile(tc_scope, code_str, origin);
 
     let script = script_result.ok_or_else(|| {
         let exception = tc_scope
@@ -347,7 +380,7 @@ pub fn swallow_v8_error<F: FnOnce() -> T, T>(f: F) -> T {
 #[cfg(test)]
 mod tests {
     use crate::analysis::ddsa_lib::common::{
-        compile_script, v8_interned, v8_string, DDSAJsRuntimeError,
+        compile_script, v8_interned, v8_string, DDSAJsRuntimeError, JsCallSite,
     };
     use crate::analysis::ddsa_lib::runtime::inner_make_deno_core_runtime;
     use crate::analysis::ddsa_lib::test_utils::{cfg_test_v8, try_execute};
@@ -358,7 +391,7 @@ mod tests {
         let invalid_js = r#"
 const invalidSyntax = const;
 "#;
-        let err = compile_script(&mut runtime.v8_handle_scope(), invalid_js).unwrap_err();
+        let err = compile_script(&mut runtime.v8_handle_scope(), invalid_js, None).unwrap_err();
         let DDSAJsRuntimeError::Interpreter { reason } = err else {
             panic!("error variant should be `Interpreter`");
         };
@@ -371,7 +404,7 @@ const invalidSyntax = const;
         let invalid_js = r#"
 const validSyntax = 123;
 "#;
-        assert!(compile_script(&mut runtime.v8_handle_scope(), invalid_js).is_ok());
+        assert!(compile_script(&mut runtime.v8_handle_scope(), invalid_js, None).is_ok());
     }
 
     /// Tests that [`inner_make_deno_core_runtime`]  can modify the default v8::Context for
@@ -395,7 +428,13 @@ const validSyntax = 123;
             None,
         );
         let value = try_execute(&mut runtime.handle_scope(), code).unwrap_err();
-        assert_eq!(value, "ReferenceError: Map is not defined".to_string());
+        let DDSAJsRuntimeError::Execution { error: js_error } = value else {
+            panic!("should be this variant")
+        };
+        assert_eq!(
+            js_error.message,
+            "Uncaught ReferenceError: Map is not defined"
+        );
     }
 
     // One byte characters
@@ -444,4 +483,47 @@ const validSyntax = 123;
             assert!(result.is_err());
         }
     }
+
+    /// The v8 exception stack trace string is properly parsed
+    #[test]
+    fn v8_exception_stack_trace_parsing() {
+        let mut runtime = cfg_test_v8().deno_core_rt();
+        // language=javascript
+        let code = "\
+function someFunction() {
+    someFunctionThatDoesntExist();
+}
+someFunction();
+";
+        let DDSAJsRuntimeError::Execution { error: js_error } =
+            try_execute(&mut runtime.handle_scope(), code).unwrap_err()
+        else {
+            panic!("should be this variant")
+        };
+        let expected = vec![
+            JsCallSite {
+                type_name: None,
+                function_name: Some("someFunction".to_string()),
+                method_name: Some("someFunction".to_string()),
+                file_name: None,
+                line_number: 2,
+                column_number: 5,
+                v8_formatted_string: "    at someFunction (<anonymous>:2:5)".to_string(),
+            },
+            JsCallSite {
+                type_name: None,
+                function_name: None,
+                method_name: None,
+                file_name: None,
+                line_number: 4,
+                column_number: 1,
+                v8_formatted_string: "    at <anonymous>:4:1".to_string(),
+            },
+        ];
+        assert_eq!(js_error.stack_trace_frames, expected);
+        assert_eq!(
+            js_error.message,
+            "Uncaught ReferenceError: someFunctionThatDoesntExist is not defined"
+        );
+    }
 }

crates/static-analysis-kernel/src/analysis/ddsa_lib/js/capture.rs

@@ -139,7 +139,7 @@ assert(CAPTURE.name === "alpha", "name was incorrect");
 assert(CAPTURE.nodeId === 16, "nodeId was incorrect");
 "#;
         let result = try_execute(scope, code).map(|v| v.to_rust_string_lossy(scope));
-        assert_eq!(result, Ok("undefined".to_string()));
+        assert_eq!(result.unwrap(), "undefined");
     }
 
     /// Verifies the object shape of a `MultiCapture`.
@@ -158,7 +158,7 @@ assert(Array.isArray(CAPTURE.nodeIds), "nodeIds had wrong type");
 assert(CAPTURE.nodeIds.join(",") === "16,32,48", "nodeIds were incorrect");
 "#;
         let result = try_execute(scope, code).map(|v| v.to_rust_string_lossy(scope));
-        assert_eq!(result, Ok("undefined".to_string()));
+        assert_eq!(result.unwrap(), "undefined");
 
         // Single capture within an array should still be an array
         let capture = template.new_instance(scope, "bravo", &[16]);
@@ -168,6 +168,6 @@ assert(Array.isArray(CAPTURE.nodeIds), "nodeIds had wrong type");
 assert(CAPTURE.nodeIds.join(",") === "16", "nodeIds were incorrect");
 "#;
         let result = try_execute(scope, code).map(|v| v.to_rust_string_lossy(scope));
-        assert_eq!(result, Ok("undefined".to_string()));
+        assert_eq!(result.unwrap(), "undefined");
     }
 }

crates/static-analysis-kernel/src/analysis/ddsa_lib/js/flow/graph.rs

@@ -802,7 +802,7 @@ graph.adjacencyList;
         js_script += "\
 [graph.adjacencyList, transpose(graph.adjacencyList)];
 ";
-        let js_script = compile_script(&mut rt.v8_handle_scope(), &js_script).unwrap();
+        let js_script = compile_script(&mut rt.v8_handle_scope(), &js_script, None).unwrap();
 
         let vertex_transformer = |node: &dot_structures::Node| -> Option<dot_structures::Node> {
             let vid = id_str(&node.id.0).parse::<u32>().unwrap();
@@ -1039,7 +1039,7 @@ const vidPaths = _findTaintFlows(adjList, cst(1), false).map((flow) => {
 const serialized = vidPaths.map((flow) => DDSA_Console.stringify(flow)).join('\\n');
 serialized;
 ";
-        let js_code = compile_script(&mut rt.v8_handle_scope(), js_code).unwrap();
+        let js_code = compile_script(&mut rt.v8_handle_scope(), js_code, None).unwrap();
         let res = rt
             .scoped_execute(&js_code, |sc, value| value.to_rust_string_lossy(sc), None)
             .unwrap();
@@ -1062,7 +1062,7 @@ serialized;
 const flow = new TaintFlow([], false);
 flow instanceof Array;
 ";
-        let js_code = compile_script(&mut rt.v8_handle_scope(), js_code).unwrap();
+        let js_code = compile_script(&mut rt.v8_handle_scope(), js_code, None).unwrap();
         let has_array_proto = rt
             .scoped_execute(&js_code, |_, value| value.is_true(), None)
             .unwrap();

crates/static-analysis-kernel/src/analysis/ddsa_lib/js/flow/java.rs

@@ -177,7 +177,7 @@ __ddsaPrivate__.graphToDOT(methodFlow.graph, \"cst_v8_full\");
 ",
                 CLASS_NAME, method_decl_id
             );
-            let script = compile_script(&mut rt.v8_handle_scope(), &script).unwrap();
+            let script = compile_script(&mut rt.v8_handle_scope(), &script, None).unwrap();
             let full_str = rt
                 .scoped_execute(&script, |sc, val| val.to_rust_string_lossy(sc), None)
                 .unwrap();
@@ -289,7 +289,7 @@ public class TestClass {
                 .unwrap_or("undefined".to_string());
 
             let script = format!("{}.findContainingMethod(getNode({}))?.id;", CLASS_NAME, nid);
-            let script = compile_script(&mut rt.v8_handle_scope(), &script).unwrap();
+            let script = compile_script(&mut rt.v8_handle_scope(), &script, None).unwrap();
             let exe_result = rt.scoped_execute(&script, |sc, v| v.to_rust_string_lossy(sc), None);
             assert_eq!(exe_result.unwrap(), expected);
         }
@@ -400,7 +400,7 @@ for (const flow of [sourceFlows[0], sinkFlows[0]]) {{
 serialized;
 "#,
         );
-        let script = compile_script(&mut rt.v8_handle_scope(), &script).unwrap();
+        let script = compile_script(&mut rt.v8_handle_scope(), &script, None).unwrap();
         let res = rt
             .scoped_execute(&script, |sc, value| value.to_rust_string_lossy(sc), None)
             .unwrap();
@@ -456,7 +456,7 @@ const v = Violation.new("flow violation", sourceFlows[0]);
 v;
 "#,
         );
-        let script = compile_script(&mut rt.v8_handle_scope(), &script).unwrap();
+        let script = compile_script(&mut rt.v8_handle_scope(), &script, None).unwrap();
         let violation = rt
             .scoped_execute(
                 &script,

crates/static-analysis-kernel/src/analysis/ddsa_lib/js/query_match.rs

@@ -158,7 +158,7 @@ assert(cap_node_ids.length === 1, "array must have exactly one elements");
 assert(cap_node_ids[0] === 10, "nodeId was incorrect");
 "#;
         let result = try_execute(scope, code).map(|v| v.to_rust_string_lossy(scope));
-        assert_eq!(result, Ok("undefined".to_string()));
+        assert_eq!(result.unwrap(), "undefined");
 
         // A capture name that isn't present should return undefined
         let code = r#"QUERY_MATCH._getManyIds("missing_from_captures");"#;
@@ -185,7 +185,7 @@ assert(cap_node_ids.length === 3, "array must have exactly three elements");
 assert(cap_node_ids.join(",") === "10,20,30", "nodeIds were incorrect");
 "#;
         let result = try_execute(scope, code).map(|v| v.to_rust_string_lossy(scope));
-        assert_eq!(result, Ok("undefined".to_string()));
+        assert_eq!(result.unwrap(), "undefined");
     }
 
     /// Tests that a call to `_getManyIds` on a `MultiCapture` that happens to have only a single node id
@@ -207,7 +207,7 @@ assert(cap_node_ids.length === 1, "array must have exactly one elements");
 assert(cap_node_ids[0] === 10, "nodeId was incorrect");
 "#;
         let result = try_execute(scope, code).map(|v| v.to_rust_string_lossy(scope));
-        assert_eq!(result, Ok("undefined".to_string()));
+        assert_eq!(result.unwrap(), "undefined");
     }
 
     /// Tests that the [`RustConverter`] creates a JS class instance with `undefined` passed in as the captures array (`_captures`).
@@ -244,7 +244,7 @@ assert(cap_node_ids[0] === 10, "nodeId was incorrect");
 
         let code = r#"assert(QUERY_MATCH.get("cap_name").id === 10);"#;
         let result = try_execute(scope, code).map(|v| v.to_rust_string_lossy(scope));
-        assert_eq!(result, Ok("undefined".to_string()));
+        assert_eq!(result.unwrap(), "undefined");
     }
 
     /// Tests that `getMany` properly retrieves nodes from the `TsNodeBridge`.
@@ -270,6 +270,6 @@ assert(stubNodes[0].id === 10);
 assert(stubNodes[1].id === 20);
 "#;
         let result = try_execute(scope, code).map(|v| v.to_rust_string_lossy(scope));
-        assert_eq!(result, Ok("undefined".to_string()));
+        assert_eq!(result.unwrap(), "undefined");
     }
 }