[PM-28813] Implement encryption diagnostics & recovery tool

[quexten]

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-28813
bitwarden/server#6659

📔 Objective

To get diagnostics for certain types of encryption related issues, and to fix users vaults, this PR adds a tool that:

• Verifies integrity of encryption for private keys, folders, vault items
• Offers the option to "repair" the vault

Repairing the vault here means replacing the private keys, should they be corrupt, and removing vault items / folders, should they be corrupt.

This will help CS track down issues and resolve issues with customers faster.

📸 Screenshots

Note: In the video the private key stays corrupt. This is because the code used when recording the video marks all private keys as corrupt, just for testing / demo purposes.

Screen.Recording.2025-12-01.at.12.21.28.mov

⏰ Reminders before review

• Contributor guidelines followed
• All formatters and local linters executed and passed
• Written new unit and / or integration tests where applicable
• Protected functional changes with optionality (feature flags)
• Used internationalization (i18n) for all UI strings
• CI builds passed
• Communicated to DevOps any deployment requirements
• Updated any necessary documentation (Confluence, contributing docs) or informed the documentation team

🦮 Reviewer guidelines

• 👍 (:+1:) or similar for great changes
• 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
• ❓ (:question:) for questions
• 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion
• 🎨 (:art:) for suggestions / improvements
• ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
• 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
• ⛏ (:pick:) for minor or nitpick changes

[github-actions]

Checkmarx One – Scan Summary & Details – 4310cf9c-7529-4899-b71e-81f83e82962a

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

[codecov]

Codecov Report

❌ Patch coverage is 54.09253% with 129 lines in your changes missing coverage. Please review.
✅ Project coverage is 41.74%. Comparing base (ec5a54f) to head (d13ecc2).
⚠️ Report is 41 commits behind head on main.
✅ All tests successful. No failed tests found.

Files with missing lines	Patch %	Lines
.../key-management/data-recovery/steps/folder-step.ts	16.27%	36 Missing ⚠️
...management/data-recovery/steps/private-key-step.ts	20.51%	31 Missing ⚠️
.../key-management/data-recovery/steps/cipher-step.ts	17.14%	29 Missing ⚠️
...y-management/data-recovery/steps/user-info-step.ts	26.08%	17 Missing ⚠️
...pp/key-management/data-recovery/steps/sync-step.ts	36.84%	12 Missing ⚠️
...anagement/data-recovery/data-recovery.component.ts	97.11%	3 Missing ⚠️
apps/web/src/app/oss-routing.module.ts	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #17673      +/-   ##
==========================================
+ Coverage   41.70%   41.74%   +0.04%     
==========================================
  Files        3571     3580       +9     
  Lines      103769   104208     +439     
  Branches    15613    15670      +57     
==========================================
+ Hits        43281    43506     +225     
- Misses      58653    58863     +210     
- Partials     1835     1839       +4     

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

🚀 New features to boost your workflow:

• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[claude]

Claude finished @quexten's task —— View job

---

Code Review Completed

• Launch Bitwarden code reviewer agent
• Analyze PR changes
• Post review findings

---

Overall Assessment: REQUEST CHANGES

This PR implements a valuable encryption diagnostics and recovery tool with good architecture, but has critical security and error handling issues that must be addressed before merge.

Critical Issues:

• ❌ PII logging violation (User ID)
• ❌ Unsafe non-null assertion without verification
• ⚠️ Missing error context in diagnostics
• ⚠️ Inconsistent error handling between steps
• ⚠️ Zero test coverage for core encryption logic

---

Critical Findings

1. PII Logging Violation ❌

File: apps/web/src/app/key-management/data-recovery/steps/user-info-step.ts:27

logger.record(`User ID: ${userId}`);

Issue: This violates the project rule "NEVER log decrypted data, encryption keys, or PII". User IDs are PII.

Fix:

logger.record("User information retrieved successfully");

---

2. Unsafe Non-Null Assertion ❌

File: apps/web/src/app/key-management/data-recovery/steps/private-key-step.ts:89

await this.privateKeyRegenerationService.regenerateUserPublicKeyEncryptionKeyPair(
  workingData.userId!,  // Non-null assertion without verification
);

Issue: The code uses ! operator but doesn't verify userId exists. If null, this causes a runtime error.

Fix:

async runRecovery(workingData: RecoveryWorkingData, logger: LogRecorder): Promise<void> {
  if (!workingData.userId) {
    logger.record("Cannot recover: missing user ID");
    throw new Error("User ID required for private key recovery");
  }
  
  // ... existing dialog code ...
  
  logger.record("Replacing private key");
  await this.privateKeyRegenerationService.regenerateUserPublicKeyEncryptionKeyPair(
    workingData.userId  // No assertion needed
  );
  logger.record("Private key replaced successfully");
}

---

3. Missing Error Context in Diagnostics ⚠️

Files:

• apps/web/src/app/key-management/data-recovery/steps/private-key-step.ts:41,51
• apps/web/src/app/key-management/data-recovery/steps/cipher-step.ts:30
• apps/web/src/app/key-management/data-recovery/steps/folder-step.ts:39

Issue: Empty catch blocks don't capture error details that would help CS diagnose issues (the stated purpose of this tool).

Fix for private-key-step.ts:

} catch (error) {
  logger.record(`Private key decryption failed: ${error instanceof Error ? error.message : 'Unknown error'}`);
  workingData.isPrivateKeyCorrupt = true;
  return false;
}

Apply similar pattern to all empty catch blocks in cipher-step and folder-step.

---

4. Inconsistent Error Handling ⚠️

File: apps/web/src/app/key-management/data-recovery/steps/folder-step.ts:86-88

} catch (error) {
  logger.record(`Failed to delete folder ${folderId}: ${error}`);
  // Does NOT rethrow - continues to next folder
}

Compare with cipher-step.ts:72-76:

} catch (error) {
  const errorMessage = error instanceof Error ? error.message : String(error);
  logger.record(`Failed to delete cipher ${cipherId}: ${errorMessage}`);
  throw error;  // Rethrows to halt recovery
}

Issue: Folder deletion continues on error while cipher deletion halts. This makes recovery behavior unpredictable and could leave vault in partial state.

Recommendation: Make folder-step match cipher-step (fail-fast) for consistency:

} catch (error) {
  const errorMessage = error instanceof Error ? error.message : String(error);
  logger.record(`Failed to delete folder ${folderId}: ${errorMessage}`);
  throw error;
}

---

5. Test Coverage Gap - Zero Tests for Core Logic ⚠️

Coverage: 54% patch coverage with 127 missing lines

All step implementation files have zero test coverage:

• private-key-step.ts: 18.91% coverage (30 lines missing)
• cipher-step.ts: 17.14% coverage (29 lines missing)
• folder-step.ts: 14.63% coverage (35 lines missing)
• sync-step.ts: 36.84% coverage (12 lines missing)
• user-info-step.ts: 26.08% coverage (17 lines missing)

Required: Each step needs unit tests covering:

• Success paths
• Error scenarios
• Edge cases (null values, empty lists, etc.)
• SDK loading failures
• User cancellations

These tests are critical for code handling encryption keys and user data.

---

Additional Findings

6. Unused Public Method 🎨

File: apps/web/src/app/key-management/data-recovery/steps/folder-step.ts:94-96

getUndecryptableFolderIds(): string[] {
  return this.undecryptableFolderIds;
}

Never called. Remove or document purpose if intended for future use.

---

7. workingData State Management ♻️

File: apps/web/src/app/key-management/data-recovery/data-recovery.component.ts:77,101-108

Technical Debt: The workingData object is reassigned on each run and mutated by steps. Steps store state in instance variables which persist across diagnostic runs. This could cause stale state issues if diagnostics are run multiple times.

Current Behavior:

• Run diagnostics → finds 3 corrupt ciphers → stores in instance variable
• User cancels recovery
• Run diagnostics again → finds 0 corrupt ciphers → but instance variable still has 3 until step executes

Recommendation for future: Consider making steps stateless by returning diagnostic results instead of storing in instance variables.

---

8. SDK Loading Pattern ❓

File: apps/web/src/app/key-management/data-recovery/steps/private-key-step.ts:36

The pattern await SdkLoadService.Ready + PureCrypto direct usage is valid but differs from other code using SdkService via DI. Was this an intentional choice? Just confirming consistency.

---

Positive Aspects ✅

1. Good architecture - Clean step pattern, easily extensible
2. User-friendly - Confirmation dialogs before destructive operations
3. Feature flag protected - Proper feature flag implementation
4. Comprehensive logging - Good diagnostic coverage (once PII fixed)
5. i18n - All strings properly internationalized
6. Signals usage - Appropriate use of Angular Signals
7. Service abstraction - Good reuse of existing services

---

Summary

Must Fix Before Merge:

1. ❌ Remove User ID from logging
2. ❌ Add null check for userId before usage
3. ⚠️ Add error context to all catch blocks
4. ⚠️ Make error handling consistent (folder-step vs cipher-step)

Strongly Recommended:
5. ⚠️ Add unit tests for all step implementations

Consider for Future:
6. Remove unused method or document purpose
7. Refactor workingData state management

The architecture is solid and the tool will be valuable for CS diagnostics, but the critical issues must be resolved to ensure security and reliability.

---

[Thomas-Avery]

Sweet tool and great work. Would like to see more test coverage for example on the steps. A few things to look at from my side.

[quexten]

@Thomas-Avery apologies for the force push, I had a wrong main revision which pulled in some incorrect / unrelated commits here.

[Thomas-Avery]

Looked over the force push. Just the items left unresolved and one missing async action.