Skip to content

[PM-40237] Require cipher Name only when Data is absent - #8252

Merged
quexten merged 3 commits into
mainfrom
vault/pm-41447-optional-cipher-name
Aug 25, 2026
Merged

[PM-40237] Require cipher Name only when Data is absent#8252
quexten merged 3 commits into
mainfrom
vault/pm-41447-optional-cipher-name

Conversation

@quexten

@quexten quexten commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

🎟️ Tracking

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

SDK counterpart: bitwarden/sdk-internal#1408

📔 Objective

Blob-encrypted ciphers carry all their content in Data, so Name goes unused. The unconditional [Required] attribute on CipherRequestModel.Name forced clients (and the SDK) to send a placeholder encrypted empty string.

Name is now validated via IValidatableObject, keyed off Cipher.IsDataBlobEncrypted(): required for every format except blob encryption. Note Data is populated for non-blob ciphers too. No change for existing clients.

Blob-encrypted ciphers carry all content in Data, so Name goes unused.
The unconditional [Required] attribute forced clients to send a
placeholder encrypted empty string. Move the check into
IValidatableObject and key it off Cipher.IsDataBlobEncrypted, since
Data is also populated for non-blob ciphers.
@quexten
quexten force-pushed the vault/pm-41447-optional-cipher-name branch from 330a4dc to a73cd57 Compare August 25, 2026 06:16
@quexten quexten changed the title [PM-41447] Require cipher Name only when Data is absent [PM-40237] Require cipher Name only when Data is absent Aug 25, 2026
@quexten
quexten requested review from shane-melton and removed request for jaasen-livefront August 25, 2026 06:18
@quexten quexten added the t:feature Change Type - Feature Development label Aug 25, 2026
@quexten
quexten marked this pull request as ready for review August 25, 2026 06:20
@github-actions

github-actions Bot commented Aug 25, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

This PR drops the unconditional [Required] on CipherRequestModel.Name and replaces it with an IValidatableObject.Validate implementation keyed off Cipher.IsDataBlobEncrypted(), so blob-encrypted ciphers no longer need a placeholder encrypted name. The relaxation is narrow and backwards compatible: string.IsNullOrWhiteSpace matches RequiredAttribute semantics for strings, EncryptedStringAttribute already treats null as valid so no property-level validator masks the new rule, nested CipherRequestModel instances in CipherCreateRequestModel/CipherShareRequestModel/CipherBulkShareRequestModel and the import array models are still reached by the MVC validation visitor, and the ToCipher fallback that consumes Name only runs when Data is absent — a case validation still requires a Name for. Blob-format Data is already accepted unconditionally elsewhere in the API (CipherResponseModel, ValidateClientVersionForFido2CredentialSupport), so this introduces no new surface, and the four added unit tests cover the blob, non-blob, neither, and name-only permutations.

Code Review Details

No findings. The XML doc comment defect flagged in the previous review round (dangling </summary> on IsOrganizationCipher) is resolved in a73cd576f/fixup — the <summary> tags in src/Api/Vault/Models/Request/CipherRequestModel.cs are now balanced and the Api build will not trip CS1570.

Comment thread src/Api/Vault/Models/Request/CipherRequestModel.cs
@quexten
quexten marked this pull request as draft August 25, 2026 06:41
@quexten
quexten marked this pull request as ready for review August 25, 2026 06:46
@quexten
quexten enabled auto-merge (squash) August 25, 2026 06:48
@codecov

codecov Bot commented Aug 25, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 63.21%. Comparing base (aebb9c4) to head (e431d61).

❗ There is a different number of reports uploaded between BASE (aebb9c4) and HEAD (e431d61). Click for more details.

HEAD has 1 upload less than BASE
Flag BASE (aebb9c4) HEAD (e431d61)
2 1
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8252      +/-   ##
==========================================
- Coverage   68.88%   63.21%   -5.68%     
==========================================
  Files        2410     2410              
  Lines      104442   104450       +8     
  Branches     9457     9458       +1     
==========================================
- Hits        71946    66026    -5920     
- Misses      30115    36165    +6050     
+ Partials     2381     2259     -122     

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

@quexten
quexten merged commit d5a350e into main Aug 25, 2026
46 checks passed
@quexten
quexten deleted the vault/pm-41447-optional-cipher-name branch August 25, 2026 16:44
quexten added a commit to bitwarden/sdk-internal that referenced this pull request Aug 26, 2026
## 🎟️ Tracking

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

Server counterpart: bitwarden/server#8252

## 📔 Objective

Blob-encrypted ciphers keep all content in the blob, but the server
required a non-empty `Name` on cipher requests, so
`encrypt_blob_cipher_with_wrapping_key` emitted an encrypted empty
string as a placeholder.

The server now requires `Name` only for non-blob ciphers, so send `None`
and drop the TODOs.

Depends on the server change being deployed first.

## 🚨 Breaking Changes

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

Labels

t:feature Change Type - Feature Development

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants