feat: add SARIF output format for policy violations - #43
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| artifact_location: SarifArtifactLocation, | ||
| } | ||
|
|
||
| #[derive(Debug, Serialize)] |
There was a problem hiding this comment.
🟠 Important: Potential unbounded SARIF report size without max_results limit
The SARIF report generation iterates over all violations in the query results and collects them into the report. Although there is a max_results configuration field, it defaults to 0 (unlimited). Without explicit configuration, a large number of violations could cause excessive memory usage and slow serialization, risking resource exhaustion in production. Consider enforcing a reasonable default limit or documenting the need to set max_results to prevent OOM or degraded performance.
| #[derive(Debug, Serialize)] | ||
| struct SarifArtifactLocation { | ||
| uri: String, | ||
| #[serde(rename = "uriBaseId")] |
There was a problem hiding this comment.
🟠 Important: Non-deterministic iteration over violation objects in SARIF report generation
The function extract_violations returns a Vec<&Value> by iterating over Value::Set or Value::Array. Value::Set is a BTreeSet, which is deterministic, but Value::Array is a Vec, which preserves insertion order. If the input data or policy results contain arrays with non-deterministic ordering or are constructed from hash maps, the SARIF output could vary between runs. This may violate the determinism requirement. Ensure that input data or violation collections are deterministic or convert arrays to sorted sets before SARIF conversion.
| #[serde(rename = "physicalLocation")] | ||
| physical_location: SarifPhysicalLocation, | ||
| } | ||
|
|
There was a problem hiding this comment.
🟠 Important: Use of unwrap-like behavior in SARIF report generation error handling
The SarifReport::from_query_results method returns Result<Self, String> and uses map_err to convert errors from SARIF generation and JSON serialization. However, the error messages include the original error string, which may leak sensitive or internal information. While this aids debugging, it may expose sensitive data in production logs. Consider sanitizing or limiting error details to avoid information leakage.
| artifact_location: SarifArtifactLocation, | ||
| } | ||
|
|
||
| #[derive(Debug, Serialize)] |
There was a problem hiding this comment.
🔵 Suggestion: Lack of explicit resource limits on SARIF report generation CPU time
The SARIF report generation code processes all query results and violations without explicit CPU time or iteration limits. Although the engine enforces instruction limits during evaluation, the SARIF conversion is post-processing and could be expensive for large result sets. Consider adding cooperative checks or limits to prevent long-running SARIF generation that could degrade service responsiveness.
| use alloc::string::{String, ToString}; | ||
| use alloc::vec; | ||
| use alloc::vec::Vec; | ||
|
|
There was a problem hiding this comment.
🔵 Suggestion: No validation of SARIFConfig fields for determinism and safety
The SarifConfig struct allows customization of field names and base URI but does not validate these inputs. Malformed or malicious configuration could cause invalid SARIF output or injection of unexpected values. Adding validation or sanitization of config fields would improve robustness and prevent malformed reports.
| artifact_location: SarifArtifactLocation, | ||
| } | ||
|
|
||
| #[derive(Debug, Serialize)] |
There was a problem hiding this comment.
🔵 Suggestion: No concurrency considerations in SARIF report generation
The SARIF report generation code is single-threaded and uses Vec and other collections without synchronization. If future evaluation or reporting becomes concurrent, shared state such as rules_seen vector could cause race conditions. Consider documenting concurrency assumptions or using thread-safe collections if concurrency is introduced.
|
|
||
| #[derive(Debug, Serialize)] | ||
| struct SarifResult { | ||
| #[serde(rename = "ruleId")] |
There was a problem hiding this comment.
🔵 Suggestion: No explicit panic safety checks on string operations in SARIF code
The sanitize_rule_id function replaces invalid characters with underscores but assumes input strings are valid UTF-8 and non-empty. While unlikely to panic, malformed input could cause unexpected behavior. Adding explicit checks or using safe string handling would improve panic safety.
| @@ -114,8 +115,17 @@ fn rego_eval( | |||
| // to use. | |||
| let results = engine.eval_query(query, enable_tracing)?; | |||
|
|
|||
| println!("{}", serde_json::to_string_pretty(&results)?); | |||
|
|
|||
| match format { | |||
There was a problem hiding this comment.
🟠 Important: No dual-path testing verification for SARIF output
The new SARIF output format option in the rego_eval function introduces a new code path for formatting results. However, there is no evidence of tests verifying that this SARIF output path works correctly under both the interpreter and RVM execution modes. Dual-path testing is critical to ensure consistent behavior across both evaluation engines.
| @@ -0,0 +1,435 @@ | |||
| // Copyright (c) Microsoft Corporation. | |||
There was a problem hiding this comment.
🟠 Important: Lack of test coverage for SARIF report generation and serialization
The new sarif.rs module implements comprehensive SARIF report generation from query results, including configuration options and JSON serialization. However, the provided tests are limited and do not cover edge cases such as malformed input, missing fields, large result sets, or error handling during serialization. Additional tests should be added to cover these scenarios and verify robustness.
| @@ -0,0 +1,435 @@ | |||
| // Copyright (c) Microsoft Corporation. | |||
There was a problem hiding this comment.
🟠 Important: No tests verifying SARIF output correctness against OPA conformance
Since SARIF output is a new feature related to policy evaluation results, it is important to verify that it does not regress OPA conformance. There are no tests or references to running OPA conformance tests with SARIF output enabled. Adding OPA conformance tests that include SARIF output generation would help ensure correctness and compatibility.
| @@ -114,8 +115,17 @@ fn rego_eval( | |||
| // to use. | |||
| let results = engine.eval_query(query, enable_tracing)?; | |||
|
|
|||
| println!("{}", serde_json::to_string_pretty(&results)?); | |||
|
|
|||
| match format { | |||
There was a problem hiding this comment.
🔵 Suggestion: Add tests for SARIF output with feature flag combinations
The SARIF output feature may interact with other feature flags such as coverage or v0 Rego language mode. Tests should be designed to exercise SARIF output under various feature flag combinations to catch any feature-gated code coverage gaps or unexpected interactions.
| @@ -0,0 +1,435 @@ | |||
| // Copyright (c) Microsoft Corporation. | |||
There was a problem hiding this comment.
🔵 Suggestion: Test SARIF output with empty and large result sets
While there is a test for empty results, additional tests should verify SARIF report generation with very large result sets, including respecting the max_results limit. This will help catch performance or memory issues and ensure the max_results configuration is correctly enforced.
There was a problem hiding this comment.
Pull request overview
Adds first-class SARIF v2.1.0 report generation to Regorus so policy violations can be consumed by SARIF-compatible tooling (e.g., GitHub code scanning / Azure DevOps), and wires up a CLI output-format switch to emit SARIF.
Changes:
- Introduces
src/sarif.rswithSarifReport+SarifConfigand JSON serialization. - Exposes the SARIF module from the library (
pub mod sarif;). - Extends the example CLI (
examples/regorus.rs) with--format sarifto print SARIF instead of raw JSON query results.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
src/sarif.rs |
New SARIF report generator, including severity mapping, URI construction, rule ID sanitization, and unit tests. |
src/lib.rs |
Exports the new sarif module from the crate. |
examples/regorus.rs |
Adds an output-format flag and emits SARIF when requested. |
|
|
||
| let rule_id = extract_string_field(&violation, &config.rule_id_field) | ||
| .unwrap_or_else(|| "policy-violation".to_string()); | ||
| let rule_id = sanitize_rule_id(&rule_id); |
There was a problem hiding this comment.
After sanitize_rule_id, rule_id can still end up empty (e.g., the violation contains an explicit empty string for the configured rule_id_field). An empty SARIF ruleId is typically invalid / unhelpful; consider falling back to the default (e.g., policy-violation) when the sanitized result is empty.
| let rule_id = sanitize_rule_id(&rule_id); | |
| let rule_id = { | |
| let sanitized = sanitize_rule_id(&rule_id); | |
| if sanitized.is_empty() { | |
| "policy-violation".to_string() | |
| } else { | |
| sanitized | |
| } | |
| }; |
| impl Default for SarifConfig { | ||
| fn default() -> Self { | ||
| Self { | ||
| tool_name: "regorus".to_string(), | ||
| tool_version: env!("CARGO_PKG_VERSION").to_string(), | ||
| base_uri: String::new(), | ||
| severity_field: "severity".to_string(), | ||
| message_field: "msg".to_string(), | ||
| location_field: "file".to_string(), | ||
| rule_id_field: "rule_id".to_string(), | ||
| max_results: 0, | ||
| } |
There was a problem hiding this comment.
The PR description calls out a max-results limit to prevent unbounded output, but SarifConfig::default() sets max_results to 0 (unlimited) and the CLI path in examples/regorus.rs uses the default config. Consider choosing a safe non-zero default and/or wiring the CLI to set a limit (with an opt-out) so the protection is effective by default.
| /// Output format (json or sarif). | ||
| #[arg(long, short = 'F', default_value = "json")] | ||
| format: String, | ||
| }, /// Tokenize a Rego policy. |
There was a problem hiding this comment.
There’s a formatting issue where the doc comment for the next enum variant appears on the same line as the closing brace of Eval (}, /// Tokenize ...). This breaks rustdoc attachment and is easy to miss in review; please put the closing }, on its own line (and run rustfmt) so the /// Tokenize a Rego policy. comment applies to Lex.
| }, /// Tokenize a Rego policy. | |
| }, | |
| /// Tokenize a Rego policy. |
| } else { | ||
| // Strip leading slash/dot from relative path | ||
| let relative = location.trim_start_matches('/').trim_start_matches("./"); | ||
| (relative.to_string(), Some("SRCROOT".to_string())) | ||
| } |
There was a problem hiding this comment.
When base_uri is set, build_artifact_uri always treats location as relative and sets uriBaseId=SRCROOT, even if location is an absolute path or already a URI (e.g., file://...). This contradicts the documented behavior and can produce incorrect SARIF artifact locations. Consider detecting absolute paths / URIs first (even when base_uri is set) and only using SRCROOT for truly relative paths.
| /// Construct an artifact URI from the location field value. | ||
| /// If base_uri is configured, the location is treated as relative. | ||
| fn build_artifact_uri(location: &str, base_uri: &str) -> (String, Option<String>) { | ||
| if base_uri.is_empty() { | ||
| // Use location as-is; if it looks like an absolute path, convert to file URI | ||
| if location.starts_with('/') { | ||
| (format!("file://{location}"), None) |
There was a problem hiding this comment.
build_artifact_uri only detects Unix-style absolute paths via location.starts_with('/'). On Windows, absolute paths (e.g. C:\...) or UNC paths (\\server\share\...) would be emitted as non-URI strings, which is typically invalid for SARIF artifactLocation.uri. Consider adding simple Windows/UNC absolute-path detection and converting those to file:/// URIs as well.
| /// Construct an artifact URI from the location field value. | |
| /// If base_uri is configured, the location is treated as relative. | |
| fn build_artifact_uri(location: &str, base_uri: &str) -> (String, Option<String>) { | |
| if base_uri.is_empty() { | |
| // Use location as-is; if it looks like an absolute path, convert to file URI | |
| if location.starts_with('/') { | |
| (format!("file://{location}"), None) | |
| fn is_windows_drive_absolute_path(location: &str) -> bool { | |
| let bytes = location.as_bytes(); | |
| if bytes.len() < 3 { | |
| return false; | |
| } | |
| bytes[0].is_ascii_alphabetic() | |
| && bytes[1] == b':' | |
| && (bytes[2] == b'\\' || bytes[2] == b'/') | |
| } | |
| fn absolute_path_to_file_uri(location: &str) -> Option<String> { | |
| if location.starts_with("//") || location.starts_with("\\\\") { | |
| let trimmed = location.trim_start_matches('/').trim_start_matches('\\'); | |
| Some(format!("file://{}", trimmed.replace('\\', "/"))) | |
| } else if location.starts_with('/') { | |
| Some(format!("file://{location}")) | |
| } else if is_windows_drive_absolute_path(location) { | |
| Some(format!("file:///{}", location.replace('\\', "/"))) | |
| } else { | |
| None | |
| } | |
| } | |
| /// Construct an artifact URI from the location field value. | |
| /// If base_uri is configured, the location is treated as relative. | |
| fn build_artifact_uri(location: &str, base_uri: &str) -> (String, Option<String>) { | |
| if base_uri.is_empty() { | |
| // Use location as-is; if it looks like an absolute path, convert to file URI | |
| if let Some(uri) = absolute_path_to_file_uri(location) { | |
| (uri, None) |
| for query_result in query_results.result.iter() { | ||
| for expression in query_result.expressions.iter() { | ||
| let violations = extract_violations(&expression.value); | ||
| for violation in violations { | ||
| if config.max_results > 0 && results.len() >= config.max_results { | ||
| break; |
There was a problem hiding this comment.
max_results enforcement currently only breaks the innermost loop, so once the limit is reached the code still continues scanning remaining query results/expressions (doing repeated limit checks and extra iteration). For large result sets this adds avoidable overhead; consider using a labeled break / early return once the limit is hit so the whole traversal stops immediately.
| for query_result in query_results.result.iter() { | |
| for expression in query_result.expressions.iter() { | |
| let violations = extract_violations(&expression.value); | |
| for violation in violations { | |
| if config.max_results > 0 && results.len() >= config.max_results { | |
| break; | |
| 'results_loop: for query_result in query_results.result.iter() { | |
| for expression in query_result.expressions.iter() { | |
| let violations = extract_violations(&expression.value); | |
| for violation in violations { | |
| if config.max_results > 0 && results.len() >= config.max_results { | |
| break 'results_loop; |
| // Track unique rules | ||
| if !rules_seen.iter().any(|r: &String| *r == rule_id) { | ||
| rules_seen.push(rule_id.clone()); | ||
| } |
There was a problem hiding this comment.
Tracking rules_seen in a Vec and checking membership with iter().any(...) makes rule collection O(n²) in the number of distinct rule IDs. Using an ordered set (e.g., BTreeSet<String>) would avoid repeated linear scans and keeps output stable without additional sorting.
| Value::Array(arr) => arr.iter().collect(), | ||
| Value::Set(set) => set.iter().collect(), |
There was a problem hiding this comment.
extract_violations returns all elements from arrays/sets, even when they are not objects. Those non-object values will then be converted into SARIF results with default rule_id/message/location, which can silently produce misleading SARIF output for unexpected query shapes. Consider filtering to Value::Object elements (and/or returning an error when encountering non-object entries).
| Value::Array(arr) => arr.iter().collect(), | |
| Value::Set(set) => set.iter().collect(), | |
| Value::Array(arr) => arr | |
| .iter() | |
| .filter(|entry| matches!(entry, Value::Object(_))) | |
| .collect(), | |
| Value::Set(set) => set | |
| .iter() | |
| .filter(|entry| matches!(entry, Value::Object(_))) | |
| .collect(), |
| @@ -114,8 +115,17 @@ fn rego_eval( | |||
| // to use. | |||
| let results = engine.eval_query(query, enable_tracing)?; | |||
|
|
|||
| println!("{}", serde_json::to_string_pretty(&results)?); | |||
|
|
|||
| match format { | |||
There was a problem hiding this comment.
🟠 Important: New SARIF output format option added without cross-binding parity verification
The PR adds a new output format option "sarif" to the rego_eval function and CLI, which affects the public API surface by introducing a new output format. This change impacts all 9 FFI binding targets since it modifies the public CLI and example usage. There is no indication that corresponding changes or parity updates have been made in the other language bindings (C, C++, C#, Go, Java, Python, Ruby, WASM). This could lead to inconsistent API behavior or missing features in other bindings.
| pub mod target; | ||
| #[cfg(any(test, all(feature = "yaml", feature = "std")))] | ||
| pub mod test_utils; | ||
| pub mod sarif; |
There was a problem hiding this comment.
🟠 Important: New public module sarif added without documented deprecation or versioning impact
The PR introduces a new public module sarif in the core library, expanding the public API surface. This is a non-breaking addition but should be accompanied by appropriate semver minor versioning and documentation updates. There is no mention of version bump or changelog updates in the diff, which is important for consumers to track new features. Also, no deprecation or migration notes are needed here, but the versioning discipline should be confirmed.
| @@ -0,0 +1,435 @@ | |||
| // Copyright (c) Microsoft Corporation. | |||
There was a problem hiding this comment.
🟠 Important: Large new SARIF module added without explicit changelog or documentation update
A substantial new module implementing SARIF report generation is added, significantly expanding the public API surface. This addition introduces new public structs, functions, and configuration options that downstream consumers and bindings may need to support. The PR does not show any changelog or documentation updates reflecting this new feature, which is important for API consumers and for maintaining semver discipline.
| @@ -114,8 +115,17 @@ fn rego_eval( | |||
| // to use. | |||
| let results = engine.eval_query(query, enable_tracing)?; | |||
|
|
|||
| println!("{}", serde_json::to_string_pretty(&results)?); | |||
|
|
|||
| match format { | |||
There was a problem hiding this comment.
🔵 Suggestion: Consider adding deprecation warnings or migration notes if SARIF output replaces or supersedes existing formats
If the new SARIF output format is intended to replace or supersede any existing output formats or APIs, it is recommended to add deprecation warnings on the old formats and provide migration guidance. Currently, the PR adds SARIF as an additional format without deprecating existing JSON output, which is fine, but if future plans include deprecations, those should be signaled properly.
| @@ -0,0 +1,435 @@ | |||
| // Copyright (c) Microsoft Corporation. | |||
There was a problem hiding this comment.
🟠 Important: SARIF module added in src/ breaks existing module boundary conventions
The new SARIF output module is added directly under src/ as src/sarif.rs, which deviates from the existing module organization conventions. Typically, language backends go under src/languages/, builtins under src/builtins/, and so forth. Adding a new top-level module without clear categorization may lead to module boundary pollution and maintenance challenges as the codebase grows.
| @@ -0,0 +1,435 @@ | |||
| // Copyright (c) Microsoft Corporation. | |||
There was a problem hiding this comment.
🟠 Important: SARIF module introduces significant new feature without no_std compatibility consideration
The SARIF module uses alloc and serde_json crates and does not indicate any no_std compatibility. Given regorus's support for no_std environments, this new feature may not be usable in constrained environments or embedded targets. Consider isolating SARIF behind a feature flag or ensuring it can compile with no_std to maintain broad compatibility.
|
|
||
| #[derive(Debug, Serialize)] | ||
| struct SarifLocation { | ||
| #[serde(rename = "physicalLocation")] |
There was a problem hiding this comment.
🟠 Important: SARIF report generation logic tightly couples to specific query result structure
The SARIF report generation expects query results to have specific fields like 'msg', 'severity', 'file', and 'rule_id'. This tight coupling to a particular result shape may reduce flexibility and complicate adding new policy languages or evaluation paths that produce different result formats. Consider abstracting or providing adapters to support diverse result schemas.
|
|
||
| #[derive(Debug, Serialize)] | ||
| struct SarifLocation { | ||
| #[serde(rename = "physicalLocation")] |
There was a problem hiding this comment.
🟠 Important: SARIF report generation does not appear to handle interpreter vs RVM execution path differences
The SARIF report generation code processes QueryResults without distinguishing between interpreter and RVM execution paths. Since these paths may produce different result structures or metadata, the SARIF module should explicitly handle or document compatibility with both to avoid subtle bugs or incomplete reports.
| @@ -114,8 +115,17 @@ fn rego_eval( | |||
| // to use. | |||
| let results = engine.eval_query(query, enable_tracing)?; | |||
|
|
|||
| println!("{}", serde_json::to_string_pretty(&results)?); | |||
|
|
|||
| match format { | |||
There was a problem hiding this comment.
🟠 Important: New SARIF output format option added to examples/regorus.rs lacks integration with FFI and bindings
The example CLI now supports outputting SARIF format, but this new output format is only implemented in the Rust example binary. There is no indication of corresponding changes to the public API or FFI bindings to expose SARIF report generation to all 9 binding targets. This limits the feature's accessibility and may cause fragmentation.
|
|
||
| #[derive(Debug, Serialize)] | ||
| struct SarifLocation { | ||
| #[serde(rename = "physicalLocation")] |
There was a problem hiding this comment.
🔵 Suggestion: SARIF report generation uses string matching for severity and fields which may be error-prone
The SARIF module extracts fields like severity and message by string matching keys in Value objects. This approach may be fragile if field names change or if policies produce unexpected data shapes. Consider using a more robust schema validation or typed intermediate representation to improve maintainability and error handling.
|
|
||
| #[derive(Debug, Serialize)] | ||
| struct SarifLocation { | ||
| #[serde(rename = "physicalLocation")] |
There was a problem hiding this comment.
🔵 Suggestion: SARIF module uses Vec and BTreeSet without explicit consideration for performance at scale
The SARIF report accumulates results and rules in Vec and uses linear searches to track unique rules. At large scale, this may cause performance bottlenecks. Consider using HashSet or other data structures optimized for large datasets, especially since SARIF reports can be large in real-world scenarios.
|
|
||
| #[derive(Debug, Serialize)] | ||
| struct SarifLocation { | ||
| #[serde(rename = "physicalLocation")] |
There was a problem hiding this comment.
🟠 Important: Potential allocation overhead in SARIF report generation
The SARIF report generation code builds multiple intermediate Vec collections (e.g., results, rules_seen, rules) and repeatedly clones strings such as rule IDs and messages. While cloning Rc is cheap, the current approach may cause allocations proportional to the number of violations. Consider reusing buffers or streaming serialization to reduce peak memory usage and allocations, especially for large result sets.
|
|
||
| #[derive(Debug, Serialize)] | ||
| struct SarifLocation { | ||
| #[serde(rename = "physicalLocation")] |
There was a problem hiding this comment.
🟠 Important: Nested iteration over query results and expressions may cause O(n*m) complexity
The SARIF report generation iterates over all query results and then over all expressions within each result, extracting violations and processing each. If the number of results and expressions is large, this nested iteration could lead to quadratic scaling in processing time. Evaluate if this is on the evaluation hot path and consider optimizations or early filtering to reduce complexity.
|
|
||
| #[derive(Debug, Serialize)] | ||
| struct SarifLocation { | ||
| #[serde(rename = "physicalLocation")] |
There was a problem hiding this comment.
🔵 Suggestion: Use of Vec for rules_seen to track unique rules can be inefficient
The code uses a Vec to track unique rule IDs and checks for existence with iter().any(), which is O(n) per insertion. For large numbers of unique rules, this can degrade performance. Using a HashSet or BTreeSet would provide O(1) or O(log n) lookups and improve scalability.
|
|
||
| #[derive(Debug, Serialize)] | ||
| struct SarifLocation { | ||
| #[serde(rename = "physicalLocation")] |
There was a problem hiding this comment.
🔵 Suggestion: String formatting in extract_string_field may cause unnecessary allocations
In extract_string_field, when the field value is not a string, the code formats it with format!("{other}") to produce a string. This may cause unnecessary allocations in the hot path if many violations have non-string fields. Consider returning a borrowed string or avoiding formatting unless necessary.
|
|
||
| #[derive(Debug, Serialize)] | ||
| struct SarifLocation { | ||
| #[serde(rename = "physicalLocation")] |
There was a problem hiding this comment.
🔵 Suggestion: SARIF report serialization uses serde_json::to_string_pretty which allocates full JSON string
The to_json method serializes the entire SARIF report into a pretty-printed JSON string in memory. For large reports, this can cause high memory usage and allocation overhead. Consider supporting streaming serialization or compact JSON output for performance-sensitive scenarios.
| @@ -114,8 +115,17 @@ fn rego_eval( | |||
| // to use. | |||
| let results = engine.eval_query(query, enable_tracing)?; | |||
|
|
|||
| println!("{}", serde_json::to_string_pretty(&results)?); | |||
|
|
|||
| match format { | |||
There was a problem hiding this comment.
🔵 Suggestion: In examples/regorus.rs, match on format string uses default arm for 'json' and others
The match on format string uses a default arm that includes 'json' and any other strings. This is fine functionally but may cause unexpected behavior if an unsupported format string is passed. Consider validating the format string earlier to avoid unnecessary processing or errors.
| @@ -114,8 +115,17 @@ fn rego_eval( | |||
| // to use. | |||
| let results = engine.eval_query(query, enable_tracing)?; | |||
|
|
|||
| println!("{}", serde_json::to_string_pretty(&results)?); | |||
|
|
|||
| match format { | |||
There was a problem hiding this comment.
🟠 Important: Unvalidated 'format' parameter may cause unexpected output or injection
The 'format' parameter is accepted as a string and used to select output format without validation beyond matching 'sarif' or 'json'. An attacker could supply unexpected values to trigger the default JSON output or potentially cause confusion in output handling. While this is low risk, explicit validation or enumeration of allowed formats would improve robustness and prevent misuse.
| artifact_location: SarifArtifactLocation, | ||
| } | ||
|
|
||
| #[derive(Debug, Serialize)] |
There was a problem hiding this comment.
🟠 Important: SARIF report generation trusts unvalidated query result fields
The SARIF report generation extracts fields like 'severity', 'msg', 'file', and 'rule_id' from arbitrary query result objects without strict validation or sanitization beyond rule ID character filtering. Maliciously crafted policy results could contain unexpected or malformed data, potentially causing incorrect SARIF output or injection of misleading information into the report. Additional validation and sanitization of these fields is recommended.
| artifact_location: SarifArtifactLocation, | ||
| } | ||
|
|
||
| #[derive(Debug, Serialize)] |
There was a problem hiding this comment.
🟠 Important: SARIF report may produce large output without resource limits
The SARIF report generation supports a 'max_results' limit, but defaults to 0 (unlimited). If an attacker crafts a policy result with a very large number of violations, this could cause excessive memory and CPU usage during report generation and serialization, leading to DoS. Enforcing a reasonable default limit and/or validating input size is advised.
| artifact_location: SarifArtifactLocation, | ||
| } | ||
|
|
||
| #[derive(Debug, Serialize)] |
There was a problem hiding this comment.
🟠 Important: Potential for unbounded iteration over query results in SARIF report
The code iterates over all query results and their expressions to extract violations without explicit upper bounds except the optional max_results. If the input query results are deeply nested or very large, this could lead to high CPU and memory consumption. Defensive programming with input size checks or iteration limits would mitigate resource exhaustion risks.
|
|
||
| #[derive(Debug, Serialize)] | ||
| struct SarifResult { | ||
| #[serde(rename = "ruleId")] |
There was a problem hiding this comment.
🔵 Suggestion: Rule ID sanitization replaces invalid characters with underscore
The sanitize_rule_id function replaces any character not alphanumeric, dash, dot, or slash with an underscore. While this prevents invalid SARIF rule IDs, it may cause collisions or loss of meaningful identifiers if many invalid characters are replaced. Consider logging or warning on sanitization to detect potential issues.
| #[serde(rename = "uriBaseId")] | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| uri_base_id: Option<String>, | ||
| } |
There was a problem hiding this comment.
🟠 Important: Potential non-deterministic iteration over violation objects in SARIF report generation
The function extract_violations returns a vector of references to violation objects extracted from a Value. When the value is a Set, it uses set.iter(), which is a BTreeSet and thus deterministic. However, if the underlying Value representation changes in the future to use hash-based collections (as noted in the documentation), iteration order may become non-deterministic. This could cause SARIF reports to vary between runs with the same input, violating evaluation determinism requirements.
| text: String, | ||
| } | ||
|
|
||
| /// Maps OPA-style severity strings to SARIF levels. |
There was a problem hiding this comment.
🟠 Important: Unbounded SARIF results accumulation may cause resource exhaustion
The SARIF report generation accumulates results from all query results and expressions without strict limits unless max_results is set. If max_results is zero (unlimited), a large or adversarially crafted query result could cause unbounded memory allocation and CPU usage, risking OOM or degraded performance. Consider enforcing a reasonable default limit or validating input size to bound resource usage.
| match severity.to_lowercase().as_str() { | ||
| "error" | "critical" | "high" => "error", | ||
| "warning" | "medium" => "warning", | ||
| "info" | "note" | "low" => "note", |
There was a problem hiding this comment.
🟠 Important: Use of unwrap_or_else with string formatting may mask errors and cause inconsistent messages
The extract_string_field function returns a string representation of the field value, using format! on non-string values. This may produce inconsistent or unexpected messages if the field is not a string, potentially confusing operators. Additionally, no validation is done on the extracted strings, which could lead to malformed SARIF output or injection of unexpected content.
| use alloc::string::{String, ToString}; | ||
| use alloc::vec; | ||
| use alloc::vec::Vec; | ||
|
|
There was a problem hiding this comment.
🟠 Important: Lack of validation on SARIF configuration fields may lead to malformed output or errors
The SarifConfig struct allows configuration of field names and base URI without validation. If invalid or malicious values are provided (e.g., empty or malformed URIs, invalid field names), the SARIF report generation may produce invalid JSON or incorrect references, complicating operator diagnosis and integration with SARIF consumers.
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| uri_base_id: Option<String>, | ||
| } | ||
|
|
There was a problem hiding this comment.
🔵 Suggestion: Consider adding error handling for unexpected value types in SARIF violation extraction
Currently, extract_violations returns an empty vector for any Value variant other than Array, Set, or Object. It may be helpful to log or report unexpected types to aid debugging and ensure that no violations are silently dropped, improving operational observability.
|
|
||
| /// Sanitize a string for use as a SARIF rule ID. | ||
| /// Rule IDs must be stable identifiers — only alphanumeric, dash, dot, slash allowed. | ||
| fn sanitize_rule_id(raw: &str) -> String { |
There was a problem hiding this comment.
🔵 Suggestion: Explicitly document and enforce deterministic ordering of SARIF rules and results
The SARIF report collects unique rules in a Vec<String> and then maps them to descriptors. The order depends on insertion order, which may vary if the underlying data changes. Using a BTreeSet or sorting the rules before serialization would guarantee deterministic ordering, improving reproducibility and operator confidence.
| "error" | "critical" | "high" => "error", | ||
| "warning" | "medium" => "warning", | ||
| "info" | "note" | "low" => "note", | ||
| _ => "warning", |
There was a problem hiding this comment.
🔵 Suggestion: Validate and sanitize SARIF message and location strings to prevent injection or formatting issues
The SARIF message and location fields are directly taken from policy violation data without sanitization. Malicious or malformed strings could break JSON formatting or cause issues in SARIF consumers. Adding sanitization or escaping would improve robustness and security.
| @@ -114,8 +115,17 @@ fn rego_eval( | |||
| // to use. | |||
| let results = engine.eval_query(query, enable_tracing)?; | |||
|
|
|||
| println!("{}", serde_json::to_string_pretty(&results)?); | |||
|
|
|||
| match format { | |||
There was a problem hiding this comment.
🟠 Important: Unvalidated 'format' parameter usage in output formatting
The 'format' parameter is used to select output format (JSON or SARIF) without explicit validation of allowed values. Although the match statement defaults to JSON, an attacker or malformed input could potentially cause unexpected behavior if additional formats are added later or if the input is manipulated. It is recommended to validate or sanitize the 'format' input strictly to known safe values to prevent misuse or injection.
|
|
||
| /// Tokenize a Rego policy. | ||
| /// Output format (json or sarif). |
There was a problem hiding this comment.
🔵 Suggestion: Lack of input validation on 'format' CLI argument
The 'format' argument is accepted as a String from the command line without validation. While the code currently handles only 'json' and 'sarif', other inputs will default to JSON output. Explicit validation or enumeration of allowed values at the CLI parsing stage would improve robustness and prevent unexpected inputs.
| _ => "warning", | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🔵 Suggestion: SARIF report generation does not limit or sanitize violation fields strictly
The SARIF report generation extracts fields like 'rule_id', 'msg', 'severity', and 'file' from arbitrary query results without strict validation or sanitization beyond basic character replacement for rule IDs. Malicious or malformed policy evaluation results could inject unexpected content into the SARIF output. Consider adding stricter validation, escaping, or schema enforcement on these fields to prevent injection or malformed SARIF output.
| } | ||
| } | ||
|
|
||
| impl SarifReport { |
There was a problem hiding this comment.
🔵 Suggestion: Potential information disclosure via detailed SARIF error messages
Error messages from SARIF generation and JSON serialization include detailed error strings that may expose internal state or implementation details. While useful for debugging, in production or untrusted contexts these messages could leak sensitive information. Consider sanitizing or generalizing error messages before exposing them to end users.
| _ => "warning", | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🔵 Suggestion: No resource limits or DoS protections in SARIF report generation
The SARIF report generation processes all query results and violations without explicit resource limits except for an optional max_results cap. Large or maliciously crafted query results could cause high memory or CPU usage during SARIF conversion. Ensure that upstream query evaluation enforces resource limits and consider adding additional safeguards in SARIF generation to prevent DoS.
| @@ -114,8 +115,17 @@ fn rego_eval( | |||
| // to use. | |||
| let results = engine.eval_query(query, enable_tracing)?; | |||
|
|
|||
| println!("{}", serde_json::to_string_pretty(&results)?); | |||
|
|
|||
| match format { | |||
There was a problem hiding this comment.
🟠 Important: No dual-path testing coverage for SARIF output feature
The new SARIF output format feature added to the rego_eval function and CLI is not accompanied by explicit tests verifying that the SARIF output works correctly under both the interpreter and RVM execution paths. Given the critical requirement that all evaluation results must be consistent across both paths, tests exercising SARIF output under both configurations should be added to prevent regressions or discrepancies.
| match format { | ||
| "sarif" => { | ||
| let config = regorus::sarif::SarifConfig::default(); | ||
| let report = regorus::sarif::SarifReport::from_query_results(&results, &config) |
There was a problem hiding this comment.
🟠 Important: Lack of error path tests for SARIF report generation failures
The SARIF report generation code calls functions that can fail (e.g., SarifReport::from_query_results and to_json) and converts errors to anyhow errors. However, there is no indication that error or failure paths (such as malformed query results or serialization failures) are tested. Tests should be designed to cover these failure modes to ensure robust error handling and correct error propagation.
| @@ -0,0 +1,435 @@ | |||
| // Copyright (c) Microsoft Corporation. | |||
There was a problem hiding this comment.
🟠 Important: No tests for SARIF configuration variations and feature matrix coverage
The SARIF module introduces a configuration struct with multiple fields controlling tool name, version, severity mapping, and base URI. There is no evidence of tests covering different configuration permutations or feature flag combinations. Given the complexity of the SARIF output and its integration with external tools, tests should cover various config scenarios to ensure correct behavior across the feature matrix.
| @@ -0,0 +1,435 @@ | |||
| // Copyright (c) Microsoft Corporation. | |||
There was a problem hiding this comment.
🟠 Important: No dual-path evaluation tests for SARIF report generation from query results
The SarifReport::from_query_results function converts query results into SARIF format. Since query evaluation results can differ between interpreter and RVM, tests should verify that SARIF reports generated from both evaluation paths are consistent and correct. Currently, no such dual-path tests are present, risking undetected discrepancies.
| @@ -0,0 +1,435 @@ | |||
| // Copyright (c) Microsoft Corporation. | |||
There was a problem hiding this comment.
🔵 Suggestion: Add property-based or fuzz testing for SARIF report generation
Given the SARIF report generation processes arbitrary query results that may contain diverse and potentially malformed data, property-based testing or fuzzing could help uncover edge cases and robustness issues. Adding such tests would improve confidence that the SARIF output handles all valid and invalid inputs gracefully.
anakrish
left a comment
anakrish
left a comment
There was a problem hiding this comment.
🔒 Security Auditor (Deep Review) — 4 finding(s)
🟠 Important: Lack of sanitization for SARIF message and location fields
The SARIF report generation extracts message and location fields from policy evaluation results and includes them verbatim in the SARIF JSON output. While rule IDs are sanitized and severity is mapped safely, the message and location fields are not escaped or sanitized beyond string conversion. This could allow injection of control characters or malformed JSON if untrusted input contains malicious content, potentially leading to information disclosure or malformed SARIF reports. It is recommended to implement escaping or sanitization of these fields to ensure safe output.
🟠 Important: Resource limit enforcement to prevent DoS
The SARIF report generation respects a configurable max_results limit that caps the number of results included in the SARIF output. This prevents excessive memory or CPU consumption when processing large policy evaluation results, mitigating denial-of-service risks. The limit is enforced during iteration over violations, and tests verify correct behavior.
🟠 Important: Robust error handling and fail-closed behavior
The SARIF module returns Result types with descriptive error messages on failure, avoiding panics or unwraps without fallback. The example CLI converts SARIF generation errors into anyhow errors and fails gracefully. This ensures that errors in SARIF report generation do not cause crashes or unsafe states, adhering to fail-closed security principles.
🔵 Suggestion: Document input assumptions and sanitization for SARIF output
The SARIF module and example usage are well documented and tested, but there is no explicit documentation on input assumptions or sanitization requirements for fields included in the SARIF output. Adding documentation on expected input formats, sanitization performed, and recommendations for policy authors would improve audit readiness and security transparency.
Copilot instructions, skills, prompts, and knowledge files for regorus development. Includes: - copilot-instructions.md: coding rules, build commands, repo layout - copilot-code-review-instructions.md: review focus areas - 6 skills: thorough-review, security-review, add-builtin, design-alternatives, opa-conformance, verification - 11 audit prompts for direct invocation - 21 knowledge files covering all major subsystems The thorough-review skill uses parallel focused rubber-duck reviewers (7 focus areas: 3 always-run + 4 triggered) with adversarial defense filtering for high-confidence findings. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add a new `sarif` module that converts policy evaluation results into SARIF v2.1.0 format, enabling integration with GitHub Advanced Security, Azure DevOps, and other SARIF-consuming tools. - New `src/sarif.rs` module with `SarifReport` and `SarifConfig` - CLI: `--format sarif` option on the `eval` command - Configurable field mapping (msg, severity, file, rule_id) - Max results limit support - Severity mapping (OPA-style → SARIF levels) - Proper URI handling with base URI support Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
ef8eca3 to
c2019bb
Compare
b3cc8ab to
369c82a
Compare
Summary
Add SARIF (Static Analysis Results Interchange Format) v2.1.0 output support to regorus, enabling integration with GitHub Advanced Security code scanning, Azure DevOps, and other SARIF-consuming tools.
Changes
src/sarif.rs: Core SARIF report generation with configurable field mapping--format sarifoption on theevalcommandUsage
Design Decisions
msg,severity,file, andrule_idfields (configurable)file://URIs; relative paths useSRCROOTbase URITesting
7 unit tests covering: