fix: CVE notifications during scan + help chat doc loading + Dockerfile #55
sharang
Reference in New Issue
Block a user
Delete Branch "fix/multiple-issues"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Bug fixes: 1. CVE notifications now created during scan pipeline (not just hourly) - Previously, notifications were only created by the scheduled monitor_cves job. Users with 4 CVE alerts saw 0 notifications. - Now the scan pipeline (Stage 3) creates notifications immediately when CVE alerts are discovered, with the same dedup logic. 2. Help chat doc context loading fixed for Docker/production - Added HELP_DOCS_PATH env var for explicit doc root configuration - Added fallback chain: env var → binary location → cwd → Docker paths - Dockerfile.agent now copies README.md and docs/ into /app and sets HELP_DOCS_PATH=/app so the help chat has doc context in production Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>Compliance scan found 8 issue(s) in this PR:
@@ -107,0 +108,4 @@/// Discovery order:/// 1. `HELP_DOCS_PATH` env var (explicit override)/// 2. Walk up from the binary location/// 3. Current working directory[medium] Complex boolean expression in doc_context function
The doc_context function contains multiple nested conditional checks with complex boolean logic that combines environment variable checking, file system operations, and path validation. The logic for determining the documentation source involves several branches that are hard to follow and could lead to incorrect behavior if any condition is misinterpreted.
Suggested fix: Extract the logic for each discovery step into separate helper functions with clear names, and simplify the main function to just orchestrate these steps. This would make the control flow much clearer and reduce the chance of bugs from misinterpreting the complex boolean conditions.
*Scanner: code-review/complexity | *
@@ -107,1 +110,4 @@/// 2. Walk up from the binary location/// 3. Current working directory/// 4. Common Docker paths (/app, /opt/compliance-scanner)fn doc_context() -> &'static str {[high] Path Traversal in Help Chat Documentation Loading
The help chat feature loads documentation from paths specified by the HELP_DOCS_PATH environment variable. If an attacker can control this environment variable, they can manipulate the path to traverse the filesystem and load arbitrary files. This occurs because the code directly uses the value of HELP_DOCS_PATH without validating or sanitizing it, allowing for directory traversal attacks.
Suggested fix: Validate and sanitize the HELP_DOCS_PATH environment variable to ensure it points to a legitimate directory within expected boundaries. Consider using a whitelist of allowed paths or canonicalizing the path to prevent directory traversal.
Scanner: code-review/security | CWE: CWE-23
@@ -108,1 +114,4 @@DOC_CONTEXT.get_or_init(|| {// 1. Explicit env varif let Ok(path) = std::env::var("HELP_DOCS_PATH") {let p = PathBuf::from(&path);[medium] Inconsistent error handling in doc context loading
In
compliance-agent/src/api/handlers/help_chat.rs, the code mixesunwrap_or_elsewith explicit error logging andok()chaining. The inconsistency in how errors are handled (some useunwrap_or_else, others useok()andand_then) makes the error recovery behavior less predictable and harder to maintain.Suggested fix: Standardize error handling approach throughout the function, either consistently using
?operator or consistentunwrap_or_elsepatterns with appropriate logging.*Scanner: code-review/convention | *
@@ -109,0 +118,4 @@if p.join("README.md").is_file() || p.join("docs").is_dir() {tracing::info!("help_chat: loading docs from HELP_DOCS_PATH={path}");return load_docs(&p);}[medium] Potential panic from unwrap in help chat documentation loading
In
compliance-agent/src/api/handlers/help_chat.rs, line 121 usesunwrap_or_else(|| PathBuf::from("."))which can panic ifstd::env::current_exe()fails. While it's wrapped inok(), the fallback path still assumes the current directory exists, which might not always be true.Suggested fix: Ensure the fallback path handles all possible error cases gracefully without relying on assumptions about filesystem state.
*Scanner: code-review/convention | *
@@ -109,0 +122,4 @@tracing::warn!("help_chat: HELP_DOCS_PATH={path} has no README.md or docs/");}// 2. Walk up from binary location[high] Incorrect fallback logic in doc_context() when walking up from binary location
In the
doc_context()function, after checking for a project root from the binary location, the code attempts to find a project root from the current working directory. However, it should only fall back to the current working directory if no project root is found from the binary location. Currently, it checks for a project root in the current working directory even when a project root was already found from the binary location.Suggested fix: Move the current working directory check outside of the
if let Some(root) = find_project_root(&start)block so it only executes when no project root is found from the binary location.*Scanner: code-review/logic | *
@@ -331,1 +318,3 @@.await?;// Persist CVE alerts and create notifications{use compliance_core::models::notification::{parse_severity, CveNotification};[medium] Function with multiple interleaved responsibilities
The CVE alert persistence section in orchestrator.rs handles both upserting CVE alerts and creating notifications within the same code block. This creates a complex function that performs multiple distinct operations (database updates, notification creation, counting) which makes it harder to reason about and increases the likelihood of bugs when modifying either part.
Suggested fix: Split this section into two separate logical blocks: one for persisting CVE alerts and another for creating notifications. Consider extracting the notification creation logic into its own function to improve readability and maintainability.
*Scanner: code-review/complexity | *
@@ -332,0 +338,4 @@.await?;// Create notification (dedup by cve_id + repo + package + version)let notif_filter = doc! {[medium] Unwrapped MongoDB BSON conversion in CVE alert persistence
In
compliance-agent/src/pipeline/orchestrator.rs, the code uses.unwrap_or_else(|_| doc! {})and.unwrap_or_default()when converting structs to BSON documents. This can cause panics if BSON serialization fails, which should be handled more gracefully to prevent runtime crashes.Suggested fix: Replace unwraps with proper error handling using
?operator or logging and fallback mechanisms instead of panicking.*Scanner: code-review/convention | *
@@ -332,0 +346,4 @@};let severity = parse_severity(alert.severity.as_deref(), alert.cvss_score);let mut notification = CveNotification::new(alert.cve_id.clone(),[high] Potential panic in notification creation due to unwrap_or_default() usage
In the pipeline orchestrator, when creating notifications, the code uses
mongodb::bson::to_bson(¬ification).unwrap_or_default()which can panic if the notification struct cannot be converted to BSON. This is dangerous because it assumes the conversion will always succeed, but BSON serialization can fail for complex types or invalid data. This could lead to a crash during notification creation.Suggested fix: Replace
unwrap_or_default()with proper error handling using?operator or logging and skipping the notification creation if BSON conversion fails.*Scanner: code-review/logic | *
Compliance scan found 14 issue(s) in this PR:
@@ -104,28 +104,58 @@ fn load_docs(root: &Path) -> String {/// Returns a reference to the cached doc context string, initialised on/// first call via `OnceLock`.///[medium] Complex boolean expression in doc_context function
The doc_context function contains a complex series of nested conditionals and path checks that make it difficult to follow the control flow and potential failure cases. The logic for checking multiple fallback locations (binary location, current working directory, Docker paths) is spread across multiple if blocks with early returns, increasing cognitive load.
Suggested fix: Refactor into smaller helper functions that each handle one discovery method (env var, binary location, cwd, docker paths) with clear return values. This would flatten the nesting and make each path easier to reason about.
*Scanner: code-review/complexity | *
@@ -107,0 +107,4 @@////// Discovery order:/// 1. `HELP_DOCS_PATH` env var (explicit override)/// 2. Walk up from the binary location[high] Path Traversal via HELP_DOCS_PATH Environment Variable
The help_chat feature allows loading documentation from a path specified by the HELP_DOCS_PATH environment variable. If an attacker can control this variable, they can point it to arbitrary directories on the filesystem, potentially leading to path traversal. Combined with the ability to load files from the filesystem through the doc_context() function, this could allow reading sensitive files like configuration files, secrets, or other system files.
Suggested fix: Validate and sanitize the HELP_DOCS_PATH environment variable to ensure it points to a safe, expected directory. Implement strict path validation to prevent traversal outside intended directories.
Scanner: code-review/security | CWE: CWE-22
@@ -107,2 +111,4 @@/// 3. Current working directory/// 4. Common Docker paths (/app, /opt/compliance-scanner)fn doc_context() -> &'static str {DOC_CONTEXT.get_or_init(|| {[medium] Inconsistent error handling in doc_context function
The
doc_contextfunction uses inconsistent error handling patterns. It usesunwrap_or_elseforstd::env::current_dir()butok()followed byand_thenforstd::env::current_exe(). This inconsistency could lead to unhandled errors in edge cases where the current directory is inaccessible but the binary path is still valid.Suggested fix: Use consistent error handling throughout. Replace
unwrap_or_else(|| PathBuf::from("."))withok().and_then(|p| p.parent().map(Path::to_path_buf)).unwrap_or_else(|| PathBuf::from("."))to maintain consistency.*Scanner: code-review/convention | *
@@ -109,0 +122,4 @@tracing::warn!("help_chat: HELP_DOCS_PATH={path} has no README.md or docs/");}// 2. Walk up from binary location[high] Incorrect fallback logic in doc_context() function
The doc_context() function attempts to find documentation in the current working directory by first calling find_project_root() on the cwd, but then falls back to checking if README.md exists directly. However, if find_project_root() fails on the cwd, it should still attempt to load docs from the cwd if README.md exists there. Currently, the logic skips this check when find_project_root() fails.
Suggested fix: Reorder the logic so that after failing to find a project root from cwd, we still check if the cwd itself contains README.md. The fix should ensure that if find_project_root() fails but cwd has README.md, we load docs from cwd.
*Scanner: code-review/logic | *
@@ -110,4 +127,4 @@.ok().and_then(|p| p.parent().map(Path::to_path_buf)).unwrap_or_else(|| PathBuf::from("."));[medium] Insecure Direct Object Reference in Documentation Loading
The application loads documentation from paths determined by the file system structure. If an attacker can manipulate the file system or influence the location of documentation files, they may be able to access unintended files or directories. This is particularly concerning because the code attempts to load documentation from multiple locations including common Docker paths, which might expose internal files if not properly secured.
Suggested fix: Implement access controls and validate that loaded documentation paths are within expected boundaries. Consider using a whitelist approach for allowed documentation directories.
Scanner: code-review/security | CWE: CWE-284
@@ -129,0 +144,4 @@// 4. Common Docker/deployment pathsfor candidate in ["/app", "/opt/compliance-scanner", "/srv/compliance-scanner"] {let p = PathBuf::from(candidate);[medium] Missing error propagation in help chat documentation loading
The
doc_contextfunction silently returns an empty string when it cannot locate documentation files instead of propagating the error. This could mask underlying issues with documentation setup and make debugging harder.Suggested fix: Consider returning a Result type instead of String to propagate the error condition, allowing callers to handle missing documentation appropriately rather than silently failing.
*Scanner: code-review/convention | *
@@ -18,0 +20,4 @@// Security headers (defense-in-depth, primary enforcement via Traefik).layer(SetResponseHeaderLayer::overriding(axum::http::header::STRICT_TRANSPORT_SECURITY,HeaderValue::from_static("max-age=31536000; includeSubDomains"),[medium] Deeply nested middleware layering in API server
The API server setup creates deeply nested middleware layers (4+ levels) with security headers being added through SetResponseHeaderLayer calls. While this isn't inherently wrong, the pattern of adding multiple header layers sequentially increases the chance of ordering dependencies being misunderstood during future modifications.
Suggested fix: Consider grouping related security headers together in a single middleware layer or using a builder pattern to make the intent clearer and reduce the visual nesting depth.
*Scanner: code-review/complexity | *
@@ -18,0 +22,4 @@axum::http::header::STRICT_TRANSPORT_SECURITY,HeaderValue::from_static("max-age=31536000; includeSubDomains"),)).layer(SetResponseHeaderLayer::overriding([low] Potential panic in security header configuration
The security header configuration uses
HeaderValue::from_static()which can only accept string literals known at compile time. If any of the header values were to be constructed dynamically, this would cause a compilation error. While currently safe, this pattern makes the code less flexible for future modifications.Suggested fix: Consider using
HeaderValue::from_str()for dynamic values or ensure all header values remain compile-time constants to maintain safety.*Scanner: code-review/convention | *
@@ -332,0 +329,4 @@"repo_id": &alert.repo_id,};let update = mongodb::bson::to_document(alert).map(|d| doc! { "$set": d })[medium] Potential BSON Serialization Error Leading to Data Corruption
The code uses
mongodb::bson::to_document(alert)andmongodb::bson::to_bson(¬ification)which can fail during serialization. When serialization fails, the code falls back to empty documents (doc! {}ordoc! {"$setOnInsert": bson::Bson::Document(bson::Document::new())}), potentially leading to incomplete or corrupted data being stored in MongoDB. This could result in missing alert data or incorrect notification creation.Suggested fix: Add proper error handling for BSON serialization failures and log them appropriately. Consider using a more robust serialization approach or ensuring that all fields in the alert and notification structs are serializable.
Scanner: code-review/security | CWE: CWE-704
[medium] Incorrect notification deduplication filter
The notification deduplication filter uses 'package_name' and 'package_version' but should use 'package_name' and 'package_version' from the alert struct. Looking at the code, it appears that 'alert.affected_package' and 'alert.affected_version' are being used correctly, but there's a potential mismatch in field names between what's stored in the database and what's being queried. However, since we're using the same fields from the alert object, this seems correct.
Suggested fix: Verify that the field names in the MongoDB collection match exactly with 'package_name' and 'package_version' as they appear in the CveNotification struct. If they differ, the deduplication will fail.
*Scanner: code-review/logic | *
[medium] Inconsistent error handling with unwrap_or_else
The code uses
unwrap_or_else(|_| doc! {})andunwrap_or_default()which can hide serialization errors. If MongoDB serialization fails, these operations silently create empty documents instead of propagating the error, potentially leading to data loss or incorrect database state.Suggested fix: Replace
unwrap_or_else(|_| doc! {})with proper error propagation using?operator or explicit error logging before falling back to default behavior*Scanner: code-review/convention | *
@@ -332,0 +332,4 @@.map(|d| doc! { "$set": d }).unwrap_or_else(|_| doc! {});self.db.cve_alerts()[medium] Complex boolean expression with multiple nested conditions
The code contains a complex boolean expression in the notification creation logic that combines multiple conditions and error handling. The nested
if let Ok(result)check with the subsequentif result.upserted_id.is_some()creates a deeply nested flow that's hard to follow and increases risk of logic errors when modifying.Suggested fix: Extract the database operation into a separate function and flatten the conditional logic. Consider using early returns or a match statement to handle the upsert result more clearly.
*Scanner: code-review/complexity | *
@@ -332,0 +346,4 @@};let severity = parse_severity(alert.severity.as_deref(), alert.cvss_score);let mut notification = CveNotification::new(alert.cve_id.clone(),[high] Potential panic in notification creation
The code uses
unwrap_or_default()when converting the notification to BSON, which could panic if the conversion fails. While this is unlikely, it could cause a crash if the notification struct contains data that cannot be serialized to BSON. This is a potential runtime error.Suggested fix: Replace
unwrap_or_default()with proper error handling to avoid panicking. Consider logging the error and continuing processing instead of crashing.*Scanner: code-review/logic | *
@@ -332,0 +348,4 @@let mut notification = CveNotification::new(alert.cve_id.clone(),repo_id.clone(),repo_name.clone(),[medium] Inconsistent error handling with upsert operation
The code handles the result of the upsert operation for notifications with a
if let Ok(result) = ...check, but then ignores potential errors from the database operation itself. This creates inconsistent error handling where some operations are properly checked while others are silently ignored.Suggested fix: Either propagate the error from the upsert operation or ensure consistent error handling across all database operations in this section
*Scanner: code-review/convention | *
Compliance scan found 29 issue(s) in this PR:
@@ -104,28 +104,58 @@ fn load_docs(root: &Path) -> String {/// Returns a reference to the cached doc context string, initialised on/// first call via `OnceLock`.///[medium] Complex boolean expression in doc_context function
The doc_context function contains a complex series of nested conditionals and path checks that make it difficult to follow the control flow and potential failure cases. The logic for checking multiple fallback locations (binary location, current working directory, Docker paths) is spread across multiple if blocks with early returns, increasing cognitive load.
Suggested fix: Refactor into smaller helper functions that each handle one discovery method (env var, binary location, cwd, docker paths) with clear return values. This would flatten the nesting and make each path easier to reason about.
*Scanner: code-review/complexity | *
@@ -107,0 +107,4 @@////// Discovery order:/// 1. `HELP_DOCS_PATH` env var (explicit override)/// 2. Walk up from the binary location[high] Path Traversal via HELP_DOCS_PATH Environment Variable
The help_chat feature allows loading documentation from a path specified by the HELP_DOCS_PATH environment variable. If an attacker can control this variable, they can point it to arbitrary directories on the filesystem, potentially leading to path traversal. Combined with the ability to load files from the filesystem through the doc_context() function, this could allow reading sensitive files like configuration files, secrets, or other system files.
Suggested fix: Validate and sanitize the HELP_DOCS_PATH environment variable to ensure it points to a safe, expected directory. Implement strict path validation to prevent traversal outside intended directories.
Scanner: code-review/security | CWE: CWE-22
@@ -107,2 +111,4 @@/// 3. Current working directory/// 4. Common Docker paths (/app, /opt/compliance-scanner)fn doc_context() -> &'static str {DOC_CONTEXT.get_or_init(|| {[medium] Inconsistent error handling in doc_context function
The
doc_contextfunction uses inconsistent error handling patterns. It usesunwrap_or_elseforstd::env::current_dir()butok()followed byand_thenforstd::env::current_exe(). This inconsistency could lead to unhandled errors in edge cases where the current directory is inaccessible but the binary path is still valid.Suggested fix: Use consistent error handling throughout. Replace
unwrap_or_else(|| PathBuf::from("."))withok().and_then(|p| p.parent().map(Path::to_path_buf)).unwrap_or_else(|| PathBuf::from("."))to maintain consistency.*Scanner: code-review/convention | *
@@ -109,0 +122,4 @@tracing::warn!("help_chat: HELP_DOCS_PATH={path} has no README.md or docs/");}// 2. Walk up from binary location[high] Incorrect fallback logic in doc_context() function
The doc_context() function attempts to find documentation in the current working directory by first calling find_project_root() on the cwd, but then falls back to checking if README.md exists directly. However, if find_project_root() fails on the cwd, it should still attempt to load docs from the cwd if README.md exists there. Currently, the logic skips this check when find_project_root() fails.
Suggested fix: Reorder the logic so that after failing to find a project root from cwd, we still check if the cwd itself contains README.md. The fix should ensure that if find_project_root() fails but cwd has README.md, we load docs from cwd.
*Scanner: code-review/logic | *
@@ -110,4 +127,4 @@.ok().and_then(|p| p.parent().map(Path::to_path_buf)).unwrap_or_else(|| PathBuf::from("."));[medium] Insecure Direct Object Reference in Documentation Loading
The application loads documentation from paths determined by the file system structure. If an attacker can manipulate the file system or influence the location of documentation files, they may be able to access unintended files or directories. This is particularly concerning because the code attempts to load documentation from multiple locations including common Docker paths, which might expose internal files if not properly secured.
Suggested fix: Implement access controls and validate that loaded documentation paths are within expected boundaries. Consider using a whitelist approach for allowed documentation directories.
Scanner: code-review/security | CWE: CWE-284
@@ -129,0 +144,4 @@// 4. Common Docker/deployment pathsfor candidate in ["/app", "/opt/compliance-scanner", "/srv/compliance-scanner"] {let p = PathBuf::from(candidate);[medium] Missing error propagation in help chat documentation loading
The
doc_contextfunction silently returns an empty string when it cannot locate documentation files instead of propagating the error. This could mask underlying issues with documentation setup and make debugging harder.Suggested fix: Consider returning a Result type instead of String to propagate the error condition, allowing callers to handle missing documentation appropriately rather than silently failing.
*Scanner: code-review/convention | *
@@ -18,0 +20,4 @@// Security headers (defense-in-depth, primary enforcement via Traefik).layer(SetResponseHeaderLayer::overriding(axum::http::header::STRICT_TRANSPORT_SECURITY,HeaderValue::from_static("max-age=31536000; includeSubDomains"),[medium] Deeply nested middleware layering in API server
The API server setup creates deeply nested middleware layers (4+ levels) with security headers being added through SetResponseHeaderLayer calls. While this isn't inherently wrong, the pattern of adding multiple header layers sequentially increases the chance of ordering dependencies being misunderstood during future modifications.
Suggested fix: Consider grouping related security headers together in a single middleware layer or using a builder pattern to make the intent clearer and reduce the visual nesting depth.
*Scanner: code-review/complexity | *
@@ -18,0 +22,4 @@axum::http::header::STRICT_TRANSPORT_SECURITY,HeaderValue::from_static("max-age=31536000; includeSubDomains"),)).layer(SetResponseHeaderLayer::overriding([low] Potential panic in security header configuration
The security header configuration uses
HeaderValue::from_static()which can only accept string literals known at compile time. If any of the header values were to be constructed dynamically, this would cause a compilation error. While currently safe, this pattern makes the code less flexible for future modifications.Suggested fix: Consider using
HeaderValue::from_str()for dynamic values or ensure all header values remain compile-time constants to maintain safety.*Scanner: code-review/convention | *
@@ -332,0 +324,4 @@for alert in &cve_alerts {// Upsert the alertlet filter = doc! {[medium] Potential Command Injection via User-Controlled Repository Name
The repository name is directly used in a logging statement without sanitization. If an attacker can control the repository name through external input (e.g., via API or configuration), they could inject malicious data into logs or potentially influence log parsing systems. While this doesn't directly lead to RCE, it's a potential vector for log-based attacks or information leakage.
Suggested fix: Sanitize or escape the repository name before including it in log messages. Consider using a structured logging approach that prevents injection.
Scanner: code-review/security | CWE: CWE-77
@@ -332,0 +327,4 @@let filter = doc! {"cve_id": &alert.cve_id,"repo_id": &alert.repo_id,};[medium] Complex boolean expression in notification creation logic
The notification creation logic contains a complex boolean expression involving multiple conditions and nested operations that could be simplified for better readability and reduced risk of logical errors.
Suggested fix: Extract the notification creation and upsert logic into a separate function with clear parameter validation, or refactor the nested conditionals into a more readable form using early returns.
*Scanner: code-review/complexity | *
@@ -332,0 +332,4 @@.map(|d| doc! { "$set": d }).unwrap_or_else(|_| doc! {});self.db.cve_alerts()[medium] Inconsistent error handling with unwrap() in database operations
The code uses
.unwrap_or_else(|_| doc! {})and.unwrap_or_default()which can hide serialization errors. This creates inconsistent error handling within the same module where failures might be silently ignored instead of being properly propagated or logged.Suggested fix: Replace unwrap_or_else with proper error handling using ? operator or explicit error logging to ensure serialization failures are not silently ignored.
*Scanner: code-review/convention | *
@@ -332,0 +335,4 @@.cve_alerts().update_one(filter, update).upsert(true).await?;[high] Incorrect upsert logic for CVE notifications
The code uses
$setOnInsertto create notifications, but the filter used for checking duplicates ('cve_id + repo + package + version') may not correctly identify all duplicate notifications. If an existing notification has different values for other fields (like summary or URL), it won't be updated even though it should be. Additionally, the notification creation logic doesn't handle potential BSON conversion errors properly, which could lead to silent failure.Suggested fix: Ensure that the notification deduplication filter is comprehensive enough to catch all relevant duplicates. Also, consider using a more robust error handling mechanism instead of
.unwrap_or_default()when converting to BSON.*Scanner: code-review/logic | *
@@ -332,0 +340,4 @@// Create notification (dedup by cve_id + repo + package + version)let notif_filter = doc! {"cve_id": &alert.cve_id,"repo_id": &alert.repo_id,[high] Insecure Direct Object Reference in Notification Creation
The code creates notifications based on CVE alerts but does not validate that the user has permission to access or modify the associated repository. This could allow a malicious actor to create notifications for repositories they don't own, potentially leading to unauthorized data exposure or manipulation.
Suggested fix: Add authorization checks to ensure that the authenticated user has appropriate permissions for the repository before creating notifications.
Scanner: code-review/security | CWE: CWE-639
@@ -332,0 +351,4 @@repo_name.clone(),alert.affected_package.clone(),alert.affected_version.clone(),severity,[medium] Potential panic from unwrap_or_default() in notification creation
The code calls
.unwrap_or_default()onmongodb::bson::to_bson(¬ification)which can panic if the notification struct cannot be converted to BSON. This is an anti-pattern in library code where panics can crash the entire service.Suggested fix: Replace unwrap_or_default() with proper error handling using ? operator or provide a default value that doesn't rely on unwrapping.
*Scanner: code-review/convention | *
[medium] Inconsistent error handling in notification creation loop
The code handles errors from the database operation with
if let Ok(result) = ...but then ignores potential errors from themongodb::bson::to_bson(¬ification).unwrap_or_default()call. This inconsistency in error handling could lead to silent failures when creating notifications.Suggested fix: Use proper error handling for the bson conversion similar to how the database operation is handled, or at least log the error if it occurs.
*Scanner: code-review/convention | *
@@ -0,0 +15,4 @@} else {"copy-btn"};[high] Incorrect escaping of single quotes in JavaScript string
The code escapes single quotes with backslashes for JavaScript, but doesn't handle the case where the value itself contains backslashes that should also be escaped. This could lead to malformed JavaScript when the copied text contains backslashes followed by single quotes.
Suggested fix: Use a more robust escaping mechanism that handles all special characters properly, or better yet, use template literals or JSON.stringify to safely pass the value to JavaScript instead of manual string concatenation.
*Scanner: code-review/logic | *
@@ -0,0 +17,4 @@};rsx! {button {[medium] Complex boolean expression in conditional rendering
The conditional rendering logic uses a complex boolean expression that mixes multiple conditions and string formatting in a way that's hard to follow. Specifically, the title attribute uses a ternary operator with string literals that could be simplified.
Suggested fix: Simplify the title attribute by extracting the conditional logic into a separate variable or using a more explicit if-else structure to improve readability.
*Scanner: code-review/complexity | *
@@ -0,0 +19,4 @@rsx! {button {class: class,title: if copied() { "Copied!" } else { "Copy to clipboard" },[medium] Potential security vulnerability in JavaScript injection
The code manually escapes single quotes and backslashes for JavaScript string interpolation, but this approach is vulnerable to XSS attacks and doesn't properly escape all special characters. The current implementation could be exploited if the copied text contains malicious content.
Suggested fix: Use a proper escaping mechanism or consider using the Clipboard API directly through web_sys instead of injecting raw JavaScript strings.
*Scanner: code-review/convention | *
@@ -0,0 +22,4 @@title: if copied() { "Copied!" } else { "Copy to clipboard" },onclick: move |_| {let val = value.clone();// Escape single quotes for JS[medium] Inconsistent error handling in copy button
The CopyButton component uses
document::eval(&js)which can fail silently. If the JavaScript execution fails, the UI will still show 'Copied!' but the actual copy operation may have failed. The component should handle potential errors from the clipboard API and provide appropriate feedback to the user.Suggested fix: Add proper error handling around the clipboard operation. Consider using a more robust approach like calling a JS function that returns a promise and handling both success and failure cases.
*Scanner: code-review/convention | *
[high] Potential XSS via Copy Button
The CopyButton component directly interpolates user-provided 'value' into JavaScript code without proper sanitization. Although the code attempts to escape single quotes and backslashes, it's insufficient to prevent XSS when the value contains malicious JavaScript. An attacker could inject a value like "'; alert(1); //" which would result in executable JavaScript being evaluated by navigator.clipboard.writeText(). This allows for potential XSS exploitation through the copy functionality.
Suggested fix: Use a more robust escaping mechanism or sanitize the input before inserting into JavaScript. Consider using a library specifically designed for HTML/JS escaping, or ensure all user inputs are properly encoded according to JavaScript string literal rules.
Scanner: code-review/security | CWE: CWE-79
@@ -0,0 +25,4 @@// Escape single quotes for JSlet escaped = val.replace('\\', "\\\\").replace('\'', "\\'");let js = format!("navigator.clipboard.writeText('{escaped}')");document::eval(&js);[medium] Inconsistent async timeout handling
The component uses different async timeout mechanisms for web vs non-web targets (gloo_timers vs tokio). This creates inconsistent behavior between platforms and could lead to timing issues or compilation errors on non-web targets.
Suggested fix: Standardize on one async runtime or ensure consistent behavior across platforms by using platform-agnostic alternatives.
*Scanner: code-review/convention | *
@@ -0,0 +26,4 @@let escaped = val.replace('\\', "\\\\").replace('\'', "\\'");let js = format!("navigator.clipboard.writeText('{escaped}')");document::eval(&js);copied.set(true);[medium] Deeply nested control flow in async block
The async block contains nested conditional compilation directives (#cfg) which creates a complex control flow path that's difficult to reason about. The branching logic between web and non-web platforms is embedded within an async context, making it harder to follow the execution path.
Suggested fix: Extract the platform-specific timeout logic into separate functions or use a platform-agnostic approach to reduce nesting and make the control flow clearer.
*Scanner: code-review/complexity | *
@@ -145,0 +142,4 @@code {style: "font-size: 11px; word-break: break-all; user-select: all;",if ssh_public_key().is_empty() {"Loading..."[medium] Potential race condition in SSH public key display
The SSH public key is checked for emptiness twice - once for displaying 'Loading...' and again for determining whether to show the copy button. If the key becomes empty between these two checks, it could result in inconsistent UI state where the copy button appears but the key is actually empty.
Suggested fix: Store the SSH public key result in a local variable before checking its emptiness to ensure consistency across both conditions.
*Scanner: code-review/logic | *
[medium] Complex boolean expression in conditional rendering
The code uses repeated calls to
ssh_public_key()which could lead to performance issues or unexpected behavior if the function has side effects. The same check!ssh_public_key().is_empty()appears twice in the same component.Suggested fix: Store the result of
ssh_public_key()in a local variable before checking its emptiness to avoid redundant function calls and potential side effects.*Scanner: code-review/complexity | *
[low] Potential duplicate computation of ssh_public_key()
The
ssh_public_key()function is called twice in the same conditional block - once for the display text and once for the CopyButton component. This could be inefficient if the function involves expensive operations or network calls.Suggested fix: Store the result of
ssh_public_key()in a local variable before the conditional check to avoid redundant calls.*Scanner: code-review/convention | *
@@ -406,0 +402,4 @@let origin = web_sys::window().and_then(|w: web_sys::Window| w.location().origin().ok()).unwrap_or_default();#[cfg(not(feature = "web"))][medium] Inconsistent error handling with unwrap_or_default() in web feature
The code uses
unwrap_or_default()onw.location().origin().ok()which can mask potential errors in URL parsing. This could lead to unexpected behavior when the origin cannot be determined, especially in non-web environments where the window object might not be available.Suggested fix: Consider using a more explicit error handling approach or logging when the origin cannot be determined, rather than silently falling back to an empty string.
*Scanner: code-review/convention | *
[medium] Potential Information Disclosure via Copy Button Implementation
The implementation introduces copy buttons for SSH public keys, webhook URLs, and secrets. While the copy functionality itself is not inherently vulnerable, the way these values are exposed through the UI could potentially lead to information disclosure if the page is cached or logged. Specifically, the secret value is directly rendered in an input field and made available via the copy button, which could be exploited if the application's logging or caching mechanisms capture this content.
Suggested fix: Ensure that sensitive values like secrets are not logged or cached in any form. Consider implementing additional safeguards such as disabling browser autocomplete for sensitive fields and ensuring that the copy functionality does not inadvertently expose these values through other means (e.g., browser history, developer tools).
Scanner: code-review/security | CWE: CWE-200
@@ -406,0 +414,4 @@value: "{webhook_url}",}crate::components::copy_button::CopyButton { value: webhook_url.clone() }}[medium] Inconsistent use of clone() for webhook URL
The webhook URL is cloned when passed to the CopyButton component, but the input field uses a string literal reference. This inconsistency could lead to unnecessary allocations or potential issues if the value needs to be used elsewhere after the clone operation.
Suggested fix: Use the same approach for both the input field and the copy button - either clone the value consistently or pass a reference to avoid unnecessary allocations.
*Scanner: code-review/logic | *
@@ -415,0 +425,4 @@r#type: "text",readonly: true,style: "font-family: monospace; font-size: 12px; flex: 1;",value: "{secret}",[low] Redundant clone() in CopyButton component
The
secret.clone()call in the CopyButton component creates an unnecessary clone of the secret string. Since the value is already owned by the component, cloning it just to pass it to another component is wasteful.Suggested fix: Pass the secret directly without cloning, or ensure the CopyButton component accepts a reference instead of owned values.
*Scanner: code-review/convention | *
Compliance scan found 28 issue(s) in this PR:
@@ -104,28 +104,58 @@ fn load_docs(root: &Path) -> String {/// Returns a reference to the cached doc context string, initialised on/// first call via `OnceLock`.///[medium] Complex nested control flow in doc_context function
The
doc_context()function contains deeply nested conditional logic with multiple fallback strategies for locating documentation. The function has 4 main discovery steps, each with their own nested checks and early returns, making it difficult to follow the execution path and increasing the risk of logic errors when modifying or extending.Suggested fix: Refactor into separate helper functions for each discovery strategy (env var, binary path, cwd, docker paths) and flatten the control flow by using a loop or early-return pattern to reduce nesting depth.
*Scanner: code-review/complexity | *
@@ -107,0 +109,4 @@/// 1. `HELP_DOCS_PATH` env var (explicit override)/// 2. Walk up from the binary location/// 3. Current working directory/// 4. Common Docker paths (/app, /opt/compliance-scanner)[high] Path Traversal via HELP_DOCS_PATH Environment Variable
The help chat feature allows loading documentation from a directory specified by the HELP_DOCS_PATH environment variable. If an attacker can control this environment variable, they can point it to a directory outside of the intended documentation paths, potentially leading to path traversal and arbitrary file reading. This occurs because the code directly uses the provided path without validating or sanitizing it.
Suggested fix: Validate that the HELP_DOCS_PATH points to a whitelisted directory or ensure it's within a known safe base path before using it.
Scanner: code-review/security | CWE: CWE-22
[high] Insecure Direct Object Reference in Documentation Loading
The application loads documentation files based on user-controllable paths through the HELP_DOCS_PATH environment variable. If an attacker can manipulate this variable, they may be able to read arbitrary files from the filesystem by pointing to directories containing sensitive information, especially if the application runs with elevated privileges.
Suggested fix: Implement strict access controls and validate all paths against a whitelist of allowed directories to prevent unauthorized file access.
Scanner: code-review/security | CWE: CWE-284
@@ -107,2 +112,4 @@/// 4. Common Docker paths (/app, /opt/compliance-scanner)fn doc_context() -> &'static str {DOC_CONTEXT.get_or_init(|| {// 1. Explicit env var[medium] Inconsistent error handling in doc_context function
The
doc_contextfunction uses inconsistent error handling patterns. It usesunwrap_or_elseforstd::env::current_dir()butok()followed byand_thenforstd::env::current_exe(). This inconsistency could lead to unhandled errors in edge cases where the current directory is inaccessible but the binary path is still valid.Suggested fix: Use consistent error handling approach throughout. Consider using
?operator orunwrap_or_elseconsistently for all path operations.*Scanner: code-review/convention | *
@@ -109,0 +120,4 @@return load_docs(&p);}tracing::warn!("help_chat: HELP_DOCS_PATH={path} has no README.md or docs/");}[high] Incorrect fallback logic in doc_context() when walking up from binary location
In the
doc_context()function, after failing to find a project root from the binary location, the code attempts to fall back to the current working directory. However, it first tries to find a project root from the current working directory before checking if it contains a README.md directly. This means that if the current working directory is not a project root but does contain a README.md, it will incorrectly skip loading docs from that directory becausefind_project_root()fails. The correct behavior should be to check for README.md existence first.Suggested fix: Reorder the logic so that we first check if the current working directory contains README.md directly, then fall back to finding a project root.
*Scanner: code-review/logic | *
@@ -127,2 +142,4 @@}}// 4. Common Docker/deployment paths[low] Missing explicit error handling for missing documentation files
The documentation loading logic in
doc_contextfalls back to an empty string when documentation cannot be located, but doesn't provide any mechanism to alert users or administrators about the missing documentation. This could make debugging difficult when help chat functionality is expected but not working due to missing docs.Suggested fix: Add a more explicit warning or logging mechanism when documentation is completely missing, potentially including a configuration option to fail fast if docs are required.
*Scanner: code-review/convention | *
@@ -16,2 +18,3 @@.layer(CorsLayer::permissive()).layer(TraceLayer::new_for_http());.layer(TraceLayer::new_for_http())// Security headers (defense-in-depth, primary enforcement via Traefik)[medium] Multiple interleaved responsibilities in API server layer configuration
The API server configuration in
start_api_servermixes security header configuration with tracing and CORS layers. This creates a single function responsible for multiple orthogonal concerns (tracing, CORS, security headers), which increases coupling and makes it harder to reason about the impact of changes to any one aspect.Suggested fix: Separate the layer configuration into distinct sections or extract security header configuration into its own function to improve modularity and reduce the cognitive load when reviewing changes to individual middleware components.
*Scanner: code-review/complexity | *
@@ -18,0 +22,4 @@axum::http::header::STRICT_TRANSPORT_SECURITY,HeaderValue::from_static("max-age=31536000; includeSubDomains"),)).layer(SetResponseHeaderLayer::overriding([medium] Potential panic in security headers layer setup
The security headers are set using
HeaderValue::from_staticwhich can panic at runtime if the header values are invalid. While the values appear correct, this pattern could be fragile if configuration changes are made later without proper validation.Suggested fix: Consider using
HeaderValue::try_fromwith proper error handling instead offrom_staticto prevent potential panics during header value construction.*Scanner: code-review/convention | *
@@ -332,0 +324,4 @@for alert in &cve_alerts {// Upsert the alertlet filter = doc! {[medium] Potential Command Injection via User-Controlled Repository Name
The repository name is directly used in a logging statement without sanitization. If an attacker can control the repository name through external input (e.g., via API or configuration), they could inject malicious data into logs or potentially influence log parsing systems. While this doesn't directly lead to RCE, it's a potential vector for log-based attacks or information leakage.
Suggested fix: Sanitize or escape the repository name before including it in log messages. Consider using a structured logging approach that prevents injection.
Scanner: code-review/security | CWE: CWE-77
@@ -332,0 +327,4 @@let filter = doc! {"cve_id": &alert.cve_id,"repo_id": &alert.repo_id,};[medium] Complex boolean expression in notification creation logic
The notification creation logic contains a complex boolean expression involving multiple conditions and nested operations that could be simplified for better readability and reduced risk of logical errors.
Suggested fix: Extract the notification creation and upsert logic into a separate function with clear parameter validation, or refactor the nested conditionals into a more readable form using early returns.
*Scanner: code-review/complexity | *
@@ -332,0 +332,4 @@.map(|d| doc! { "$set": d }).unwrap_or_else(|_| doc! {});self.db.cve_alerts()[medium] Inconsistent error handling with unwrap() in database operations
The code uses
.unwrap_or_else(|_| doc! {})and.unwrap_or_default()which can hide serialization errors. This creates inconsistent error handling within the same module where failures might be silently ignored instead of being properly propagated or logged.Suggested fix: Replace unwrap_or_else with proper error handling using ? operator or explicit error logging to ensure serialization failures are not silently ignored.
*Scanner: code-review/convention | *
@@ -332,0 +335,4 @@.cve_alerts().update_one(filter, update).upsert(true).await?;[high] Incorrect upsert logic for CVE notifications
The code uses
$setOnInsertto create notifications, but the filter used for checking duplicates ('cve_id + repo + package + version') may not correctly identify all duplicate notifications. If an existing notification has different values for other fields (like summary or URL), it won't be updated even though it should be. Additionally, the notification creation logic doesn't handle potential BSON conversion errors properly, which could lead to silent failure.Suggested fix: Ensure that the notification deduplication filter is comprehensive enough to catch all relevant duplicates. Also, consider using a more robust error handling mechanism instead of
.unwrap_or_default()when converting to BSON.*Scanner: code-review/logic | *
@@ -332,0 +340,4 @@// Create notification (dedup by cve_id + repo + package + version)let notif_filter = doc! {"cve_id": &alert.cve_id,"repo_id": &alert.repo_id,[high] Insecure Direct Object Reference in Notification Creation
The code creates notifications based on CVE alerts but does not validate that the user has permission to access or modify the associated repository. This could allow a malicious actor to create notifications for repositories they don't own, potentially leading to unauthorized data exposure or manipulation.
Suggested fix: Add authorization checks to ensure that the authenticated user has appropriate permissions for the repository before creating notifications.
Scanner: code-review/security | CWE: CWE-639
@@ -332,0 +351,4 @@repo_name.clone(),alert.affected_package.clone(),alert.affected_version.clone(),severity,[medium] Potential panic from unwrap_or_default() in notification creation
The code calls
.unwrap_or_default()onmongodb::bson::to_bson(¬ification)which can panic if the notification struct cannot be converted to BSON. This is an anti-pattern in library code where panics can crash the entire service.Suggested fix: Replace unwrap_or_default() with proper error handling using ? operator or provide a default value that doesn't rely on unwrapping.
*Scanner: code-review/convention | *
[medium] Inconsistent error handling in notification creation loop
The code handles errors from the database operation with
if let Ok(result) = ...but then ignores potential errors from themongodb::bson::to_bson(¬ification).unwrap_or_default()call. This inconsistency in error handling could lead to silent failures when creating notifications.Suggested fix: Use proper error handling for the bson conversion similar to how the database operation is handled, or at least log the error if it occurs.
*Scanner: code-review/convention | *
@@ -0,0 +19,4 @@rsx! {button {class: class,title: if copied() { "Copied!" } else { "Copy to clipboard" },[medium] Potential security vulnerability in JavaScript string escaping
The JavaScript string escaping logic only handles backslash, single quote, newline, and carriage return characters. However, it doesn't escape other potentially dangerous characters like double quotes or control characters that could break out of the JavaScript string context when used in certain contexts, potentially leading to XSS vulnerabilities.
Suggested fix: Use a more comprehensive escaping mechanism or a dedicated JavaScript escaping library to properly escape all special characters that could break out of JavaScript string literals.
*Scanner: code-review/convention | *
@@ -0,0 +21,4 @@class: class,title: if copied() { "Copied!" } else { "Copy to clipboard" },onclick: move |_| {let val = value.clone();[high] Incorrect JavaScript string escaping in copy functionality
The JavaScript string escaping in the copy button implementation doesn't properly handle all cases that could break the generated JavaScript. Specifically, if the copied text contains a single quote followed by a backslash, it will result in malformed JavaScript. For example, if
valueis "It's a test", the escaped version becomes "It\'s a test" which when embedded in the JS string literal becomes: navigator.clipboard.writeText('It\'s a test') - this will not work correctly because the single quote is not properly escaped within the JS string.Suggested fix: Use a proper JavaScript string escaping function or encode the value using a more robust method like JSON.stringify instead of manual replacement. Alternatively, use a template literal approach with proper escaping.
*Scanner: code-review/logic | *
@@ -0,0 +24,4 @@let val = value.clone();// Escape for JS single-quoted stringlet escaped = val.replace('\\', "\\\\")[high] Potential XSS via Copy Button
The CopyButton component directly incorporates user-provided 'value' into JavaScript code without proper sanitization. While the code attempts to escape certain characters, it's insufficient to prevent XSS when the value contains malicious content that could be interpreted as JavaScript when executed in the browser context.
Suggested fix: Use a more robust escaping mechanism or avoid inline JavaScript execution entirely. Consider using a safer method like creating a temporary textarea element and using document.execCommand('copy') instead of navigator.clipboard.writeText().
Scanner: code-review/security | CWE: CWE-79
[medium] Complex boolean expression in conditional rendering
The conditional rendering logic uses a complex boolean expression with multiple nested conditions that could be simplified for better readability and reduced bug risk. The 'if copied()' condition is used both for determining the button's icon and for setting the title attribute.
Suggested fix: Extract the conditional logic into separate variables or functions to make the intent clearer and reduce the chance of errors when modifying the component behavior.
*Scanner: code-review/complexity | *
@@ -0,0 +25,4 @@// Escape for JS single-quoted stringlet escaped = val.replace('\\', "\\\\").replace('\'', "\\'")[medium] Inconsistent error handling in copy button
The CopyButton component uses
document::eval(&js)to execute JavaScript for copying to clipboard, but doesn't handle potential errors from the clipboard API. If the clipboard operation fails (which can happen due to browser permissions or other restrictions), the UI will still show success (copied state) without any indication of failure.Suggested fix: Add proper error handling around the clipboard operation by either catching JavaScript errors or using a more robust clipboard API approach that provides feedback on success/failure.
*Scanner: code-review/convention | *
@@ -0,0 +29,4 @@.replace('\n', "\\n").replace('\r', "\\r");let js = format!("navigator.clipboard.writeText('{escaped}')");document::eval(&js);[medium] Inconsistent async behavior between web and non-web targets
The CopyButton component uses different async mechanisms for different targets (gloo_timers::future::TimeoutFuture for web, tokio::time::sleep for non-web). This inconsistency could lead to different timing behaviors or potential runtime errors if the non-web target doesn't have tokio available or if the async runtime isn't properly configured.
Suggested fix: Standardize on a single async approach or ensure both paths are properly handled with appropriate feature detection and fallbacks.
*Scanner: code-review/convention | *
@@ -145,0 +142,4 @@code {style: "font-size: 11px; word-break: break-all; user-select: all;",if ssh_public_key().is_empty() {"Loading..."[low] Potential duplicate computation of ssh_public_key()
The
ssh_public_key()function is called twice in the same conditional block - once for the display text and once for the CopyButton component. This could be inefficient if the function involves expensive operations or network calls.Suggested fix: Store the result of
ssh_public_key()in a local variable before the conditional check to avoid redundant calls.*Scanner: code-review/convention | *
[medium] Complex boolean expression in conditional rendering
The code uses repeated calls to
ssh_public_key()which could lead to performance issues or unexpected behavior if the function has side effects. The same check!ssh_public_key().is_empty()appears twice in the same component.Suggested fix: Store the result of
ssh_public_key()in a local variable before checking its emptiness to avoid redundant function calls and potential side effects.*Scanner: code-review/complexity | *
[medium] Potential race condition in SSH public key display
The SSH public key is checked for emptiness twice - once for displaying 'Loading...' and again for determining whether to show the copy button. If the key becomes empty between these two checks, it could result in inconsistent UI state where the copy button appears but the key is actually empty.
Suggested fix: Store the SSH public key result in a local variable before checking its emptiness to ensure consistency across both conditions.
*Scanner: code-review/logic | *
@@ -406,0 +402,4 @@let origin = web_sys::window().and_then(|w: web_sys::Window| w.location().origin().ok()).unwrap_or_default();#[cfg(not(feature = "web"))][medium] Potential Information Disclosure via Copy Button Implementation
The implementation introduces copy buttons for SSH public keys, webhook URLs, and secrets. While the copy functionality itself is not inherently vulnerable, the way these values are exposed through the UI could potentially lead to information disclosure if the page is cached or logged. Specifically, the secret value is directly rendered in an input field and made available via the copy button, which could be exploited if the application's logging or caching mechanisms capture this content.
Suggested fix: Ensure that sensitive values like secrets are not logged or cached in any form. Consider implementing additional safeguards such as disabling browser autocomplete for sensitive fields and ensuring that the copy functionality does not inadvertently expose these values through other means (e.g., browser history, developer tools).
Scanner: code-review/security | CWE: CWE-200
[medium] Inconsistent error handling with unwrap_or_default() in web feature
The code uses
unwrap_or_default()onw.location().origin().ok()which can mask potential errors in URL parsing. This could lead to unexpected behavior when the origin cannot be determined, especially in non-web environments where the window object might not be available.Suggested fix: Consider using a more explicit error handling approach or logging when the origin cannot be determined, rather than silently falling back to an empty string.
*Scanner: code-review/convention | *
@@ -406,0 +414,4 @@value: "{webhook_url}",}crate::components::copy_button::CopyButton { value: webhook_url.clone() }}[medium] Inconsistent use of clone() for webhook URL
The webhook URL is cloned when passed to the CopyButton component, but the input field uses a string literal reference. This inconsistency could lead to unnecessary allocations or potential issues if the value needs to be used elsewhere after the clone operation.
Suggested fix: Use the same approach for both the input field and the copy button - either clone the value consistently or pass a reference to avoid unnecessary allocations.
*Scanner: code-review/logic | *
@@ -415,0 +425,4 @@r#type: "text",readonly: true,style: "font-family: monospace; font-size: 12px; flex: 1;",value: "{secret}",[low] Redundant clone() in CopyButton component
The
secret.clone()call in the CopyButton component creates an unnecessary clone of the secret string. Since the value is already owned by the component, cloning it just to pass it to another component is wasteful.Suggested fix: Pass the secret directly without cloning, or ensure the CopyButton component accepts a reference instead of owned values.
*Scanner: code-review/convention | *