diff --git a/crates/cli/src/csv.rs b/crates/cli/src/csv.rs index b3da79d4..0320d0a8 100644 --- a/crates/cli/src/csv.rs +++ b/crates/cli/src/csv.rs @@ -85,6 +85,7 @@ mod tests { fixes: vec![], taint_flow: None, is_suppressed: false, + enclosing_function: None, }], errors: vec![], execution_error: None, diff --git a/crates/cli/src/file_utils.rs b/crates/cli/src/file_utils.rs index 224aa375..46c698c0 100644 --- a/crates/cli/src/file_utils.rs +++ b/crates/cli/src/file_utils.rs @@ -453,6 +453,7 @@ mod tests { fixes: vec![], taint_flow: None, is_suppressed: false, + enclosing_function: None, }; let directory_string = d.into_os_string().into_string().unwrap(); let fingerprint = get_fingerprint_for_violation( @@ -498,6 +499,7 @@ mod tests { fixes: vec![], taint_flow: Some(vec![region0, region1]), is_suppressed: false, + enclosing_function: None, }; let fingerprint = get_fingerprint_for_violation( "taint_flow_rule".to_string(), @@ -529,6 +531,7 @@ mod tests { fixes: vec![], taint_flow: None, is_suppressed: false, + enclosing_function: None, }; let directory_string = d.into_os_string().into_string().unwrap(); diff --git a/crates/cli/src/rule_utils.rs b/crates/cli/src/rule_utils.rs index 3b253935..1e3c109f 100644 --- a/crates/cli/src/rule_utils.rs +++ b/crates/cli/src/rule_utils.rs @@ -81,6 +81,7 @@ pub fn convert_secret_result_to_rule_result(secret_result: &SecretResult) -> Rul fixes: vec![], taint_flow: None, is_suppressed: v.is_suppressed, + enclosing_function: None, }) .collect(), } @@ -163,6 +164,7 @@ mod tests { fixes: vec![], taint_flow: None, is_suppressed: false, + enclosing_function: None, }, Violation { start: Position { line: 10, col: 12 }, @@ -173,6 +175,7 @@ mod tests { fixes: vec![], taint_flow: None, is_suppressed: false, + enclosing_function: None, }, Violation { start: Position { line: 10, col: 12 }, @@ -183,6 +186,7 @@ mod tests { fixes: vec![], taint_flow: None, is_suppressed: false, + enclosing_function: None, }, ], errors: vec![], @@ -227,6 +231,7 @@ mod tests { fixes: vec![], taint_flow: None, is_suppressed: false, + enclosing_function: None, }, Violation { start: Position { line: 20, col: 1 }, @@ -237,6 +242,7 @@ mod tests { fixes: vec![], taint_flow: None, is_suppressed: true, + enclosing_function: None, }, ], errors: vec![], diff --git a/crates/cli/src/sarif/sarif_utils.rs b/crates/cli/src/sarif/sarif_utils.rs index 22cf71f1..475c8637 100644 --- a/crates/cli/src/sarif/sarif_utils.rs +++ b/crates/cli/src/sarif/sarif_utils.rs @@ -22,9 +22,10 @@ use secrets::model::secret_result::{SecretResult, SecretValidationStatus, Valida use secrets::model::secret_rule::SecretRule; use serde_sarif::sarif::{ self, Artifact, ArtifactBuilder, ArtifactChangeBuilder, ArtifactLocationBuilder, FixBuilder, - LocationBuilder, MessageBuilder, PhysicalLocationBuilder, PropertyBagBuilder, RegionBuilder, - Replacement, ReportingDescriptor, Result as SarifResult, ResultBuilder, RunBuilder, Sarif, - SarifBuilder, SuppressionBuilder, Tool, ToolBuilder, ToolComponent, ToolComponentBuilder, + LocationBuilder, LogicalLocationBuilder, MessageBuilder, PhysicalLocationBuilder, + PropertyBagBuilder, RegionBuilder, Replacement, ReportingDescriptor, Result as SarifResult, + ResultBuilder, RunBuilder, Sarif, SarifBuilder, SuppressionBuilder, Tool, ToolBuilder, + ToolComponent, ToolComponentBuilder, }; use crate::file_utils::get_fingerprint_for_violation; @@ -196,6 +197,7 @@ impl SarifRuleResult { fixes: vec![], taint_flow: None, is_suppressed: r.is_suppressed, + enclosing_function: None, }, r.validation_status.clone(), ) @@ -638,21 +640,27 @@ fn generate_results( .map(move |sarif_violation| { let violation = sarif_violation.get_violation(); // if we find the rule for this violation, get the id, level and category - let location = LocationBuilder::default() - .physical_location( - PhysicalLocationBuilder::default() - .artifact_location(artifact_loc.clone()) - .region( - RegionBuilder::default() - .start_line(violation.start.line) - .start_column(violation.start.col) - .end_line(violation.end.line) - .end_column(violation.end.col) - .build()?, - ) - .build()?, - ) - .build()?; + let mut location_builder = LocationBuilder::default(); + location_builder.physical_location( + PhysicalLocationBuilder::default() + .artifact_location(artifact_loc.clone()) + .region( + RegionBuilder::default() + .start_line(violation.start.line) + .start_column(violation.start.col) + .end_line(violation.end.line) + .end_column(violation.end.col) + .build()?, + ) + .build()?, + ); + if let Some(ref ef) = violation.enclosing_function { + location_builder.logical_locations(vec![LogicalLocationBuilder::default() + .name(ef.name.clone()) + .kind("function".to_string()) + .build()?]); + } + let location = location_builder.build()?; let fixes: Vec = violation .fixes @@ -965,7 +973,7 @@ mod tests { use super::*; use assert_json_diff::{assert_json_eq, assert_json_include}; use common::model::position::{Position, PositionBuilder, Region}; - use kernel::model::violation::{Fix, Violation}; + use kernel::model::violation::{EnclosingFunction, Fix, Violation}; use kernel::model::{ common::Language, rule::{RuleBuilder, RuleCategory, RuleResultBuilder, RuleSeverity, RuleType}, @@ -996,6 +1004,7 @@ mod tests { fixes: vec![], taint_flow: None, is_suppressed: false, + enclosing_function: None, })); // good location in the violation location and no fixes @@ -1008,6 +1017,7 @@ mod tests { fixes: vec![], taint_flow: None, is_suppressed: false, + enclosing_function: None, })); // bad location in the fixes location @@ -1028,6 +1038,7 @@ mod tests { }], taint_flow: None, is_suppressed: false, + enclosing_function: None, })); // good location everywhere @@ -1048,6 +1059,7 @@ mod tests { }], taint_flow: None, is_suppressed: false, + enclosing_function: None, })); } @@ -1116,6 +1128,7 @@ mod tests { fixes: vec![], taint_flow: Some(vec![region0, region1, region2]), is_suppressed: false, + enclosing_function: None, }; let rule_result_single_region = RuleResultBuilder::default() @@ -1298,6 +1311,106 @@ mod tests { assert!(validate_data(&sarif_report_to_string)); } + #[test] + fn test_generate_sarif_report_logical_location() { + let rule = RuleBuilder::default() + .name("my-rule".to_string()) + .description_base64(None) + .language(Language::Python) + .checksum("abc".to_string()) + .pattern(None) + .tree_sitter_query_base64(None) + .category(RuleCategory::BestPractices) + .code_base64("Zm9v".to_string()) + .short_description_base64(None) + .entity_checked(None) + .rule_type(RuleType::TreeSitterQuery) + .severity(RuleSeverity::Error) + .cwe(None) + .arguments(vec![]) + .tests(vec![]) + .is_testing(false) + .documentation_url(None) + .build() + .unwrap(); + + let violation_with_method = Violation { + start: Position { line: 10, col: 1 }, + end: Position { line: 10, col: 20 }, + message: "some violation".to_string(), + severity: RuleSeverity::Error, + category: RuleCategory::BestPractices, + fixes: vec![], + taint_flow: None, + is_suppressed: false, + enclosing_function: Some(EnclosingFunction { + name: "my_method".to_string(), + }), + }; + let violation_without_method = Violation { + start: Position { line: 20, col: 1 }, + end: Position { line: 20, col: 5 }, + message: "another violation".to_string(), + severity: RuleSeverity::Error, + category: RuleCategory::BestPractices, + fixes: vec![], + taint_flow: None, + is_suppressed: false, + enclosing_function: None, + }; + + let rule_result = RuleResult { + rule_name: "my-rule".to_string(), + filename: "myfile.py".to_string(), + violations: vec![violation_with_method, violation_without_method], + errors: vec![], + execution_error: None, + output: None, + execution_time_ms: 0, + parsing_time_ms: 0, + query_node_time_ms: 0, + }; + + let sarif_report = generate_sarif_report( + &[rule.into()], + &[rule_result.try_into().unwrap()], + &"mydir".to_string(), + SarifReportMetadata { + add_git_info: false, + debug: false, + config_digest: "abc".to_string(), + diff_aware_parameters: None, + execution_time_secs: 0, + }, + &Default::default(), + ) + .expect("generate sarif report"); + + let sarif_json = serde_json::to_value(sarif_report).unwrap(); + + // Violation with enclosing_function: logicalLocations must carry name and kind. + let logical_locations = sarif_json + .pointer("/runs/0/results/0/locations/0/logicalLocations") + .expect("logicalLocations should be present when enclosing_function is set"); + assert_json_include!( + actual: logical_locations, + expected: serde_json::json!([{ + "kind": "function", + "name": "my_method" + }]) + ); + + // Violation without enclosing_function: no logicalLocations key at all. + let no_logical_locations = + sarif_json.pointer("/runs/0/results/1/locations/0/logicalLocations"); + assert!( + no_logical_locations.is_none(), + "logicalLocations should be absent when enclosing_function is None" + ); + + assert!(validate_data(&sarif_json)); + } + // Ensure that diff-aware scanning information are correctly surfaced #[test] fn test_generate_sarif_diff_aware_scanning() { @@ -1960,6 +2073,7 @@ mod tests { fixes: vec![], taint_flow: None, is_suppressed: false, + enclosing_function: None, }; let rr = RuleResult { rule_name: format!("rule-{idx}"), @@ -2053,6 +2167,7 @@ mod tests { fixes: vec![], taint_flow: None, is_suppressed: false, + enclosing_function: None, }; let rule_results = [TEST_FILE_PATH, NON_TEST_FILE_PATH] .into_iter() diff --git a/crates/static-analysis-kernel/src/analysis/ddsa_lib/js/violation.rs b/crates/static-analysis-kernel/src/analysis/ddsa_lib/js/violation.rs index 5dcb41e6..d0bb1a28 100644 --- a/crates/static-analysis-kernel/src/analysis/ddsa_lib/js/violation.rs +++ b/crates/static-analysis-kernel/src/analysis/ddsa_lib/js/violation.rs @@ -55,6 +55,7 @@ impl Violation { fixes, taint_flow, is_suppressed: false, + enclosing_function: None, } } } diff --git a/crates/static-analysis-kernel/src/analysis/ddsa_lib/runtime.rs b/crates/static-analysis-kernel/src/analysis/ddsa_lib/runtime.rs index ecbacda0..06e29c86 100644 --- a/crates/static-analysis-kernel/src/analysis/ddsa_lib/runtime.rs +++ b/crates/static-analysis-kernel/src/analysis/ddsa_lib/runtime.rs @@ -200,6 +200,18 @@ impl JsRuntime { let violations = js_violations .into_iter() .map(|v| v.into_violation(rule.severity, rule.category)) + .map(|mut v| { + v.enclosing_function = analysis::languages::find_enclosing_function_with_tree( + source_text.as_ref(), + source_tree.as_ref(), + v.start.line, + v.start.col, + v.end.line, + v.end.col, + rule.language, + ); + v + }) .collect::>(); let timing = ExecutionTimingCompat { diff --git a/crates/static-analysis-kernel/src/analysis/languages.rs b/crates/static-analysis-kernel/src/analysis/languages.rs index 9a4a724b..c6a2fee1 100644 --- a/crates/static-analysis-kernel/src/analysis/languages.rs +++ b/crates/static-analysis-kernel/src/analysis/languages.rs @@ -9,6 +9,98 @@ pub mod javascript; pub mod python; pub mod typescript; +use crate::model::common::Language; +use crate::model::violation::EnclosingFunction; + +/// Returns the enclosing function for the given source position, or `None` if the position +/// is not inside any named function or the language has no implementation. +/// +/// This function parses the source code from scratch. +/// If you already have a parsed tree, use [`find_enclosing_function_with_tree`]. +pub fn find_enclosing_function( + source_code: &str, + start_line: u32, + start_col: u32, + end_line: u32, + end_col: u32, + language: Language, +) -> Option { + match language { + Language::Java => java::methods::find_enclosing_function( + source_code, + start_line, + start_col, + end_line, + end_col, + ), + Language::Python + | Language::Go + | Language::JavaScript + | Language::TypeScript + | Language::Csharp + | Language::Dockerfile + | Language::Elixir + | Language::Json + | Language::Kotlin + | Language::Ruby + | Language::Rust + | Language::Swift + | Language::Terraform + | Language::Yaml + | Language::Starlark + | Language::Bash + | Language::PHP + | Language::Markdown + | Language::Apex + | Language::R + | Language::SQL => None, + } +} + +/// Returns the enclosing function for the given source position, reusing an already-parsed tree. +/// See [`find_enclosing_function`] for documentation. +pub fn find_enclosing_function_with_tree( + source_code: &str, + tree: &tree_sitter::Tree, + start_line: u32, + start_col: u32, + end_line: u32, + end_col: u32, + language: Language, +) -> Option { + match language { + Language::Java => java::methods::find_enclosing_function_with_tree( + source_code, + tree, + start_line, + start_col, + end_line, + end_col, + ), + Language::Python + | Language::Go + | Language::JavaScript + | Language::TypeScript + | Language::Csharp + | Language::Dockerfile + | Language::Elixir + | Language::Json + | Language::Kotlin + | Language::Ruby + | Language::Rust + | Language::Swift + | Language::Terraform + | Language::Yaml + | Language::Starlark + | Language::Bash + | Language::PHP + | Language::Markdown + | Language::Apex + | Language::R + | Language::SQL => None, + } +} + /// Returns the text that `node` spans. /// /// This is simply a wrapper around [`tree_sitter::Node::utf8_text`] diff --git a/crates/static-analysis-kernel/src/analysis/languages/java.rs b/crates/static-analysis-kernel/src/analysis/languages/java.rs index c42b51a1..5d613425 100644 --- a/crates/static-analysis-kernel/src/analysis/languages/java.rs +++ b/crates/static-analysis-kernel/src/analysis/languages/java.rs @@ -4,3 +4,4 @@ mod imports; pub use imports::*; +pub mod methods; diff --git a/crates/static-analysis-kernel/src/analysis/languages/java/methods.rs b/crates/static-analysis-kernel/src/analysis/languages/java/methods.rs new file mode 100644 index 00000000..1ddbdb17 --- /dev/null +++ b/crates/static-analysis-kernel/src/analysis/languages/java/methods.rs @@ -0,0 +1,173 @@ +// Unless explicitly stated otherwise all files in this repository are licensed under the Apache License, Version 2.0. +// This product includes software developed at Datadog (https://www.datadoghq.com/). +// Copyright 2024 Datadog, Inc. + +use crate::analysis::languages::ts_node_text; +use crate::analysis::tree_sitter::get_tree; +use crate::model::common::Language; +use crate::model::violation::EnclosingFunction; + +/// Returns the enclosing method or constructor for the given source position, or `None` if the +/// position is not inside any method. +/// +/// This function parses the source code from scratch. +/// If you already have a parsed tree, use [`find_enclosing_function_with_tree`]. +pub fn find_enclosing_function( + source_code: &str, + start_line: u32, + start_col: u32, + end_line: u32, + end_col: u32, +) -> Option { + get_tree(source_code, &Language::Java).and_then(|tree| { + find_enclosing_function_with_tree( + source_code, + &tree, + start_line, + start_col, + end_line, + end_col, + ) + }) +} + +/// Returns the enclosing method or constructor for the given source position. +pub fn find_enclosing_function_with_tree( + source_code: &str, + tree: &tree_sitter::Tree, + start_line: u32, + start_col: u32, + end_line: u32, + end_col: u32, +) -> Option { + let start = tree_sitter::Point { + row: start_line.saturating_sub(1) as usize, + column: start_col.saturating_sub(1) as usize, + }; + let end = tree_sitter::Point { + row: end_line.saturating_sub(1) as usize, + column: end_col.saturating_sub(1) as usize, + }; + let mut node = tree + .root_node() + .named_descendant_for_point_range(start, end)?; + loop { + match node.kind() { + "method_declaration" | "constructor_declaration" => { + let name = node + .child_by_field_name("name") + .map(|n| ts_node_text(source_code, n).to_owned())?; + return Some(EnclosingFunction { name }); + } + _ => {} + } + node = node.parent()?; + } +} + +#[cfg(test)] +mod tests { + use super::find_enclosing_function_with_tree; + use crate::analysis::tree_sitter::get_tree; + use crate::model::common::Language; + use crate::model::violation::EnclosingFunction; + + fn find(source: &str, line: u32, col: u32) -> Option { + let tree = get_tree(source, &Language::Java).unwrap(); + find_enclosing_function_with_tree(source, &tree, line, col, line, col) + } + + fn ef(name: &str) -> Option { + Some(EnclosingFunction { + name: name.to_string(), + }) + } + + #[test] + fn inside_method() { + let src = "\ +class Foo { + public void doSomething() { + int x = 1; + } +} +"; + assert_eq!(find(src, 3, 9), ef("doSomething")); + } + + #[test] + fn inside_constructor() { + let src = "\ +class Foo { + public Foo() { + this.x = 0; + } +} +"; + assert_eq!(find(src, 3, 9), ef("Foo")); + } + + #[test] + fn with_package() { + let src = "\ +package com.example; +class Foo { + public void doSomething() { + int x = 1; + } +} +"; + assert_eq!(find(src, 4, 9), ef("doSomething")); + } + + #[test] + fn with_params() { + let src = "\ +class Foo { + public void handle(String s, int n) { + int x = 1; + } +} +"; + assert_eq!(find(src, 3, 9), ef("handle")); + } + + #[test] + fn annotations_ignored() { + let src = "\ +class Foo { + @Override + public void doSomething() { + int x = 1; + } +} +"; + assert_eq!(find(src, 4, 9), ef("doSomething")); + } + + #[test] + fn top_level_field() { + let src = "\ +class Foo { + int x = 1; +} +"; + assert_eq!(find(src, 2, 9), None); + } + + // Lambdas are not named, so we report the nearest enclosing named method instead. + // Naming individual lambdas is not implemented. + #[test] + fn inside_lambda_reports_enclosing_method() { + let src = "\ +class Foo { + public void doWork() { + Runnable r = () -> { + int x = 1; + }; + } +} +"; + assert_eq!(find(src, 4, 13), ef("doWork")); + } +} diff --git a/crates/static-analysis-kernel/src/model/violation.rs b/crates/static-analysis-kernel/src/model/violation.rs index 574a7a15..b251d85b 100644 --- a/crates/static-analysis-kernel/src/model/violation.rs +++ b/crates/static-analysis-kernel/src/model/violation.rs @@ -4,6 +4,13 @@ use common::model::position::{Position, Region}; use derive_builder::Builder; use serde::{Deserialize, Serialize}; +/// The function or method that encloses a violation. +#[derive(Deserialize, Debug, Serialize, Clone, PartialEq)] +pub struct EnclosingFunction { + /// Simple identifier (e.g. `handle`, `doSomething`). + pub name: String, +} + #[derive(Copy, Clone, Deserialize, Debug, Serialize, Eq, PartialEq)] pub enum EditType { #[serde(rename = "ADD")] @@ -42,4 +49,8 @@ pub struct Violation { #[serde(default)] #[builder(default)] pub is_suppressed: bool, + /// The function or method enclosing this violation, if any. + #[serde(default)] + #[builder(default)] + pub enclosing_function: Option, } diff --git a/misc/integration-test-method-name.sh b/misc/integration-test-method-name.sh new file mode 100755 index 00000000..ed6282b9 --- /dev/null +++ b/misc/integration-test-method-name.sh @@ -0,0 +1,96 @@ +#!/bin/bash + +# Integration test: verify that enclosing method names are populated in SARIF +# logicalLocations when a violation falls inside a named method. +# +# Uses a self-contained dummy rule so the test does not depend on external +# rulesets or repositories. + +set -euo pipefail + +ANALYZER="./target/release-dev/datadog-static-analyzer" + +cargo fetch +cargo build --locked --profile release-dev --bin datadog-static-analyzer + +WORK_DIR=$(mktemp -d) +trap 'rm -rf "${WORK_DIR}"' EXIT + +# --------------------------------------------------------------------------- +# Source file: one class with one method containing one local variable. +# --------------------------------------------------------------------------- +cat > "${WORK_DIR}/Sample.java" << 'EOF' +class Sample { + public void doWork() { + int x = 1; + } +} +EOF + +# --------------------------------------------------------------------------- +# Rule: flag every local_variable_declaration. +# +# code (base64 of): +# function visit(node, filename, code) { +# const n = node.captures["decl"]; +# addError(buildError(n.start.line, n.start.col, n.end.line, n.end.col, +# "test violation", "WARNING", "BEST_PRACTICES")); +# } +# +# tree_sitter_query (base64 of): +# (local_variable_declaration) @decl +# --------------------------------------------------------------------------- +cat > "${WORK_DIR}/rules.json" << 'EOF' +[{ + "name": "test-ruleset", + "description": "dGVzdA==", + "rules": [{ + "name": "test-ruleset/flag-local-var", + "short_description": "dGVzdA==", + "description": "dGVzdA==", + "category": "BEST_PRACTICES", + "severity": "WARNING", + "language": "JAVA", + "rule_type": "TREE_SITTER_QUERY", + "entity_checked": null, + "code": "ZnVuY3Rpb24gdmlzaXQobm9kZSwgZmlsZW5hbWUsIGNvZGUpIHsKICBjb25zdCBuID0gbm9kZS5jYXB0dXJlc1siZGVjbCJdOwogIGFkZEVycm9yKGJ1aWxkRXJyb3Iobi5zdGFydC5saW5lLCBuLnN0YXJ0LmNvbCwgbi5lbmQubGluZSwgbi5lbmQuY29sLCAidGVzdCB2aW9sYXRpb24iLCAiV0FSTklORyIsICJCRVNUX1BSQUNUSUNFUyIpKTsKfQo=", + "checksum": "ed0928bb71c63712480323e22437d5e955e998e7658d3bb24ba9ed89eebc9723", + "pattern": null, + "tree_sitter_query": "KGxvY2FsX3ZhcmlhYmxlX2RlY2xhcmF0aW9uKSBAZGVjbAo=", + "tests": [], + "is_testing": false + }] +}] +EOF + +"${ANALYZER}" \ + --directory "${WORK_DIR}" \ + -r "${WORK_DIR}/rules.json" \ + -o "${WORK_DIR}/results.json" \ + -f sarif \ + -b + +# --------------------------------------------------------------------------- +# Assertions +# --------------------------------------------------------------------------- +TOTAL=$(jq '.runs[0].results | length' "${WORK_DIR}/results.json") +if [ "${TOTAL}" -ne 1 ]; then + echo "FAIL: expected 1 violation, got ${TOTAL}" + exit 1 +fi + +METHOD_NAME=$(jq -r '.runs[0].results[0].locations[0].logicalLocations[0].name // empty' "${WORK_DIR}/results.json") +KIND=$(jq -r '.runs[0].results[0].locations[0].logicalLocations[0].kind // empty' "${WORK_DIR}/results.json") + +if [ "${METHOD_NAME}" != "doWork" ]; then + echo "FAIL: expected logicalLocations name 'doWork', got '${METHOD_NAME}'" + exit 1 +fi + +if [ "${KIND}" != "function" ]; then + echo "FAIL: expected logicalLocations kind 'function', got '${KIND}'" + exit 1 +fi + +echo "PASS: logicalLocations name=${METHOD_NAME}, kind=${KIND}" +exit 0