diff --git a/README.md b/README.md index ecda55f..e94d9c2 100644 --- a/README.md +++ b/README.md @@ -44,7 +44,8 @@ docker compose up --build | Store (SPA) | http://localhost:3000/store | | **Mailpit** — every email the app sends | http://localhost:8025 | | API + Scalar (interactive API UI) | http://localhost:8080/scalar/v1 | -| Health | http://localhost:8080/health | +| Health (liveness — no database) | http://localhost:8080/health | +| Readiness (queries the database) | http://localhost:8080/health/ready | Migrations and demo seed run automatically on API start. For running the API on the host with fast iteration (and the exact port/user-secrets details), see diff --git a/docs/handbook/03-setup-and-run.md b/docs/handbook/03-setup-and-run.md index 8f6b531..8654b8f 100644 --- a/docs/handbook/03-setup-and-run.md +++ b/docs/handbook/03-setup-and-run.md @@ -29,7 +29,8 @@ Then open: | Store (SPA) | http://localhost:3000/store | | **Mailpit** — every email the app sends | http://localhost:8025 | | API + Scalar (interactive API UI) | http://localhost:8080/scalar/v1 | -| Health | http://localhost:8080/health | +| Health (liveness — no database) | http://localhost:8080/health | +| Readiness (queries the database) | http://localhost:8080/health/ready | The landing page at `/` explains the demo, states that no payment is ever taken, and lists the accounts below with what each role can do — it's the same guide, in the app. @@ -109,4 +110,5 @@ values and why. [Configuration](04-configuration-and-2fa.md#reading-real-mail-locally-mailpit). - **API up but every request 503** — the database was unreachable at startup, so migrations were skipped and the app is running degraded on purpose rather than restart-looping. - `/health` says so; fix the connection and restart the API. + `/health` says so; fix the connection and restart the API. Once running, `/health/ready` + is the one that keeps checking — `/health` only reports how startup went. diff --git a/docs/handbook/04-configuration-and-2fa.md b/docs/handbook/04-configuration-and-2fa.md index f87f929..2d10be8 100644 --- a/docs/handbook/04-configuration-and-2fa.md +++ b/docs/handbook/04-configuration-and-2fa.md @@ -56,6 +56,9 @@ by policy. | `Email:Host/Port/Username/Password/...` | SMTP settings | env / secret store; user-secrets in dev | Password is secret. | | `Cors:AllowedOrigins` | Browser origins allowed to call the API | env / appsettings | Not secret. | | `App:BaseUrl` | Public SPA URL (used in email links) | env / appsettings | Not secret. | +| `RateLimiting:TrustForwardedFor` | Whether `X-Forwarded-For` may be believed | env / appsettings | Not secret, but **load-bearing** — see below. | +| `RateLimiting:Auth\|Checkout\|Lookup` | Throttling budgets per caller | env / appsettings | Not secret; tune during an incident without a redeploy. | +| `Reservations:*` | Stale-reservation sweep window, interval, batch, on/off | env / appsettings | Not secret. | | `VITE_API_BASE_URL`, `VITE_GOOGLE_CLIENT_ID` | Web build-time config | GitHub Actions **Variables** (CI) / `web/.env.local` (dev) | Public; injected at build time, never committed. | **Why user-secrets vs `.env`:** user-secrets is read by the .NET app when you run it directly @@ -64,6 +67,67 @@ container can't read your host user-secrets, so the Docker path uses a git-ignor Compose variable substitution. In CI/prod, use **GitHub Actions Secrets** and **Azure App Service settings / Key Vault**. See [`docs/local-development.md`](../local-development.md). +## Throttling, and the one setting that can cause an outage + +Sign-in, registration, password reset, guest order lookup and checkout are rate limited. Budgets +live in `appsettings.json` so they can be tightened during an incident without a redeploy: + +```json +"RateLimiting": { + "TrustForwardedFor": false, + "Auth": { "PermitLimit": 20, "WindowSeconds": 60 }, + "Checkout": { "PermitLimit": 8, "WindowSeconds": 60 }, + "Lookup": { "PermitLimit": 10, "WindowSeconds": 60 } +} +``` + +**`TrustForwardedFor` decides whether throttling works at all behind a proxy.** The limiter +partitions by caller. Behind a reverse proxy every request arrives carrying the *proxy's* address, +so with this left `false` every caller in the world collapses into one partition and the limiter +becomes a global cap that the first busy minute trips for everybody — a self-inflicted outage with +nothing in the logs to explain it. + +The inverse is the security mistake: set `true` with no proxy in front and a caller can forge the +header, minting a fresh partition per request and opting out of throttling entirely. + +| Deployment | Setting | +|---|---| +| Behind App Service, a load balancer, Cloudflare, any reverse proxy | `true` | +| Direct to the app, local development, container with no proxy | `false` | + +The app watches real traffic and logs a warning **once** when the setting and the traffic disagree, +in either direction. If you see either warning, the setting is wrong — it is not advisory. + +There is deliberately **no global limiter**: a catalogue page issues several requests in a burst, so +a global cap would throttle ordinary browsing while adding nothing an endpoint policy does not +already do. + +## Reclaiming stock from unfinished payments + +An order whose payment settles asynchronously holds its stock while it waits for a provider webhook. +If that webhook never arrives — provider outage, dropped delivery, a shopper closing the tab at the +bank's redirect — the stock would be held forever. A background sweep returns it: + +```json +"Reservations": { + "Enabled": true, + "ExpireAfterMinutes": 15, + "SweepIntervalMinutes": 5, + "BatchSize": 100 +} +``` + +`ExpireAfterMinutes` 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 returning shopper +rarely finds the item gone. + +`BatchSize` caps one pass, so a backlog is worked through over several sweeps rather than one long +transaction. `Enabled: false` turns the sweep off for a host that should not run background work. + +Releasing reuses the same transactional path as a reported payment failure, so a webhook landing +during a sweep cannot double-release: whichever writes first wins and the other is declined. + ## Email setup `IEmailSender` has two adapters, chosen by `Email:Provider`: diff --git a/docs/handbook/08-bugs-and-lessons.md b/docs/handbook/08-bugs-and-lessons.md index 3a0686f..4dd6f52 100644 --- a/docs/handbook/08-bugs-and-lessons.md +++ b/docs/handbook/08-bugs-and-lessons.md @@ -56,6 +56,12 @@ Rows 1–11 are from the first phase, 12–32 from the second, 33–39 from the | 37 | A challenge-token test failed against a correct implementation | writing tests around `ValidateChallengeTokenAsync` | Issuance uses the injected `TimeProvider` but `TokenValidationParameters` has no such hook, so lifetime is validated against the **system** clock. A token minted at a fixed past date is born expired | Anchor those tests on real time and move the *issuing* clock to express age | "Inject time everywhere" holds only as far as the libraries let it; find the seams that don't take your clock | | 38 | An integration test asserted a uniqueness rule the schema does not have | the test failed | `ix_widgets_live_name` is a plain index for ordering the live set, not a unique one. Only SKU is unique (case-folded via `upper(sku)`) | Assert the rule that exists, and add a test documenting that names are deliberately **not** unique | Read the migration, not your memory of it — a test that asserts an imagined constraint fails honestly, but the same assumption in code would not | | 39 | New frontend tests passed but `tsc` failed the build | running `npm run build`, not just `npm test` | `.at(-1)` again — the **same ES2022-against-ES2020 trap as row 23** — plus a `let x = null` only assigned inside a Promise executor, which TypeScript narrows to `never` | Index arithmetic, and `let release!: () => void` | Vitest transpiles without type-checking, so a green test run says nothing about the build. Run the gate CI runs | +| 40 | An admin renaming a widget could silently revert a live reservation | reading the write path after the inventory work | One `UpdateAsync` wrote **every** column from the caller's in-memory object, and three handlers used load-change-save. A reservation taken between the read and the save was overwritten — an oversell caused by an edit, with no attacker and no error | Split the write path by intent — details, stock, archive — and move the stock arithmetic and its guards into the `UPDATE` itself | A repository method that writes every column turns every caller into a potential lost update. Write what you mean, not the whole row | +| 41 | Two deliveries of one payment webhook released the same stock twice | tracing the settlement path | The handler guarded with read-check-write, which two concurrent deliveries both pass. The second compensation decremented `quantity_reserved` again and ate stock held by a *different* order | Compare-and-set: each payment write names the statuses it may apply from and returns whether it won; only the winner compensates | Application-level status checks are courtesy. If two callers can race, the row has to be the arbiter | +| 42 | Order numbers collided at a few thousand orders a day | arithmetic, not a failure | The suffix was 6 hex characters of the order's Guid — 24 bits, scoped to one day. `order_number` is uniquely indexed, so a collision was never a leak, but it rolled the placement back: a hard checkout failure | Widen to 10 characters (40 bits) and pin the width with a test | Collisions arrive by the birthday bound, not when the space runs out. 16.7 million values is a coin flip at 5,000 a day | +| 43 | The API test suite passed individually and failed together | adding rate limiting | The test server sends no remote address, so every request shared one throttling partition and the suite exhausted a realistic budget between its own tests | Raise the budgets in the fixture explicitly, and prove rejection separately with a host configured down to two | The suite reproduced the exact production failure mode — all callers in one partition — which is the trap behind any reverse proxy | +| 44 | The health endpoint could never report unhealthy | asking an operations question, not a security one | `/health` closed over a variable captured at **startup**, so once the process was up it answered `ok` forever — database gone, still `ok`. The keep-warm ping held it in rotation on an answer that could not change | Add `/health/ready` that queries the database; leave `/health` shallow | The obvious fix — query the database in `/health` — would have cost ~180 CU-hrs against a 100 CU-hr budget. The shallow probe was deliberate and said so in a comment | +| 45 | CodeQL found log injection in the code written to fix a logging gap | the scanner blocked the merge | The new exception handler logged `Request.Path.Value` raw. That is the **decoded** path, so `%0A` in a URL arrives as a real newline and the caller writes their own log entry (CWE-117) | `LogSafe.Text` strips control characters from the path and method, keeping printable oddities | The correlation id on the line beside it *was* sanitised, with a comment citing this exact weakness. Knowing a rule is not the same as applying it everywhere it holds | ## Lessons learned @@ -73,6 +79,22 @@ Rows 1–11 are from the first phase, 12–32 from the second, 33–39 from the **and** by a database trigger — a bug or a direct SQL write still can’t violate it. - **Never trust the client for money.** Totals — subtotal, shipping, and per-state tax — are recomputed server-side at checkout; the browser’s numbers are display-only. +- **Let the row arbitrate, not the handler.** Read-check-write reads like a guard and is not + one: any two callers who can race will both pass it. Compare-and-set — naming the states a + write may apply from and acting only when it wins — turned duplicate webhooks, out-of-order + events and concurrent sweeps from bugs into no-ops, and it is one `where` clause. +- **Write what you mean, not the whole row.** A single repository method that set every + column made every caller a possible lost update, including an admin editing a product name + during a checkout. Splitting writes by intent removed a whole class of defect rather than + one instance of it. +- **Read the reasoning before changing the code.** The obvious fix for a health check that + could not fail was to make it query the database — which would have doubled the database + bill, for reasons written down in the workflow that pings it. It would have passed every + test and looked right in review. +- **The gates catch what the author cannot.** Three real defects in this round were found by + CI, by the coverage floor, and by CodeQL — not by re-reading the diff. The one that stings + is the last: log injection in a handler whose *neighbouring line* was sanitised against + exactly that weakness. - **Secrets discipline pays off.** `.gitignore` + `.gitleaks.toml` + an always-on secret scan meant “no secret in the repo” was enforced, not aspirational — and the one allowed exception (documented demo creds) is explicit. diff --git a/docs/handbook/09-runbook.md b/docs/handbook/09-runbook.md index 12eff25..928922f 100644 --- a/docs/handbook/09-runbook.md +++ b/docs/handbook/09-runbook.md @@ -25,6 +25,54 @@ Restart the API after changing config. --- +## Health probes — which one to point at what + +Two endpoints answering two different questions. Wiring the wrong one is how a monitoring signal +ends up unable to report bad news. + +| Endpoint | Answers | Touches the database | Point this at | +|---|---|---|---| +| `GET /health` | Did this process start correctly? | **No** | Keep-warm pings, first-boot provisioning checks | +| `GET /health/ready` | Can this instance serve a request right now? | **Yes** — `select 1` | Platform health probe, alerting, load-balancer rotation | + +```bash +curl -i http://localhost:8080/health # {"status":"ok",...} +curl -i http://localhost:8080/health/ready # {"status":"ready","database":"ok",...} +``` + +**Why they are separate, and why it matters for the bill.** `/health` is pinged every few minutes to +hold a free-tier App Service instance loaded. If that ping woke the database each time it would hold +a metered resource awake around the clock — roughly 180 CU-hrs against Neon's 100 CU-hr monthly free +allowance. Warming the app while letting the database sleep is deliberate. + +So: **never point a scheduled warm-up at `/health/ready`**, and never make `/health` query the +database. They look interchangeable and are not. + +`/health/ready` returns `503` with the failing exception *type* when the database does not answer — +never the message, because a connection error can carry a host name or a user and the endpoint is +anonymous. + +## Tracing a failure a customer reports + +Every response carries an `X-Correlation-Id` header, and a `500` repeats it in the body: + +```json +{ "error": "Something went wrong on our side. Quote the reference below if you contact us.", + "correlationId": "0HN7…" } +``` + +The same id is on the log line for that request, so a customer report becomes a lookup rather than a +search through everything that happened at that minute: + +```bash +# Azure App Service log stream, or wherever logs land +az webapp log tail --name --resource-group | grep 0HN7 +``` + +If the caller supplied `X-Correlation-Id`, that value is kept so a trace spans several services — +sanitised first, because it reaches log messages and text carrying newlines could otherwise forge +whole entries. + ## Email ### Test locally — Dev sender (default, zero setup) diff --git a/src/WidgetWorks.WebApi/appsettings.json b/src/WidgetWorks.WebApi/appsettings.json index 48a66c9..18b9bb7 100644 --- a/src/WidgetWorks.WebApi/appsettings.json +++ b/src/WidgetWorks.WebApi/appsettings.json @@ -17,5 +17,26 @@ "DemoAdminEmail": "admin@widgetworks.demo", "DemoCustomerEmail": "demo@widgetworks.demo", "DemoManagerEmail": "manager@widgetworks.demo" + }, + "RateLimiting": { + "TrustForwardedFor": false, + "Auth": { + "PermitLimit": 20, + "WindowSeconds": 60 + }, + "Checkout": { + "PermitLimit": 8, + "WindowSeconds": 60 + }, + "Lookup": { + "PermitLimit": 10, + "WindowSeconds": 60 + } + }, + "Reservations": { + "Enabled": true, + "ExpireAfterMinutes": 15, + "SweepIntervalMinutes": 5, + "BatchSize": 100 } } diff --git a/tests/WidgetWorks.ApiTests/ShippedConfigurationTests.cs b/tests/WidgetWorks.ApiTests/ShippedConfigurationTests.cs new file mode 100644 index 0000000..c28e901 --- /dev/null +++ b/tests/WidgetWorks.ApiTests/ShippedConfigurationTests.cs @@ -0,0 +1,112 @@ +using System.Text.Json; +using WidgetWorks.Application.Checkout.ReleaseStale; +using WidgetWorks.WebApi.RateLimiting; +using Xunit; + +namespace WidgetWorks.ApiTests; + +/// +/// Keeps the shipped appsettings.json honest. +/// +/// Writing these settings into the file made them discoverable — an operator can now see that +/// throttling budgets and the reservation sweep are tunable at all, and that TrustForwardedFor +/// exists, which matters because getting it wrong turns per-caller throttling into a global cap. +/// +/// The cost of that is two sources of truth. If the file and the code defaults drift, the file +/// starts describing an application that no longer behaves that way, which is worse than not +/// documenting the setting at all. These tests are the guard: change a default in code without the +/// file, or the file without the code, and they fail. +/// +public class ShippedConfigurationTests +{ + private static JsonElement Section(string name) + { + var json = JsonDocument.Parse(File.ReadAllText(AppSettingsPath())); + Assert.True( + json.RootElement.TryGetProperty(name, out var section), + $"appsettings.json has no '{name}' section, so the settings it controls are invisible to anyone deploying this."); + return section.Clone(); + } + + /// + /// Walks up from the test binary to the repository root, identified by the solution file, so + /// this resolves the same way locally and on a build agent. + /// + private static string AppSettingsPath() + { + var dir = new DirectoryInfo(AppContext.BaseDirectory); + while (dir is not null && !File.Exists(Path.Combine(dir.FullName, "WidgetWorks.slnx"))) + { + dir = dir.Parent; + } + + Assert.NotNull(dir); + var path = Path.Combine(dir!.FullName, "src", "WidgetWorks.WebApi", "appsettings.json"); + Assert.True(File.Exists(path), $"Expected appsettings.json at {path}."); + return path; + } + + private static int Number(JsonElement section, params string[] path) + { + var current = section; + foreach (var step in path) + { + Assert.True(current.TryGetProperty(step, out current), $"Missing '{string.Join(':', path)}'."); + } + + return current.GetInt32(); + } + + [Fact] + public void The_throttling_budgets_in_the_file_match_the_code_defaults() + { + var shipped = Section("RateLimiting"); + var code = new RateLimitOptions(); + + Assert.Equal(code.Auth.PermitLimit, Number(shipped, "Auth", "PermitLimit")); + Assert.Equal(code.Auth.WindowSeconds, Number(shipped, "Auth", "WindowSeconds")); + Assert.Equal(code.Checkout.PermitLimit, Number(shipped, "Checkout", "PermitLimit")); + Assert.Equal(code.Checkout.WindowSeconds, Number(shipped, "Checkout", "WindowSeconds")); + Assert.Equal(code.Lookup.PermitLimit, Number(shipped, "Lookup", "PermitLimit")); + Assert.Equal(code.Lookup.WindowSeconds, Number(shipped, "Lookup", "WindowSeconds")); + } + + [Fact] + public void The_shipped_default_does_not_trust_a_forwarded_header() + { + var shipped = Section("RateLimiting"); + + Assert.True(shipped.TryGetProperty("TrustForwardedFor", out var trust)); + // False is the safe default: believing the header with no proxy in front lets a caller forge + // it and give itself unlimited throttling partitions. A deployment behind a proxy must turn + // it on deliberately, which is why it is written here rather than left implicit. + Assert.False(trust.GetBoolean()); + Assert.False(new RateLimitOptions().TrustForwardedFor); + } + + [Fact] + public void The_reservation_sweep_settings_in_the_file_match_the_code_defaults() + { + var shipped = Section("Reservations"); + var code = new ReservationOptions(); + + Assert.Equal(code.ExpireAfterMinutes, Number(shipped, "ExpireAfterMinutes")); + Assert.Equal(code.SweepIntervalMinutes, Number(shipped, "SweepIntervalMinutes")); + Assert.Equal(code.BatchSize, Number(shipped, "BatchSize")); + + Assert.True(shipped.TryGetProperty("Enabled", out var enabled)); + Assert.Equal(code.Enabled, enabled.GetBoolean()); + } + + [Fact] + public void The_sweep_window_is_longer_than_the_interval_that_checks_it() + { + var code = new ReservationOptions(); + + // A window shorter than the sweep interval would mean orders sit expired but unreleased for + // most of their life, which quietly defeats the point of having a sweep. + Assert.True( + code.ExpireAfterMinutes > code.SweepIntervalMinutes, + "Reservations should expire over a longer span than the interval that looks for them."); + } +}