fix: CVE notifications during scan + help chat doc loading + Dockerfile #55
@@ -33,6 +33,11 @@ RUN pip3 install --break-system-packages ruff
|
|||||||
|
|
||||||
COPY --from=builder /app/target/release/compliance-agent /usr/local/bin/compliance-agent
|
COPY --from=builder /app/target/release/compliance-agent /usr/local/bin/compliance-agent
|
||||||
|
|
||||||
|
# Copy documentation for the help chat assistant
|
||||||
|
COPY --from=builder /app/README.md /app/README.md
|
||||||
|
COPY --from=builder /app/docs /app/docs
|
||||||
|
ENV HELP_DOCS_PATH=/app
|
||||||
|
|
||||||
# Ensure SSH key directory exists
|
# Ensure SSH key directory exists
|
||||||
RUN mkdir -p /data/compliance-scanner/ssh
|
RUN mkdir -p /data/compliance-scanner/ssh
|
||||||
|
|
||||||
|
|||||||
@@ -104,28 +104,58 @@ fn load_docs(root: &Path) -> String {
|
|||||||
|
|
||||||
/// Returns a reference to the cached doc context string, initialised on
|
/// Returns a reference to the cached doc context string, initialised on
|
||||||
/// first call via `OnceLock`.
|
/// first call via `OnceLock`.
|
||||||
|
///
|
||||||
|
|
|||||||
|
/// Discovery order:
|
||||||
|
/// 1. `HELP_DOCS_PATH` env var (explicit override)
|
||||||
|
/// 2. Walk up from the binary location
|
||||||
sharang
commented
[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 **[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*
<!-- compliance-fp:00aa66f7e2ffea3b1a8ee39c326749a4827b79e42fd515371710af68bed01b10 -->
sharang
commented
[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 **[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*
<!-- compliance-fp:00aa66f7e2ffea3b1a8ee39c326749a4827b79e42fd515371710af68bed01b10 -->
|
|||||||
|
/// 3. Current working directory
|
||||||
|
sharang
commented
[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 | * **[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 | *
<!-- compliance-fp:7dc65b429f3fce19afde4f5eb7635f94760e1d52d5c3880dc7d6f987556d63d2 -->
|
|||||||
|
/// 4. Common Docker paths (/app, /opt/compliance-scanner)
|
||||||
|
sharang
commented
[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] 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*
<!-- compliance-fp:61715420584e0ba7f70a3fadb2b0e3b1d1f159565211c7ec5d0816205cdcfb23 -->
sharang
commented
[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 **[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*
<!-- compliance-fp:134ca9b1f535e609f77f7975c9740c5cad3a678d0f68347f347163ff64e537e4 -->
|
|||||||
fn doc_context() -> &'static str {
|
fn doc_context() -> &'static str {
|
||||||
|
sharang
commented
[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 **[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*
<!-- compliance-fp:5eca5a85566090e4f096a694085036223ea94cd61dbb2c99ef69f8416229cec7 -->
|
|||||||
DOC_CONTEXT.get_or_init(|| {
|
DOC_CONTEXT.get_or_init(|| {
|
||||||
|
sharang
commented
[medium] Inconsistent error handling in doc_context function The Suggested fix: Use consistent error handling throughout. Replace *Scanner: code-review/convention | * **[medium] Inconsistent error handling in doc_context function**
The `doc_context` function uses inconsistent error handling patterns. It uses `unwrap_or_else` for `std::env::current_dir()` but `ok()` followed by `and_then` for `std::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("."))` with `ok().and_then(|p| p.parent().map(Path::to_path_buf)).unwrap_or_else(|| PathBuf::from("."))` to maintain consistency.
*Scanner: code-review/convention | *
<!-- compliance-fp:c75caa8760702eeb637b6cb99e496754100caf0208767e35eb6b3c7f3e7fc1f7 -->
sharang
commented
[medium] Inconsistent error handling in doc_context function The Suggested fix: Use consistent error handling throughout. Replace *Scanner: code-review/convention | * **[medium] Inconsistent error handling in doc_context function**
The `doc_context` function uses inconsistent error handling patterns. It uses `unwrap_or_else` for `std::env::current_dir()` but `ok()` followed by `and_then` for `std::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("."))` with `ok().and_then(|p| p.parent().map(Path::to_path_buf)).unwrap_or_else(|| PathBuf::from("."))` to maintain consistency.
*Scanner: code-review/convention | *
<!-- compliance-fp:c75caa8760702eeb637b6cb99e496754100caf0208767e35eb6b3c7f3e7fc1f7 -->
|
|||||||
|
// 1. Explicit env var
|
||||||
|
sharang
commented
[medium] Inconsistent error handling in doc_context function The Suggested fix: Use consistent error handling approach throughout. Consider using *Scanner: code-review/convention | * **[medium] Inconsistent error handling in doc_context function**
The `doc_context` function uses inconsistent error handling patterns. It uses `unwrap_or_else` for `std::env::current_dir()` but `ok()` followed by `and_then` for `std::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 or `unwrap_or_else` consistently for all path operations.
*Scanner: code-review/convention | *
<!-- compliance-fp:8159a43e8ee2b244c58a28a6c77cbfb4e7eb88f61b98d60af6c93be1ac86ed46 -->
|
|||||||
|
if let Ok(path) = std::env::var("HELP_DOCS_PATH") {
|
||||||
|
let p = PathBuf::from(&path);
|
||||||
|
sharang
commented
[medium] Inconsistent error handling in doc context loading In Suggested fix: Standardize error handling approach throughout the function, either consistently using *Scanner: code-review/convention | * **[medium] Inconsistent error handling in doc context loading**
In `compliance-agent/src/api/handlers/help_chat.rs`, the code mixes `unwrap_or_else` with explicit error logging and `ok()` chaining. The inconsistency in how errors are handled (some use `unwrap_or_else`, others use `ok()` and `and_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 consistent `unwrap_or_else` patterns with appropriate logging.
*Scanner: code-review/convention | *
<!-- compliance-fp:33c028f66996ed5f511f26b8a43411d5dc9fc0689e74a9f9bc61d5e717c2ad70 -->
|
|||||||
|
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);
|
||||||
|
}
|
||||||
|
sharang
commented
[medium] Potential panic from unwrap in help chat documentation loading In Suggested fix: Ensure the fallback path handles all possible error cases gracefully without relying on assumptions about filesystem state. *Scanner: code-review/convention | * **[medium] Potential panic from unwrap in help chat documentation loading**
In `compliance-agent/src/api/handlers/help_chat.rs`, line 121 uses `unwrap_or_else(|| PathBuf::from("."))` which can panic if `std::env::current_exe()` fails. While it's wrapped in `ok()`, 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 | *
<!-- compliance-fp:79d043dff27dfadb733d7394c689892f81c4c7b0e9fc464472589c70fa3c3388 -->
|
|||||||
|
tracing::warn!("help_chat: HELP_DOCS_PATH={path} has no README.md or docs/");
|
||||||
|
}
|
||||||
|
sharang
commented
[high] Incorrect fallback logic in doc_context() when walking up from binary location In the 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 | * **[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 because `find_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 | *
<!-- compliance-fp:61023414dd2575a397f863199350fb483c3c1a1db3908f65bce5bd69e16459f1 -->
|
|||||||
|
|
||||||
|
// 2. Walk up from binary location
|
||||||
|
sharang
commented
[high] Incorrect fallback logic in doc_context() when walking up from binary location In the Suggested fix: Move the current working directory check outside of the *Scanner: code-review/logic | * **[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 | *
<!-- compliance-fp:bd2858b2bd5c9b82d622cadd5d5f35ab4b82c425b14ecc775e919c37dc0f9124 -->
sharang
commented
[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 | * **[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 | *
<!-- compliance-fp:e1f25705557e3bcb6b4425144f13960e51b83a44d83a153f145128aa2d6847ca -->
sharang
commented
[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 | * **[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 | *
<!-- compliance-fp:e1f25705557e3bcb6b4425144f13960e51b83a44d83a153f145128aa2d6847ca -->
|
|||||||
let start = std::env::current_exe()
|
let start = std::env::current_exe()
|
||||||
.ok()
|
.ok()
|
||||||
.and_then(|p| p.parent().map(Path::to_path_buf))
|
.and_then(|p| p.parent().map(Path::to_path_buf))
|
||||||
.unwrap_or_else(|| PathBuf::from("."));
|
.unwrap_or_else(|| PathBuf::from("."));
|
||||||
|
|
||||||
|
sharang
commented
[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 **[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*
<!-- compliance-fp:b572a798e69f5b67e66de818040066ff1c0ad0f43b56c152338be92c8ecd114d -->
sharang
commented
[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 **[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*
<!-- compliance-fp:b572a798e69f5b67e66de818040066ff1c0ad0f43b56c152338be92c8ecd114d -->
|
|||||||
match find_project_root(&start) {
|
if let Some(root) = find_project_root(&start) {
|
||||||
Some(root) => load_docs(&root),
|
return load_docs(&root);
|
||||||
None => {
|
}
|
||||||
// Fallback: try current working directory
|
|
||||||
let cwd = std::env::current_dir().unwrap_or_else(|_| PathBuf::from("."));
|
// 3. Current working directory
|
||||||
|
if let Ok(cwd) = std::env::current_dir() {
|
||||||
|
if let Some(root) = find_project_root(&cwd) {
|
||||||
|
return load_docs(&root);
|
||||||
|
}
|
||||||
if cwd.join("README.md").is_file() {
|
if cwd.join("README.md").is_file() {
|
||||||
return load_docs(&cwd);
|
return load_docs(&cwd);
|
||||||
}
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
// 4. Common Docker/deployment paths
|
||||||
|
sharang
commented
[low] Missing explicit error handling for missing documentation files The documentation loading logic in 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 | * **[low] Missing explicit error handling for missing documentation files**
The documentation loading logic in `doc_context` falls 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 | *
<!-- compliance-fp:0110c439fc7b149c397417112d7b7842804b90f777aed9f84335a28bd6df4a3b -->
|
|||||||
|
for candidate in ["/app", "/opt/compliance-scanner", "/srv/compliance-scanner"] {
|
||||||
|
let p = PathBuf::from(candidate);
|
||||||
|
sharang
commented
[medium] Missing error propagation in help chat documentation loading The 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 | * **[medium] Missing error propagation in help chat documentation loading**
The `doc_context` function 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 | *
<!-- compliance-fp:35000ffa8dc26256e2753e03953bbc60e27e369bb61a8530f1b3885b51607ac6 -->
sharang
commented
[medium] Missing error propagation in help chat documentation loading The 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 | * **[medium] Missing error propagation in help chat documentation loading**
The `doc_context` function 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 | *
<!-- compliance-fp:35000ffa8dc26256e2753e03953bbc60e27e369bb61a8530f1b3885b51607ac6 -->
|
|||||||
|
if p.join("README.md").is_file() || p.join("docs").is_dir() {
|
||||||
|
tracing::info!("help_chat: found docs at {candidate}");
|
||||||
|
return load_docs(&p);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
tracing::error!(
|
tracing::error!(
|
||||||
"help_chat: could not locate project root from {}; doc context will be empty",
|
"help_chat: could not locate project root; doc context will be empty. \
|
||||||
start.display()
|
Set HELP_DOCS_PATH to the directory containing README.md and docs/"
|
||||||
);
|
);
|
||||||
String::new()
|
String::new()
|
||||||
}
|
|
||||||
}
|
|
||||||
})
|
})
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -315,8 +315,15 @@ impl PipelineOrchestrator {
|
|||||||
.await?;
|
.await?;
|
||||||
}
|
}
|
||||||
|
|
||||||
// Persist CVE alerts (upsert by cve_id + repo_id)
|
// Persist CVE alerts and create notifications
|
||||||
|
{
|
||||||
|
use compliance_core::models::notification::{parse_severity, CveNotification};
|
||||||
|
sharang
commented
[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 | * **[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 | *
<!-- compliance-fp:3929573118957a6a558a5b1532813d58b7813f81b7a12f290290c6dcffaa9306 -->
|
|||||||
|
|
||||||
|
let repo_name = repo.name.clone();
|
||||||
|
let mut new_notif_count = 0u32;
|
||||||
|
|
||||||
for alert in &cve_alerts {
|
for alert in &cve_alerts {
|
||||||
|
// Upsert the alert
|
||||||
let filter = doc! {
|
let filter = doc! {
|
||||||
|
sharang
commented
[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 **[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*
<!-- compliance-fp:a075f965458cb0bfa8e1ee2d9356386bf867485fab75ab66dcfac8a11864fefe -->
sharang
commented
[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 **[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*
<!-- compliance-fp:a075f965458cb0bfa8e1ee2d9356386bf867485fab75ab66dcfac8a11864fefe -->
|
|||||||
"cve_id": &alert.cve_id,
|
"cve_id": &alert.cve_id,
|
||||||
"repo_id": &alert.repo_id,
|
"repo_id": &alert.repo_id,
|
||||||
@@ -329,6 +336,46 @@ impl PipelineOrchestrator {
|
|||||||
.update_one(filter, update)
|
.update_one(filter, update)
|
||||||
.upsert(true)
|
.upsert(true)
|
||||||
.await?;
|
.await?;
|
||||||
|
sharang
commented
[high] Incorrect upsert logic for CVE notifications The code uses 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 *Scanner: code-review/logic | * **[high] Incorrect upsert logic for CVE notifications**
The code uses `$setOnInsert` to 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 | *
<!-- compliance-fp:df6a18453fe7e474f92f9e88bea252239905b755ced17e65b0fad7fc9aea2a17 -->
sharang
commented
[high] Incorrect upsert logic for CVE notifications The code uses 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 *Scanner: code-review/logic | * **[high] Incorrect upsert logic for CVE notifications**
The code uses `$setOnInsert` to 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 | *
<!-- compliance-fp:df6a18453fe7e474f92f9e88bea252239905b755ced17e65b0fad7fc9aea2a17 -->
|
|||||||
|
|
||||||
|
// Create notification (dedup by cve_id + repo + package + version)
|
||||||
|
let notif_filter = doc! {
|
||||||
|
sharang
commented
[medium] Unwrapped MongoDB BSON conversion in CVE alert persistence In Suggested fix: Replace unwraps with proper error handling using *Scanner: code-review/convention | * **[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 | *
<!-- compliance-fp:fcd0cac0688b79c023cecce0b945651bc7c542c4ce2907bb27d4dd544d3322ed -->
|
|||||||
|
"cve_id": &alert.cve_id,
|
||||||
|
"repo_id": &alert.repo_id,
|
||||||
|
sharang
commented
[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 **[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*
<!-- compliance-fp:0b184fda0c6104d8341dcd3a030b072e33c2661980bbf6421e9829ad0ff714e0 -->
sharang
commented
[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 **[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*
<!-- compliance-fp:0b184fda0c6104d8341dcd3a030b072e33c2661980bbf6421e9829ad0ff714e0 -->
|
|||||||
|
"package_name": &alert.affected_package,
|
||||||
|
"package_version": &alert.affected_version,
|
||||||
|
};
|
||||||
|
let severity = parse_severity(alert.severity.as_deref(), alert.cvss_score);
|
||||||
|
let mut notification = CveNotification::new(
|
||||||
|
alert.cve_id.clone(),
|
||||||
|
sharang
commented
[high] Potential panic in notification creation due to unwrap_or_default() usage In the pipeline orchestrator, when creating notifications, the code uses Suggested fix: Replace *Scanner: code-review/logic | * **[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-fp:39bd762291403bd7e5c4252f17f1c5d6010aa6010df84bcb5b6d9e22689bb597 -->
sharang
commented
[high] Potential panic in notification creation The code uses Suggested fix: Replace *Scanner: code-review/logic | * **[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 | *
<!-- compliance-fp:6384481dcb77ec79193d7bfbdc56b4cf7b84ff4c2d88a0b88ea05126c91d2c1b -->
|
|||||||
|
repo_id.clone(),
|
||||||
|
repo_name.clone(),
|
||||||
|
sharang
commented
[medium] Inconsistent error handling with upsert operation The code handles the result of the upsert operation for notifications with a 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 | * **[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-fp:73924f1829a8bbfde69e5809b6d2205d4169134fa5648d88817dd946b912aa30 -->
|
|||||||
|
alert.affected_package.clone(),
|
||||||
|
alert.affected_version.clone(),
|
||||||
|
severity,
|
||||||
|
sharang
commented
[medium] Potential panic from unwrap_or_default() in notification creation The code calls 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] Potential panic from unwrap_or_default() in notification creation**
The code calls `.unwrap_or_default()` on `mongodb::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 | *
<!-- compliance-fp:1202ca6d5c8814de4ef1846326fca483741c12e6d8a44326983dbc07f91b97c5 -->
sharang
commented
[medium] Inconsistent error handling in notification creation loop The code handles errors from the database operation with 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 | * **[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 the `mongodb::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 | *
<!-- compliance-fp:e94539fc27c1864a6376acaa88576f419ea113d1329a6e03dea47612e0b0d088 -->
sharang
commented
[medium] Potential panic from unwrap_or_default() in notification creation The code calls 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] Potential panic from unwrap_or_default() in notification creation**
The code calls `.unwrap_or_default()` on `mongodb::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 | *
<!-- compliance-fp:1202ca6d5c8814de4ef1846326fca483741c12e6d8a44326983dbc07f91b97c5 -->
sharang
commented
[medium] Inconsistent error handling in notification creation loop The code handles errors from the database operation with 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 | * **[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 the `mongodb::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 | *
<!-- compliance-fp:e94539fc27c1864a6376acaa88576f419ea113d1329a6e03dea47612e0b0d088 -->
|
|||||||
|
);
|
||||||
|
notification.cvss_score = alert.cvss_score;
|
||||||
|
notification.summary = alert.summary.clone();
|
||||||
|
notification.url = Some(format!("https://osv.dev/vulnerability/{}", alert.cve_id));
|
||||||
|
|
||||||
|
let notif_update = doc! {
|
||||||
|
"$setOnInsert": mongodb::bson::to_bson(¬ification).unwrap_or_default()
|
||||||
|
};
|
||||||
|
if let Ok(result) = self
|
||||||
|
.db
|
||||||
|
.cve_notifications()
|
||||||
|
.update_one(notif_filter, notif_update)
|
||||||
|
.upsert(true)
|
||||||
|
.await
|
||||||
|
{
|
||||||
|
if result.upserted_id.is_some() {
|
||||||
|
new_notif_count += 1;
|
||||||
|
}
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
if new_notif_count > 0 {
|
||||||
|
tracing::info!("[{repo_id}] Created {new_notif_count} CVE notification(s)");
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
// Stage 6: Issue Creation
|
// Stage 6: Issue Creation
|
||||||
|
|||||||
[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 | *
[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 | *
[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 | *