[PM-38743] Honor a partially supplied event date range

[AlexRubik]

🎟️ Tracking

PM-38743

📔 Objective

Every event-log endpoint resolves its start / end query parameters through a guard that reads if (!end.HasValue || !start.HasValue). That treats "one bound supplied" the same as "no bounds supplied", so a caller who sends only ?start= or only ?end= has both values discarded and silently receives the default last 30 days, with no error and no warning.

The ticket points at EventFilterRequestModel.ToDateRange(), which serves one endpoint (GET /public/events). That is 1 of 12 affected entry points. The other 11 route through ApiHelpers.GetDateRange, which holds a byte-identical copy of the same guard, already diverged on its exception message. This PR fixes the shared helper and collapses the duplicate into it, so all 12 entry points are repaired from one place.

Each bound is now resolved independently:

Request	Resolved range
neither bound	last 30 days (unchanged)
start only	start through the end of the current day
end only	the 30 days before end
both bounds	unchanged, including the inverted-range swap and the 367-day cap

The swap and the 367-day cap now apply to every case rather than only to fully supplied ranges, which also closes a latent bug: a future-dated start-only request would previously have produced an inverted range that no branch corrected.

Two details worth a reviewer's attention:

1. ToDateRange() still writes the resolved bounds back onto the model. This is load-bearing, not stylistic: EventDiagnosticLogger.LogAggregateData reads request.Start and request.End after the call to log the query's effective filters, so dropping the write-back would silently start logging null for defaulted requests. The invariant is pinned by ToDateRange_WritesResolvedBoundsBackOntoTheModelForDiagnosticLogging.
2. The 367-day cap message is standardized on the more descriptive of the two variants that existed. That is the string the Public API already returned, so no externally visible message changes; the internal API's message improves.

GetDateRange previously had zero unit coverage despite serving eleven endpoints, which is how this shipped. This PR adds 10 tests covering every branch. All three tests that reproduce the bug were confirmed failing against main before the fix was applied.

Open questions before this leaves draft

GET /public/events is an externally consumed contract, so this changes behavior for existing integrations. Three questions need a decision:

1. Is the response change acceptable without a version bump? An integration sending only start today receives the last 30 days and will now receive the range it actually asked for.
2. A start-only request more than 367 days back now returns 400 (Date range must be < 367 days.) where it previously returned 200. Accept that, or clamp the inferred end to start + 367 days and keep returning 200? Clamping avoids the new 400 but silently truncates.
3. Confirm that end only should resolve to a 30-day window rather than "everything up to end", which would be unbounded and would immediately hit the cap.

Also worth noting: this changes GET /sm/events/service-accounts/{id}, which is owned by Secrets Manager. A reviewer from that team would be useful.

📸 Screenshots

Not applicable, no UI changes.

🤖 Testing

• dotnet format clean on all five changed files.
• dotnet build test/Api.Test/Api.Test.csproj: 0 errors, and 0 warnings originating from the changed files.
• dotnet test test/Api.Test/Api.Test.csproj: 1955 passed, 0 failed. This includes the 13 pre-existing EventsControllerTests and the 6 EventDiagnosticLoggerTests, all unmodified and all still green.
• Fail-first verified: with ApiHelpers.cs reverted to main, GetDateRange_OnlyStartSupplied_KeepsStartAndRunsToEndOfToday, GetDateRange_OnlyEndSupplied_KeepsEndAndStartsThirtyDaysBefore, and ToDateRange_OnlyStartSupplied_DoesNotFallBackToThirtyDayDefault all fail.

Manual verification against a local server has not been run yet. To reproduce the original bug, compare the response counts of GET /public/events?start=<2 hours ago> and GET /public/events with no query string: before this change they are identical, after it the filtered call returns fewer events.

[github-actions]

🤖 Bitwarden Claude Code Review

Overall Assessment: REQUEST CHANGES

Reviewed the rewrite of ApiHelpers.GetDateRange, the collapse of EventFilterRequestModel.ToDateRange() into it, the two doc-comment updates, and the 12 new tests. The independent-bound resolution, the swap, and the 367-day cap all behave as the description states, and the write-back onto the model that EventDiagnosticLogger.LogAggregateData depends on is preserved and pinned by a test. One new failure mode: end?.AddDays(-30) is unguarded against underflow, so an end bound near DateTime.MinValue now produces a 500 where it previously produced a 200. Behavior changes reach all 12 entry points including GET /sm/events/service-accounts/{id}, and no callers outside src/Api depend on the old "Range too large." message.

Code Review Details

• ⚠️ : end bound within 30 days of DateTime.MinValue throws ArgumentOutOfRangeException, returning 500
  • src/Api/Utilities/ApiHelpers.cs:91

Notes on the three open questions in the description, which need product/AppSec input rather than a code change: questions 1 and 2 (externally visible response change on GET /public/events, and start-only beyond the cap now returning 400) apply equally to the 11 internal endpoints, not just the Public API — worth stating that scope explicitly when this leaves draft.

[claude]

⚠️ IMPORTANT: An end bound within 30 days of DateTime.MinValue makes this throw, turning a previously-200 request into a 500.

Details and fix

end?.AddDays(-30) throws ArgumentOutOfRangeException when the result falls below DateTime.MinValue. GET /public/events?end=0001-01-10 binds successfully (no range validation on EventFilterRequestModel.End), reaches this line, and throws. ExceptionHandlerFilterAttribute has no branch for that exception type, so it lands in the final else: HTTP 500 plus an error-level "Unhandled exception" log entry.

Before this change the end-only branch discarded the supplied value and never did arithmetic on it, so the same request returned 200 with the last 30 days. This is a new failure mode rather than a pre-existing one, and it reaches all 12 entry points that route through this helper.

Clamping keeps the rest of the method unchanged — the 367-day cap then rejects the resulting range with a 400, which is the right answer for this input:

start ??= end.HasValue
    ? (end.Value > DateTime.MinValue.AddDays(30) ? end.Value.AddDays(-30) : DateTime.MinValue)
    : DateTime.UtcNow.Date.AddDays(-30);

Worth a companion test alongside GetDateRange_OnlyEndSupplied_KeepsEndAndStartsThirtyDaysBefore.