Skip to content

[PM-40047] refactor: detect blob ciphers via format_version probe#7930

Draft
shane-melton wants to merge 1 commit into
mainfrom
vault/pm-40047/cipher-blob-json-refactor
Draft

[PM-40047] refactor: detect blob ciphers via format_version probe#7930
shane-melton wants to merge 1 commit into
mainfrom
vault/pm-40047/cipher-blob-json-refactor

Conversation

@shane-melton

Copy link
Copy Markdown
Member

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-40047

📔 Objective

The SDK now stores blob-encrypted cipher payloads as JSON, which starts with { and collides with the leading-byte heuristic previously used to tell blob from legacy field-level CipherData. This replaces that heuristic in Cipher.IsDataBlobEncrypted() with a top-level format_version property probe (via Utf8JsonReader, so large payloads aren't fully materialized), matching the SDK. Tests updated. Paired with the sdk-internal change for PM-40047.

@shane-melton shane-melton added the ai-review Request a Claude code review label Jul 6, 2026
@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the refactor of Cipher.IsDataBlobEncrypted() from a leading-byte heuristic to a top-level format_version property probe using Utf8JsonReader. Traced both callers (CipherResponseModel and CiphersController.ValidateClientVersionForFido2CredentialSupport) — they correctly skip legacy field-level deserialization when data is blob-encrypted, so the new classification integrates cleanly. The probe correctly handles leading whitespace, ignores nested format_version keys via reader.Skip(), and swallows JsonException on malformed input. Test coverage is thorough, including the bare-string "format_version" and {"other_key":1} negative cases.

Code Review Details

No blocking findings.

Notes considered and not raised as findings:

  • The reclassification of opaque non-JSON strings (e.g., 2.iv|ct|mac) from blob-encrypted to legacy is an intentional, coordinated change. The PR is explicitly paired with the sdk-internal PM-40047 change where the SDK now writes blob payloads as {"format_version": ...} JSON, so this is deliberate design rather than a regression.
  • The comment states large payloads "aren't fully materialized." Utf8JsonReader avoids materializing the parsed document graph, though Encoding.UTF8.GetBytes(Data) still allocates the raw byte buffer. This is still cheaper than full deserialization and not a concern.

@codecov

codecov Bot commented Jul 6, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 16 lines in your changes missing coverage. Please review.
✅ Project coverage is 14.90%. Comparing base (f0f04eb) to head (024f0a4).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/Core/Vault/Entities/Cipher.cs 0.00% 16 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #7930       +/-   ##
===========================================
- Coverage   61.34%   14.90%   -46.44%     
===========================================
  Files        2244     1374      -870     
  Lines       98673    59652    -39021     
  Branches     8912     4725     -4187     
===========================================
- Hits        60531     8894    -51637     
- Misses      36006    50606    +14600     
+ Partials     2136      152     -1984     

☔ View full report in Codecov by Harness.
📢 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Request a Claude code review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant