fix: review follow-ups for #5 and the Swift lexical preview #8

Merged
trevon merged 9 commits from fix/review-followups into main 2026-09-25 13:33:41 +00:00
Showing only changes of commit 3a0f2adf59 - Show all commits

fix(fold): decode SARIF escapes safely, lane per run, follow absolute symlinked paths

Three import fixes from review of #5:

- sarif_uri_path sliced `&rest[i + 1..i + 3]` by byte index, so a `%`
  followed by a multibyte character (`file:///src/%aé.c`) panicked the
  whole build, and from_str_radix accepted `%+1` as byte 0x01. Escapes
  are now decoded on bytes and only from two hex digits.
- Whether findings keep Fold's Symbolic lane was decided for the whole
  report with all(): a merged SARIF log with one Fold run and one CodeQL
  run demoted every Fold finding to Imported. The lane is now decided
  per SARIF run (per report for native findings), and the diagnostic
  names only the non-Fold producers.
- The rule that `..` after a symlinked directory follows the filesystem
  applied only to relative paths; `/root/linkout/../src/main.rs` and its
  file:// form still normalized lexically onto an in-repository file.
  Absolute paths that exist now resolve through the filesystem too.

Tests (each failed before this change): percent-escape decoding without
panics, per-run lanes in a merged SARIF, and absolute/file:// paths
through a symlinked directory.
trvon 2026-09-24 20:54:36 -06:00
Signed by: trevon
GPG key ID: 38686EA797D7A8A7

View file

@ -31,16 +31,18 @@ pub fn import_findings(building: &mut Building, report: &Path) -> Result<usize>
.collect(),
None => vec![value.pointer("/tool/name").and_then(Value::as_str)],
};
let from_fold = !producers.is_empty() && producers.iter().all(|name| *name == Some("fold"));
if !from_fold {
// Decided per run, not per report: a merged SARIF log with one Fold run and one
// other tool's run must keep the Fold run's Symbolic lane.
if producers.is_empty() || producers.iter().any(|name| *name != Some("fold")) {
let named: Vec<&str> = producers
.iter()
.filter(|name| **name != Some("fold"))
.map(|name| name.unwrap_or("unnamed"))
.collect();
building.diagnostics.push(crate::model::Diagnostic {
level: "warning".into(),
message: format!(
"report {} was not produced by Fold (producer: {}); its findings are imported, never symbolic",
"report {} has findings not produced by Fold (producer: {}); those are imported, never symbolic",
report.display(),
named.join(", ")
),
@ -62,7 +64,12 @@ pub fn import_findings(building: &mut Building, report: &Path) -> Result<usize>
let (findings, unlocated) = if sarif_runs.is_some() {
sarif_findings(&value)
} else {
native_findings(&value)
let from_fold = value.pointer("/tool/name").and_then(Value::as_str) == Some("fold");
let (mut findings, unlocated) = native_findings(&value);
for finding in &mut findings {
finding.from_fold = from_fold;
}
(findings, unlocated)
};
if unlocated > 0 {
building.diagnostics.push(crate::model::Diagnostic {
@ -139,7 +146,7 @@ pub fn import_findings(building: &mut Building, report: &Path) -> Result<usize>
.unwrap_or_else(|| format!("{}:{}:{}", finding.rule, relative_path, finding.line));
let node_id = stable_id("finding", &identity);
let evidence = Evidence {
lane: fold_lane(&backend, finding.confidence.as_deref(), from_fold),
lane: fold_lane(&backend, finding.confidence.as_deref(), finding.from_fold),
backend: format!("fold:discover:{backend}"),
confidence: finding.confidence.clone().map(Confidence::Label),
provenance: format!("{}:{}", report.display(), finding.line),
@ -195,6 +202,8 @@ struct Finding {
line: usize,
confidence: Option<String>,
backend: Option<String>,
/// Produced by Fold (native report tool, or this SARIF run's driver).
from_fold: bool,
}
/// Findings plus the number dropped for lack of a file location.
@ -218,6 +227,7 @@ fn native_findings(value: &Value) -> (Vec<Finding>, usize) {
line: native_line(item),
confidence: string(item, ["confidence"]),
backend: native_backend(item),
from_fold: false,
})
})
.collect();
@ -227,22 +237,25 @@ fn native_findings(value: &Value) -> (Vec<Finding>, usize) {
/// Findings plus the number dropped for lack of a file location.
fn sarif_findings(value: &Value) -> (Vec<Finding>, usize) {
let items: Vec<&Value> = value
let items: Vec<(&Value, bool)> = value
.get("runs")
.and_then(Value::as_array)
.into_iter()
.flatten()
.flat_map(|run| {
let from_fold =
run.pointer("/tool/driver/name").and_then(Value::as_str) == Some("fold");
run.get("results")
.and_then(Value::as_array)
.into_iter()
.flatten()
.map(move |item| (item, from_fold))
})
.collect();
let total = items.len();
let findings: Vec<Finding> = items
.into_iter()
.filter_map(|item| {
.filter_map(|(item, from_fold)| {
let location = item.pointer("/locations/0/physicalLocation")?;
Some(Finding {
stable_id: item
@ -275,6 +288,7 @@ fn sarif_findings(value: &Value) -> (Vec<Finding>, usize) {
.pointer("/properties/backend")
.and_then(Value::as_str)
.map(str::to_owned),
from_fold,
})
})
.collect();
@ -293,11 +307,16 @@ fn sarif_uri_path(uri: &str) -> String {
let mut decoded = Vec::with_capacity(bytes.len());
let mut i = 0;
while i < bytes.len() {
// Decode on bytes, and only two hex digits: slicing the &str at `i + 1..i + 3`
// panicked when a multibyte character followed `%`, and from_str_radix
// accepted a leading `+`.
if bytes[i] == b'%'
&& i + 2 < bytes.len()
&& let Ok(byte) = u8::from_str_radix(&rest[i + 1..i + 3], 16)
&& let (Some(high), Some(low)) = (
bytes.get(i + 1).and_then(|b| (*b as char).to_digit(16)),
bytes.get(i + 2).and_then(|b| (*b as char).to_digit(16)),
)
{
decoded.push(byte);
decoded.push((high * 16 + low) as u8);
i += 3;
continue;
}
@ -348,7 +367,20 @@ fn native_backend(value: &Value) -> Option<String> {
fn find_file(building: &Building, path: &str) -> Option<String> {
let normalized = normalize_lexical_path(path);
let relative = if is_portable_absolute(path) {
let relative = if is_portable_absolute(path)
&& path.contains("..")
&& let (Ok(canonical), Ok(root)) = (
Path::new(path).canonicalize(),
Path::new(&building.root).canonicalize(),
) {
// As for relative paths: a `..` in a path that exists here follows the
// filesystem, so `/root/link/../x` names what the OS resolves through `link`.
canonical
.strip_prefix(&root)
.ok()?
.to_str()?
.replace('\\', "/")
} else if is_portable_absolute(path) {
let root = normalize_lexical_path(&building.root);
let prefix = format!("{}/", root.trim_end_matches('/'));
if let Some(relative) = normalized.strip_prefix(&prefix) {
@ -881,4 +913,100 @@ mod tests {
.any(|d| d.message.contains("did not match a scanned file"))
);
}
#[test]
fn sarif_percent_escapes_decode_only_hex_digits_and_never_panic() {
// `&rest[i + 1..i + 3]` sliced by byte index: a `%` before a multibyte
// character panicked on a non-char boundary, and from_str_radix accepted `+`.
use super::sarif_uri_path;
assert_eq!(sarif_uri_path("file:///src/%aé.c"), "/src/%aé.c");
assert_eq!(sarif_uri_path("file:///src/%+1.c"), "/src/%+1.c");
assert_eq!(sarif_uri_path("file:///src/a%20b.c"), "/src/a b.c");
assert_eq!(sarif_uri_path("file:///src/%C3%A9.c"), "/src/é.c");
assert_eq!(sarif_uri_path("file:///src/%4"), "/src/%4");
}
#[test]
fn merged_sarif_decides_the_symbolic_lane_per_run() {
// The lane was decided for the whole report with all(): one non-Fold run
// in a merged log demoted every Fold finding to Imported.
let dir = tempfile::tempdir().unwrap();
fs::write(dir.path().join("a.cpp"), "int a() { return 0; }\n").unwrap();
fs::write(dir.path().join("b.cpp"), "int b() { return 0; }\n").unwrap();
let result = |file: &str, rule: &str| {
serde_json::json!({"ruleId": rule, "message": {"text": "x"},
"locations": [{"physicalLocation": {"artifactLocation": {"uri": file},
"region": {"startLine": 1}}}],
"properties": {"confidence": "proved", "backend": "source"}})
};
let value = serde_json::json!({
"version": "2.1.0",
"runs": [
{"tool": {"driver": {"name": "fold"}}, "results": [result("a.cpp", "fold-rule")]},
{"tool": {"driver": {"name": "codeql"}}, "results": [result("b.cpp", "codeql-rule")]}
]
});
let report = dir.path().join("merged.sarif");
fs::write(&report, serde_json::to_vec(&value).unwrap()).unwrap();
let mut building = scan(dir.path()).unwrap();
assert_eq!(import_findings(&mut building, &report).unwrap(), 2);
let lane_for = |rule: &str| {
let node = building
.nodes
.iter()
.find(|node| node.kind == NodeKind::Finding && node.name == rule)
.unwrap();
building
.edges
.iter()
.find(|edge| edge.source == node.id && edge.kind == EdgeKind::Flags)
.unwrap()
.evidence[0]
.lane
};
assert_eq!(lane_for("fold-rule"), crate::model::EvidenceLane::Symbolic);
assert_eq!(
lane_for("codeql-rule"),
crate::model::EvidenceLane::Imported
);
}
#[cfg(unix)]
#[test]
fn absolute_parent_steps_after_a_symlinked_directory_follow_the_filesystem() {
// The filesystem rule covered only relative paths: `/root/linkout/../src/x`
// (or its file:// form) still normalized lexically into the repository.
use std::os::unix::fs::symlink;
let workspace = tempfile::tempdir().unwrap();
let root = workspace.path().join("repo");
fs::create_dir_all(root.join("src")).unwrap();
fs::write(root.join("src/main.rs"), "fn main() {}\n").unwrap();
let outside = workspace.path().join("elsewhere/deeper");
fs::create_dir_all(&outside).unwrap();
fs::create_dir_all(workspace.path().join("elsewhere/src")).unwrap();
fs::write(
workspace.path().join("elsewhere/src/main.rs"),
"fn other() {}\n",
)
.unwrap();
symlink(&outside, root.join("linkout")).unwrap();
let mut model = scan(&root).unwrap();
let absolute = format!(
"{}/linkout/../src/main.rs",
model.root.trim_end_matches('/')
);
for path in [absolute.clone(), format!("file://{absolute}")] {
let report = workspace.path().join("fold.sarif");
let value = serde_json::json!({
"version": "2.1.0",
"runs": [{"tool": {"driver": {"name": "fold"}}, "results": [{
"ruleId": "through-link", "message": {"text": "x"},
"locations": [{"physicalLocation": {"artifactLocation": {"uri": path},
"region": {"startLine": 1}}}],
"properties": {"confidence": "heuristic", "backend": "source"}}]}]
});
fs::write(&report, serde_json::to_vec(&value).unwrap()).unwrap();
assert_eq!(import_findings(&mut model, &report).unwrap(), 0, "{path}");
}
}
}