From ac5028a314329e521177e600d9770fb2efe16936 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 25 Aug 2026 23:41:21 +0000 Subject: [PATCH] docs: make the operational settings discoverable and the handbook true Two gaps left by the security and operability work, both of the same kind: the behaviour changed and nothing outside the code said so. The settings were invisible. Throttling budgets, the reservation sweep window and TrustForwardedFor existed only as C# defaults, so nobody deploying this could discover they were tunable at all. That made the load-bearing one a trap: the app warns when TrustForwardedFor is wrong for its traffic, but there was nothing in configuration to set right. appsettings.json now carries both sections with the values written out. Writing them down costs a second source of truth, so ShippedConfigurationTests guards it: the file and the code defaults are asserted equal in both directions, and a fourth test pins that the sweep window stays longer than the interval that looks for it - a window shorter than its own sweep leaves orders expired but unreleased for most of their life. Change one side without the other and the build fails rather than the file quietly starting to lie. The handbook did not know what the application does. Zero mentions of health/ready, correlation ids, TrustForwardedFor or the sweep. On a project whose handbook is a large part of what it is for, that drift undoes the work: a reader reads the handbook, not the diff. Configuration chapter gains a section on throttling, built around the setting that can cause an outage - what each direction of getting it wrong does, a table of which deployments need which value, and the fact that the warning it logs is not advisory - and a section on the sweep explaining ExpireAfterMinutes as a trade rather than a knob. Runbook gains the two probes with a table of which to point at what, and why merging them would cost roughly 180 CU-hrs against a 100 CU-hr budget; plus how to trace a customer's failure report through a correlation id. Bugs table gains the six defects from this round, including the three the tooling found rather than the author, and the lessons list gains four: let the row arbitrate rather than the handler, write what you mean rather than the whole row, read the reasoning before changing the code, and the gates catch what the author cannot. README and the setup guide list both endpoints, and the troubleshooting note now says which one keeps checking. 518 backend tests pass; build clean under -warnaserror; dotnet format clean. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01EA4mmpcb1rcvNntHR1iG6j --- README.md | 3 +- docs/handbook/03-setup-and-run.md | 6 +- docs/handbook/04-configuration-and-2fa.md | 64 ++++++++++ docs/handbook/08-bugs-and-lessons.md | 22 ++++ docs/handbook/09-runbook.md | 48 ++++++++ src/WidgetWorks.WebApi/appsettings.json | 21 ++++ .../ShippedConfigurationTests.cs | 112 ++++++++++++++++++ 7 files changed, 273 insertions(+), 3 deletions(-) create mode 100644 tests/WidgetWorks.ApiTests/ShippedConfigurationTests.cs 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."); + } +}