[PM-3836] Tools - Make Controllers, Services and API Models nullable by harr1424 · Pull Request #7212 · bitwarden/server

harr1424 · 2026-03-13T15:08:25Z

🎟️ Tracking

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

📔 Objective

Update Tools owned code to make Controllers, Services and API Models nullable

See related PR #7266

github-actions · 2026-03-13T15:19:49Z

Checkmarx One – Scan Summary & Details – c0f3bcca-178d-4e50-82ce-80285454780d

New Issues (2)

Checkmarx found the following issues in this Pull Request

#	Severity	Issue	Source File / Package	Checkmarx Insight
1		CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 1592	details Method at line 1592 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. This parameter value flows ... Attack Vector
2		CSRF	/src/Api/Vault/Controllers/CiphersController.cs: 1419	details Method at line 1419 of /src/Api/Vault/Controllers/CiphersController.cs gets a parameter from a user request from id. This parameter value flows ... Attack Vector

Fixed Issues (1)

Great job! The following issues were fixed in this Pull Request

Severity	Issue	Source File / Package
	~~CSRF~~	/src/Api/Vault/Controllers/CiphersController.cs: 293

codecov · 2026-03-13T15:29:20Z

Codecov Report

❌ Patch coverage is 67.03297% with 30 lines in your changes missing coverage. Please review.
✅ Project coverage is 57.61%. Comparing base (5aae028) to head (f80a89f).
⚠️ Report is 4 commits behind head on main.

Files with missing lines	Patch %	Lines
...ndFeatures/Services/AzureSendFileStorageService.cs	0.00%	12 Missing ⚠️
...s/SendFeatures/Services/LocalSendStorageService.cs	0.00%	6 Missing ⚠️
src/Api/Tools/Models/Request/SendRequestModel.cs	80.00%	1 Missing and 4 partials ⚠️
...c/Api/Tools/Controllers/ImportCiphersController.cs	0.00%	0 Missing and 2 partials ⚠️
...i/Tools/Models/Response/SendAccessResponseModel.cs	84.61%	0 Missing and 2 partials ⚠️
src/Api/Tools/Models/Response/SendResponseModel.cs	87.50%	0 Missing and 2 partials ⚠️
src/Api/Tools/Controllers/SendsController.cs	50.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7212      +/-   ##
==========================================
- Coverage   57.62%   57.61%   -0.01%     
==========================================
  Files        2033     2033              
  Lines       89619    89629      +10     
  Branches     7978     7992      +14     
==========================================
+ Hits        51642    51644       +2     
+ Misses      36117    36114       -3     
- Partials     1860     1871      +11

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

…ntrollers-Services-and-API-Models-nullable merge main

…oadDataResponseModel > nullable

…API-Models-nullable

sonarqubecloud · 2026-03-19T22:29:11Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
14.9% Duplication on New Code

See analysis details on SonarQube Cloud

harr1424 · 2026-03-19T22:28:29Z

src/Api/Tools/Models/Request/Accounts/ImportCiphersRequestModel.cs

 namespace Bit.Api.Tools.Models.Request.Accounts;

 public class ImportCiphersRequestModel
 {


See corresponding model in clients

harr1424 · 2026-03-19T22:29:16Z

src/Api/Tools/Models/Request/Organizations/ImportOrganizationCiphersRequestModel.cs

 namespace Bit.Api.Tools.Models.Request.Organizations;

 public class ImportOrganizationCiphersRequestModel
 {


See corresponding model in clients

harr1424 · 2026-03-19T22:31:49Z

src/Api/Tools/Models/Request/SendRequestModel.cs

-        }, sendAuthorizationService);
-        var data = new SendFileData(Name, Notes, fileName);
+        var send = ToSendBase(new Send { Type = Type, UserId = (Guid?)userId }, sendAuthorizationService);
+        var data = new SendFileData(Name ?? string.Empty, Notes, fileName);


Only the CLI can create or edit Sends having no name, the former behavior serialized and deserialized them as 'null', I've taken this opportunity to instead assign an empty string. Suggestions for a different default are welcome.

I think empty string is fine, definitely an improvement over the former behavior. It does seem weird that the CLI is allowed to do that though, was that a deliberate design choice?

I am not sure about this, I'd like to raise this with Product next time we sync. Typically, we strive for parity among clients. I didn't want to increase the scope of this work by making any changes in clients without raising this to product first; I think the CLI appeals to power-admins and wasn't sure if this would lead to any breaking changes in scrips used for batch generating Sends.

harr1424 · 2026-03-19T22:35:13Z

src/Api/Tools/Models/Request/SendRequestModel.cs

                    break;
                case Core.Tools.Enums.AuthType.Password:
-                    existingSend.Password = authorizationService.HashPassword(Password);
+                    existingSend.Password = authorizationService.HashPassword(Password!);


Clients all enforce that a password protected Send must have a password

harr1424 · 2026-03-19T22:35:14Z

src/Api/Tools/Models/Request/SendRequestModel.cs

        existingSend.Key = Key;
        existingSend.ExpirationDate = ExpirationDate;
-        existingSend.DeletionDate = DeletionDate.Value;
+        existingSend.DeletionDate = DeletionDate!.Value;


This is safe since DeletionDate is required during model binding.

harr1424 · 2026-03-19T22:36:16Z

src/Api/Tools/Models/Response/SendResponseModel.cs

        Type = send.Type;
        AuthType = send.AuthType ?? SendUtilities.InferAuthType(send);
-        Key = send.Key;
+        Key = send.Key!;


See Send propoerty definition for Key, it is guarenteed to be non-null

mcamirault

One question about a possible change and one question for my own curiosity

Looks great overall!

mcamirault · 2026-03-20T19:47:30Z

src/Api/Tools/Models/Request/SendRequestModel.cs

-        }, sendAuthorizationService);
-        var data = new SendFileData(Name, Notes, fileName);
+        var send = ToSendBase(new Send { Type = Type, UserId = (Guid?)userId }, sendAuthorizationService);
+        var data = new SendFileData(Name ?? string.Empty, Notes, fileName);


I think empty string is fine, definitely an improvement over the former behavior. It does seem weird that the CLI is allowed to do that though, was that a deliberate design choice?

mcamirault · 2026-03-20T19:56:42Z

src/Core/Tools/SendFeatures/Services/AzureSendFileStorageService.cs

+        else if (send.OrganizationId.HasValue)
        {
            metadata.Add("organizationId", send.OrganizationId.Value.ToString());
        }


❓ Should we add logic enforcing that the Send has to have one of UserId or OrganizationId in its metadata?

I don't actually think we would gain much from this, but it also wouldn't hurt to be more defensive. I am not sure if this PR is the most appropriate place to make this change, mainly because I've tried to introduce behavioral changes only as strictly necessary to satisfy the compiler which is now enforcing nullability.

The throws introduced in this PR are not so much intended to tighten enforcement but instead replace the existing potential NullReferenceExceptions with exceptions wrapping an error message, they could have almost all been refactored from no-null-handling to using the null-forgiving operator and this would have more closely matched the behavior when #nullable disable was present in these files, but I think the throws both:

indirectly document the expected variable state

surface the same exception types, but with additional context

Right now, OrganizationId isn't actually being used in clients, and UserId is guaranteed to be assigned before the logic in this code is reached:

server/src/Api/Tools/Models/Request/SendRequestModel.cs

Line 125 in f80a89f

var send = new Send { Type = Type, UserId = (Guid?)userId };

server/src/Api/Tools/Models/Request/SendRequestModel.cs

Line 141 in f80a89f

var send = ToSendBase(new Send { Type = Type, UserId = (Guid?)userId }, sendAuthorizationService);

This assumes that _userService.GetProperUserId(User) does return a valid UserId, and if it does not, that case is already handled.

mcamirault · 2026-03-20T19:57:28Z

src/Core/Tools/SendFeatures/Services/AzureSendFileStorageService.cs

                metadata["userId"] = send.UserId.Value.ToString();
            }
-            else
+            else if (send.OrganizationId.HasValue)


❓ Same as above

enable nullable on AzureSendFileStorageService

4400bbd

harr1424 added 8 commits March 13, 2026 19:10

LocalSendStorageService nullable

835aed9

revert primary constructor and begin updating some models

45ef350

Merge remote-tracking branch 'origin/main' into PM-3836-Tools-Make-Co…

d0b0ce0

…ntrollers-Services-and-API-Models-nullable merge main

SendRequestModel > nullable

abe9d60

SendAccessResponseModel SendFileDownloadDataResponseModel SendFileUpl…

da8043d

…oadDataResponseModel > nullable

SendResponseModel SendFileModel SendTextModel > nullable

3dbc9fd

Merge branch 'main' into PM-3836-Tools-Make-Controllers-Services-and-…

a783f60

…API-Models-nullable

ImportCiphersController > nullable

f80a89f

harr1424 commented Mar 19, 2026

View reviewed changes

harr1424 marked this pull request as ready for review March 19, 2026 22:39

harr1424 requested a review from a team as a code owner March 19, 2026 22:39

harr1424 mentioned this pull request Mar 20, 2026

[PM-4128] Remove nullability of Send.Data and Send.Keys #7266

Open

harr1424 added the needs-qa label Mar 20, 2026

mcamirault reviewed Mar 20, 2026

View reviewed changes

harr1424 requested a review from mcamirault March 20, 2026 23:13

Conversation

harr1424 commented Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🎟️ Tracking

📔 Objective

Uh oh!

github-actions bot commented Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sonarqubecloud bot commented Mar 19, 2026

Quality Gate passed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcamirault left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

harr1424 commented Mar 13, 2026 •

edited

Loading

github-actions bot commented Mar 13, 2026 •

edited

Loading

codecov bot commented Mar 13, 2026 •

edited

Loading