diff --git a/specs/016-native-tools/quickstart.md b/specs/016-native-tools/quickstart.md index f625b92..9fac4e2 100644 --- a/specs/016-native-tools/quickstart.md +++ b/specs/016-native-tools/quickstart.md @@ -252,11 +252,13 @@ falsifying the record — while every rendering escapes them: `x = "\x1b[2J\x1b[ \u{202e}`. The screen does not clear. This is the 014 `safe_text` property applied to a new input channel. -**Known gap**: `Finding.advisory_level` is always `None` for Opengrep. Opengrep reports severity in -`tool.driver.rules[].defaultConfiguration.level`, not on the result, and `src/sarif.rs` deliberately -never materialises `tool.driver.rules` — that array is the 99.96% of the report that research R4 -says not to read. The field is harmless (advisory level never becomes `Severity`, FR-005) but -presently unreachable through this adapter. +**Where the advisory level comes from**: Opengrep reports severity on the *rule* +(`tool.driver.rules[].defaultConfiguration.level`), not on the result. That array is the 99.96% of +the report research R4 says not to read, so `src/sarif.rs` streams it and retains only `id → level` +— two short strings per rule, bounded by `MAX_RULE_LEVELS` (4096) — while the descriptions, help +text, and tags are parsed and dropped. A result carrying its own `level` overrides the rule default. +The level is recorded and rendered as advisory and never becomes a `Severity` (FR-005); an +over-cap catalogue costs advisory levels for the overflow and never a finding or a scan. ## US4 — severity is computed, not guessed diff --git a/specs/016-native-tools/tasks.md b/specs/016-native-tools/tasks.md index 538e10b..56c60dd 100644 --- a/specs/016-native-tools/tasks.md +++ b/specs/016-native-tools/tasks.md @@ -225,7 +225,7 @@ malformed vector is rejected; a caller-supplied score is rejected rather than re Three things the walkthrough surfaced that are code decisions, not documentation fixes. None blocks the slice; all are recorded in `quickstart.md` beside the check that found them. -- [ ] T070 `Finding.advisory_level` is dead for the one shipped adapter. Opengrep reports severity in `tool.driver.rules[].defaultConfiguration.level`, not on the result, and `src/sarif.rs` deliberately never materialises `tool.driver.rules` — that array is the 99.96% of the report research R4 says not to read. Either reach the level without materialising the catalogue (a streaming id→level pass over the rules array in `sarif-worker`, which is in-scope and bounded), or drop the field and say why. Harmless today (an advisory level never becomes `Severity`, FR-005), but a field that can never be populated is a lie in the data model. +- [X] T070 `Finding.advisory_level` is dead for the one shipped adapter. Opengrep reports severity in `tool.driver.rules[].defaultConfiguration.level`, not on the result, and `src/sarif.rs` deliberately never materialises `tool.driver.rules` — that array is the 99.96% of the report research R4 says not to read. Either reach the level without materialising the catalogue (a streaming id→level pass over the rules array in `sarif-worker`, which is in-scope and bounded), or drop the field and say why. Harmless today (an advisory level never becomes `Severity`, FR-005), but a field that can never be populated is a lie in the data model. *(Fixed by the first option: `retain_rule_levels` in `src/sarif.rs` is a streaming seq visitor that collapses the catalogue to `id → level` as it passes — two short strings per rule, capped at `MAX_RULE_LEVELS` (4096) — so help text, descriptions, and tags are still parsed and dropped rather than allocated. A result's own `level` wins over the rule default (SARIF §3.27.10); a rule past the cap costs only its advisory level, never the finding, because the visitor drains the sequence rather than aborting the deserializer and turning a big catalogue into "unparseable report". FR-005 is unmoved: `to_finding` carries the level and leaves `severity` `None`, asserted in `a_rule_level_reaches_the_ledger_without_becoming_a_severity`.)* - [X] T071 The FR-005 score guard keys on the flat `severity_score`. A caller who nests `severity = { score = … }` has the object dropped by serde, so no score reaches the ledger — FR-005 is not violated — but the caller is *silently ignored* rather than refused, which is precisely the failure mode the guard's own doc-comment says it exists to prevent. *(Fixed: `#[serde(deny_unknown_fields)]` on `RecordArgs`, so there is no spelling of "here is my score" bee accepts quietly. Covered by `a_score_nested_under_another_name_is_refused_too`.)* - [ ] T072 `repl_command::no_provider_at_all_reports_what_is_missing` fails on any non-`enforce` build — the unenforced-session refusal fires before the configuration-completeness check the test asserts on. **Pre-existing on `main`**, not a 016 regression (verified by running the test on both branches at default features), and the only failure in the whole suite. It needs `--host`, or the `skip_on_enforcement_build()` treatment its neighbours have, inverted. diff --git a/src/sarif.rs b/src/sarif.rs index 1043435..4d7d376 100644 --- a/src/sarif.rs +++ b/src/sarif.rs @@ -7,8 +7,14 @@ //! **1,912,546 bytes**, of which **99.96% is the embedded rule catalogue** — 1074 rule objects //! carrying descriptions, help text, and tags — to deliver **839 bytes** of results. A faithful //! model of the 2.1.0 schema materialises all of it to reach the part that matters. The structs -//! below name only the fields bee reads; serde ignores the rest, so `tool.driver.rules` is never -//! allocated at all. +//! below name only the fields bee reads; serde ignores the rest. +//! +//! One thing bee does need from that catalogue: Opengrep reports a rule's severity on the *rule* +//! (`defaultConfiguration.level`), not on the result, so a normaliser that skipped the array +//! entirely could never populate `advisory_level` at all. [`retain_rule_levels`] streams the array +//! and retains **only** `id → level` — two short strings per rule, bounded by +//! [`MAX_RULE_LEVELS`] — while the descriptions, help text, and tags that are the 99.96% are parsed +//! and dropped without ever being allocated. //! //! ## Why this runs in a child rather than in the harness //! @@ -26,6 +32,8 @@ //! scanner's `level` is recorded as advisory only — it never becomes a [`Severity`], because //! severity comes from the computed path alone (FR-005). +use std::collections::BTreeMap; + use serde::Deserialize; use crate::findings::{Finding, FindingSource}; @@ -39,6 +47,13 @@ pub const MAX_FINDINGS_PER_SCAN: usize = 200; /// one would under-report a scan that ran correctly. pub const WHOLE_PROGRAM: &str = "(whole program)"; +/// The cap on `id → level` pairs retained from one report's rule catalogue. +/// +/// The measured registry ruleset carries 1074 rules; 4096 leaves room for a corpus several times +/// that while keeping a hostile report from turning an unbounded array into unbounded retention. +/// Exceeding it costs only advisory levels for the overflow — never a finding, and never the scan. +pub const MAX_RULE_LEVELS: usize = 4096; + // ── The subset bee reads (data-model.md §Scanner Report) ───────────────────────────────────────── #[derive(Debug, Deserialize)] @@ -53,6 +68,78 @@ struct Run { invocations: Vec, #[serde(default)] results: Vec, + /// Read for exactly one thing: the rule → level map (see [`retain_rule_levels`]). + #[serde(default)] + tool: Option, +} + +#[derive(Debug, Deserialize)] +struct Tool { + #[serde(default)] + driver: Option, +} + +#[derive(Debug, Deserialize)] +struct Driver { + /// The catalogue, collapsed to `id → level` as it streams past. Never materialised as objects. + #[serde(default, deserialize_with = "retain_rule_levels")] + rules: BTreeMap, +} + +#[derive(Debug, Deserialize)] +struct RuleMeta { + #[serde(default)] + id: Option, + #[serde(rename = "defaultConfiguration", default)] + default_configuration: Option, +} + +#[derive(Debug, Deserialize)] +struct DefaultConfiguration { + #[serde(default)] + level: Option, +} + +/// Stream `tool.driver.rules`, keeping `id → level` and discarding everything else. +/// +/// The sequence is drained to the end even once [`MAX_RULE_LEVELS`] is reached — stopping early +/// would abort the deserializer mid-document and turn a large catalogue into "unparseable report", +/// i.e. a scan that ran fine reading as a failure. Overflow costs advisory levels, nothing more. +fn retain_rule_levels<'de, D>(d: D) -> Result, D::Error> +where + D: serde::Deserializer<'de>, +{ + struct Collapse; + + impl<'de> serde::de::Visitor<'de> for Collapse { + type Value = BTreeMap; + + fn expecting(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str("a SARIF rule catalogue") + } + + fn visit_seq(self, mut seq: A) -> Result + where + A: serde::de::SeqAccess<'de>, + { + let mut levels = BTreeMap::new(); + while let Some(rule) = seq.next_element::()? { + if levels.len() >= MAX_RULE_LEVELS { + continue; + } + let level = rule + .default_configuration + .and_then(|c| c.level) + .filter(|l| !l.trim().is_empty()); + if let (Some(id), Some(level)) = (rule.id, level) { + levels.insert(id, level); + } + } + Ok(levels) + } + } + + d.deserialize_seq(Collapse) } #[derive(Debug, Deserialize)] @@ -184,12 +271,18 @@ pub fn normalise(raw: &str, _scanner: &str, limit: usize) -> Result= limit { truncated = true; break; } - findings.push(normalise_result(result)); + findings.push(normalise_result(result, levels)); } if truncated { break; @@ -203,7 +296,21 @@ pub fn normalise(raw: &str, _scanner: &str, limit: usize) -> Result ScanFinding { +fn normalise_result( + result: &SarifResult, + rule_levels: Option<&BTreeMap>, +) -> ScanFinding { + // A result's own `level` overrides the rule's default, per SARIF 2.1.0 §3.27.10 — the rule + // default is what applies when the result is silent, which for Opengrep is always. + let level = result + .level + .clone() + .filter(|l| !l.trim().is_empty()) + .or_else(|| { + let id = result.rule_id.as_deref()?; + rule_levels?.get(id).cloned() + }); + let physical = result .locations .first() @@ -235,7 +342,7 @@ fn normalise_result(result: &SarifResult) -> ScanFinding { .and_then(|r| r.snippet.as_ref()) .and_then(|s| s.text.clone()) .filter(|t| !t.trim().is_empty()) - .unwrap_or_else(|| match (&result.rule_id, &result.level) { + .unwrap_or_else(|| match (&result.rule_id, &level) { (Some(rule), Some(level)) => format!("rule {rule} fired at {level} with no snippet"), (Some(rule), None) => format!("rule {rule} fired with no snippet"), _ => "the scanner reported no snippet".to_string(), @@ -254,7 +361,7 @@ fn normalise_result(result: &SarifResult) -> ScanFinding { line: region.and_then(|r| r.start_line), end_line: region.and_then(|r| r.end_line), rule_id: result.rule_id.clone(), - advisory_level: result.level.clone(), + advisory_level: level, } } @@ -364,6 +471,73 @@ mod tests { assert!(f.severity.is_none(), "a level must never become a score"); } + #[test] + fn a_rule_default_level_reaches_a_result_that_carries_none() { + // Opengrep's actual shape: the result is silent, the catalogue holds the severity. + let report = r#"{"runs":[{ + "tool":{"driver":{"name":"Opengrep","rules":[ + {"id":"r1","defaultConfiguration":{"level":"error"}, + "help":{"text":"a long help string that must never be retained"}}]}}, + "invocations":[{"executionSuccessful":true}], + "results":[{"ruleId":"r1","message":{"text":"m"}}]}]}"#; + let out = normalise(report, "opengrep", 10).unwrap(); + assert_eq!(out.findings[0].advisory_level.as_deref(), Some("error")); + assert!( + out.findings[0].to_finding("opengrep").severity.is_none(), + "reaching the level must not start scoring findings" + ); + } + + #[test] + fn a_results_own_level_wins_over_the_rules_default() { + let report = r#"{"runs":[{ + "tool":{"driver":{"rules":[{"id":"r1","defaultConfiguration":{"level":"note"}}]}}, + "invocations":[{"executionSuccessful":true}], + "results":[{"ruleId":"r1","level":"error","message":{"text":"m"}}]}]}"#; + let out = normalise(report, "opengrep", 10).unwrap(); + assert_eq!(out.findings[0].advisory_level.as_deref(), Some("error")); + } + + #[test] + fn a_rule_id_with_no_matching_catalogue_entry_is_simply_unlevelled() { + let report = r#"{"runs":[{ + "tool":{"driver":{"rules":[{"id":"other","defaultConfiguration":{"level":"error"}}]}}, + "invocations":[{"executionSuccessful":true}], + "results":[{"ruleId":"r1","message":{"text":"m"}}]}]}"#; + let out = normalise(report, "opengrep", 10).unwrap(); + assert!(out.findings[0].advisory_level.is_none()); + } + + #[test] + fn an_oversized_catalogue_costs_levels_and_never_the_scan() { + // The retention cap must degrade, not abort: the finding still lands. + let rules: Vec = (0..MAX_RULE_LEVELS + 50) + .map(|i| format!(r#"{{"id":"r{i}","defaultConfiguration":{{"level":"error"}}}}"#)) + .collect(); + let report = format!( + r#"{{"runs":[{{"tool":{{"driver":{{"rules":[{}]}}}}, + "invocations":[{{"executionSuccessful":true}}], + "results":[{{"ruleId":"r{}","message":{{"text":"m"}}}}]}}]}}"#, + rules.join(","), + MAX_RULE_LEVELS + 10 + ); + let out = normalise(&report, "opengrep", 10).expect("an overlong catalogue still parses"); + assert_eq!(out.findings.len(), 1); + assert!( + out.findings[0].advisory_level.is_none(), + "a rule past the cap has no retained level — but the finding survives" + ); + } + + #[test] + fn a_report_with_no_catalogue_at_all_still_normalises() { + let report = r#"{"runs":[{"invocations":[{"executionSuccessful":true}], + "results":[{"ruleId":"r1","message":{"text":"m"}}]}]}"#; + let out = normalise(report, "opengrep", 10).unwrap(); + assert_eq!(out.findings.len(), 1); + assert!(out.findings[0].advisory_level.is_none()); + } + #[test] fn a_scanner_relative_path_normalises_like_every_other_finding_path() { let report = r#"{"runs":[{"invocations":[{"executionSuccessful":true}], diff --git a/tests/fixtures/sarif/NOTES.md b/tests/fixtures/sarif/NOTES.md index 94ddcae..997e3f1 100644 --- a/tests/fixtures/sarif/NOTES.md +++ b/tests/fixtures/sarif/NOTES.md @@ -43,8 +43,12 @@ is not reviewable, and the trimmed variant exercises every field the normaliser ### Two things this fixture pins that are easy to get wrong 1. **`result.level` is absent.** Opengrep puts the severity on the *rule* - (`tool.driver.rules[].defaultConfiguration.level`), not the result. bee never materialises the - rules array (research R5), so `advisory_level` is legitimately `None` here — and that is fine, - because a scanner's level is advisory only and never becomes a `Severity` (FR-005). + (`tool.driver.rules[].defaultConfiguration.level`), not the result. A normaliser that skipped the + catalogue entirely could therefore never populate `advisory_level` at all, so `src/sarif.rs` + streams that array and retains only `id → level`, bounded by `MAX_RULE_LEVELS` — the + descriptions, help text, and tags that are the 99.96% are still never materialised (research R5). + This fixture pins that path: its one rule carries `"level": "error"`, and the finding it produces + comes out `advisory_level: Some("error")` with `severity: None`, because a scanner's level is + advisory only and never becomes a `Severity` (FR-005). 2. **`executionSuccessful: true` sits in `runs[].invocations[]`.** It, not the exit status, is what says the scan actually ran (research R6). diff --git a/tests/scanner_adapter.rs b/tests/scanner_adapter.rs index 6d1a710..db29071 100644 --- a/tests/scanner_adapter.rs +++ b/tests/scanner_adapter.rs @@ -507,12 +507,25 @@ fn the_measured_fixture_normalises_to_one_finding() { assert!(f.title.contains("shell=True")); assert!(f.evidence.contains("subprocess.call")); assert_eq!(f.class, "subprocess-shell-true"); - // Opengrep puts severity on the rule, not the result, and bee never materialises the rules - // array — so this is legitimately absent (see fixtures/sarif/NOTES.md). - assert!(f.advisory_level.is_none()); + // Opengrep puts severity on the rule, not the result. The normaliser reaches it through the + // streaming id→level pass over the catalogue (T070) rather than materialising the catalogue. + assert_eq!(f.advisory_level.as_deref(), Some("error")); assert!(!out.truncated); } +#[test] +fn a_rule_level_reaches_the_ledger_without_becoming_a_severity() { + // The whole point of populating `advisory_level`: it is carried, attributed, and still never a + // score. FR-005 holds on the path that now has something to carry. + let out = sarif::normalise(FIXTURE, "opengrep", 200).unwrap(); + let f = out.findings[0].to_finding("opengrep"); + assert_eq!(f.advisory_level.as_deref(), Some("error")); + assert!( + f.severity.is_none(), + "a scanner's own level must never become a computed severity" + ); +} + #[test] fn the_finding_set_is_bounded_before_rendering() { let results: Vec = (0..50)