[PM-26925] Complete security tasks when deleting

[nick-livefront]

🎟️ Tracking

PM-26925

📔 Objective

When a cipher is soft deleted and has associated security tasks, they should be marked as completed.

📸 Screenshots

delete-at-risk-ciphers.mov

[claude]

Claude finished @nick-livefront's task —— View job

---

PR Review: Complete security tasks when deleting

Summary of Changes

This PR implements functionality to automatically mark security tasks as completed when their associated ciphers are soft deleted. The implementation spans the repository layer (both Dapper and EF), service layer, database stored procedure, and includes comprehensive test coverage.

Changed files:

• Repository interface and implementations (Dapper & EF)
• CipherService with soft delete logic
• SQL stored procedure for bulk completion
• Database migration script
• Unit and integration tests

---

Critical Issues

❌ 1. SQL Injection Risk in Dapper Implementation

Location: src/Infrastructure.Dapper/Vault/Repositories/SecurityTaskRepository.cs:100

The Dapper implementation uses ToGuidIdArrayTVP() extension method to pass cipher IDs, which should be safe for SQL injection, but this needs verification. Ensure ToGuidIdArrayTVP() properly validates and parameterizes the input.

Code snippet

await connection.ExecuteAsync(
    $"[{Schema}].[SecurityTask_MarkCompleteByCipherIds]",
    new { CipherIds = cipherIds.ToGuidIdArrayTVP() },
    commandType: CommandType.StoredProcedure);

Recommendation: Verify that ToGuidIdArrayTVP() creates a properly typed table-valued parameter. If not already documented, add a code comment confirming this is SQL injection safe.

---

⚠️ 2. Order of Operations in SoftDeleteAsync

Location: src/Core/Vault/Services/Implementations/CipherService.cs:730-732

Security tasks are marked as complete before the cipher is actually soft deleted in the database. If the cipher deletion fails, security tasks will be incorrectly marked as completed.

Current code

await _securityTaskRepository.MarkAsCompleteByCipherIds([cipherDetails.Id]);
await _cipherRepository.UpsertAsync(cipherDetails);
await _eventService.LogCipherEventAsync(cipherDetails, EventType.Cipher_SoftDeleted);

Recommendation: Either:

1. Move MarkAsCompleteByCipherIds after UpsertAsync to ensure atomicity
2. Wrap both operations in a transaction
3. Document why the current order is acceptable if there's a specific reason

---

❓ 3. Missing Transaction Management

Location: Both CipherService.cs:730-732 and CipherService.cs:757

The soft delete operations don't use transactions. If MarkAsCompleteByCipherIds succeeds but UpsertAsync/SoftDeleteAsync fails, you'll have inconsistent state (completed tasks with non-deleted ciphers).

Recommendation: Consider wrapping related operations in a database transaction to ensure atomicity, or document why eventual consistency is acceptable here.

---

💭 4. Performance Consideration: Bulk Operations

Location: src/Core/Vault/Services/Implementations/CipherService.cs:757

In SoftDeleteManyAsync, the security task completion happens after getting the filtered ciphers but could potentially be optimized.

Current flow:

1. Get ciphers
2. Filter by permissions
3. Soft delete in DB
4. Then mark security tasks complete

Consideration: This ordering is actually correct (security tasks should only be completed for ciphers that were successfully deleted), but there's no error handling if MarkAsCompleteByCipherIds fails after deletion succeeds.

---

Suggested Improvements

🎨 1. Empty Collection Handling Inconsistency

Locations:

• SecurityTaskRepository.cs:92 (Dapper) - checks !cipherIds.Any()
• SecurityTaskRepository.cs:103 (EF) - checks !cipherIds.Any()

Both implementations correctly handle empty collections by returning early. This is good! However, consider if callers should ever pass empty collections.

Suggestion: Add guard clause in CipherService to avoid unnecessary repository calls when there are no cipher IDs.

---

🎨 2. Magic Numbers in SQL

Locations:

• SecurityTask_MarkCompleteByCipherIds.sql:10 - uses 1 for completed status
• Migration script line 10 - uses same magic number

Current:

[Status] = 1, -- completed

