[PM-39979] Add full API support for Item-type Sends - #8192
Conversation
🤖 Bitwarden Claude Code ReviewOverall Assessment: REQUEST CHANGES Reviewed the full API support for Item-type Sends: the structured Code Review Details
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if (send.Type == SendType.Item) | ||
| { | ||
| return AuthType.Email; | ||
| } |
There was a problem hiding this comment.
❌ CRITICAL: Item Sends created without an explicit AuthType end up accessible with no verification while reporting AuthType.Email
Details and fix
SendRequestModel.AuthType is optional, and the new "Item Sends require email verification" guard in ToSendBase only runs inside if (AuthType != null). When a request omits AuthType, the legacy else branch runs and, with no Emails/Password supplied, leaves send.Emails == null and then sets send.AuthType = InferAuthType(send) — which now returns Email for Item Sends.
SendAuthenticationQuery.GetAuthenticationMethod then evaluates:
var s when s.AuthType == AuthType.Email && s.Emails is not null => EmailOtp(s.Emails), // false: Emails is null
var s when s.AuthType == AuthType.Password && s.Password is not null => ..., // false
_ => NOT_AUTHENTICATEDNotAuthenticated causes SendAccessGrantValidator to auto-issue a send access token, so anyone with the link accesses the Item Send without email OTP — while SendResponseModel.AuthType and SendAccessResponseModel.AuthType both report Email.
Note Emails = "" (explicit AuthType.Email with an empty list) is safe because it yields EmailOtp([]), which denies everyone. The dangerous state is specifically Emails == null.
Suggested fix: enforce the Item requirement independently of whether the client sent AuthType (e.g. validate in UpdateSend/ToSendItemData that the resulting Send has AuthType.Email and a non-empty Emails list), and only infer Email here when send.Emails is actually populated. The same invariant is also bypassable via PUT /sends/{id}/remove-auth, which sets AuthType = None with no type check.
| if (model.Type == SendType.Item && !_featureService.IsEnabled(FeatureFlagKeys.TemporaryItemSharing)) | ||
| { | ||
| throw new BadRequestException("Item type Sends are not yet enabled"); | ||
| } | ||
| var userId = _userService.GetProperUserId(User) ?? throw new InvalidOperationException("User ID not found"); | ||
| var hasPremium = await _hasPremiumAccessQuery.HasPremiumAccessAsync(userId); | ||
|
|
||
| if (!hasPremium && model.Type == SendType.Item) | ||
| { | ||
| throw new BadRequestException("Item type Sends require a premium membership"); | ||
| } |
There was a problem hiding this comment.
Put gates on the client-supplied model.Type, but UpdateSend branches on the stored existingSend.Type
Details and fix
A client editing an existing Item Send can send Type: Text in the body and skip all three Item guards:
- the feature-flag check here,
- the premium check below,
- the
Type == SendType.Item && AuthType != Emailguard inSendRequestModel.ToSendBase.
UpdateSend switches on existingSend.Type, so the Send is still written through the Item branch — meaning an Item Send can be edited while the flag is off, by a non-premium user, and can be given AuthType.Password despite the new email-verification requirement. NonAnonymousSendCommand.SaveSendAsync performs no type check either.
Fix: load the Send first and evaluate the Item guards against send.Type (or reject when model.Type != send.Type):
var send = await _sendRepository.GetByIdAsync(new Guid(id));
if (send == null || send.UserId != userId) { throw new NotFoundException(); }
if (send.Type == SendType.Item || model.Type == SendType.Item) { /* flag + premium checks */ }
| var responses = sends.Select(s => new SendResponseModel(s)); | ||
| if (!_featureService.IsEnabled(FeatureFlagKeys.TemporaryItemSharing)) | ||
| { | ||
| responses = responses.Where(s => s.Type != SendType.Item); | ||
| } |
There was a problem hiding this comment.
Details and fix
Select is lazy, so the Where predicate forces the SendResponseModel constructor to run for every Item Send even when the kill switch is off. The constructor now does JsonSerializer.Deserialize<SendItemData>(send.Data) and throws JsonException on anything that is not SendItemData JSON.
Item Sends persisted by the previous implementation stored send.Data as the raw client string (existingSend.Data = Data), and this PR adds no migration for them. Such a row makes GET /sends fail entirely for that user — and turning the flag off does not mitigate it.
Filter the entities before projecting:
var sends = await _sendOwnerQuery.GetOwned(User);
if (!_featureService.IsEnabled(FeatureFlagKeys.TemporaryItemSharing))
{
sends = sends.Where(s => s.Type != SendType.Item);
}
var result = new ListResponseModel<SendResponseModel>(sends.Select(s => new SendResponseModel(s)));Worth confirming separately whether any Item Sends already exist in the old format in cloud/QA data.
1 participant
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-39979
📔 Objective
This PR adds the full backend logic necessary to create and edit Item-type Sends.