sharang/compliance-scanner-agent
1
0
Fork 0
Code Issues 51 Pull Requests 1 Actions Packages Projects Releases 1 Wiki Activity

fix: cascade-delete DAST targets, pentests, and downstream data on repo delete #50

Merged
sharang merged 1 commits from fix/cascade-delete-repo into main 2026-03-30 07:11:24 +00:00
Conversation 1 Commits 1 Files Changed 1 +87
1 changed files with 87 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files

87
compliance-agent/src/api/handlers/repos.rs
View File

@@ -237,5 +237,92 @@ pub async fn delete_repository(
.delete_many(doc! { "repo_id": &id })
.await;
// Cascade delete DAST targets linked to this repo, and all their downstream data
sharang commented 2026-03-29 21:29:50 +00:00
Review
Copy Link

[medium] Complex nested control flow in delete_repository function

The delete_repository function has deeply nested control flow with multiple if-let statements and while loops, making it difficult to follow the execution path and increasing the risk of logic errors.

Suggested fix: Refactor the cascading deletion logic into separate functions with clear responsibilities. Consider using early returns or extracting the cursor processing logic into helper functions.

*Scanner: code-review/complexity | *

**[medium] Complex nested control flow in delete_repository function** The delete_repository function has deeply nested control flow with multiple if-let statements and while loops, making it difficult to follow the execution path and increasing the risk of logic errors. Suggested fix: Refactor the cascading deletion logic into separate functions with clear responsibilities. Consider using early returns or extracting the cursor processing logic into helper functions. *Scanner: code-review/complexity | * <!-- compliance-fp:1de068468b5f1bac1ee6f892a7a8748749ab9eb1b1bb69054ffb8af125309bf6 -->
sharang commented 2026-03-29 21:29:50 +00:00
Review
Copy Link

[high] Insecure Direct Object Reference (IDOR)

The delete_repository function allows deletion of repository-related data including DAST targets and pentest sessions. However, there's no explicit authorization check to ensure that the requesting user has permission to delete these resources. This could allow unauthorized users to delete sensitive data associated with repositories they don't own.

Suggested fix: Add authentication and authorization checks before performing deletions. Verify that the authenticated user has appropriate permissions to delete the repository and its associated data.

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

**[high] Insecure Direct Object Reference (IDOR)** The `delete_repository` function allows deletion of repository-related data including DAST targets and pentest sessions. However, there's no explicit authorization check to ensure that the requesting user has permission to delete these resources. This could allow unauthorized users to delete sensitive data associated with repositories they don't own. Suggested fix: Add authentication and authorization checks before performing deletions. Verify that the authenticated user has appropriate permissions to delete the repository and its associated data. *Scanner: code-review/security | CWE: CWE-639* <!-- compliance-fp:c725e1ce0c994e82b8b0ef51055bf2acb56639d0af8f616a398e93a8da5083ec -->
sharang commented 2026-03-29 21:29:50 +00:00
Review
Copy Link

[medium] Potential Race Condition in Deletion Logic

Multiple database operations are performed sequentially within the cascade delete functions. There's a potential race condition where concurrent requests might interfere with each other during deletion, especially when deleting related entities like attack chain nodes or messages. This could result in inconsistent state or partial deletions.

Suggested fix: Implement atomic operations or use transactions where possible to ensure consistency during cascading deletions. Consider batching related deletions to reduce the window for race conditions.

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

**[medium] Potential Race Condition in Deletion Logic** Multiple database operations are performed sequentially within the cascade delete functions. There's a potential race condition where concurrent requests might interfere with each other during deletion, especially when deleting related entities like attack chain nodes or messages. This could result in inconsistent state or partial deletions. Suggested fix: Implement atomic operations or use transactions where possible to ensure consistency during cascading deletions. Consider batching related deletions to reduce the window for race conditions. *Scanner: code-review/security | CWE: CWE-362* <!-- compliance-fp:13cd53e05ca2433cf85c93bd675d6ed0c28a17d6cc3352a30d5fb2207fbaae60 -->
// (scan runs, findings, pentest sessions, attack chains, messages)
sharang commented 2026-03-29 21:29:50 +00:00
Review
Copy Link

[medium] Potential panic from unwrap_or_default() on empty ObjectId

The code uses target_id.map(|oid| oid.to_hex()).unwrap_or_default() which can result in an empty string when the ObjectId is None. This is then checked with if !target_id.is_empty() but the empty string case is not explicitly handled, potentially leading to unexpected behavior when deleting targets.

Suggested fix: Consider explicitly handling the case where target.id is None by returning early or logging a warning, rather than relying on the default empty string behavior.

*Scanner: code-review/convention | *

**[medium] Potential panic from unwrap_or_default() on empty ObjectId** The code uses `target_id.map(|oid| oid.to_hex()).unwrap_or_default()` which can result in an empty string when the ObjectId is None. This is then checked with `if !target_id.is_empty()` but the empty string case is not explicitly handled, potentially leading to unexpected behavior when deleting targets. Suggested fix: Consider explicitly handling the case where target.id is None by returning early or logging a warning, rather than relying on the default empty string behavior. *Scanner: code-review/convention | * <!-- compliance-fp:4810d633ba841d5e411efc3deb6bfa02dd90d704d5a8e228d0a483fbe8b63a63 -->
if let Ok(mut cursor) = db.dast_targets().find(doc! { "repo_id": &id }).await {
sharang commented 2026-03-29 21:29:50 +00:00
Review
Copy Link

[high] Missing error handling for database operations

Multiple database operations use let _ = ... which silently ignores errors. If any of these operations fail (e.g., network issues, permission problems), the cascade delete will be incomplete and inconsistent state may result.

Suggested fix: Handle errors appropriately instead of ignoring them, possibly logging failures and considering rollback mechanisms

*Scanner: code-review/logic | *

**[high] Missing error handling for database operations** Multiple database operations use `let _ = ...` which silently ignores errors. If any of these operations fail (e.g., network issues, permission problems), the cascade delete will be incomplete and inconsistent state may result. Suggested fix: Handle errors appropriately instead of ignoring them, possibly logging failures and considering rollback mechanisms *Scanner: code-review/logic | * <!-- compliance-fp:fab160369253e83242ea1a6d157fc432a4ef1e70016062ba8c4e670b49f8881f -->
use futures_util::StreamExt;
while let Some(Ok(target)) = cursor.next().await {
sharang commented 2026-03-29 21:29:50 +00:00
Review
Copy Link

[medium] Inconsistent error handling in cascade deletion functions

The cascade deletion functions use let _ = ... for all database operations, which silently ignores errors. This pattern is inconsistent with the rest of the codebase where errors are properly handled or logged. The cascade delete operations should either propagate errors or log them appropriately.

Suggested fix: Replace let _ = ... with proper error handling. Either return errors from the function or log them using the appropriate logging mechanism.

*Scanner: code-review/convention | *

**[medium] Inconsistent error handling in cascade deletion functions** The cascade deletion functions use `let _ = ...` for all database operations, which silently ignores errors. This pattern is inconsistent with the rest of the codebase where errors are properly handled or logged. The cascade delete operations should either propagate errors or log them appropriately. Suggested fix: Replace `let _ = ...` with proper error handling. Either return errors from the function or log them using the appropriate logging mechanism. *Scanner: code-review/convention | * <!-- compliance-fp:cd4c032bcf86e2d03aefd8117fa812b5926dc1536ac452cfbe060cdb89fd9167 -->
let target_id = target.id.map(|oid| oid.to_hex()).unwrap_or_default();
if !target_id.is_empty() {
cascade_delete_dast_target(db, &target_id).await;
sharang commented 2026-03-29 21:29:50 +00:00
Review
Copy Link

[high] Potential panic from unwrapping empty ObjectId

The code uses unwrap_or_default() on target.id.map(|oid| oid.to_hex()) which can return an empty string when the ObjectId is None. Later, mongodb::bson::oid::ObjectId::parse_str(target_id) is called with this potentially empty string, which will cause a panic because parsing an empty string into an ObjectId fails.

Suggested fix: Check if target_id is not empty before attempting to parse it as ObjectId, or handle the error case properly instead of using unwrap_or_default()

*Scanner: code-review/logic | *

**[high] Potential panic from unwrapping empty ObjectId** The code uses `unwrap_or_default()` on `target.id.map(|oid| oid.to_hex())` which can return an empty string when the ObjectId is None. Later, `mongodb::bson::oid::ObjectId::parse_str(target_id)` is called with this potentially empty string, which will cause a panic because parsing an empty string into an ObjectId fails. Suggested fix: Check if target_id is not empty before attempting to parse it as ObjectId, or handle the error case properly instead of using unwrap_or_default() *Scanner: code-review/logic | * <!-- compliance-fp:c5513c584b76f8c7df0f172b47f2188506ae82104f6f879b7c7de4dfc42d7b02 -->
}
}
}
// Also delete pentest sessions linked directly to this repo (not via target)
if let Ok(mut cursor) = db.pentest_sessions().find(doc! { "repo_id": &id }).await {
use futures_util::StreamExt;
while let Some(Ok(session)) = cursor.next().await {
let session_id = session.id.map(|oid| oid.to_hex()).unwrap_or_default();
if !session_id.is_empty() {
sharang commented 2026-03-29 21:29:50 +00:00
Review
Copy Link

[medium] Code duplication in session cleanup logic

The same pattern of deleting attack chain nodes, pentest messages, and dast findings by session_id appears twice - once in delete_repository and once in cascade_delete_dast_target, increasing maintenance burden and potential for inconsistencies.

Suggested fix: Extract the session cleanup logic into a reusable async function that takes the database connection and session ID as parameters.

*Scanner: code-review/complexity | *

**[medium] Code duplication in session cleanup logic** The same pattern of deleting attack chain nodes, pentest messages, and dast findings by session_id appears twice - once in delete_repository and once in cascade_delete_dast_target, increasing maintenance burden and potential for inconsistencies. Suggested fix: Extract the session cleanup logic into a reusable async function that takes the database connection and session ID as parameters. *Scanner: code-review/complexity | * <!-- compliance-fp:30cd1dc3e184e6038bdda5c61606a7719de680b603d0667d121c5a64ce8ee6da -->
let _ = db
.attack_chain_nodes()
.delete_many(doc! { "session_id": &session_id })
.await;
let _ = db
sharang commented 2026-03-29 21:29:50 +00:00
Review
Copy Link

[high] Potential Command Injection via MongoDB Query

The code constructs MongoDB queries using user-provided data without proper sanitization. Specifically, in cascade_delete_dast_target, the target_id parameter is used directly in a query filter (doc! { "_id": oid }) where oid is parsed from target_id. If target_id contains malicious input that can be interpreted as a MongoDB operator, it could lead to unauthorized data access or modification.

Suggested fix: Validate and sanitize the target_id before parsing it into an ObjectId. Ensure that the input conforms to expected format (e.g., valid hexadecimal string of 24 characters) and reject any inputs that may contain MongoDB operators.

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

**[high] Potential Command Injection via MongoDB Query** The code constructs MongoDB queries using user-provided data without proper sanitization. Specifically, in `cascade_delete_dast_target`, the `target_id` parameter is used directly in a query filter (`doc! { "_id": oid }`) where `oid` is parsed from `target_id`. If `target_id` contains malicious input that can be interpreted as a MongoDB operator, it could lead to unauthorized data access or modification. Suggested fix: Validate and sanitize the `target_id` before parsing it into an ObjectId. Ensure that the input conforms to expected format (e.g., valid hexadecimal string of 24 characters) and reject any inputs that may contain MongoDB operators. *Scanner: code-review/security | CWE: CWE-94* <!-- compliance-fp:48865e66939732e4f3a91ac9a2b3b93677cf7a05f4f2df60724b1458df173e19 -->
.pentest_messages()
.delete_many(doc! { "session_id": &session_id })
.await;
// Delete DAST findings produced by this session
let _ = db
.dast_findings()
.delete_many(doc! { "session_id": &session_id })
.await;
}
}
sharang commented 2026-03-29 21:29:50 +00:00
Review
Copy Link

[medium] Missing error propagation in cascade_delete_dast_target

The cascade_delete_dast_target function uses let _ = ... for all database operations, completely ignoring potential errors. This could lead to partial cleanup operations where some related data is deleted but others aren't, leaving the database in an inconsistent state.

Suggested fix: Either propagate errors from the database operations or implement proper error logging to ensure that failures during cascading deletes are not silently ignored.

*Scanner: code-review/convention | *

**[medium] Missing error propagation in cascade_delete_dast_target** The `cascade_delete_dast_target` function uses `let _ = ...` for all database operations, completely ignoring potential errors. This could lead to partial cleanup operations where some related data is deleted but others aren't, leaving the database in an inconsistent state. Suggested fix: Either propagate errors from the database operations or implement proper error logging to ensure that failures during cascading deletes are not silently ignored. *Scanner: code-review/convention | * <!-- compliance-fp:a8733363c3bd94ece2871d8bb5be9933e04936746e949fabd7f9b2210c540541 -->
}
sharang commented 2026-03-29 21:29:50 +00:00
Review
Copy Link

[medium] Unnecessary database operations in cascade delete

In cascade_delete_dast_target, after deleting pentest sessions, the code deletes DAST findings again using session_id. However, these findings were already deleted when deleting pentest sessions. This results in redundant database calls that could be avoided.

Suggested fix: Remove the redundant deletion of DAST findings based on session_id since they're already deleted during pentest session deletion

*Scanner: code-review/logic | *

**[medium] Unnecessary database operations in cascade delete** In `cascade_delete_dast_target`, after deleting pentest sessions, the code deletes DAST findings again using session_id. However, these findings were already deleted when deleting pentest sessions. This results in redundant database calls that could be avoided. Suggested fix: Remove the redundant deletion of DAST findings based on session_id since they're already deleted during pentest session deletion *Scanner: code-review/logic | * <!-- compliance-fp:dcc84b0d7f562283a9ff2b05702f3e0a576607c096203ed574bd2572e7087470 -->
let _ = db
.pentest_sessions()
sharang commented 2026-03-29 21:29:50 +00:00
Review
Copy Link

[high] High cyclomatic complexity in cascade_delete_dast_target function

The cascade_delete_dast_target function contains multiple nested operations with several database calls and conditional checks, creating high complexity that makes it prone to bugs and difficult to test.

Suggested fix: Break down this function into smaller, focused functions for each deletion category (pentest sessions, findings, scan runs, target deletion). Each function should handle one responsibility and return appropriate error types.

*Scanner: code-review/complexity | *

**[high] High cyclomatic complexity in cascade_delete_dast_target function** The cascade_delete_dast_target function contains multiple nested operations with several database calls and conditional checks, creating high complexity that makes it prone to bugs and difficult to test. Suggested fix: Break down this function into smaller, focused functions for each deletion category (pentest sessions, findings, scan runs, target deletion). Each function should handle one responsibility and return appropriate error types. *Scanner: code-review/complexity | * <!-- compliance-fp:78a484c9c710e57ff31f512d2aa3ca6b01320e2dfa39af8e3b4429e876cb3211 -->
.delete_many(doc! { "repo_id": &id })
.await;
Ok(Json(serde_json::json!({ "status": "deleted" })))
}
/// Cascade-delete a DAST target and all its downstream data.
async fn cascade_delete_dast_target(db: &crate::database::Database, target_id: &str) {
// Delete pentest sessions for this target (and their attack chains + messages)
if let Ok(mut cursor) = db
.pentest_sessions()
.find(doc! { "target_id": target_id })
.await
{
use futures_util::StreamExt;
while let Some(Ok(session)) = cursor.next().await {
let session_id = session.id.map(|oid| oid.to_hex()).unwrap_or_default();
if !session_id.is_empty() {
let _ = db
.attack_chain_nodes()
.delete_many(doc! { "session_id": &session_id })
.await;
let _ = db
.pentest_messages()
.delete_many(doc! { "session_id": &session_id })
.await;
let _ = db
.dast_findings()
.delete_many(doc! { "session_id": &session_id })
.await;
}
}
}
let _ = db
.pentest_sessions()
.delete_many(doc! { "target_id": target_id })
.await;
// Delete DAST scan runs and their findings
let _ = db
.dast_findings()
.delete_many(doc! { "target_id": target_id })
.await;
let _ = db
.dast_scan_runs()
.delete_many(doc! { "target_id": target_id })
.await;
// Delete the target itself
if let Ok(oid) = mongodb::bson::oid::ObjectId::parse_str(target_id) {
let _ = db.dast_targets().delete_one(doc! { "_id": oid }).await;
}
}