Update CoreClient's BwHttpClient to include new default headers

[JaredSnider-Bitwarden]

🎟️ Tracking

n/a but required for #549

📔 Objective

Adds the following new optional ClientSettings:

1. device_identifier
2. bitwarden_package_type
   In order to achieve feature parity with the web clients ApiService. Note: eventually device_identifier and bitwarden_client_version should be required.

Added the following as new default headers:

1. Device-Identifier
2. Bitwarden-Package-Type
3. reqwest::header::USER_AGENT

Worked with @dani-garcia to add proper clean up tickets as well.

⏰ 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 – 4ff9dd58-7654-4945-a889-54226a494681

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

[claude]

Claude finished @JaredSnider-Bitwarden's task in 2m 38s —— View job

---

Code Review Complete

• Starting code review
• Analyzing changes
• Posting review comments

Review Summary

I've reviewed PR #621 which adds device_identifier and bitwarden_package_type as optional ClientSettings and includes them as default HTTP headers. I found critical security issues related to header validation that must be addressed before merge.

Critical Issues Found:

1. Security/DoS Vulnerability (lines 129, 145, 152, 169): The code uses .expect() when converting user-provided strings to HTTP header values, creating a panic/crash vector
2. Missing Test Coverage: No tests for the new header building logic
3. Missing Test Plan: PR description doesn't describe how changes were tested

Detailed Findings:

Finding 1: Panic on Invalid Header Values (CRITICAL)

Location: crates/bitwarden-core/src/client/client.rs:129, 145, 152, 169

Issue: The build_default_headers function uses .expect() when converting user-provided strings to HTTP headers. This will panic and crash the SDK if users provide invalid input such as strings containing newlines (\n), null bytes (\0), certain control characters, or invalid UTF-8 sequences.

Security Impact: This is a denial-of-service vulnerability. A malicious actor or buggy client code can crash the SDK by providing malformed header values.

Fix Required: Replace .expect() with proper error handling - return Result<header::HeaderMap, Error> and propagate errors through the call chain.

---

Finding 2: Missing Validation for Security-Sensitive Headers (IMPORTANT)

Location: crates/bitwarden-core/src/client/client_settings.rs:44, 49

Issue: The new fields accept arbitrary String values without format validation. Should device_identifier be a UUID? Should bitwarden_package_type be an enum of allowed values?

Recommendation: Consider adding typed validation or at least document the expected format.

---

Finding 3: Missing Test Coverage (IMPORTANT)

Issue: The new build_default_headers function has no unit tests.

Required Tests:

1. Valid headers are set correctly
2. Optional headers are omitted when None
3. Invalid input handling (newlines, null bytes, non-ASCII) - currently would panic
4. USER_AGENT header is always set
5. Device-Type header is always set with correct numeric value

---

Finding 4: PR Description Missing Test Plan (IMPORTANT)

Issue: The PR checklist mentions "Written new unit and / or integration tests where applicable" but no tests were added and no test plan is documented.

Required: Please document how you tested these changes and what verification steps were taken.

---

Finding 5: Inconsistent Error Handling Pattern (TECHNICAL DEBT)

Location: crates/bitwarden-core/src/client/client.rs:44-46, 50-53

Issue: The code uses .expect() for client building operations. While unlikely to fail, this is inconsistent with Rust error handling best practices for a library crate.

Recommendation: Consider returning Result from Client::new() constructors.

---

Finding 6: API Compatibility - UniFFI/WASM Considerations (QUESTION)

Have you manually verified:

1. Mobile clients (Swift/Kotlin) can properly handle the new optional fields?
2. WASM bindings correctly serialize/deserialize with camelCase naming?
3. TypeScript types are correctly generated?

The breaking change detection passed, which is good, but manual verification is recommended.

---

Recommendations Summary

Must Fix Before Merge:

1. Replace all .expect() calls in header building with proper Result error handling
2. Add comprehensive unit tests for header building, especially error cases
3. Document test plan in PR description

Should Consider:
4. Add format validation for device_identifier and bitwarden_package_type
5. Return Result from Client::new() constructors for better error handling

Please Verify:
6. Manual testing with invalid header values
7. UniFFI/WASM binding compatibility

---

[github-actions]

🔍 SDK Breaking Change Detection Results

SDK Version: http-client-add-bw-package-type-to-default-headers (8314880)
Completed: 2025-12-18 17:34:07 UTC
Total Time: 246s

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

❌ Patch coverage is 69.69697% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.52%. Comparing base (0a9599e) to head (8314880).

Files with missing lines	Patch %	Lines
crates/bitwarden-core/src/client/client.rs	65.45%	19 Missing ⚠️
crates/bitwarden-core/src/client/internal.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #621      +/-   ##
==========================================
- Coverage   78.54%   78.52%   -0.02%     
==========================================
  Files         283      283              
  Lines       29251    29286      +35     
==========================================
+ Hits        22975    22997      +22     
- Misses       6276     6289      +13     

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