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 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
|
||||
RUN mkdir -p /data/compliance-scanner/ssh
|
||||
|
||||
|
||||
@@ -25,7 +25,7 @@ uuid = { workspace = true }
|
||||
secrecy = { workspace = true }
|
||||
regex = { workspace = true }
|
||||
axum = "0.8"
|
||||
tower-http = { version = "0.6", features = ["cors", "trace"] }
|
||||
tower-http = { version = "0.6", features = ["cors", "trace", "set-header"] }
|
||||
git2 = "0.20"
|
||||
octocrab = "0.44"
|
||||
tokio-cron-scheduler = "0.13"
|
||||
|
||||
@@ -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`.
|
||||
///
|
||||
|
|
||||
/// 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 {
|
||||
|
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(|| {
|
||||
|
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()
|
||||
.ok()
|
||||
.and_then(|p| p.parent().map(Path::to_path_buf))
|
||||
.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) {
|
||||
Some(root) => load_docs(&root),
|
||||
None => {
|
||||
// Fallback: try current working directory
|
||||
let cwd = std::env::current_dir().unwrap_or_else(|_| PathBuf::from("."));
|
||||
if cwd.join("README.md").is_file() {
|
||||
return load_docs(&cwd);
|
||||
}
|
||||
tracing::error!(
|
||||
"help_chat: could not locate project root from {}; doc context will be empty",
|
||||
start.display()
|
||||
);
|
||||
String::new()
|
||||
if let Some(root) = find_project_root(&start) {
|
||||
return load_docs(&root);
|
||||
}
|
||||
|
||||
// 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() {
|
||||
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!(
|
||||
"help_chat: could not locate project root; doc context will be empty. \
|
||||
Set HELP_DOCS_PATH to the directory containing README.md and docs/"
|
||||
);
|
||||
String::new()
|
||||
})
|
||||
}
|
||||
|
||||
|
||||
@@ -1,8 +1,10 @@
|
||||
use std::sync::Arc;
|
||||
|
||||
use axum::http::HeaderValue;
|
||||
use axum::{middleware, Extension};
|
||||
use tokio::sync::RwLock;
|
||||
use tower_http::cors::CorsLayer;
|
||||
use tower_http::set_header::SetResponseHeaderLayer;
|
||||
use tower_http::trace::TraceLayer;
|
||||
|
||||
use crate::agent::ComplianceAgent;
|
||||
@@ -14,7 +16,24 @@ pub async fn start_api_server(agent: ComplianceAgent, port: u16) -> Result<(), A
|
||||
let mut app = routes::build_router()
|
||||
.layer(Extension(Arc::new(agent.clone())))
|
||||
.layer(CorsLayer::permissive())
|
||||
.layer(TraceLayer::new_for_http());
|
||||
.layer(TraceLayer::new_for_http())
|
||||
// Security headers (defense-in-depth, primary enforcement via Traefik)
|
||||
|
sharang
commented
[medium] Multiple interleaved responsibilities in API server layer configuration The API server configuration in 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 | * **[medium] Multiple interleaved responsibilities in API server layer configuration**
The API server configuration in `start_api_server` mixes 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 | *
<!-- compliance-fp:cfa3d4742885ed8e7cc4978c4bac2cc944b611fbcc21c89dee64b50c19136eb5 -->
|
||||
.layer(SetResponseHeaderLayer::overriding(
|
||||
axum::http::header::STRICT_TRANSPORT_SECURITY,
|
||||
HeaderValue::from_static("max-age=31536000; includeSubDomains"),
|
||||
|
sharang
commented
[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 | * **[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 | *
<!-- compliance-fp:e798830824894b5925dd637b0aee1bb13c3cbbb8b0c9bfe31670ffb2bf4a6382 -->
sharang
commented
[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 | * **[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 | *
<!-- compliance-fp:e798830824894b5925dd637b0aee1bb13c3cbbb8b0c9bfe31670ffb2bf4a6382 -->
|
||||
))
|
||||
.layer(SetResponseHeaderLayer::overriding(
|
||||
|
sharang
commented
[low] Potential panic in security header configuration The security header configuration uses Suggested fix: Consider using *Scanner: code-review/convention | * **[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 | *
<!-- compliance-fp:636dcbdcbcb853a5757ce79c40e14a22fe93b0a58e7bc81c706e172f0a61f8f9 -->
sharang
commented
[low] Potential panic in security header configuration The security header configuration uses Suggested fix: Consider using *Scanner: code-review/convention | * **[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 | *
<!-- compliance-fp:636dcbdcbcb853a5757ce79c40e14a22fe93b0a58e7bc81c706e172f0a61f8f9 -->
sharang
commented
[medium] Potential panic in security headers layer setup The security headers are set using Suggested fix: Consider using *Scanner: code-review/convention | * **[medium] Potential panic in security headers layer setup**
The security headers are set using `HeaderValue::from_static` which 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_from` with proper error handling instead of `from_static` to prevent potential panics during header value construction.
*Scanner: code-review/convention | *
<!-- compliance-fp:b87da311777ac8e85d01e46443bb47c79442d5c67c9d26f4eae3cd92249c5ed3 -->
|
||||
axum::http::header::X_FRAME_OPTIONS,
|
||||
HeaderValue::from_static("DENY"),
|
||||
))
|
||||
.layer(SetResponseHeaderLayer::overriding(
|
||||
axum::http::header::X_CONTENT_TYPE_OPTIONS,
|
||||
HeaderValue::from_static("nosniff"),
|
||||
))
|
||||
.layer(SetResponseHeaderLayer::overriding(
|
||||
axum::http::header::REFERRER_POLICY,
|
||||
HeaderValue::from_static("strict-origin-when-cross-origin"),
|
||||
));
|
||||
|
||||
if let (Some(kc_url), Some(kc_realm)) =
|
||||
(&agent.config.keycloak_url, &agent.config.keycloak_realm)
|
||||
|
||||
@@ -315,20 +315,67 @@ impl PipelineOrchestrator {
|
||||
.await?;
|
||||
}
|
||||
|
||||
// Persist CVE alerts (upsert by cve_id + repo_id)
|
||||
for alert in &cve_alerts {
|
||||
let filter = doc! {
|
||||
"cve_id": &alert.cve_id,
|
||||
"repo_id": &alert.repo_id,
|
||||
};
|
||||
let update = mongodb::bson::to_document(alert)
|
||||
.map(|d| doc! { "$set": d })
|
||||
.unwrap_or_else(|_| doc! {});
|
||||
self.db
|
||||
.cve_alerts()
|
||||
.update_one(filter, update)
|
||||
.upsert(true)
|
||||
.await?;
|
||||
// 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 {
|
||||
// Upsert the alert
|
||||
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,
|
||||
"repo_id": &alert.repo_id,
|
||||
};
|
||||
|
sharang
commented
[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 | * **[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 | *
<!-- compliance-fp:7ca2d159b9c13ac87c142262a33945b36612d1ed1e907fcaafc92c36e464d192 -->
sharang
commented
[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 | * **[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 | *
<!-- compliance-fp:7ca2d159b9c13ac87c142262a33945b36612d1ed1e907fcaafc92c36e464d192 -->
|
||||
let update = mongodb::bson::to_document(alert)
|
||||
.map(|d| doc! { "$set": d })
|
||||
|
sharang
commented
[medium] Potential BSON Serialization Error Leading to Data Corruption The code uses 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] Potential BSON Serialization Error Leading to Data Corruption**
The code uses `mongodb::bson::to_document(alert)` and `mongodb::bson::to_bson(¬ification)` which can fail during serialization. When serialization fails, the code falls back to empty documents (`doc! {}` or `doc! {"$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*
<!-- compliance-fp:84dadb66b688d919a1db18374a22cc8f2235416602981bf5b30004596c157780 -->
sharang
commented
[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] 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 | *
<!-- compliance-fp:c693f270c90062570089be8fde4f39f1c7b26c80ffec74a454780152474211ab -->
sharang
commented
[medium] Inconsistent error handling with unwrap_or_else The code uses Suggested fix: Replace *Scanner: code-review/convention | * **[medium] Inconsistent error handling with unwrap_or_else**
The code uses `unwrap_or_else(|_| doc! {})` and `unwrap_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 | *
<!-- compliance-fp:3675b51807afe8a585e158697eaeea66f075b863f169c513f124da0cfebc7660 -->
|
||||
.unwrap_or_else(|_| doc! {});
|
||||
self.db
|
||||
.cve_alerts()
|
||||
|
sharang
commented
[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 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 | * **[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 subsequent `if 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 | *
<!-- compliance-fp:268961484b51c79946e6877a6ac72562b6af260a54d7e2f2c252a23b53290eee -->
sharang
commented
[medium] Inconsistent error handling with unwrap() in database operations The code uses 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 | * **[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 | *
<!-- compliance-fp:7dd95346c7cae07344a75c6bc2b615b1c5d19742abc61bf141e303ec0ee8efee -->
sharang
commented
[medium] Inconsistent error handling with unwrap() in database operations The code uses 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 | * **[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 | *
<!-- compliance-fp:7dd95346c7cae07344a75c6bc2b615b1c5d19742abc61bf141e303ec0ee8efee -->
|
||||
.update_one(filter, update)
|
||||
.upsert(true)
|
||||
.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
|
||||
|
||||
@@ -3877,3 +3877,15 @@ tbody tr:last-child td {
|
||||
.notification-item-pkg { font-size: 12px; color: var(--text-primary); font-family: 'JetBrains Mono', monospace; }
|
||||
.notification-item-repo { font-size: 11px; color: var(--text-secondary); margin-bottom: 4px; }
|
||||
.notification-item-summary { font-size: 11px; color: var(--text-secondary); line-height: 1.4; display: -webkit-box; -webkit-line-clamp: 2; -webkit-box-orient: vertical; overflow: hidden; }
|
||||
|
||||
/* ═══════════════════════════════════════════════════════════════
|
||||
COPY BUTTON — Reusable clipboard copy component
|
||||
═══════════════════════════════════════════════════════════════ */
|
||||
.copy-btn { background: none; border: 1px solid var(--border); border-radius: 6px; padding: 5px 7px; color: var(--text-secondary); cursor: pointer; display: inline-flex; align-items: center; transition: color 0.15s, border-color 0.15s, background 0.15s; flex-shrink: 0; }
|
||||
.copy-btn:hover { color: var(--accent); border-color: var(--accent); background: var(--accent-muted); }
|
||||
.copy-btn-sm { padding: 3px 5px; border-radius: 4px; }
|
||||
/* Copyable inline field pattern: value + copy button side by side */
|
||||
.copyable { display: flex; align-items: center; gap: 6px; }
|
||||
.copyable code, .copyable .mono { flex: 1; min-width: 0; overflow: hidden; text-overflow: ellipsis; white-space: nowrap; }
|
||||
.code-snippet-wrapper { position: relative; }
|
||||
.code-snippet-header { display: flex; align-items: center; justify-content: space-between; margin-bottom: 4px; gap: 8px; }
|
||||
|
||||
@@ -1,5 +1,7 @@
|
||||
use dioxus::prelude::*;
|
||||
|
||||
use crate::components::copy_button::CopyButton;
|
||||
|
||||
#[component]
|
||||
pub fn CodeSnippet(
|
||||
code: String,
|
||||
@@ -7,15 +9,18 @@ pub fn CodeSnippet(
|
||||
#[props(default)] line_number: u32,
|
||||
) -> Element {
|
||||
rsx! {
|
||||
div {
|
||||
if !file_path.is_empty() {
|
||||
div {
|
||||
style: "font-size: 12px; color: var(--text-secondary); margin-bottom: 4px; font-family: monospace;",
|
||||
"{file_path}"
|
||||
if line_number > 0 {
|
||||
":{line_number}"
|
||||
div { class: "code-snippet-wrapper",
|
||||
div { class: "code-snippet-header",
|
||||
if !file_path.is_empty() {
|
||||
span {
|
||||
style: "font-size: 12px; color: var(--text-secondary); font-family: monospace;",
|
||||
"{file_path}"
|
||||
if line_number > 0 {
|
||||
":{line_number}"
|
||||
}
|
||||
}
|
||||
}
|
||||
CopyButton { value: code.clone(), small: true }
|
||||
}
|
||||
pre { class: "code-block", "{code}" }
|
||||
}
|
||||
|
||||
49
compliance-dashboard/src/components/copy_button.rs
Normal file
@@ -0,0 +1,49 @@
|
||||
use dioxus::prelude::*;
|
||||
use dioxus_free_icons::icons::bs_icons::*;
|
||||
use dioxus_free_icons::Icon;
|
||||
|
||||
/// A small copy-to-clipboard button that shows a checkmark after copying.
|
||||
///
|
||||
/// Usage: `CopyButton { value: "text to copy" }`
|
||||
#[component]
|
||||
pub fn CopyButton(value: String, #[props(default = false)] small: bool) -> Element {
|
||||
let mut copied = use_signal(|| false);
|
||||
|
||||
let size = if small { 12 } else { 14 };
|
||||
let class = if small {
|
||||
"copy-btn copy-btn-sm"
|
||||
} else {
|
||||
"copy-btn"
|
||||
};
|
||||
|
||||
|
sharang
commented
[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 | * **[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 | *
<!-- compliance-fp:d98f12d3c9e9970884fd2bbca4685535c6c55a08ad824421361edef74cce9863 -->
|
||||
rsx! {
|
||||
button {
|
||||
|
sharang
commented
[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 | * **[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 | *
<!-- compliance-fp:62e907b94ed58051a23185bad2f5d84e935d8bc4fc3bca5c12eede2541bf6430 -->
|
||||
class: class,
|
||||
title: if copied() { "Copied!" } else { "Copy to clipboard" },
|
||||
|
sharang
commented
[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 | * **[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 | *
<!-- compliance-fp:a46aa9630125de1e1b4b1490360747499e9f97248f9af42f563623df18c8175b -->
sharang
commented
[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 | * **[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 | *
<!-- compliance-fp:067fb60c50814224f51114ba611aa5fea72282e2db5dd9895be8749d3c59bc35 -->
|
||||
onclick: move |_| {
|
||||
let val = value.clone();
|
||||
|
sharang
commented
[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 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 | * **[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 `value` is "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 | *
<!-- compliance-fp:7cb3d70d745dbaf52acd935b838873788fb638662e5b49b96cc6c05ce2a07505 -->
|
||||
// Escape for JS single-quoted string
|
||||
|
sharang
commented
[medium] Inconsistent error handling in copy button The CopyButton component uses 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 | * **[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 | *
<!-- compliance-fp:e4197dc6c9724e894671628be9b225edf52664ac3dd631e1890740783d6f1cc5 -->
sharang
commented
[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 **[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*
<!-- compliance-fp:40464c023887db6f7315f26111f91822d77feef57d239367c5edd5bc0ba492f9 -->
|
||||
let escaped = val
|
||||
.replace('\\', "\\\\")
|
||||
|
sharang
commented
[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 **[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*
<!-- compliance-fp:f846e60574550662dea9fe90e0e2e40f7dc0c1e669c1096a2d1e77c7f89b07b0 -->
sharang
commented
[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 | * **[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 | *
<!-- compliance-fp:afdff2902065a38a633365c759efaf6cd7e9f479161e3e2e1c1b83d85328a183 -->
|
||||
.replace('\'', "\\'")
|
||||
|
sharang
commented
[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 | * **[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 | *
<!-- compliance-fp:1314f362a2fe49b29ffc8b06d8bfb2d678a13f666bef7e32b57ede106c27cf88 -->
sharang
commented
[medium] Inconsistent error handling in copy button The CopyButton component uses 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 | * **[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 | *
<!-- compliance-fp:ba7d78289cd65f3daed40ba740a4378eb55ee28d01cc6e24c43d997a9d3eba30 -->
|
||||
.replace('\n', "\\n")
|
||||
|
sharang
commented
[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 | * **[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 | *
<!-- compliance-fp:8559df88144649599c41bf223ef2f2b952a1c708827cdfeb7c91825776b339be -->
|
||||
.replace('\r', "\\r");
|
||||
let js = format!("navigator.clipboard.writeText('{escaped}')");
|
||||
document::eval(&js);
|
||||
|
sharang
commented
[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 | * **[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 | *
<!-- compliance-fp:2655fb38ae454e02f649af573b7dbca84badbc20ddc0c6e2347b4e7b294a6797 -->
|
||||
copied.set(true);
|
||||
spawn(async move {
|
||||
#[cfg(feature = "web")]
|
||||
gloo_timers::future::TimeoutFuture::new(2000).await;
|
||||
#[cfg(not(feature = "web"))]
|
||||
tokio::time::sleep(std::time::Duration::from_secs(2)).await;
|
||||
copied.set(false);
|
||||
});
|
||||
},
|
||||
if copied() {
|
||||
Icon { icon: BsCheckLg, width: size, height: size }
|
||||
} else {
|
||||
Icon { icon: BsClipboard, width: size, height: size }
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -2,6 +2,7 @@ pub mod app_shell;
|
||||
pub mod attack_chain;
|
||||
pub mod code_inspector;
|
||||
pub mod code_snippet;
|
||||
pub mod copy_button;
|
||||
pub mod file_tree;
|
||||
pub mod help_chat;
|
||||
pub mod notification_bell;
|
||||
|
||||
@@ -259,7 +259,10 @@ pub fn McpServersPage() -> Element {
|
||||
div { class: "mcp-detail-row",
|
||||
Icon { icon: BsGlobe, width: 13, height: 13 }
|
||||
span { class: "mcp-detail-label", "Endpoint" }
|
||||
code { class: "mcp-detail-value", "{server.endpoint_url}" }
|
||||
div { class: "copyable",
|
||||
code { class: "mcp-detail-value", "{server.endpoint_url}" }
|
||||
crate::components::copy_button::CopyButton { value: server.endpoint_url.clone(), small: true }
|
||||
}
|
||||
}
|
||||
div { class: "mcp-detail-row",
|
||||
Icon { icon: BsHddNetwork, width: 13, height: 13 }
|
||||
|
||||
@@ -137,11 +137,18 @@ pub fn RepositoriesPage() -> Element {
|
||||
"For SSH URLs: add this deploy key (read-only) to your repository"
|
||||
}
|
||||
div {
|
||||
style: "margin-top: 4px; padding: 8px; background: var(--bg-secondary); border-radius: 4px; font-family: monospace; font-size: 11px; word-break: break-all; user-select: all;",
|
||||
if ssh_public_key().is_empty() {
|
||||
"Loading..."
|
||||
} else {
|
||||
"{ssh_public_key}"
|
||||
class: "copyable",
|
||||
style: "margin-top: 4px; padding: 8px; background: var(--bg-secondary); border-radius: 4px;",
|
||||
code {
|
||||
style: "font-size: 11px; word-break: break-all; user-select: all;",
|
||||
if ssh_public_key().is_empty() {
|
||||
"Loading..."
|
||||
|
sharang
commented
[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] 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 | *
<!-- compliance-fp:8e8d7aa2dde6cf0672a1a99ad7f35ed0ff16798088569a6517f4d1ab05885d45 -->
sharang
commented
[medium] Complex boolean expression in conditional rendering The code uses repeated calls to Suggested fix: Store the result of *Scanner: code-review/complexity | * **[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 | *
<!-- compliance-fp:700fccab879542d3afc49c9c3861d1bcfb217fd5f0789f4276a7f5e062ee56f5 -->
sharang
commented
[low] Potential duplicate computation of ssh_public_key() The Suggested fix: Store the result of *Scanner: code-review/convention | * **[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 | *
<!-- compliance-fp:0b9c380c78999b1706d166cde1e45752aa92ca1300b6cc90c7169d1b9f6513e3 -->
sharang
commented
[low] Potential duplicate computation of ssh_public_key() The Suggested fix: Store the result of *Scanner: code-review/convention | * **[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 | *
<!-- compliance-fp:0b9c380c78999b1706d166cde1e45752aa92ca1300b6cc90c7169d1b9f6513e3 -->
sharang
commented
[medium] Complex boolean expression in conditional rendering The code uses repeated calls to Suggested fix: Store the result of *Scanner: code-review/complexity | * **[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 | *
<!-- compliance-fp:700fccab879542d3afc49c9c3861d1bcfb217fd5f0789f4276a7f5e062ee56f5 -->
sharang
commented
[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] 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 | *
<!-- compliance-fp:8e8d7aa2dde6cf0672a1a99ad7f35ed0ff16798088569a6517f4d1ab05885d45 -->
|
||||
} else {
|
||||
"{ssh_public_key}"
|
||||
}
|
||||
}
|
||||
if !ssh_public_key().is_empty() {
|
||||
crate::components::copy_button::CopyButton { value: ssh_public_key(), small: true }
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -390,28 +397,37 @@ pub fn RepositoriesPage() -> Element {
|
||||
}
|
||||
div { class: "form-group",
|
||||
label { "Webhook URL" }
|
||||
input {
|
||||
r#type: "text",
|
||||
readonly: true,
|
||||
style: "font-family: monospace; font-size: 12px;",
|
||||
value: {
|
||||
#[cfg(feature = "web")]
|
||||
let origin = web_sys::window()
|
||||
.and_then(|w: web_sys::Window| w.location().origin().ok())
|
||||
.unwrap_or_default();
|
||||
#[cfg(not(feature = "web"))]
|
||||
let origin = String::new();
|
||||
format!("{origin}/webhook/{}/{eid}", edit_webhook_tracker())
|
||||
},
|
||||
{
|
||||
#[cfg(feature = "web")]
|
||||
let origin = web_sys::window()
|
||||
.and_then(|w: web_sys::Window| w.location().origin().ok())
|
||||
.unwrap_or_default();
|
||||
#[cfg(not(feature = "web"))]
|
||||
|
sharang
commented
[medium] Inconsistent error handling with unwrap_or_default() in web feature The code uses 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] Inconsistent error handling with unwrap_or_default() in web feature**
The code uses `unwrap_or_default()` on `w.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 | *
<!-- compliance-fp:f8286e8285fd0f26e4373284aee8d24d930f1dadb527be31347172ea45c0d418 -->
sharang
commented
[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] 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*
<!-- compliance-fp:115226cd8daa91f657d135e9134c31ab662335f8df71f562499ffa941063f6b0 -->
sharang
commented
[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] 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*
<!-- compliance-fp:115226cd8daa91f657d135e9134c31ab662335f8df71f562499ffa941063f6b0 -->
sharang
commented
[medium] Inconsistent error handling with unwrap_or_default() in web feature The code uses 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] Inconsistent error handling with unwrap_or_default() in web feature**
The code uses `unwrap_or_default()` on `w.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 | *
<!-- compliance-fp:f8286e8285fd0f26e4373284aee8d24d930f1dadb527be31347172ea45c0d418 -->
|
||||
let origin = String::new();
|
||||
let webhook_url = format!("{origin}/webhook/{}/{eid}", edit_webhook_tracker());
|
||||
rsx! {
|
||||
div { class: "copyable",
|
||||
input {
|
||||
r#type: "text",
|
||||
readonly: true,
|
||||
style: "font-family: monospace; font-size: 12px; flex: 1;",
|
||||
value: "{webhook_url}",
|
||||
}
|
||||
crate::components::copy_button::CopyButton { value: webhook_url.clone() }
|
||||
}
|
||||
|
sharang
commented
[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 | * **[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 | *
<!-- compliance-fp:f0b50d83ec153658f4955ca523c0c4be075bec84a0c7080f940868a9cea49a85 -->
sharang
commented
[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 | * **[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 | *
<!-- compliance-fp:f0b50d83ec153658f4955ca523c0c4be075bec84a0c7080f940868a9cea49a85 -->
|
||||
}
|
||||
}
|
||||
}
|
||||
div { class: "form-group",
|
||||
label { "Webhook Secret" }
|
||||
input {
|
||||
r#type: "text",
|
||||
readonly: true,
|
||||
style: "font-family: monospace; font-size: 12px;",
|
||||
value: "{secret}",
|
||||
div { class: "copyable",
|
||||
input {
|
||||
r#type: "text",
|
||||
readonly: true,
|
||||
style: "font-family: monospace; font-size: 12px; flex: 1;",
|
||||
value: "{secret}",
|
||||
|
sharang
commented
[low] Redundant clone() in CopyButton component The Suggested fix: Pass the secret directly without cloning, or ensure the CopyButton component accepts a reference instead of owned values. *Scanner: code-review/convention | * **[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-fp:93f575d103be7ace49c35795a7d43b5619798aa1f37e45cfa2f5c08bca0b7d34 -->
sharang
commented
[low] Redundant clone() in CopyButton component The Suggested fix: Pass the secret directly without cloning, or ensure the CopyButton component accepts a reference instead of owned values. *Scanner: code-review/convention | * **[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-fp:93f575d103be7ace49c35795a7d43b5619798aa1f37e45cfa2f5c08bca0b7d34 -->
|
||||
}
|
||||
crate::components::copy_button::CopyButton { value: secret.clone() }
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
[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 | *