Skip to content

[PM-34909] Serve only validated report files from get-latest and self-hosted file endpoints #7465

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
AlexRubik wants to merge 9 commits into
base: main
Choose a base branch
from
Open

[PM-34909] Serve only validated report files from get-latest and self-hosted file endpoints #7465

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
091d503
feat(dirt): enforce validated state in download and upload endpoints
AlexRubik May 19, 2026
d69fab5
feat(dirt): add ReadLatest SP and skip empty ReportData in V1 SP
AlexRubik May 19, 2026
c50e572
feat(dirt): dispatch GetLatest endpoint by V2 feature flag
AlexRubik May 19, 2026
8561e00
docs(dirt): document JSON substring coupling on EF ReadLatest filter
AlexRubik May 20, 2026
d5c0bd9
style(dirt): reformat OrganizationReport SPs per SQL keyword style
AlexRubik May 21, 2026
dcd4ec0
Update src/Sql/dbo/Dirt/Stored Procedures/OrganizationReport_GetLates…
mkincaid-bw May 21, 2026
d3e0cf9
Update src/Sql/dbo/Dirt/Stored Procedures/OrganizationReport_ReadLate…
mkincaid-bw May 21, 2026
c38911a
Update util/Migrator/DbScripts/2026-05-19_00_AddOrganizationReportRea…
mkincaid-bw May 21, 2026
06adae0
Update util/Migrator/DbScripts/2026-05-19_01_UpdateOrganizationReport…
mkincaid-bw May 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 12 additions & 8 deletions src/Api/Dirt/Controllers/OrganizationReportsController.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,19 +141,18 @@ public async Task<IActionResult> GetLatestOrganizationReportAsync(Guid organizat

await AuthorizeAsync(organizationId);

var latestReport = await _getOrganizationReportQuery.GetLatestOrganizationReportAsync(organizationId);
var isAccessIntelligenceV2 = _featureService.IsEnabled(FeatureFlagKeys.AccessIntelligenceVersion2);

if (latestReport == null)
{
throw new NotFoundException();
}
Comment on lines -146 to -149
Copy link
Copy Markdown
Contributor

@Banrion Banrion May 13, 2026

Choose a reason for hiding this comment

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

⚠️ We still need to check for null in the case that no report exists

Copy link
Copy Markdown
Contributor Author

@AlexRubik AlexRubik May 20, 2026

Choose a reason for hiding this comment

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

Null check moved down a layer. Both GetLatestOrganizationReportAsync and ReadLatestOrganizationReportAsync in GetOrganizationReportQuery (.../Reports/ReportFeatures/GetOrganizationReportQuery.cs) throw NotFoundException on null, covered by _NullResult_ThrowsNotFoundException tests for each in GetOrganizationReportQueryTests.cs. So the controller never sees a null latestReport. Shipped in c50e572d8.

var latestReport = isAccessIntelligenceV2
? await _getOrganizationReportQuery.ReadLatestOrganizationReportAsync(organizationId)
: await _getOrganizationReportQuery.GetLatestOrganizationReportAsync(organizationId);

var response = new OrganizationReportResponseModel(latestReport);

if (_featureService.IsEnabled(FeatureFlagKeys.AccessIntelligenceVersion2))
if (isAccessIntelligenceV2)
{
var fileData = latestReport.GetReportFile();
if (fileData is { Validated: true })
if (fileData != null)
{
response.ReportFileDownloadUrl = await _storageService.GetReportDataDownloadUrlAsync(latestReport, fileData);
response.FileUploadType = _storageService.FileUploadType;
Expand Down Expand Up @@ -393,7 +392,7 @@ public async Task UploadReportFileAsync(Guid organizationId, Guid reportId, [Fro
}

var fileData = report.GetReportFile();
if (fileData == null || fileData.Id != reportFileId)
if (fileData == null || fileData.Id != reportFileId || fileData.Validated)
{
throw new NotFoundException();
}
Expand Down Expand Up @@ -449,6 +448,11 @@ public async Task<IActionResult> DownloadReportFileAsync(Guid organizationId, Gu
throw new NotFoundException();
}

if (_featureService.IsEnabled(FeatureFlagKeys.AccessIntelligenceVersion2) && !fileData.Validated)
Copy link
Copy Markdown
Contributor

@Banrion Banrion May 13, 2026

Choose a reason for hiding this comment

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

⚠️ This route is already gated by the feature flag and validated will always be true from the query. This can be removed

Copy link
Copy Markdown
Contributor Author

@AlexRubik AlexRubik May 20, 2026

Choose a reason for hiding this comment

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

We look the report row up by report id via GetAuthorizedReportAsync which doesn't filter by validated (only the new ReadLatestByOrganizationIdAsync in the repo layer does, and that's the latest-by-org path, not by-id). The route (DownloadReportFileAsync) also has no [RequireFeature] attribute, just [SelfHosted(SelfHostedOnly = true)]. So with the flag ON, this guard is the only block against streaming an unvalidated file.

Guard shipped in 091d50362.

{
throw new NotFoundException();
}

var stream = await _storageService.GetReportReadStreamAsync(report, fileData);
if (stream == null)
{
Expand Down
15 changes: 15 additions & 0 deletions src/Core/Dirt/Reports/ReportFeatures/GetOrganizationReportQuery.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,21 @@ public async Task<OrganizationReport> GetLatestOrganizationReportAsync(Guid orga
return result;
}

public async Task<OrganizationReport> ReadLatestOrganizationReportAsync(Guid organizationId)
{
_logger.LogInformation(Constants.BypassFiltersEventId,
"Reading latest validated-file organization report for organization {organizationId}",
organizationId);
var result = await _organizationReportRepo.ReadLatestByOrganizationIdAsync(organizationId);

if (result == null)
{
throw new NotFoundException($"No validated report found for organization: {organizationId}");
}

return result;
}

public async Task<OrganizationReport> GetOrganizationReportAsync(Guid reportId)
{
_logger.LogInformation(Constants.BypassFiltersEventId, "Fetching organization reports for organization by Id: {reportId}", reportId);
Expand Down
1 change: 1 addition & 0 deletions src/Core/Dirt/Reports/ReportFeatures/Interfaces/IGetOrganizationReportQuery.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,5 @@ public interface IGetOrganizationReportQuery
{
Task<OrganizationReport> GetOrganizationReportAsync(Guid organizationId);
Task<OrganizationReport> GetLatestOrganizationReportAsync(Guid organizationId);
Task<OrganizationReport> ReadLatestOrganizationReportAsync(Guid organizationId);
}
1 change: 1 addition & 0 deletions src/Core/Dirt/Repositories/IOrganizationReportRepository.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ public interface IOrganizationReportRepository : IRepository<OrganizationReport,
{
// Whole OrganizationReport methods
Task<OrganizationReport> GetLatestByOrganizationIdAsync(Guid organizationId);
Task<OrganizationReport> ReadLatestByOrganizationIdAsync(Guid organizationId);

// SummaryData methods
Task<IEnumerable<OrganizationReportSummaryDataResponse>> GetSummaryDataByDateRangeAsync(Guid organizationId, DateTime startDate, DateTime endDate);
Expand Down
13 changes: 13 additions & 0 deletions src/Infrastructure.Dapper/Dirt/OrganizationReportRepository.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,19 @@ public async Task<OrganizationReport> GetLatestByOrganizationIdAsync(Guid organi
}
}

public async Task<OrganizationReport> ReadLatestByOrganizationIdAsync(Guid organizationId)
{
using (var connection = new SqlConnection(ReadOnlyConnectionString))
{
var result = await connection.QuerySingleOrDefaultAsync<OrganizationReport>(
$"[{Schema}].[OrganizationReport_ReadLatestByOrganizationId]",
new { OrganizationId = organizationId },
commandType: CommandType.StoredProcedure);

return result;
}
}

public async Task<OrganizationReport> UpdateSummaryDataAsync(Guid organizationId, Guid reportId, string summaryData)
{
using (var connection = new SqlConnection(ConnectionString))
Expand Down
26 changes: 25 additions & 1 deletion src/Infrastructure.EntityFramework/Dirt/Repositories/OrganizationReportRepository.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,31 @@ public async Task<OrganizationReport> GetLatestByOrganizationIdAsync(Guid organi
{
var dbContext = GetDatabaseContext(scope);
var result = await dbContext.OrganizationReports
.Where(p => p.OrganizationId == organizationId)
.Where(p => p.OrganizationId == organizationId
&& p.ReportData != string.Empty)
.OrderByDescending(p => p.RevisionDate)
.Take(1)
.FirstOrDefaultAsync();

if (result == null) return default;

return Mapper.Map<OrganizationReport>(result);
}
}

public async Task<OrganizationReport> ReadLatestByOrganizationIdAsync(Guid organizationId)
{
using (var scope = ServiceScopeFactory.CreateScope())
{
var dbContext = GetDatabaseContext(scope);
// Substring match is coupled to SetReportFile (OrganizationReport.cs) writing via
// JsonHelpers.IgnoreWritingNull (PascalCase, no whitespace). The Dapper/MSSQL path
// uses JSON_VALUE which is format-agnostic; changing the serializer options would
// silently return zero rows here without affecting the MSSQL path.
var result = await dbContext.OrganizationReports
.Where(p => p.OrganizationId == organizationId
&& p.ReportFile != null
&& p.ReportFile.Contains("\"Validated\":true"))
Comment on lines +52 to +54
Copy link
Copy Markdown
Contributor

@claude claude Bot May 19, 2026

Choose a reason for hiding this comment

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

🎨 SUGGESTED: EF filter relies on a raw JSON substring match, which drifts from the Dapper JSON_VALUE filter and is fragile if serialization options ever change.

Details and fix

The Dapper/MSSQL stored procedure filters with JSON_VALUE([ReportFile], '$.Validated') = 'true', while the EF version uses p.ReportFile.Contains("\"Validated\":true"). The substring match works today only because OrganizationReport.SetReportFile uses JsonHelpers.IgnoreWritingNull (no whitespace, default PascalCase) and ReportFile is the only writer. Any future change to the serializer (e.g., adding WriteIndented, switching to camelCase, or routing through Newtonsoft) would silently make this filter return zero rows — and the dual-ORM bug would only show up on Postgres/MySQL/SQLite.

Consider using EF.Functions.JsonExtract (provider-specific) or pulling the candidates and filtering in memory by deserializing with GetReportFile(). At minimum, anchor the substring with a brace (e.g., "\"Validated\":true," plus "\"Validated\":true}") or add a comment documenting the coupling to SetReportFile's serializer options so future maintainers don't break this implicitly.

Copy link
Copy Markdown
Contributor Author

@AlexRubik AlexRubik May 20, 2026

Choose a reason for hiding this comment

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

The substring match is our repo's convention for these cases currently. Introducing this new json extract convention is out of scope for this pr but I still made a comment here 8561e00

.OrderByDescending(p => p.RevisionDate)
.Take(1)
.FirstOrDefaultAsync();
Expand Down
10 changes: 7 additions & 3 deletions src/Sql/dbo/Dirt/Stored Procedures/OrganizationReport_GetLatestByOrganizationId.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,11 @@ BEGIN

SELECT TOP 1
*
FROM [dbo].[OrganizationReportView]
WHERE [OrganizationId] = @OrganizationId
ORDER BY [RevisionDate] DESC
FROM
[dbo].[OrganizationReportView]
WHERE
[OrganizationId] = @OrganizationId
AND [ReportData] <> ''
ORDER BY
[RevisionDate] DESC
END
16 changes: 16 additions & 0 deletions src/Sql/dbo/Dirt/Stored Procedures/OrganizationReport_ReadLatestByOrganizationId.sql
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
CREATE PROCEDURE [dbo].[OrganizationReport_ReadLatestByOrganizationId]
Copy link
Copy Markdown
Contributor

@mkincaid-bw mkincaid-bw May 20, 2026

Choose a reason for hiding this comment

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

Since OrganizationReport_GetLatestByOrganizationId should have been named OrganizationReport_ReadLatestByOrganizationId, the naming of this new procedure insinuates these two procs return the same data. If this new proc will eventually replace OrganizationReport_GetLatestByOrganizationId, then I'm okay with it; otherwise I think it should be named something else to differentiate that it returns different data, maybe OrganizationReport_ReadLatestValidatedByOrganizationId?

Copy link
Copy Markdown
Contributor Author

@AlexRubik AlexRubik May 21, 2026

Choose a reason for hiding this comment

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

If this new proc will eventually replace OrganizationReport_GetLatestByOrganizationId, then I'm okay with it

Yes that's the plan, once pm-31920-access-intelligence-azure-file-storage flag is permanently ON, the V1 OrganizationReport_GetLatestByOrganizationId SP becomes dead code and we remove it, leaving OrganizationReport_ReadLatestByOrganizationId as the only latest-by-org SP.

@OrganizationId UNIQUEIDENTIFIER
AS
BEGIN
SET NOCOUNT ON

SELECT TOP 1
*
FROM
[dbo].[OrganizationReportView]
WHERE
[OrganizationId] = @OrganizationId
AND JSON_VALUE([ReportFile], '$.Validated') = 'true'
ORDER BY
[RevisionDate] DESC
END
193 changes: 192 additions & 1 deletion test/Api.Test/Dirt/OrganizationReportsControllerTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
using Bit.Core.Utilities;
using Bit.Test.Common.AutoFixture;
using Bit.Test.Common.AutoFixture.Attributes;
using Microsoft.AspNetCore.Http;
using Microsoft.AspNetCore.Mvc;
using Microsoft.Extensions.Logging;
using NSubstitute;
Expand Down Expand Up @@ -48,7 +49,7 @@ public async Task GetLatestOrganizationReportAsync_WithValidatedFile_ReturnsOkWi
.Returns(true);

sutProvider.GetDependency<IGetOrganizationReportQuery>()
.GetLatestOrganizationReportAsync(orgId)
.ReadLatestOrganizationReportAsync(orgId)
.Returns(expectedReport);

sutProvider.GetDependency<IOrganizationReportStorageService>()
Expand All @@ -69,6 +70,77 @@ public async Task GetLatestOrganizationReportAsync_WithValidatedFile_ReturnsOkWi
Assert.Equal(FileUploadType.Azure, response.FileUploadType);
}

[Theory, BitAutoData]
public async Task GetLatestOrganizationReportAsync_FlagOn_CallsReadLatest(
SutProvider<OrganizationReportsController> sutProvider,
Guid orgId,
OrganizationReport expectedReport)
{
// Arrange
expectedReport.OrganizationId = orgId;
var reportFile = new ReportFile { Id = "file-id", FileName = "report.json", Size = 1024, Validated = true };
expectedReport.SetReportFile(reportFile);

SetupAuthorization(sutProvider, orgId);

sutProvider.GetDependency<IFeatureService>()
.IsEnabled(FeatureFlagKeys.AccessIntelligenceVersion2)
.Returns(true);

sutProvider.GetDependency<IGetOrganizationReportQuery>()
.ReadLatestOrganizationReportAsync(orgId)
.Returns(expectedReport);

sutProvider.GetDependency<IOrganizationReportStorageService>()
.GetReportDataDownloadUrlAsync(expectedReport, Arg.Any<ReportFile>())
.Returns("https://download-url");

// Act
var result = await sutProvider.Sut.GetLatestOrganizationReportAsync(orgId);

// Assert
Assert.IsType<OkObjectResult>(result);
await sutProvider.GetDependency<IGetOrganizationReportQuery>()
.Received(1)
.ReadLatestOrganizationReportAsync(orgId);
await sutProvider.GetDependency<IGetOrganizationReportQuery>()
.DidNotReceive()
.GetLatestOrganizationReportAsync(Arg.Any<Guid>());
}

[Theory, BitAutoData]
public async Task GetLatestOrganizationReportAsync_FlagOff_CallsGetLatest(
SutProvider<OrganizationReportsController> sutProvider,
Guid orgId,
OrganizationReport expectedReport)
{
// Arrange
expectedReport.OrganizationId = orgId;
expectedReport.ReportFile = null;

SetupAuthorization(sutProvider, orgId);

sutProvider.GetDependency<IFeatureService>()
.IsEnabled(FeatureFlagKeys.AccessIntelligenceVersion2)
.Returns(false);

sutProvider.GetDependency<IGetOrganizationReportQuery>()
.GetLatestOrganizationReportAsync(orgId)
.Returns(expectedReport);

// Act
var result = await sutProvider.Sut.GetLatestOrganizationReportAsync(orgId);

// Assert
Assert.IsType<OkObjectResult>(result);
await sutProvider.GetDependency<IGetOrganizationReportQuery>()
.Received(1)
.GetLatestOrganizationReportAsync(orgId);
await sutProvider.GetDependency<IGetOrganizationReportQuery>()
.DidNotReceive()
.ReadLatestOrganizationReportAsync(Arg.Any<Guid>());
}

[Theory, BitAutoData]
public async Task GetLatestOrganizationReportAsync_WithNoFile_ReturnsOkWithNullDownloadUrl(
SutProvider<OrganizationReportsController> sutProvider,
Expand Down Expand Up @@ -1401,6 +1473,125 @@ await sutProvider.GetDependency<ICreateOrganizationReportCommand>()
.CreateAsync(Arg.Any<AddOrganizationReportRequest>());
}

// DownloadReportFileAsync - validated file enforcement

[Theory, BitAutoData]
public async Task DownloadReportFileAsync_FlagOn_UnvalidatedFile_ThrowsNotFoundException(
SutProvider<OrganizationReportsController> sutProvider,
Guid orgId,
OrganizationReport report)
{
// Arrange
report.OrganizationId = orgId;
var fileData = new ReportFile { Id = "file-id", FileName = "report.json", Size = 1024, Validated = false };
report.SetReportFile(fileData);

SetupV2Authorization(sutProvider, orgId);

sutProvider.GetDependency<IGetOrganizationReportQuery>()
.GetOrganizationReportAsync(report.Id)
.Returns(report);

// Act & Assert
await Assert.ThrowsAsync<NotFoundException>(() =>
sutProvider.Sut.DownloadReportFileAsync(orgId, report.Id));

await sutProvider.GetDependency<IOrganizationReportStorageService>()
.DidNotReceive()
.GetReportReadStreamAsync(Arg.Any<OrganizationReport>(), Arg.Any<ReportFile>());
}

[Theory, BitAutoData]
public async Task DownloadReportFileAsync_FlagOn_ValidatedFile_ReturnsFile(
SutProvider<OrganizationReportsController> sutProvider,
Guid orgId,
OrganizationReport report)
{
// Arrange
report.OrganizationId = orgId;
var fileData = new ReportFile { Id = "file-id", FileName = "report.json", Size = 1024, Validated = true };
report.SetReportFile(fileData);

SetupV2Authorization(sutProvider, orgId);

sutProvider.GetDependency<IGetOrganizationReportQuery>()
.GetOrganizationReportAsync(report.Id)
.Returns(report);

var stream = new MemoryStream(new byte[] { 1, 2, 3 });
sutProvider.GetDependency<IOrganizationReportStorageService>()
.GetReportReadStreamAsync(report, Arg.Any<ReportFile>())
.Returns(stream);

// Act
var result = await sutProvider.Sut.DownloadReportFileAsync(orgId, report.Id);

// Assert
var fileResult = Assert.IsType<FileStreamResult>(result);
Assert.Equal("application/octet-stream", fileResult.ContentType);
}

[Theory, BitAutoData]
public async Task DownloadReportFileAsync_FlagOff_UnvalidatedFile_ReturnsFile(
SutProvider<OrganizationReportsController> sutProvider,
Guid orgId,
OrganizationReport report)
{
// Arrange
report.OrganizationId = orgId;
var fileData = new ReportFile { Id = "file-id", FileName = "report.json", Size = 1024, Validated = false };
report.SetReportFile(fileData);

SetupAuthorization(sutProvider, orgId);

sutProvider.GetDependency<IFeatureService>()
.IsEnabled(FeatureFlagKeys.AccessIntelligenceVersion2)
.Returns(false);

sutProvider.GetDependency<IGetOrganizationReportQuery>()
.GetOrganizationReportAsync(report.Id)
.Returns(report);

var stream = new MemoryStream(new byte[] { 1, 2, 3 });
sutProvider.GetDependency<IOrganizationReportStorageService>()
.GetReportReadStreamAsync(report, Arg.Any<ReportFile>())
.Returns(stream);

// Act
var result = await sutProvider.Sut.DownloadReportFileAsync(orgId, report.Id);

// Assert
Assert.IsType<FileStreamResult>(result);
}

// UploadReportFileAsync - re-upload guard

[Theory, BitAutoData]
public async Task UploadReportFileAsync_AlreadyValidated_ThrowsNotFoundException(
SutProvider<OrganizationReportsController> sutProvider,
Guid orgId,
OrganizationReport report)
{
// Arrange
report.OrganizationId = orgId;
var fileData = new ReportFile { Id = "file-id", FileName = "report.json", Size = 1024, Validated = true };
report.SetReportFile(fileData);

SetupV2Authorization(sutProvider, orgId);

sutProvider.GetDependency<IGetOrganizationReportQuery>()
.GetOrganizationReportAsync(report.Id)
.Returns(report);

var httpContext = new DefaultHttpContext();
httpContext.Request.ContentType = "multipart/form-data";
sutProvider.Sut.ControllerContext = new ControllerContext { HttpContext = httpContext };

// Act & Assert
await Assert.ThrowsAsync<NotFoundException>(() =>
sutProvider.Sut.UploadReportFileAsync(orgId, report.Id, "file-id"));
}

// Helper methods for authorization mocks

private static void SetupAuthorization(
Expand Down
Loading
Loading