mirror of
https://github.com/imjasonh/rustvulncheck
synced 2026-07-16 20:43:49 +00:00
Fix three enrichment quality issues found by spot-checking
1. Impl body changes with no fn context: When the fn declaration is too far above for git to include in hunk headers or context lines, body changes were silently dropped. Now emits symbols at the impl-type level with a <method> placeholder (e.g. KeccakXofState::<method>). 2. PR diff uses files endpoint: Previously resolved PRs to their merge commit, which could be a version-only bump with no .rs files (e.g. quinn-proto PR #2559). Now uses /pulls/{n}/files to get all changes across the entire PR. 3. Nested generics in impl regex: The impl block regex used [^>]* which broke on nested generics like impl<T: Foo<Bar>>. Now uses .* for greedy matching to the last >. https://claude.ai/code/session_01AeNG3herWfKhos9uZVUY5d
This commit is contained in:
Claude
parent
00c14b043f
commit
4f330e94a2
2 changed files with 127 additions and 39 deletions
|
|
@ -36,8 +36,10 @@ pub fn extract_symbols(diff: &PatchDiff) -> Vec<VulnerableSymbol> {
|
|||
)
|
||||
.unwrap();
|
||||
let hunk_header_re = Regex::new(r"^@@.*@@\s*(.*)$").unwrap();
|
||||
// Match impl blocks, allowing nested generics (e.g. impl<T: Foo<Bar>> Type<T>)
|
||||
// We match `impl` then skip everything up to the last `>>` or `>` before the type name.
|
||||
let impl_re =
|
||||
Regex::new(r"impl(?:<[^>]*>)?\s+(?:([a-zA-Z_][a-zA-Z0-9_:]*)\s+for\s+)?([a-zA-Z_][a-zA-Z0-9_:<>, ]*)").unwrap();
|
||||
Regex::new(r"impl\b(?:<.*>)?\s+(?:([a-zA-Z_][a-zA-Z0-9_:]*)\s+for\s+)?([a-zA-Z_][a-zA-Z0-9_:<>, ]*)").unwrap();
|
||||
|
||||
for file_patch in &diff.files {
|
||||
// Skip test files — they aren't callable library code
|
||||
|
|
@ -113,7 +115,7 @@ pub fn extract_symbols(diff: &PatchDiff) -> Vec<VulnerableSymbol> {
|
|||
});
|
||||
}
|
||||
} else if is_added || is_removed {
|
||||
// Changed line inside a known function context
|
||||
// Changed line inside a function body
|
||||
let content = &line[1..];
|
||||
if content.trim().is_empty() || content.trim_start().starts_with("//") {
|
||||
continue;
|
||||
|
|
@ -128,6 +130,20 @@ pub fn extract_symbols(diff: &PatchDiff) -> Vec<VulnerableSymbol> {
|
|||
change_type: ChangeType::Modified,
|
||||
});
|
||||
}
|
||||
} else if current_impl_type.is_some() {
|
||||
// We have code changes inside an impl block but the fn
|
||||
// declaration is too far above for git to include it in
|
||||
// the hunk header or context. Still record the change at
|
||||
// the impl-type level rather than silently dropping it.
|
||||
let qualified =
|
||||
qualify_fn_name(&module_path, ¤t_impl_type, "<method>");
|
||||
if !symbols.iter().any(|s| s.function == qualified) {
|
||||
symbols.push(VulnerableSymbol {
|
||||
file: file_patch.filename.clone(),
|
||||
function: qualified,
|
||||
change_type: ChangeType::Modified,
|
||||
});
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
|
@ -259,29 +275,24 @@ mod tests {
|
|||
|
||||
#[test]
|
||||
fn test_consecutive_hunks_preserve_fn_context() {
|
||||
// Reproduces the xof.rs scenario: two hunks in the same function,
|
||||
// where hunk 1 header shows `fn squeeze` and hunk 2 header shows
|
||||
// `impl KeccakXofState` (git chose the impl as the nearest scope).
|
||||
// When consecutive hunks are in the same function and hunk 1 header
|
||||
// shows `fn foo`, hunk 2 (with `impl` header) should preserve context.
|
||||
let diff = PatchDiff {
|
||||
commit_sha: "6bbe15ec".to_string(),
|
||||
commit_sha: "abc123".to_string(),
|
||||
files: vec![crate::github::FilePatch {
|
||||
filename: "crates/algorithms/sha3/src/generic_keccak/xof.rs".to_string(),
|
||||
filename: "src/keccak/xof.rs".to_string(),
|
||||
patch: concat!(
|
||||
"@@ -290,6 +290,12 @@ pub(crate) fn squeeze<const PARALLEL_LANES: usize>(\n",
|
||||
" out: &mut [u8],\n",
|
||||
"+ /// NOTE: calling squeeze multiple times only gives correct output\n",
|
||||
"+ /// if all squeezed chunks (except the last) are RATE bytes long.\n",
|
||||
" ) {\n",
|
||||
"@@ -322,20 +330,25 @@ impl<const RATE: usize, const PARALLEL_LANES: usize> KeccakXofState<RATE, PARALLEL_LANES>\n",
|
||||
"@@ -100,6 +100,8 @@ pub(crate) fn squeeze(out: &mut [u8]) {\n",
|
||||
" let x = 1;\n",
|
||||
"+ let y = 2;\n",
|
||||
" }\n",
|
||||
"@@ -200,10 +202,12 @@ impl<const RATE: usize> KeccakXofState<RATE> {\n",
|
||||
" let blocks = out_len / RATE;\n",
|
||||
"- for i in 0..blocks {\n",
|
||||
"- self.inner.keccakf1600();\n",
|
||||
"+ // Extract first block without permutation\n",
|
||||
"+ self.inner.squeeze(0, RATE);\n",
|
||||
"+ for i in 1..blocks {\n",
|
||||
"+ self.inner.keccakf1600();\n",
|
||||
" }\n",
|
||||
).to_string(),
|
||||
)
|
||||
.to_string(),
|
||||
}],
|
||||
};
|
||||
|
||||
|
|
@ -293,6 +304,49 @@ mod tests {
|
|||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_impl_body_changes_without_fn_context() {
|
||||
// Reproduces the real xof.rs scenario: BOTH hunk headers show `impl`,
|
||||
// fn declaration is too far above for git to include. Changes should
|
||||
// still be captured at the impl-type level.
|
||||
let diff = PatchDiff {
|
||||
commit_sha: "6bbe15ec".to_string(),
|
||||
files: vec![crate::github::FilePatch {
|
||||
filename: "crates/algorithms/sha3/src/generic_keccak/xof.rs".to_string(),
|
||||
patch: concat!(
|
||||
"@@ -290,6 +290,12 @@ impl<const PARALLEL_LANES: usize, const RATE: usize> KeccakState<PARALLEL_LANES>\n",
|
||||
" /// Squeeze\n",
|
||||
"+/// Note that calling squeeze multiple times will only give correct\n",
|
||||
"+/// output if all sqeezed chunks are RATE bytes long.\n",
|
||||
" #[hax_lib::attributes]\n",
|
||||
"@@ -316,42 +322,38 @@ impl<const RATE: usize, STATE: KeccakItem<1>> KeccakXofState<1, RATE, STATE> {\n",
|
||||
" self.inner.keccakf1600();\n",
|
||||
" }\n",
|
||||
" \n",
|
||||
"- if out_len <= RATE {\n",
|
||||
"- self.inner.squeeze::<RATE>(out, 0, out_len);\n",
|
||||
"+ if out_len > 0 {\n",
|
||||
"+ let blocks = out_len / RATE;\n",
|
||||
"+ self.inner.squeeze::<RATE>(out, 0, RATE);\n",
|
||||
"+ for i in 1..blocks {\n",
|
||||
" }\n",
|
||||
)
|
||||
.to_string(),
|
||||
}],
|
||||
};
|
||||
|
||||
let symbols = extract_symbols(&diff);
|
||||
assert!(
|
||||
!symbols.is_empty(),
|
||||
"Expected symbols from impl body changes, got none"
|
||||
);
|
||||
assert!(
|
||||
symbols.iter().any(|s| s.function.contains("KeccakXofState")),
|
||||
"Expected KeccakXofState in symbol, got: {:?}",
|
||||
symbols
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_extract_fn_from_diff() {
|
||||
let diff = PatchDiff {
|
||||
|
|
|
|||
|
|
@ -71,27 +71,12 @@ impl GithubClient {
|
|||
repo,
|
||||
number,
|
||||
} => {
|
||||
// Get the merge commit SHA from the PR
|
||||
let url = format!(
|
||||
"https://api.github.com/repos/{}/{}/pulls/{}",
|
||||
owner, repo, number
|
||||
);
|
||||
let resp: PrResponse = self
|
||||
.get(&url)
|
||||
.send()
|
||||
.context("fetching PR metadata")?
|
||||
.error_for_status()
|
||||
.context("PR API error")?
|
||||
.json()
|
||||
.context("parsing PR response")?;
|
||||
|
||||
if let Some(sha) = resp.merge_commit_sha {
|
||||
let diff = self.fetch_commit_diff(owner, repo, &sha)?;
|
||||
Ok(Some(diff))
|
||||
} else {
|
||||
eprintln!(" PR {}/{}/pull/{} has no merge commit", owner, repo, number);
|
||||
Ok(None)
|
||||
}
|
||||
// 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))
|
||||
}
|
||||
GithubRef::Issue { .. } => {
|
||||
// Issues don't have diffs directly; skip.
|
||||
|
|
@ -100,6 +85,55 @@ 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.
|
||||
fn fetch_pr_diff(&self, owner: &str, repo: &str, number: u64) -> Result<PatchDiff> {
|
||||
// Get merge commit SHA for the entry metadata
|
||||
let pr_url = format!(
|
||||
"https://api.github.com/repos/{}/{}/pulls/{}",
|
||||
owner, repo, number
|
||||
);
|
||||
let pr_resp: PrResponse = self
|
||||
.get(&pr_url)
|
||||
.send()
|
||||
.context("fetching PR metadata")?
|
||||
.error_for_status()
|
||||
.context("PR API error")?
|
||||
.json()
|
||||
.context("parsing PR response")?;
|
||||
|
||||
let commit_sha = pr_resp
|
||||
.merge_commit_sha
|
||||
.unwrap_or_else(|| format!("pr-{}", number));
|
||||
|
||||
// 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, files })
|
||||
}
|
||||
|
||||
fn fetch_commit_diff(&self, owner: &str, repo: &str, sha: &str) -> Result<PatchDiff> {
|
||||
let url = format!(
|
||||
"https://api.github.com/repos/{}/{}/commits/{}",
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue