feat: hourly CVE alerting with notification bell and API #53

[low] Inconsistent version pinning in Dockerfile

The Dockerfile changes from using an unspecified version of dioxus-cli to explicitly pinning version 0.7.3 with --locked flag. While this provides reproducibility, it's inconsistent with previous behavior and may introduce compatibility issues if not carefully managed.

Suggested fix: Ensure that version pinning is intentional and documented, or consider using a more flexible approach like specifying a version range instead of exact version.

*Scanner: code-review/convention | *

**[low] Inconsistent version pinning in Dockerfile** The Dockerfile changes from using an unspecified version of dioxus-cli to explicitly pinning version 0.7.3 with --locked flag. While this provides reproducibility, it's inconsistent with previous behavior and may introduce compatibility issues if not carefully managed. Suggested fix: Ensure that version pinning is intentional and documented, or consider using a more flexible approach like specifying a version range instead of exact version. *Scanner: code-review/convention | *

						
				@@ -0,0 +15,4 @@

				    axum::extract::Query(params): axum::extract::Query<NotificationFilter>,

				) -> Result<Json<ApiResponse<Vec<CveNotification>>>, StatusCode> {

				    let mut filter = doc! {};

sharang commented

[medium] Complex boolean expression in notification filtering

The status filtering logic in list_notifications uses a complex match expression that's hard to follow and could lead to incorrect filtering behavior if the status parameter contains unexpected values. The logic mixes default cases with explicit matches in a way that's not immediately clear.

Suggested fix: Simplify the status filtering by using a more direct approach like if let Some(status) = params.status followed by explicit handling of 'all' case, or extract the filtering logic into a separate helper function.

*Scanner: code-review/complexity | *

**[medium] Complex boolean expression in notification filtering** The status filtering logic in `list_notifications` uses a complex match expression that's hard to follow and could lead to incorrect filtering behavior if the status parameter contains unexpected values. The logic mixes default cases with explicit matches in a way that's not immediately clear. Suggested fix: Simplify the status filtering by using a more direct approach like `if let Some(status) = params.status` followed by explicit handling of 'all' case, or extract the filtering logic into a separate helper function. *Scanner: code-review/complexity | *

						
				@@ -0,0 +30,4 @@

				    // Filter by severity

				    if let Some(ref sev) = params.severity {

				        filter.insert("severity", sev.as_str());

				    }

sharang commented

[medium] Potential Denial of Service via Unbounded Query Parameters

The list_notifications handler accepts page and limit parameters but does not enforce strict limits on the maximum allowed values. While there's a minimum limit of 1 and maximum limit of 200, attackers could potentially send very large page numbers which would cause MongoDB to skip a large number of documents, leading to performance degradation or timeouts.

Suggested fix: Add stricter bounds checking on both page and limit parameters. For example, cap page to a reasonable maximum value (e.g., 1000) to prevent excessive skipping in database queries.

Scanner: code-review/security | CWE: CWE-400

**[medium] Potential Denial of Service via Unbounded Query Parameters** The `list_notifications` handler accepts `page` and `limit` parameters but does not enforce strict limits on the maximum allowed values. While there's a minimum limit of 1 and maximum limit of 200, attackers could potentially send very large page numbers which would cause MongoDB to skip a large number of documents, leading to performance degradation or timeouts. Suggested fix: Add stricter bounds checking on both `page` and `limit` parameters. For example, cap `page` to a reasonable maximum value (e.g., 1000) to prevent excessive skipping in database queries. *Scanner: code-review/security | CWE: CWE-400*

						
				@@ -0,0 +40,4 @@

				    let page = params.page.unwrap_or(1).max(1);

				    let limit = params.limit.unwrap_or(50).min(200);

				    let skip = (page - 1) * limit as u64;

sharang commented

[medium] Inconsistent handling of page and limit parameters in notifications listing

The list_notifications function uses page and limit parameters to calculate skip, but it doesn't validate that limit is within acceptable bounds before using it in the MongoDB query. If limit is set to a very large value, it could cause performance issues or resource exhaustion.

Suggested fix: Add validation to ensure limit is within reasonable bounds (e.g., between 1 and 1000) before applying it to the MongoDB query.

*Scanner: code-review/logic | *

**[medium] Inconsistent handling of page and limit parameters in notifications listing** The `list_notifications` function uses `page` and `limit` parameters to calculate `skip`, but it doesn't validate that `limit` is within acceptable bounds before using it in the MongoDB query. If `limit` is set to a very large value, it could cause performance issues or resource exhaustion. Suggested fix: Add validation to ensure `limit` is within reasonable bounds (e.g., between 1 and 1000) before applying it to the MongoDB query. *Scanner: code-review/logic | *

						
				@@ -0,0 +42,4 @@

				    let skip = (page - 1) * limit as u64;

				    let total = agent

				        .db

sharang commented

[medium] Inconsistent error handling with unwrap() in production code

The code uses unwrap_or(0) on database count operations which can mask failures and lead to incorrect behavior. This pattern should be replaced with proper error propagation to ensure failures are handled appropriately rather than silently defaulting to 0.

Suggested fix: Replace unwrap_or(0) with proper error handling using ? operator or explicit error mapping to maintain consistency with the rest of the error handling pattern in the function.

*Scanner: code-review/convention | *

**[medium] Inconsistent error handling with unwrap() in production code** The code uses `unwrap_or(0)` on database count operations which can mask failures and lead to incorrect behavior. This pattern should be replaced with proper error propagation to ensure failures are handled appropriately rather than silently defaulting to 0. Suggested fix: Replace `unwrap_or(0)` with proper error handling using `?` operator or explicit error mapping to maintain consistency with the rest of the error handling pattern in the function. *Scanner: code-review/convention | *

						
				@@ -0,0 +49,4 @@

				        .unwrap_or(0);

				    let notifications: Vec<CveNotification> = match agent

				        .db

sharang commented

[high] Missing error handling for database operations in notifications listing

The list_notifications function uses unwrap_or(0) when counting documents, which can mask potential database errors. If the database connection fails or there's a network issue, this will silently return 0 instead of propagating the error.

Suggested fix: Replace unwrap_or(0) with proper error propagation to ensure database errors are handled appropriately.

*Scanner: code-review/logic | *

**[high] Missing error handling for database operations in notifications listing** The `list_notifications` function uses `unwrap_or(0)` when counting documents, which can mask potential database errors. If the database connection fails or there's a network issue, this will silently return 0 instead of propagating the error. Suggested fix: Replace `unwrap_or(0)` with proper error propagation to ensure database errors are handled appropriately. *Scanner: code-review/logic | *

						
				@@ -0,0 +51,4 @@

				    let notifications: Vec<CveNotification> = match agent

				        .db

				        .cve_notifications()

				        .find(filter)

sharang commented

[high] Insecure Direct Object Reference (IDOR) in Notification Management

The notification management endpoints allow users to manipulate notifications by ID without proper authorization checks. Specifically, the mark_read, dismiss_notification, and notification_count handlers do not validate whether the requesting user has permission to access or modify the specified notification. This could allow an attacker to read, mark as read, or dismiss notifications belonging to other users if they can guess or obtain valid notification IDs.

Suggested fix: Implement proper authorization checks before allowing modification or access to notifications. Ensure that each notification's owner or authorized user can only interact with their own notifications. Add a check to verify that the authenticated user has permission to access the notification before performing any operations.

Scanner: code-review/security | CWE: CWE-285

**[high] Insecure Direct Object Reference (IDOR) in Notification Management** The notification management endpoints allow users to manipulate notifications by ID without proper authorization checks. Specifically, the `mark_read`, `dismiss_notification`, and `notification_count` handlers do not validate whether the requesting user has permission to access or modify the specified notification. This could allow an attacker to read, mark as read, or dismiss notifications belonging to other users if they can guess or obtain valid notification IDs. Suggested fix: Implement proper authorization checks before allowing modification or access to notifications. Ensure that each notification's owner or authorized user can only interact with their own notifications. Add a check to verify that the authenticated user has permission to access the notification before performing any operations. *Scanner: code-review/security | CWE: CWE-285*

						
				@@ -0,0 +63,4 @@

				            let mut cursor = cursor;

				            while let Some(Ok(n)) = cursor.next().await {

				                items.push(n);

				            }

sharang commented

[medium] Inconsistent error handling with unwrap() in production code

The code uses unwrap_or(0) on database count operations which can mask failures and lead to incorrect behavior. This pattern should be replaced with proper error propagation to ensure failures are handled appropriately rather than silently defaulting to 0.

Suggested fix: Replace unwrap_or(0) with proper error handling using ? operator or explicit error mapping to maintain consistency with the rest of the error handling pattern in the function.

*Scanner: code-review/convention | *

**[medium] Inconsistent error handling with unwrap() in production code** The code uses `unwrap_or(0)` on database count operations which can mask failures and lead to incorrect behavior. This pattern should be replaced with proper error propagation to ensure failures are handled appropriately rather than silently defaulting to 0. Suggested fix: Replace `unwrap_or(0)` with proper error handling using `?` operator or explicit error mapping to maintain consistency with the rest of the error handling pattern in the function. *Scanner: code-review/convention | *

sharang commented

[medium] Incorrect MongoDB query for notification count

The notification_count function filters for notifications with status 'new', but the database may contain notifications with other statuses that should also be considered unread. This could lead to an undercount of unread notifications.

Suggested fix: Update the filter to include all statuses that represent unread notifications, such as 'new' and potentially others like 'unseen'.

*Scanner: code-review/logic | *

**[medium] Incorrect MongoDB query for notification count** The `notification_count` function filters for notifications with status 'new', but the database may contain notifications with other statuses that should also be considered unread. This could lead to an undercount of unread notifications. Suggested fix: Update the filter to include all statuses that represent unread notifications, such as 'new' and potentially others like 'unseen'. *Scanner: code-review/logic | *

compliance-agent/src/config.rs

						
				@@ -43,3 +43,3 @@

				        scan_schedule: env_var_opt("SCAN_SCHEDULE").unwrap_or_else(|| "0 0 */6 * * *".to_string()),

				        cve_monitor_schedule: env_var_opt("CVE_MONITOR_SCHEDULE")

				            .unwrap_or_else(|| "0 0 0 * * *".to_string()),

				            .unwrap_or_else(|| "0 0 * * * *".to_string()),

sharang commented

[medium] Complex boolean expression in schedule configuration

The CVE monitor schedule default value was changed from '0 0 0 * * *' to '0 0 * * * *'. This change affects the cron schedule parsing logic which could introduce bugs if the parsing implementation doesn't properly handle the new format or if there are assumptions about the number of fields in cron expressions.

Suggested fix: Add explicit validation for the cron schedule format to ensure the new default value is correctly parsed and handled by the underlying cron library. Consider adding unit tests for different cron formats to verify parsing behavior.

*Scanner: code-review/complexity | *

**[medium] Complex boolean expression in schedule configuration** The CVE monitor schedule default value was changed from '0 0 0 * * *' to '0 0 * * * *'. This change affects the cron schedule parsing logic which could introduce bugs if the parsing implementation doesn't properly handle the new format or if there are assumptions about the number of fields in cron expressions. Suggested fix: Add explicit validation for the cron schedule format to ensure the new default value is correctly parsed and handled by the underlying cron library. Consider adding unit tests for different cron formats to verify parsing behavior. *Scanner: code-review/complexity | *

sharang commented

[high] Insecure Default Cron Schedule

The default cron schedule for CVE monitoring was changed from '0 0 0 * * *' (daily at midnight) to '0 0 * * * *' (every minute). This introduces a significant performance issue and potential resource exhaustion as the system will now run the CVE monitor every minute instead of daily, leading to excessive database queries and processing overhead.

Suggested fix: Revert to the original daily schedule '0 0 0 * * *' or implement proper rate limiting to prevent excessive execution frequency.

Scanner: code-review/security | CWE: CWE-707

**[high] Insecure Default Cron Schedule** The default cron schedule for CVE monitoring was changed from '0 0 0 * * *' (daily at midnight) to '0 0 * * * *' (every minute). This introduces a significant performance issue and potential resource exhaustion as the system will now run the CVE monitor every minute instead of daily, leading to excessive database queries and processing overhead. Suggested fix: Revert to the original daily schedule '0 0 0 * * *' or implement proper rate limiting to prevent excessive execution frequency. *Scanner: code-review/security | CWE: CWE-707*

sharang commented

[high] Incorrect cron schedule syntax

The CVE monitor schedule was changed from "0 0 0 * * *" to "0 0 * * * *". This is incorrect cron syntax - it should be "0 0 0 * * *" (which means 'at 00:00 every day') rather than "0 0 * * * *" (which would mean 'at 00:00 every minute'). The new value will cause the scheduler to run every minute instead of daily.

Suggested fix: Change back to "0 0 0 * * *" to maintain daily execution at midnight

*Scanner: code-review/logic | *

**[high] Incorrect cron schedule syntax** The CVE monitor schedule was changed from "0 0 0 * * *" to "0 0 * * * *". This is incorrect cron syntax - it should be "0 0 0 * * *" (which means 'at 00:00 every day') rather than "0 0 * * * *" (which would mean 'at 00:00 every minute'). The new value will cause the scheduler to run every minute instead of daily. Suggested fix: Change back to "0 0 0 * * *" to maintain daily execution at midnight *Scanner: code-review/logic | *

sharang commented

[medium] Inconsistent error handling with unwrap_or_else

The code uses unwrap_or_else consistently for environment variable parsing, but this pattern can mask configuration loading failures. If an environment variable is present but invalid, it will fall back to the default value instead of failing fast, potentially leading to unexpected behavior.

Suggested fix: Consider using unwrap_or instead of unwrap_or_else to make the fallback behavior more explicit and avoid potential silent failures when environment variables contain invalid values.

*Scanner: code-review/convention | *

**[medium] Inconsistent error handling with unwrap_or_else** The code uses `unwrap_or_else` consistently for environment variable parsing, but this pattern can mask configuration loading failures. If an environment variable is present but invalid, it will fall back to the default value instead of failing fast, potentially leading to unexpected behavior. Suggested fix: Consider using `unwrap_or` instead of `unwrap_or_else` to make the fallback behavior more explicit and avoid potential silent failures when environment variables contain invalid values. *Scanner: code-review/convention | *

compliance-agent/src/database.rs

						
				@@ -78,6 +78,25 @@ impl Database {

				            )

				            .await?;

				        // cve_notifications: unique cve_id + repo_id + package, status filter

sharang commented

[medium] Missing error propagation in database index creation

The database index creation operations use .await? but don't propagate errors properly. If index creation fails, the application might continue running with missing indexes, which could lead to data consistency issues or performance problems. The error handling should ensure that index creation failures are properly reported.

Suggested fix: Add explicit error handling around the index creation calls to ensure that failures during index creation are properly propagated up the call stack rather than being silently ignored.

*Scanner: code-review/convention | *

**[medium] Missing error propagation in database index creation** The database index creation operations use `.await?` but don't propagate errors properly. If index creation fails, the application might continue running with missing indexes, which could lead to data consistency issues or performance problems. The error handling should ensure that index creation failures are properly reported. Suggested fix: Add explicit error handling around the index creation calls to ensure that failures during index creation are properly propagated up the call stack rather than being silently ignored. *Scanner: code-review/convention | *

compliance-agent/src/database.rs

						
				@@ -222,6 +241,12 @@ impl Database {

				        self.inner.collection("cve_alerts")

				    }

				    pub fn cve_notifications(

sharang commented

[low] Inconsistent public API design

The cve_notifications method is added as a public getter but doesn't follow the same pattern as other collection getters like cve_alerts and tracker_issues. This creates an inconsistent API surface where some collections have dedicated methods while others don't, potentially confusing users of the Database struct.

Suggested fix: Either add similar getter methods for all collections or remove the inconsistency by making all collection access consistent through a single pattern.

*Scanner: code-review/convention | *

**[low] Inconsistent public API design** The `cve_notifications` method is added as a public getter but doesn't follow the same pattern as other collection getters like `cve_alerts` and `tracker_issues`. This creates an inconsistent API surface where some collections have dedicated methods while others don't, potentially confusing users of the Database struct. Suggested fix: Either add similar getter methods for all collections or remove the inconsistency by making all collection access consistent through a single pattern. *Scanner: code-review/convention | *

						
				@@ -84,2 +84,4 @@

				async fn monitor_cves(agent: &ComplianceAgent) {

				    use compliance_core::models::notification::{parse_severity, CveNotification};

				    use compliance_core::models::SbomEntry;

				    use futures_util::StreamExt;

sharang commented

[medium] Deeply nested control flow in CVE monitoring

The monitor_cves function contains deeply nested control flow with multiple levels of error handling and conditional logic. The main loop processing entries by repo contains nested conditionals for scanning, upserting alerts, updating entries, and creating notifications, making it difficult to follow the logical flow and increasing risk of bugs during maintenance.

Suggested fix: Refactor the main loop into smaller functions with single responsibilities. Extract the scanning logic, alert upserting, entry updates, and notification creation into separate functions to flatten the nesting and improve readability.

*Scanner: code-review/complexity | *

**[medium] Deeply nested control flow in CVE monitoring** The `monitor_cves` function contains deeply nested control flow with multiple levels of error handling and conditional logic. The main loop processing entries by repo contains nested conditionals for scanning, upserting alerts, updating entries, and creating notifications, making it difficult to follow the logical flow and increasing risk of bugs during maintenance. Suggested fix: Refactor the main loop into smaller functions with single responsibilities. Extract the scanning logic, alert upserting, entry updates, and notification creation into separate functions to flatten the nesting and improve readability. *Scanner: code-review/complexity | *

						
				@@ -103,2 +103,2 @@

				    // The actual CVE checking is handled by the CveScanner in the pipeline

				    // This is a simplified version that just logs the activity

				    tracing::info!(

				        "CVE monitor: checking {} dependencies for new CVEs",

sharang commented

[medium] Potential panic from unwrap_or_else() in notification creation

The code uses repo_names.get(&repo_id).cloned().unwrap_or_else(|| repo_id.clone()) which can panic if repo_id contains invalid UTF-8. While this is unlikely in practice, it's an anti-pattern that could cause runtime panics if the underlying data is malformed. The code should handle this case more gracefully.

Suggested fix: Use a more robust fallback mechanism that doesn't rely on unwrap_or_else() for string operations, or validate that repo_id is valid UTF-8 before using it as a key.

*Scanner: code-review/convention | *

**[medium] Potential panic from unwrap_or_else() in notification creation** The code uses `repo_names.get(&repo_id).cloned().unwrap_or_else(|| repo_id.clone())` which can panic if repo_id contains invalid UTF-8. While this is unlikely in practice, it's an anti-pattern that could cause runtime panics if the underlying data is malformed. The code should handle this case more gracefully. Suggested fix: Use a more robust fallback mechanism that doesn't rely on unwrap_or_else() for string operations, or validate that repo_id is valid UTF-8 before using it as a key. *Scanner: code-review/convention | *

sharang commented

[medium] Sensitive Data Exposure in Logs

The code logs sensitive information such as repo.name and alert.cve_id in debug and info level logs. Although these logs are typically not exposed externally, if they are stored or transmitted without proper access controls, they could expose repository names and CVE identifiers to unauthorized parties.

Suggested fix: Avoid logging sensitive data like repository names and CVE IDs unless strictly necessary for debugging. Consider using structured logging with configurable log levels to prevent accidental exposure.

Scanner: code-review/security | CWE: CWE-532

**[medium] Sensitive Data Exposure in Logs** The code logs sensitive information such as `repo.name` and `alert.cve_id` in debug and info level logs. Although these logs are typically not exposed externally, if they are stored or transmitted without proper access controls, they could expose repository names and CVE identifiers to unauthorized parties. Suggested fix: Avoid logging sensitive data like repository names and CVE IDs unless strictly necessary for debugging. Consider using structured logging with configurable log levels to prevent accidental exposure. *Scanner: code-review/security | CWE: CWE-532*

sharang commented

[medium] Potential Command Injection via User-Controlled Repository Name

The code constructs a repository name from user-provided data (repo.name) and uses it in logging and notification creation. While this doesn't directly execute commands, if any downstream system processes this name as part of a shell command or file path, it could lead to command injection. Specifically, the repo_name variable is used in tracing::info! log messages and passed to CveNotification::new() which may be used in contexts where it's processed further.

Suggested fix: Sanitize or validate the repo.name field before using it in logs or notifications. Ensure that any downstream processing treats this value as literal text rather than executable code.

Scanner: code-review/security | CWE: CWE-78

**[medium] Potential Command Injection via User-Controlled Repository Name** The code constructs a repository name from user-provided data (`repo.name`) and uses it in logging and notification creation. While this doesn't directly execute commands, if any downstream system processes this name as part of a shell command or file path, it could lead to command injection. Specifically, the `repo_name` variable is used in `tracing::info!` log messages and passed to `CveNotification::new()` which may be used in contexts where it's processed further. Suggested fix: Sanitize or validate the `repo.name` field before using it in logs or notifications. Ensure that any downstream processing treats this value as literal text rather than executable code. *Scanner: code-review/security | CWE: CWE-78*

						
				@@ -105,0 +122,4 @@

				    let nvd_key = agent.config.nvd_api_key.as_ref().map(|k| {

				        use secrecy::ExposeSecret;

				        k.expose_secret().to_string()

				    });

sharang commented

[medium] Inconsistent error handling with unwrap() in critical path

The code uses mongodb::bson::to_bson(...).unwrap_or_default() multiple times in the CVE monitoring flow. While this prevents panics, it silently discards serialization errors which could lead to missing data in database operations. This inconsistent error handling approach (mixing unwrap_or_default with proper error logging) could hide failures that should be properly reported.

Suggested fix: Replace unwrap_or_default() with proper error handling that logs the serialization failure and potentially aborts the operation or retries, rather than silently ignoring BSON conversion errors.

*Scanner: code-review/convention | *

**[medium] Inconsistent error handling with unwrap() in critical path** The code uses `mongodb::bson::to_bson(...).unwrap_or_default()` multiple times in the CVE monitoring flow. While this prevents panics, it silently discards serialization errors which could lead to missing data in database operations. This inconsistent error handling approach (mixing unwrap_or_default with proper error logging) could hide failures that should be properly reported. Suggested fix: Replace unwrap_or_default() with proper error handling that logs the serialization failure and potentially aborts the operation or retries, rather than silently ignoring BSON conversion errors. *Scanner: code-review/convention | *

						
				@@ -105,0 +129,4 @@

				        nvd_key,

				    );

				    // Group entries by repo for scanning

sharang commented

[medium] Incorrect handling of empty known_vulnerabilities in SBOM entry updates

In the CVE monitoring logic, when updating SBOM entries with known vulnerabilities, the code checks if entry.known_vulnerabilities.is_empty() and skips the update if true. However, this check is flawed because it should only skip updating if there are NO vulnerabilities to report, but the current logic prevents any update even when vulnerabilities exist but are empty. This could lead to stale data in the database.

Suggested fix: The condition should be inverted to only skip when there are NO vulnerabilities to process. Change if entry.known_vulnerabilities.is_empty() to if !entry.known_vulnerabilities.is_empty() or better yet, remove the check entirely since we want to update regardless of whether vulnerabilities exist.

*Scanner: code-review/logic | *

**[medium] Incorrect handling of empty known_vulnerabilities in SBOM entry updates** In the CVE monitoring logic, when updating SBOM entries with known vulnerabilities, the code checks `if entry.known_vulnerabilities.is_empty()` and skips the update if true. However, this check is flawed because it should only skip updating if there are NO vulnerabilities to report, but the current logic prevents any update even when vulnerabilities exist but are empty. This could lead to stale data in the database. Suggested fix: The condition should be inverted to only skip when there are NO vulnerabilities to process. Change `if entry.known_vulnerabilities.is_empty()` to `if !entry.known_vulnerabilities.is_empty()` or better yet, remove the check entirely since we want to update regardless of whether vulnerabilities exist. *Scanner: code-review/logic | *

						
				@@ -105,0 +131,4 @@

				    // Group entries by repo for scanning

				    let mut entries_by_repo: std::collections::HashMap<String, Vec<SbomEntry>> =

				        std::collections::HashMap::new();

sharang commented

[high] Insecure Direct Object Reference (IDOR) in CVE Notification Creation

When creating CVE notifications, the code uses a MongoDB filter based on cve_id, repo_id, package_name, and package_version. However, there's no explicit authorization check to ensure that the authenticated user has access to the specified repo_id. If an attacker can forge a request to create a notification for a repository they don't own, they could potentially pollute the notification system with false positives or malicious data.

Suggested fix: Add an authorization check before creating CVE notifications to verify that the requesting entity has permission to access the given repo_id. This should involve validating the user's role or ownership of the repository.

Scanner: code-review/security | CWE: CWE-285

**[high] Insecure Direct Object Reference (IDOR) in CVE Notification Creation** When creating CVE notifications, the code uses a MongoDB filter based on `cve_id`, `repo_id`, `package_name`, and `package_version`. However, there's no explicit authorization check to ensure that the authenticated user has access to the specified `repo_id`. If an attacker can forge a request to create a notification for a repository they don't own, they could potentially pollute the notification system with false positives or malicious data. Suggested fix: Add an authorization check before creating CVE notifications to verify that the requesting entity has permission to access the given `repo_id`. This should involve validating the user's role or ownership of the repository. *Scanner: code-review/security | CWE: CWE-285*

compliance-core/src/models/mod.rs

						
				@@ -27,6 +28,7 @@ pub use graph::{

				};

				pub use issue::{IssueStatus, TrackerIssue, TrackerType};

				pub use mcp::{McpServerConfig, McpServerStatus, McpTransport};

				pub use notification::{CveNotification, NotificationSeverity, NotificationStatus};

sharang commented

[low] Public API exposure of internal notification module

The notification module was made public in the models re-export, but it appears to be an internal implementation detail used only within the compliance-agent. Making it public exposes an internal API contract that may change without notice, breaking consumers who depend on it.

Suggested fix: Consider making the notification module private or moving it to a separate internal crate if it's truly meant to be internal, or document clearly that it's part of the public API contract.

*Scanner: code-review/convention | *

**[low] Public API exposure of internal notification module** The notification module was made public in the models re-export, but it appears to be an internal implementation detail used only within the compliance-agent. Making it public exposes an internal API contract that may change without notice, breaking consumers who depend on it. Suggested fix: Consider making the notification module private or moving it to a separate internal crate if it's truly meant to be internal, or document clearly that it's part of the public API contract. *Scanner: code-review/convention | *

						
				@@ -0,0 +65,4 @@

				        severity: NotificationSeverity,

				    ) -> Self {

				        Self {

				            id: None,

sharang commented

[medium] Potential panic in notification creation due to missing error propagation

The CveNotification::new constructor creates a new notification with default values but doesn't validate any of its parameters. If any of the required fields (like cve_id, repo_id, etc.) are empty or invalid, this could lead to inconsistent state in the notification system without proper error reporting.

Suggested fix: Consider adding validation checks or returning a Result type from the constructor to propagate potential errors during notification creation.

*Scanner: code-review/convention | *

**[medium] Potential panic in notification creation due to missing error propagation** The `CveNotification::new` constructor creates a new notification with default values but doesn't validate any of its parameters. If any of the required fields (like `cve_id`, `repo_id`, etc.) are empty or invalid, this could lead to inconsistent state in the notification system without proper error reporting. Suggested fix: Consider adding validation checks or returning a Result type from the constructor to propagate potential errors during notification creation. *Scanner: code-review/convention | *

						
				@@ -0,0 +81,4 @@

				        }

				    }

				}

sharang commented

[medium] Complex boolean expression in severity parsing

The parse_severity function contains a complex boolean expression that mixes CVSS score checking with string severity mapping. This increases cognitive load when trying to understand the logic flow and could lead to incorrect severity assignment if the conditions are reordered or modified.

Suggested fix: Break down the conditional logic into separate functions or use early returns to simplify the boolean expression and make the severity mapping more readable.

*Scanner: code-review/complexity | *

**[medium] Complex boolean expression in severity parsing** The `parse_severity` function contains a complex boolean expression that mixes CVSS score checking with string severity mapping. This increases cognitive load when trying to understand the logic flow and could lead to incorrect severity assignment if the conditions are reordered or modified. Suggested fix: Break down the conditional logic into separate functions or use early returns to simplify the boolean expression and make the severity mapping more readable. *Scanner: code-review/complexity | *

						
				@@ -0,0 +84,4 @@

				/// Map an OSV/NVD severity string to our notification severity

				pub fn parse_severity(s: Option<&str>, cvss: Option<f64>) -> NotificationSeverity {

				    // Prefer CVSS score if available

sharang commented

[high] Off-by-one error in CVSS severity thresholds

The parse_severity function uses inclusive thresholds for CVSS scores that should be exclusive. Specifically, a score of exactly 9.0 is classified as Critical, but according to CVSS standards, 9.0 should be considered High (scores 7.0-8.9). This causes incorrect categorization of vulnerabilities with CVSS scores at boundary values.

Suggested fix: Change the comparison operators to use strict inequality for boundary values. For example, change s if s >= 9.0 => NotificationSeverity::Critical to s if s > 9.0 => NotificationSeverity::Critical and adjust other thresholds accordingly.

*Scanner: code-review/logic | *

**[high] Off-by-one error in CVSS severity thresholds** The `parse_severity` function uses inclusive thresholds for CVSS scores that should be exclusive. Specifically, a score of exactly 9.0 is classified as Critical, but according to CVSS standards, 9.0 should be considered High (scores 7.0-8.9). This causes incorrect categorization of vulnerabilities with CVSS scores at boundary values. Suggested fix: Change the comparison operators to use strict inequality for boundary values. For example, change `s if s >= 9.0 => NotificationSeverity::Critical` to `s if s > 9.0 => NotificationSeverity::Critical` and adjust other thresholds accordingly. *Scanner: code-review/logic | *

						
				@@ -0,0 +85,4 @@

				/// Map an OSV/NVD severity string to our notification severity

				pub fn parse_severity(s: Option<&str>, cvss: Option<f64>) -> NotificationSeverity {

				    // Prefer CVSS score if available

				    if let Some(score) = cvss {

sharang commented

[medium] Inconsistent error handling in notification severity parsing

The parse_severity function uses pattern matching on Option<&str> but doesn't handle the case where the string might be invalid or unexpected. While it handles None cases properly, it assumes all non-None strings are valid severity indicators, which could lead to incorrect severity assignment if malformed input is encountered.

Suggested fix: Add explicit error handling or validation for unexpected string values in the severity parsing logic to ensure robustness against malformed inputs.

*Scanner: code-review/convention | *

**[medium] Inconsistent error handling in notification severity parsing** The `parse_severity` function uses pattern matching on `Option<&str>` but doesn't handle the case where the string might be invalid or unexpected. While it handles None cases properly, it assumes all non-None strings are valid severity indicators, which could lead to incorrect severity assignment if malformed input is encountered. Suggested fix: Add explicit error handling or validation for unexpected string values in the severity parsing logic to ensure robustness against malformed inputs. *Scanner: code-review/convention | *

compliance-dashboard/assets/main.css

						
				@@ -3850,0 +3876,4 @@

				.notification-dismiss-btn:hover { color: var(--danger); }

				.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; }

sharang commented

[high] Potential XSS via CVE ID in Notification Panel

The CVE ID field in the notification panel is rendered directly into HTML without sanitization. If an attacker can control the CVE ID value (e.g., through a malicious OSV/NVD feed or compromised data source), they could inject malicious script tags into the notification item's HTML structure, leading to XSS when the notification panel is displayed.

Suggested fix: Sanitize all user-provided data before rendering it in HTML templates. Specifically, ensure that the CVE ID field is escaped when inserted into the DOM. Use a templating engine with automatic escaping or manually escape special characters like <, >, &, ", ' before inserting into HTML.

Scanner: code-review/security | CWE: CWE-79

**[high] Potential XSS via CVE ID in Notification Panel** The CVE ID field in the notification panel is rendered directly into HTML without sanitization. If an attacker can control the CVE ID value (e.g., through a malicious OSV/NVD feed or compromised data source), they could inject malicious script tags into the notification item's HTML structure, leading to XSS when the notification panel is displayed. Suggested fix: Sanitize all user-provided data before rendering it in HTML templates. Specifically, ensure that the CVE ID field is escaped when inserted into the DOM. Use a templating engine with automatic escaping or manually escape special characters like <, >, &, ", ' before inserting into HTML. *Scanner: code-review/security | CWE: CWE-79*

compliance-dashboard/src/components/app_shell.rs

						
				@@ -21,6 +22,7 @@ pub fn AppShell() -> Element {

				                    main { class: "main-content",

				                        Outlet::<Route> {}

				                    }

				                    NotificationBell {}

sharang commented

[medium] Missing Input Validation in NotificationBell Component

The NotificationBell component appears to be added to the app shell without any validation of user-provided data that might be passed to it. If this component processes any user input directly (such as notification messages, titles, or metadata) without sanitization or validation, it could lead to XSS vulnerabilities or other injection attacks. Without seeing the full implementation of NotificationBell, we cannot determine the exact risk, but adding a new component that handles potentially untrusted data without proper validation creates an attack surface.

Suggested fix: Review the NotificationBell component implementation to ensure all user-provided data is properly sanitized and validated before rendering or processing. Implement input validation and output encoding to prevent XSS and other injection vulnerabilities.

Scanner: code-review/security | CWE: CWE-79

**[medium] Missing Input Validation in NotificationBell Component** The NotificationBell component appears to be added to the app shell without any validation of user-provided data that might be passed to it. If this component processes any user input directly (such as notification messages, titles, or metadata) without sanitization or validation, it could lead to XSS vulnerabilities or other injection attacks. Without seeing the full implementation of NotificationBell, we cannot determine the exact risk, but adding a new component that handles potentially untrusted data without proper validation creates an attack surface. Suggested fix: Review the NotificationBell component implementation to ensure all user-provided data is properly sanitized and validated before rendering or processing. Implement input validation and output encoding to prevent XSS and other injection vulnerabilities. *Scanner: code-review/security | CWE: CWE-79*

sharang commented

[high] Missing import of NotificationBell in app_shell.rs

The NotificationBell component is used in AppShell but not properly imported. While the module is declared in mod.rs, the specific component needs to be imported in the app_shell.rs file to be available for use.

Suggested fix: Add use crate::components::notification_bell::NotificationBell; at the top of the file after the other imports.

*Scanner: code-review/logic | *

**[high] Missing import of NotificationBell in app_shell.rs** The NotificationBell component is used in AppShell but not properly imported. While the module is declared in mod.rs, the specific component needs to be imported in the app_shell.rs file to be available for use. Suggested fix: Add `use crate::components::notification_bell::NotificationBell;` at the top of the file after the other imports. *Scanner: code-review/logic | *

sharang commented

[medium] Inconsistent error handling in auth check

The check_auth function in src/components/app_shell.rs uses expect() which will panic on failure, while other parts of the application likely use proper error propagation. This inconsistency can cause unexpected crashes instead of graceful error handling.

Suggested fix: Replace expect() with proper error handling using ? operator or explicit error propagation to maintain consistency with the rest of the application's error handling pattern.

*Scanner: code-review/convention | *

**[medium] Inconsistent error handling in auth check** The `check_auth` function in `src/components/app_shell.rs` uses `expect()` which will panic on failure, while other parts of the application likely use proper error propagation. This inconsistency can cause unexpected crashes instead of graceful error handling. Suggested fix: Replace `expect()` with proper error handling using `?` operator or explicit error propagation to maintain consistency with the rest of the application's error handling pattern. *Scanner: code-review/convention | *

sharang commented

[medium] Missing error handling in component initialization

The AppShell component calls check_auth() without handling potential errors, which could lead to unhandled panics when authentication fails. The component should handle authentication errors gracefully rather than crashing.

Suggested fix: Add proper error handling around the check_auth() call to catch and handle authentication failures appropriately, such as redirecting to login or showing an error message.

*Scanner: code-review/convention | *

**[medium] Missing error handling in component initialization** The `AppShell` component calls `check_auth()` without handling potential errors, which could lead to unhandled panics when authentication fails. The component should handle authentication errors gracefully rather than crashing. Suggested fix: Add proper error handling around the `check_auth()` call to catch and handle authentication failures appropriately, such as redirecting to login or showing an error message. *Scanner: code-review/convention | *

compliance-dashboard/src/components/notification_bell.rs

						
				@@ -0,0 +23,4 @@

				            #[cfg(feature = "web")]

				            {

				                gloo_timers::future::TimeoutFuture::new(30_000).await;

				            }

sharang commented

[medium] Inconsistent Error Handling in Resource Polling

The polling loop in use_resource handles errors from fetch_notification_count() by logging and continuing, while other similar operations in the same module use more explicit error reporting. This inconsistency could mask failures in notification fetching.

Suggested fix: Consider propagating errors from fetch_notification_count() to a global error state or logging them more explicitly to maintain consistency with the module's error handling patterns.

*Scanner: code-review/convention | *

**[medium] Inconsistent Error Handling in Resource Polling** The polling loop in `use_resource` handles errors from `fetch_notification_count()` by logging and continuing, while other similar operations in the same module use more explicit error reporting. This inconsistency could mask failures in notification fetching. Suggested fix: Consider propagating errors from `fetch_notification_count()` to a global error state or logging them more explicitly to maintain consistency with the module's error handling patterns. *Scanner: code-review/convention | *

compliance-dashboard/src/components/notification_bell.rs

						
				@@ -0,0 +31,4 @@

				        }

				    });

				    // Load notifications when panel opens

sharang commented

[high] Race condition in notification loading

In the load_notifications function, there's a potential race condition between setting is_loading.set(true) and the subsequent async block execution. If the component unmounts or is_open changes before the async block completes, the state updates might occur on an unmounted component, leading to undefined behavior or panics. The current implementation doesn't guard against such scenarios.

Suggested fix: Use Dioxus's built-in lifecycle management or cancellation tokens to ensure that async operations don't continue after the component has been unmounted or the state has changed unexpectedly.

*Scanner: code-review/logic | *

**[high] Race condition in notification loading** In the `load_notifications` function, there's a potential race condition between setting `is_loading.set(true)` and the subsequent async block execution. If the component unmounts or `is_open` changes before the async block completes, the state updates might occur on an unmounted component, leading to undefined behavior or panics. The current implementation doesn't guard against such scenarios. Suggested fix: Use Dioxus's built-in lifecycle management or cancellation tokens to ensure that async operations don't continue after the component has been unmounted or the state has changed unexpectedly. *Scanner: code-review/logic | *

compliance-dashboard/src/components/notification_bell.rs

						
				@@ -0,0 +45,4 @@

				            // Mark all as read when panel opens

				            let _ = mark_all_notifications_read().await;

				            count.set(0);

				            is_loading.set(false);

sharang commented

[high] Incorrect notification dismissal logic

The notification dismissal logic uses a nested and_then chain to extract the $oid field from the notification ID, but this approach is flawed. If the ID structure doesn't exactly match the expected format (e.g., missing $oid key or different nesting), it will return None and the comparison != Some(&id) will fail, causing notifications to not be properly removed from the UI. Additionally, the dismissal handler passes id.clone() to the async block, but then tries to compare against n.id.as_ref().and_then(...).and_then(...) which may not match due to the complex extraction logic.

Suggested fix: Simplify the notification dismissal logic by using a direct string comparison on the notification ID field, assuming it's already a valid string. Alternatively, ensure the ID extraction logic is robust and handles all possible ID formats correctly.

*Scanner: code-review/logic | *

**[high] Incorrect notification dismissal logic** The notification dismissal logic uses a nested `and_then` chain to extract the `$oid` field from the notification ID, but this approach is flawed. If the ID structure doesn't exactly match the expected format (e.g., missing `$oid` key or different nesting), it will return `None` and the comparison `!= Some(&id)` will fail, causing notifications to not be properly removed from the UI. Additionally, the dismissal handler passes `id.clone()` to the async block, but then tries to compare against `n.id.as_ref().and_then(...).and_then(...)` which may not match due to the complex extraction logic. Suggested fix: Simplify the notification dismissal logic by using a direct string comparison on the notification ID field, assuming it's already a valid string. Alternatively, ensure the ID extraction logic is robust and handles all possible ID formats correctly. *Scanner: code-review/logic | *

compliance-dashboard/src/components/notification_bell.rs

						
				@@ -0,0 +55,4 @@

				            notifications.write().retain(|n| {

				                n.id.as_ref()

				                    .and_then(|v| v.get("$oid"))

				                    .and_then(|v| v.as_str())

sharang commented

[medium] Complex boolean expression in notification dismissal

The notification dismissal logic uses a complex chain of and_then calls to extract the OID from the notification ID, which is hard to follow and prone to runtime panics if the structure doesn't match expectations. This could lead to silent failures or crashes when processing notifications.

Suggested fix: Extract the OID extraction into a separate helper function with proper error handling, or use a more robust deserialization approach instead of chaining multiple and_then calls.

*Scanner: code-review/complexity | *

**[medium] Complex boolean expression in notification dismissal** The notification dismissal logic uses a complex chain of `and_then` calls to extract the OID from the notification ID, which is hard to follow and prone to runtime panics if the structure doesn't match expectations. This could lead to silent failures or crashes when processing notifications. Suggested fix: Extract the OID extraction into a separate helper function with proper error handling, or use a more robust deserialization approach instead of chaining multiple `and_then` calls. *Scanner: code-review/complexity | *

compliance-dashboard/src/components/notification_bell.rs

						
				@@ -0,0 +82,4 @@

				                        button {

				                            class: "notification-close-btn",

				                            onclick: move |_| is_open.set(false),

				                            Icon { icon: BsX, width: 16, height: 16 }

sharang commented

[medium] Deeply nested control flow in notification rendering

The notification panel rendering contains deeply nested conditional logic (if/else chains) that makes it difficult to understand the flow and increases the risk of logic errors when modifying the component. The nested structure makes it hard to reason about what gets rendered under what conditions.

Suggested fix: Flatten the conditional rendering by extracting each state (loading, empty, notifications) into separate components or functions, reducing nesting levels and improving readability.

*Scanner: code-review/complexity | *

**[medium] Deeply nested control flow in notification rendering** The notification panel rendering contains deeply nested conditional logic (if/else chains) that makes it difficult to understand the flow and increases the risk of logic errors when modifying the component. The nested structure makes it hard to reason about what gets rendered under what conditions. Suggested fix: Flatten the conditional rendering by extracting each state (loading, empty, notifications) into separate components or functions, reducing nesting levels and improving readability. *Scanner: code-review/complexity | *

compliance-dashboard/src/components/notification_bell.rs

						
				@@ -0,0 +127,4 @@

				                                                button {

				                                                    class: "notification-dismiss-btn",

				                                                    title: "Dismiss",

				                                                    onclick: move |_| on_dismiss(dismiss_id.clone()),

sharang commented

[high] Potential XSS vulnerability in notification CVE ID rendering

The notification component renders CVE IDs directly from API responses without sanitization. If an attacker can control the CVE ID field in the notification data, they could inject malicious HTML/JavaScript into the page. This occurs in the notification item rendering where '{notif.cve_id}' is directly inserted into the DOM.

Suggested fix: Sanitize the CVE ID before rendering it to prevent XSS. Use a library like dioxus-html or manually escape HTML characters in the CVE ID field before displaying it.

Scanner: code-review/security | CWE: CWE-79

**[high] Potential XSS vulnerability in notification CVE ID rendering** The notification component renders CVE IDs directly from API responses without sanitization. If an attacker can control the CVE ID field in the notification data, they could inject malicious HTML/JavaScript into the page. This occurs in the notification item rendering where '{notif.cve_id}' is directly inserted into the DOM. Suggested fix: Sanitize the CVE ID before rendering it to prevent XSS. Use a library like `dioxus-html` or manually escape HTML characters in the CVE ID field before displaying it. *Scanner: code-review/security | CWE: CWE-79*

sharang commented

[medium] Potential Panic in Notification Dismissal

The on_dismiss handler uses unwrap_or("") when extracting the notification ID, which could panic if the ObjectId structure changes unexpectedly. This creates an inconsistency with the module's established pattern of graceful error handling.

Suggested fix: Use a more robust approach like map_or_else or match to handle the case where the ObjectId might not be present, maintaining consistency with the module's error handling strategy.

*Scanner: code-review/convention | *

**[medium] Potential Panic in Notification Dismissal** The `on_dismiss` handler uses `unwrap_or("")` when extracting the notification ID, which could panic if the ObjectId structure changes unexpectedly. This creates an inconsistency with the module's established pattern of graceful error handling. Suggested fix: Use a more robust approach like `map_or_else` or `match` to handle the case where the ObjectId might not be present, maintaining consistency with the module's error handling strategy. *Scanner: code-review/convention | *

sharang commented

[medium] Unwrapped Option in Notification Processing

The code uses .unwrap_or("") on a potentially critical value extraction path, which could lead to silent failures if the MongoDB ObjectId structure changes or becomes malformed. This violates the module's error handling consistency where other operations use if let Ok(...) patterns.

Suggested fix: Replace .unwrap_or("") with proper error handling using if let Some(...) or match to handle the case where the ObjectId might be missing or malformed, ensuring consistent error propagation throughout the module.

*Scanner: code-review/convention | *

**[medium] Unwrapped Option in Notification Processing** The code uses `.unwrap_or("")` on a potentially critical value extraction path, which could lead to silent failures if the MongoDB ObjectId structure changes or becomes malformed. This violates the module's error handling consistency where other operations use `if let Ok(...)` patterns. Suggested fix: Replace `.unwrap_or("")` with proper error handling using `if let Some(...)` or `match` to handle the case where the ObjectId might be missing or malformed, ensuring consistent error propagation throughout the module. *Scanner: code-review/convention | *

compliance-dashboard/src/components/notification_bell.rs

						
				@@ -0,0 +140,4 @@

				                                            if let Some(ref summary) = notif.summary {

				                                                div { class: "notification-item-summary",

				                                                    "{summary}"

				                                                }

sharang commented

[high] Potential XSS vulnerability in notification summary rendering

The notification component renders summary text directly from API responses without sanitization. If an attacker can control the summary field in notification data, they could inject malicious HTML/JavaScript. This occurs in the notification item rendering where '{summary}' is directly inserted into the DOM.

Suggested fix: Sanitize the summary text before rendering it to prevent XSS. Either escape HTML characters or use a proper HTML sanitization library before inserting into the DOM.

Scanner: code-review/security | CWE: CWE-79

**[high] Potential XSS vulnerability in notification summary rendering** The notification component renders summary text directly from API responses without sanitization. If an attacker can control the summary field in notification data, they could inject malicious HTML/JavaScript. This occurs in the notification item rendering where '{summary}' is directly inserted into the DOM. Suggested fix: Sanitize the summary text before rendering it to prevent XSS. Either escape HTML characters or use a proper HTML sanitization library before inserting into the DOM. *Scanner: code-review/security | CWE: CWE-79*

compliance-dashboard/src/infrastructure/notifications.rs

						
				@@ -0,0 +12,4 @@

				pub struct CveNotificationData {

				    #[serde(rename = "_id")]

				    pub id: Option<serde_json::Value>,

				    pub cve_id: String,

sharang commented

[medium] Unvalidated API responses in notification data structures

The CveNotificationData struct uses serde_json::Value for id and created_at fields which bypasses compile-time type checking. This could lead to runtime panics or unexpected behavior if the API returns unexpected data types for these fields, especially since they're marked as optional but still parsed as generic JSON values.

Suggested fix: Consider using more specific types for id and created_at fields, or add validation logic to ensure these fields contain expected data types when deserializing from the API response.

*Scanner: code-review/convention | *

**[medium] Unvalidated API responses in notification data structures** The CveNotificationData struct uses serde_json::Value for id and created_at fields which bypasses compile-time type checking. This could lead to runtime panics or unexpected behavior if the API returns unexpected data types for these fields, especially since they're marked as optional but still parsed as generic JSON values. Suggested fix: Consider using more specific types for id and created_at fields, or add validation logic to ensure these fields contain expected data types when deserializing from the API response. *Scanner: code-review/convention | *

compliance-dashboard/src/infrastructure/notifications.rs

						
				@@ -0,0 +24,4 @@

				    #[serde(default)]

				    pub created_at: Option<serde_json::Value>,

				}

sharang commented

[high] Missing error handling for HTTP request failures

The fetch_notification_count, fetch_notifications, mark_all_notifications_read, and dismiss_notification functions do not check the HTTP status code of the response. If the API returns an error status (like 4xx or 5xx), the code will still attempt to parse the response body as success, potentially leading to incorrect behavior or panics when trying to deserialize error responses.

Suggested fix: Add explicit status code checking before deserializing the response body. For example: if !resp.status().is_success() { return Err(ServerFnError::new(format!("HTTP Error: {}", resp.status()))); }

*Scanner: code-review/logic | *

**[high] Missing error handling for HTTP request failures** The `fetch_notification_count`, `fetch_notifications`, `mark_all_notifications_read`, and `dismiss_notification` functions do not check the HTTP status code of the response. If the API returns an error status (like 4xx or 5xx), the code will still attempt to parse the response body as success, potentially leading to incorrect behavior or panics when trying to deserialize error responses. Suggested fix: Add explicit status code checking before deserializing the response body. For example: `if !resp.status().is_success() { return Err(ServerFnError::new(format!("HTTP Error: {}", resp.status()))); }` *Scanner: code-review/logic | *

sharang commented

[high] Server-Side Request Forgery (SSRF) via User-Controlled URL

The fetch_notifications, fetch_notification_count, mark_all_notifications_read, and dismiss_notification server functions construct URLs by concatenating user-provided data from state.agent_api_url without proper validation. If an attacker can control the agent_api_url configuration value, they could redirect requests to internal services or external malicious endpoints, enabling SSRF attacks.

Suggested fix: Validate and sanitize the agent_api_url configuration value to ensure it points to allowed domains or use a whitelist approach. Consider using a proxy or restricted network access for API calls.

Scanner: code-review/security | CWE: CWE-918

**[high] Server-Side Request Forgery (SSRF) via User-Controlled URL** The `fetch_notifications`, `fetch_notification_count`, `mark_all_notifications_read`, and `dismiss_notification` server functions construct URLs by concatenating user-provided data from `state.agent_api_url` without proper validation. If an attacker can control the `agent_api_url` configuration value, they could redirect requests to internal services or external malicious endpoints, enabling SSRF attacks. Suggested fix: Validate and sanitize the `agent_api_url` configuration value to ensure it points to allowed domains or use a whitelist approach. Consider using a proxy or restricted network access for API calls. *Scanner: code-review/security | CWE: CWE-918*

compliance-dashboard/src/infrastructure/notifications.rs

						
				@@ -0,0 +34,4 @@

				pub async fn fetch_notification_count() -> Result<u64, ServerFnError> {

				    let state: super::server_state::ServerState =

				        dioxus_fullstack::FullstackContext::extract().await?;

sharang commented

[medium] Complex boolean expressions in error handling

The error handling pattern uses nested map_err calls which can make it difficult to trace the flow of errors and may lead to less informative error messages during debugging.

Suggested fix: Consider using a more explicit error handling approach with early returns or a custom error conversion function to improve readability and maintainability.

*Scanner: code-review/complexity | *

**[medium] Complex boolean expressions in error handling** The error handling pattern uses nested `map_err` calls which can make it difficult to trace the flow of errors and may lead to less informative error messages during debugging. Suggested fix: Consider using a more explicit error handling approach with early returns or a custom error conversion function to improve readability and maintainability. *Scanner: code-review/complexity | *

sharang commented

[medium] Inconsistent error handling in notification API

The notification API functions use inconsistent error handling patterns. Some functions map errors to ServerFnError::new(e.to_string()) while others could potentially hide failures by not properly propagating HTTP status codes or response parsing errors. The error mapping should be consistent across all server functions.

Suggested fix: Standardize error handling by ensuring all HTTP request failures and JSON parsing errors are consistently wrapped with appropriate ServerFnError variants, and consider including HTTP status codes in error messages for better debugging.

*Scanner: code-review/convention | *

**[medium] Inconsistent error handling in notification API** The notification API functions use inconsistent error handling patterns. Some functions map errors to ServerFnError::new(e.to_string()) while others could potentially hide failures by not properly propagating HTTP status codes or response parsing errors. The error mapping should be consistent across all server functions. Suggested fix: Standardize error handling by ensuring all HTTP request failures and JSON parsing errors are consistently wrapped with appropriate ServerFnError variants, and consider including HTTP status codes in error messages for better debugging. *Scanner: code-review/convention | *

compliance-dashboard/src/infrastructure/notifications.rs

						
				@@ -0,0 +79,4 @@

				#[server]

				pub async fn dismiss_notification(id: String) -> Result<(), ServerFnError> {

				    let state: super::server_state::ServerState =

				        dioxus_fullstack::FullstackContext::extract().await?;

sharang commented

[medium] Potential silent failure in dismiss_notification

The dismiss_notification function sends a PATCH request but doesn't check the HTTP response status code before considering the operation successful. This could lead to silent failures when the API returns an error status code, making it appear as though the notification was dismissed when it wasn't.

Suggested fix: Add response status code validation using resp.status().is_success() or similar before considering the operation complete, and return appropriate ServerFnError for non-success status codes.

*Scanner: code-review/convention | *

**[medium] Potential silent failure in dismiss_notification** The dismiss_notification function sends a PATCH request but doesn't check the HTTP response status code before considering the operation successful. This could lead to silent failures when the API returns an error status code, making it appear as though the notification was dismissed when it wasn't. Suggested fix: Add response status code validation using `resp.status().is_success()` or similar before considering the operation complete, and return appropriate ServerFnError for non-success status codes. *Scanner: code-review/convention | *

compliance-dashboard/src/infrastructure/notifications.rs

						
				@@ -0,0 +80,4 @@

				pub async fn dismiss_notification(id: String) -> Result<(), ServerFnError> {

				    let state: super::server_state::ServerState =

				        dioxus_fullstack::FullstackContext::extract().await?;

sharang commented