Skip to content

[PM-39978] Add Item type Send support#8008

Open
mcamirault wants to merge 7 commits into
mainfrom
tools/pm-39978/support-item-type-sends
Open

[PM-39978] Add Item type Send support#8008
mcamirault wants to merge 7 commits into
mainfrom
tools/pm-39978/support-item-type-sends

Conversation

@mcamirault

Copy link
Copy Markdown
Contributor

🎟️ Tracking

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

📔 Objective

This PR is the first step in the Temporary Item Sharing effort. This PR allows the server to accept Sends of type Item by adding a new value to the SendType enum and handling it in associated Send code. Since these new Item Sends will be blob-encrypted instead of field by field we also add a new Data property to the Send request model that allows clients to pass the entire encrypted payload (this field will also be usable when we eventually migrate Text and File Sends to do the same). Because the field is encrypted we can't do any validation of its structure as the ticket originally stated; we'll have to rely on clients to do this validation instead.

@mcamirault
mcamirault requested a review from a team as a code owner July 17, 2026 18:21
@mcamirault mcamirault added ai-review Request a Claude code review t:feature Change Type - Feature Development labels Jul 17, 2026
@github-actions

github-actions Bot commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the addition of the Item SendType and the new blob Data field on the Send request/response models. The zero-knowledge invariant is preserved: Data is an opaque client-encrypted blob that the server stores and returns without inspection, and the [StringLength(500000)] limit fits the existing VARCHAR(MAX) column (no schema change required). The request/response model refactors correctly assign Name/Notes in every switch branch, the access-count increment now includes Item, and the new unit tests cover the Item success, null-data, and non-deserializable paths.

Code Review Details
  • ❓ : Send event logging miscategorizes Item Sends as File
    • src/Core/Tools/SendFeatures/Commands/NonAnonymousSendCommand.cs:149, :99, :116
    • ResolveSendCreatedEventType, LogSendUpdatedEventAsync, and LogSendDeletedEventAsync fall through their else/_ branch to Send_*_File for any non-Text type. With SendEventLogging enabled, creating/editing/deleting an Item Send emits File audit events. No Send_*_Item EventType values exist yet, so this may be intentionally deferred to a later PR in the Temporary Item Sharing effort — is Item event logging planned separately? (These lines are outside the diff, so this is noted here rather than inline.)

@codecov

codecov Bot commented Jul 20, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 68.42105% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 62.51%. Comparing base (a93cf75) to head (d63cef3).
⚠️ Report is 24 commits behind head on main.

Files with missing lines Patch % Lines
...i/Tools/Models/Response/SendAccessResponseModel.cs 50.00% 2 Missing and 1 partial ⚠️
src/Api/Tools/Models/Response/SendResponseModel.cs 66.66% 2 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8008      +/-   ##
==========================================
+ Coverage   62.23%   62.51%   +0.28%     
==========================================
  Files        2287     2298      +11     
  Lines       99665   100050     +385     
  Branches     9004     9026      +22     
==========================================
+ Hits        62024    62550     +526     
+ Misses      35470    35319     -151     
- Partials     2171     2181      +10     

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

@harr1424 harr1424 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor suggestions and outstanding question on unexpectedly large EncryptedStringLength

Comment thread src/Api/Tools/Models/Request/SendRequestModel.cs Outdated
Comment thread src/Api/Tools/Models/Response/SendAccessResponseModel.cs Outdated
Comment thread src/Api/Tools/Models/Response/SendResponseModel.cs Outdated
Comment thread src/Api/Tools/Models/Response/SendResponseModel.cs Outdated
Comment thread src/Core/Tools/Enums/SendType.cs
@harr1424
harr1424 self-requested a review July 21, 2026 13:29
harr1424
harr1424 previously approved these changes Jul 21, 2026

@harr1424 harr1424 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good but I believe a doc comment is out of date

Comment thread src/Api/Tools/Models/Request/SendRequestModel.cs Outdated
Co-authored-by: John Harrington <84741727+harr1424@users.noreply.github.com>
@mcamirault
mcamirault requested a review from harr1424 July 23, 2026 14:58
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 needs-qa t:feature Change Type - Feature Development

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants