Resolve follow-up comments

[quexten]

🎟️ Tracking

📔 Objective

Resolve: #592 (review)

🚨 Breaking Changes

⏰ 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

[claude]

Claude finished @quexten's task —— View job

---

Code Review Complete

Summary

This PR successfully addresses the follow-up comment from PR #592 review by @Hinton, who requested adding disclaimers to RSA functions about using AsymmEncString abstractions.

---

Changes Analysis

1. HAZMAT Warnings Added ✅

The PR adds appropriate warning comments to all 4 RSA-related functions in crates/bitwarden-wasm-internal/src/pure_crypto.rs:

• rsa_extract_public_key (line 329)
• rsa_generate_keypair (line 341)
• rsa_decrypt_data (line 351)
• rsa_encrypt_data (line 365)

Each now includes:

/// HAZMAT WARNING: Do not use outside of implementing cryptofunctionservice

This directly addresses Hinton's concern about guiding developers toward better abstractions.

2. Workspace Dependency Consolidation ✅

Moved rand, rsa, and sha1 to workspace-level dependencies:

• Root Cargo.toml: Added 3 dependencies to [workspace.dependencies]
• bitwarden-crypto/Cargo.toml: Changed 3 inline versions to { workspace = true }
• bitwarden-wasm-internal/Cargo.toml: Changed 3 inline versions to { workspace = true }

This improves consistency and maintainability across the workspace.

---

Findings

Finding 1: Warning message could be more specific

💭 The HAZMAT warning says "Do not use outside of implementing cryptofunctionservice" but could be more actionable:

Suggested improvement (optional)

/// HAZMAT WARNING: Do not use outside of implementing cryptofunctionservice.
/// For SDK usage, prefer AsymmEncString abstractions for better security and memory protection.

This explicitly mentions the alternative Hinton referenced. However, given these functions are already in the temporary PureCrypto module (which itself has a large disclaimer at lines 20-23), the current warning is acceptable.

Finding 2: All RSA functions properly tagged

👍 The PR correctly identified and tagged all 4 RSA-related functions. The warning placement is consistent and appears in the public API documentation.

---

PR Quality Assessment

Title: ✅ "Resolve follow-up comments" - Clear and concise
Objective: ⚠️ Links to the review but doesn't explain what follow-up comments or how they were resolved

Suggested PR description improvement

The Objective section could be more descriptive:

## 📔 Objective

Resolves follow-up from PR #592 by:
1. Adding HAZMAT warnings to RSA functions per @Hinton's request to discourage use of raw RSA functions in favor of AsymmEncString abstractions
2. Consolidating rand, rsa, and sha1 to workspace dependencies for consistency

Resolve: https://github.com/bitwarden/sdk-internal/pull/592#pullrequestreview-3524083207

This helps reviewers immediately understand what changed without clicking through to the previous PR.

---

Good Practices Observed

1. Complete coverage: All 4 RSA functions have warnings
2. Workspace consistency: Properly uses { workspace = true } pattern
3. Version preservation: Workspace versions match previous inline versions exactly
4. Clean commit history: Single focused commit

---

Final Assessment

This PR successfully addresses the follow-up request from PR #592. The HAZMAT warnings guide developers away from using these low-level functions directly, while the workspace dependency consolidation improves maintainability.

Recommendation: ✅ Approve

The PR description could be more detailed (Finding 2 in PR Quality section), but the code changes are correct and complete.

---

[github-actions]

Checkmarx One – Scan Summary & Details – 3afbc51a-3743-459e-a973-457b4777ab05

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

[github-actions]

🔍 SDK Breaking Change Detection Results

SDK Version: km/follow-up (65976af)
Completed: 2025-12-02 12:04:39 UTC
Total Time: 249s

Client	Status	Details
typescript	✅ No breaking changes detected	TypeScript compilation passed with new SDK version - View Details

---

Breaking change detection completed. View SDK workflow

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.52%. Comparing base (0107af7) to head (65976af).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #595   +/-   ##
=======================================
  Coverage   79.52%   79.52%           
=======================================
  Files         302      302           
  Lines       32362    32366    +4     
=======================================
+ Hits        25735    25739    +4     
  Misses       6627     6627           

☔ 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.