PM-31920 adding the whole report endpoints v2#7079
PM-31920 adding the whole report endpoints v2#7079prograhamming wants to merge 27 commits intomainfrom
Conversation
|
New Issues (2)Checkmarx found the following issues in this Pull Request
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
…-access-intelligence
…-access-intelligence
…elligence' of github.com:bitwarden/server into dirt/pm-31920-whole-report-data-v2-endpoints-access-intelligence
…-access-intelligence
…elligence' of github.com:bitwarden/server into dirt/pm-31920-whole-report-data-v2-endpoints-access-intelligence
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #7079 +/- ##
===========================================
- Coverage 56.88% 13.87% -43.02%
===========================================
Files 2028 1248 -780
Lines 88826 53222 -35604
Branches 7918 4113 -3805
===========================================
- Hits 50528 7384 -43144
- Misses 36468 45706 +9238
+ Partials 1830 132 -1698 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
…-access-intelligence
…-access-intelligence
|
@claude review this pr |
|
Claude finished @prograhamming's task in 3m 15s —— View job Code Review: PM-31920 V2 Report Endpoints
I reviewed all 39 changed files in this PR. Many issues from previous review rounds have been addressed (null checks, path traversal guards, typos, Security1. 2. Bugs / Correctness3. 4. V1 Design5. 6. V2 update Test Coverage7. No controller tests for 8. SummaryThe PR is in good shape overall. The path traversal guard, null checks, FileSize on update, and old file cleanup have all been addressed from previous review rounds. The remaining items are mostly low-severity design/naming concerns and test coverage gaps. The most actionable items are:
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| if (request.RequiresNewFileUpload) | ||
| { | ||
| var fileData = new ReportFile | ||
| { | ||
| Id = CoreHelpers.SecureRandomString(32, upper: false, special: false), | ||
| FileName = "report-data.json", | ||
| Validated = false, | ||
| Size = 0 | ||
| }; | ||
| existingReport.SetReportFile(fileData); | ||
| } |
There was a problem hiding this comment.
Question: When RequiresNewFileUpload is true, the new ReportFile is created with Size = 0. But the controller's UploadReportFileAsync (line 349) uses a 1MB leeway around fileData.Size for validation: Math.Max(0, fileData.Size - leeway) which for Size = 0 means minimum = 0.
For CREATE this works because AddOrganizationReportRequest.FileSize is used. But for UPDATE, the UpdateOrganizationReportV2Request has no FileSize property, so the size is always 0 and the leeway check effectively becomes 0 <= length <= 501MB. This means no meaningful file size validation occurs for update uploads.
Should UpdateOrganizationReportV2Request also carry a FileSize field when RequiresNewFileUpload is true, so the upload validation is meaningful?
| var blobProperties = await blobClient.GetPropertiesAsync(); | ||
| var metadata = blobProperties.Value.Metadata; | ||
| metadata["organizationId"] = report.OrganizationId.ToString(); | ||
| await blobClient.SetMetadataAsync(metadata); | ||
|
|
||
| var headers = new BlobHttpHeaders | ||
| { | ||
| ContentDisposition = $"attachment; filename=\"{fileData.FileName}\"" | ||
| }; | ||
| await blobClient.SetHttpHeadersAsync(headers); | ||
|
|
There was a problem hiding this comment.
Side effect in validation: ValidateFileAsync not only validates the blob but also sets metadata (organizationId) and HTTP headers (Content-Disposition) on it. This is unexpected for a method named "Validate". Consider either:
- Renaming to something like
ValidateAndFinalizeFileAsyncto clarify the side effects, or - Extracting the metadata/header setting into a separate method called during finalization
src/Core/Dirt/Reports/Services/NoopOrganizationReportStorageService.cs
Outdated
Show resolved
Hide resolved
| @@ -32,6 +35,7 @@ public OrganizationReportResponseModel(OrganizationReport organizationReport) | |||
| PasswordCount = organizationReport.PasswordCount; | |||
| PasswordAtRiskCount = organizationReport.PasswordAtRiskCount; | |||
| MemberCount = organizationReport.MemberCount; | |||
| ReportFile = organizationReport.GetReportFile(); | |||
| CreationDate = organizationReport.CreationDate; | |||
| RevisionDate = organizationReport.RevisionDate; | |||
| } | |||
There was a problem hiding this comment.
Nit: The response model only maps a subset of the entity's metric fields (PasswordCount, PasswordAtRiskCount, MemberCount). The entity has 12 metric fields (application, member, password, each with total/atRisk/critical/criticalAtRisk). If the client needs these metrics, the response model should map all of them. If they're intentionally excluded from the API response, a comment explaining why would help future maintainers.
…-access-intelligence
| var report = await _getOrganizationReportQuery.GetOrganizationReportAsync(reportId); | ||
| if (report.OrganizationId != organizationId) |
There was a problem hiding this comment.
Bug (High): Same null dereference pattern as already flagged in GetOrganizationReportAsync at line 191. If GetOrganizationReportAsync(reportId) returns null, accessing report.OrganizationId on line 333 will throw a NullReferenceException.
| var report = await _getOrganizationReportQuery.GetOrganizationReportAsync(reportId); | |
| if (report.OrganizationId != organizationId) | |
| var report = await _getOrganizationReportQuery.GetOrganizationReportAsync(reportId); | |
| if (report == null) | |
| { | |
| throw new NotFoundException(); | |
| } | |
| if (report.OrganizationId != organizationId) |
test/Core.Test/Dirt/Reports/Services/LocalOrganizationReportStorageServiceTests.cs
Outdated
Show resolved
Hide resolved
| public Task DeleteReportFilesAsync(OrganizationReport report, string reportFileId) | ||
| { | ||
| var dirPath = Path.Combine(_baseDirPath, report.OrganizationId.ToString(), | ||
| report.CreationDate.ToString("MM-dd-yyyy"), report.Id.ToString(), reportFileId); | ||
| if (Directory.Exists(dirPath)) | ||
| { | ||
| Directory.Delete(dirPath, true); | ||
| } | ||
| return Task.CompletedTask; | ||
| } |
There was a problem hiding this comment.
Security (Medium): The EnsurePathWithinBaseDir guard was correctly added to WriteFileAsync and ValidateFileAsync, but DeleteReportFilesAsync still constructs a path from reportFileId via Path.Combine and passes it to Directory.Delete(dirPath, true) without the guard. For consistency with the defense-in-depth approach applied to the other methods, add the same check here:
| public Task DeleteReportFilesAsync(OrganizationReport report, string reportFileId) | |
| { | |
| var dirPath = Path.Combine(_baseDirPath, report.OrganizationId.ToString(), | |
| report.CreationDate.ToString("MM-dd-yyyy"), report.Id.ToString(), reportFileId); | |
| if (Directory.Exists(dirPath)) | |
| { | |
| Directory.Delete(dirPath, true); | |
| } | |
| return Task.CompletedTask; | |
| } | |
| public Task DeleteReportFilesAsync(OrganizationReport report, string reportFileId) | |
| { | |
| var dirPath = Path.Combine(_baseDirPath, report.OrganizationId.ToString(), | |
| report.CreationDate.ToString("MM-dd-yyyy"), report.Id.ToString(), reportFileId); | |
| EnsurePathWithinBaseDir(dirPath); | |
| if (Directory.Exists(dirPath)) | |
| { | |
| Directory.Delete(dirPath, true); | |
| } | |
| return Task.CompletedTask; | |
| } |
| Id = CoreHelpers.SecureRandomString(32, upper: false, special: false), | ||
| FileName = "report-data.json", | ||
| Validated = false, | ||
| Size = 0 |
There was a problem hiding this comment.
Size = 0 here means the self-hosted UploadReportFileAsync controller method will compute a validation range of [0, 1MB] (since minimum = Max(0, 0 - 1MB) = 0 and maximum = Min(0 + 1MB, 501MB) = 1MB). Any file larger than 1MB uploaded through the update path on self-hosted instances will fail validation with "File received does not match expected constraints."
The create path works correctly because AddOrganizationReportRequest includes a FileSize property that the client provides, but UpdateOrganizationReportV2Request has no equivalent field.
To fix, add a FileSize property to UpdateOrganizationReportV2Request, validate it in the controller (similar to the create path at lines 143-151 of the controller), and use it here:
| Size = 0 | |
| Size = request.FileSize ?? 0 |
This was flagged in a previous review and remains unfixed.
| Id = CoreHelpers.SecureRandomString(32, upper: false, special: false), | ||
| FileName = "report-data.json", | ||
| Size = request.FileSize ?? 0, | ||
| Validated = false |
There was a problem hiding this comment.
UpdateOrganizationReportV2Command correctly set Validated = false for new files. However, the ReportFile class (src/Core/Dirt/Models/Data/ReportFile.cs line 29) declares the default as public bool Validated { get; set; } = true;.
This is a fail-open default for a security-sensitive field. If System.Text.Json deserialization ever produces a ReportFile instance without an explicit Validated value (corrupted JSON, a field missing from the serialized data, etc.), the file would be treated as validated, and the controller would generate a download URL for a potentially unverified file.
The safe default should be false (fail-closed). Since both creation paths already set Validated = false explicitly, and the validation paths set it to true explicitly, changing the default in ReportFile.cs to false will not affect normal code flow but will protect against edge cases.
| if (request.RequiresNewFileUpload) | ||
| { | ||
| var fileData = new ReportFile | ||
| { | ||
| Id = CoreHelpers.SecureRandomString(32, upper: false, special: false), | ||
| FileName = "report-data.json", | ||
| Validated = false, | ||
| Size = 0 | ||
| }; | ||
| existingReport.SetReportFile(fileData); | ||
| } |
There was a problem hiding this comment.
Bug (Medium): When RequiresNewFileUpload is true, the old file blob/directory is never cleaned up. The existing ReportFile (with its old Id) is overwritten by SetReportFile(fileData) with a new ID, but no call is made to _storageService.DeleteReportFilesAsync() to remove the previous file from storage.
This will cause orphaned blobs/files to accumulate over time. The command doesn't inject IOrganizationReportStorageService, so it can't perform cleanup.
Consider either:
- Injecting
IOrganizationReportStorageServiceand deleting the old file before replacing, or - Documenting that cleanup happens elsewhere (e.g., a background job)
| if (request.RequiresNewFileUpload) | |
| { | |
| var fileData = new ReportFile | |
| { | |
| Id = CoreHelpers.SecureRandomString(32, upper: false, special: false), | |
| FileName = "report-data.json", | |
| Validated = false, | |
| Size = 0 | |
| }; | |
| existingReport.SetReportFile(fileData); | |
| } | |
| if (request.RequiresNewFileUpload) | |
| { | |
| // TODO: Clean up old file from storage before replacing | |
| // var oldFileData = existingReport.GetReportFile(); | |
| // if (oldFileData?.Id != null) | |
| // { | |
| // await _storageService.DeleteReportFilesAsync(existingReport, oldFileData.Id); | |
| // } | |
| var fileData = new ReportFile | |
| { | |
| Id = CoreHelpers.SecureRandomString(32, upper: false, special: false), | |
| FileName = "report-data.json", | |
| Validated = false, | |
| Size = 0 | |
| }; | |
| existingReport.SetReportFile(fileData); | |
| } |
src/Api/Dirt/Models/Response/OrganizationReportResponseModel.cs
Outdated
Show resolved
Hide resolved
| PasswordCount = organizationReport.PasswordCount; | ||
| PasswordAtRiskCount = organizationReport.PasswordAtRiskCount; | ||
| MemberCount = organizationReport.MemberCount; | ||
| ReportFile = organizationReport.GetReportFile(); |
There was a problem hiding this comment.
Information leak: The ReportFile object (containing internal fields like Id, Size, Validated, FileName) is exposed directly to API consumers via the response model. The Id is a CoreHelpers.SecureRandomString that's used as a path component for blob storage — leaking it gives clients knowledge of internal storage paths.
Consider either removing this property from the response model (the download URL is already exposed via ReportFileDownloadUrl), or creating a slimmed-down DTO that only exposes what the client needs (e.g., FileName and Size).
|
|
||
| var leeway = 1024L * 1024L; // 1 MB | ||
| var minimum = Math.Max(0, fileData.Size - leeway); | ||
| var maximum = Math.Min(fileData.Size + leeway, Constants.FileSize501mb); | ||
| var (valid, length) = await _storageService.ValidateFileAsync(report, fileData, minimum, maximum); |
There was a problem hiding this comment.
Bug (Medium): File size validation is ineffective for update-triggered uploads. When RequiresNewFileUpload is true via the update path, UpdateOrganizationReportV2Command creates a new ReportFile with Size = 0 (line 98 of that command). Here, the leeway calculation becomes:
minimum = Math.Max(0, 0 - 1MB) = 0maximum = Math.Min(0 + 1MB, 501MB) = 1MB
This means uploaded files from the update path are silently limited to ~1MB instead of 501MB. For the create path this works correctly because AddOrganizationReportRequest.FileSize is set by the client.
UpdateOrganizationReportV2Request should also carry a FileSize field when RequiresNewFileUpload is true, so the validation boundary is meaningful.
| // UPDATE Whole Report | ||
| [HttpPatch("{organizationId}/{reportId}")] | ||
| public async Task<IActionResult> UpdateOrganizationReportAsync(Guid organizationId, [FromBody] UpdateOrganizationReportRequest request) | ||
| [RequestSizeLimit(Constants.FileSize501mb)] | ||
| public async Task<IActionResult> UpdateOrganizationReportAsync( | ||
| Guid organizationId, | ||
| Guid reportId, | ||
| [FromBody] UpdateOrganizationReportV2Request request) | ||
| { | ||
| if (_featureService.IsEnabled(FeatureFlagKeys.AccessIntelligenceVersion2)) | ||
| { | ||
| await AuthorizeAsync(organizationId); | ||
|
|
||
| request.OrganizationId = organizationId; | ||
| request.ReportId = reportId; | ||
|
|
There was a problem hiding this comment.
Design: In the V2 path, the controller overwrites request.OrganizationId and request.ReportId from route params (lines 244-245), making the client-sent values in the body irrelevant. This is correct from a security standpoint (prevents ID spoofing), but in the V1 path (line 268), the controller validates that request.OrganizationId matches the route param and rejects mismatches.
This means a client sending { "organizationId": "wrong-id" } will succeed in V2 (silently overwritten) but fail in V1 (rejected). Consider adding [JsonIgnore] to OrganizationId and ReportId on UpdateOrganizationReportV2Request to make the API contract explicit that these come from the route.
| public async Task<(bool valid, long length)> ValidateFileAsync( | ||
| OrganizationReport report, ReportFile fileData, long minimum, long maximum) | ||
| { | ||
| await InitAsync(); | ||
|
|
||
| var blobClient = _containerClient!.GetBlobClient(BlobPath(report, fileData.Id!, fileData.FileName)); | ||
|
|
||
| try | ||
| { | ||
| var blobProperties = await blobClient.GetPropertiesAsync(); | ||
| var metadata = blobProperties.Value.Metadata; | ||
| metadata["organizationId"] = report.OrganizationId.ToString(); | ||
| await blobClient.SetMetadataAsync(metadata); | ||
|
|
||
| var headers = new BlobHttpHeaders | ||
| { | ||
| ContentDisposition = $"attachment; filename=\"{fileData.FileName}\"" | ||
| }; | ||
| await blobClient.SetHttpHeadersAsync(headers); | ||
|
|
||
| var length = blobProperties.Value.ContentLength; | ||
| var valid = minimum <= length && length <= maximum; | ||
|
|
||
| return (valid, length); | ||
| } | ||
| catch (Exception ex) | ||
| { | ||
| _logger.LogError(ex, "A storage operation failed in {MethodName}", nameof(ValidateFileAsync)); | ||
| return (false, -1); | ||
| } | ||
| } |
There was a problem hiding this comment.
Naming / side effects: ValidateFileAsync does more than validation — it also sets blob metadata (organizationId) and HTTP headers (Content-Disposition). This makes it surprising for callers who only expect a boolean + length result. Consider renaming to ValidateAndFinalizeFileAsync, or extracting the metadata/header logic into a separate method called during finalization.
| return await ApiHelpers.HandleAzureEvents(Request, new Dictionary<string, Func<Azure.Messaging.EventGrid.EventGridEvent, Task>> | ||
| { | ||
| { | ||
| "Microsoft.Storage.BlobCreated", async (eventGridEvent) => | ||
| { | ||
| try | ||
| { | ||
| var blobName = | ||
| eventGridEvent.Subject.Split($"{AzureOrganizationReportStorageService.ContainerName}/blobs/")[1]; | ||
| var reportId = AzureOrganizationReportStorageService.ReportIdFromBlobName(blobName); | ||
| var report = await _organizationReportRepo.GetByIdAsync(new Guid(reportId)); | ||
| if (report == null) | ||
| { | ||
| if (_storageService is AzureOrganizationReportStorageService azureStorageService) | ||
| { | ||
| await azureStorageService.DeleteBlobAsync(blobName); | ||
| } | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| var fileData = report.GetReportFile(); | ||
| if (fileData == null) | ||
| { | ||
| return; | ||
| } | ||
|
|
||
| await _validateCommand.ValidateAsync(report, fileData.Id!); | ||
| } | ||
| catch (Exception e) | ||
| { | ||
| _logger.LogError(e, "Uncaught exception occurred while handling event grid event: {Event}", | ||
| JsonSerializer.Serialize(eventGridEvent)); | ||
| } | ||
| } | ||
| } | ||
| }); |
There was a problem hiding this comment.
Nit: In the AzureValidateFile endpoint, the catch block at line 409 logs the serialized eventGridEvent. Verify that event data cannot contain PII or sensitive blob content. The current event only contains the blob subject path (org ID, date, report ID, file ID), which appears safe. But since Bitwarden policy is to never log PII, worth confirming that no Event Grid subscription custom data could leak into this path.
Also, consider limiting the scope of the try/catch. Currently, if _organizationReportRepo.GetByIdAsync or DeleteBlobAsync throw, the exception is swallowed and logged. This could mask issues like database connectivity problems.
| @@ -36,8 +50,15 @@ public OrganizationReportsController( | |||
| IGetOrganizationReportDataQuery getOrganizationReportDataQuery, | |||
| IUpdateOrganizationReportDataCommand updateOrganizationReportDataCommand, | |||
| IGetOrganizationReportApplicationDataQuery getOrganizationReportApplicationDataQuery, | |||
| IUpdateOrganizationReportApplicationDataCommand updateOrganizationReportApplicationDataCommand | |||
| ) | |||
| IUpdateOrganizationReportApplicationDataCommand updateOrganizationReportApplicationDataCommand, | |||
| IFeatureService featureService, | |||
| IApplicationCacheService applicationCacheService, | |||
| IOrganizationReportStorageService storageService, | |||
| ICreateOrganizationReportCommand createReportCommand, | |||
| IOrganizationReportRepository organizationReportRepo, | |||
| IUpdateOrganizationReportV2Command updateReportV2Command, | |||
| IValidateOrganizationReportFileCommand validateCommand, | |||
| ILogger<OrganizationReportsController> logger) | |||
| { | |||
| _currentContext = currentContext; | |||
| _getOrganizationReportQuery = getOrganizationReportQuery; | |||
| @@ -50,68 +71,195 @@ IUpdateOrganizationReportApplicationDataCommand updateOrganizationReportApplicat | |||
| _updateOrganizationReportDataCommand = updateOrganizationReportDataCommand; | |||
| _getOrganizationReportApplicationDataQuery = getOrganizationReportApplicationDataQuery; | |||
| _updateOrganizationReportApplicationDataCommand = updateOrganizationReportApplicationDataCommand; | |||
| _featureService = featureService; | |||
| _applicationCacheService = applicationCacheService; | |||
| _storageService = storageService; | |||
| _createReportCommand = createReportCommand; | |||
| _organizationReportRepo = organizationReportRepo; | |||
| _updateReportV2Command = updateReportV2Command; | |||
| _validateCommand = validateCommand; | |||
| _logger = logger; | |||
| } | |||
There was a problem hiding this comment.
Nit: This controller has 17 constructor parameters. While this is understandable given the V1/V2 coexistence, it's worth considering whether V2-specific dependencies could be extracted into a separate controller (e.g., OrganizationReportsV2Controller) behind the [RequireFeature] attribute. This would also simplify the feature-flag branching throughout the action methods.
Not a blocking issue — just something to consider for maintainability once the V1 paths are removed.
|
1 participant
🎟️ Tracking
This is a PR for a sub-task (PM-32520) on user story PM-31923
📔 Objective
Creating new V2 endpoints for read and update operations on the whole report in the database. This will also include the logic for saving a reportData file in Azure Blob storage and server if self-hosted.
📸 Screenshots