mirror of
https://github.com/imjasonh/rustvulncheck
synced 2026-07-08 04:08:07 +00:00
Add AST-based symbol extraction using syn, with regex fallback
Instead of regex-matching unified diff lines, the enrichment pipeline now fetches the full before/after file contents from GitHub, parses them with syn::parse_file, walks the AST to extract all fn items with their fully-qualified paths (including impl block context), and diffs the two symbol sets to classify Added/Modified/Deleted. This eliminates entire classes of regex bugs: - impl Trait for Type: syn's ItemImpl has separate trait_ and self_ty fields, so the implementing type is always correct - where clauses: parsed into generics.where_clause, never in the type - nested generics: syn handles all nesting correctly - #[cfg(test)] modules: checked via attributes, not path heuristics - Duplicate symbols: set-based diffing produces each symbol exactly once Falls back to regex per-file when: - AST parsing fails (syntax errors, macro-heavy files) - File contents can't be fetched (missing metadata, API errors) - File didn't exist at either ref (handled as all-Added or all-Deleted) New files: - src/ast_differ.rs: AST visitor + symbol set diffing (8 tests) Changes: - src/github.rs: Added parent_sha, owner, repo to PatchDiff; added fetch_file_contents() for raw file retrieval - src/diff_analyzer.rs: extract_symbols() now tries AST first; regex path renamed to extract_symbols_regex() - Cargo.toml: Added quote dependency for token stream hashing https://claude.ai/code/session_01P1LKP6aqGt68rQAXrF6kSE
This commit is contained in:
parent
51945d6b42
commit
6740de1687
6 changed files with 673 additions and 63 deletions
1
Cargo.lock
generated
1
Cargo.lock
generated
|
|
@ -104,6 +104,7 @@ dependencies = [
|
|||
"anyhow",
|
||||
"clap",
|
||||
"git2",
|
||||
"quote",
|
||||
"regex",
|
||||
"reqwest",
|
||||
"semver",
|
||||
|
|
|
|||
|
|
@ -15,6 +15,7 @@ walkdir = "2"
|
|||
anyhow = "1"
|
||||
semver = "1"
|
||||
syn = { version = "2", features = ["full", "visit"] }
|
||||
quote = "1"
|
||||
|
||||
[dev-dependencies]
|
||||
tempfile = "3"
|
||||
|
|
|
|||
460
src/ast_differ.rs
Normal file
460
src/ast_differ.rs
Normal file
|
|
@ -0,0 +1,460 @@
|
|||
//! AST-based symbol extraction using `syn`.
|
||||
//!
|
||||
//! Instead of regex-matching unified diff lines, this module parses
|
||||
//! the before/after Rust source files into ASTs and diffs the function
|
||||
//! sets to precisely identify Added, Modified, and Deleted symbols.
|
||||
|
||||
use std::collections::{HashMap, HashSet};
|
||||
|
||||
use syn::visit::Visit;
|
||||
use syn::{ItemFn, ItemImpl, ItemMod, ImplItemFn};
|
||||
|
||||
use crate::diff_analyzer::{ChangeType, VulnerableSymbol};
|
||||
|
||||
/// A function symbol extracted from a parsed AST.
|
||||
#[derive(Debug, Clone)]
|
||||
struct FnSymbol {
|
||||
/// Fully qualified name: `module::Type::method` or `module::free_fn`
|
||||
qualified_name: String,
|
||||
/// Hash of the function body tokens for detecting modifications
|
||||
body_hash: u64,
|
||||
}
|
||||
|
||||
/// Extract all function symbols from a Rust source string.
|
||||
///
|
||||
/// Parses the source with `syn::parse_file` and walks the AST to find
|
||||
/// all `fn` items, including methods inside `impl` blocks. Returns
|
||||
/// `None` if parsing fails.
|
||||
pub fn extract_fn_symbols(source: &str, module_path: &str) -> Option<Vec<FnSymbol>> {
|
||||
let file = syn::parse_file(source).ok()?;
|
||||
let mut visitor = SymbolVisitor {
|
||||
module_path: module_path.to_string(),
|
||||
impl_type_stack: Vec::new(),
|
||||
in_test: false,
|
||||
symbols: Vec::new(),
|
||||
};
|
||||
visitor.visit_file(&file);
|
||||
Some(visitor.symbols)
|
||||
}
|
||||
|
||||
/// Diff two sets of function symbols to produce VulnerableSymbol entries.
|
||||
///
|
||||
/// - Functions in `after` but not `before`: Added
|
||||
/// - Functions in `before` but not `after`: Deleted
|
||||
/// - Functions in both but with different body hashes: Modified
|
||||
/// - Functions unchanged: omitted
|
||||
pub fn diff_symbols(
|
||||
before: &[FnSymbol],
|
||||
after: &[FnSymbol],
|
||||
file_path: &str,
|
||||
) -> Vec<VulnerableSymbol> {
|
||||
let before_map: HashMap<&str, &FnSymbol> = before
|
||||
.iter()
|
||||
.map(|s| (s.qualified_name.as_str(), s))
|
||||
.collect();
|
||||
let after_map: HashMap<&str, &FnSymbol> = after
|
||||
.iter()
|
||||
.map(|s| (s.qualified_name.as_str(), s))
|
||||
.collect();
|
||||
|
||||
let before_names: HashSet<&str> = before_map.keys().copied().collect();
|
||||
let after_names: HashSet<&str> = after_map.keys().copied().collect();
|
||||
|
||||
let mut results = Vec::new();
|
||||
|
||||
// Added: in after but not before
|
||||
for name in after_names.difference(&before_names) {
|
||||
results.push(VulnerableSymbol {
|
||||
file: file_path.to_string(),
|
||||
function: name.to_string(),
|
||||
change_type: ChangeType::Added,
|
||||
});
|
||||
}
|
||||
|
||||
// Deleted: in before but not after
|
||||
for name in before_names.difference(&after_names) {
|
||||
results.push(VulnerableSymbol {
|
||||
file: file_path.to_string(),
|
||||
function: name.to_string(),
|
||||
change_type: ChangeType::Deleted,
|
||||
});
|
||||
}
|
||||
|
||||
// Modified: in both but with different body hash
|
||||
for name in before_names.intersection(&after_names) {
|
||||
let b = before_map[name];
|
||||
let a = after_map[name];
|
||||
if b.body_hash != a.body_hash {
|
||||
results.push(VulnerableSymbol {
|
||||
file: file_path.to_string(),
|
||||
function: name.to_string(),
|
||||
change_type: ChangeType::Modified,
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
results
|
||||
}
|
||||
|
||||
/// Extract symbols from before/after source and diff them.
|
||||
/// Returns `None` if either file fails to parse.
|
||||
pub fn ast_diff_symbols(
|
||||
before_source: Option<&str>,
|
||||
after_source: Option<&str>,
|
||||
module_path: &str,
|
||||
file_path: &str,
|
||||
) -> Option<Vec<VulnerableSymbol>> {
|
||||
match (before_source, after_source) {
|
||||
(Some(before), Some(after)) => {
|
||||
let before_syms = extract_fn_symbols(before, module_path)?;
|
||||
let after_syms = extract_fn_symbols(after, module_path)?;
|
||||
Some(diff_symbols(&before_syms, &after_syms, file_path))
|
||||
}
|
||||
(None, Some(after)) => {
|
||||
// New file — all functions are Added
|
||||
let after_syms = extract_fn_symbols(after, module_path)?;
|
||||
Some(
|
||||
after_syms
|
||||
.into_iter()
|
||||
.map(|s| VulnerableSymbol {
|
||||
file: file_path.to_string(),
|
||||
function: s.qualified_name,
|
||||
change_type: ChangeType::Added,
|
||||
})
|
||||
.collect(),
|
||||
)
|
||||
}
|
||||
(Some(before), None) => {
|
||||
// Deleted file — all functions are Deleted
|
||||
let before_syms = extract_fn_symbols(before, module_path)?;
|
||||
Some(
|
||||
before_syms
|
||||
.into_iter()
|
||||
.map(|s| VulnerableSymbol {
|
||||
file: file_path.to_string(),
|
||||
function: s.qualified_name,
|
||||
change_type: ChangeType::Deleted,
|
||||
})
|
||||
.collect(),
|
||||
)
|
||||
}
|
||||
(None, None) => Some(Vec::new()),
|
||||
}
|
||||
}
|
||||
|
||||
// ── AST Visitor ──────────────────────────────────────────────────────
|
||||
|
||||
struct SymbolVisitor {
|
||||
module_path: String,
|
||||
/// Stack of impl type names (for nested impls, though rare in practice)
|
||||
impl_type_stack: Vec<String>,
|
||||
/// Whether we're inside a #[cfg(test)] module
|
||||
in_test: bool,
|
||||
symbols: Vec<FnSymbol>,
|
||||
}
|
||||
|
||||
impl SymbolVisitor {
|
||||
/// Build the fully qualified name for a function.
|
||||
fn qualify(&self, fn_name: &str) -> String {
|
||||
let mut parts: Vec<&str> = Vec::new();
|
||||
if !self.module_path.is_empty() {
|
||||
parts.push(&self.module_path);
|
||||
}
|
||||
if let Some(impl_type) = self.impl_type_stack.last() {
|
||||
parts.push(impl_type);
|
||||
}
|
||||
parts.push(fn_name);
|
||||
parts.join("::")
|
||||
}
|
||||
|
||||
/// Check if an item has #[test] or #[cfg(test)] attributes.
|
||||
fn has_test_attr(attrs: &[syn::Attribute]) -> bool {
|
||||
for attr in attrs {
|
||||
if attr.path().is_ident("test") {
|
||||
return true;
|
||||
}
|
||||
if attr.path().is_ident("cfg") {
|
||||
// Check for #[cfg(test)]
|
||||
let tokens = attr.meta.to_token_stream().to_string();
|
||||
if tokens.contains("test") {
|
||||
return true;
|
||||
}
|
||||
}
|
||||
}
|
||||
false
|
||||
}
|
||||
|
||||
/// Extract a simple type name from a syn::Type, stripping generics.
|
||||
fn type_name(ty: &syn::Type) -> Option<String> {
|
||||
match ty {
|
||||
syn::Type::Path(type_path) => {
|
||||
// Use the last segment's ident (e.g., for `std::convert::From<T>` → `From`,
|
||||
// but for `MyStruct<T>` → `MyStruct`)
|
||||
let last = type_path.path.segments.last()?;
|
||||
Some(last.ident.to_string())
|
||||
}
|
||||
syn::Type::Reference(type_ref) => Self::type_name(&type_ref.elem),
|
||||
_ => None,
|
||||
}
|
||||
}
|
||||
|
||||
/// Hash function body tokens for modification detection.
|
||||
fn hash_body(block: &syn::Block) -> u64 {
|
||||
use std::hash::{Hash, Hasher};
|
||||
// Use the token stream string as the hash input.
|
||||
// This normalizes whitespace but preserves semantics.
|
||||
let tokens = block.to_token_stream().to_string();
|
||||
let mut hasher = std::collections::hash_map::DefaultHasher::new();
|
||||
tokens.hash(&mut hasher);
|
||||
hasher.finish()
|
||||
}
|
||||
|
||||
fn record_fn(&mut self, name: &str, attrs: &[syn::Attribute], body: &syn::Block) {
|
||||
// Skip test functions
|
||||
if Self::has_test_attr(attrs) || name.starts_with("test_") {
|
||||
return;
|
||||
}
|
||||
if self.in_test {
|
||||
return;
|
||||
}
|
||||
|
||||
self.symbols.push(FnSymbol {
|
||||
qualified_name: self.qualify(name),
|
||||
body_hash: Self::hash_body(body),
|
||||
});
|
||||
}
|
||||
}
|
||||
|
||||
impl<'ast> Visit<'ast> for SymbolVisitor {
|
||||
fn visit_item_fn(&mut self, node: &'ast ItemFn) {
|
||||
let name = node.sig.ident.to_string();
|
||||
self.record_fn(&name, &node.attrs, &node.block);
|
||||
// Don't recurse into nested items — we handle those at the top level
|
||||
}
|
||||
|
||||
fn visit_item_impl(&mut self, node: &'ast ItemImpl) {
|
||||
// For `impl Trait for Type`, use Type (self_ty), not Trait.
|
||||
// For `impl Type`, also use self_ty. syn gives us exactly the right field.
|
||||
if let Some(type_name) = Self::type_name(&node.self_ty) {
|
||||
self.impl_type_stack.push(type_name);
|
||||
|
||||
// Visit each method in the impl block
|
||||
for item in &node.items {
|
||||
if let syn::ImplItem::Fn(method) = item {
|
||||
self.visit_impl_item_fn(method);
|
||||
}
|
||||
}
|
||||
|
||||
self.impl_type_stack.pop();
|
||||
}
|
||||
// Don't call the default visit — we manually visited methods above
|
||||
}
|
||||
|
||||
fn visit_impl_item_fn(&mut self, node: &'ast ImplItemFn) {
|
||||
let name = node.sig.ident.to_string();
|
||||
self.record_fn(&name, &node.attrs, &node.block);
|
||||
}
|
||||
|
||||
fn visit_item_mod(&mut self, node: &'ast ItemMod) {
|
||||
// Skip #[cfg(test)] modules entirely
|
||||
if Self::has_test_attr(&node.attrs) {
|
||||
return;
|
||||
}
|
||||
|
||||
// Recurse into inline modules (those with `mod foo { ... }`)
|
||||
if let Some((_, items)) = &node.content {
|
||||
let old_module = self.module_path.clone();
|
||||
let mod_name = node.ident.to_string();
|
||||
self.module_path = if self.module_path.is_empty() {
|
||||
mod_name
|
||||
} else {
|
||||
format!("{}::{}", self.module_path, mod_name)
|
||||
};
|
||||
|
||||
for item in items {
|
||||
self.visit_item(item);
|
||||
}
|
||||
|
||||
self.module_path = old_module;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
use quote::ToTokens;
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use super::*;
|
||||
|
||||
#[test]
|
||||
fn test_extract_simple_fn() {
|
||||
let source = r#"
|
||||
pub fn hello() {
|
||||
println!("hello");
|
||||
}
|
||||
"#;
|
||||
let symbols = extract_fn_symbols(source, "mymod").unwrap();
|
||||
assert_eq!(symbols.len(), 1);
|
||||
assert_eq!(symbols[0].qualified_name, "mymod::hello");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_extract_impl_methods() {
|
||||
let source = r#"
|
||||
struct Request;
|
||||
impl Request {
|
||||
pub fn parse(buf: &[u8]) -> Self { Request }
|
||||
fn internal() {}
|
||||
}
|
||||
"#;
|
||||
let symbols = extract_fn_symbols(source, "http::request").unwrap();
|
||||
assert_eq!(symbols.len(), 2);
|
||||
let names: Vec<&str> = symbols.iter().map(|s| s.qualified_name.as_str()).collect();
|
||||
assert!(names.contains(&"http::request::Request::parse"));
|
||||
assert!(names.contains(&"http::request::Request::internal"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_trait_impl_uses_type_not_trait() {
|
||||
// This is the key test — impl Trait for Type should use Type
|
||||
let source = r#"
|
||||
struct PyObject;
|
||||
impl<T> From<T> for PyObject {
|
||||
fn from(val: T) -> Self { PyObject }
|
||||
}
|
||||
"#;
|
||||
let symbols = extract_fn_symbols(source, "instance").unwrap();
|
||||
assert_eq!(symbols.len(), 1);
|
||||
assert_eq!(
|
||||
symbols[0].qualified_name, "instance::PyObject::from",
|
||||
"Should use implementing type (PyObject), not trait (From)"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_complex_generics_no_leak() {
|
||||
// impl<T> From<Py<T>> for PyObject where T: AsRef<PyAny>
|
||||
let source = r#"
|
||||
struct PyObject;
|
||||
struct Py<T>(T);
|
||||
trait AsRef<T> {}
|
||||
impl<T> From<Py<T>> for PyObject where T: AsRef<PyObject> {
|
||||
fn from(val: Py<T>) -> Self { PyObject }
|
||||
}
|
||||
"#;
|
||||
let symbols = extract_fn_symbols(source, "instance").unwrap();
|
||||
assert_eq!(symbols.len(), 1);
|
||||
let name = &symbols[0].qualified_name;
|
||||
assert!(
|
||||
!name.contains("for "),
|
||||
"No 'for' leak: got '{}'", name
|
||||
);
|
||||
assert!(
|
||||
!name.contains("where"),
|
||||
"No 'where' leak: got '{}'", name
|
||||
);
|
||||
assert_eq!(name, "instance::PyObject::from");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_test_functions_skipped() {
|
||||
let source = r#"
|
||||
pub fn real_function() {}
|
||||
|
||||
#[test]
|
||||
fn test_something() {}
|
||||
|
||||
fn test_other_thing() {}
|
||||
"#;
|
||||
let symbols = extract_fn_symbols(source, "").unwrap();
|
||||
assert_eq!(symbols.len(), 1);
|
||||
assert_eq!(symbols[0].qualified_name, "real_function");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_cfg_test_module_skipped() {
|
||||
let source = r#"
|
||||
pub fn real_function() {}
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
fn helper() {}
|
||||
#[test]
|
||||
fn test_it() {}
|
||||
}
|
||||
"#;
|
||||
let symbols = extract_fn_symbols(source, "mymod").unwrap();
|
||||
assert_eq!(symbols.len(), 1);
|
||||
assert_eq!(symbols[0].qualified_name, "mymod::real_function");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_diff_added_deleted_modified() {
|
||||
let before = vec![
|
||||
FnSymbol { qualified_name: "foo".into(), body_hash: 100 },
|
||||
FnSymbol { qualified_name: "bar".into(), body_hash: 200 },
|
||||
FnSymbol { qualified_name: "old_fn".into(), body_hash: 300 },
|
||||
];
|
||||
let after = vec![
|
||||
FnSymbol { qualified_name: "foo".into(), body_hash: 100 }, // unchanged
|
||||
FnSymbol { qualified_name: "bar".into(), body_hash: 999 }, // modified
|
||||
FnSymbol { qualified_name: "new_fn".into(), body_hash: 400 }, // added
|
||||
];
|
||||
|
||||
let result = diff_symbols(&before, &after, "src/lib.rs");
|
||||
|
||||
let added: Vec<_> = result.iter().filter(|s| matches!(s.change_type, ChangeType::Added)).collect();
|
||||
let deleted: Vec<_> = result.iter().filter(|s| matches!(s.change_type, ChangeType::Deleted)).collect();
|
||||
let modified: Vec<_> = result.iter().filter(|s| matches!(s.change_type, ChangeType::Modified)).collect();
|
||||
|
||||
assert_eq!(added.len(), 1);
|
||||
assert_eq!(added[0].function, "new_fn");
|
||||
|
||||
assert_eq!(deleted.len(), 1);
|
||||
assert_eq!(deleted[0].function, "old_fn");
|
||||
|
||||
assert_eq!(modified.len(), 1);
|
||||
assert_eq!(modified[0].function, "bar");
|
||||
|
||||
// foo is unchanged — should NOT appear
|
||||
assert!(!result.iter().any(|s| s.function == "foo"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_ast_diff_end_to_end() {
|
||||
let before_src = r#"
|
||||
struct Decoder;
|
||||
impl Decoder {
|
||||
pub fn decode(buf: &[u8]) -> Self { Decoder }
|
||||
fn old_helper() {}
|
||||
}
|
||||
"#;
|
||||
let after_src = r#"
|
||||
struct Decoder;
|
||||
impl Decoder {
|
||||
pub fn decode(buf: &[u8]) -> Self {
|
||||
let x = 1;
|
||||
Decoder
|
||||
}
|
||||
fn new_helper() {}
|
||||
}
|
||||
"#;
|
||||
|
||||
let result = ast_diff_symbols(
|
||||
Some(before_src),
|
||||
Some(after_src),
|
||||
"proto::h1",
|
||||
"src/proto/h1/decode.rs",
|
||||
)
|
||||
.unwrap();
|
||||
|
||||
let names: Vec<(&str, &ChangeType)> = result
|
||||
.iter()
|
||||
.map(|s| (s.function.as_str(), &s.change_type))
|
||||
.collect();
|
||||
|
||||
assert!(names.iter().any(|(n, t)| *n == "proto::h1::Decoder::decode" && matches!(t, ChangeType::Modified)));
|
||||
assert!(names.iter().any(|(n, t)| *n == "proto::h1::Decoder::old_helper" && matches!(t, ChangeType::Deleted)));
|
||||
assert!(names.iter().any(|(n, t)| *n == "proto::h1::Decoder::new_helper" && matches!(t, ChangeType::Added)));
|
||||
}
|
||||
}
|
||||
|
|
@ -22,14 +22,82 @@ pub enum ChangeType {
|
|||
Deleted,
|
||||
}
|
||||
|
||||
/// Extract function signatures from a patch diff.
|
||||
/// Extract function signatures using AST parsing where possible, with regex fallback.
|
||||
///
|
||||
/// Strategy:
|
||||
/// 1. Look at unified diff hunk headers (`@@ ... @@ fn ...`) which often contain
|
||||
/// the enclosing function name.
|
||||
/// 2. Look at added/removed lines containing `fn ` declarations.
|
||||
/// 3. Infer module path from the file path.
|
||||
pub fn extract_symbols(diff: &PatchDiff) -> Vec<VulnerableSymbol> {
|
||||
/// For each changed `.rs` file, fetches the full before/after contents from GitHub,
|
||||
/// parses them with `syn`, and diffs the function sets to find Added/Modified/Deleted
|
||||
/// symbols. Falls back to regex-based extraction per-file when AST parsing fails
|
||||
/// (syntax errors, macro-heavy files) or when file contents can't be fetched.
|
||||
pub fn extract_symbols(
|
||||
diff: &PatchDiff,
|
||||
gh: &crate::github::GithubClient,
|
||||
) -> Vec<VulnerableSymbol> {
|
||||
let mut all_symbols = Vec::new();
|
||||
let has_metadata = !diff.owner.is_empty() && !diff.repo.is_empty();
|
||||
|
||||
for file_patch in &diff.files {
|
||||
if is_test_file(&file_patch.filename) {
|
||||
continue;
|
||||
}
|
||||
|
||||
let module_path = file_path_to_module(&file_patch.filename);
|
||||
|
||||
// Try AST-based extraction if we have the metadata to fetch files
|
||||
if has_metadata {
|
||||
if let Some(parent) = &diff.parent_sha {
|
||||
let before = gh
|
||||
.fetch_file_contents(
|
||||
&diff.owner,
|
||||
&diff.repo,
|
||||
&file_patch.filename,
|
||||
parent,
|
||||
)
|
||||
.ok()
|
||||
.flatten();
|
||||
|
||||
let after = gh
|
||||
.fetch_file_contents(
|
||||
&diff.owner,
|
||||
&diff.repo,
|
||||
&file_patch.filename,
|
||||
&diff.commit_sha,
|
||||
)
|
||||
.ok()
|
||||
.flatten();
|
||||
|
||||
if let Some(ast_symbols) = crate::ast_differ::ast_diff_symbols(
|
||||
before.as_deref(),
|
||||
after.as_deref(),
|
||||
&module_path,
|
||||
&file_patch.filename,
|
||||
) {
|
||||
if !ast_symbols.is_empty() || before.is_some() || after.is_some() {
|
||||
// AST succeeded — use its results (even if empty means no changes)
|
||||
all_symbols.extend(ast_symbols);
|
||||
continue;
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
// Fallback: regex-based extraction for this file
|
||||
let file_diff = PatchDiff {
|
||||
commit_sha: diff.commit_sha.clone(),
|
||||
owner: String::new(),
|
||||
repo: String::new(),
|
||||
parent_sha: None,
|
||||
files: vec![file_patch.clone()],
|
||||
};
|
||||
all_symbols.extend(extract_symbols_regex(&file_diff));
|
||||
}
|
||||
|
||||
all_symbols
|
||||
}
|
||||
|
||||
/// Regex-based fallback for extracting function signatures from unified diffs.
|
||||
///
|
||||
/// Used when AST parsing fails or file contents can't be fetched.
|
||||
pub(crate) fn extract_symbols_regex(diff: &PatchDiff) -> Vec<VulnerableSymbol> {
|
||||
let mut symbols: Vec<VulnerableSymbol> = Vec::new();
|
||||
let fn_decl_re = Regex::new(
|
||||
r"(?:pub\s+(?:\(crate\)\s+)?)?(?:unsafe\s+)?(?:async\s+)?fn\s+([a-zA-Z_][a-zA-Z0-9_]*)",
|
||||
|
|
@ -507,9 +575,8 @@ mod tests {
|
|||
fn test_consecutive_hunks_preserve_fn_context() {
|
||||
// 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: "abc123".to_string(),
|
||||
files: vec![crate::github::FilePatch {
|
||||
let diff = PatchDiff::for_test("abc123",
|
||||
vec![crate::github::FilePatch {
|
||||
filename: "src/keccak/xof.rs".to_string(),
|
||||
patch: concat!(
|
||||
"@@ -100,6 +100,8 @@ pub(crate) fn squeeze(out: &mut [u8]) {\n",
|
||||
|
|
@ -524,9 +591,9 @@ mod tests {
|
|||
)
|
||||
.to_string(),
|
||||
}],
|
||||
};
|
||||
);
|
||||
|
||||
let symbols = extract_symbols(&diff);
|
||||
let symbols = extract_symbols_regex(&diff);
|
||||
assert!(
|
||||
symbols.iter().any(|s| s.function.contains("squeeze")),
|
||||
"Expected squeeze to be found, got: {:?}",
|
||||
|
|
@ -539,9 +606,8 @@ mod tests {
|
|||
// 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 {
|
||||
let diff = PatchDiff::for_test("6bbe15ec",
|
||||
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",
|
||||
|
|
@ -563,9 +629,9 @@ mod tests {
|
|||
)
|
||||
.to_string(),
|
||||
}],
|
||||
};
|
||||
);
|
||||
|
||||
let symbols = extract_symbols(&diff);
|
||||
let symbols = extract_symbols_regex(&diff);
|
||||
assert!(
|
||||
!symbols.is_empty(),
|
||||
"Expected symbols from impl body changes, got none"
|
||||
|
|
@ -579,9 +645,8 @@ mod tests {
|
|||
|
||||
#[test]
|
||||
fn test_extract_fn_from_diff() {
|
||||
let diff = PatchDiff {
|
||||
commit_sha: "abc123".to_string(),
|
||||
files: vec![crate::github::FilePatch {
|
||||
let diff = PatchDiff::for_test("abc123",
|
||||
vec![crate::github::FilePatch {
|
||||
filename: "src/http/request.rs".to_string(),
|
||||
patch: r#"@@ -10,6 +10,8 @@ impl Request {
|
||||
pub fn parse(buf: &[u8]) -> Result<Self> {
|
||||
|
|
@ -592,9 +657,9 @@ mod tests {
|
|||
"#
|
||||
.to_string(),
|
||||
}],
|
||||
};
|
||||
);
|
||||
|
||||
let symbols = extract_symbols(&diff);
|
||||
let symbols = extract_symbols_regex(&diff);
|
||||
assert!(!symbols.is_empty());
|
||||
assert!(symbols.iter().any(|s| s.function.contains("parse")));
|
||||
}
|
||||
|
|
@ -647,9 +712,8 @@ mod tests {
|
|||
#[test]
|
||||
fn test_for_keyword_no_longer_leaks_into_symbol() {
|
||||
// Reproduces the pyo3 RUSTSEC-2020-0074 bug
|
||||
let diff = PatchDiff {
|
||||
commit_sha: "abc123".to_string(),
|
||||
files: vec![crate::github::FilePatch {
|
||||
let diff = PatchDiff::for_test("abc123",
|
||||
vec![crate::github::FilePatch {
|
||||
filename: "src/instance.rs".to_string(),
|
||||
patch: concat!(
|
||||
"@@ -495,7 +495,9 @@ impl<T> std::convert::From<Py<T>> for PyObject\n",
|
||||
|
|
@ -660,9 +724,9 @@ mod tests {
|
|||
)
|
||||
.to_string(),
|
||||
}],
|
||||
};
|
||||
);
|
||||
|
||||
let symbols = extract_symbols(&diff);
|
||||
let symbols = extract_symbols_regex(&diff);
|
||||
for s in &symbols {
|
||||
assert!(
|
||||
!s.function.contains("for "),
|
||||
|
|
@ -681,9 +745,8 @@ mod tests {
|
|||
#[test]
|
||||
fn test_where_clause_no_longer_leaks_into_symbol() {
|
||||
// Reproduces the lock_api RUSTSEC-2020-0070 bug
|
||||
let diff = PatchDiff {
|
||||
commit_sha: "abc123".to_string(),
|
||||
files: vec![crate::github::FilePatch {
|
||||
let diff = PatchDiff::for_test("abc123",
|
||||
vec![crate::github::FilePatch {
|
||||
filename: "lock_api/src/mutex.rs".to_string(),
|
||||
patch: concat!(
|
||||
"@@ -100,6 +100,8 @@ impl<R: RawMutex, T: ?Sized> Mutex<R, T> where R: Send {\n",
|
||||
|
|
@ -692,9 +755,9 @@ mod tests {
|
|||
)
|
||||
.to_string(),
|
||||
}],
|
||||
};
|
||||
);
|
||||
|
||||
let symbols = extract_symbols(&diff);
|
||||
let symbols = extract_symbols_regex(&diff);
|
||||
for s in &symbols {
|
||||
assert!(
|
||||
!s.function.contains("where"),
|
||||
|
|
@ -714,9 +777,8 @@ mod tests {
|
|||
#[test]
|
||||
fn test_test_functions_filtered_by_attribute() {
|
||||
// #[test] functions in library source should be skipped
|
||||
let diff = PatchDiff {
|
||||
commit_sha: "abc123".to_string(),
|
||||
files: vec![crate::github::FilePatch {
|
||||
let diff = PatchDiff::for_test("abc123",
|
||||
vec![crate::github::FilePatch {
|
||||
filename: "src/pycell/impl_.rs".to_string(),
|
||||
patch: concat!(
|
||||
"@@ -100,6 +100,12 @@ impl PyCell {\n",
|
||||
|
|
@ -731,9 +793,9 @@ mod tests {
|
|||
)
|
||||
.to_string(),
|
||||
}],
|
||||
};
|
||||
);
|
||||
|
||||
let symbols = extract_symbols(&diff);
|
||||
let symbols = extract_symbols_regex(&diff);
|
||||
assert!(
|
||||
!symbols.iter().any(|s| s.function.contains("test_inherited")),
|
||||
"test_ functions should be filtered out, got: {:?}",
|
||||
|
|
@ -750,9 +812,8 @@ mod tests {
|
|||
#[test]
|
||||
fn test_test_functions_filtered_by_name_prefix() {
|
||||
// test_ prefix functions should be filtered even without #[test] attribute
|
||||
let diff = PatchDiff {
|
||||
commit_sha: "abc123".to_string(),
|
||||
files: vec![crate::github::FilePatch {
|
||||
let diff = PatchDiff::for_test("abc123",
|
||||
vec![crate::github::FilePatch {
|
||||
filename: "src/lib.rs".to_string(),
|
||||
patch: concat!(
|
||||
"@@ -10,6 +10,8 @@\n",
|
||||
|
|
@ -765,9 +826,9 @@ mod tests {
|
|||
)
|
||||
.to_string(),
|
||||
}],
|
||||
};
|
||||
);
|
||||
|
||||
let symbols = extract_symbols(&diff);
|
||||
let symbols = extract_symbols_regex(&diff);
|
||||
assert!(
|
||||
!symbols.iter().any(|s| s.function.contains("test_combine")),
|
||||
"test_ prefix functions should be filtered, got: {:?}",
|
||||
|
|
@ -783,9 +844,8 @@ mod tests {
|
|||
#[test]
|
||||
fn test_cfg_test_functions_filtered() {
|
||||
// Functions after #[cfg(test)] should be filtered
|
||||
let diff = PatchDiff {
|
||||
commit_sha: "abc123".to_string(),
|
||||
files: vec![crate::github::FilePatch {
|
||||
let diff = PatchDiff::for_test("abc123",
|
||||
vec![crate::github::FilePatch {
|
||||
filename: "src/core.rs".to_string(),
|
||||
patch: concat!(
|
||||
"@@ -200,6 +200,10 @@\n",
|
||||
|
|
@ -796,9 +856,9 @@ mod tests {
|
|||
)
|
||||
.to_string(),
|
||||
}],
|
||||
};
|
||||
);
|
||||
|
||||
let symbols = extract_symbols(&diff);
|
||||
let symbols = extract_symbols_regex(&diff);
|
||||
assert!(
|
||||
!symbols.iter().any(|s| s.function.contains("helper_for_tests")),
|
||||
"#[cfg(test)] functions should be filtered, got: {:?}",
|
||||
|
|
@ -809,9 +869,8 @@ mod tests {
|
|||
#[test]
|
||||
fn test_duplicate_fn_deduped_as_modified() {
|
||||
// When a fn is both removed (-) and added (+), it should appear once as Modified
|
||||
let diff = PatchDiff {
|
||||
commit_sha: "abc123".to_string(),
|
||||
files: vec![crate::github::FilePatch {
|
||||
let diff = PatchDiff::for_test("abc123",
|
||||
vec![crate::github::FilePatch {
|
||||
filename: "src/reader.rs".to_string(),
|
||||
patch: concat!(
|
||||
"@@ -10,6 +10,8 @@ impl Reader {\n",
|
||||
|
|
@ -823,9 +882,9 @@ mod tests {
|
|||
)
|
||||
.to_string(),
|
||||
}],
|
||||
};
|
||||
);
|
||||
|
||||
let symbols = extract_symbols(&diff);
|
||||
let symbols = extract_symbols_regex(&diff);
|
||||
let open_mmap_syms: Vec<_> = symbols
|
||||
.iter()
|
||||
.filter(|s| s.function.contains("open_mmap"))
|
||||
|
|
@ -884,9 +943,8 @@ mod tests {
|
|||
fn test_trait_as_type_not_in_symbols() {
|
||||
// End-to-end: hunk header with trait impl truncated should not
|
||||
// produce symbols with trait name as type
|
||||
let diff = PatchDiff {
|
||||
commit_sha: "abc123".to_string(),
|
||||
files: vec![crate::github::FilePatch {
|
||||
let diff = PatchDiff::for_test("abc123",
|
||||
vec![crate::github::FilePatch {
|
||||
filename: "src/util.rs".to_string(),
|
||||
patch: concat!(
|
||||
"@@ -10,6 +10,8 @@ impl From<Error>\n",
|
||||
|
|
@ -897,9 +955,9 @@ mod tests {
|
|||
)
|
||||
.to_string(),
|
||||
}],
|
||||
};
|
||||
);
|
||||
|
||||
let symbols = extract_symbols(&diff);
|
||||
let symbols = extract_symbols_regex(&diff);
|
||||
for s in &symbols {
|
||||
assert!(
|
||||
!s.function.contains("From::"),
|
||||
|
|
|
|||
|
|
@ -13,8 +13,14 @@ pub struct GithubClient {
|
|||
}
|
||||
|
||||
#[derive(Debug, Deserialize)]
|
||||
struct PrResponse {
|
||||
struct PrDetailResponse {
|
||||
merge_commit_sha: Option<String>,
|
||||
base: Option<PrBranch>,
|
||||
}
|
||||
|
||||
#[derive(Debug, Deserialize)]
|
||||
struct PrBranch {
|
||||
sha: String,
|
||||
}
|
||||
|
||||
#[derive(Debug, Deserialize)]
|
||||
|
|
@ -27,6 +33,11 @@ struct CommitFile {
|
|||
#[derive(Debug, Clone)]
|
||||
pub struct PatchDiff {
|
||||
pub commit_sha: String,
|
||||
/// Owner/repo needed for fetching full file contents for AST parsing.
|
||||
pub owner: String,
|
||||
pub repo: String,
|
||||
/// Parent commit SHA, used to fetch the "before" version of files.
|
||||
pub parent_sha: Option<String>,
|
||||
pub files: Vec<FilePatch>,
|
||||
}
|
||||
|
||||
|
|
@ -36,6 +47,20 @@ pub struct FilePatch {
|
|||
pub patch: String,
|
||||
}
|
||||
|
||||
impl PatchDiff {
|
||||
/// Create a PatchDiff for testing (no owner/repo/parent metadata).
|
||||
#[cfg(test)]
|
||||
pub fn for_test(commit_sha: &str, files: Vec<FilePatch>) -> Self {
|
||||
Self {
|
||||
commit_sha: commit_sha.to_string(),
|
||||
owner: String::new(),
|
||||
repo: String::new(),
|
||||
parent_sha: None,
|
||||
files,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
impl GithubClient {
|
||||
pub fn new(token: Option<String>) -> Self {
|
||||
Self {
|
||||
|
|
@ -88,12 +113,12 @@ 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
|
||||
// Get merge commit SHA and base ref for the entry metadata
|
||||
let pr_url = format!(
|
||||
"https://api.github.com/repos/{}/{}/pulls/{}",
|
||||
owner, repo, number
|
||||
);
|
||||
let pr_resp: PrResponse = self
|
||||
let pr_resp: PrDetailResponse = self
|
||||
.get(&pr_url)
|
||||
.send()
|
||||
.context("fetching PR metadata")?
|
||||
|
|
@ -106,6 +131,8 @@ impl GithubClient {
|
|||
.merge_commit_sha
|
||||
.unwrap_or_else(|| format!("pr-{}", number));
|
||||
|
||||
let parent_sha = 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",
|
||||
|
|
@ -131,7 +158,13 @@ impl GithubClient {
|
|||
})
|
||||
.collect();
|
||||
|
||||
Ok(PatchDiff { commit_sha, files })
|
||||
Ok(PatchDiff {
|
||||
commit_sha,
|
||||
owner: owner.to_string(),
|
||||
repo: repo.to_string(),
|
||||
parent_sha,
|
||||
files,
|
||||
})
|
||||
}
|
||||
|
||||
fn fetch_commit_diff(&self, owner: &str, repo: &str, sha: &str) -> Result<PatchDiff> {
|
||||
|
|
@ -150,11 +183,23 @@ impl GithubClient {
|
|||
#[derive(Deserialize)]
|
||||
struct CommitResponse {
|
||||
sha: String,
|
||||
parents: Option<Vec<CommitParent>>,
|
||||
files: Option<Vec<CommitFile>>,
|
||||
}
|
||||
|
||||
#[derive(Deserialize)]
|
||||
struct CommitParent {
|
||||
sha: String,
|
||||
}
|
||||
|
||||
let commit: CommitResponse = resp.json().context("parsing commit response")?;
|
||||
|
||||
let parent_sha = commit
|
||||
.parents
|
||||
.as_ref()
|
||||
.and_then(|p| p.first())
|
||||
.map(|p| p.sha.clone());
|
||||
|
||||
let files = commit
|
||||
.files
|
||||
.unwrap_or_default()
|
||||
|
|
@ -170,7 +215,51 @@ impl GithubClient {
|
|||
|
||||
Ok(PatchDiff {
|
||||
commit_sha: commit.sha,
|
||||
owner: owner.to_string(),
|
||||
repo: repo.to_string(),
|
||||
parent_sha,
|
||||
files,
|
||||
})
|
||||
}
|
||||
|
||||
/// Fetch the raw contents of a file at a specific ref (commit SHA, branch, tag).
|
||||
/// Returns `Ok(None)` if the file doesn't exist at that ref (404).
|
||||
pub fn fetch_file_contents(
|
||||
&self,
|
||||
owner: &str,
|
||||
repo: &str,
|
||||
path: &str,
|
||||
ref_: &str,
|
||||
) -> Result<Option<String>> {
|
||||
let url = format!(
|
||||
"https://api.github.com/repos/{}/{}/contents/{}?ref={}",
|
||||
owner, repo, path, ref_
|
||||
);
|
||||
|
||||
let resp = self
|
||||
.client
|
||||
.get(&url)
|
||||
.header(USER_AGENT, "cargo-deep-audit/0.1")
|
||||
.header(ACCEPT, "application/vnd.github.v3.raw");
|
||||
|
||||
let resp = if let Some(ref token) = self.token {
|
||||
resp.header(AUTHORIZATION, format!("Bearer {}", token))
|
||||
} else {
|
||||
resp
|
||||
};
|
||||
|
||||
let resp = resp.send().context("fetching file contents")?;
|
||||
|
||||
if resp.status() == reqwest::StatusCode::NOT_FOUND {
|
||||
return Ok(None);
|
||||
}
|
||||
|
||||
let body = resp
|
||||
.error_for_status()
|
||||
.context("file contents API error")?
|
||||
.text()
|
||||
.context("reading file contents")?;
|
||||
|
||||
Ok(Some(body))
|
||||
}
|
||||
}
|
||||
|
|
|
|||
|
|
@ -1,5 +1,6 @@
|
|||
mod advisory;
|
||||
mod analyzer;
|
||||
mod ast_differ;
|
||||
mod db;
|
||||
mod diff_analyzer;
|
||||
mod github;
|
||||
|
|
@ -245,7 +246,7 @@ fn run_enrich(args: EnrichArgs) -> Result<()> {
|
|||
diff.files.len()
|
||||
);
|
||||
new_sha = Some(diff.commit_sha.clone());
|
||||
new_symbols = extract_symbols(&diff);
|
||||
new_symbols = extract_symbols(&diff, &gh);
|
||||
break;
|
||||
}
|
||||
Ok(None) => {
|
||||
|
|
@ -368,7 +369,7 @@ fn run_enrich(args: EnrichArgs) -> Result<()> {
|
|||
diff.files.len()
|
||||
);
|
||||
entry.commit_sha = Some(diff.commit_sha.clone());
|
||||
let symbols = extract_symbols(&diff);
|
||||
let symbols = extract_symbols(&diff, &gh);
|
||||
if symbols.is_empty() {
|
||||
println!(" No function signatures extracted from diff");
|
||||
} else {
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue