Security and correctness hardening: throttling, cart authorization, settlement races, stale reservations, server-side catalogue - #86
Merged
Conversation
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EA4mmpcb1rcvNntHR1iG6j
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EA4mmpcb1rcvNntHR1iG6j
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EA4mmpcb1rcvNntHR1iG6j
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01EA4mmpcb1rcvNntHR1iG6j
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the six items from the review, in four commits that can be read independently.
1 · Throttling and cart authorization
There was no rate limiting anywhere in the application. Sign-in, registration, reset, guest lookup and checkout were all unbounded; account lockout caps attempts per account, which stops nothing spread across many accounts and nothing at all on the anonymous endpoints.
Named policies now sit on those surfaces, bound from configuration so a limit can be tightened during an incident without a redeploy.
UseRateLimiterruns ahead of authentication, so a flood is rejected before the app validates anything. There is deliberately no global limiter — a catalogue page issues several requests in a burst, so a global cap would throttle browsing while adding nothing.The partition key is where limiters go wrong: behind a proxy every request carries the proxy's address, callers collapse into one bucket, and the cap trips for everybody.
X-Forwarded-Foris therefore honoured only when configuration says a trusted proxy is in front. This suite demonstrated the failure mode on itself — the test server sends no remote address, so the API tests shared one partition and passed alone but failed together. The fixture raises its own budgets explicitly; production defaults were not weakened.Cart.UserIdexisted and nothing consulted it. Read, update, remove and checkout each took a cart id and used it.CartAccessis now the single rule — guest carts stay open (that is what lets a visitor shop before signing in), claimed carts admit only their owner. Refusals reuse the not-found wording, because a distinct forbidden confirms the id exists.RequestedByis required, not optional: the compiler made all fourteen call sites state intent.2 · Settlement races
ConfirmPaymentHandlerguarded payment writes by read → check → write. Two concurrent deliveries both pass that check. A duplicate success sent a second receipt; a duplicate failure released the reservation twice, and since the guard only stops the column going negative, the second release consumed stock still held by another order. A late success could mark an order Paid whose stock was already back on sale.The three payment writes are now compare-and-set, each returning whether it won.
MarkPaymentFailedAsynctests its own result inside the transaction and compensates only as the winner, so a redelivered webhook cannot double-release.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 status is the idempotency key, it is durable, and it is the invariant that matters. Adding a table to assert it twice is ceremony.
3 · Stale reservations
An order parked in
AwaitingPaymentheld stock until a provider actively reported failure. A webhook that never arrives held it forever — the engine of the inventory-exhaustion path, since checkout is anonymous by design.Policy is a plain handler (testable without a timer); scheduling is a
BackgroundServicethat owns nothing but the clock. It takes a fresh scope per tick — the repositories are Scoped, and a singleton holding one would pin a connection for the life of the process. A failed sweep is logged and the loop continues, because a transient blip must not silently stop reclaiming stock until someone restarts the host.4 · Server-side catalogue
Category and sort were applied in the browser to one fetched page, making
PAGE_SIZEa silent ceiling: products fell off every shelf with no error, and "price, low to high" meant "cheapest of the first hundred".Both moved into the query. Sort maps through a fixed set of clauses and is never interpolated; every clause ends with
nameso paging is total.refine()is deleted rather than left unused, and its thirteen unit tests are replaced, not dropped — six integration tests against real PostgreSQL now cover the behaviour where it lives, including a sort value carrying a SQL fragment that the whitelist renders inert.Still open, deliberately: there is no pager. The ceiling is now per shelf rather than per catalogue and ordering is correct across the whole set, but growing past
PAGE_SIZEneeds pagination in the UI — a feature, not a constant.Verification
Run locally against real PostgreSQL 16, not inferred:
dotnet build -warnaserrorclean ·dotnet format --verify-no-changescleanGenerated by Claude Code