Suggestion: While the comment helps, consider using a named constant or at minimum ensure this matches the enum value SecurityTaskStatus.Completed (which appears to be 1 based on the C# code).

---

🎨 3. Missing XML Documentation

Location: src/Core/Vault/Repositories/ISecurityTaskRepository.cs:43

Good documentation exists for the method, but could be enhanced:

Current:

/// <summary>
/// Marks all tasks associated with the respective ciphers as complete.
/// </summary>
/// <param name="cipherIds">Collection of cipher IDs</param>
Task MarkAsCompleteByCipherIds(IEnumerable<Guid> cipherIds);

Suggestion: Add more detail:

• What happens if cipher IDs don't exist?
• What happens if tasks are already completed?
• Does this update the RevisionDate?

/// <summary>
/// Marks all pending security tasks associated with the specified ciphers as complete.
/// Already completed tasks are not modified. Updates the RevisionDate to current UTC time.
/// </summary>
/// <param name="cipherIds">Collection of cipher IDs. Empty collections are safely ignored.</param>
/// <returns>A task representing the asynchronous operation</returns>

---

🎨 4. Test Coverage - Edge Case

Location: test/Core.Test/Vault/Services/CipherServiceTests.cs

The unit tests verify that MarkAsCompleteByCipherIds is called, but don't test what happens if it throws an exception.

Suggestion: Add a test case:

[Theory, BitAutoData]
public async Task SoftDeleteAsync_SecurityTaskRepositoryThrows_PropagatesException(...)
{
    // Verify that if MarkAsCompleteByCipherIds fails, the soft delete doesn't proceed
}

---

🎨 5. Null Reference Potential (Minor)

Location: src/Core/Vault/Services/Implementations/CipherService.cs:757

The code calls .Select(c => c.Id) on deletingCiphers list. While unlikely to be null given the logic flow, defensive programming suggests:

await _securityTaskRepository.MarkAsCompleteByCipherIds(deletingCiphers?.Select(c => c.Id) ?? Enumerable.Empty<Guid>());

However, given the repository already handles empty collections gracefully, this is very low priority.

---

Good Practices Observed

👍 Strong test coverage - Both unit tests in CipherServiceTests and integration tests in SecurityTaskRepositoryTests

👍 Consistent implementation - Both Dapper and Entity Framework implementations handle the same edge cases

👍 Empty collection guards - Repository implementations properly handle empty inputs

👍 SQL optimization - Stored procedure only updates non-completed tasks (AND [Status] <> 1)

👍 Proper use of TVP - Table-valued parameters for bulk operations instead of dynamic SQL

👍 RevisionDate updates - Properly updates timestamps when tasks are completed

---

Action Items for Author

High Priority

1. ⚠️ Verify order of operations in SoftDeleteAsync - should security tasks be marked complete before or after cipher deletion?
2. ❌ Consider transaction management - should these operations be atomic?
3. ❓ Verify SQL injection safety - confirm ToGuidIdArrayTVP() is properly implemented

Medium Priority

4. 🎨 Add test case for exception handling when MarkAsCompleteByCipherIds fails
5. 🎨 Enhance XML documentation with more implementation details

Low Priority

6. 🎨 Consider adding guard clause in service to avoid calling repository with empty collections
7. ⛏ Minor: Add more descriptive comments for magic number 1 representing completed status

---

Security & Compliance Notes

✅ Zero-knowledge compliance - No sensitive data is logged or exposed

✅ Parameterized queries - Uses stored procedures with TVPs (assuming ToGuidIdArrayTVP() is implemented correctly)

✅ Permission checks - Soft delete already has permission validation upstream

✅ Audit logging - Cipher events are properly logged

---

Overall Assessment

This is a well-implemented feature with good test coverage and proper consideration for both data persistence strategies (Dapper and EF). The main concerns are around atomicity and error handling - specifically the order of operations and lack of transaction management. Please address the critical issues around operation ordering and transaction boundaries.

---

[codecov]

Codecov Report

❌ Patch coverage is 77.77778% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.04%. Comparing base (b15913c) to head (d0e9cf5).
⚠️ Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
...apper/Vault/Repositories/SecurityTaskRepository.cs	70.00%	2 Missing and 1 partial ⚠️
...ework/Vault/Repositories/SecurityTaskRepository.cs	76.92%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6492      +/-   ##
==========================================
+ Coverage   51.98%   56.04%   +4.05%     
==========================================
  Files        1872     1876       +4     
  Lines       82537    82660     +123     
  Branches     7299     7319      +20     
==========================================
+ Hits        42908    46326    +3418     
+ Misses      37972    34598    -3374     
- Partials     1657     1736      +79     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[github-actions]

Checkmarx One – Scan Summary & Details – f206a7f1-35aa-45ff-99f9-6e8a45c5ec42

Great job! No new security vulnerabilities introduced in this pull request

[shane-melton]

Changes look good! Nice work!

[shane-melton]

🧪 🎉 Thanks!

[nick-livefront]

@bitwarden/dept-dbops Could this one get a review when convenient?

[withinfocus]

Database work seems fine. @rkac-bw and @mkincaid-bw are the true DbOps team at this point so they can chime in if necessary.