fix(search): move pagination clamp to controller (fixes cleanup regression)
Phase 5 re-validation caught a functional regression: the V-10 clamp in SearchService.SearchAsync also capped CleanupService's internal target resolution (pageSize 10000 -> 200), silently limiting bulk cleanup-by-query to 200 emails. The clamp belongs at the user-facing trust boundary, not the shared service: move MaxPageSize (200) enforcement into SearchController (both the POST body and GET query paths). Internal callers of ISearchService now request large pages unhindered, while user requests are still bounded. Adds a regression test proving SearchService returns a 250-row page uncapped. No security regressions per Phase 5. All 39 tests green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
@@ -7,19 +7,31 @@ namespace InboxIntel.Api.Controllers;
|
||||
|
||||
public class SearchController : ApiControllerBase
|
||||
{
|
||||
/// <summary>Hard upper bound on a user-requested page (V-10: DoS via huge pageSize).
|
||||
/// Enforced HERE, at the user-facing trust boundary, so internal callers of
|
||||
/// ISearchService (e.g. cleanup target resolution) can still request large pages.</summary>
|
||||
private const int MaxPageSize = 200;
|
||||
|
||||
private readonly ISearchService _search;
|
||||
public SearchController(ISearchService search) => _search = search;
|
||||
|
||||
/// <summary>Structured search via JSON body.</summary>
|
||||
[HttpPost]
|
||||
public async Task<IActionResult> Search([FromBody] SearchRequestDto request, CancellationToken ct)
|
||||
=> Ok(await _search.SearchAsync(UserId, request, ct));
|
||||
{
|
||||
var clamped = request with
|
||||
{
|
||||
Page = Math.Max(1, request.Page),
|
||||
PageSize = Math.Clamp(request.PageSize, 1, MaxPageSize)
|
||||
};
|
||||
return Ok(await _search.SearchAsync(UserId, clamped, ct));
|
||||
}
|
||||
|
||||
/// <summary>Gmail-like query string search, e.g. ?q=from:github.com is:unread.</summary>
|
||||
[HttpGet]
|
||||
public async Task<IActionResult> Query([FromQuery] string? q, [FromQuery] int page = 1, [FromQuery] int pageSize = 50, CancellationToken ct = default)
|
||||
{
|
||||
var parsed = GmailQueryParser.Parse(q, page, pageSize);
|
||||
var parsed = GmailQueryParser.Parse(q, Math.Max(1, page), Math.Clamp(pageSize, 1, MaxPageSize));
|
||||
return Ok(await _search.SearchAsync(UserId, parsed, ct));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -17,17 +17,11 @@ public class SearchService : ISearchService
|
||||
private readonly AppDbContext _db;
|
||||
public SearchService(AppDbContext db) => _db = db;
|
||||
|
||||
/// <summary>Hard upper bound on a user-facing page of results (V-10: DoS via huge pageSize).</summary>
|
||||
public const int MaxPageSize = 200;
|
||||
|
||||
public async Task<PagedResult<EmailSummaryDto>> SearchAsync(Guid userId, SearchRequestDto r, CancellationToken ct = default)
|
||||
{
|
||||
// V-10: clamp pagination at the user-facing chokepoint (covers both the GET
|
||||
// query-string path and the POST body path) so a caller cannot request an
|
||||
// unbounded materialisation. Internal callers (e.g. cleanup target resolution)
|
||||
// do not go through this service, so their larger pages are unaffected.
|
||||
r = r with { Page = Math.Max(1, r.Page), PageSize = Math.Clamp(r.PageSize, 1, MaxPageSize) };
|
||||
|
||||
// NOTE: pagination is clamped at the user-facing trust boundary (SearchController),
|
||||
// NOT here, so internal callers (e.g. CleanupService target resolution, which
|
||||
// legitimately requests large pages) are unaffected. See V-10 fix.
|
||||
var q = _db.Emails.AsNoTracking().Where(e => e.UserId == userId);
|
||||
|
||||
if (!string.IsNullOrWhiteSpace(r.Sender))
|
||||
|
||||
@@ -0,0 +1,51 @@
|
||||
using FluentAssertions;
|
||||
using InboxIntel.Application.Abstractions;
|
||||
using InboxIntel.Application.Search;
|
||||
using InboxIntel.Domain.Entities;
|
||||
using InboxIntel.Infrastructure.Persistence;
|
||||
using InboxIntel.Infrastructure.Search;
|
||||
using Microsoft.EntityFrameworkCore;
|
||||
using Xunit;
|
||||
|
||||
namespace InboxIntel.IntegrationTests;
|
||||
|
||||
/// <summary>
|
||||
/// Regression guard for the V-10 pagination clamp: the clamp must live at the
|
||||
/// user-facing controller boundary, NOT in SearchService — otherwise internal
|
||||
/// callers (CleanupService resolves targets with pageSize 10000) would be silently
|
||||
/// capped, breaking bulk cleanup-by-query. This proves the service itself honours a
|
||||
/// large page.
|
||||
/// </summary>
|
||||
public class SearchPaginationTests
|
||||
{
|
||||
private sealed class FakeCurrentUser : ICurrentUser
|
||||
{
|
||||
public Guid UserId { get; set; }
|
||||
public bool IsAuthenticated => UserId != Guid.Empty;
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public async Task SearchService_does_not_clamp_large_internal_page()
|
||||
{
|
||||
var user = Guid.NewGuid();
|
||||
var opts = new DbContextOptionsBuilder<AppDbContext>()
|
||||
.UseInMemoryDatabase(nameof(SearchService_does_not_clamp_large_internal_page)).Options;
|
||||
|
||||
using (var seed = new AppDbContext(opts, new FakeCurrentUser()))
|
||||
{
|
||||
var sender = new Sender { UserId = user, Address = "sender@example.com", DisplayName = "Sender" };
|
||||
seed.Senders.Add(sender);
|
||||
for (var i = 0; i < 250; i++)
|
||||
seed.Emails.Add(new Email { UserId = user, GmailMessageId = $"m{i}", Subject = $"s{i}", SenderId = sender.Id, Sender = sender });
|
||||
await seed.SaveChangesAsync();
|
||||
}
|
||||
|
||||
using var ctx = new AppDbContext(opts, new FakeCurrentUser { UserId = user });
|
||||
// Mirrors how CleanupService resolves targets: a large page via the parser.
|
||||
var request = GmailQueryParser.Parse(null, page: 1, pageSize: 10_000);
|
||||
var result = await new SearchService(ctx).SearchAsync(user, request);
|
||||
|
||||
result.Items.Should().HaveCount(250); // not capped at 200
|
||||
result.TotalCount.Should().Be(250);
|
||||
}
|
||||
}
|
||||
Reference in New Issue
Block a user