imjasonh/rustvulncheck
1
0
Fork 0
mirror of https://github.com/imjasonh/rustvulncheck synced 2026-07-06 19:02:25 +00:00
Code Activity

Fix non-deterministic enrichment: remove PR fallback, use merge commit diff

Browse source 
Three issues caused RUSTSEC-2026-0013 to get corrupted during CI enrichment:

1. fetch_pr_diff fell back to a fake "pr-N" commit_sha when merge_commit_sha
   was missing from the GitHub API response. Now returns an error instead —
   commit_sha must always be a real commit.

2. fetch_pr_diff used the PR files endpoint (all changes across the PR) instead
   of the merge commit's diff. This produced hundreds of false positive symbols
   from unrelated files. Now resolves the PR to its merge commit SHA and fetches
   that commit's diff directly — deterministic and scoped.

3. Re-enrich and incomplete-retry paths unconditionally overwrote existing
   entries even when the fetch failed or produced no data. Now preserves
   existing entries when fetches fail.

https://claude.ai/code/session_01RFE3T5f4nSC94KNSJfXHyb
This commit is contained in:
Claude 2026-03-26 01:06:23 +00:00
parent 8a9eb177b0
commit 4e0e8f1ecc
No known key found for this signature in database
2 changed files with 39 additions and 95 deletions
Download patch file Download diff file Expand all files Collapse all files

101
src/github.rs
View file

@ -15,12 +15,6 @@ pub struct GithubClient {
#[derive(Debug, Deserialize)]
struct PrDetailResponse {
merge_commit_sha: Option<String>,
base: Option<PrBranch>,
}
#[derive(Debug, Deserialize)]
struct PrBranch {
sha: String,
}
#[derive(Debug, Deserialize)]
@ -84,10 +78,6 @@ impl GithubClient {
repo,
number,
} => {
// Use the PR files endpoint to get all changes across the PR,
// rather than a single commit which may miss changes (e.g. if
// the merge commit is a version bump, or changes span multiple
// commits).
let diff = self.fetch_pr_diff(owner, repo, *number)?;
Ok(Some(diff))
}
@ -98,10 +88,10 @@ impl GithubClient {
}
}
/// Fetch all .rs file changes from a PR using the PR files endpoint.
/// This captures all changes across the PR, not just a single commit.
/// Resolve a PR to its merge commit, then fetch that commit's diff.
/// This gives the actual merged changeset rather than the full PR diff
/// (which can include unrelated changes from the base branch).
fn fetch_pr_diff(&self, owner: &str, repo: &str, number: u64) -> Result<PatchDiff> {
// Get merge commit SHA and base ref for the entry metadata
let pr_url = format!(
"https://api.github.com/repos/{}/{}/pulls/{}",
owner, repo, number
@ -115,81 +105,18 @@ impl GithubClient {
.json()
.context("parsing PR response")?;
let commit_sha = pr_resp
.merge_commit_sha
.unwrap_or_else(|| format!("pr-{}", number));
let merge_sha = pr_resp.merge_commit_sha.ok_or_else(|| {
anyhow::anyhow!(
"PR {}/{}/pull/{} has no merge_commit_sha (not yet merged?)",
owner,
repo,
number
)
})?;
// For the parent SHA, prefer the merge commit's first parent (the base
// branch at merge time) over `base.sha` (which reflects the *current*
// branch tip and may have drifted for old PRs).
let parent_sha = self
.fetch_commit_parent(owner, repo, &commit_sha)
.ok()
.flatten()
.or_else(|| pr_resp.base.map(|b| b.sha));
// Fetch all files changed in the PR (paginated, up to 300 files)
let files_url = format!(
"https://api.github.com/repos/{}/{}/pulls/{}/files?per_page=100",
owner, repo, number
);
let resp = self
.get(&files_url)
.send()
.context("fetching PR files")?
.error_for_status()
.context("PR files API error")?;
let pr_files: Vec<CommitFile> = resp.json().context("parsing PR files response")?;
let files = pr_files
.into_iter()
.filter(|f| f.filename.ends_with(".rs"))
.filter_map(|f| {
f.patch.map(|patch| FilePatch {
filename: f.filename,
patch,
})
})
.collect();
Ok(PatchDiff {
commit_sha,
owner: owner.to_string(),
repo: repo.to_string(),
parent_sha,
files,
})
}
/// Fetch just the first parent SHA of a commit (lightweight, no file data).
fn fetch_commit_parent(&self, owner: &str, repo: &str, sha: &str) -> Result<Option<String>> {
let url = format!(
"https://api.github.com/repos/{}/{}/commits/{}",
owner, repo, sha
);
let resp = self
.get(&url)
.send()
.context("fetching commit for parent")?
.error_for_status()
.context("commit API error")?;
#[derive(Deserialize)]
struct ParentOnly {
parents: Option<Vec<CommitParentRef>>,
}
#[derive(Deserialize)]
struct CommitParentRef {
sha: String,
}
let commit: ParentOnly = resp.json().context("parsing commit for parent")?;
Ok(commit
.parents
.as_ref()
.and_then(|p| p.first())
.map(|p| p.sha.clone()))
// Use the merge commit's diff directly — this is deterministic and
// scoped to exactly the changes that were merged.
self.fetch_commit_diff(owner, repo, &merge_sha)
}
fn fetch_commit_diff(&self, owner: &str, repo: &str, sha: &str) -> Result<PatchDiff> {

33
src/main.rs
View file

@ -237,6 +237,7 @@ fn run_enrich(args: EnrichArgs) -> Result<()> {
let mut new_symbols = Vec::new();
let mut new_sha = entry.commit_sha.clone();
let mut fetched = false;
for gh_ref in &gh_refs {
match gh.fetch_diff(gh_ref) {
Ok(Some(diff)) => {
@ -247,6 +248,7 @@ fn run_enrich(args: EnrichArgs) -> Result<()> {
);
new_sha = Some(diff.commit_sha.clone());
new_symbols = extract_symbols(&diff, &gh, &adv.package);
fetched = true;
break;
}
Ok(None) => {
@ -258,6 +260,13 @@ fn run_enrich(args: EnrichArgs) -> Result<()> {
}
}
if !fetched {
println!(" Could not fetch any diff, keeping existing entry unchanged");
re_enriched_count += 1;
println!();
continue;
}
let new_symbol_count = new_symbols.len();
if new_symbol_count != old_symbol_count {
println!(
@ -376,6 +385,7 @@ fn run_enrich(args: EnrichArgs) -> Result<()> {
};
// Try each ref until we get a diff
let mut fetched = false;
for gh_ref in &gh_refs {
match gh.fetch_diff(gh_ref) {
Ok(Some(diff)) => {
@ -395,6 +405,7 @@ fn run_enrich(args: EnrichArgs) -> Result<()> {
}
}
entry.vulnerable_symbols = symbols;
fetched = true;
break;
}
Ok(None) => {
@ -406,17 +417,23 @@ fn run_enrich(args: EnrichArgs) -> Result<()> {
}
}
// If this is an incomplete entry being retried, update it in-place
// If this is an incomplete entry being retried, only update if we got new data
if incomplete_ids.contains(&adv.id) {
if let Some(existing) = vuln_db
.entries
.iter_mut()
.find(|e| e.advisory_id == adv.id)
{
existing.commit_sha = entry.commit_sha;
existing.vulnerable_symbols = entry.vulnerable_symbols;
if fetched {
if let Some(existing) = vuln_db
.entries
.iter_mut()
.find(|e| e.advisory_id == adv.id)
{
existing.commit_sha = entry.commit_sha;
existing.vulnerable_symbols = entry.vulnerable_symbols;
}
} else {
println!(" Could not fetch any diff, keeping existing entry unchanged");
}
} else {
// New entry: always add to DB (even without symbols) so it's tracked.
// It will be retried via the incomplete-entry path on future runs.
vuln_db.entries.push(entry);
}
new_count += 1;