From 3722e818ce79737160882d56304384941933b0b6 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 25 Aug 2026 22:19:02 +0000 Subject: [PATCH 1/4] feat(security): throttle the abusable surface and enforce cart ownership Two controls the storefront was missing entirely. Rate limiting. There was none anywhere in the application, so sign-in, registration, password reset, guest order lookup and order placement were all unbounded. Account lockout caps attempts per account, which does nothing against one attempt each across many accounts and nothing at all on the anonymous endpoints. Named policies now sit on the credential surface, on checkout and on guest lookup, bound from configuration so an operator can tighten one during an incident without a redeploy. UseRateLimiter runs ahead of authentication so a flood is rejected before the app spends work validating anything. There is deliberately no global limiter - a catalogue page issues several requests in a burst, and a global cap would throttle ordinary browsing while adding nothing. The partition key is the caller, and getting that wrong is how a limiter becomes an outage: behind a proxy every request carries the proxy's address, all callers collapse into one bucket, and the first busy minute trips the cap for everybody. X-Forwarded-For is therefore read only when configuration says a trusted proxy is in front, since a client can otherwise forge it and mint unlimited partitions. This suite demonstrated that failure mode on itself. The test server supplies no remote address, so every request shared one partition and the API tests exhausted a realistic budget between themselves - passing alone, failing together. The fixture now raises its own budgets explicitly rather than the production defaults being weakened, and RateLimitingApiTests overrides them back down to prove rejection, the Retry-After hint, and that browsing still works while the auth budget is spent. Cart ownership. Cart.UserId existed and nothing consulted it: read, update, remove and checkout each took a cart id and used it. Only Guid entropy stood between a caller and someone else's basket, and a signed-in user's cart was no better protected than a guest's. CartAccess is now the single rule - a guest cart stays reachable by anyone holding its id, which is what lets a visitor shop before signing in, while a claimed cart admits only its owner. It lives in one place so a new cart operation cannot ship without the check. A refusal answers with the same wording as a missing cart, because a distinct forbidden would confirm the id exists. RequestedBy is a required parameter rather than an optional one with a default: the compiler made all fourteen call sites state their intent, which is the point of it being required. 460 backend tests pass locally against PostgreSQL 16; dotnet format clean; build clean under -warnaserror. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01EA4mmpcb1rcvNntHR1iG6j --- .../Carts/AddItem/AddCartItemHandler.cs | 5 +- .../Carts/CartAccess.cs | 25 +++ .../Carts/GetCart/GetCartHandler.cs | 10 +- .../Carts/RemoveItem/RemoveCartItemHandler.cs | 10 +- .../Carts/UpdateItem/UpdateCartItemHandler.cs | 10 +- .../Checkout/PlaceOrder/CheckoutHandler.cs | 4 +- src/WidgetWorks.WebApi/Auth/AuthEndpoints.cs | 3 +- src/WidgetWorks.WebApi/Carts/CartEndpoints.cs | 12 +- .../Checkout/CheckoutEndpoints.cs | 3 +- .../Orders/OrderEndpoints.cs | 148 +++++++++--------- src/WidgetWorks.WebApi/Program.cs | 6 + .../RateLimiting/ClientAddress.cs | 63 ++++++++ .../RateLimiting/RateLimitOptions.cs | 46 ++++++ .../RateLimiting/RateLimitingExtensions.cs | 73 +++++++++ tests/WidgetWorks.ApiTests/ApiFixture.cs | 44 +++++- .../ClientAddressTests.cs | 91 +++++++++++ .../RateLimitingApiTests.cs | 74 +++++++++ .../WidgetWorks.UnitTests/CartAccessTests.cs | 95 +++++++++++ .../WidgetWorks.UnitTests/CartHandlerTests.cs | 2 +- .../WidgetWorks.UnitTests/OrderQueryTests.cs | 10 +- .../SmallServicesTests.cs | 16 +- 21 files changed, 643 insertions(+), 107 deletions(-) create mode 100644 src/WidgetWorks.Application/Carts/CartAccess.cs create mode 100644 src/WidgetWorks.WebApi/RateLimiting/ClientAddress.cs create mode 100644 src/WidgetWorks.WebApi/RateLimiting/RateLimitOptions.cs create mode 100644 src/WidgetWorks.WebApi/RateLimiting/RateLimitingExtensions.cs create mode 100644 tests/WidgetWorks.ApiTests/ClientAddressTests.cs create mode 100644 tests/WidgetWorks.ApiTests/RateLimitingApiTests.cs create mode 100644 tests/WidgetWorks.UnitTests/CartAccessTests.cs diff --git a/src/WidgetWorks.Application/Carts/AddItem/AddCartItemHandler.cs b/src/WidgetWorks.Application/Carts/AddItem/AddCartItemHandler.cs index 58a8a63..80841f9 100644 --- a/src/WidgetWorks.Application/Carts/AddItem/AddCartItemHandler.cs +++ b/src/WidgetWorks.Application/Carts/AddItem/AddCartItemHandler.cs @@ -40,7 +40,10 @@ public async Task> Handle(AddCartItemCommand command, Cancellat private async Task ResolveCartAsync(Guid? cartId, Guid? userId, CancellationToken ct) { - if (cartId is { } id && await carts.GetAsync(id, ct) is { } existing) + // A supplied id is only honoured when the caller may actually use that cart. A foreign one + // falls through to the caller's own rather than erroring, so an attacker learns nothing about + // whether the id existed and an honest client with a stale id simply carries on. + if (cartId is { } id && await carts.GetAsync(id, ct) is { } existing && CartAccess.IsPermitted(existing, userId)) { return existing; } diff --git a/src/WidgetWorks.Application/Carts/CartAccess.cs b/src/WidgetWorks.Application/Carts/CartAccess.cs new file mode 100644 index 0000000..1e4a9c4 --- /dev/null +++ b/src/WidgetWorks.Application/Carts/CartAccess.cs @@ -0,0 +1,25 @@ +using WidgetWorks.Domain.Carts; + +namespace WidgetWorks.Application.Carts; + +/// +/// The single rule for who may touch a cart. +/// +/// A guest cart carries no owner and is reachable by anyone holding its id — that is what lets a +/// visitor fill a basket before signing in, and it is a capability model: the id is the credential. +/// The moment a cart belongs to a user, only that user may reach it, so signing in genuinely +/// protects the basket instead of leaving it as exposed as a guest's. +/// +/// Kept as one function rather than repeated per handler so a new cart operation cannot quietly ship +/// without the check, and so the rule can be tested on its own. +/// +public static class CartAccess +{ + /// The cart that was loaded by id. + /// The signed-in user, or null for an anonymous caller. + public static bool IsPermitted(Cart cart, Guid? requestedBy) + { + ArgumentNullException.ThrowIfNull(cart); + return cart.UserId is null || cart.UserId == requestedBy; + } +} diff --git a/src/WidgetWorks.Application/Carts/GetCart/GetCartHandler.cs b/src/WidgetWorks.Application/Carts/GetCart/GetCartHandler.cs index 61662e1..ad8684b 100644 --- a/src/WidgetWorks.Application/Carts/GetCart/GetCartHandler.cs +++ b/src/WidgetWorks.Application/Carts/GetCart/GetCartHandler.cs @@ -1,9 +1,10 @@ using WidgetWorks.Application.Abstractions; using WidgetWorks.Domain.Common; +using WidgetWorks.Application.Carts; namespace WidgetWorks.Application.Carts.GetCart; -public sealed record GetCartQuery(Guid CartId); +public sealed record GetCartQuery(Guid CartId, Guid? RequestedBy); public sealed class GetCartHandler(ICartRepository carts, IWidgetRepository widgets) { @@ -15,6 +16,13 @@ public async Task> Handle(GetCartQuery query, CancellationToken return Result.Fail("Cart not found."); } + if (!CartAccess.IsPermitted(cart, query.RequestedBy)) + { + // Deliberately the same answer as a missing cart: telling an unauthorized caller that + // the cart exists would confirm a guess. + return Result.Fail("Cart not found."); + } + return Result.Success(await CartAssembler.BuildAsync(cart, widgets, ct)); } } diff --git a/src/WidgetWorks.Application/Carts/RemoveItem/RemoveCartItemHandler.cs b/src/WidgetWorks.Application/Carts/RemoveItem/RemoveCartItemHandler.cs index 2db1f03..b02084a 100644 --- a/src/WidgetWorks.Application/Carts/RemoveItem/RemoveCartItemHandler.cs +++ b/src/WidgetWorks.Application/Carts/RemoveItem/RemoveCartItemHandler.cs @@ -1,9 +1,10 @@ using WidgetWorks.Application.Abstractions; using WidgetWorks.Domain.Common; +using WidgetWorks.Application.Carts; namespace WidgetWorks.Application.Carts.RemoveItem; -public sealed record RemoveCartItemCommand(Guid CartId, Guid WidgetId); +public sealed record RemoveCartItemCommand(Guid CartId, Guid WidgetId, Guid? RequestedBy); public sealed class RemoveCartItemHandler(ICartRepository carts, IWidgetRepository widgets, TimeProvider clock) { @@ -15,6 +16,13 @@ public async Task> Handle(RemoveCartItemCommand command, Cancel return Result.Fail("Cart not found."); } + if (!CartAccess.IsPermitted(cart, command.RequestedBy)) + { + // Deliberately the same answer as a missing cart: telling an unauthorized caller that + // the cart exists would confirm a guess. + return Result.Fail("Cart not found."); + } + await carts.RemoveItemAsync(command.CartId, command.WidgetId, ct); await carts.TouchAsync(command.CartId, clock.GetUtcNow(), ct); diff --git a/src/WidgetWorks.Application/Carts/UpdateItem/UpdateCartItemHandler.cs b/src/WidgetWorks.Application/Carts/UpdateItem/UpdateCartItemHandler.cs index ea7bca8..07639b1 100644 --- a/src/WidgetWorks.Application/Carts/UpdateItem/UpdateCartItemHandler.cs +++ b/src/WidgetWorks.Application/Carts/UpdateItem/UpdateCartItemHandler.cs @@ -1,10 +1,11 @@ using WidgetWorks.Application.Abstractions; using WidgetWorks.Domain.Common; +using WidgetWorks.Application.Carts; namespace WidgetWorks.Application.Carts.UpdateItem; /// Sets an absolute quantity for a line; a quantity of zero removes it. -public sealed record UpdateCartItemCommand(Guid CartId, Guid WidgetId, int Quantity); +public sealed record UpdateCartItemCommand(Guid CartId, Guid WidgetId, int Quantity, Guid? RequestedBy); public sealed class UpdateCartItemHandler(ICartRepository carts, IWidgetRepository widgets, TimeProvider clock) { @@ -16,6 +17,13 @@ public async Task> Handle(UpdateCartItemCommand command, Cancel return Result.Fail("Cart not found."); } + if (!CartAccess.IsPermitted(cart, command.RequestedBy)) + { + // Deliberately the same answer as a missing cart: telling an unauthorized caller that + // the cart exists would confirm a guess. + return Result.Fail("Cart not found."); + } + var now = clock.GetUtcNow(); if (command.Quantity <= 0) { diff --git a/src/WidgetWorks.Application/Checkout/PlaceOrder/CheckoutHandler.cs b/src/WidgetWorks.Application/Checkout/PlaceOrder/CheckoutHandler.cs index c0ca741..ad6938a 100644 --- a/src/WidgetWorks.Application/Checkout/PlaceOrder/CheckoutHandler.cs +++ b/src/WidgetWorks.Application/Checkout/PlaceOrder/CheckoutHandler.cs @@ -65,8 +65,10 @@ public async Task> Handle(CheckoutCommand command, Cancel } var cart = await carts.GetAsync(command.CartId, ct); - if (cart is null) + if (cart is null || !CartAccess.IsPermitted(cart, command.UserId)) { + // One answer for "no such cart" and "not yours": checking out someone else's basket + // would otherwise disclose its contents in the resulting order. return Fail("Cart not found."); } diff --git a/src/WidgetWorks.WebApi/Auth/AuthEndpoints.cs b/src/WidgetWorks.WebApi/Auth/AuthEndpoints.cs index 94b5c12..8456ded 100644 --- a/src/WidgetWorks.WebApi/Auth/AuthEndpoints.cs +++ b/src/WidgetWorks.WebApi/Auth/AuthEndpoints.cs @@ -6,6 +6,7 @@ using WidgetWorks.Application.Auth.Register; using WidgetWorks.Application.TwoFactor.Challenge; using WidgetWorks.Application.TwoFactor.Recovery; +using WidgetWorks.WebApi.RateLimiting; namespace WidgetWorks.WebApi.Auth; @@ -23,7 +24,7 @@ public static class AuthEndpoints { public static void MapAuthEndpoints(this IEndpointRouteBuilder routes) { - var group = routes.MapGroup("/auth"); + var group = routes.MapGroup("/auth").RequireRateLimiting(RateLimitPolicies.Auth); group.MapPost("/register", async (RegisterCommand command, RegisterHandler handler, CancellationToken ct) => { diff --git a/src/WidgetWorks.WebApi/Carts/CartEndpoints.cs b/src/WidgetWorks.WebApi/Carts/CartEndpoints.cs index 984b67e..c16c005 100644 --- a/src/WidgetWorks.WebApi/Carts/CartEndpoints.cs +++ b/src/WidgetWorks.WebApi/Carts/CartEndpoints.cs @@ -13,9 +13,9 @@ public static void MapCartEndpoints(this IEndpointRouteBuilder routes) { var cart = routes.MapGroup("/cart"); - cart.MapGet("/{cartId:guid}", async (Guid cartId, GetCartHandler handler, CancellationToken ct) => + cart.MapGet("/{cartId:guid}", async (Guid cartId, ClaimsPrincipal principal, GetCartHandler handler, CancellationToken ct) => { - var result = await handler.Handle(new GetCartQuery(cartId), ct); + var result = await handler.Handle(new GetCartQuery(cartId, UserId(principal)), ct); return result.IsSuccess ? Results.Ok(result.Value) : Results.NotFound(new { error = result.Error }); }); @@ -26,15 +26,15 @@ public static void MapCartEndpoints(this IEndpointRouteBuilder routes) return result.IsSuccess ? Results.Ok(result.Value) : Results.BadRequest(new { error = result.Error }); }); - cart.MapPut("/{cartId:guid}/items/{widgetId:guid}", async (Guid cartId, Guid widgetId, UpdateItemRequest body, UpdateCartItemHandler handler, CancellationToken ct) => + cart.MapPut("/{cartId:guid}/items/{widgetId:guid}", async (Guid cartId, Guid widgetId, UpdateItemRequest body, ClaimsPrincipal principal, UpdateCartItemHandler handler, CancellationToken ct) => { - var result = await handler.Handle(new UpdateCartItemCommand(cartId, widgetId, body.Quantity), ct); + var result = await handler.Handle(new UpdateCartItemCommand(cartId, widgetId, body.Quantity, UserId(principal)), ct); return result.IsSuccess ? Results.Ok(result.Value) : Results.BadRequest(new { error = result.Error }); }); - cart.MapDelete("/{cartId:guid}/items/{widgetId:guid}", async (Guid cartId, Guid widgetId, RemoveCartItemHandler handler, CancellationToken ct) => + cart.MapDelete("/{cartId:guid}/items/{widgetId:guid}", async (Guid cartId, Guid widgetId, ClaimsPrincipal principal, RemoveCartItemHandler handler, CancellationToken ct) => { - var result = await handler.Handle(new RemoveCartItemCommand(cartId, widgetId), ct); + var result = await handler.Handle(new RemoveCartItemCommand(cartId, widgetId, UserId(principal)), ct); return result.IsSuccess ? Results.Ok(result.Value) : Results.NotFound(new { error = result.Error }); }); diff --git a/src/WidgetWorks.WebApi/Checkout/CheckoutEndpoints.cs b/src/WidgetWorks.WebApi/Checkout/CheckoutEndpoints.cs index 4475f91..055dd49 100644 --- a/src/WidgetWorks.WebApi/Checkout/CheckoutEndpoints.cs +++ b/src/WidgetWorks.WebApi/Checkout/CheckoutEndpoints.cs @@ -2,6 +2,7 @@ using WidgetWorks.Application.Abstractions; using WidgetWorks.Application.Checkout.PlaceOrder; using WidgetWorks.Application.Checkout.Quote; +using WidgetWorks.WebApi.RateLimiting; namespace WidgetWorks.WebApi.Checkout; @@ -21,7 +22,7 @@ public static void MapCheckoutEndpoints(this IEndpointRouteBuilder routes) body.PaymentToken); var result = await handler.Handle(command, ct); return result.IsSuccess ? Results.Ok(result.Value) : Results.BadRequest(new { error = result.Error }); - }); + }).RequireRateLimiting(RateLimitPolicies.Checkout); var group = routes.MapGroup("/checkout"); diff --git a/src/WidgetWorks.WebApi/Orders/OrderEndpoints.cs b/src/WidgetWorks.WebApi/Orders/OrderEndpoints.cs index 3d5c1de..e800227 100644 --- a/src/WidgetWorks.WebApi/Orders/OrderEndpoints.cs +++ b/src/WidgetWorks.WebApi/Orders/OrderEndpoints.cs @@ -1,73 +1,75 @@ -using System.Security.Claims; -using WidgetWorks.Application.Orders.Admin; -using WidgetWorks.Application.Orders.GetMine; -using WidgetWorks.Application.Orders.ListMine; -using WidgetWorks.Application.Orders.ListRecent; -using WidgetWorks.Application.Orders.Lookup; -using WidgetWorks.Application.Orders.UpdateStatus; -using WidgetWorks.WebApi.Authorization; - -namespace WidgetWorks.WebApi.Orders; - -public static class OrderEndpoints -{ - public static void MapOrderEndpoints(this IEndpointRouteBuilder routes) - { - // Guest order tracking by order number + email (anonymous). - routes.MapGet("/orders/lookup", async (string number, string email, GuestOrderLookupHandler handler, CancellationToken ct) => - { - var result = await handler.Handle(new GuestOrderLookupQuery(number, email), ct); - return result.IsSuccess ? Results.Ok(result.Value) : Results.NotFound(new { error = result.Error }); - }); - - var mine = routes.MapGroup("/orders").RequireAuthorization(); - - mine.MapGet("", async (ClaimsPrincipal principal, ListMyOrdersHandler handler, CancellationToken ct) => - { - if (UserId(principal) is not { } userId) - { - return Results.Unauthorized(); - } - - return Results.Ok(await handler.Handle(new ListMyOrdersQuery(userId), ct)); - }); - - mine.MapGet("/{id:guid}", async (Guid id, ClaimsPrincipal principal, GetMyOrderHandler handler, CancellationToken ct) => - { - if (UserId(principal) is not { } userId) - { - return Results.Unauthorized(); - } - - var result = await handler.Handle(new GetMyOrderQuery(userId, id), ct); - return result.IsSuccess ? Results.Ok(result.Value) : Results.NotFound(new { error = result.Error }); - }); - - // Admin/manager order management (ManageCatalog covers widgets, inventory, and orders). - var admin = routes.MapGroup("/admin/orders").RequireAuthorization(Policies.ManageCatalog); - - // Staff order list. Summary rows only — open one to load its items. - admin.MapGet("/", async (int? limit, ListRecentOrdersHandler handler, CancellationToken ct) => - { - var result = await handler.Handle(new ListRecentOrdersQuery(limit ?? 50), ct); - return Results.Ok(result); - }); - - admin.MapGet("/{id:guid}", async (Guid id, GetOrderByIdHandler handler, CancellationToken ct) => - { - var result = await handler.Handle(new GetOrderByIdQuery(id), ct); - return result.IsSuccess ? Results.Ok(result.Value) : Results.NotFound(new { error = result.Error }); - }); - - admin.MapPost("/{id:guid}/status", async (Guid id, UpdateStatusRequest body, UpdateOrderStatusHandler handler, CancellationToken ct) => - { - var result = await handler.Handle(new UpdateOrderStatusCommand(id, body.Status, body.TrackingNumber), ct); - return result.IsSuccess ? Results.Ok(result.Value) : Results.BadRequest(new { error = result.Error }); - }); - - static Guid? UserId(ClaimsPrincipal principal) - => Guid.TryParse(principal.FindFirst("sub")?.Value, out var id) ? id : null; - } - - public sealed record UpdateStatusRequest(string Status, string? TrackingNumber); -} +using System.Security.Claims; +using WidgetWorks.Application.Orders.Admin; +using WidgetWorks.Application.Orders.GetMine; +using WidgetWorks.Application.Orders.ListMine; +using WidgetWorks.Application.Orders.ListRecent; + +using WidgetWorks.Application.Orders.Lookup; +using WidgetWorks.Application.Orders.UpdateStatus; +using WidgetWorks.WebApi.Authorization; +using WidgetWorks.WebApi.RateLimiting; + +namespace WidgetWorks.WebApi.Orders; + +public static class OrderEndpoints +{ + public static void MapOrderEndpoints(this IEndpointRouteBuilder routes) + { + // Guest order tracking by order number + email (anonymous). + routes.MapGet("/orders/lookup", async (string number, string email, GuestOrderLookupHandler handler, CancellationToken ct) => + { + var result = await handler.Handle(new GuestOrderLookupQuery(number, email), ct); + return result.IsSuccess ? Results.Ok(result.Value) : Results.NotFound(new { error = result.Error }); + }).RequireRateLimiting(RateLimitPolicies.Lookup); + + var mine = routes.MapGroup("/orders").RequireAuthorization(); + + mine.MapGet("", async (ClaimsPrincipal principal, ListMyOrdersHandler handler, CancellationToken ct) => + { + if (UserId(principal) is not { } userId) + { + return Results.Unauthorized(); + } + + return Results.Ok(await handler.Handle(new ListMyOrdersQuery(userId), ct)); + }); + + mine.MapGet("/{id:guid}", async (Guid id, ClaimsPrincipal principal, GetMyOrderHandler handler, CancellationToken ct) => + { + if (UserId(principal) is not { } userId) + { + return Results.Unauthorized(); + } + + var result = await handler.Handle(new GetMyOrderQuery(userId, id), ct); + return result.IsSuccess ? Results.Ok(result.Value) : Results.NotFound(new { error = result.Error }); + }); + + // Admin/manager order management (ManageCatalog covers widgets, inventory, and orders). + var admin = routes.MapGroup("/admin/orders").RequireAuthorization(Policies.ManageCatalog); + + // Staff order list. Summary rows only — open one to load its items. + admin.MapGet("/", async (int? limit, ListRecentOrdersHandler handler, CancellationToken ct) => + { + var result = await handler.Handle(new ListRecentOrdersQuery(limit ?? 50), ct); + return Results.Ok(result); + }); + + admin.MapGet("/{id:guid}", async (Guid id, GetOrderByIdHandler handler, CancellationToken ct) => + { + var result = await handler.Handle(new GetOrderByIdQuery(id), ct); + return result.IsSuccess ? Results.Ok(result.Value) : Results.NotFound(new { error = result.Error }); + }); + + admin.MapPost("/{id:guid}/status", async (Guid id, UpdateStatusRequest body, UpdateOrderStatusHandler handler, CancellationToken ct) => + { + var result = await handler.Handle(new UpdateOrderStatusCommand(id, body.Status, body.TrackingNumber), ct); + return result.IsSuccess ? Results.Ok(result.Value) : Results.BadRequest(new { error = result.Error }); + }); + + static Guid? UserId(ClaimsPrincipal principal) + => Guid.TryParse(principal.FindFirst("sub")?.Value, out var id) ? id : null; + } + + public sealed record UpdateStatusRequest(string Status, string? TrackingNumber); +} diff --git a/src/WidgetWorks.WebApi/Program.cs b/src/WidgetWorks.WebApi/Program.cs index 06c13bc..34ede74 100644 --- a/src/WidgetWorks.WebApi/Program.cs +++ b/src/WidgetWorks.WebApi/Program.cs @@ -17,6 +17,7 @@ using WidgetWorks.WebApi.Orders; using WidgetWorks.WebApi.Payments; using WidgetWorks.WebApi.Security; +using WidgetWorks.WebApi.RateLimiting; using WidgetWorks.WebApi.TwoFactor; var builder = WebApplication.CreateBuilder(args); @@ -24,6 +25,7 @@ builder.Services.AddApplication(); builder.Services.AddInfrastructure(builder.Configuration); builder.Services.AddOpenApi(); +builder.Services.AddWidgetWorksRateLimiting(builder.Configuration); // CORS for the browser SPA (origins from config; sensible localhost defaults for dev). const string SpaCorsPolicy = "spa"; @@ -120,6 +122,10 @@ await users.GetSecurityStampAsync(userId, context.HttpContext.RequestAborted) is } app.UseCors(SpaCorsPolicy); + +// Ahead of authentication on purpose: a throttled request is rejected before the app spends +// work validating credentials, which is what keeps a guessing flood cheap to absorb. +app.UseRateLimiter(); app.UseAuthentication(); app.UseAuthorization(); diff --git a/src/WidgetWorks.WebApi/RateLimiting/ClientAddress.cs b/src/WidgetWorks.WebApi/RateLimiting/ClientAddress.cs new file mode 100644 index 0000000..33c8ac4 --- /dev/null +++ b/src/WidgetWorks.WebApi/RateLimiting/ClientAddress.cs @@ -0,0 +1,63 @@ +using Microsoft.AspNetCore.Http; + +namespace WidgetWorks.WebApi.RateLimiting; + +/// +/// Works out which caller a request belongs to, so throttling partitions by client rather than by +/// process. Kept separate from the limiter wiring because this is the part with rules worth testing: +/// everything else is framework configuration. +/// +public static class ClientAddress +{ + /// Partition used when no address can be determined, so those callers share a budget. + public const string Unknown = "unknown"; + + /// + /// Resolves the partition key for . + /// + /// X-Forwarded-For is a client-supplied header and is read only when + /// says a trusted proxy is in front. Its leftmost entry is + /// the original client; entries to the right are the proxies it passed through. + /// + public static string Resolve(HttpContext context, bool trustForwardedFor) + { + ArgumentNullException.ThrowIfNull(context); + + if (trustForwardedFor) + { + var forwarded = FirstForwardedFor(context.Request.Headers["X-Forwarded-For"]); + if (forwarded is not null) + { + return forwarded; + } + } + + return context.Connection.RemoteIpAddress?.ToString() ?? Unknown; + } + + /// + /// Takes the leftmost address from an X-Forwarded-For chain, which may arrive as one + /// comma-separated header or as several repeated headers. Returns null when nothing usable is + /// present so the caller can fall back to the connection address. + /// + private static string? FirstForwardedFor(IEnumerable headerValues) + { + foreach (var value in headerValues) + { + if (string.IsNullOrWhiteSpace(value)) + { + continue; + } + + foreach (var candidate in value.Split(',', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries)) + { + if (candidate.Length > 0) + { + return candidate; + } + } + } + + return null; + } +} diff --git a/src/WidgetWorks.WebApi/RateLimiting/RateLimitOptions.cs b/src/WidgetWorks.WebApi/RateLimiting/RateLimitOptions.cs new file mode 100644 index 0000000..8f53258 --- /dev/null +++ b/src/WidgetWorks.WebApi/RateLimiting/RateLimitOptions.cs @@ -0,0 +1,46 @@ +namespace WidgetWorks.WebApi.RateLimiting; + +/// +/// Throttling budgets, bound from the RateLimiting configuration section so an operator can +/// tighten a limit during an incident without a redeploy. Defaults are deliberately generous enough +/// that a real customer never meets them and tight enough that scripted abuse does. +/// +public sealed class RateLimitOptions +{ + /// + /// Whether an X-Forwarded-For header may be believed when identifying the caller. + /// + /// This is the setting that decides whether the limiter works at all behind a reverse proxy. + /// Left false while hosted behind one, every request appears to originate from the proxy, all + /// callers collapse into a single partition, and the limiter turns into a global cap that the + /// first busy minute trips for everybody — a self-inflicted outage. Set true only when a proxy + /// you control is guaranteed to be in front, because a client can otherwise forge the header + /// and mint itself unlimited partitions. + /// + public bool TrustForwardedFor { get; set; } + + /// Sign-in, registration and password-reset requests. + public RateLimitBudget Auth { get; set; } = new() { PermitLimit = 20, WindowSeconds = 60 }; + + /// Order placement. Guards card testing and the inventory-reservation abuse path. + public RateLimitBudget Checkout { get; set; } = new() { PermitLimit = 8, WindowSeconds = 60 }; + + /// Guest order lookup, which confirms an order number against an email. + public RateLimitBudget Lookup { get; set; } = new() { PermitLimit = 10, WindowSeconds = 60 }; +} + +/// A fixed-window budget: requests per . +public sealed class RateLimitBudget +{ + public int PermitLimit { get; set; } = 10; + + public int WindowSeconds { get; set; } = 60; + + /// + /// Coerces the configured pair into a usable window. A zero or negative value from configuration + /// would otherwise throw deep inside the limiter at first request rather than at startup, so it + /// falls back to the property default instead of taking the process down. + /// + public (int Permits, TimeSpan Window) Resolve() + => (PermitLimit > 0 ? PermitLimit : 10, TimeSpan.FromSeconds(WindowSeconds > 0 ? WindowSeconds : 60)); +} diff --git a/src/WidgetWorks.WebApi/RateLimiting/RateLimitingExtensions.cs b/src/WidgetWorks.WebApi/RateLimiting/RateLimitingExtensions.cs new file mode 100644 index 0000000..0313f4a --- /dev/null +++ b/src/WidgetWorks.WebApi/RateLimiting/RateLimitingExtensions.cs @@ -0,0 +1,73 @@ +using System.Globalization; +using System.Threading.RateLimiting; +using Microsoft.AspNetCore.RateLimiting; + +namespace WidgetWorks.WebApi.RateLimiting; + +/// Named throttling policies, referenced by endpoints the way authorization policies are. +public static class RateLimitPolicies +{ + /// Sign-in, registration, password reset — the credential-guessing surface. + public const string Auth = "auth"; + + /// Order placement. + public const string Checkout = "checkout"; + + /// Guest order lookup. + public const string Lookup = "lookup"; +} + +public static class RateLimitingExtensions +{ + /// + /// Registers the throttling policies. + /// + /// Only the endpoints that are both anonymous and abusable carry a policy. There is deliberately + /// no global limiter: a catalogue page issues several requests in a burst, so a global cap would + /// throttle ordinary browsing while doing nothing an endpoint policy does not already do. + /// + public static IServiceCollection AddWidgetWorksRateLimiting(this IServiceCollection services, IConfiguration configuration) + { + var options = new RateLimitOptions(); + configuration.GetSection("RateLimiting").Bind(options); + services.AddSingleton(options); + + services.AddRateLimiter(limiter => + { + limiter.AddPolicy(RateLimitPolicies.Auth, ctx => Partition(ctx, options, options.Auth)); + limiter.AddPolicy(RateLimitPolicies.Checkout, ctx => Partition(ctx, options, options.Checkout)); + limiter.AddPolicy(RateLimitPolicies.Lookup, ctx => Partition(ctx, options, options.Lookup)); + + limiter.OnRejected = async (context, ct) => + { + context.HttpContext.Response.StatusCode = StatusCodes.Status429TooManyRequests; + + // Tell the caller when to come back. A well-behaved client backs off instead of + // retrying into the wall, and an honest one that hit the limit by accident recovers + // without a support ticket. + if (context.Lease.TryGetMetadata(MetadataName.RetryAfter, out var retryAfter)) + { + context.HttpContext.Response.Headers.RetryAfter = + ((int)retryAfter.TotalSeconds).ToString(CultureInfo.InvariantCulture); + } + + await context.HttpContext.Response.WriteAsJsonAsync( + new { error = "Too many requests. Please wait a moment and try again." }, ct); + }; + }); + + return services; + } + + /// + /// One fixed window per caller. Fixed rather than sliding because the budgets here are small and + /// the extra per-partition state a sliding window keeps is not worth paying for at this size. + /// + private static RateLimitPartition Partition(HttpContext context, RateLimitOptions options, RateLimitBudget budget) + { + var (permits, window) = budget.Resolve(); + return RateLimitPartition.GetFixedWindowLimiter( + ClientAddress.Resolve(context, options.TrustForwardedFor), + _ => new FixedWindowRateLimiterOptions { PermitLimit = permits, Window = window }); + } +} diff --git a/tests/WidgetWorks.ApiTests/ApiFixture.cs b/tests/WidgetWorks.ApiTests/ApiFixture.cs index 352addc..35aa0ee 100644 --- a/tests/WidgetWorks.ApiTests/ApiFixture.cs +++ b/tests/WidgetWorks.ApiTests/ApiFixture.cs @@ -29,6 +29,7 @@ public sealed class ApiFixture : IAsyncLifetime private const string SigningKey = "test-signing-key-api-suite-0123456789abcdef"; private string _adminConnectionString = DefaultAdmin; + private string _connectionString = DefaultAdmin; public const string AdminEmail = "api-admin@widgetworks.test"; public const string ManagerEmail = "api-manager@widgetworks.test"; @@ -57,10 +58,24 @@ public async Task InitializeAsync() // UseSetting lands in host configuration, which minimal hosting folds into // builder.Configuration BEFORE Program's own code reads it; ConfigureAppConfiguration // callbacks would run too late for the Jwt options Program binds during startup. - Factory = new WebApplicationFactory().WithWebHostBuilder(host => + _connectionString = connectionString; + Factory = BuildFactory(); + + // First client boots the host: migrations run and the demo catalog is seeded. + using var client = Factory.CreateClient(); + var health = await client.GetAsync("/health"); + health.EnsureSuccessStatusCode(); + } + + /// + /// The host every test shares. Settings go through UseSetting because minimal hosting folds + /// host configuration into builder.Configuration before Program reads it. + /// + private WebApplicationFactory BuildFactory(params (string Key, string Value)[] overrides) + => new WebApplicationFactory().WithWebHostBuilder(host => { host.UseEnvironment(Environments.Production); // no OpenAPI/Scalar noise in tests - host.UseSetting("ConnectionStrings:WidgetWorks", connectionString); + host.UseSetting("ConnectionStrings:WidgetWorks", _connectionString); host.UseSetting("Jwt:SigningKey", SigningKey); host.UseSetting("Seed:DemoAdminEmail", AdminEmail); host.UseSetting("Seed:DemoAdminPassword", Password); @@ -68,13 +83,28 @@ public async Task InitializeAsync() host.UseSetting("Seed:DemoManagerPassword", Password); host.UseSetting("Seed:DemoCustomerEmail", CustomerEmail); host.UseSetting("Seed:DemoCustomerPassword", Password); + + // Every request from the test server arrives with no remote address, so all of them + // share one throttling partition and the suite would exhaust a realistic budget + // between tests. Raised here rather than weakened in the production defaults -- + // RateLimitingApiTests overrides these back down to prove the limiter really rejects. + host.UseSetting("RateLimiting:Auth:PermitLimit", "100000"); + host.UseSetting("RateLimiting:Checkout:PermitLimit", "100000"); + host.UseSetting("RateLimiting:Lookup:PermitLimit", "100000"); + + foreach (var (key, value) in overrides) + { + host.UseSetting(key, value); + } }); - // First client boots the host: migrations run and the demo catalog is seeded. - using var client = Factory.CreateClient(); - var health = await client.GetAsync("/health"); - health.EnsureSuccessStatusCode(); - } + /// + /// A second host on the same database with configuration overrides applied — for tests that + /// need to exercise behaviour the shared host deliberately turns down, such as throttling. + /// The caller owns the returned factory and should dispose it. + /// + public WebApplicationFactory FactoryWith(params (string Key, string Value)[] overrides) + => BuildFactory(overrides); public async Task DisposeAsync() { diff --git a/tests/WidgetWorks.ApiTests/ClientAddressTests.cs b/tests/WidgetWorks.ApiTests/ClientAddressTests.cs new file mode 100644 index 0000000..06f2058 --- /dev/null +++ b/tests/WidgetWorks.ApiTests/ClientAddressTests.cs @@ -0,0 +1,91 @@ +using System.Net; +using Microsoft.AspNetCore.Http; +using WidgetWorks.WebApi.RateLimiting; +using Xunit; + +namespace WidgetWorks.ApiTests; + +/// +/// Which caller a request is attributed to. This is the whole correctness of throttling: get the +/// partition key wrong and the limiter either caps everybody together or caps nobody at all. +/// +public class ClientAddressTests +{ + private static HttpContext Request(string? remoteIp, params string[] forwardedFor) + { + var context = new DefaultHttpContext(); + if (remoteIp is not null) + { + context.Connection.RemoteIpAddress = IPAddress.Parse(remoteIp); + } + + if (forwardedFor.Length > 0) + { + context.Request.Headers["X-Forwarded-For"] = forwardedFor; + } + + return context; + } + + [Fact] + public void The_connection_address_identifies_the_caller_by_default() + => Assert.Equal("203.0.113.7", ClientAddress.Resolve(Request("203.0.113.7"), trustForwardedFor: false)); + + [Fact] + public void A_forwarded_header_is_ignored_unless_a_proxy_is_trusted() + { + // Anyone can send this header. Believing it without a proxy in front would let a caller + // mint a fresh partition per request and opt out of throttling entirely. + var context = Request("203.0.113.7", "198.51.100.9"); + + Assert.Equal("203.0.113.7", ClientAddress.Resolve(context, trustForwardedFor: false)); + } + + [Fact] + public void A_trusted_proxy_reveals_the_original_client() + { + var context = Request("10.0.0.1", "198.51.100.9"); + + Assert.Equal("198.51.100.9", ClientAddress.Resolve(context, trustForwardedFor: true)); + } + + [Fact] + public void The_leftmost_entry_in_a_forwarded_chain_is_the_client() + { + // Proxies append, so the client is first and every hop after it is infrastructure. + var context = Request("10.0.0.1", "198.51.100.9, 10.0.0.8, 10.0.0.1"); + + Assert.Equal("198.51.100.9", ClientAddress.Resolve(context, trustForwardedFor: true)); + } + + [Fact] + public void A_chain_split_across_repeated_headers_is_read_the_same_way() + { + var context = Request("10.0.0.1", "198.51.100.9", "10.0.0.8"); + + Assert.Equal("198.51.100.9", ClientAddress.Resolve(context, trustForwardedFor: true)); + } + + [Theory] + [InlineData("")] + [InlineData(" ")] + [InlineData(",")] + public void An_empty_forwarded_header_falls_back_to_the_connection(string headerValue) + { + var context = Request("203.0.113.7", headerValue); + + Assert.Equal("203.0.113.7", ClientAddress.Resolve(context, trustForwardedFor: true)); + } + + [Fact] + public void Callers_with_no_determinable_address_share_one_budget() + { + // Failing closed: unattributable traffic is throttled together rather than exempted. + Assert.Equal(ClientAddress.Unknown, ClientAddress.Resolve(Request(null), trustForwardedFor: false)); + Assert.Equal(ClientAddress.Unknown, ClientAddress.Resolve(Request(null), trustForwardedFor: true)); + } + + [Fact] + public void A_null_context_is_a_programming_error_not_a_silent_pass() + => Assert.Throws(() => ClientAddress.Resolve(null!, trustForwardedFor: false)); +} diff --git a/tests/WidgetWorks.ApiTests/RateLimitingApiTests.cs b/tests/WidgetWorks.ApiTests/RateLimitingApiTests.cs new file mode 100644 index 0000000..b0f60dd --- /dev/null +++ b/tests/WidgetWorks.ApiTests/RateLimitingApiTests.cs @@ -0,0 +1,74 @@ +using System.Net; +using System.Net.Http.Json; +using Xunit; + +namespace WidgetWorks.ApiTests; + +/// +/// Proves the throttling actually engages. The shared fixture raises every budget so the suite can +/// run, which would otherwise leave this control shipped but unexercised — so these tests stand up +/// their own host with a budget of two and drive straight past it. +/// +[Collection(ApiCollection.Name)] +public sealed class RateLimitingApiTests(ApiFixture fixture) +{ + [Fact] + public async Task A_flood_of_sign_in_attempts_is_rejected_once_the_budget_is_spent() + { + using var factory = fixture.FactoryWith(("RateLimiting:Auth:PermitLimit", "2")); + using var client = factory.CreateClient(); + + var attempt = new { Email = "nobody@widgetworks.test", Password = "WrongPassword!1" }; + + // Two are allowed. Whether they succeed is beside the point: a wrong password is still a + // request, which is exactly why throttling has to sit in front of authentication. + var first = await client.PostAsJsonAsync("/auth/login", attempt); + var second = await client.PostAsJsonAsync("/auth/login", attempt); + Assert.NotEqual(HttpStatusCode.TooManyRequests, first.StatusCode); + Assert.NotEqual(HttpStatusCode.TooManyRequests, second.StatusCode); + + var third = await client.PostAsJsonAsync("/auth/login", attempt); + + Assert.Equal(HttpStatusCode.TooManyRequests, third.StatusCode); + } + + [Fact] + public async Task A_rejected_caller_is_told_how_long_to_wait() + { + using var factory = fixture.FactoryWith(("RateLimiting:Auth:PermitLimit", "1")); + using var client = factory.CreateClient(); + + var attempt = new { Email = "nobody@widgetworks.test", Password = "WrongPassword!1" }; + await client.PostAsJsonAsync("/auth/login", attempt); + var rejected = await client.PostAsJsonAsync("/auth/login", attempt); + + Assert.Equal(HttpStatusCode.TooManyRequests, rejected.StatusCode); + + // Retry-After turns a wall into a queue: a well-behaved client backs off instead of + // hammering, and an honest caller who tripped the limit recovers without asking anyone. + Assert.True(rejected.Headers.TryGetValues("Retry-After", out var retryAfter)); + Assert.True(int.TryParse(retryAfter!.First(), out var seconds) && seconds > 0); + + var body = await rejected.Content.ReadFromJsonAsync(); + Assert.False(string.IsNullOrWhiteSpace(body!.Error)); + } + + [Fact] + public async Task Throttling_is_scoped_to_the_policy_not_the_whole_api() + { + using var factory = fixture.FactoryWith(("RateLimiting:Auth:PermitLimit", "1")); + using var client = factory.CreateClient(); + + var attempt = new { Email = "nobody@widgetworks.test", Password = "WrongPassword!1" }; + await client.PostAsJsonAsync("/auth/login", attempt); + var rejected = await client.PostAsJsonAsync("/auth/login", attempt); + Assert.Equal(HttpStatusCode.TooManyRequests, rejected.StatusCode); + + // Browsing must keep working while the auth budget is spent. A global limiter would fail + // this, which is the reason there isn't one. + var browsing = await client.GetAsync("/catalog/widgets?pageSize=5"); + Assert.Equal(HttpStatusCode.OK, browsing.StatusCode); + } + + private sealed record ErrorBody(string Error); +} diff --git a/tests/WidgetWorks.UnitTests/CartAccessTests.cs b/tests/WidgetWorks.UnitTests/CartAccessTests.cs new file mode 100644 index 0000000..96e1bb9 --- /dev/null +++ b/tests/WidgetWorks.UnitTests/CartAccessTests.cs @@ -0,0 +1,95 @@ +using WidgetWorks.Application.Carts; +using WidgetWorks.Application.Carts.GetCart; +using WidgetWorks.Domain.Carts; +using WidgetWorks.UnitTests.Fakes; +using Xunit; + +namespace WidgetWorks.UnitTests; + +/// +/// The cart authorization rule and its effect at the boundary. A cart id is a capability, so these +/// tests pin the line between "anyone holding the id" (guest) and "only the owner" (claimed). +/// +public class CartAccessTests +{ + private static Cart CartOwnedBy(Guid? owner) => new() { Id = Guid.NewGuid(), UserId = owner }; + + [Fact] + public void A_guest_cart_is_reachable_by_anyone_holding_its_id() + { + var cart = CartOwnedBy(null); + + // This is what lets a visitor shop before signing in. + Assert.True(CartAccess.IsPermitted(cart, null)); + Assert.True(CartAccess.IsPermitted(cart, Guid.NewGuid())); + } + + [Fact] + public void A_claimed_cart_is_reachable_only_by_its_owner() + { + var owner = Guid.NewGuid(); + var cart = CartOwnedBy(owner); + + Assert.True(CartAccess.IsPermitted(cart, owner)); + Assert.False(CartAccess.IsPermitted(cart, Guid.NewGuid())); + Assert.False(CartAccess.IsPermitted(cart, null)); + } + + [Fact] + public async Task Reading_someone_elses_cart_is_refused_and_says_only_not_found() + { + var carts = new InMemoryCartRepository(); + var widgets = new InMemoryWidgetRepository(); + var owner = Guid.NewGuid(); + var cart = await carts.CreateAsync(owner, CancellationToken.None); + + var result = await new GetCartHandler(carts, widgets) + .Handle(new GetCartQuery(cart.Id, Guid.NewGuid()), CancellationToken.None); + + Assert.False(result.IsSuccess); + // The wording matters as much as the refusal: a distinct "forbidden" would confirm that a + // cart with this id exists, which is exactly what a guesser wants to learn. + Assert.Equal("Cart not found.", result.Error); + } + + [Fact] + public async Task An_anonymous_caller_cannot_reach_a_claimed_cart() + { + var carts = new InMemoryCartRepository(); + var widgets = new InMemoryWidgetRepository(); + var cart = await carts.CreateAsync(Guid.NewGuid(), CancellationToken.None); + + var result = await new GetCartHandler(carts, widgets) + .Handle(new GetCartQuery(cart.Id, null), CancellationToken.None); + + Assert.False(result.IsSuccess); + } + + [Fact] + public async Task The_owner_can_still_read_their_own_cart() + { + var carts = new InMemoryCartRepository(); + var widgets = new InMemoryWidgetRepository(); + var owner = Guid.NewGuid(); + var cart = await carts.CreateAsync(owner, CancellationToken.None); + + var result = await new GetCartHandler(carts, widgets) + .Handle(new GetCartQuery(cart.Id, owner), CancellationToken.None); + + Assert.True(result.IsSuccess); + } + + [Fact] + public async Task A_guest_cart_still_works_for_an_anonymous_shopper() + { + var carts = new InMemoryCartRepository(); + var widgets = new InMemoryWidgetRepository(); + var cart = await carts.CreateAsync(null, CancellationToken.None); + + var result = await new GetCartHandler(carts, widgets) + .Handle(new GetCartQuery(cart.Id, null), CancellationToken.None); + + // The whole guest checkout flow depends on this staying true. + Assert.True(result.IsSuccess); + } +} diff --git a/tests/WidgetWorks.UnitTests/CartHandlerTests.cs b/tests/WidgetWorks.UnitTests/CartHandlerTests.cs index d72e9ea..6874e0e 100644 --- a/tests/WidgetWorks.UnitTests/CartHandlerTests.cs +++ b/tests/WidgetWorks.UnitTests/CartHandlerTests.cs @@ -75,7 +75,7 @@ public async Task Update_to_zero_removes_line() var created = await add.Handle(new AddCartItemCommand(null, null, widget.Id, 2), CancellationToken.None); var update = new UpdateCartItemHandler(carts, widgets, Clock()); - var result = await update.Handle(new UpdateCartItemCommand(created.Value!.Id, widget.Id, 0), CancellationToken.None); + var result = await update.Handle(new UpdateCartItemCommand(created.Value!.Id, widget.Id, 0, null), CancellationToken.None); Assert.True(result.IsSuccess); Assert.Empty(result.Value!.Items); diff --git a/tests/WidgetWorks.UnitTests/OrderQueryTests.cs b/tests/WidgetWorks.UnitTests/OrderQueryTests.cs index 261b8e9..a24b98f 100644 --- a/tests/WidgetWorks.UnitTests/OrderQueryTests.cs +++ b/tests/WidgetWorks.UnitTests/OrderQueryTests.cs @@ -316,7 +316,7 @@ private static CartCtx CartSetup() public async Task Get_cart_prices_the_lines() { var c = CartSetup(); - var result = await new GetCartHandler(c.Carts, c.Widgets).Handle(new GetCartQuery(c.Cart.Id), CancellationToken.None); + var result = await new GetCartHandler(c.Carts, c.Widgets).Handle(new GetCartQuery(c.Cart.Id, null), CancellationToken.None); Assert.True(result.IsSuccess); Assert.Equal(2, result.Value!.ItemCount); @@ -327,7 +327,7 @@ public async Task Get_cart_prices_the_lines() public async Task Get_cart_fails_for_an_unknown_cart() { var c = CartSetup(); - var result = await new GetCartHandler(c.Carts, c.Widgets).Handle(new GetCartQuery(Guid.NewGuid()), CancellationToken.None); + var result = await new GetCartHandler(c.Carts, c.Widgets).Handle(new GetCartQuery(Guid.NewGuid(), null), CancellationToken.None); Assert.False(result.IsSuccess); Assert.Equal("Cart not found.", result.Error); @@ -340,7 +340,7 @@ public async Task Removing_an_item_empties_the_cart_and_touches_it() var clock = new FakeTimeProvider(Now.AddHours(1)); var result = await new RemoveCartItemHandler(c.Carts, c.Widgets, clock) - .Handle(new RemoveCartItemCommand(c.Cart.Id, c.Widget.Id), CancellationToken.None); + .Handle(new RemoveCartItemCommand(c.Cart.Id, c.Widget.Id, null), CancellationToken.None); Assert.True(result.IsSuccess); Assert.Equal(0, result.Value!.ItemCount); @@ -354,7 +354,7 @@ public async Task Removing_an_item_that_is_not_in_the_cart_is_a_no_op() var c = CartSetup(); var result = await new RemoveCartItemHandler(c.Carts, c.Widgets, new FakeTimeProvider(Now)) - .Handle(new RemoveCartItemCommand(c.Cart.Id, Guid.NewGuid()), CancellationToken.None); + .Handle(new RemoveCartItemCommand(c.Cart.Id, Guid.NewGuid(), null), CancellationToken.None); Assert.True(result.IsSuccess); Assert.Equal(2, result.Value!.ItemCount); @@ -366,7 +366,7 @@ public async Task Removing_from_an_unknown_cart_fails() var c = CartSetup(); var result = await new RemoveCartItemHandler(c.Carts, c.Widgets, new FakeTimeProvider(Now)) - .Handle(new RemoveCartItemCommand(Guid.NewGuid(), c.Widget.Id), CancellationToken.None); + .Handle(new RemoveCartItemCommand(Guid.NewGuid(), c.Widget.Id, null), CancellationToken.None); Assert.False(result.IsSuccess); Assert.Equal("Cart not found.", result.Error); diff --git a/tests/WidgetWorks.UnitTests/SmallServicesTests.cs b/tests/WidgetWorks.UnitTests/SmallServicesTests.cs index dfcdf3d..e26b57c 100644 --- a/tests/WidgetWorks.UnitTests/SmallServicesTests.cs +++ b/tests/WidgetWorks.UnitTests/SmallServicesTests.cs @@ -162,7 +162,7 @@ public async Task Setting_a_quantity_of_zero_removes_the_line() { var c = Setup(); - var result = await Handler(c).Handle(new UpdateCartItemCommand(c.Cart.Id, c.Widget.Id, 0), CancellationToken.None); + var result = await Handler(c).Handle(new UpdateCartItemCommand(c.Cart.Id, c.Widget.Id, 0, null), CancellationToken.None); Assert.True(result.IsSuccess); Assert.Empty(result.Value!.Items); @@ -173,7 +173,7 @@ public async Task A_negative_quantity_removes_the_line_rather_than_erroring() { var c = Setup(); - var result = await Handler(c).Handle(new UpdateCartItemCommand(c.Cart.Id, c.Widget.Id, -3), CancellationToken.None); + var result = await Handler(c).Handle(new UpdateCartItemCommand(c.Cart.Id, c.Widget.Id, -3, null), CancellationToken.None); Assert.True(result.IsSuccess); Assert.Empty(result.Value!.Items); @@ -184,7 +184,7 @@ public async Task Asking_for_more_than_exists_caps_at_what_is_available() { var c = Setup(available: 5); - var result = await Handler(c).Handle(new UpdateCartItemCommand(c.Cart.Id, c.Widget.Id, 99), CancellationToken.None); + var result = await Handler(c).Handle(new UpdateCartItemCommand(c.Cart.Id, c.Widget.Id, 99, null), CancellationToken.None); Assert.True(result.IsSuccess); Assert.Equal(5, result.Value!.Items.Single().Quantity); @@ -195,7 +195,7 @@ public async Task An_out_of_stock_widget_is_refused_with_a_reason() { var c = Setup(available: 0); - var result = await Handler(c).Handle(new UpdateCartItemCommand(c.Cart.Id, c.Widget.Id, 1), CancellationToken.None); + var result = await Handler(c).Handle(new UpdateCartItemCommand(c.Cart.Id, c.Widget.Id, 1, null), CancellationToken.None); Assert.False(result.IsSuccess); Assert.Equal("This widget is out of stock.", result.Error); @@ -206,7 +206,7 @@ public async Task A_hidden_widget_cannot_be_added_to_a_cart() { var c = Setup(active: false); - var result = await Handler(c).Handle(new UpdateCartItemCommand(c.Cart.Id, c.Widget.Id, 1), CancellationToken.None); + var result = await Handler(c).Handle(new UpdateCartItemCommand(c.Cart.Id, c.Widget.Id, 1, null), CancellationToken.None); Assert.False(result.IsSuccess); Assert.Equal("Widget not found.", result.Error); @@ -217,7 +217,7 @@ public async Task An_unknown_widget_is_refused() { var c = Setup(); - var result = await Handler(c).Handle(new UpdateCartItemCommand(c.Cart.Id, Guid.NewGuid(), 1), CancellationToken.None); + var result = await Handler(c).Handle(new UpdateCartItemCommand(c.Cart.Id, Guid.NewGuid(), 1, null), CancellationToken.None); Assert.False(result.IsSuccess); Assert.Equal("Widget not found.", result.Error); @@ -228,7 +228,7 @@ public async Task An_unknown_cart_is_refused() { var c = Setup(); - var result = await Handler(c).Handle(new UpdateCartItemCommand(Guid.NewGuid(), c.Widget.Id, 1), CancellationToken.None); + var result = await Handler(c).Handle(new UpdateCartItemCommand(Guid.NewGuid(), c.Widget.Id, 1, null), CancellationToken.None); Assert.False(result.IsSuccess); Assert.Equal("Cart not found.", result.Error); @@ -239,7 +239,7 @@ public async Task Updating_a_quantity_touches_the_cart() { var c = Setup(); - await Handler(c).Handle(new UpdateCartItemCommand(c.Cart.Id, c.Widget.Id, 3), CancellationToken.None); + await Handler(c).Handle(new UpdateCartItemCommand(c.Cart.Id, c.Widget.Id, 3, null), CancellationToken.None); Assert.Equal(Now, c.Carts.Store[c.Cart.Id].UpdatedAt); } From 83757bf59fc3610be5377c377cfb9836eed56d01 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 25 Aug 2026 22:24:36 +0000 Subject: [PATCH 2/4] fix(payments): let the row decide settlement, not the caller Payment status was written unconditionally, and ConfirmPaymentHandler guarded it by reading the order, checking the status, then writing. That is check-then-act: two concurrent deliveries of the same webhook both read AwaitingPayment, both pass the check, and both proceed. The consequences were not symmetric. A duplicate success sent the customer a second receipt. A duplicate failure ran the compensation twice and decremented quantity_reserved by twice the order quantity - and because the guard only stops the column going negative, the second release quietly consumed stock still held by a different order. An event arriving out of order was worse still: a late success could mark an order Paid whose stock had already gone back on sale. The three payment writes are now compare-and-set. Each carries the statuses it may apply from and returns whether it won: MarkAwaitingPaymentAsync from Pending MarkPaidAsync from Pending or AwaitingPayment MarkPaymentFailedAsync from Pending or AwaitingPayment MarkPaymentFailedAsync tests its own result inside the transaction and releases the reservation only when it won, so compensation runs exactly once no matter how many times the event is delivered. ConfirmPaymentHandler now treats the returned bool as the decision rather than the status read it does first, so only the winner emails a receipt. CheckoutHandler discards the result with an explicit _ and a comment: those calls act on an order created moments earlier that is still Pending, so they cannot decline. The contended path is the webhook, and that is where the answer is acted on. This also settles the separate event-id dedupe that was on the list. A dedupe table would restate the same guarantee at a weaker layer - the order's own status is the idempotency key, it is durable, and it is the invariant that actually matters. Adding a table to assert it twice would be ceremony, not defence. Three integration tests pin the behaviour: a repeated failure releases stock once, a late settlement cannot overwrite a failed order, and a stale failure cannot overwrite a paid one or strip a reservation the customer is owed. 465 backend tests pass locally against PostgreSQL 16. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01EA4mmpcb1rcvNntHR1iG6j --- .../Abstractions/IOrderRepository.cs | 21 ++++++-- .../ConfirmPayment/ConfirmPaymentHandler.cs | 17 +++++- .../Checkout/PlaceOrder/CheckoutHandler.cs | 9 ++-- .../Persistence/OrderRepository.cs | 50 ++++++++++++----- .../OrderRepositoryTests.cs | 54 +++++++++++++++++++ tests/WidgetWorks.UnitTests/Fakes.cs | 33 +++++++++--- 6 files changed, 156 insertions(+), 28 deletions(-) diff --git a/src/WidgetWorks.Application/Abstractions/IOrderRepository.cs b/src/WidgetWorks.Application/Abstractions/IOrderRepository.cs index 927ec5d..624a726 100644 --- a/src/WidgetWorks.Application/Abstractions/IOrderRepository.cs +++ b/src/WidgetWorks.Application/Abstractions/IOrderRepository.cs @@ -7,13 +7,24 @@ public interface IOrderRepository /// Atomically inserts the order and reserves stock. Returns false (rolled back) if any line is short. Task TryPlaceAsync(Order order, CancellationToken ct); - /// Records the provider + reference and parks the order in AwaitingPayment (async settlement). - Task MarkAwaitingPaymentAsync(Guid orderId, string provider, string reference, DateTimeOffset now, CancellationToken ct); + /// + /// Records the provider + reference and parks the order in AwaitingPayment (async settlement). + /// Returns false when the order had already moved on, so the write was declined. + /// + Task MarkAwaitingPaymentAsync(Guid orderId, string provider, string reference, DateTimeOffset now, CancellationToken ct); - Task MarkPaidAsync(Guid orderId, string provider, string reference, DateTimeOffset now, CancellationToken ct); + /// + /// Settles the order. Returns false when it was no longer awaiting settlement — a duplicate or + /// out-of-order provider event, which must not overwrite a decided order. + /// + Task MarkPaidAsync(Guid orderId, string provider, string reference, DateTimeOffset now, CancellationToken ct); - /// Marks the order failed and releases its inventory reservations. - Task MarkPaymentFailedAsync(Order order, string reason, DateTimeOffset now, CancellationToken ct); + /// + /// Marks the order failed and releases its inventory reservations, atomically. Returns false + /// when the order had already moved on; the caller must treat that as "someone else handled it" + /// rather than retrying, because the stock has already been dealt with. + /// + Task MarkPaymentFailedAsync(Order order, string reason, DateTimeOffset now, CancellationToken ct); /// /// Persists a fulfilment transition together with the inventory movement it implies, in one diff --git a/src/WidgetWorks.Application/Checkout/ConfirmPayment/ConfirmPaymentHandler.cs b/src/WidgetWorks.Application/Checkout/ConfirmPayment/ConfirmPaymentHandler.cs index 725189b..9da1842 100644 --- a/src/WidgetWorks.Application/Checkout/ConfirmPayment/ConfirmPaymentHandler.cs +++ b/src/WidgetWorks.Application/Checkout/ConfirmPayment/ConfirmPaymentHandler.cs @@ -38,7 +38,14 @@ public async Task> Handle(ConfirmPaymentCommand command, Cancella if (command.Type == PaymentEventType.Succeeded) { - await orders.MarkPaidAsync(order.Id, order.PaymentProvider ?? command.Provider, command.Reference, now, ct); + // The status check above is a courtesy; this is the decision. Two deliveries of the + // same event both pass that check, and the database picks exactly one winner. Only the + // winner sends a receipt, so a retried webhook cannot email the customer twice. + if (!await orders.MarkPaidAsync(order.Id, order.PaymentProvider ?? command.Provider, command.Reference, now, ct)) + { + return Result.Success(order.Status); + } + order.Status = OrderStatus.Paid; try @@ -58,7 +65,13 @@ public async Task> Handle(ConfirmPaymentCommand command, Cancella return Result.Success(OrderStatus.Paid); } - await orders.MarkPaymentFailedAsync(order, "Payment failed.", now, ct); + // Same contract on the failure path, and it matters more here: the losing caller must not + // release the reservation a second time. + if (!await orders.MarkPaymentFailedAsync(order, "Payment failed.", now, ct)) + { + return Result.Success(order.Status); + } + order.Status = OrderStatus.PaymentFailed; return Result.Success(OrderStatus.PaymentFailed); } diff --git a/src/WidgetWorks.Application/Checkout/PlaceOrder/CheckoutHandler.cs b/src/WidgetWorks.Application/Checkout/PlaceOrder/CheckoutHandler.cs index ad6938a..251a431 100644 --- a/src/WidgetWorks.Application/Checkout/PlaceOrder/CheckoutHandler.cs +++ b/src/WidgetWorks.Application/Checkout/PlaceOrder/CheckoutHandler.cs @@ -95,7 +95,10 @@ public async Task> Handle(CheckoutCommand command, Cancel if (payment.Status == PaymentStatus.Declined) { - await orders.MarkPaymentFailedAsync(order, payment.Error ?? "Payment failed.", clock.GetUtcNow(), ct); + // Discarded deliberately: this order was created moments ago and is still Pending, so + // the compare-and-set cannot decline. A webhook arriving later is the contended path, + // and ConfirmPaymentHandler is where the answer is acted on. + _ = await orders.MarkPaymentFailedAsync(order, payment.Error ?? "Payment failed.", clock.GetUtcNow(), ct); return Fail(payment.Error ?? "Payment failed."); } @@ -105,7 +108,7 @@ public async Task> Handle(CheckoutCommand command, Cancel { // Async settlement (redirect/BNPL): keep the reservation, park the order, and let the // provider webhook finalize it. The receipt email is sent on confirmation, not here. - await orders.MarkAwaitingPaymentAsync(order.Id, payment.Provider, reference, clock.GetUtcNow(), ct); + _ = await orders.MarkAwaitingPaymentAsync(order.Id, payment.Provider, reference, clock.GetUtcNow(), ct); order.Status = OrderStatus.AwaitingPayment; await carts.DeleteAsync(cart.Id, ct); @@ -115,7 +118,7 @@ public async Task> Handle(CheckoutCommand command, Cancel } // Synchronous success. - await orders.MarkPaidAsync(order.Id, payment.Provider, reference, clock.GetUtcNow(), ct); + _ = await orders.MarkPaidAsync(order.Id, payment.Provider, reference, clock.GetUtcNow(), ct); order.Status = OrderStatus.Paid; await carts.DeleteAsync(cart.Id, ct); diff --git a/src/WidgetWorks.Infrastructure/Persistence/OrderRepository.cs b/src/WidgetWorks.Infrastructure/Persistence/OrderRepository.cs index 6abbd5a..e942e78 100644 --- a/src/WidgetWorks.Infrastructure/Persistence/OrderRepository.cs +++ b/src/WidgetWorks.Infrastructure/Persistence/OrderRepository.cs @@ -75,31 +75,56 @@ public async Task TryPlaceAsync(Order order, CancellationToken ct) } } - public async Task MarkAwaitingPaymentAsync(Guid orderId, string provider, string reference, DateTimeOffset now, CancellationToken ct) + /// + /// The statuses a settlement outcome may still be applied from. Anything else means the order + /// has already moved on and a late or repeated event must not touch it. + /// + private static readonly string[] AwaitingSettlement = [OrderStatus.Pending, OrderStatus.AwaitingPayment]; + + public async Task MarkAwaitingPaymentAsync(Guid orderId, string provider, string reference, DateTimeOffset now, CancellationToken ct) { using var db = await factory.OpenAsync(ct); - await db.ExecuteAsync( - "update orders set status = @Status, payment_provider = @Provider, payment_reference = @Reference, updated_at = @Now where id = @Id", - new { Id = orderId, Status = OrderStatus.AwaitingPayment, Provider = provider, Reference = reference, Now = now }); + var affected = await db.ExecuteAsync(new CommandDefinition( + @"update orders set status = @Status, payment_provider = @Provider, payment_reference = @Reference, updated_at = @Now + where id = @Id and status = @Expected", + new { Id = orderId, Status = OrderStatus.AwaitingPayment, Provider = provider, Reference = reference, Now = now, Expected = OrderStatus.Pending }, + cancellationToken: ct)); + return affected == 1; } - public async Task MarkPaidAsync(Guid orderId, string provider, string reference, DateTimeOffset now, CancellationToken ct) + public async Task MarkPaidAsync(Guid orderId, string provider, string reference, DateTimeOffset now, CancellationToken ct) { using var db = await factory.OpenAsync(ct); - await db.ExecuteAsync( - "update orders set status = @Status, payment_provider = @Provider, payment_reference = @Reference, updated_at = @Now where id = @Id", - new { Id = orderId, Status = OrderStatus.Paid, Provider = provider, Reference = reference, Now = now }); + var affected = await db.ExecuteAsync(new CommandDefinition( + @"update orders set status = @Status, payment_provider = @Provider, payment_reference = @Reference, updated_at = @Now + where id = @Id and status = any(@Expected)", + new { Id = orderId, Status = OrderStatus.Paid, Provider = provider, Reference = reference, Now = now, Expected = AwaitingSettlement }, + cancellationToken: ct)); + return affected == 1; } - public async Task MarkPaymentFailedAsync(Order order, string reason, DateTimeOffset now, CancellationToken ct) + public async Task MarkPaymentFailedAsync(Order order, string reason, DateTimeOffset now, CancellationToken ct) { using var db = await factory.OpenAsync(ct); using var tx = db.BeginTransaction(); try { - await db.ExecuteAsync(new CommandDefinition( - "update orders set status = @Status, updated_at = @Now where id = @Id", - new { Id = order.Id, Status = OrderStatus.PaymentFailed, Now = now }, tx, cancellationToken: ct)); + // Compare-and-set inside the transaction, so the row decides who wins rather than the + // caller. Two concurrent deliveries of the same failure both pass an application-level + // status check; only one can win this update, and only the winner releases the + // reservation. Without it a redelivered webhook decrements quantity_reserved a second + // time and eats stock still held by a different order. + var applied = await db.ExecuteAsync(new CommandDefinition( + @"update orders set status = @Status, updated_at = @Now + where id = @Id and status = any(@Expected)", + new { Id = order.Id, Status = OrderStatus.PaymentFailed, Now = now, Expected = AwaitingSettlement }, + tx, cancellationToken: ct)); + + if (applied != 1) + { + tx.Rollback(); + return false; + } foreach (var item in order.Items) { @@ -108,6 +133,7 @@ await db.ExecuteAsync(new CommandDefinition( } tx.Commit(); + return true; } catch { diff --git a/tests/WidgetWorks.IntegrationTests/OrderRepositoryTests.cs b/tests/WidgetWorks.IntegrationTests/OrderRepositoryTests.cs index c558462..2cfb7f0 100644 --- a/tests/WidgetWorks.IntegrationTests/OrderRepositoryTests.cs +++ b/tests/WidgetWorks.IntegrationTests/OrderRepositoryTests.cs @@ -246,6 +246,60 @@ public async Task Updating_status_stores_the_tracking_number() Assert.Equal("1Z-TRACK", stored.TrackingNumber); } + [Fact] + public async Task A_repeated_payment_failure_releases_the_reservation_only_once() + { + var widget = await GivenWidget(onHand: 10); + var order = OrderFor(widget, 3); + await Orders.TryPlaceAsync(order, CancellationToken.None); + + var first = await Orders.MarkPaymentFailedAsync(order, "declined", Now, CancellationToken.None); + var second = await Orders.MarkPaymentFailedAsync(order, "declined", Now, CancellationToken.None); + + Assert.True(first); + // The row, not the caller, decides. A redelivered webhook is declined rather than + // decrementing quantity_reserved a second time and eating another order's stock. + Assert.False(second); + + var after = await Widgets.GetByIdAsync(widget.Id, CancellationToken.None); + Assert.Equal(10, after!.QuantityOnHand); + Assert.Equal(0, after.QuantityReserved); + } + + [Fact] + public async Task A_late_settlement_cannot_overwrite_an_order_that_already_failed() + { + var widget = await GivenWidget(onHand: 10); + var order = OrderFor(widget, 2); + await Orders.TryPlaceAsync(order, CancellationToken.None); + await Orders.MarkPaymentFailedAsync(order, "declined", Now, CancellationToken.None); + + var settled = await Orders.MarkPaidAsync(order.Id, "Mock", "ref-1", Now.AddMinutes(1), CancellationToken.None); + + // Marking it paid here would claim money for an order whose stock is already back on sale. + Assert.False(settled); + var stored = await Orders.GetByIdAsync(order.Id, CancellationToken.None); + Assert.Equal(OrderStatus.PaymentFailed, stored!.Status); + } + + [Fact] + public async Task A_settled_order_cannot_be_failed_by_a_stale_event() + { + var widget = await GivenWidget(onHand: 10); + var order = OrderFor(widget, 2); + await Orders.TryPlaceAsync(order, CancellationToken.None); + await Orders.MarkPaidAsync(order.Id, "Mock", "ref-2", Now, CancellationToken.None); + + var failed = await Orders.MarkPaymentFailedAsync(order, "stale", Now.AddMinutes(1), CancellationToken.None); + + Assert.False(failed); + var stored = await Orders.GetByIdAsync(order.Id, CancellationToken.None); + Assert.Equal(OrderStatus.Paid, stored!.Status); + // The reservation must survive: the goods are sold and still owed to this order. + var after = await Widgets.GetByIdAsync(widget.Id, CancellationToken.None); + Assert.Equal(2, after!.QuantityReserved); + } + [Fact] public async Task Shipping_turns_the_reservation_into_a_real_decrement() { diff --git a/tests/WidgetWorks.UnitTests/Fakes.cs b/tests/WidgetWorks.UnitTests/Fakes.cs index d9520f1..a4ec58b 100644 --- a/tests/WidgetWorks.UnitTests/Fakes.cs +++ b/tests/WidgetWorks.UnitTests/Fakes.cs @@ -199,29 +199,47 @@ public Task TryPlaceAsync(Order order, CancellationToken ct) return Task.FromResult(true); } - public Task MarkAwaitingPaymentAsync(Guid orderId, string provider, string reference, DateTimeOffset now, CancellationToken ct) + public Task MarkAwaitingPaymentAsync(Guid orderId, string provider, string reference, DateTimeOffset now, CancellationToken ct) { var order = Orders.First(o => o.Id == orderId); + if (order.Status != OrderStatus.Pending) + { + return Task.FromResult(false); + } + order.Status = OrderStatus.AwaitingPayment; order.PaymentProvider = provider; order.PaymentReference = reference; order.UpdatedAt = now; - return Task.CompletedTask; + return Task.FromResult(true); } - public Task MarkPaidAsync(Guid orderId, string provider, string reference, DateTimeOffset now, CancellationToken ct) + public Task MarkPaidAsync(Guid orderId, string provider, string reference, DateTimeOffset now, CancellationToken ct) { var order = Orders.First(o => o.Id == orderId); + if (!AwaitingSettlement(order.Status)) + { + return Task.FromResult(false); + } + order.Status = OrderStatus.Paid; order.PaymentProvider = provider; order.PaymentReference = reference; order.UpdatedAt = now; - return Task.CompletedTask; + return Task.FromResult(true); } - public Task MarkPaymentFailedAsync(Order order, string reason, DateTimeOffset now, CancellationToken ct) + public Task MarkPaymentFailedAsync(Order order, string reason, DateTimeOffset now, CancellationToken ct) { var stored = Orders.First(o => o.Id == order.Id); + + // Mirrors the repository's compare-and-set: a second delivery of the same failure is + // declined, so the reservation is released exactly once. + if (!AwaitingSettlement(stored.Status)) + { + return Task.FromResult(false); + } + stored.Status = OrderStatus.PaymentFailed; stored.UpdatedAt = now; foreach (var item in order.Items) @@ -232,9 +250,12 @@ public Task MarkPaymentFailedAsync(Order order, string reason, DateTimeOffset no } } - return Task.CompletedTask; + return Task.FromResult(true); } + private static bool AwaitingSettlement(string status) + => status is OrderStatus.Pending or OrderStatus.AwaitingPayment; + public Task UpdateStatusAsync(Order order, DateTimeOffset now, CancellationToken ct) { var stored = Orders.First(o => o.Id == order.Id); From 6f53ded2ddead03c5d37f6596d149467d6359937 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 25 Aug 2026 22:28:27 +0000 Subject: [PATCH 3/4] feat(checkout): return stock held by orders that never settled Checkout reserves stock the moment an order is placed. When settlement is asynchronous the order parks in AwaitingPayment and waits for a provider webhook. Nothing handled the webhook that never arrives - a provider outage, a dropped delivery, a shopper who closed the tab at the bank's redirect - so that reservation was held for the life of the database and the only way back was an administrator editing inventory counts by hand. That is also the engine of the abuse path: order placement is anonymous by design, so without a sweep an unauthenticated script could drive availability to zero across the catalogue and leave it there. Throttling raises the cost of doing that; this is what makes the damage temporary. The policy is a plain handler. It finds orders unsettled past a threshold and releases them through MarkPaymentFailedAsync, which already sets the status and returns the stock in one transaction, and whose compare-and-set means a webhook landing during a sweep cannot double-release. PaymentFailed is the honest description of a settlement that never came, so no new status was needed. Scheduling is a separate BackgroundService that owns nothing but the clock. It takes a fresh scope per tick, because the repositories are Scoped and a singleton holding one would pin a connection for the life of the process. It waits a full interval before the first pass, since startup is the worst moment to add database work. A failed sweep is logged and the loop continues: a transient database blip must not silently stop reclaiming stock until someone restarts the host. Cancellation during shutdown ends the loop quietly rather than surfacing as an error, and is checked between orders so a large batch stops cleanly. Every knob is configuration - window, interval, batch size, and an off switch for a host that should not run background work. The batch cap means a backlog is worked through over several passes rather than one long transaction. Seven tests cover the policy against a fake clock, so none of them wait: stale orders release, orders inside the window are left alone, settled orders are never swept, a second sweep is a no-op rather than a second decrement, the batch cap holds, the oldest go first, and a cancelled sweep stops. 475 backend tests pass locally against PostgreSQL 16. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01EA4mmpcb1rcvNntHR1iG6j --- .../Abstractions/IOrderRepository.cs | 9 + .../ReleaseStaleReservationsHandler.cs | 99 ++++++++++ .../DependencyInjection.cs | 2 + .../Persistence/OrderRepository.cs | 21 +++ .../Hosting/ReservationSweeper.cs | 78 ++++++++ src/WidgetWorks.WebApi/Program.cs | 10 + tests/WidgetWorks.UnitTests/Fakes.cs | 7 + .../ReleaseStaleReservationsTests.cs | 174 ++++++++++++++++++ 8 files changed, 400 insertions(+) create mode 100644 src/WidgetWorks.Application/Checkout/ReleaseStale/ReleaseStaleReservationsHandler.cs create mode 100644 src/WidgetWorks.WebApi/Hosting/ReservationSweeper.cs create mode 100644 tests/WidgetWorks.UnitTests/ReleaseStaleReservationsTests.cs diff --git a/src/WidgetWorks.Application/Abstractions/IOrderRepository.cs b/src/WidgetWorks.Application/Abstractions/IOrderRepository.cs index 624a726..7419c5d 100644 --- a/src/WidgetWorks.Application/Abstractions/IOrderRepository.cs +++ b/src/WidgetWorks.Application/Abstractions/IOrderRepository.cs @@ -34,6 +34,15 @@ public interface IOrderRepository /// Task UpdateStatusAsync(Order order, DateTimeOffset now, CancellationToken ct); + /// + /// Orders still parked in AwaitingPayment since before , items loaded + /// so their reservations can be released. A settlement webhook that never arrives would + /// otherwise hold that stock forever. + /// + /// Caps one sweep, so a large backlog is worked through over several passes + /// rather than in one long transaction. + Task> GetStaleAwaitingPaymentAsync(DateTimeOffset cutoff, int limit, CancellationToken ct); + Task GetByIdAsync(Guid id, CancellationToken ct); /// Finds an order by the payment provider + reference stored at authorization time (webhook correlation). diff --git a/src/WidgetWorks.Application/Checkout/ReleaseStale/ReleaseStaleReservationsHandler.cs b/src/WidgetWorks.Application/Checkout/ReleaseStale/ReleaseStaleReservationsHandler.cs new file mode 100644 index 0000000..762391e --- /dev/null +++ b/src/WidgetWorks.Application/Checkout/ReleaseStale/ReleaseStaleReservationsHandler.cs @@ -0,0 +1,99 @@ +using Microsoft.Extensions.Logging; +using WidgetWorks.Application.Abstractions; + +namespace WidgetWorks.Application.Checkout.ReleaseStale; + +/// +/// How long an unsettled order may hold stock, and how often to look. Bound from the +/// Reservations configuration section. +/// +public sealed class ReservationOptions +{ + /// + /// How long an order may sit in AwaitingPayment before its stock is handed back. + /// + /// This is a trade, not a tuning knob: too short and a slow but honest bank redirect loses a + /// customer's basket; too long and abandoned or abusive orders hold the catalogue hostage. + /// Fifteen minutes is longer than any interactive redirect and short enough that a shopper who + /// returns to an out-of-stock item is rare. + /// + public int ExpireAfterMinutes { get; set; } = 15; + + /// How often the sweep runs. + public int SweepIntervalMinutes { get; set; } = 5; + + /// + /// Most orders released in one pass. A backlog is worked through over several sweeps rather + /// than in one long pass, so a bad day cannot turn into a slow transaction storm. + /// + public int BatchSize { get; set; } = 100; + + /// Turns the sweep off — for a host that should not run background work. + public bool Enabled { get; set; } = true; +} + +/// +/// Hands back stock held by orders whose payment never settled. +/// +/// Checkout reserves stock the moment an order is placed. When settlement is asynchronous the order +/// parks in AwaitingPayment and waits for a provider webhook. If that webhook never arrives — a +/// provider outage, a dropped delivery, a shopper who closed the tab at the bank's redirect — the +/// reservation is held forever, and without this sweep the only route back is an administrator +/// editing inventory counts by hand. +/// +/// The release itself reuses : it already sets +/// the status and releases the reservation in one transaction, and its compare-and-set means a +/// webhook landing at the same moment as a sweep cannot both act. PaymentFailed is also the honest +/// description of a settlement that never came, so no new status is needed. +/// +/// Scheduling deliberately lives elsewhere. This type is a plain handler so the policy can be tested +/// without a timer, a host, or a clock that really waits. +/// +public sealed class ReleaseStaleReservationsHandler( + IOrderRepository orders, + TimeProvider clock, + ReservationOptions options, + ILogger logger) +{ + /// Runs one sweep. Returns how many orders had their stock returned to sale. + public async Task Handle(CancellationToken ct) + { + var now = clock.GetUtcNow(); + var cutoff = now.AddMinutes(-Math.Max(1, options.ExpireAfterMinutes)); + var stale = await orders.GetStaleAwaitingPaymentAsync(cutoff, Math.Max(1, options.BatchSize), ct); + + var released = 0; + foreach (var order in stale) + { + // Checked between orders rather than only at the top: a shutdown midway through a large + // batch should stop cleanly, and each release is already committed on its own. + ct.ThrowIfCancellationRequested(); + + if (await orders.MarkPaymentFailedAsync(order, "Payment was not completed in time.", now, ct)) + { + released++; + } + else + { + // The order moved on between the query and the write — almost always a webhook + // that landed first, which is the good outcome. Recorded at debug because it is + // expected, not a fault. + logger.LogDebug( + "Order {OrderNumber} settled before the sweep reached it; nothing released.", + order.OrderNumber); + } + } + + if (released > 0) + { + // Worth a real log line: it means customers or scripts are abandoning payments, and a + // rising count is the signal that something upstream is wrong. + logger.LogInformation( + "Released stock held by {Released} order(s) unsettled since before {Cutoff:o}.", + released, + cutoff); + } + + return released; + } +} diff --git a/src/WidgetWorks.Application/DependencyInjection.cs b/src/WidgetWorks.Application/DependencyInjection.cs index 387aae8..e0b0795 100644 --- a/src/WidgetWorks.Application/DependencyInjection.cs +++ b/src/WidgetWorks.Application/DependencyInjection.cs @@ -19,6 +19,7 @@ using WidgetWorks.Application.Catalog.Update; using WidgetWorks.Application.Checkout.ConfirmPayment; using WidgetWorks.Application.Checkout.PlaceOrder; +using WidgetWorks.Application.Checkout.ReleaseStale; using WidgetWorks.Application.Checkout.Quote; using WidgetWorks.Application.Orders.Admin; using WidgetWorks.Application.Orders.GetMine; @@ -67,6 +68,7 @@ public static IServiceCollection AddApplication(this IServiceCollection services services.AddScoped(); services.AddScoped(); services.AddScoped(); + services.AddScoped(); services.AddScoped(); services.AddScoped(); services.AddScoped(); diff --git a/src/WidgetWorks.Infrastructure/Persistence/OrderRepository.cs b/src/WidgetWorks.Infrastructure/Persistence/OrderRepository.cs index e942e78..294de97 100644 --- a/src/WidgetWorks.Infrastructure/Persistence/OrderRepository.cs +++ b/src/WidgetWorks.Infrastructure/Persistence/OrderRepository.cs @@ -185,6 +185,27 @@ await db.ExecuteAsync(new CommandDefinition( } } + public async Task> GetStaleAwaitingPaymentAsync(DateTimeOffset cutoff, int limit, CancellationToken ct) + { + using var db = await factory.OpenAsync(ct); + var orders = (await db.QueryAsync(new CommandDefinition( + $@"select {OrderColumns} from orders + where status = @Status and updated_at < @Cutoff + order by updated_at + limit @Limit", + new { Status = OrderStatus.AwaitingPayment, Cutoff = cutoff, Limit = limit }, + cancellationToken: ct))).ToList(); + + foreach (var order in orders) + { + order.Items = (await db.QueryAsync(new CommandDefinition( + $"select {ItemColumns} from order_items where order_id = @id order by name", + new { id = order.Id }, cancellationToken: ct))).ToList(); + } + + return orders; + } + public async Task GetByIdAsync(Guid id, CancellationToken ct) { using var db = await factory.OpenAsync(ct); diff --git a/src/WidgetWorks.WebApi/Hosting/ReservationSweeper.cs b/src/WidgetWorks.WebApi/Hosting/ReservationSweeper.cs new file mode 100644 index 0000000..33611bf --- /dev/null +++ b/src/WidgetWorks.WebApi/Hosting/ReservationSweeper.cs @@ -0,0 +1,78 @@ +using WidgetWorks.Application.Checkout.ReleaseStale; + +namespace WidgetWorks.WebApi.Hosting; + +/// +/// Runs on a timer. +/// +/// This type is only the clock. All of the policy — how stale is stale, how many to take, what +/// releasing means — belongs to the handler, which is why that part can be tested without waiting +/// for a timer to tick. +/// +public sealed class ReservationSweeper( + IServiceScopeFactory scopes, + ReservationOptions options, + ILogger logger) : BackgroundService +{ + protected override async Task ExecuteAsync(CancellationToken stoppingToken) + { + if (!options.Enabled) + { + logger.LogInformation("Reservation sweep is disabled by configuration."); + return; + } + + var interval = TimeSpan.FromMinutes(Math.Max(1, options.SweepIntervalMinutes)); + logger.LogInformation( + "Reservation sweep running every {Interval}, releasing orders unsettled for {ExpireAfter} minutes.", + interval, + options.ExpireAfterMinutes); + + using var timer = new PeriodicTimer(interval); + + // Waits a full interval before the first pass on purpose. Startup is the worst moment to add + // database work, and nothing is so urgent that it cannot wait one interval. + while (await SafeWaitAsync(timer, stoppingToken)) + { + // A scope per tick, because the repositories are registered Scoped. A long-lived + // singleton holding a scoped dependency is the captive-dependency bug: it would pin one + // connection for the lifetime of the process. + using var scope = scopes.CreateScope(); + var handler = scope.ServiceProvider.GetRequiredService(); + + try + { + await handler.Handle(stoppingToken); + } + catch (OperationCanceledException) when (stoppingToken.IsCancellationRequested) + { + // Shutdown, not a fault. Leave the loop quietly. + break; + } + catch (Exception ex) + { + // One bad sweep must not end the loop: a transient database blip would otherwise + // silently stop reclaiming stock for the lifetime of the process, and nothing would + // report it. Logged, then the next tick tries again. + logger.LogError(ex, "Reservation sweep failed; the next pass will retry."); + } + } + } + + /// + /// Waits for the next tick, reporting false once the host is stopping. Wrapped because + /// throws on cancellation, and a cancelled wait + /// during shutdown is an ordinary ending rather than an error worth surfacing. + /// + private static async Task SafeWaitAsync(PeriodicTimer timer, CancellationToken ct) + { + try + { + return await timer.WaitForNextTickAsync(ct); + } + catch (OperationCanceledException) + { + return false; + } + } +} diff --git a/src/WidgetWorks.WebApi/Program.cs b/src/WidgetWorks.WebApi/Program.cs index 34ede74..0591570 100644 --- a/src/WidgetWorks.WebApi/Program.cs +++ b/src/WidgetWorks.WebApi/Program.cs @@ -17,6 +17,8 @@ using WidgetWorks.WebApi.Orders; using WidgetWorks.WebApi.Payments; using WidgetWorks.WebApi.Security; +using WidgetWorks.Application.Checkout.ReleaseStale; +using WidgetWorks.WebApi.Hosting; using WidgetWorks.WebApi.RateLimiting; using WidgetWorks.WebApi.TwoFactor; @@ -27,6 +29,14 @@ builder.Services.AddOpenApi(); builder.Services.AddWidgetWorksRateLimiting(builder.Configuration); +// Stock held by an order whose payment never settles is returned to sale on a timer. Options are +// bound here so the sweep can be retuned, or turned off for a host that should not run background +// work, without a code change. +var reservationOptions = new ReservationOptions(); +builder.Configuration.GetSection("Reservations").Bind(reservationOptions); +builder.Services.AddSingleton(reservationOptions); +builder.Services.AddHostedService(); + // CORS for the browser SPA (origins from config; sensible localhost defaults for dev). const string SpaCorsPolicy = "spa"; var corsOrigins = (builder.Configuration["Cors:AllowedOrigins"] ?? "http://localhost:3000,http://localhost:5173") diff --git a/tests/WidgetWorks.UnitTests/Fakes.cs b/tests/WidgetWorks.UnitTests/Fakes.cs index a4ec58b..95718a3 100644 --- a/tests/WidgetWorks.UnitTests/Fakes.cs +++ b/tests/WidgetWorks.UnitTests/Fakes.cs @@ -286,6 +286,13 @@ public Task UpdateStatusAsync(Order order, DateTimeOffset now, CancellationToken return Task.CompletedTask; } + public Task> GetStaleAwaitingPaymentAsync(DateTimeOffset cutoff, int limit, CancellationToken ct) + => Task.FromResult>(Orders + .Where(o => o.Status == OrderStatus.AwaitingPayment && o.UpdatedAt < cutoff) + .OrderBy(o => o.UpdatedAt) + .Take(limit) + .ToList()); + public Task GetByIdAsync(Guid id, CancellationToken ct) => Task.FromResult(Orders.FirstOrDefault(o => o.Id == id)); diff --git a/tests/WidgetWorks.UnitTests/ReleaseStaleReservationsTests.cs b/tests/WidgetWorks.UnitTests/ReleaseStaleReservationsTests.cs new file mode 100644 index 0000000..0d119f3 --- /dev/null +++ b/tests/WidgetWorks.UnitTests/ReleaseStaleReservationsTests.cs @@ -0,0 +1,174 @@ +using Microsoft.Extensions.Logging.Abstractions; +using Microsoft.Extensions.Time.Testing; +using WidgetWorks.Application.Checkout.ReleaseStale; +using WidgetWorks.Domain.Catalog; +using WidgetWorks.Domain.Orders; +using WidgetWorks.UnitTests.Fakes; +using Xunit; + +namespace WidgetWorks.UnitTests; + +/// +/// The sweep that stops an unsettled order holding stock forever. Time is faked, so these prove the +/// policy — what counts as stale, what gets released — without a timer or a real wait. +/// +public class ReleaseStaleReservationsTests +{ + private static readonly DateTimeOffset Now = new(2026, 1, 1, 12, 0, 0, TimeSpan.Zero); + + private sealed record Harness( + InMemoryOrderRepository Orders, + InMemoryWidgetRepository Widgets, + ReleaseStaleReservationsHandler Handler, + Guid WidgetId); + + private static Harness Build(ReservationOptions? options = null) + { + var widgets = new InMemoryWidgetRepository(); + var orders = new InMemoryOrderRepository(widgets); + var widgetId = Guid.NewGuid(); + widgets.Store[widgetId] = new Widget + { + Id = widgetId, + Sku = "WW-001", + Name = "Standard Widget Block Cobalt", + Price = 9.99m, + IsActive = true, + QuantityOnHand = 20, + QuantityReserved = 0, + }; + + var handler = new ReleaseStaleReservationsHandler( + orders, + new FakeTimeProvider(Now), + options ?? new ReservationOptions(), + NullLogger.Instance); + + return new Harness(orders, widgets, handler, widgetId); + } + + /// Places an order holding stock and parks it in AwaitingPayment as of . + private static async Task GivenUnsettledOrder(Harness h, int quantity, DateTimeOffset updatedAt) + { + var order = new Order + { + Id = Guid.NewGuid(), + OrderNumber = "WW-" + Guid.NewGuid().ToString("N")[..6], + Email = "shopper@widgetworks.test", + Status = OrderStatus.Pending, + Total = 9.99m * quantity, + }; + order.Items.Add(new OrderItem + { + Id = Guid.NewGuid(), + WidgetId = h.WidgetId, + Sku = "WW-001", + Name = "Standard Widget Block Cobalt", + UnitPrice = 9.99m, + Quantity = quantity, + LineSubtotal = 9.99m * quantity, + }); + + await h.Orders.TryPlaceAsync(order, CancellationToken.None); + await h.Orders.MarkAwaitingPaymentAsync(order.Id, "Mock", "ref", updatedAt, CancellationToken.None); + return order; + } + + [Fact] + public async Task An_order_unsettled_past_the_threshold_gives_its_stock_back() + { + var h = Build(); + await GivenUnsettledOrder(h, quantity: 4, updatedAt: Now.AddMinutes(-30)); + + Assert.Equal(4, h.Widgets.Store[h.WidgetId].QuantityReserved); + + var released = await h.Handler.Handle(CancellationToken.None); + + Assert.Equal(1, released); + Assert.Equal(0, h.Widgets.Store[h.WidgetId].QuantityReserved); + // The goods never shipped, so on-hand is untouched and all 20 are sellable again. + Assert.Equal(20, h.Widgets.Store[h.WidgetId].QuantityOnHand); + } + + [Fact] + public async Task An_order_still_inside_the_window_is_left_alone() + { + var h = Build(); + await GivenUnsettledOrder(h, quantity: 4, updatedAt: Now.AddMinutes(-5)); + + var released = await h.Handler.Handle(CancellationToken.None); + + // A slow but honest bank redirect must not lose the customer's basket. + Assert.Equal(0, released); + Assert.Equal(4, h.Widgets.Store[h.WidgetId].QuantityReserved); + } + + [Fact] + public async Task A_settled_order_is_never_swept() + { + var h = Build(); + var order = await GivenUnsettledOrder(h, quantity: 4, updatedAt: Now.AddMinutes(-30)); + await h.Orders.MarkPaidAsync(order.Id, "Mock", "ref", Now.AddMinutes(-29), CancellationToken.None); + + var released = await h.Handler.Handle(CancellationToken.None); + + Assert.Equal(0, released); + // Paid stock is owed to the customer and must stay reserved until it ships. + Assert.Equal(4, h.Widgets.Store[h.WidgetId].QuantityReserved); + } + + [Fact] + public async Task A_sweep_is_safe_to_run_twice() + { + var h = Build(); + await GivenUnsettledOrder(h, quantity: 4, updatedAt: Now.AddMinutes(-30)); + + var first = await h.Handler.Handle(CancellationToken.None); + var second = await h.Handler.Handle(CancellationToken.None); + + Assert.Equal(1, first); + // Nothing left to do, and crucially no second decrement of a reservation already released. + Assert.Equal(0, second); + Assert.Equal(0, h.Widgets.Store[h.WidgetId].QuantityReserved); + } + + [Fact] + public async Task One_pass_takes_no_more_than_the_batch_size() + { + var h = Build(new ReservationOptions { BatchSize = 2 }); + for (var i = 0; i < 5; i++) + { + await GivenUnsettledOrder(h, quantity: 1, updatedAt: Now.AddMinutes(-30 - i)); + } + + var released = await h.Handler.Handle(CancellationToken.None); + + // A backlog is worked through over several sweeps rather than one long pass. + Assert.Equal(2, released); + Assert.Equal(3, h.Widgets.Store[h.WidgetId].QuantityReserved); + } + + [Fact] + public async Task The_oldest_unsettled_orders_are_released_first() + { + var h = Build(new ReservationOptions { BatchSize = 1 }); + var oldest = await GivenUnsettledOrder(h, quantity: 1, updatedAt: Now.AddHours(-3)); + await GivenUnsettledOrder(h, quantity: 1, updatedAt: Now.AddMinutes(-20)); + + await h.Handler.Handle(CancellationToken.None); + + Assert.Equal(OrderStatus.PaymentFailed, h.Orders.Orders.Single(o => o.Id == oldest.Id).Status); + } + + [Fact] + public async Task A_cancelled_sweep_stops_rather_than_finishing_the_batch() + { + var h = Build(); + await GivenUnsettledOrder(h, quantity: 1, updatedAt: Now.AddMinutes(-30)); + + using var cancelled = new CancellationTokenSource(); + await cancelled.CancelAsync(); + + await Assert.ThrowsAnyAsync(() => h.Handler.Handle(cancelled.Token)); + } +} From 7116828c3d1a8c624a265f33ee312b826cb03e69 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 25 Aug 2026 22:36:45 +0000 Subject: [PATCH 4/4] refactor(catalog): narrow and order in the query, not the browser Category and sort were applied in the browser to a single fetched page. That made PAGE_SIZE a hidden ceiling on the whole catalogue: past it, products fell off the end of every category shelf without any error, and a sort ordered only whatever happened to be on that page - so "price, low to high" meant "the cheapest of the first hundred". The failure was silent in both directions, which is what made it worth fixing rather than raising a number. WidgetQuery now carries Category and Sort. Category is a second AND against name and sku, so a search inside a category means both rather than either. Sort is mapped through a fixed set of order-by clauses and never interpolated, which is the only safe way to put a caller's string near an ORDER BY; an unrecognised value falls back to the default rather than reaching the database. Every clause ends with name so paging is total - without a tiebreak, rows with equal prices can appear on two pages and never on a third. Featured moved with it. It used to push sold-out items to the end of the page in the browser, which quietly meant "in stock on this page first"; the SQL applies it across the whole matching set. The listing and its count now share one parameter object, so the two can no longer drift and report a total the grid never shows. refine() and matchesCategory() are deleted rather than left unused - dead code that still looks authoritative is worse than none. Their thirteen unit tests are replaced, not dropped: the behaviour is SQL now, so six integration tests against real PostgreSQL cover category narrowing, search-and-category together, both price directions, featured ordering, and an unrecognised sort carrying a SQL fragment, which the whitelist renders inert. Two page tests changed with the contract: sorting is asserted as a request carrying the choice rather than a client-side reorder, and a new one pins that a category reaches the API as ?category=. Remaining, and deliberately not in this change: there is still no pager, so one request shows one page. The ceiling is now per shelf rather than across the catalogue, and ordering is correct over the whole set, but a catalogue past PAGE_SIZE needs pagination in the UI - a feature, not a constant. 481 backend tests and 234 frontend tests pass; frontend coverage 100% lines and functions; build clean under -warnaserror; dotnet format clean. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01EA4mmpcb1rcvNntHR1iG6j --- .../Abstractions/IWidgetRepository.cs | 26 ++++- .../Catalog/Browse/BrowseWidgetsHandler.cs | 12 +- .../Persistence/WidgetRepository.cs | 57 +++++++--- .../Catalog/CatalogEndpoints.cs | 4 +- .../CatalogAndAuthRepositoryTests.cs | 106 +++++++++++++++++- web/src/lib/catalog.test.ts | 96 ++-------------- web/src/lib/catalog.ts | 44 +------- web/src/pages/CatalogPage.tsx | 14 ++- web/src/pages/StorefrontPages.test.tsx | 30 +++-- 9 files changed, 226 insertions(+), 163 deletions(-) diff --git a/src/WidgetWorks.Application/Abstractions/IWidgetRepository.cs b/src/WidgetWorks.Application/Abstractions/IWidgetRepository.cs index cf92d81..b21bcd9 100644 --- a/src/WidgetWorks.Application/Abstractions/IWidgetRepository.cs +++ b/src/WidgetWorks.Application/Abstractions/IWidgetRepository.cs @@ -3,11 +3,35 @@ namespace WidgetWorks.Application.Abstractions; /// Filter/paging criteria for catalog listing and search. -public sealed record WidgetQuery(string? Search, bool ActiveOnly, int Page, int PageSize) +/// +/// A catalogue listing request. Search and Category are independent narrowings combined with AND, +/// so "turbine" within Mega means both, not either. +/// +/// +/// One of . Never interpolated into SQL - the repository maps it through a +/// fixed set of order-by clauses, so an unrecognised value falls back to the default rather than +/// reaching the database. +/// +public sealed record WidgetQuery( + string? Search, + bool ActiveOnly, + int Page, + int PageSize, + string? Category = null, + string? Sort = null) { public int Offset => (Math.Max(1, Page) - 1) * PageSize; } +/// The orderings the catalogue offers. Values are part of the API contract. +public static class WidgetSort +{ + public const string Featured = "featured"; + public const string PriceAscending = "price-asc"; + public const string PriceDescending = "price-desc"; + public const string Name = "name"; +} + public interface IWidgetRepository { Task GetByIdAsync(Guid id, CancellationToken ct); diff --git a/src/WidgetWorks.Application/Catalog/Browse/BrowseWidgetsHandler.cs b/src/WidgetWorks.Application/Catalog/Browse/BrowseWidgetsHandler.cs index cc3ec81..77d0403 100644 --- a/src/WidgetWorks.Application/Catalog/Browse/BrowseWidgetsHandler.cs +++ b/src/WidgetWorks.Application/Catalog/Browse/BrowseWidgetsHandler.cs @@ -2,7 +2,13 @@ namespace WidgetWorks.Application.Catalog.Browse; -public sealed record BrowseWidgetsQuery(string? Search, bool IncludeInactive, int Page, int PageSize); +public sealed record BrowseWidgetsQuery( + string? Search, + bool IncludeInactive, + int Page, + int PageSize, + string? Category = null, + string? Sort = null); public sealed class BrowseWidgetsHandler(IWidgetRepository widgets) { @@ -18,7 +24,9 @@ public async Task> Handle(BrowseWidgetsQuery query, Canc string.IsNullOrWhiteSpace(query.Search) ? null : query.Search.Trim(), ActiveOnly: !query.IncludeInactive, page, - size); + size, + Category: string.IsNullOrWhiteSpace(query.Category) ? null : query.Category.Trim(), + Sort: query.Sort); var items = await widgets.SearchAsync(repoQuery, ct); var total = await widgets.CountAsync(repoQuery, ct); diff --git a/src/WidgetWorks.Infrastructure/Persistence/WidgetRepository.cs b/src/WidgetWorks.Infrastructure/Persistence/WidgetRepository.cs index 4df8bbb..30ebd84 100644 --- a/src/WidgetWorks.Infrastructure/Persistence/WidgetRepository.cs +++ b/src/WidgetWorks.Infrastructure/Persistence/WidgetRepository.cs @@ -14,7 +14,38 @@ public sealed class WidgetRepository(IDbConnectionFactory factory) : IWidgetRepo private const string Filter = @"where archived_at is null and (@ActiveOnly = false or is_active = true) - and (@Search is null or name ilike @Pattern or sku ilike @Pattern)"; + and (@Search is null or name ilike @Pattern or sku ilike @Pattern) + and (@Category is null or name ilike @CategoryPattern or sku ilike @CategoryPattern)"; + + /// + /// Sort clauses, chosen by key rather than built from the caller's string - the only safe way to + /// put user input near an order-by. An unknown key sorts by name, which is the catalogue default. + /// Every clause ends with name so paging is stable when prices tie; without a total ordering the + /// same row can appear on two pages. + /// + private static string OrderBy(string? sort) => sort switch + { + WidgetSort.PriceAscending => "order by price asc, name asc", + WidgetSort.PriceDescending => "order by price desc, name asc", + WidgetSort.Name => "order by name asc", + + // Featured leads with what can actually be bought and pushes sold-out items to the end. + // This used to happen in the browser over one page, which quietly meant "in stock on this + // page first"; done here it holds across the whole result set. + _ => "order by ((quantity_on_hand - quantity_reserved) > 0) desc, name asc", + }; + + /// Parameters shared by the listing and its count, so the two can never diverge. + private static object FilterParameters(WidgetQuery query) => new + { + query.ActiveOnly, + query.Search, + Pattern = query.Search is null ? null : $"%{query.Search}%", + query.Category, + CategoryPattern = query.Category is null ? null : $"%{query.Category}%", + Limit = query.PageSize, + query.Offset, + }; public async Task GetByIdAsync(Guid id, CancellationToken ct) { @@ -35,34 +66,24 @@ public sealed class WidgetRepository(IDbConnectionFactory factory) : IWidgetRepo public async Task> SearchAsync(WidgetQuery query, CancellationToken ct) { using var db = await factory.OpenAsync(ct); - var rows = await db.QueryAsync( + var rows = await db.QueryAsync(new CommandDefinition( $@"select {Columns} from widgets {Filter} - order by name + {OrderBy(query.Sort)} limit @Limit offset @Offset", - new - { - query.ActiveOnly, - query.Search, - Pattern = query.Search is null ? null : $"%{query.Search}%", - Limit = query.PageSize, - query.Offset, - }); + FilterParameters(query), + cancellationToken: ct)); return rows.ToList(); } public async Task CountAsync(WidgetQuery query, CancellationToken ct) { using var db = await factory.OpenAsync(ct); - return await db.ExecuteScalarAsync( + return await db.ExecuteScalarAsync(new CommandDefinition( $@"select count(*) from widgets {Filter}", - new - { - query.ActiveOnly, - query.Search, - Pattern = query.Search is null ? null : $"%{query.Search}%", - }); + FilterParameters(query), + cancellationToken: ct)); } public async Task AddAsync(Widget widget, CancellationToken ct) diff --git a/src/WidgetWorks.WebApi/Catalog/CatalogEndpoints.cs b/src/WidgetWorks.WebApi/Catalog/CatalogEndpoints.cs index 28a6b7d..74f731b 100644 --- a/src/WidgetWorks.WebApi/Catalog/CatalogEndpoints.cs +++ b/src/WidgetWorks.WebApi/Catalog/CatalogEndpoints.cs @@ -16,9 +16,9 @@ public static void MapCatalogEndpoints(this IEndpointRouteBuilder routes) // Public storefront: active widgets only. var catalog = routes.MapGroup("/catalog"); - catalog.MapGet("/widgets", async (string? search, int? page, int? pageSize, BrowseWidgetsHandler handler, CancellationToken ct) => + catalog.MapGet("/widgets", async (string? search, string? category, string? sort, int? page, int? pageSize, BrowseWidgetsHandler handler, CancellationToken ct) => { - var result = await handler.Handle(new BrowseWidgetsQuery(search, IncludeInactive: false, page ?? 1, pageSize ?? 20), ct); + var result = await handler.Handle(new BrowseWidgetsQuery(search, IncludeInactive: false, page ?? 1, pageSize ?? 20, category, sort), ct); return Results.Ok(result); }); diff --git a/tests/WidgetWorks.IntegrationTests/CatalogAndAuthRepositoryTests.cs b/tests/WidgetWorks.IntegrationTests/CatalogAndAuthRepositoryTests.cs index 0b84ae6..f767dc7 100644 --- a/tests/WidgetWorks.IntegrationTests/CatalogAndAuthRepositoryTests.cs +++ b/tests/WidgetWorks.IntegrationTests/CatalogAndAuthRepositoryTests.cs @@ -25,7 +25,7 @@ public class CatalogAndAuthRepositoryTests(PostgresFixture db) private static string Unique(string prefix) => prefix + Guid.NewGuid().ToString("N")[..10]; - private async Task GivenWidget(int onHand = 10, bool active = true, string? name = null) + private async Task GivenWidget(int onHand = 10, bool active = true, string? name = null, decimal price = 12.5m, int reserved = 0) { var widget = new Widget { @@ -33,9 +33,9 @@ private async Task GivenWidget(int onHand = 10, bool active = true, stri Sku = Unique("SKU-").ToUpperInvariant(), Name = name ?? Unique("Widget "), Description = "Integration fixture.", - Price = 12.5m, + Price = price, QuantityOnHand = onHand, - QuantityReserved = 0, + QuantityReserved = reserved, IsActive = active, CreatedAt = Now, UpdatedAt = Now, @@ -533,4 +533,104 @@ public async Task Audit_entries_are_written_for_a_user_and_anonymously() // No read side on the port; the assertion is that neither write throws or violates the FK. Assert.NotNull(await Users.GetByIdAsync(user.Id, CancellationToken.None)); } + + // ---- category narrowing and ordering, moved here from the browser ------------------------- + // These replace the refine() unit tests: the behaviour is SQL now, so this is where it is + // proven. Every case scopes itself with a unique token so a shared database stays usable. + + [Fact] + public async Task A_category_narrows_the_listing_to_its_members() + { + var token = Unique("cat"); + await GivenWidget(name: $"Mega Widget Block {token}"); + await GivenWidget(name: $"Mega Widget Hub {token}"); + await GivenWidget(name: $"Mini Widget Block {token}"); + + var query = new WidgetQuery($"{token}", ActiveOnly: true, 1, 50, Category: "mega"); + var results = await Widgets.SearchAsync(query, CancellationToken.None); + + Assert.Equal(2, results.Count); + Assert.All(results, w => Assert.Contains("Mega", w.Name, StringComparison.Ordinal)); + // The count has to agree with the page, or the storefront reports a total it never shows. + Assert.Equal(2, await Widgets.CountAsync(query, CancellationToken.None)); + } + + [Fact] + public async Task A_search_and_a_category_narrow_together_rather_than_either_or() + { + var token = Unique("both"); + await GivenWidget(name: $"Mega Widget Turbine {token}"); + await GivenWidget(name: $"Mega Widget Block {token}"); + await GivenWidget(name: $"Mini Widget Turbine {token}"); + + var results = await Widgets.SearchAsync( + new WidgetQuery($"Turbine {token}", ActiveOnly: true, 1, 50, Category: "mega"), + CancellationToken.None); + + // "Turbine" within Mega means both conditions, not their union. + Assert.Single(results); + Assert.Contains("Mega Widget Turbine", results[0].Name, StringComparison.Ordinal); + } + + [Fact] + public async Task No_category_leaves_the_listing_alone() + { + var token = Unique("nocat"); + await GivenWidget(name: $"Mega Widget {token}"); + await GivenWidget(name: $"Mini Widget {token}"); + + var results = await Widgets.SearchAsync( + new WidgetQuery(token, ActiveOnly: true, 1, 50, Category: null), + CancellationToken.None); + + Assert.Equal(2, results.Count); + } + + [Fact] + public async Task Price_sorts_run_in_both_directions() + { + var token = Unique("price"); + await GivenWidget(name: $"B {token}", price: 30m); + await GivenWidget(name: $"A {token}", price: 10m); + await GivenWidget(name: $"C {token}", price: 20m); + + var ascending = await Widgets.SearchAsync( + new WidgetQuery(token, ActiveOnly: true, 1, 50, Sort: WidgetSort.PriceAscending), CancellationToken.None); + var descending = await Widgets.SearchAsync( + new WidgetQuery(token, ActiveOnly: true, 1, 50, Sort: WidgetSort.PriceDescending), CancellationToken.None); + + Assert.Equal([10m, 20m, 30m], ascending.Select(w => w.Price)); + Assert.Equal([30m, 20m, 10m], descending.Select(w => w.Price)); + } + + [Fact] + public async Task Featured_leads_with_what_can_actually_be_bought() + { + var token = Unique("feat"); + await GivenWidget(name: $"A sold out {token}", onHand: 5, reserved: 5); + await GivenWidget(name: $"B in stock {token}", onHand: 5); + + var results = await Widgets.SearchAsync( + new WidgetQuery(token, ActiveOnly: true, 1, 50, Sort: WidgetSort.Featured), CancellationToken.None); + + // Alphabetically the sold-out one comes first; availability outranks the name. + Assert.Contains("in stock", results[0].Name, StringComparison.Ordinal); + Assert.Contains("sold out", results[1].Name, StringComparison.Ordinal); + } + + [Fact] + public async Task An_unknown_sort_falls_back_instead_of_reaching_the_database() + { + var token = Unique("inject"); + await GivenWidget(name: $"A {token}"); + await GivenWidget(name: $"B {token}"); + + // The value is mapped through a fixed set, never interpolated, so even this is inert. + var results = await Widgets.SearchAsync( + new WidgetQuery(token, ActiveOnly: true, 1, 50, Sort: "price; drop table widgets"), + CancellationToken.None); + + Assert.Equal(2, results.Count); + Assert.Contains($"A {token}", results[0].Name, StringComparison.Ordinal); + } } diff --git a/web/src/lib/catalog.test.ts b/web/src/lib/catalog.test.ts index 48daa86..deb08fa 100644 --- a/web/src/lib/catalog.test.ts +++ b/web/src/lib/catalog.test.ts @@ -1,35 +1,10 @@ import { describe, expect, it } from 'vitest' -import type { WidgetView } from '../api/types' -import { CATEGORIES, categoryBySlug, refine } from './catalog' - -// The header scope select, the category rail and the sort control all funnel -// through refine(), so this covers the behaviour behind three UI controls. -const widget = (over: Partial): WidgetView => ({ - id: crypto.randomUUID(), - sku: 'WW-000', - name: 'Widget', - description: '', - imageUrl: null, - price: 10, - isActive: true, - quantityOnHand: 10, - quantityReserved: 0, - quantityAvailable: 10, - ...over, -}) - -const catalog: WidgetView[] = [ - widget({ sku: 'WW-001', name: 'Standard Widget', price: 9.99 }), - widget({ sku: 'WW-002', name: 'Deluxe Widget', price: 24.99 }), - widget({ sku: 'WW-003', name: 'Mega Widget', price: 49.99 }), - widget({ sku: 'WW-005', name: 'Widget Pro Kit', price: 79.99, quantityAvailable: 0 }), -] - -const names = (items: WidgetView[]) => items.map((w) => w.name) - -// Index rather than Array.prototype.at — the project compiles to ES2020. -const last = (items: string[]) => items[items.length - 1] +import { CATEGORIES, categoryBySlug } from './catalog' +// The browsing vocabulary the header scope select, the category rail and the +// sort control share. Filtering and ordering are the API's job now — that +// behaviour is covered by WidgetRepositoryTests against real SQL, which is +// where it lives. describe('categoryBySlug', () => { it('resolves a real category', () => { expect(categoryBySlug('mega')?.keyword).toBe('mega') @@ -45,60 +20,13 @@ describe('categoryBySlug', () => { expect(c.icon).not.toBe('') } }) -}) - -describe('refine — category filter', () => { - it('narrows to the matching category', () => { - expect(names(refine(catalog, 'deluxe', 'featured'))).toEqual(['Deluxe Widget']) - }) - it('matches the seeded kit by keyword', () => { - expect(names(refine(catalog, 'kit', 'featured'))).toEqual(['Widget Pro Kit']) - }) - - it('returns everything when no category is chosen', () => { - expect(refine(catalog, '', 'featured')).toHaveLength(catalog.length) - }) - - it('returns nothing for a category with no members', () => { - expect(refine(catalog, 'mini', 'featured')).toEqual([]) - }) - - it('does not mutate the source array', () => { - const original = [...catalog] - refine(catalog, '', 'price-desc') - expect(catalog).toEqual(original) - }) -}) - -describe('refine — sorting', () => { - it('sorts by price ascending', () => { - expect(names(refine(catalog, '', 'price-asc'))).toEqual([ - 'Standard Widget', 'Deluxe Widget', 'Mega Widget', 'Widget Pro Kit', - ]) - }) - - it('sorts by price descending', () => { - expect(names(refine(catalog, '', 'price-desc'))).toEqual([ - 'Widget Pro Kit', 'Mega Widget', 'Deluxe Widget', 'Standard Widget', - ]) - }) - - it('sorts by name', () => { - expect(names(refine(catalog, '', 'name'))).toEqual([ - 'Deluxe Widget', 'Mega Widget', 'Standard Widget', 'Widget Pro Kit', - ]) - }) - - it('featured pushes out-of-stock items to the end', () => { - expect(last(names(refine(catalog, '', 'featured')))).toBe('Widget Pro Kit') - }) - - it('falls back to featured ordering for an unknown sort', () => { - expect(last(names(refine(catalog, '', 'nonsense')))).toBe('Widget Pro Kit') - }) - - it('handles an empty catalog', () => { - expect(refine([], 'mega', 'price-asc')).toEqual([]) + it('resolves each slug to the keyword the API is asked to narrow on', () => { + // The rail stores a slug; the request sends the keyword. This is the + // assertion that catches a slug being renamed for the URL without the + // keyword following it. + for (const c of CATEGORIES.filter((c) => c.slug)) { + expect(categoryBySlug(c.slug)?.keyword).toBe(c.keyword) + } }) }) diff --git a/web/src/lib/catalog.ts b/web/src/lib/catalog.ts index 9b0f1df..fafbca0 100644 --- a/web/src/lib/catalog.ts +++ b/web/src/lib/catalog.ts @@ -1,14 +1,11 @@ // Storefront browsing vocabulary, shared by the header scope select, the // category rail and the catalog grid so all three stay in step. // -// The API searches free text (`?search=`) and pages server-side. Category and -// sort are refinements applied to the returned page in the browser: the store -// asks for a full page (PAGE_SIZE, the API's 100 cap) and narrows it here, -// which keeps the three controls composable without new endpoints. The demo -// catalog is 75 widgets, so one page still holds all of it - a catalog past -// PAGE_SIZE would drop its tail out of every category shelf. -import type { WidgetView } from '../api/types' - +// Search, category and sort are all applied by the API. They used to be +// narrowed here over a single fetched page, which meant a catalog larger than +// PAGE_SIZE lost its tail from every shelf and a sort only ordered whatever +// happened to be on that page. PAGE_SIZE is now just how many results one +// request asks for; growing past it needs a pager, not a bigger number. export const PAGE_SIZE = 100 export interface Category { @@ -43,36 +40,5 @@ export function categoryBySlug(slug: string): Category | undefined { return CATEGORIES.find((c) => c.slug === slug && c.slug !== '') } -function matchesCategory(w: WidgetView, keyword: string): boolean { - if (!keyword) return true - const needle = keyword.toLowerCase() - return ( - w.name.toLowerCase().includes(needle) || - w.sku.toLowerCase().includes(needle) || - w.description.toLowerCase().includes(needle) - ) -} - -/** Narrow to a category, then order — pure, so the grid can call it on render. */ -export function refine(items: WidgetView[], catSlug: string, sort: string): WidgetView[] { - const keyword = categoryBySlug(catSlug)?.keyword ?? '' - const filtered = keyword ? items.filter((w) => matchesCategory(w, keyword)) : items.slice() - - switch (sort) { - case 'price-asc': - return filtered.sort((a, b) => a.price - b.price) - case 'price-desc': - return filtered.sort((a, b) => b.price - a.price) - case 'name': - return filtered.sort((a, b) => a.name.localeCompare(b.name)) - default: - // "Featured" keeps the order the API returned, with anything out of - // stock pushed to the end so the grid leads with what can be bought. - return filtered.sort( - (a, b) => Number(b.quantityAvailable > 0) - Number(a.quantityAvailable > 0), - ) - } -} - /** Free-shipping threshold quoted in the header strip and cart nudge. */ export const FREE_SHIPPING_THRESHOLD = 75 diff --git a/web/src/pages/CatalogPage.tsx b/web/src/pages/CatalogPage.tsx index 08808f7..2109019 100644 --- a/web/src/pages/CatalogPage.tsx +++ b/web/src/pages/CatalogPage.tsx @@ -1,9 +1,9 @@ -import { useEffect, useMemo, useState } from 'react' +import { useEffect, useState } from 'react' import { Link, useSearchParams } from 'react-router-dom' import { api } from '../api/client' import type { Paged, WidgetView } from '../api/types' import { pseudoRating } from '../lib/img' -import { CATEGORIES, FREE_SHIPPING_THRESHOLD, PAGE_SIZE, SORTS, categoryBySlug, refine } from '../lib/catalog' +import { CATEGORIES, FREE_SHIPPING_THRESHOLD, PAGE_SIZE, SORTS, categoryBySlug } from '../lib/catalog' import { CategoryIcon } from '../components/CategoryIcon' import { AddToCartButton } from '../components/AddToCartButton' import { ProductImage } from '../components/ProductImage' @@ -58,14 +58,20 @@ export function CatalogPage() { setLoading(true) const sp = new URLSearchParams({ pageSize: String(PAGE_SIZE) }) if (q.trim()) sp.set('search', q.trim()) + // Category and sort are the server's job. Narrowing a single fetched page in the browser + // silently dropped anything past that page from a shelf, and sorted only what happened to be + // on it. Asking the API means both apply to the whole matching set. + const keyword = categoryBySlug(cat)?.keyword + if (keyword) sp.set('category', keyword) + if (sort) sp.set('sort', sort) api>(`/catalog/widgets?${sp}`) .then((d) => { if (active) { setData(d); setError(null) } }) .catch((e) => { if (active) setError(e.message) }) .finally(() => { if (active) setLoading(false) }) return () => { active = false } - }, [q]) + }, [q, cat, sort]) - const items = useMemo(() => refine(data?.items ?? [], cat, sort), [data, cat, sort]) + const items = data?.items ?? [] const category = categoryBySlug(cat) const browsing = !q && !cat diff --git a/web/src/pages/StorefrontPages.test.tsx b/web/src/pages/StorefrontPages.test.tsx index ffc8a09..10eb896 100644 --- a/web/src/pages/StorefrontPages.test.tsx +++ b/web/src/pages/StorefrontPages.test.tsx @@ -88,11 +88,12 @@ describe('CatalogPage', () => { }) it('counts what is shown, in the singular when only one matched', async () => { - stubFetch([['/catalog/widgets', paged()]]) + // The API returns the narrowed set now, so the fixture is the answer to + // ?category=mega rather than something the page filters afterwards. + stubFetch([['/catalog/widgets', paged([soldOut])]]) renderWithProviders(, { at: '/store?cat=mega', path: '/store' }) - // One of the two loaded widgets matches "mega". expect(await screen.findByText('1 product')).toBeInTheDocument() }) @@ -101,13 +102,13 @@ describe('CatalogPage', () => { renderWithProviders(, { at: '/store', path: '/store' }) - // Category and sort refine the page in the browser; the count must not pretend the - // catalog is only as big as the page. + // The grid shows one page; the count must not pretend the catalog is only + // as big as that page. expect(await screen.findByText('2 products of 40')).toBeInTheDocument() }) - it('sorting re-orders the grid in place', async () => { - stubFetch([['/catalog/widgets', paged()]]) + it('asks the API to re-order rather than sorting the page it already has', async () => { + const calls = stubFetch([['/catalog/widgets', paged()]]) const user = userEvent.setup() renderWithProviders(, { at: '/store', path: '/store' }) @@ -115,10 +116,19 @@ describe('CatalogPage', () => { await user.selectOptions(screen.getByLabelText('Sort by'), 'price-desc') - await waitFor(() => { - const titles = screen.getAllByRole('link', { name: /Widget$/ }).map((el) => el.textContent) - expect(titles).toEqual(['Mega Widget', 'Standard Widget']) // 99 before 12.50 - }) + // Sorting in the browser would only order whatever happened to be on this + // page. The request carries the choice so the ordering applies to the whole + // matching set. + await waitFor(() => expect(calls.some((c) => c.url.includes('sort=price-desc'))).toBe(true)) + }) + + it('narrows a category through the API, not in the browser', async () => { + const calls = stubFetch([['/catalog/widgets', paged()]]) + + renderWithProviders(, { at: '/store?cat=mega', path: '/store' }) + await screen.findByRole('heading', { name: 'Mega widgets' }) + + await waitFor(() => expect(calls.some((c) => c.url.includes('category=mega'))).toBe(true)) }) it('clears a category from the toolbar', async () => {