From 1eeea2c0dbab42b3b3fe8329ac8a9f72a0697f83 Mon Sep 17 00:00:00 2001 From: mforce <> Date: Mon, 10 Aug 2026 17:35:23 -0700 Subject: [PATCH 1/9] refactor(api): make process role explicit for boot guards MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A boot guard's scope was positional: what made #260 and #319 serving-only was sitting below CliDispatcher's return in Program.cs, so a one-shot verb had already exited before reaching them. That is not a property anyone can read off the guard, and getting it wrong is not a compile error — #331 is what wrong looks like, where the #316 OTLP validation ran at service registration and killed recover-admin with SIGABRT 134. ProcessRoles.From computes Serving | OneShot once, before the builder exists, and every role-scoped guard takes it. EnsureServingConfiguration is now called BEFORE the dispatch deliberately: a serving-only guard sitting ahead of the dispatcher and still not touching the verbs is the failure mode disarmed rather than avoided. Fixes a latent misclassification: IsCliInvocation read only CliDispatcher.Commands, which excludes healthcheck since it is not an ICliCommand, so the container's own probe classified as Serving. Harmless only because of its early return — statement position again. The verb list is now derived from Commands with healthcheck named explicitly. The TLS floor and tzdata canary take no role parameter: they apply to both, and an argument nobody branches on implies a branch that does not exist. No behaviour change in any role. --- AGENTS.md | 3 +- docs/decisions/347-process-role.md | 56 ++++++++ docs/decisions/README.md | 5 +- src/Cluckwork.Api/Cli/CliDispatcher.cs | 14 +- ...orkTelemetryServiceCollectionExtensions.cs | 15 ++- src/Cluckwork.Api/Hosting/ProcessRole.cs | 51 +++++++ .../Hosting/ServingBootGuards.cs | 75 +++++++++++ src/Cluckwork.Api/Program.cs | 66 +++------ .../AllowedHostsGuardTests.cs | 9 +- .../ProcessRoleGuardTests.cs | 127 ++++++++++++++++++ .../ProcessRoleRegistryTests.cs | 53 ++++++++ .../RecoverAdminCommandTests.cs | 4 +- .../SeedCommandTests.cs | 4 +- .../SimulationSeedCommandTests.cs | 4 +- tools/simulation/verify-harness.sh | 7 +- 15 files changed, 417 insertions(+), 76 deletions(-) create mode 100644 docs/decisions/347-process-role.md create mode 100644 src/Cluckwork.Api/Hosting/ProcessRole.cs create mode 100644 src/Cluckwork.Api/Hosting/ServingBootGuards.cs create mode 100644 tests/Cluckwork.Api.IntegrationTests/ProcessRoleGuardTests.cs create mode 100644 tests/Cluckwork.Api.IntegrationTests/ProcessRoleRegistryTests.cs diff --git a/AGENTS.md b/AGENTS.md index 0b5222566..7e12a8d00 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -54,7 +54,8 @@ dotnet test Cluckwork.sln # 688 tests as of 2026-07; integratio - **First-run admin provisioning: `bootstrap-admin` (#283).** `dotnet Cluckwork.Api.dll bootstrap-admin --email ` (run-then-exit, always available in Production) migrates, then — **only if the default account has no Owner** — creates one with a **freshly generated password (stdout only, never the logger/OTLP)** and `MustChangePassword=true`; a re-run is a **silent no-op**. While the flag is set the JWT carries `must_change_password`, `MustChangePasswordMiddleware` 403s every endpoint except `auth/change-password`+`auth/logout` (before `UseAuthorization` and before idempotency), and the SPA renders **Set your password**. Any successful reset clears the flag (one invariant in `IdentityProvider`). **Deliberately a separate credential type from #265/#308** — never conflated. → [`docs/decisions/283-first-run-admin-provisioning.md`](docs/decisions/283-first-run-admin-provisioning.md) - **Farm timezone + tzdata/ICU image constraint (#264):** the default account (a fixed `"UTC"` migration literal since #283) is provisioned once; a real farm sets its actual IANA zone via **Settings → timezone** after first login (`Account.UpdateSettings`), not at provisioning time (the `Seed:TimeZoneId` config lever from before #283 is retired along with the runtime seeder it fed). The farm clock resolves IANA zones via `TimeZoneInfo.FindSystemTimeZoneById` for every safety-sensitive local-day decision and **fails closed**, so the runtime **image MUST carry tzdata + ICU** (the Ubuntu 24.04 (Noble) `aspnet:10.0` base does; **never** an Alpine/chiseled base without tzdata, and **never** `InvariantGlobalization=true`). `Program.cs` still asserts a representative IANA canary resolves at boot (`TimeZoneAvailability.EnsureResolvable`) — a bad image fails the boot loudly instead of surfacing later as a per-request `FarmTimeZoneException`. - **Migrate command + prod migration split (#263).** `dotnet Cluckwork.Api.dll migrate` applies EF migrations then exits — the **pre-deploy-job entrypoint**. In Production `Database:MigrateOnStartup=false`, so the serving process never runs DDL; the **guarantee is ordering** — compose runs a one-shot `migrate` service before `app` waits on `service_completed_successfully`. `DatabaseReadyHealthCheck` is the backstop: `/health/ready` **503s while any migration is pending**. The remaining host step (deploy repo) is a least-privilege runtime role with no DDL, plus `ALTER DEFAULT PRIVILEGES` for the migrator. → [`docs/decisions/263-migrate-command.md`](docs/decisions/263-migrate-command.md) -- **Proxy-trust boot guard (#260):** behind a reverse proxy/edge, HSTS (#144) and the per-IP login limiter (#143) only work if the app trusts the proxy's `X-Forwarded-*`, which it does **only** for networks in `RateLimiting:TrustedProxies`. An **empty list in Production fails the boot** (a serving-process guard placed *after* the CLI dispatch, so `migrate`/`seed`/`recover-admin` are unaffected) rather than silently running with inert HSTS + a one-bucket limiter. Opt out with `RateLimiting:AllowNoTrustedProxies=true` only for a rare direct-TLS deploy with no fronting proxy. The concrete edge CIDR is deploy config (in the separate deployment/ops repo). +- **Proxy-trust boot guard (#260):** behind a reverse proxy/edge, HSTS (#144) and the per-IP login limiter (#143) only work if the app trusts the proxy's `X-Forwarded-*`, which it does **only** for networks in `RateLimiting:TrustedProxies`. An **empty list in Production fails the boot** (a `ProcessRole.Serving`-only guard, so `migrate`/`seed`/`recover-admin` are unaffected — see the process-role bullet below) rather than silently running with inert HSTS + a one-bucket limiter. Opt out with `RateLimiting:AllowNoTrustedProxies=true` only for a rare direct-TLS deploy with no fronting proxy. The concrete edge CIDR is deploy config (in the separate deployment/ops repo). +- **Process role, not statement order (#347).** A boot guard's scope is declared, never positional. `ProcessRoles.From(args)` (`Hosting/ProcessRole.cs`) computes `ProcessRole.Serving | OneShot` **once**, before the host is built, and every role-scoped guard takes it: `ServingBootGuards.EnsureServingConfiguration` (#260, #319) is called **before** the CLI dispatch on purpose and still leaves the verbs alone, and `AddCluckworkTelemetry` (#316) degrades to export-disabled for `OneShot`. The one-shot verb list is derived from `CliDispatcher.Commands` so a new verb classifies itself; **`healthcheck` is the one that must be named explicitly** (it isn't an `ICliCommand` — it needs no host) and omitting it silently classifies the container's own probe as `Serving`. **Never gate a guard on where its statement sits**: that is what #331 was — the #316 validation ran at service registration, ahead of the dispatcher, and killed `recover-admin` (the break-glass verb) with SIGABRT 134. The #261/#262 TLS floor and the #264 tzdata canary take no role: they apply to **both**, and unconditional code says so better than a parameter nobody branches on. → [`docs/decisions/347-process-role.md`](docs/decisions/347-process-role.md) - **Production Postgres TLS floor (#261/#262).** `ConnectionStrings:Default` accepts libpq URI or key-value form. In Production the effective `sslmode` is a **fail-closed allow-list**: `VerifyCA`/`VerifyFull` silent, `Require` warns, **everything else — `Disable`/`Allow`/`Prefer`, unset, or undefined — fails the boot**. `Database:AllowInsecureConnection=true` is the explicit opt-out for the co-located plaintext compose stack. When mapping a libpq param, **`ContainsKey("")==false` does NOT mean Npgsql lacks the setting** — usually the keyword is spelled differently (`keepalives`→`Tcp Keepalive`, `client_encoding`→`Client Encoding`); search the keyword list before concluding it's unmappable (that exact false negative shipped #332). Integration tests opt out of this **and** the #260 guard. → [`docs/decisions/261-postgres-tls-floor.md`](docs/decisions/261-postgres-tls-floor.md) - **GSS/Kerberos negotiation off by default (#332).** `PostgresConnectionString` appends `GSS Encryption Mode=Disable` **unless the operator set it** (detected by **presence, not value**), **textually** after the TLS floor runs (a round-trip through the Npgsql builder would reorder/requote the operator's string and throw on any keyword this version doesn't know). Without it, Npgsql's `Prefer` default makes every connector dlopen the absent `libgssapi_krb5.so.2` and print two pre-Serilog stderr lines per process. Orthogonal to `sslmode` and to GSS *authentication*. → [`docs/decisions/332-gss-kerberos.md`](docs/decisions/332-gss-kerberos.md) - **Design-time migration connection fail-closed (#318):** `AppDbContextDesignTimeFactory` (used by `dotnet ef migrations add`/`database update`, never by the `migrate` verb, which uses the built host's own config) has **no default connection** — an unset/blank `CLUCKWORK_MIGRATIONS_CONNECTION` throws immediately, naming the variable, rather than falling back to a predictable `Host=localhost;...;Username=postgres;Password=postgres`. Every target is held to the **same allow-list TLS floor as a Production boot** (#261/#262, reused via `PostgresConnectionString.NormalizeAndValidate` — no second validator), except an explicitly acknowledged **loopback** development target: `CLUCKWORK_MIGRATIONS_ALLOW_INSECURE_LOOPBACK=true` permits plaintext, but only when the connection's host is `localhost`/`127.0.0.1`/`::1` (checked via `IPAddress.IsLoopback`) — set against any other host, it fails rather than silently widening scope. diff --git a/docs/decisions/347-process-role.md b/docs/decisions/347-process-role.md new file mode 100644 index 000000000..2e9af32a3 --- /dev/null +++ b/docs/decisions/347-process-role.md @@ -0,0 +1,56 @@ +# Process role, not statement order (#347) + +> **Rule** — the one-paragraph version lives in [`AGENTS.md`](../../AGENTS.md); this file is the relocated rationale (what shipped, why the short version was insufficient, what not to break). + +## What was wrong + +The API binary serves HTTP **and** carries five run-then-exit operator verbs — `migrate`, `seed`, `recover-admin`, `bootstrap-admin`, `healthcheck`. Several Production boot guards apply to only one of those roles. Nothing said so. What made a guard serving-only was the **position of its statement** in `Program.cs`: below `CliDispatcher.TryRunAsync`'s return, so a one-shot verb had already exited before reaching it. + +That is not a property anyone can read off the guard, and getting it wrong is not a compile error. It went wrong twice: + +- **#331 — the one with teeth.** The #316 OTLP endpoint validation ran at *service registration*, ahead of the dispatcher. A plaintext `Otlp:Endpoint` therefore killed `recover-admin` with SIGABRT 134 — the break-glass verb, the one that exists for when everything else is broken, taken out by a guard that protects a serving process's telemetry. The fix at the time was a `!CliDispatcher.IsCliInvocation(args)` bool threaded into registration: correct, but it made the *second* mechanism for the same idea. +- **A latent third.** `IsCliInvocation` was derived from `CliDispatcher.Commands`, which holds only the four verbs that dispatch after `Build()`. `healthcheck` is not among them — it is not an `ICliCommand` (it needs no host, so it takes no `WebApplication`) and `Program.cs` dispatches it before the builder exists. So the predicate classified the container's own health probe as a **serving** process. Harmless only because of that early return, i.e. harmless because of statement position again. + +## What shipped + +`ProcessRoles.From(args)` (`src/Cluckwork.Api/Hosting/ProcessRole.cs`) computes `ProcessRole.Serving | OneShot` **once**, before `WebApplication.CreateBuilder`, and every role-scoped guard takes it as an argument: + +| Guard | Role | Where | +|---|---|---| +| #260 trusted proxies | `Serving` only | `ServingBootGuards.EnsureServingConfiguration` | +| #319 AllowedHosts | `Serving` only | `ServingBootGuards.EnsureServingConfiguration` | +| #316 OTLP endpoint | `Serving` throws; `OneShot` degrades to export-disabled | `AddCluckworkTelemetry` | +| #261/#262 Postgres TLS floor | **both** | `AddCluckworkPersistence`, unconditional | +| #264 tzdata/ICU canary | **both** | `AddCluckworkPersistence`, unconditional | + +Three details are load-bearing. + +**`EnsureServingConfiguration` is called BEFORE the CLI dispatch, on purpose.** Leaving it below would have worked and proved nothing. A serving-only guard sitting *ahead* of the dispatcher and still not touching the verbs is the #331 failure mode disarmed rather than avoided, and it is the worked example the next guard author needs. Move that call again and nothing breaks — that is the point. + +**The verb list is derived, not restated.** `OneShotVerbs` reads `CliDispatcher.Commands`, so a sixth verb classifies itself without anyone remembering this file. `healthcheck` is the single name written by hand, because it structurally cannot come from there. Omitting it is the latent bug above. + +**The both-roles guards take no `ProcessRole` parameter.** An argument nobody branches on implies a branch that does not exist, and the next reader has to check whether it is ignored deliberately or by accident. Unconditional code states "applies to both" more strongly than a parameter can; this table is where that classification is recorded. + +## What was rejected + +**A separate `Cluckwork.Cli` assembly**, which #347 originally asked for alongside this. Its stated payoff was "verbs gain unit-level coverage that does not need a web host" — but `ICliCommand.RunAsync` takes a `WebApplication`, so a class library holding the same files still needs a fully built web app to run anything. The move delivers the churn and none of the benefit. Getting the benefit means changing the verb signature, which is a different and larger change, and one [#423](https://github.com/mforce/cluckwork/pull/423)'s Phase 10 ("convert CLI verbs to contracts") rewrites anyway. + +**An `IBootGuard` collection with a runner.** It reads well and matches how the issue described the target shape, but the thing you would then be trusting is the registration list, and a guard nobody registers silently never runs. Per the [guard-writing rules](407-writing-a-guard.md): prefer the boring guard, because complexity costs double when the complicated thing is the thing you are trusting. + +**Converting #316's degrade to a pre-check.** `if (role is Serving) validate…` reads more uniformly, but the current `catch … when (role is OneShot)` writes `warning: OTLP export disabled for this command — ` to stderr. That line is the only thing that tells an operator why a one-shot run is exporting nothing. Skipping validation entirely would silently disable export with no reason attached. + +## How it is pinned + +`ProcessRoleGuardTests` (integration, subprocess) is the guarantee, and it is deliberately **two-sided** — asserting that `migrate` exits 0 under a hostile serving configuration proves nothing unless that configuration genuinely would fail a serving boot. Arm 1 runs the real binary with `ASPNETCORE_ENVIRONMENT=Production`, `TrustedProxies` empty, `AllowedHosts=*` and a plaintext `Otlp:Endpoint`, and requires exit 0. Arm 2 starts the same binary with the same environment and **no verb**, and requires it to die with a message naming one of the three guards. Which guard wins is not asserted: #316 throws during registration and the other two after `Build()`, and pinning that order would turn a legitimate reordering into a red test. + +`ProcessRoleRegistryTests` (fast, no host) covers the classification itself, including the one case the subprocess suite structurally cannot reach — `healthcheck` returns before a host exists, so no boot guard can observe its role today. + +Mutation evidence, run before the claim (baseline green, restore green): + +| Mutation | Result | +|---|---| +| `EnsureServingConfiguration` drops its `role` check | **red** (`migrate` aborts on #260) | +| #316's degrade filter flipped to `Serving` | **red** — this is #331 verbatim | +| `healthcheck` removed from `OneShotVerbs` | **red** (registry suite) | + +One process note, because it nearly produced false evidence: the first mutation script restored with `git checkout --`, which **silently refuses to restore untracked files**. Two of the three targets were new files, so mutants stacked instead of reverting and two "red" results were measured against an already-mutated tree. Snapshot by file copy, and assert the restored tree is green again. diff --git a/docs/decisions/README.md b/docs/decisions/README.md index e6723b758..036715e92 100644 --- a/docs/decisions/README.md +++ b/docs/decisions/README.md @@ -23,6 +23,7 @@ follow the `→` link from the `AGENTS.md` bullet to get here. | [Seed command and simulation profile (#280, #284, #279)](280-seed-and-simulation.md) | AGENTS · Conventions | | [First-run admin provisioning: `bootstrap-admin` (#283)](283-first-run-admin-provisioning.md) | AGENTS · Conventions | | [Migrate command + prod migration split (#263)](263-migrate-command.md) | AGENTS · Conventions | +| [Process role, not statement order (#347)](347-process-role.md) | AGENTS · Conventions | | [Production Postgres TLS floor + libpq mapping (#261/#262)](261-postgres-tls-floor.md) | AGENTS · Conventions | | [GSS/Kerberos negotiation off by default (#332)](332-gss-kerberos.md) | AGENTS · Conventions | | [Container health probe: the `healthcheck` verb (#266)](266-container-health-probe.md) | AGENTS · Conventions | @@ -40,4 +41,6 @@ The five short-enough Conventions bullets that kept their full rationale inline `AGENTS.md` — break-glass recovery (#265), farm timezone (#264), the proxy-trust boot guard (#260), the design-time migration connection (#318), and container image hardening (#267) — have no record here because none of their text was -relocated. +relocated. #260 is the partial exception: its *rule* is still inline, but why it +applies to the serving process and not to the CLI verbs now lives in the +[#347 record](347-process-role.md) along with #319 and #316. diff --git a/src/Cluckwork.Api/Cli/CliDispatcher.cs b/src/Cluckwork.Api/Cli/CliDispatcher.cs index 04658a026..ef1ecad34 100644 --- a/src/Cluckwork.Api/Cli/CliDispatcher.cs +++ b/src/Cluckwork.Api/Cli/CliDispatcher.cs @@ -20,13 +20,13 @@ public static class CliDispatcher new BootstrapAdminCliCommand(), ]; - // Whether these args will dispatch to a one-off verb rather than start the - // web host. TryRunAsync can only answer this AFTER Build(), but service - // registration needs to know sooner: a registration-time guard that throws - // would abort a verb before it ever dispatched. Same reason #260/#319 place - // their serving-process guards after this dispatcher. - public static bool IsCliInvocation(string[] args) => - args.Length > 0 && Array.Exists(Commands, c => c.Name == args[0]); + // Whether these args dispatch to a one-off verb rather than start the web + // host is answered by ProcessRoles.From(args) (Hosting/ProcessRole.cs), which + // reads Commands above so the two can't disagree. It deliberately does NOT + // live here: this dispatcher only knows the four verbs that run AFTER + // Build(), and `healthcheck` — dispatched before the host exists — is a + // one-shot process too. A predicate built from Commands alone called it + // Serving (#347). public static async Task TryRunAsync(WebApplication app, string[] args) { diff --git a/src/Cluckwork.Api/Hosting/CluckworkTelemetryServiceCollectionExtensions.cs b/src/Cluckwork.Api/Hosting/CluckworkTelemetryServiceCollectionExtensions.cs index 1e87a36bd..d024d48f8 100644 --- a/src/Cluckwork.Api/Hosting/CluckworkTelemetryServiceCollectionExtensions.cs +++ b/src/Cluckwork.Api/Hosting/CluckworkTelemetryServiceCollectionExtensions.cs @@ -10,15 +10,18 @@ namespace Cluckwork.Api.Hosting; internal static class CluckworkTelemetryServiceCollectionExtensions { - // isServingProcess is false for the one-off CLI verbs (migrate / seed / - // recover-admin). They dispatch after Build(), so a throw during service - // registration would abort them before CliDispatcher ever ran — see the - // endpoint-resolution block below for why that matters. + // role is OneShot for the operator verbs (migrate / seed / recover-admin / + // bootstrap-admin / healthcheck). They dispatch after Build(), so a throw + // during service registration would abort them before CliDispatcher ever ran + // — see the endpoint-resolution block below for why that matters. #347: the + // role is computed once in Program.cs and passed in, so this is the guard's + // own declared scope rather than a positional bool whose meaning lives at a + // call site far away. public static CluckworkTelemetryRegistration AddCluckworkTelemetry( this IServiceCollection services, IConfiguration configuration, IHostEnvironment environment, - bool isServingProcess = true) + ProcessRole role = ProcessRole.Serving) { // ReadFrom.Services lets DI-registered enrichers/sinks join the pipeline — // the integration tests tap the logger this way (#214). @@ -68,7 +71,7 @@ public static CluckworkTelemetryRegistration AddCluckworkTelemetry( traceEndpoint = otlp.ResolveTraceEndpoint(isProduction); metricsEndpoint = otlp.ResolveMetricsEndpoint(isProduction); } - catch (InvalidOperationException ex) when (!isServingProcess) + catch (InvalidOperationException ex) when (role is ProcessRole.OneShot) { // A one-off verb must not die on a telemetry misconfiguration it // does not depend on. `recover-admin` in particular is the diff --git a/src/Cluckwork.Api/Hosting/ProcessRole.cs b/src/Cluckwork.Api/Hosting/ProcessRole.cs new file mode 100644 index 000000000..d604fa7b6 --- /dev/null +++ b/src/Cluckwork.Api/Hosting/ProcessRole.cs @@ -0,0 +1,51 @@ +namespace Cluckwork.Api.Hosting; + +using Cluckwork.Api.Cli; + +// #347 — what this process was started to BE, decided once from args before +// anything is registered or built. +// +// The problem it replaces: several boot guards were serving-process-only, and +// what made them so was WHERE their statement sat in Program.cs — after the CLI +// dispatch meant a one-shot verb never reached them. That is not a property +// anyone can read off the guard, so every new guard reopened the question "does +// this apply to `migrate`?", and getting it wrong is not a compile error. #331 +// is what getting it wrong looks like: the #316 OTLP endpoint validation ran at +// service registration, ahead of the dispatcher, and killed `recover-admin` — +// the break-glass verb, the one that has to work when everything else is +// broken — with SIGABRT 134. +// +// Now each guard checks the role itself, so its scope survives being moved. +public enum ProcessRole +{ + // Started to serve HTTP traffic: Kestrel binds, hosted services run. + Serving, + + // Started to run one operator verb and exit. Kestrel never binds and the + // hosted services never run, so guards protecting the SERVING process's + // security posture have nothing to protect here — and a guard that aborts + // one of these verbs takes out an operational escape hatch instead. + OneShot, +} + +public static class ProcessRoles +{ + // Every verb whose presence at args[0] makes this a one-shot process. + // + // Derived from CliDispatcher.Commands rather than restated, so a sixth verb + // added to the dispatcher is classified correctly without anyone + // remembering this file. `healthcheck` cannot come from there: it is not an + // ICliCommand (it needs no host, so it takes no WebApplication) and is + // dispatched by Program.cs before the host is built — which is exactly why + // it is named here explicitly. Leaving it out is not harmless-by-luck: it + // would classify the container's own health probe as a SERVING process, and + // the only thing standing between that and a guard abort would once again + // be a statement's position in Program.cs. + internal static readonly string[] OneShotVerbs = + [.. CliDispatcher.Commands.Select(command => command.Name), HealthCheckCliCommand.Verb]; + + public static ProcessRole From(string[] args) => + args.Length > 0 && Array.IndexOf(OneShotVerbs, args[0]) >= 0 + ? ProcessRole.OneShot + : ProcessRole.Serving; +} diff --git a/src/Cluckwork.Api/Hosting/ServingBootGuards.cs b/src/Cluckwork.Api/Hosting/ServingBootGuards.cs new file mode 100644 index 000000000..b3d0852c9 --- /dev/null +++ b/src/Cluckwork.Api/Hosting/ServingBootGuards.cs @@ -0,0 +1,75 @@ +namespace Cluckwork.Api.Hosting; + +// #347 — the Production boot guards that protect the SERVING process's security +// posture, gathered behind one role-checked entry point. +// +// Both of these used to sit inline in Program.cs BELOW the CLI dispatcher's +// return, and that position was the whole of what stopped them aborting +// `migrate`/`seed`/`recover-admin`/`bootstrap-admin`. Program.cs now calls this +// BEFORE the dispatch, deliberately: a serving-only guard sitting ahead of the +// dispatcher and still not touching the one-shot verbs is the #331 failure mode +// disarmed rather than merely avoided. Move this call again and nothing breaks. +internal static class ServingBootGuards +{ + // Gated on IsProduction() (not !IsDevelopment()) deliberately: the + // integration Testing environment is also empty-proxied and wildcard-hosted + // and must still boot; a real Staging serving environment, if ever + // introduced, would be added to this gate. + public static void EnsureServingConfiguration( + ProcessRole role, + IHostEnvironment environment, + IConfiguration configuration, + CluckworkRateLimitingRegistration rateLimiting) + { + if (role is not ProcessRole.Serving || !environment.IsProduction()) + return; + + EnsureTrustedProxiesConfigured(rateLimiting); + EnsureAllowedHostsPinned(configuration); + } + + // #260 — the forwarded-headers middleware honours X-Forwarded-Proto/-For + // solely from the trustedProxies networks; with that list empty in + // Production two controls silently go inert: HSTS (#144) never sees the real + // HTTPS scheme and stops emitting, and the per-IP login rate limiter (#143) + // collapses to one global bucket (every request looks like it came from the + // proxy hop). Fail the boot loudly rather than run degraded. + private static void EnsureTrustedProxiesConfigured(CluckworkRateLimitingRegistration rateLimiting) + { + if (rateLimiting.TrustedProxies.Length != 0 || rateLimiting.Options.AllowNoTrustedProxies) + return; + + throw new InvalidOperationException( + "RateLimiting:TrustedProxies is empty in Production, so the app trusts no " + + "proxy's X-Forwarded-* headers. Two security controls then silently go " + + "inert: HSTS (#144) never sees the real HTTPS scheme and stops emitting, " + + "and the per-IP login rate limiter (#143) collapses to a single global " + + "bucket. Fix ONE of: (1) set RateLimiting:TrustedProxies to the edge " + + "proxy/load-balancer network CIDR (the hop that terminates TLS and adds " + + "X-Forwarded-*); or (2) for a rare deploy that terminates TLS itself with " + + "no fronting proxy, set RateLimiting:AllowNoTrustedProxies=true to " + + "acknowledge the direct-exposure trade-off and boot anyway."); + } + + // #319 — appsettings.json defaults AllowedHosts to "*"; a deploy that omits + // or misnames the host variable (a blank ${CLUCKWORK_HOST} substitution was + // observed) then silently disables Host-header filtering (#144) and a forged + // Host header is accepted. Fail the boot loudly unless a concrete public host + // is pinned. Loopback is force-added for health probes later + // (AddCluckworkEdgeSecurity), so it need not appear in config here. + private static void EnsureAllowedHostsPinned(IConfiguration configuration) + { + var configuredHosts = (configuration["AllowedHosts"] ?? string.Empty) + .Split(';', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries); + var hasConcretePublicHost = configuredHosts.Length > 0 && configuredHosts.All(h => h != "*"); + if (hasConcretePublicHost) + return; + + throw new InvalidOperationException( + "AllowedHosts is missing, blank, or wildcard ('*') in Production, so Host-header " + + "filtering (#144) is off and a forged Host header is accepted. Set AllowedHosts to " + + "the concrete public hostname the app serves (the deployment supplies it as " + + "CLUCKWORK_HOST). Loopback (localhost/127.0.0.1/[::1]) is always allowed for " + + "container health probes, so it need not be listed."); + } +} diff --git a/src/Cluckwork.Api/Program.cs b/src/Cluckwork.Api/Program.cs index 0d4b013bd..ae0c6dfa5 100644 --- a/src/Cluckwork.Api/Program.cs +++ b/src/Cluckwork.Api/Program.cs @@ -39,10 +39,17 @@ if (args is [HealthCheckCliCommand.Verb, ..]) return await HealthCheckCliCommand.RunAsync(args); +// #347 — decide ONCE what this process was started to be. Every role-scoped boot +// guard below takes this rather than relying on where its statement sits +// relative to the CLI dispatch. `healthcheck` is a one-shot verb too and is +// classified as one (see ProcessRoles.OneShotVerbs); its early return above is +// a startup-cost optimisation, not what makes it safe. +var processRole = ProcessRoles.From(args); + var builder = WebApplication.CreateBuilder(args); var telemetry = builder.Services.AddCluckworkTelemetry( - builder.Configuration, builder.Environment, !CliDispatcher.IsCliInvocation(args)); + builder.Configuration, builder.Environment, processRole); var persistence = builder.Services.AddCluckworkPersistence( builder.Configuration, @@ -92,6 +99,13 @@ foreach (var connectionStringWarning in persistence.ConnectionStringWarnings) app.Logger.LogWarning("{ConnectionStringWarning}", connectionStringWarning); +// #260/#319 — Production boot guards for the SERVING process's security posture. +// Deliberately called BEFORE the CLI dispatch below: what spares the one-shot +// verbs is the role check inside, not this call's position, and putting it here +// is the standing demonstration of that (#347). See Hosting/ServingBootGuards.cs. +ServingBootGuards.EnsureServingConfiguration( + processRole, app.Environment, builder.Configuration, rateLimiting); + // One-off operator commands (seed / migrate / recover-admin) run then EXIT // before the web host starts — Kestrel and the hosted services never run for // these. Each lives in Cluckwork.Api.Cli; the dispatcher returns the exit @@ -100,56 +114,6 @@ if (await CliDispatcher.TryRunAsync(app, args) is int cliExitCode) return cliExitCode; -// #260 — proxy-trust boot guard, SERVING process only. The forwarded-headers -// middleware above honours X-Forwarded-Proto/-For solely from the trustedProxies -// networks; with that list empty in Production two controls silently go inert: -// HSTS (#144) never sees the real HTTPS scheme and stops emitting, and the per-IP -// login rate limiter (#143) collapses to one global bucket (every request looks -// like it came from the proxy hop). Fail the boot loudly rather than run degraded. -// Placed AFTER the CLI dispatcher's return so the one-off migrate/seed/recover-admin -// verbs — which never serve traffic — are unaffected. Opt out only for a rare -// direct-TLS-exposure deploy via RateLimiting:AllowNoTrustedProxies. -// Gated on IsProduction() (not !IsDevelopment()) deliberately: the integration -// Testing env is also empty-proxied and must still boot; a real Staging serving -// env, if ever introduced, would be added to this gate. -if (app.Environment.IsProduction() - && rateLimiting.TrustedProxies.Length == 0 - && !rateLimiting.Options.AllowNoTrustedProxies) -{ - throw new InvalidOperationException( - "RateLimiting:TrustedProxies is empty in Production, so the app trusts no " - + "proxy's X-Forwarded-* headers. Two security controls then silently go " - + "inert: HSTS (#144) never sees the real HTTPS scheme and stops emitting, " - + "and the per-IP login rate limiter (#143) collapses to a single global " - + "bucket. Fix ONE of: (1) set RateLimiting:TrustedProxies to the edge " - + "proxy/load-balancer network CIDR (the hop that terminates TLS and adds " - + "X-Forwarded-*); or (2) for a rare deploy that terminates TLS itself with " - + "no fronting proxy, set RateLimiting:AllowNoTrustedProxies=true to " - + "acknowledge the direct-exposure trade-off and boot anyway."); -} - -// #319 — AllowedHosts boot guard, SERVING process only. appsettings.json defaults -// AllowedHosts to "*"; a deploy that omits or misnames the host variable (a blank -// ${CLUCKWORK_HOST} substitution was observed) then silently disables Host-header -// filtering (#144) and a forged Host header is accepted. Fail the boot loudly -// unless a concrete public host is pinned. Loopback is force-added for health -// probes later (AddCluckworkEdgeSecurity), so it need not appear in config here. -// Placed AFTER the CLI dispatcher's return (like #260) so the one-off migrate/ -// seed/recover-admin verbs are unaffected; healthcheck already early-dispatches. -if (app.Environment.IsProduction()) -{ - var configuredHosts = (builder.Configuration["AllowedHosts"] ?? string.Empty) - .Split(';', StringSplitOptions.RemoveEmptyEntries | StringSplitOptions.TrimEntries); - var hasConcretePublicHost = configuredHosts.Length > 0 && configuredHosts.All(h => h != "*"); - if (!hasConcretePublicHost) - throw new InvalidOperationException( - "AllowedHosts is missing, blank, or wildcard ('*') in Production, so Host-header " - + "filtering (#144) is off and a forged Host header is accepted. Set AllowedHosts to " - + "the concrete public hostname the app serves (the deployment supplies it as " - + "CLUCKWORK_HOST). Loopback (localhost/127.0.0.1/[::1]) is always allowed for " - + "container health probes, so it need not be listed."); -} - // One boot line makes export misconfiguration observable — a typo'd env var // name otherwise silently disables the whole pipeline (#226 review). if (telemetry.TraceEndpoint is not null) diff --git a/tests/Cluckwork.Api.IntegrationTests/AllowedHostsGuardTests.cs b/tests/Cluckwork.Api.IntegrationTests/AllowedHostsGuardTests.cs index c963fb2c9..3b8b16236 100644 --- a/tests/Cluckwork.Api.IntegrationTests/AllowedHostsGuardTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/AllowedHostsGuardTests.cs @@ -11,10 +11,11 @@ namespace Cluckwork.Api.IntegrationTests; // public host is pinned. The base factory runs "Testing" (guard dormant); this // factory flips to Production with a concrete host so the good path boots, and the // failure cases derive a host that removes the concrete value — mirroring -// TrustedProxyGuardTests. CLI-verb ordering (the guard sits AFTER the CLI -// dispatcher's return) is covered by MigrateCommandTests, which already run in -// Production with no AllowedHosts set — i.e. the appsettings "*" wildcard — and -// must stay green, proving migrate never reaches this serving-only guard. +// TrustedProxyGuardTests. That the guard leaves the one-shot CLI verbs alone is +// no longer a matter of where it sits (#347 moved it ahead of the dispatcher and +// gave it an explicit ProcessRole check): it is covered by ProcessRoleGuardTests, +// and by MigrateCommandTests, which already runs in Production with no +// AllowedHosts set — i.e. the appsettings "*" wildcard — and must stay green. public sealed class AllowedHostsProductionFactory : CluckworkWebApplicationFactory { public const string PublicHost = "cluckwork.example"; diff --git a/tests/Cluckwork.Api.IntegrationTests/ProcessRoleGuardTests.cs b/tests/Cluckwork.Api.IntegrationTests/ProcessRoleGuardTests.cs new file mode 100644 index 000000000..2a8138d74 --- /dev/null +++ b/tests/Cluckwork.Api.IntegrationTests/ProcessRoleGuardTests.cs @@ -0,0 +1,127 @@ +namespace Cluckwork.Api.IntegrationTests; + +using System.Diagnostics; +using Cluckwork.Api.IntegrationTests.Infrastructure; +using Testcontainers.PostgreSql; + +// #347 — a SERVING-process boot guard must never abort a ONE-SHOT verb. +// +// This is the #331 shape, generalised. #331: the #316 OTLP endpoint validation +// ran at service registration, before CliDispatcher, so a plaintext endpoint +// killed `recover-admin` with SIGABRT 134 — the break-glass verb, the one that +// has to work when everything else is broken, taken out by a guard that exists +// for the serving process. The fix at the time was a `!IsCliInvocation(args)` +// bool threaded into registration; the fix here is a ProcessRole computed once +// and checked by each guard, so the answer to "does this guard fire for +// migrate?" is a property of the guard rather than of where its statement sits +// in Program.cs. +// +// The test is deliberately TWO-SIDED, because the one-sided version is worthless: +// asserting `migrate` exits 0 under a hostile serving configuration proves +// nothing unless that configuration genuinely WOULD fail a serving boot. So the +// second arm starts the same binary with the same environment and NO verb, and +// requires it to die naming a guard. Together: the config is serving-hostile, +// and the one-shot verb survives it. +// +// Each of the three settings below is independently load-bearing in the first +// arm — misclassify any single guard as applying to OneShot and `migrate` +// aborts, so this goes red without needing one subprocess per guard. +public sealed class ProcessRoleGuardTests +{ + private static readonly string ApiDllPath = typeof(Program).Assembly.Location; + + // The migrate arm applies a full schema to a fresh database; the serving arm + // is expected to die during boot, long before it would bind Kestrel, so it + // gets a much shorter bound — a serving start that DOESN'T die is the failure + // this arm is looking for, and waiting 120s to discover that is pure delay. + private static readonly TimeSpan MigrateTimeout = TimeSpan.FromSeconds(120); + private static readonly TimeSpan ServingBootTimeout = TimeSpan.FromSeconds(45); + + // Configuration that a Production SERVING boot must reject on three separate, + // independent guards: + // #260 RateLimiting:TrustedProxies empty -> HSTS + the per-IP login limiter + // silently go inert. + // #319 AllowedHosts "*" -> Host-header filtering is off. + // #316 Otlp:Endpoint plaintext http in -> telemetry and Otlp:Headers + // Production, no AllowInsecureEndpoint credentials exposed in transit. + // None of the three has any bearing on what a one-shot verb does. + private static void ApplyHostileServingConfiguration(ProcessStartInfo psi) + { + // Left EMPTY on purpose (no RateLimiting__TrustedProxies__0 entry) — that + // absence IS the #260 condition. + psi.Environment["AllowedHosts"] = "*"; + psi.Environment["Otlp__Endpoint"] = "http://collector.invalid:4317"; + } + + private static Process Start(string verb, string connectionString) + { + var arguments = verb.Length == 0 ? $"\"{ApiDllPath}\"" : $"\"{ApiDllPath}\" {verb}"; + var psi = new ProcessStartInfo("dotnet", arguments) + { + RedirectStandardOutput = true, + RedirectStandardError = true, + UseShellExecute = false, + }; + psi.Environment["ASPNETCORE_ENVIRONMENT"] = "Production"; + psi.Environment["ConnectionStrings__Default"] = connectionString; + psi.Environment["Database__Provider"] = "Postgres"; + // The Testcontainers database is plaintext. The #261/#262 TLS floor applies + // to BOTH roles by design, so it is opted out of here rather than being one + // of the hostile settings under test — this suite is about role-scoped + // guards, and the TLS floor deliberately isn't one. + psi.Environment["Database__AllowInsecureConnection"] = "true"; + psi.Environment["Jwt__Issuer"] = "cluckwork-test"; + psi.Environment["Jwt__Audience"] = "cluckwork-api-test"; + psi.Environment["Jwt__PublicKeyPem"] = TestJwtKeys.PublicKeyPem; + psi.Environment["Jwt__PrivateKeyPem"] = TestJwtKeys.PrivateKeyPem; + // Port 0 = an ephemeral port, so the serving arm cannot collide with + // anything else on the machine in the (failing) case where it boots. + psi.Environment["ASPNETCORE_URLS"] = "http://127.0.0.1:0"; + ApplyHostileServingConfiguration(psi); + return Process.Start(psi)!; + } + + [Fact] + public async Task ServingOnlyGuards_DoNotAbortAOneShotVerb_AndStillFailAServingBoot() + { + await using var db = new PostgreSqlBuilder("postgres:18.4-trixie@sha256:3a82e1f56c8f0f5616a11103ac3d47e632c3938698946a7ad26da0df1334744a").Build(); + await db.StartAsync(); + var connectionString = db.GetConnectionString(); + + // ARM 1 — the guarantee. The one-shot verb runs to completion under the + // hostile serving configuration. This is what #331 broke. + var (migrateExit, migrateOut, migrateErr) = await SeedCommandRunner.RunToCompletionAsync( + Start("migrate", connectionString), MigrateTimeout); + + Assert.True( + 0 == migrateExit, + "a serving-only boot guard aborted the one-shot `migrate` verb (the #331 regression). " + + $"expected exit 0, got {migrateExit}. stdout={migrateOut} stderr={migrateErr}"); + + // ARM 2 — proof that arm 1 was not vacuous. Same binary, same environment, + // no verb: this is a serving start, and it must die. Asserting only "exit + // non-zero" would accept a death from an unrelated cause (a missing key, an + // unreachable database), so the message has to name one of the three + // guards. Which one wins is NOT asserted: #316 throws during service + // registration and the other two after Build(), and pinning that order here + // would turn a legitimate future reordering into a red test for no reason. + var (servingExit, servingOut, servingErr) = await SeedCommandRunner.RunToCompletionAsync( + Start(string.Empty, connectionString), ServingBootTimeout); + + Assert.True( + servingExit != 0, + "the configuration this test calls hostile did not actually fail a serving boot, so " + + "arm 1 proved nothing. Re-check the #260/#316/#319 conditions. " + + $"stdout={servingOut} stderr={servingErr}"); + + var namesAGuard = + servingErr.Contains("RateLimiting:TrustedProxies", StringComparison.Ordinal) + || servingErr.Contains("AllowedHosts", StringComparison.Ordinal) + || servingErr.Contains("Otlp:Endpoint", StringComparison.Ordinal); + Assert.True( + namesAGuard, + "the serving boot failed, but not on any of the three guards this test configures — " + + "so arm 1 is not exercising what it claims. " + + $"exit={servingExit} stdout={servingOut} stderr={servingErr}"); + } +} diff --git a/tests/Cluckwork.Api.IntegrationTests/ProcessRoleRegistryTests.cs b/tests/Cluckwork.Api.IntegrationTests/ProcessRoleRegistryTests.cs new file mode 100644 index 000000000..74200eaf9 --- /dev/null +++ b/tests/Cluckwork.Api.IntegrationTests/ProcessRoleRegistryTests.cs @@ -0,0 +1,53 @@ +namespace Cluckwork.Api.IntegrationTests; + +using Cluckwork.Api.Cli; +using Cluckwork.Api.Hosting; + +// #347 — the verb registry ProcessRoles.From reads. Fast, no host, no container. +// +// This suite is NOT the guarantee: proving that a serving-only guard leaves a +// one-shot verb alone needs the real binary, and that is ProcessRoleGuardTests. +// What this covers is the classification itself, and one case the subprocess +// suite structurally cannot reach — `healthcheck` returns before the host is +// ever built, so no boot guard can observe its role today. Which is precisely +// why the classification needs pinning here: the only thing keeping the health +// probe safe would otherwise be, once again, where a statement sits in +// Program.cs. +public sealed class ProcessRoleRegistryTests +{ + [Fact] + public void EveryDispatchedVerb_IsAOneShotProcess() + { + // Walk the dispatcher's own registry rather than a list written here: a + // sixth verb must be classified without anyone remembering this test. + foreach (var command in CliDispatcher.Commands) + Assert.Equal(ProcessRole.OneShot, ProcessRoles.From([command.Name])); + } + + // Not an ICliCommand and not dispatched by CliDispatcher, so a predicate + // derived from Commands alone classifies it Serving — the latent bug #347 + // found. Named explicitly here because that is the only way it can be wrong. + [Fact] + public void Healthcheck_IsAOneShotProcess() + { + Assert.Equal(ProcessRole.OneShot, ProcessRoles.From([HealthCheckCliCommand.Verb])); + Assert.Contains(HealthCheckCliCommand.Verb, ProcessRoles.OneShotVerbs); + } + + [Fact] + public void AVerbCarryingItsOwnFlags_IsStillAOneShotProcess() + { + Assert.Equal( + ProcessRole.OneShot, + ProcessRoles.From(["recover-admin", "--email", "owner@example.test", "--reason", "drill"])); + } + + [Fact] + public void AnythingElse_IsAServingProcess() + { + Assert.Equal(ProcessRole.Serving, ProcessRoles.From([])); // plain serving start + Assert.Equal(ProcessRole.Serving, ProcessRoles.From(["not-a-verb"])); // unknown first argument + Assert.Equal(ProcessRole.Serving, ProcessRoles.From(["--urls", "http://+:8080"])); // host arguments, no verb + Assert.Equal(ProcessRole.Serving, ProcessRoles.From(["--profile", "migrate"])); // a verb NAME, not at args[0] + } +} diff --git a/tests/Cluckwork.Api.IntegrationTests/RecoverAdminCommandTests.cs b/tests/Cluckwork.Api.IntegrationTests/RecoverAdminCommandTests.cs index 615089ee0..f2da7c89e 100644 --- a/tests/Cluckwork.Api.IntegrationTests/RecoverAdminCommandTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/RecoverAdminCommandTests.cs @@ -42,7 +42,9 @@ private Process StartRecoverCommand( psi.Environment["ConnectionStrings__Default"] = _factory.ConnectionString; psi.Environment["Database__Provider"] = "Postgres"; // The Testcontainers DB is plaintext; opt out of the #262 Production TLS floor so - // recover-admin runs (it returns before the #260 serving guard, so only this is needed). + // recover-admin runs. Only this one: the #260/#319 serving guards check + // ProcessRole and skip a one-shot verb (#347), and the TLS floor applies + // to both roles by design. psi.Environment["Database__AllowInsecureConnection"] = "true"; psi.Environment["Jwt__Issuer"] = "cluckwork-test"; psi.Environment["Jwt__Audience"] = "cluckwork-api-test"; diff --git a/tests/Cluckwork.Api.IntegrationTests/SeedCommandTests.cs b/tests/Cluckwork.Api.IntegrationTests/SeedCommandTests.cs index a3f3f942c..80e268d02 100644 --- a/tests/Cluckwork.Api.IntegrationTests/SeedCommandTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/SeedCommandTests.cs @@ -49,7 +49,9 @@ private Process StartSeedCommand(string? profile, string environment = "Testing" psi.Environment["ConnectionStrings__Default"] = connectionString ?? _factory.ConnectionString; psi.Environment["Database__Provider"] = "Postgres"; // The Testcontainers DB is plaintext; opt out of the #262 Production TLS floor so - // the seed verb runs (it returns before the #260 serving guard, so only this is needed). + // the seed verb runs. Only this one: the #260/#319 serving guards check + // ProcessRole and skip a one-shot verb (#347), and the TLS floor applies + // to both roles by design. psi.Environment["Database__AllowInsecureConnection"] = "true"; psi.Environment["Jwt__Issuer"] = "cluckwork-test"; psi.Environment["Jwt__Audience"] = "cluckwork-api-test"; diff --git a/tests/Cluckwork.Api.IntegrationTests/SimulationSeedCommandTests.cs b/tests/Cluckwork.Api.IntegrationTests/SimulationSeedCommandTests.cs index 09366f3f6..f949623de 100644 --- a/tests/Cluckwork.Api.IntegrationTests/SimulationSeedCommandTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/SimulationSeedCommandTests.cs @@ -76,7 +76,9 @@ private Process StartSeedCommand( psi.Environment["ConnectionStrings__Default"] = connectionString ?? _factory.ConnectionString; psi.Environment["Database__Provider"] = "Postgres"; // The Testcontainers DB is plaintext; opt out of the #262 Production TLS floor so the - // simulation seed verb runs (it returns before the #260 serving guard, so only this is needed). + // simulation seed verb runs. Only this one: the #260/#319 serving guards + // check ProcessRole and skip a one-shot verb (#347), and the TLS floor + // applies to both roles by design. psi.Environment["Database__AllowInsecureConnection"] = "true"; psi.Environment["Jwt__Issuer"] = "cluckwork-test"; psi.Environment["Jwt__Audience"] = "cluckwork-api-test"; diff --git a/tools/simulation/verify-harness.sh b/tools/simulation/verify-harness.sh index 15c5dc8ba..c035be8df 100755 --- a/tools/simulation/verify-harness.sh +++ b/tools/simulation/verify-harness.sh @@ -34,7 +34,7 @@ # "AllowedHosts OK (cluckwork-sim.local)" while the app would receive `*` # and fail its boot. reset.sh documents that exact precedence 40 lines up, # for COMPOSE_PROJECT_NAME. Reproduced, not theorised. -# * it checked `AllowedHosts` was non-empty and had no `*` — but Program.cs +# * it checked `AllowedHosts` was non-empty and had no `*` — but the app # splits on ';', trims and drops empties, so `; ;` passed here and failed # the boot. # * it grepped the compose FILE for Database__AllowInsecureConnection, so the @@ -52,7 +52,7 @@ # ================== WHAT THIS IS NOT ================== # # It is NOT a boot simulator, and must not grow into one. The app raises -# InvalidOperationException from ~15 sites across 7 files (Program.cs, +# InvalidOperationException from ~15 sites across 7 files (ServingBootGuards, # RateLimitingOptions, OtlpOptions, PostgresConnectionString, # DatabaseResilienceOptions, the two Hosting extensions). Mirroring all of them # here means reimplementing the app's config validation in Python and keeping @@ -229,7 +229,8 @@ else: ok.append(f"FarmBanner upload limit OK ({banner_limit} bytes)") # --- #319 AllowedHosts --------------------------------------------------- -# Parsed EXACTLY as Program.cs does: split on ';', trim, drop empties, then +# Parsed EXACTLY as ServingBootGuards.EnsureAllowedHostsPinned does: split on +# ';', trim, drop empties, then # require at least one entry and none equal to '*'. A raw non-empty/no-'*' # check passes "; ;" and fails the boot. raw = env.get("AllowedHosts") From 5dcae6e6d23ecdbf4790474b40980d24dd1b9bfe Mon Sep 17 00:00:00 2001 From: mforce <> Date: Mon, 10 Aug 2026 17:53:14 -0700 Subject: [PATCH 2/9] test(api): prove the serving guards hostile, not just one of them Local review found the two-sided guard test weaker than it read. #316 is validated during service registration, ahead of ServingBootGuards, so with all three settings violated the serving arm ALWAYS dies at #316 and never reaches #260 or #319. It proved one guard hostile, not three, and either of the other two could have quietly stopped being violated with every arm green and the migrate arm's coverage of them silently vacuous. Arm 3 sets Otlp:AllowInsecureEndpoint so #316 passes and the boot has to die on #260 or #319. Pinned by a new mutant: with the serving guards enforcing nothing, arms 1 and 2 stay green and only arm 3 goes red. Verified it fails for that reason and not another. Same family, two smaller ones. The subprocess environment is inherited from the test host and #260's condition is the ABSENCE of RateLimiting__*, so an inherited value would disarm it silently; the harness now strips the section. SeedCommandRunner's timeout message asserted the seed suites' regression, which for the serving arms means the opposite of what a timeout there means. Also enforces the healthcheck invariant instead of asserting it in prose. Its early return means no production path observes its role, and the safety direction inverted in this change: the retired IsCliInvocation called it Serving, which failed closed, where OneShot fails open into app.Run. Program now throws if it reaches the serving path with a OneShot role. Unreachable today, red the moment the coupling breaks. ProcessRole types drop to internal, matching every sibling in Hosting. Corrects a pre-existing AGENTS.md claim that Program.cs holds the tzdata canary; it is in AddCluckworkPersistence, and applies to both roles. --- AGENTS.md | 2 +- docs/decisions/347-process-role.md | 28 ++++++--- src/Cluckwork.Api/Hosting/ProcessRole.cs | 4 +- src/Cluckwork.Api/Program.cs | 15 +++++ .../Infrastructure/SeedCommandRunner.cs | 14 ++++- .../ProcessRoleGuardTests.cs | 58 +++++++++++++++++-- .../ProcessRoleRegistryTests.cs | 9 ++- 7 files changed, 108 insertions(+), 22 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 7e12a8d00..758e01597 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -52,7 +52,7 @@ dotnet test Cluckwork.sln # 688 tests as of 2026-07; integratio - **Seed / simulation data is never boot-seeded (#280, #284, #279).** Run it explicitly against an **already-base-seeded, non-Production** database: `ASPNETCORE_ENVIRONMENT=Development dotnet Cluckwork.Api.dll seed --profile demo|simulation` (an unset env → Production → blocked). The verb migrates, seeds, then exits — **authoritative** (ignores config) and **fail-loud** (a real exit code, never a silent no-op). `simulation` additionally records a durable date anchor + completion marker, so a clean re-run — even across a UTC-midnight rollover — converges to `AlreadySeeded`; a polluted account fails the exact-count validation **closed**. → [`docs/decisions/280-seed-and-simulation.md`](docs/decisions/280-seed-and-simulation.md) - **Break-glass recovery (#265):** `recover-admin` is a second one-off CLI verb on the same binary, same run-then-exit shape as `seed` — but deliberately **NOT** environment-gated (it must work against a real Production database): `dotnet Cluckwork.Api.dll recover-admin --email [--account ] [--reason ]`. For a locked-out account (the sole-Owner-lost-password case, no email/SMTP reset path) it, in one transaction, resets to a **freshly generated** temporary password (never one passed on the command line), rotates the security stamp, revokes every refresh token, and writes a conspicuous `User.BreakGlassReset` audit row carrying `--reason`. The temp password is printed to **stdout only** (never the logger/OTLP) and exit `0`; failures go to stderr with exit `1` and change nothing. `AdminRecoveryService` orchestrates; `IdentityProvider.BreakGlassResetAsync` shares the reset/revoke core with `SetUserPasswordAsync`. Full procedure + verification drill: `docs/runbooks/break-glass-account-recovery.md`. - **First-run admin provisioning: `bootstrap-admin` (#283).** `dotnet Cluckwork.Api.dll bootstrap-admin --email ` (run-then-exit, always available in Production) migrates, then — **only if the default account has no Owner** — creates one with a **freshly generated password (stdout only, never the logger/OTLP)** and `MustChangePassword=true`; a re-run is a **silent no-op**. While the flag is set the JWT carries `must_change_password`, `MustChangePasswordMiddleware` 403s every endpoint except `auth/change-password`+`auth/logout` (before `UseAuthorization` and before idempotency), and the SPA renders **Set your password**. Any successful reset clears the flag (one invariant in `IdentityProvider`). **Deliberately a separate credential type from #265/#308** — never conflated. → [`docs/decisions/283-first-run-admin-provisioning.md`](docs/decisions/283-first-run-admin-provisioning.md) -- **Farm timezone + tzdata/ICU image constraint (#264):** the default account (a fixed `"UTC"` migration literal since #283) is provisioned once; a real farm sets its actual IANA zone via **Settings → timezone** after first login (`Account.UpdateSettings`), not at provisioning time (the `Seed:TimeZoneId` config lever from before #283 is retired along with the runtime seeder it fed). The farm clock resolves IANA zones via `TimeZoneInfo.FindSystemTimeZoneById` for every safety-sensitive local-day decision and **fails closed**, so the runtime **image MUST carry tzdata + ICU** (the Ubuntu 24.04 (Noble) `aspnet:10.0` base does; **never** an Alpine/chiseled base without tzdata, and **never** `InvariantGlobalization=true`). `Program.cs` still asserts a representative IANA canary resolves at boot (`TimeZoneAvailability.EnsureResolvable`) — a bad image fails the boot loudly instead of surfacing later as a per-request `FarmTimeZoneException`. +- **Farm timezone + tzdata/ICU image constraint (#264):** the default account (a fixed `"UTC"` migration literal since #283) is provisioned once; a real farm sets its actual IANA zone via **Settings → timezone** after first login (`Account.UpdateSettings`), not at provisioning time (the `Seed:TimeZoneId` config lever from before #283 is retired along with the runtime seeder it fed). The farm clock resolves IANA zones via `TimeZoneInfo.FindSystemTimeZoneById` for every safety-sensitive local-day decision and **fails closed**, so the runtime **image MUST carry tzdata + ICU** (the Ubuntu 24.04 (Noble) `aspnet:10.0` base does; **never** an Alpine/chiseled base without tzdata, and **never** `InvariantGlobalization=true`). `AddCluckworkPersistence` still asserts a representative IANA canary resolves at boot (`TimeZoneAvailability.EnsureResolvable`), for **both** process roles — a bad image fails the boot loudly instead of surfacing later as a per-request `FarmTimeZoneException`. - **Migrate command + prod migration split (#263).** `dotnet Cluckwork.Api.dll migrate` applies EF migrations then exits — the **pre-deploy-job entrypoint**. In Production `Database:MigrateOnStartup=false`, so the serving process never runs DDL; the **guarantee is ordering** — compose runs a one-shot `migrate` service before `app` waits on `service_completed_successfully`. `DatabaseReadyHealthCheck` is the backstop: `/health/ready` **503s while any migration is pending**. The remaining host step (deploy repo) is a least-privilege runtime role with no DDL, plus `ALTER DEFAULT PRIVILEGES` for the migrator. → [`docs/decisions/263-migrate-command.md`](docs/decisions/263-migrate-command.md) - **Proxy-trust boot guard (#260):** behind a reverse proxy/edge, HSTS (#144) and the per-IP login limiter (#143) only work if the app trusts the proxy's `X-Forwarded-*`, which it does **only** for networks in `RateLimiting:TrustedProxies`. An **empty list in Production fails the boot** (a `ProcessRole.Serving`-only guard, so `migrate`/`seed`/`recover-admin` are unaffected — see the process-role bullet below) rather than silently running with inert HSTS + a one-bucket limiter. Opt out with `RateLimiting:AllowNoTrustedProxies=true` only for a rare direct-TLS deploy with no fronting proxy. The concrete edge CIDR is deploy config (in the separate deployment/ops repo). - **Process role, not statement order (#347).** A boot guard's scope is declared, never positional. `ProcessRoles.From(args)` (`Hosting/ProcessRole.cs`) computes `ProcessRole.Serving | OneShot` **once**, before the host is built, and every role-scoped guard takes it: `ServingBootGuards.EnsureServingConfiguration` (#260, #319) is called **before** the CLI dispatch on purpose and still leaves the verbs alone, and `AddCluckworkTelemetry` (#316) degrades to export-disabled for `OneShot`. The one-shot verb list is derived from `CliDispatcher.Commands` so a new verb classifies itself; **`healthcheck` is the one that must be named explicitly** (it isn't an `ICliCommand` — it needs no host) and omitting it silently classifies the container's own probe as `Serving`. **Never gate a guard on where its statement sits**: that is what #331 was — the #316 validation ran at service registration, ahead of the dispatcher, and killed `recover-admin` (the break-glass verb) with SIGABRT 134. The #261/#262 TLS floor and the #264 tzdata canary take no role: they apply to **both**, and unconditional code says so better than a parameter nobody branches on. → [`docs/decisions/347-process-role.md`](docs/decisions/347-process-role.md) diff --git a/docs/decisions/347-process-role.md b/docs/decisions/347-process-role.md index 2e9af32a3..6764e94b7 100644 --- a/docs/decisions/347-process-role.md +++ b/docs/decisions/347-process-role.md @@ -2,7 +2,7 @@ > **Rule** — the one-paragraph version lives in [`AGENTS.md`](../../AGENTS.md); this file is the relocated rationale (what shipped, why the short version was insufficient, what not to break). -## What was wrong +### What was wrong The API binary serves HTTP **and** carries five run-then-exit operator verbs — `migrate`, `seed`, `recover-admin`, `bootstrap-admin`, `healthcheck`. Several Production boot guards apply to only one of those roles. Nothing said so. What made a guard serving-only was the **position of its statement** in `Program.cs`: below `CliDispatcher.TryRunAsync`'s return, so a one-shot verb had already exited before reaching it. @@ -11,7 +11,7 @@ That is not a property anyone can read off the guard, and getting it wrong is no - **#331 — the one with teeth.** The #316 OTLP endpoint validation ran at *service registration*, ahead of the dispatcher. A plaintext `Otlp:Endpoint` therefore killed `recover-admin` with SIGABRT 134 — the break-glass verb, the one that exists for when everything else is broken, taken out by a guard that protects a serving process's telemetry. The fix at the time was a `!CliDispatcher.IsCliInvocation(args)` bool threaded into registration: correct, but it made the *second* mechanism for the same idea. - **A latent third.** `IsCliInvocation` was derived from `CliDispatcher.Commands`, which holds only the four verbs that dispatch after `Build()`. `healthcheck` is not among them — it is not an `ICliCommand` (it needs no host, so it takes no `WebApplication`) and `Program.cs` dispatches it before the builder exists. So the predicate classified the container's own health probe as a **serving** process. Harmless only because of that early return, i.e. harmless because of statement position again. -## What shipped +### What shipped `ProcessRoles.From(args)` (`src/Cluckwork.Api/Hosting/ProcessRole.cs`) computes `ProcessRole.Serving | OneShot` **once**, before `WebApplication.CreateBuilder`, and every role-scoped guard takes it as an argument: @@ -31,7 +31,7 @@ Three details are load-bearing. **The both-roles guards take no `ProcessRole` parameter.** An argument nobody branches on implies a branch that does not exist, and the next reader has to check whether it is ignored deliberately or by accident. Unconditional code states "applies to both" more strongly than a parameter can; this table is where that classification is recorded. -## What was rejected +### What was rejected **A separate `Cluckwork.Cli` assembly**, which #347 originally asked for alongside this. Its stated payoff was "verbs gain unit-level coverage that does not need a web host" — but `ICliCommand.RunAsync` takes a `WebApplication`, so a class library holding the same files still needs a fully built web app to run anything. The move delivers the churn and none of the benefit. Getting the benefit means changing the verb signature, which is a different and larger change, and one [#423](https://github.com/mforce/cluckwork/pull/423)'s Phase 10 ("convert CLI verbs to contracts") rewrites anyway. @@ -39,11 +39,19 @@ Three details are load-bearing. **Converting #316's degrade to a pre-check.** `if (role is Serving) validate…` reads more uniformly, but the current `catch … when (role is OneShot)` writes `warning: OTLP export disabled for this command — ` to stderr. That line is the only thing that tells an operator why a one-shot run is exporting nothing. Skipping validation entirely would silently disable export with no reason attached. -## How it is pinned +### How it is pinned -`ProcessRoleGuardTests` (integration, subprocess) is the guarantee, and it is deliberately **two-sided** — asserting that `migrate` exits 0 under a hostile serving configuration proves nothing unless that configuration genuinely would fail a serving boot. Arm 1 runs the real binary with `ASPNETCORE_ENVIRONMENT=Production`, `TrustedProxies` empty, `AllowedHosts=*` and a plaintext `Otlp:Endpoint`, and requires exit 0. Arm 2 starts the same binary with the same environment and **no verb**, and requires it to die with a message naming one of the three guards. Which guard wins is not asserted: #316 throws during registration and the other two after `Build()`, and pinning that order would turn a legitimate reordering into a red test. +`ProcessRoleGuardTests` (integration, subprocess) is the guarantee, and it is deliberately **three-armed** — asserting that `migrate` exits 0 under a hostile serving configuration proves nothing unless that configuration genuinely would fail a serving boot. -`ProcessRoleRegistryTests` (fast, no host) covers the classification itself, including the one case the subprocess suite structurally cannot reach — `healthcheck` returns before a host exists, so no boot guard can observe its role today. +- **Arm 1** runs the real binary with `ASPNETCORE_ENVIRONMENT=Production`, `TrustedProxies` empty, `AllowedHosts=*` and a plaintext `Otlp:Endpoint`, and requires exit 0. +- **Arm 2** starts the same binary with the same environment and **no verb**, and requires it to die naming one of the three guards. Which guard wins is not asserted: #316 throws during registration and the other two after `Build()`, and pinning that order would turn a legitimate reordering into a red test. +- **Arm 3** exists because arm 2 alone is weaker than it reads, and this was found in review rather than by writing it. #316 is validated during **service registration**, ahead of `ServingBootGuards`, so with all three settings violated the serving process **always** dies at #316 — arm 2 never reaches #260 or #319 and therefore proves only that *one* of the three is hostile. #260 or #319 could quietly stop being violated (an appsettings default gaining a concrete `AllowedHosts`, `AllowNoTrustedProxies` flipping true, a CI runner exporting `RateLimiting__*`) with both arms green and arm 1's coverage of them silently vacuous. Arm 3 sets `Otlp:AllowInsecureEndpoint=true` so #316 passes, and requires the boot to die on one of the other two. + +Two smaller things the same review surfaced, both of the same family — a fixture that passes for a reason other than the one claimed. The subprocess environment is inherited from the test host, and **#260's condition is the ABSENCE of `RateLimiting__*`**, so the harness now strips that whole section rather than trusting it to be unset. And `SeedCommandRunner`'s timeout message asserted the seed suites' regression ("falling through into `app.Run()`"), which for arms 2 and 3 means the exact opposite — a timeout there means the serving boot *succeeded* — so the message is now per-caller. + +`ProcessRoleRegistryTests` (fast, no host) covers the classification itself, including the one case the subprocess suite structurally cannot reach — `healthcheck` returns before a host exists, so no boot guard can observe its role today. Note what it does **not** do: `OneShotVerbs` is derived from `CliDispatcher.Commands`, so walking that list cannot go red when a verb is added or removed. That property comes from the production derivation; the test catches `From` itself regressing. + +**The `healthcheck` classification is enforced, not merely asserted.** Its early return means no production path can observe its role — and the safety direction *inverted* in this change: the retired `IsCliInvocation` called it `Serving` (fail closed — a misconfigured Production deploy aborted at a guard), where `OneShot` fails open (guards skipped, `TryRunAsync` finds no match, execution falls through into `app.Run()` and a health probe tries to become a server). An invariant that only a comment defends is not defended, so `Program.cs` throws if it ever reaches the serving path with a `OneShot` role — unreachable today, red the moment the coupling breaks. Mutation evidence, run before the claim (baseline green, restore green): @@ -52,5 +60,11 @@ Mutation evidence, run before the claim (baseline green, restore green): | `EnsureServingConfiguration` drops its `role` check | **red** (`migrate` aborts on #260) | | #316's degrade filter flipped to `Serving` | **red** — this is #331 verbatim | | `healthcheck` removed from `OneShotVerbs` | **red** (registry suite) | +| the serving guards enforce nothing at all | **red — via arm 3 only**; arms 1 and 2 stay green | +| `healthcheck`'s early return removed from `Program.cs` | **red** (the assertion fires, exit 134) | + +Two process notes, because each nearly produced false evidence. + +The first mutation script restored with `git checkout --`, which **silently refuses to restore untracked files**. Two of the three targets were new files, so mutants stacked instead of reverting and two "red" results were measured against an already-mutated tree. Snapshot by file copy, and assert the restored tree is green again. -One process note, because it nearly produced false evidence: the first mutation script restored with `git checkout --`, which **silently refuses to restore untracked files**. Two of the three targets were new files, so mutants stacked instead of reverting and two "red" results were measured against an already-mutated tree. Snapshot by file copy, and assert the restored tree is green again. +And a mutant being red is not the same as being red *for the stated reason*. The fourth was re-run on its own to confirm it fails on arm 3's timeout — the serving process boots clean, logging `OTLP export enabled` — rather than on arm 1 or 2, which is the entire claim behind adding arm 3. diff --git a/src/Cluckwork.Api/Hosting/ProcessRole.cs b/src/Cluckwork.Api/Hosting/ProcessRole.cs index d604fa7b6..7929560fb 100644 --- a/src/Cluckwork.Api/Hosting/ProcessRole.cs +++ b/src/Cluckwork.Api/Hosting/ProcessRole.cs @@ -16,7 +16,7 @@ namespace Cluckwork.Api.Hosting; // broken — with SIGABRT 134. // // Now each guard checks the role itself, so its scope survives being moved. -public enum ProcessRole +internal enum ProcessRole { // Started to serve HTTP traffic: Kestrel binds, hosted services run. Serving, @@ -28,7 +28,7 @@ public enum ProcessRole OneShot, } -public static class ProcessRoles +internal static class ProcessRoles { // Every verb whose presence at args[0] makes this a one-shot process. // diff --git a/src/Cluckwork.Api/Program.cs b/src/Cluckwork.Api/Program.cs index ae0c6dfa5..8e685a362 100644 --- a/src/Cluckwork.Api/Program.cs +++ b/src/Cluckwork.Api/Program.cs @@ -114,6 +114,21 @@ if (await CliDispatcher.TryRunAsync(app, args) is int cliExitCode) return cliExitCode; +// #347 — reaching here with a OneShot role is impossible by construction, so +// this line exists to make that a checked fact rather than a comment. The only +// verb in OneShotVerbs that CliDispatcher cannot handle is `healthcheck`, and +// it returned at the top of this file long before now; if it ever stops doing +// so, execution would otherwise fall through into app.Run() and a health probe +// would try to become a server. Classifying healthcheck as OneShot is what makes +// that direction fail OPEN (guards skipped) where the retired IsCliInvocation +// failed closed, so the invariant that kept it safe now needs enforcing. +if (processRole is ProcessRole.OneShot) + throw new InvalidOperationException( + $"'{args[0]}' is a one-shot verb (ProcessRoles.OneShotVerbs) but nothing dispatched it, so " + + "the serving host was about to start instead. A verb was added to the role registry " + + "without a matching CliDispatcher command, or healthcheck's early return above was " + + "moved or made conditional."); + // One boot line makes export misconfiguration observable — a typo'd env var // name otherwise silently disables the whole pipeline (#226 review). if (telemetry.TraceEndpoint is not null) diff --git a/tests/Cluckwork.Api.IntegrationTests/Infrastructure/SeedCommandRunner.cs b/tests/Cluckwork.Api.IntegrationTests/Infrastructure/SeedCommandRunner.cs index efe6bec93..7efb053a8 100644 --- a/tests/Cluckwork.Api.IntegrationTests/Infrastructure/SeedCommandRunner.cs +++ b/tests/Cluckwork.Api.IntegrationTests/Infrastructure/SeedCommandRunner.cs @@ -21,8 +21,14 @@ namespace Cluckwork.Api.IntegrationTests.Infrastructure; // process) so the test fails fast with a clear message instead of hanging. public static class SeedCommandRunner { + // timeoutMeaning: what a timeout tells the CALLER. The default states the + // regression the seed/migrate suites are looking for — a verb falling through + // into app.Run() instead of exiting. ProcessRoleGuardTests' serving arms + // deliberately start a process with NO verb and expect it to die on a boot + // guard, so for them a timeout means the exact opposite (the boot succeeded), + // and the default message would send the reader hunting the wrong bug. public static async Task<(int ExitCode, string Stdout, string Stderr)> RunToCompletionAsync( - Process proc, TimeSpan timeout) + Process proc, TimeSpan timeout, string? timeoutMeaning = null) { using (proc) { @@ -42,8 +48,10 @@ public static class SeedCommandRunner var partialStderr = await stderrTask; Assert.Fail( $"`{proc.StartInfo.FileName} {proc.StartInfo.Arguments}` did not exit within {timeout}. " + - "This is the exact regression under test: falling through into app.Run() instead of " + - $"returning would hang here. Killed the process tree. stdout={partialStdout} stderr={partialStderr}"); + (timeoutMeaning + ?? "This is the exact regression under test: falling through into app.Run() instead of " + + "returning would hang here.") + + $" Killed the process tree. stdout={partialStdout} stderr={partialStderr}"); } var stdout = await stdoutTask; diff --git a/tests/Cluckwork.Api.IntegrationTests/ProcessRoleGuardTests.cs b/tests/Cluckwork.Api.IntegrationTests/ProcessRoleGuardTests.cs index 2a8138d74..48a38097c 100644 --- a/tests/Cluckwork.Api.IntegrationTests/ProcessRoleGuardTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/ProcessRoleGuardTests.cs @@ -45,15 +45,30 @@ public sealed class ProcessRoleGuardTests // #316 Otlp:Endpoint plaintext http in -> telemetry and Otlp:Headers // Production, no AllowInsecureEndpoint credentials exposed in transit. // None of the three has any bearing on what a one-shot verb does. - private static void ApplyHostileServingConfiguration(ProcessStartInfo psi) + // + // satisfyOtlp acknowledges the plaintext endpoint, so #316 passes and a + // serving boot has to fail on #260/#319 instead. See arm 3 for why that + // matters: #316 is validated during service registration, ahead of the other + // two, so with all three violated the serving process ALWAYS dies at #316 and + // the other two are never reached. + private static void ApplyHostileServingConfiguration(ProcessStartInfo psi, bool satisfyOtlp) { - // Left EMPTY on purpose (no RateLimiting__TrustedProxies__0 entry) — that - // absence IS the #260 condition. + // #260's condition is the ABSENCE of these keys, so an inherited value + // from the test host or a CI runner would silently disarm it — and the + // suite would stay green while proving less than it claims. Strip the + // whole section rather than the two names known today. + foreach (var inherited in psi.Environment.Keys + .Where(k => k.StartsWith("RateLimiting__", StringComparison.Ordinal)) + .ToList()) + psi.Environment.Remove(inherited); + psi.Environment["AllowedHosts"] = "*"; psi.Environment["Otlp__Endpoint"] = "http://collector.invalid:4317"; + if (satisfyOtlp) + psi.Environment["Otlp__AllowInsecureEndpoint"] = "true"; } - private static Process Start(string verb, string connectionString) + private static Process Start(string verb, string connectionString, bool satisfyOtlp = false) { var arguments = verb.Length == 0 ? $"\"{ApiDllPath}\"" : $"\"{ApiDllPath}\" {verb}"; var psi = new ProcessStartInfo("dotnet", arguments) @@ -77,7 +92,7 @@ private static Process Start(string verb, string connectionString) // Port 0 = an ephemeral port, so the serving arm cannot collide with // anything else on the machine in the (failing) case where it boots. psi.Environment["ASPNETCORE_URLS"] = "http://127.0.0.1:0"; - ApplyHostileServingConfiguration(psi); + ApplyHostileServingConfiguration(psi, satisfyOtlp); return Process.Start(psi)!; } @@ -106,7 +121,10 @@ public async Task ServingOnlyGuards_DoNotAbortAOneShotVerb_AndStillFailAServingB // registration and the other two after Build(), and pinning that order here // would turn a legitimate future reordering into a red test for no reason. var (servingExit, servingOut, servingErr) = await SeedCommandRunner.RunToCompletionAsync( - Start(string.Empty, connectionString), ServingBootTimeout); + Start(string.Empty, connectionString), + ServingBootTimeout, + "a serving boot that SUCCEEDS here means this configuration is not hostile after all, " + + "so arm 1 above proved nothing"); Assert.True( servingExit != 0, @@ -123,5 +141,33 @@ public async Task ServingOnlyGuards_DoNotAbortAOneShotVerb_AndStillFailAServingB "the serving boot failed, but not on any of the three guards this test configures — " + "so arm 1 is not exercising what it claims. " + $"exit={servingExit} stdout={servingOut} stderr={servingErr}"); + + // ARM 3 — the half arm 2 structurally cannot cover. #316 is validated + // during SERVICE REGISTRATION, ahead of ServingBootGuards, so with all + // three settings violated a serving boot always dies at #316: arm 2 alone + // proves only that ONE of the three is hostile, and #260/#319 could + // quietly stop being violated (an appsettings default gaining a concrete + // AllowedHosts, AllowNoTrustedProxies flipping true, a CI runner exporting + // RateLimiting__*) with both arms still green and arm 1's coverage of them + // silently vacuous. Acknowledge the plaintext endpoint so #316 passes, and + // require the boot to die on one of the other two. + var (pinnedExit, pinnedOut, pinnedErr) = await SeedCommandRunner.RunToCompletionAsync( + Start(string.Empty, connectionString, satisfyOtlp: true), + ServingBootTimeout, + "a serving boot that SUCCEEDS here means #260 and #319 are no longer violated by this " + + "configuration, so arm 1 no longer covers them"); + + Assert.True( + pinnedExit != 0, + "with #316 satisfied, the serving boot survived — #260 and #319 are not actually " + + $"violated by this configuration. stdout={pinnedOut} stderr={pinnedErr}"); + + // Either may win; both are ServingBootGuards, and pinning which would make + // a legitimate reordering red for no reason. + Assert.True( + pinnedErr.Contains("RateLimiting:TrustedProxies", StringComparison.Ordinal) + || pinnedErr.Contains("AllowedHosts", StringComparison.Ordinal), + "with #316 satisfied the serving boot failed, but on neither #260 nor #319 — so " + + $"arm 1's coverage of those two is not established. exit={pinnedExit} stderr={pinnedErr}"); } } diff --git a/tests/Cluckwork.Api.IntegrationTests/ProcessRoleRegistryTests.cs b/tests/Cluckwork.Api.IntegrationTests/ProcessRoleRegistryTests.cs index 74200eaf9..7828aaa82 100644 --- a/tests/Cluckwork.Api.IntegrationTests/ProcessRoleRegistryTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/ProcessRoleRegistryTests.cs @@ -18,8 +18,12 @@ public sealed class ProcessRoleRegistryTests [Fact] public void EveryDispatchedVerb_IsAOneShotProcess() { - // Walk the dispatcher's own registry rather than a list written here: a - // sixth verb must be classified without anyone remembering this test. + // Honest about its reach: OneShotVerbs is DERIVED from CliDispatcher.Commands, + // so this cannot go red when a verb is added to or removed from the + // dispatcher — that property is delivered by the production derivation, not + // by this assertion. What it does catch is `From` itself regressing: reading + // the wrong argument index, comparing against the wrong list, or matching on + // something other than an exact name. foreach (var command in CliDispatcher.Commands) Assert.Equal(ProcessRole.OneShot, ProcessRoles.From([command.Name])); } @@ -31,7 +35,6 @@ public void EveryDispatchedVerb_IsAOneShotProcess() public void Healthcheck_IsAOneShotProcess() { Assert.Equal(ProcessRole.OneShot, ProcessRoles.From([HealthCheckCliCommand.Verb])); - Assert.Contains(HealthCheckCliCommand.Verb, ProcessRoles.OneShotVerbs); } [Fact] From 73dcbf9a405cf913ee36722e32de2adc39a3c750 Mon Sep 17 00:00:00 2001 From: mforce <> Date: Mon, 10 Aug 2026 18:32:10 -0700 Subject: [PATCH 3/9] test(api): derive one guard arm per guard instead of listing arms Round two of local review found the third arm carrying the same defect as the second, one level down. Both asserted the boot died naming ONE OF a set of guards, and both times the guards fire in a fixed order, so the later disjuncts were dead. v1: OTLP validates at service registration and always won, so the trusted-proxy and AllowedHosts guards were never proven hostile. v2: added an arm satisfying OTLP and then let the trusted-proxy guard always win ahead of AllowedHosts. A mutant deleting the AllowedHosts call outright survived every arm. Two misses of the same shape mean the method is wrong, not that the list needs another entry. So there is no list of arms now. Every serving-only guard is a row in a table and the suite derives one arm per row: violate exactly that guard, satisfy every other, require the boot to die naming that guard and not naming any other. No arm has two violations to choose between, so ordering cannot hide anything, and the negative half turns a different guard catching the boot from silently green into red. The same method had leaked three ways into the child environment. The strip was Ordinal while Linux env keys are case-sensitive and config keys are not, ASPNETCORE_ and DOTNET_ prefixed variables bind to the same config keys with the prefix removed, and Otlp__ was never stripped at all, which would have made the OTLP arm vacuous for the exact regression this suite exists for. The child environment is built from scratch now: cleared, a small OS allow-list, then every application setting stated explicitly. Both mutants that survived v2 are now red, and both are in the recorded set so they stay that way. Also fixes a run-on timeout diagnostic and a class comment still describing the two-armed version. --- docs/decisions/347-process-role.md | 24 +- .../Infrastructure/SeedCommandRunner.cs | 15 +- .../ProcessRoleGuardTests.cs | 268 ++++++++++-------- 3 files changed, 181 insertions(+), 126 deletions(-) diff --git a/docs/decisions/347-process-role.md b/docs/decisions/347-process-role.md index 6764e94b7..3f5e77504 100644 --- a/docs/decisions/347-process-role.md +++ b/docs/decisions/347-process-role.md @@ -41,27 +41,37 @@ Three details are load-bearing. ### How it is pinned -`ProcessRoleGuardTests` (integration, subprocess) is the guarantee, and it is deliberately **three-armed** — asserting that `migrate` exits 0 under a hostile serving configuration proves nothing unless that configuration genuinely would fail a serving boot. +`ProcessRoleGuardTests` (integration, subprocess) is the guarantee. Asserting that `migrate` exits 0 under a hostile serving configuration proves nothing unless that configuration genuinely would fail a serving boot — so the suite also has to establish the hostility, and **how it establishes it is the part that took three attempts.** -- **Arm 1** runs the real binary with `ASPNETCORE_ENVIRONMENT=Production`, `TrustedProxies` empty, `AllowedHosts=*` and a plaintext `Otlp:Endpoint`, and requires exit 0. -- **Arm 2** starts the same binary with the same environment and **no verb**, and requires it to die naming one of the three guards. Which guard wins is not asserted: #316 throws during registration and the other two after `Build()`, and pinning that order would turn a legitimate reordering into a red test. -- **Arm 3** exists because arm 2 alone is weaker than it reads, and this was found in review rather than by writing it. #316 is validated during **service registration**, ahead of `ServingBootGuards`, so with all three settings violated the serving process **always** dies at #316 — arm 2 never reaches #260 or #319 and therefore proves only that *one* of the three is hostile. #260 or #319 could quietly stop being violated (an appsettings default gaining a concrete `AllowedHosts`, `AllowNoTrustedProxies` flipping true, a CI runner exporting `RateLimiting__*`) with both arms green and arm 1's coverage of them silently vacuous. Arm 3 sets `Otlp:AllowInsecureEndpoint=true` so #316 passes, and requires the boot to die on one of the other two. +The first two versions both asserted *"the boot died naming ONE OF these guards"*, and in both the guards run in a fixed order, so the later disjuncts were dead: -Two smaller things the same review surfaced, both of the same family — a fixture that passes for a reason other than the one claimed. The subprocess environment is inherited from the test host, and **#260's condition is the ABSENCE of `RateLimiting__*`**, so the harness now strips that whole section rather than trusting it to be unset. And `SeedCommandRunner`'s timeout message asserted the seed suites' regression ("falling through into `app.Run()`"), which for arms 2 and 3 means the exact opposite — a timeout there means the serving boot *succeeded* — so the message is now per-caller. +- **v1** — three guards, one disjunction. #316 is validated during **service registration**, ahead of `ServingBootGuards`, so the boot always died there. #260 and #319 were never proven hostile at all. +- **v2** — added an arm that satisfied #316, and repeated the mistake one level down. `EnsureServingConfiguration` calls #260 then #319 unconditionally, so the boot always died at #260 and the `AllowedHosts` disjunct was dead. **A mutant deleting the #319 call outright survived every arm.** + +Two misses of the same shape mean the method is wrong, not that the list needs another entry ([guard-writing rules](407-writing-a-guard.md)). So v3 has no list of arms. Every serving-only guard is a **row in a table**, and the suite derives one arm per row: violate exactly that guard, **satisfy every other**, and require the boot to die naming that guard **and not naming any other**. No arm ever has two violations to choose between, so guard ordering cannot hide anything, and the negative half turns "a different guard caught it" from silently green into red. Adding a fourth guard is adding a row. + +The same "list what I thought of" method had also sprung three leaks in how the child process's environment was built. The subprocess inherits the test host's environment and **#260's condition is the ABSENCE of `RateLimiting__*`**, so v2 stripped that prefix — but the strip was `Ordinal` while Linux environment keys are case-sensitive and .NET configuration keys are not (`ratelimiting__…` survived it), `ASPNETCORE_`- and `DOTNET_`-prefixed variables bind to the same configuration keys with the prefix removed, and `Otlp__` was never stripped at all — so an inherited `Otlp__AllowInsecureEndpoint` (a documented sim-harness setting) would have silently voided the #316 arm. The child environment is now **built from scratch**: cleared, then a small allow-list of OS variables, then every application setting stated explicitly. + +One smaller thing from the same review, same family: `SeedCommandRunner`'s timeout message asserted the seed suites' regression ("falling through into `app.Run()`"), which for the serving arms means the exact opposite — a timeout there means the boot *succeeded* — so the message is now per-caller. `ProcessRoleRegistryTests` (fast, no host) covers the classification itself, including the one case the subprocess suite structurally cannot reach — `healthcheck` returns before a host exists, so no boot guard can observe its role today. Note what it does **not** do: `OneShotVerbs` is derived from `CliDispatcher.Commands`, so walking that list cannot go red when a verb is added or removed. That property comes from the production derivation; the test catches `From` itself regressing. **The `healthcheck` classification is enforced, not merely asserted.** Its early return means no production path can observe its role — and the safety direction *inverted* in this change: the retired `IsCliInvocation` called it `Serving` (fail closed — a misconfigured Production deploy aborted at a guard), where `OneShot` fails open (guards skipped, `TryRunAsync` finds no match, execution falls through into `app.Run()` and a health probe tries to become a server). An invariant that only a comment defends is not defended, so `Program.cs` throws if it ever reaches the serving path with a `OneShot` role — unreachable today, red the moment the coupling breaks. -Mutation evidence, run before the claim (baseline green, restore green): +Mutation evidence, run before the claim: | Mutation | Result | |---|---| +| baseline, unmutated | **green** | | `EnsureServingConfiguration` drops its `role` check | **red** (`migrate` aborts on #260) | | #316's degrade filter flipped to `Serving` | **red** — this is #331 verbatim | | `healthcheck` removed from `OneShotVerbs` | **red** (registry suite) | -| the serving guards enforce nothing at all | **red — via arm 3 only**; arms 1 and 2 stay green | +| the #319 `AllowedHosts` guard deleted outright | **red** — *survived v2 entirely* | +| the #260 `TrustedProxies` guard neutered | **red** — *survived v2 entirely* | | `healthcheck`'s early return removed from `Program.cs` | **red** (the assertion fires, exit 134) | +| restored | **green** | + +The two marked rows are the ones that matter: under v2 both mutants left every arm green, which is how the dead disjuncts were found. They are the reason the per-guard table exists, and they are the regression check on it. Two process notes, because each nearly produced false evidence. diff --git a/tests/Cluckwork.Api.IntegrationTests/Infrastructure/SeedCommandRunner.cs b/tests/Cluckwork.Api.IntegrationTests/Infrastructure/SeedCommandRunner.cs index 7efb053a8..67437db0b 100644 --- a/tests/Cluckwork.Api.IntegrationTests/Infrastructure/SeedCommandRunner.cs +++ b/tests/Cluckwork.Api.IntegrationTests/Infrastructure/SeedCommandRunner.cs @@ -21,12 +21,15 @@ namespace Cluckwork.Api.IntegrationTests.Infrastructure; // process) so the test fails fast with a clear message instead of hanging. public static class SeedCommandRunner { - // timeoutMeaning: what a timeout tells the CALLER. The default states the - // regression the seed/migrate suites are looking for — a verb falling through - // into app.Run() instead of exiting. ProcessRoleGuardTests' serving arms - // deliberately start a process with NO verb and expect it to die on a boot - // guard, so for them a timeout means the exact opposite (the boot succeeded), - // and the default message would send the reader hunting the wrong bug. + // timeoutMeaning: what a timeout tells the CALLER, as one or more COMPLETE + // SENTENCES — it is spliced between two others, so it must start capitalized + // and end with a period or the diagnostic reads as a run-on. The default + // states the regression the seed/migrate suites are looking for — a verb + // falling through into app.Run() instead of exiting. ProcessRoleGuardTests' + // serving arms deliberately start a process with NO verb and expect it to die + // on a boot guard, so for them a timeout means the exact opposite (the boot + // succeeded), and the default message would send the reader hunting the wrong + // bug. public static async Task<(int ExitCode, string Stdout, string Stderr)> RunToCompletionAsync( Process proc, TimeSpan timeout, string? timeoutMeaning = null) { diff --git a/tests/Cluckwork.Api.IntegrationTests/ProcessRoleGuardTests.cs b/tests/Cluckwork.Api.IntegrationTests/ProcessRoleGuardTests.cs index 48a38097c..9b6a454bd 100644 --- a/tests/Cluckwork.Api.IntegrationTests/ProcessRoleGuardTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/ProcessRoleGuardTests.cs @@ -10,65 +10,97 @@ namespace Cluckwork.Api.IntegrationTests; // ran at service registration, before CliDispatcher, so a plaintext endpoint // killed `recover-admin` with SIGABRT 134 — the break-glass verb, the one that // has to work when everything else is broken, taken out by a guard that exists -// for the serving process. The fix at the time was a `!IsCliInvocation(args)` -// bool threaded into registration; the fix here is a ProcessRole computed once -// and checked by each guard, so the answer to "does this guard fire for -// migrate?" is a property of the guard rather than of where its statement sits -// in Program.cs. +// for the serving process. // -// The test is deliberately TWO-SIDED, because the one-sided version is worthless: -// asserting `migrate` exits 0 under a hostile serving configuration proves -// nothing unless that configuration genuinely WOULD fail a serving boot. So the -// second arm starts the same binary with the same environment and NO verb, and -// requires it to die naming a guard. Together: the config is serving-hostile, -// and the one-shot verb survives it. +// ── Why the structure below is a TABLE and not a list of arms ────────────── // -// Each of the three settings below is independently load-bearing in the first -// arm — misclassify any single guard as applying to OneShot and `migrate` -// aborts, so this goes red without needing one subprocess per guard. +// Two earlier versions of this test had the same defect, which is what decided +// the shape. Both asserted "the boot died naming ONE OF these guards", and in +// both cases the guards run in a fixed order, so the later ones were never +// reached and their disjunct was dead: +// +// v1: three guards, one disjunction. #316 is validated during SERVICE +// REGISTRATION, ahead of ServingBootGuards, so the boot ALWAYS died there +// and #260/#319 were never proven hostile at all. +// v2: added an arm satisfying #316 — and repeated the mistake one level down. +// EnsureServingConfiguration calls #260 then #319 unconditionally, so the +// boot always died at #260 and the `AllowedHosts` disjunct was dead. A +// mutant deleting the #319 call entirely survived every arm. +// +// AGENTS.md: two misses of the same shape mean the METHOD is wrong — prefer +// "walk everything, exclude deliberately" over "list what I thought of". So +// there is no list of arms here. Every serving-only guard is a row, and the +// suite derives one arm per row: violate exactly THAT guard, satisfy every +// other, and require the boot to die naming that guard AND NOT naming any +// other. Ordering between guards then cannot hide anything, because no arm ever +// has two violations to choose between. Adding a fourth guard is a row. public sealed class ProcessRoleGuardTests { private static readonly string ApiDllPath = typeof(Program).Assembly.Location; - // The migrate arm applies a full schema to a fresh database; the serving arm - // is expected to die during boot, long before it would bind Kestrel, so it - // gets a much shorter bound — a serving start that DOESN'T die is the failure - // this arm is looking for, and waiting 120s to discover that is pure delay. + // Arm 1 applies a full schema to a fresh database. The serving arms are + // expected to die during boot, before any DB connection or Kestrel bind, so + // they get a much shorter bound — a serving start that does NOT die is the + // failure those arms look for, and waiting 120s to discover it is pure delay. private static readonly TimeSpan MigrateTimeout = TimeSpan.FromSeconds(120); private static readonly TimeSpan ServingBootTimeout = TimeSpan.FromSeconds(45); - // Configuration that a Production SERVING boot must reject on three separate, - // independent guards: - // #260 RateLimiting:TrustedProxies empty -> HSTS + the per-IP login limiter - // silently go inert. - // #319 AllowedHosts "*" -> Host-header filtering is off. - // #316 Otlp:Endpoint plaintext http in -> telemetry and Otlp:Headers - // Production, no AllowInsecureEndpoint credentials exposed in transit. - // None of the three has any bearing on what a one-shot verb does. + // A Production SERVING boot must reject each of these, and none of them has + // any bearing on what a one-shot verb does. // - // satisfyOtlp acknowledges the plaintext endpoint, so #316 passes and a - // serving boot has to fail on #260/#319 instead. See arm 3 for why that - // matters: #316 is validated during service registration, ahead of the other - // two, so with all three violated the serving process ALWAYS dies at #316 and - // the other two are never reached. - private static void ApplyHostileServingConfiguration(ProcessStartInfo psi, bool satisfyOtlp) - { - // #260's condition is the ABSENCE of these keys, so an inherited value - // from the test host or a CI runner would silently disarm it — and the - // suite would stay green while proving less than it claims. Strip the - // whole section rather than the two names known today. - foreach (var inherited in psi.Environment.Keys - .Where(k => k.StartsWith("RateLimiting__", StringComparison.Ordinal)) - .ToList()) - psi.Environment.Remove(inherited); - - psi.Environment["AllowedHosts"] = "*"; - psi.Environment["Otlp__Endpoint"] = "http://collector.invalid:4317"; - if (satisfyOtlp) - psi.Environment["Otlp__AllowInsecureEndpoint"] = "true"; - } + // Violate is what BREAKS the guard; Satisfy is the minimum that clears it. + // For #260 the violation is the ABSENCE of configuration, which is exactly + // why this suite builds the child environment from scratch (see Start): an + // inherited value would silently satisfy it and no assertion would notice. + private sealed record ServingGuard( + string Issue, + string MessageToken, + Action Violate, + Action Satisfy); + + private static readonly ServingGuard[] ServingOnlyGuards = + [ + // #316 — plaintext OTLP in Production exposes telemetry and Otlp:Headers + // credentials in transit. Satisfied by acknowledging the plaintext peer, + // which keeps the endpoint set so the same code path still runs. + new("#316", "Otlp:Endpoint", + Violate: psi => psi.Environment["Otlp__Endpoint"] = "http://collector.invalid:4317", + Satisfy: psi => + { + psi.Environment["Otlp__Endpoint"] = "http://collector.invalid:4317"; + psi.Environment["Otlp__AllowInsecureEndpoint"] = "true"; + }), + + // #260 — an empty trusted-proxy list silently makes HSTS inert and + // collapses the per-IP login limiter to one global bucket. + new("#260", "RateLimiting:TrustedProxies", + Violate: _ => { /* the ABSENCE of the key IS the violation */ }, + Satisfy: psi => psi.Environment["RateLimiting__TrustedProxies__0"] = "10.0.0.0/8"), + + // #319 — a wildcard AllowedHosts turns Host-header filtering off, so a + // forged Host is accepted. + new("#319", "AllowedHosts", + Violate: psi => psi.Environment["AllowedHosts"] = "*", + Satisfy: psi => psi.Environment["AllowedHosts"] = "cluckwork-test.example"), + ]; - private static Process Start(string verb, string connectionString, bool satisfyOtlp = false) + // Only these are inherited from the test host. Everything else the child sees + // is set explicitly below. + // + // Built from scratch rather than by stripping known-bad prefixes, because + // stripping is the same "list what I thought of" method that produced the + // dead disjuncts above, and it had already sprung three leaks: the strip was + // Ordinal while Linux env keys are case-sensitive and .NET config keys are + // not (`ratelimiting__…` survived it), ASPNETCORE_-/DOTNET_-prefixed + // variables bind to the same config keys with the prefix removed, and + // `Otlp__` was never stripped at all — so an inherited + // Otlp__AllowInsecureEndpoint (a documented sim-harness setting) would have + // silently voided the #316 arm. + private static readonly string[] InheritedOsVariables = + ["PATH", "HOME", "DOTNET_ROOT", "TMPDIR", "LANG", "LC_ALL", "USER"]; + + private static Process Start( + string verb, string connectionString, ServingGuard? violate) { var arguments = verb.Length == 0 ? $"\"{ApiDllPath}\"" : $"\"{ApiDllPath}\" {verb}"; var psi = new ProcessStartInfo("dotnet", arguments) @@ -77,97 +109,107 @@ private static Process Start(string verb, string connectionString, bool satisfyO RedirectStandardError = true, UseShellExecute = false, }; + + psi.Environment.Clear(); + foreach (var name in InheritedOsVariables) + if (Environment.GetEnvironmentVariable(name) is { } value) + psi.Environment[name] = value; + psi.Environment["ASPNETCORE_ENVIRONMENT"] = "Production"; psi.Environment["ConnectionStrings__Default"] = connectionString; psi.Environment["Database__Provider"] = "Postgres"; - // The Testcontainers database is plaintext. The #261/#262 TLS floor applies - // to BOTH roles by design, so it is opted out of here rather than being one - // of the hostile settings under test — this suite is about role-scoped + // The Testcontainers database is plaintext. The #261/#262 TLS floor + // applies to BOTH roles by design, so it is opted out of rather than + // being one of the guards under test — this suite is about role-scoped // guards, and the TLS floor deliberately isn't one. psi.Environment["Database__AllowInsecureConnection"] = "true"; psi.Environment["Jwt__Issuer"] = "cluckwork-test"; psi.Environment["Jwt__Audience"] = "cluckwork-api-test"; psi.Environment["Jwt__PublicKeyPem"] = TestJwtKeys.PublicKeyPem; psi.Environment["Jwt__PrivateKeyPem"] = TestJwtKeys.PrivateKeyPem; - // Port 0 = an ephemeral port, so the serving arm cannot collide with - // anything else on the machine in the (failing) case where it boots. + // An ephemeral port, so a serving arm that wrongly BOOTS cannot collide + // with anything else on the machine. psi.Environment["ASPNETCORE_URLS"] = "http://127.0.0.1:0"; - ApplyHostileServingConfiguration(psi, satisfyOtlp); + + // violate == null means "violate them all" (arm 1). Otherwise exactly one + // guard is violated and every other is satisfied. + foreach (var guard in ServingOnlyGuards) + { + if (violate is null || ReferenceEquals(guard, violate)) + guard.Violate(psi); + else + guard.Satisfy(psi); + } + return Process.Start(psi)!; } + // ARM 1 — the guarantee. Every serving-only guard is violated at once and the + // one-shot verb still runs to completion. This is what #331 broke, and each + // guard is independently load-bearing here: misclassify any single one as + // applying to OneShot and `migrate` aborts. [Fact] - public async Task ServingOnlyGuards_DoNotAbortAOneShotVerb_AndStillFailAServingBoot() + public async Task AllServingGuardsViolated_DoesNotAbortAOneShotVerb() { await using var db = new PostgreSqlBuilder("postgres:18.4-trixie@sha256:3a82e1f56c8f0f5616a11103ac3d47e632c3938698946a7ad26da0df1334744a").Build(); await db.StartAsync(); - var connectionString = db.GetConnectionString(); - // ARM 1 — the guarantee. The one-shot verb runs to completion under the - // hostile serving configuration. This is what #331 broke. - var (migrateExit, migrateOut, migrateErr) = await SeedCommandRunner.RunToCompletionAsync( - Start("migrate", connectionString), MigrateTimeout); + var (exitCode, stdout, stderr) = await SeedCommandRunner.RunToCompletionAsync( + Start("migrate", db.GetConnectionString(), violate: null), MigrateTimeout); Assert.True( - 0 == migrateExit, + 0 == exitCode, "a serving-only boot guard aborted the one-shot `migrate` verb (the #331 regression). " - + $"expected exit 0, got {migrateExit}. stdout={migrateOut} stderr={migrateErr}"); - - // ARM 2 — proof that arm 1 was not vacuous. Same binary, same environment, - // no verb: this is a serving start, and it must die. Asserting only "exit - // non-zero" would accept a death from an unrelated cause (a missing key, an - // unreachable database), so the message has to name one of the three - // guards. Which one wins is NOT asserted: #316 throws during service - // registration and the other two after Build(), and pinning that order here - // would turn a legitimate future reordering into a red test for no reason. - var (servingExit, servingOut, servingErr) = await SeedCommandRunner.RunToCompletionAsync( - Start(string.Empty, connectionString), - ServingBootTimeout, - "a serving boot that SUCCEEDS here means this configuration is not hostile after all, " - + "so arm 1 above proved nothing"); + + $"expected exit 0, got {exitCode}. stdout={stdout} stderr={stderr}"); + } - Assert.True( - servingExit != 0, - "the configuration this test calls hostile did not actually fail a serving boot, so " - + "arm 1 proved nothing. Re-check the #260/#316/#319 conditions. " - + $"stdout={servingOut} stderr={servingErr}"); - - var namesAGuard = - servingErr.Contains("RateLimiting:TrustedProxies", StringComparison.Ordinal) - || servingErr.Contains("AllowedHosts", StringComparison.Ordinal) - || servingErr.Contains("Otlp:Endpoint", StringComparison.Ordinal); - Assert.True( - namesAGuard, - "the serving boot failed, but not on any of the three guards this test configures — " - + "so arm 1 is not exercising what it claims. " - + $"exit={servingExit} stdout={servingOut} stderr={servingErr}"); - - // ARM 3 — the half arm 2 structurally cannot cover. #316 is validated - // during SERVICE REGISTRATION, ahead of ServingBootGuards, so with all - // three settings violated a serving boot always dies at #316: arm 2 alone - // proves only that ONE of the three is hostile, and #260/#319 could - // quietly stop being violated (an appsettings default gaining a concrete - // AllowedHosts, AllowNoTrustedProxies flipping true, a CI runner exporting - // RateLimiting__*) with both arms still green and arm 1's coverage of them - // silently vacuous. Acknowledge the plaintext endpoint so #316 passes, and - // require the boot to die on one of the other two. - var (pinnedExit, pinnedOut, pinnedErr) = await SeedCommandRunner.RunToCompletionAsync( - Start(string.Empty, connectionString, satisfyOtlp: true), + // One arm per guard, so arm 1 above is never vacuous for ANY of them. + // + // Each case violates exactly one guard and satisfies the rest, then requires + // the boot to die naming THAT guard and no other. The negative half is what + // kills the defect two earlier versions of this test shipped: if the guards + // were reordered, or one stopped firing and a different one caught the boot + // instead, a "names one of them" assertion would stay green. This cannot. + public static TheoryData EveryServingGuard() + { + var data = new TheoryData(); + foreach (var guard in ServingOnlyGuards) + data.Add(guard.Issue); + return data; + } + + [Theory] + [MemberData(nameof(EveryServingGuard))] + public async Task EachServingGuard_AloneFailsAServingBoot(string issue) + { + var guard = ServingOnlyGuards.Single(g => g.Issue == issue); + + await using var db = new PostgreSqlBuilder("postgres:18.4-trixie@sha256:3a82e1f56c8f0f5616a11103ac3d47e632c3938698946a7ad26da0df1334744a").Build(); + await db.StartAsync(); + + var (exitCode, stdout, stderr) = await SeedCommandRunner.RunToCompletionAsync( + Start(string.Empty, db.GetConnectionString(), violate: guard), ServingBootTimeout, - "a serving boot that SUCCEEDS here means #260 and #319 are no longer violated by this " - + "configuration, so arm 1 no longer covers them"); + $"A serving boot that SUCCEEDS with {issue} violated means {issue} no longer fails a " + + "Production boot, so arm 1's coverage of it proves nothing."); Assert.True( - pinnedExit != 0, - "with #316 satisfied, the serving boot survived — #260 and #319 are not actually " - + $"violated by this configuration. stdout={pinnedOut} stderr={pinnedErr}"); + exitCode != 0, + $"{issue} alone did not fail a Production serving boot, so arm 1 does not actually " + + $"exercise it. stdout={stdout} stderr={stderr}"); - // Either may win; both are ServingBootGuards, and pinning which would make - // a legitimate reordering red for no reason. Assert.True( - pinnedErr.Contains("RateLimiting:TrustedProxies", StringComparison.Ordinal) - || pinnedErr.Contains("AllowedHosts", StringComparison.Ordinal), - "with #316 satisfied the serving boot failed, but on neither #260 nor #319 — so " - + $"arm 1's coverage of those two is not established. exit={pinnedExit} stderr={pinnedErr}"); + stderr.Contains(guard.MessageToken, StringComparison.Ordinal), + $"the serving boot failed with only {issue} violated, but the message does not name it " + + $"('{guard.MessageToken}'). exit={exitCode} stderr={stderr}"); + + // The half that makes this ordering-proof: no OTHER guard may be what + // stopped the boot, because no other guard is violated. + foreach (var other in ServingOnlyGuards.Where(g => !ReferenceEquals(g, guard))) + Assert.False( + stderr.Contains(other.MessageToken, StringComparison.Ordinal), + $"only {issue} was violated, but the boot failed naming {other.Issue} " + + $"('{other.MessageToken}') — that guard is firing when it should not, or " + + $"{issue} is not what stopped the boot. exit={exitCode} stderr={stderr}"); } } From 7141cfca212bc9c4d56f979f98b3a2fee543655d Mon Sep 17 00:00:00 2001 From: mforce <> Date: Mon, 10 Aug 2026 18:48:32 -0700 Subject: [PATCH 4/9] docs(decisions): drop v2 arm numbering from the 347 process notes --- docs/decisions/347-process-role.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/docs/decisions/347-process-role.md b/docs/decisions/347-process-role.md index 3f5e77504..ce92d6a2f 100644 --- a/docs/decisions/347-process-role.md +++ b/docs/decisions/347-process-role.md @@ -64,11 +64,11 @@ Mutation evidence, run before the claim: |---|---| | baseline, unmutated | **green** | | `EnsureServingConfiguration` drops its `role` check | **red** (`migrate` aborts on #260) | -| #316's degrade filter flipped to `Serving` | **red** — this is #331 verbatim | +| #316's degrade filter flipped to `Serving` | **red** — #331 verbatim | | `healthcheck` removed from `OneShotVerbs` | **red** (registry suite) | | the #319 `AllowedHosts` guard deleted outright | **red** — *survived v2 entirely* | | the #260 `TrustedProxies` guard neutered | **red** — *survived v2 entirely* | -| `healthcheck`'s early return removed from `Program.cs` | **red** (the assertion fires, exit 134) | +| `healthcheck`'s early return removed from `Program.cs` | **red** (the new assertion fires, exit 134) | | restored | **green** | The two marked rows are the ones that matter: under v2 both mutants left every arm green, which is how the dead disjuncts were found. They are the reason the per-guard table exists, and they are the regression check on it. @@ -77,4 +77,4 @@ Two process notes, because each nearly produced false evidence. The first mutation script restored with `git checkout --`, which **silently refuses to restore untracked files**. Two of the three targets were new files, so mutants stacked instead of reverting and two "red" results were measured against an already-mutated tree. Snapshot by file copy, and assert the restored tree is green again. -And a mutant being red is not the same as being red *for the stated reason*. The fourth was re-run on its own to confirm it fails on arm 3's timeout — the serving process boots clean, logging `OTLP export enabled` — rather than on arm 1 or 2, which is the entire claim behind adding arm 3. +And a mutant being red is not the same as being red *for the stated reason*. Each mutant is re-checked against the specific arm it should redden: the two single-guard rows must fail their own guard's arm — the serving process otherwise boots clean, logging `OTLP export enabled` — and not the one-shot arm, which is the whole claim behind deriving an arm per guard. From e26529763ea78a78034fc8c60336eca7caaa14da Mon Sep 17 00:00:00 2001 From: mforce <> Date: Mon, 10 Aug 2026 19:11:03 -0700 Subject: [PATCH 5/9] fix(api): stop serving-only config from aborting the one-shot verbs Round three of local review found two live bugs of the class this branch exists to eliminate, both reproduced against the real binary. The first is #331 verbatim, still reachable, three lines above its own fix. AddCluckworkTelemetry role-checked the endpoint resolution, but ParseProtocol sat outside the try, so Otlp__Protocol=bogus killed recover-admin with SIGABRT 134. The break-glass verb, taken out by a telemetry typo. The existing #331 regression test stepped over it by setting a valid protocol alongside the bad endpoint it was testing. The second is RateLimitingOptions.Validate, which ran at registration with no role check, so a malformed CIDR or a nonzero ReportsConcurrency queue limit aborted every verb. That left RateLimiting TrustedProxies empty correctly serving-only while the same key malformed was hostile to all roles. Opposite classifications for one key, visible from nowhere. Rate limiting is inbound-HTTP machinery a run-then-exit verb never serves, so the whole of its validation is now serving-only, degrading to defaults rather than the operator values just declared unusable. A malformed CIDR also fails as a named error instead of a bare FormatException from inside IPNetwork.Parse, which named neither the setting nor the value. The lesson is to scope the subsystem, not the single setting that bit you. Two test-side findings from the same round. Deleting a row from the guard table silently deleted its arms, so the earlier survivor was re-openable by editing the test; ServingGuardCoverageTests now reflects over the three places a serving-only guard can be added and fails when one has no row. And the ValidateOnStart upload caps were missing from the table entirely, being serving-only by mechanism rather than by a role check. Rows are per violation now, not per subsystem, and both directions are per row: two rows key on the same setting in mutually exclusive states, so a combined run could never violate both and whichever lost was covered by nothing. Serving arms drop their containers, and the whole class shares one. Nine mutants, all red, baseline and restore green. --- AGENTS.md | 2 +- docs/decisions/347-process-role.md | 32 +++- .../Configuration/OtlpOptions.cs | 7 +- ...RateLimitingServiceCollectionExtensions.cs | 32 +++- ...orkTelemetryServiceCollectionExtensions.cs | 48 ++++-- src/Cluckwork.Api/Program.cs | 2 +- .../RateLimiting/RateLimitingOptions.cs | 24 ++- .../ProcessRoleGuardTests.cs | 151 ++++++++++++++---- .../ServingGuardCoverageTests.cs | 146 +++++++++++++++++ 9 files changed, 379 insertions(+), 65 deletions(-) create mode 100644 tests/Cluckwork.Api.IntegrationTests/ServingGuardCoverageTests.cs diff --git a/AGENTS.md b/AGENTS.md index 758e01597..e9930a957 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -55,7 +55,7 @@ dotnet test Cluckwork.sln # 688 tests as of 2026-07; integratio - **Farm timezone + tzdata/ICU image constraint (#264):** the default account (a fixed `"UTC"` migration literal since #283) is provisioned once; a real farm sets its actual IANA zone via **Settings → timezone** after first login (`Account.UpdateSettings`), not at provisioning time (the `Seed:TimeZoneId` config lever from before #283 is retired along with the runtime seeder it fed). The farm clock resolves IANA zones via `TimeZoneInfo.FindSystemTimeZoneById` for every safety-sensitive local-day decision and **fails closed**, so the runtime **image MUST carry tzdata + ICU** (the Ubuntu 24.04 (Noble) `aspnet:10.0` base does; **never** an Alpine/chiseled base without tzdata, and **never** `InvariantGlobalization=true`). `AddCluckworkPersistence` still asserts a representative IANA canary resolves at boot (`TimeZoneAvailability.EnsureResolvable`), for **both** process roles — a bad image fails the boot loudly instead of surfacing later as a per-request `FarmTimeZoneException`. - **Migrate command + prod migration split (#263).** `dotnet Cluckwork.Api.dll migrate` applies EF migrations then exits — the **pre-deploy-job entrypoint**. In Production `Database:MigrateOnStartup=false`, so the serving process never runs DDL; the **guarantee is ordering** — compose runs a one-shot `migrate` service before `app` waits on `service_completed_successfully`. `DatabaseReadyHealthCheck` is the backstop: `/health/ready` **503s while any migration is pending**. The remaining host step (deploy repo) is a least-privilege runtime role with no DDL, plus `ALTER DEFAULT PRIVILEGES` for the migrator. → [`docs/decisions/263-migrate-command.md`](docs/decisions/263-migrate-command.md) - **Proxy-trust boot guard (#260):** behind a reverse proxy/edge, HSTS (#144) and the per-IP login limiter (#143) only work if the app trusts the proxy's `X-Forwarded-*`, which it does **only** for networks in `RateLimiting:TrustedProxies`. An **empty list in Production fails the boot** (a `ProcessRole.Serving`-only guard, so `migrate`/`seed`/`recover-admin` are unaffected — see the process-role bullet below) rather than silently running with inert HSTS + a one-bucket limiter. Opt out with `RateLimiting:AllowNoTrustedProxies=true` only for a rare direct-TLS deploy with no fronting proxy. The concrete edge CIDR is deploy config (in the separate deployment/ops repo). -- **Process role, not statement order (#347).** A boot guard's scope is declared, never positional. `ProcessRoles.From(args)` (`Hosting/ProcessRole.cs`) computes `ProcessRole.Serving | OneShot` **once**, before the host is built, and every role-scoped guard takes it: `ServingBootGuards.EnsureServingConfiguration` (#260, #319) is called **before** the CLI dispatch on purpose and still leaves the verbs alone, and `AddCluckworkTelemetry` (#316) degrades to export-disabled for `OneShot`. The one-shot verb list is derived from `CliDispatcher.Commands` so a new verb classifies itself; **`healthcheck` is the one that must be named explicitly** (it isn't an `ICliCommand` — it needs no host) and omitting it silently classifies the container's own probe as `Serving`. **Never gate a guard on where its statement sits**: that is what #331 was — the #316 validation ran at service registration, ahead of the dispatcher, and killed `recover-admin` (the break-glass verb) with SIGABRT 134. The #261/#262 TLS floor and the #264 tzdata canary take no role: they apply to **both**, and unconditional code says so better than a parameter nobody branches on. → [`docs/decisions/347-process-role.md`](docs/decisions/347-process-role.md) +- **Process role, not statement order (#347).** A boot guard's scope is declared, never positional. `ProcessRoles.From(args)` (`Hosting/ProcessRole.cs`) computes `ProcessRole.Serving | OneShot` **once**, before the host is built, and every role-scoped guard takes it: `ServingBootGuards.EnsureServingConfiguration` (#260, #319) is called **before** the CLI dispatch on purpose and still leaves the verbs alone, and `AddCluckworkTelemetry` (#316) and `AddCluckworkRateLimiting` (all of `RateLimitingOptions.Validate`) degrade with a stderr warning for `OneShot`. The one-shot verb list is derived from `CliDispatcher.Commands` so a new verb classifies itself; **`healthcheck` is the one that must be named explicitly** (it isn't an `ICliCommand` — it needs no host) and omitting it silently classifies the container's own probe as `Serving`. **Never gate a guard on where its statement sits**: that is what #331 was — the #316 validation ran at service registration, ahead of the dispatcher, and killed `recover-admin` (the break-glass verb) with SIGABRT 134. **Scope the whole subsystem, not the one setting that bit you**: #316's *endpoint* check was role-scoped while `ParseProtocol` two lines above it was not, so `Otlp:Protocol=bogus` still killed `recover-admin`; and `RateLimiting:TrustedProxies` **empty** was correctly serving-only while the **same key malformed** aborted every verb. **Every serving-only guard needs a row in `ProcessRoleGuardTests.ServingOnlyGuards`, one per VIOLATION rather than per subsystem** — `ServingGuardCoverageTests` reflects over `ServingBootGuards`, `RateLimitingOptions` and every `.ValidateOnStart()` registration and fails when a row is missing. Note that `.ValidateOnStart()` is serving-only *by mechanism* (it runs from `Host.StartAsync`, which the CLI dispatcher never reaches), so converting one to an eager registration-time check aborts every verb. The #261/#262 TLS floor and the #264 tzdata canary take no role: they apply to **both**, and unconditional code says so better than a parameter nobody branches on. → [`docs/decisions/347-process-role.md`](docs/decisions/347-process-role.md) - **Production Postgres TLS floor (#261/#262).** `ConnectionStrings:Default` accepts libpq URI or key-value form. In Production the effective `sslmode` is a **fail-closed allow-list**: `VerifyCA`/`VerifyFull` silent, `Require` warns, **everything else — `Disable`/`Allow`/`Prefer`, unset, or undefined — fails the boot**. `Database:AllowInsecureConnection=true` is the explicit opt-out for the co-located plaintext compose stack. When mapping a libpq param, **`ContainsKey("")==false` does NOT mean Npgsql lacks the setting** — usually the keyword is spelled differently (`keepalives`→`Tcp Keepalive`, `client_encoding`→`Client Encoding`); search the keyword list before concluding it's unmappable (that exact false negative shipped #332). Integration tests opt out of this **and** the #260 guard. → [`docs/decisions/261-postgres-tls-floor.md`](docs/decisions/261-postgres-tls-floor.md) - **GSS/Kerberos negotiation off by default (#332).** `PostgresConnectionString` appends `GSS Encryption Mode=Disable` **unless the operator set it** (detected by **presence, not value**), **textually** after the TLS floor runs (a round-trip through the Npgsql builder would reorder/requote the operator's string and throw on any keyword this version doesn't know). Without it, Npgsql's `Prefer` default makes every connector dlopen the absent `libgssapi_krb5.so.2` and print two pre-Serilog stderr lines per process. Orthogonal to `sslmode` and to GSS *authentication*. → [`docs/decisions/332-gss-kerberos.md`](docs/decisions/332-gss-kerberos.md) - **Design-time migration connection fail-closed (#318):** `AppDbContextDesignTimeFactory` (used by `dotnet ef migrations add`/`database update`, never by the `migrate` verb, which uses the built host's own config) has **no default connection** — an unset/blank `CLUCKWORK_MIGRATIONS_CONNECTION` throws immediately, naming the variable, rather than falling back to a predictable `Host=localhost;...;Username=postgres;Password=postgres`. Every target is held to the **same allow-list TLS floor as a Production boot** (#261/#262, reused via `PostgresConnectionString.NormalizeAndValidate` — no second validator), except an explicitly acknowledged **loopback** development target: `CLUCKWORK_MIGRATIONS_ALLOW_INSECURE_LOOPBACK=true` permits plaintext, but only when the connection's host is `localhost`/`127.0.0.1`/`::1` (checked via `IPAddress.IsLoopback`) — set against any other host, it fails rather than silently widening scope. diff --git a/docs/decisions/347-process-role.md b/docs/decisions/347-process-role.md index ce92d6a2f..7d7d134df 100644 --- a/docs/decisions/347-process-role.md +++ b/docs/decisions/347-process-role.md @@ -17,13 +17,24 @@ That is not a property anyone can read off the guard, and getting it wrong is no | Guard | Role | Where | |---|---|---| -| #260 trusted proxies | `Serving` only | `ServingBootGuards.EnsureServingConfiguration` | +| #260 trusted proxies (empty) | `Serving` only | `ServingBootGuards.EnsureServingConfiguration` | | #319 AllowedHosts | `Serving` only | `ServingBootGuards.EnsureServingConfiguration` | -| #316 OTLP endpoint | `Serving` throws; `OneShot` degrades to export-disabled | `AddCluckworkTelemetry` | +| #316 OTLP endpoint **and protocol** | `Serving` throws; `OneShot` degrades to export-disabled | `AddCluckworkTelemetry` | +| All of `RateLimitingOptions.Validate` — malformed CIDR, window limits, the #311 queue limit | `Serving` throws; `OneShot` degrades to defaults | `AddCluckworkRateLimiting` | +| `FarmLogo` / `FarmBanner` upload caps | `Serving` only, **by mechanism** | `AddCluckworkFeatures`, via `.ValidateOnStart()` | | #261/#262 Postgres TLS floor | **both** | `AddCluckworkPersistence`, unconditional | | #264 tzdata/ICU canary | **both** | `AddCluckworkPersistence`, unconditional | -Three details are load-bearing. +**Scope the whole subsystem, not the one setting that bit you.** Rows 3 and 4 read as they do because the first pass scoped exactly the setting #331 named and nothing else, which left two live bugs of the same shape — both found in review, both reproduced against the real binary: + +- `AddCluckworkTelemetry` role-checked the *endpoint* resolution, but `ParseProtocol()` sat three lines above the `try`. `Otlp:Protocol=bogus` therefore killed `recover-admin` with SIGABRT 134 — **#331 verbatim, still reproducible, immediately next to its own fix.** The #331 regression test stepped straight over it by setting a valid protocol alongside the bad endpoint it was testing. +- `RateLimitingOptions.Validate()` ran at registration with no role check, so a malformed CIDR or a nonzero `ReportsConcurrency:QueueLimit` aborted every verb. That left `RateLimiting:TrustedProxies` **empty** correctly serving-only while the **same key malformed** was hostile to all roles — opposite classifications for one key, visible from nowhere. + +Rate limiting is inbound-HTTP machinery a run-then-exit verb never serves, so the whole of its validation is serving-only. Its degrade falls back to **defaults, not the operator's values** — the operator's are the ones just declared unusable. A malformed CIDR now also fails as a named `InvalidOperationException` rather than a bare `FormatException` from inside `IPNetwork.Parse`, which named neither the setting nor the value and (not being an `InvalidOperationException`) also slipped past the role filter. + +The last serving-only row is serving-only for a different **reason**: `.ValidateOnStart()` runs from `Host.StartAsync`, and `CliDispatcher` operates on the built host without ever starting it. Those two behave correctly today and are listed because nothing said so — convert either to an eager check inside `AddCluckworkFeatures`, the shape #316 had, and every verb aborts. + +Three further details are load-bearing. **`EnsureServingConfiguration` is called BEFORE the CLI dispatch, on purpose.** Leaving it below would have worked and proved nothing. A serving-only guard sitting *ahead* of the dispatcher and still not touching the verbs is the #331 failure mode disarmed rather than avoided, and it is the worked example the next guard author needs. Move that call again and nothing breaks — that is the point. @@ -48,7 +59,13 @@ The first two versions both asserted *"the boot died naming ONE OF these guards" - **v1** — three guards, one disjunction. #316 is validated during **service registration**, ahead of `ServingBootGuards`, so the boot always died there. #260 and #319 were never proven hostile at all. - **v2** — added an arm that satisfied #316, and repeated the mistake one level down. `EnsureServingConfiguration` calls #260 then #319 unconditionally, so the boot always died at #260 and the `AllowedHosts` disjunct was dead. **A mutant deleting the #319 call outright survived every arm.** -Two misses of the same shape mean the method is wrong, not that the list needs another entry ([guard-writing rules](407-writing-a-guard.md)). So v3 has no list of arms. Every serving-only guard is a **row in a table**, and the suite derives one arm per row: violate exactly that guard, **satisfy every other**, and require the boot to die naming that guard **and not naming any other**. No arm ever has two violations to choose between, so guard ordering cannot hide anything, and the negative half turns "a different guard caught it" from silently green into red. Adding a fourth guard is adding a row. +Two misses of the same shape mean the method is wrong, not that the list needs another entry ([guard-writing rules](407-writing-a-guard.md)). So v3 has no list of arms. Every serving-only guard is a **row in a table**, and the suite derives **two** arms per row: one runs `migrate` with that guard violated and requires exit 0 (the guarantee), the other starts a serving process with that guard violated and **every other satisfied**, and requires it to die naming that guard **and not naming any other**. No arm ever has two violations to choose between, so guard ordering cannot hide anything, and the negative half turns "a different guard caught it" from silently green into red. + +Both directions are per-row, which is not symmetry for its own sake: two rows key on the **same setting in mutually exclusive states** (`RateLimiting:TrustedProxies` absent vs malformed), so a single combined "violate everything" run structurally cannot violate both, and whichever lost would have been covered by nothing — the dead-disjunct shape one level further out. Violations are applied *after* every `Satisfy` for the same reason; the first attempt at that pair failed loudly because a later `Satisfy` un-violated the row under test, which is what the negative assertions are for. + +**Rows are per VIOLATION, not per subsystem.** `Otlp:Endpoint` and `Otlp:Protocol` are separate rows because they had separate role behaviour, and that is exactly how the second #331 was found. + +**And the table itself is enforced, because it was not.** Deleting the #319 row deleted its arms and the suite went from green to green — so the v2 survivor was re-openable by editing the *test*, and any guard added later was covered by nothing. "Adding a guard is a row" was an invariant living in a comment, which is a bug unless a line enforces it. `ServingGuardCoverageTests` now reflects over the three places a serving-only guard can be added — `ServingBootGuards`' check methods, `RateLimitingOptions`' validation methods, and every `IValidateOptions<>` registered with `.ValidateOnStart()` — and fails when one has no row, in both directions so a rename cannot leave a stale entry claiming coverage. The same "list what I thought of" method had also sprung three leaks in how the child process's environment was built. The subprocess inherits the test host's environment and **#260's condition is the ABSENCE of `RateLimiting__*`**, so v2 stripped that prefix — but the strip was `Ordinal` while Linux environment keys are case-sensitive and .NET configuration keys are not (`ratelimiting__…` survived it), `ASPNETCORE_`- and `DOTNET_`-prefixed variables bind to the same configuration keys with the prefix removed, and `Otlp__` was never stripped at all — so an inherited `Otlp__AllowInsecureEndpoint` (a documented sim-harness setting) would have silently voided the #316 arm. The child environment is now **built from scratch**: cleared, then a small allow-list of OS variables, then every application setting stated explicitly. @@ -68,10 +85,13 @@ Mutation evidence, run before the claim: | `healthcheck` removed from `OneShotVerbs` | **red** (registry suite) | | the #319 `AllowedHosts` guard deleted outright | **red** — *survived v2 entirely* | | the #260 `TrustedProxies` guard neutered | **red** — *survived v2 entirely* | -| `healthcheck`'s early return removed from `Program.cs` | **red** (the new assertion fires, exit 134) | +| rate-limiting validation's role scope reverted | **red** — the live bug above | +| a row deleted from the table | **red** — *was silent before the coverage suite* | +| `ParseProtocol` moved back outside the role-checked `try` | **red** — the other live bug above | +| `healthcheck`'s early return removed from `Program.cs` | **red** (the assertion fires, exit 134) | | restored | **green** | -The two marked rows are the ones that matter: under v2 both mutants left every arm green, which is how the dead disjuncts were found. They are the reason the per-guard table exists, and they are the regression check on it. +The marked rows are the ones that matter. The two "survived v2" mutants are how the dead disjuncts were found and are the regression check on the per-guard table. The "row deleted" mutant is the regression check on the coverage suite — before it, deleting a row was green. Two process notes, because each nearly produced false evidence. diff --git a/src/Cluckwork.Api/Configuration/OtlpOptions.cs b/src/Cluckwork.Api/Configuration/OtlpOptions.cs index 5ff8846f2..5130a91ea 100644 --- a/src/Cluckwork.Api/Configuration/OtlpOptions.cs +++ b/src/Cluckwork.Api/Configuration/OtlpOptions.cs @@ -31,9 +31,14 @@ public sealed class OtlpOptions public bool Enabled => !string.IsNullOrWhiteSpace(Endpoint); + // What an unset Otlp:Protocol means, and what a one-shot verb falls back to + // when it degrades to export-disabled (#347) — nothing is exported in that + // state, so the value only has to be a definite one rather than the right one. + public const OtlpExportProtocol DefaultProtocol = OtlpExportProtocol.Grpc; + public OtlpExportProtocol ParseProtocol() => Protocol?.Trim().ToLowerInvariant() switch { - null or "" or "grpc" => OtlpExportProtocol.Grpc, + null or "" or "grpc" => DefaultProtocol, "http/protobuf" => OtlpExportProtocol.HttpProtobuf, var other => throw new InvalidOperationException( $"Otlp:Protocol must be 'grpc' or 'http/protobuf', got '{other}'.") diff --git a/src/Cluckwork.Api/Hosting/CluckworkRateLimitingServiceCollectionExtensions.cs b/src/Cluckwork.Api/Hosting/CluckworkRateLimitingServiceCollectionExtensions.cs index daa921787..5f4be8324 100644 --- a/src/Cluckwork.Api/Hosting/CluckworkRateLimitingServiceCollectionExtensions.cs +++ b/src/Cluckwork.Api/Hosting/CluckworkRateLimitingServiceCollectionExtensions.cs @@ -11,15 +11,41 @@ namespace Cluckwork.Api.Hosting; internal static class CluckworkRateLimitingServiceCollectionExtensions { + // #347 review — role is OneShot for the operator verbs. Every limiter built + // here exists to shape INBOUND HTTP, which a run-then-exit verb never serves, + // so this whole section is serving-only machinery and its validation must be + // scoped like one. It was not: a malformed CIDR or a nonzero + // ReportsConcurrency:QueueLimit aborted `migrate`/`recover-admin` at service + // registration — #331's shape again, and a stranger inconsistency than that, + // because RateLimiting:TrustedProxies being EMPTY was already correctly + // serving-only (#260) while the SAME key being malformed was hostile to every + // role. public static CluckworkRateLimitingRegistration AddCluckworkRateLimiting( this IServiceCollection services, - IConfiguration configuration) + IConfiguration configuration, + ProcessRole role = ProcessRole.Serving) { var rateLimiting = configuration .GetSection(RateLimitingOptions.SectionName) .Get() ?? new RateLimitingOptions(); - rateLimiting.Validate(); - var trustedProxies = rateLimiting.ParseTrustedProxies(); + IPNetwork[] trustedProxies; + try + { + rateLimiting.Validate(); + trustedProxies = rateLimiting.ParseTrustedProxies(); + } + catch (InvalidOperationException ex) when (role is ProcessRole.OneShot) + { + // Same degrade as the OTLP one: warn on stderr and carry on with + // defaults that nothing in this process will consult, rather than + // taking out an operational escape hatch over configuration for a + // server this process is not going to be. Defaults, not the operator's + // values — the operator's are the ones just declared unusable. + Console.Error.WriteLine( + $"warning: rate limiting not configured for this command — {ex.Message}"); + rateLimiting = new RateLimitingOptions(); + trustedProxies = []; + } services.AddRateLimiter(limiter => { diff --git a/src/Cluckwork.Api/Hosting/CluckworkTelemetryServiceCollectionExtensions.cs b/src/Cluckwork.Api/Hosting/CluckworkTelemetryServiceCollectionExtensions.cs index d024d48f8..69b366025 100644 --- a/src/Cluckwork.Api/Hosting/CluckworkTelemetryServiceCollectionExtensions.cs +++ b/src/Cluckwork.Api/Hosting/CluckworkTelemetryServiceCollectionExtensions.cs @@ -60,30 +60,46 @@ public static CluckworkTelemetryRegistration AddCluckworkTelemetry( // eagerly so configuration errors fail at boot. var otlp = configuration.GetSection(OtlpOptions.SectionName).Get() ?? new OtlpOptions(); - var protocol = otlp.ParseProtocol(); var isProduction = environment.IsProduction(); + var protocol = OtlpOptions.DefaultProtocol; Uri? traceEndpoint = null; Uri? metricsEndpoint = null; - if (otlp.Enabled) + try { - try + // #347 review — ParseProtocol MUST sit inside this try. It used to be + // three lines above it, outside, which left #331 live in a second + // form: `Otlp:Protocol=bogus` killed `recover-admin` with SIGABRT 134, + // the break-glass verb taken out by a telemetry typo. The existing + // #331 regression test stepped over it by setting a valid protocol + // alongside the bad endpoint it was testing. Validate the WHOLE + // telemetry configuration in one place so no part of it can be + // serving-only by accident. + protocol = otlp.ParseProtocol(); + if (otlp.Enabled) { traceEndpoint = otlp.ResolveTraceEndpoint(isProduction); metricsEndpoint = otlp.ResolveMetricsEndpoint(isProduction); } - catch (InvalidOperationException ex) when (role is ProcessRole.OneShot) - { - // A one-off verb must not die on a telemetry misconfiguration it - // does not depend on. `recover-admin` in particular is the - // break-glass path for a locked-out farm (#265) and is - // deliberately NOT environment-gated — an unrelated bad - // Otlp:Endpoint blocking an emergency password reset would be a - // worse failure than the one this validation prevents. Degrade to - // export DISABLED rather than exporting insecurely: the verb runs, - // and nothing leaves the process over an unvalidated endpoint. - Console.Error.WriteLine( - $"warning: OTLP export disabled for this command — {ex.Message}"); - } + } + catch (InvalidOperationException ex) when (role is ProcessRole.OneShot) + { + // A one-off verb must not die on a telemetry misconfiguration it does + // not depend on. `recover-admin` in particular is the break-glass path + // for a locked-out farm (#265) and is deliberately NOT + // environment-gated — an unrelated bad Otlp:* setting blocking an + // emergency password reset would be a worse failure than the one this + // validation prevents. Degrade to export DISABLED rather than + // exporting insecurely: the verb runs, and nothing leaves the process + // over an unvalidated endpoint. + // + // BOTH endpoints are cleared, not just the one that threw: if the + // trace endpoint resolved and the metrics endpoint did not, keeping + // the first would export half a signal set from a configuration this + // process has already declared unusable. + traceEndpoint = null; + metricsEndpoint = null; + Console.Error.WriteLine( + $"warning: OTLP export disabled for this command — {ex.Message}"); } Action ConfigureOtlpExporter(Uri endpoint) => options => diff --git a/src/Cluckwork.Api/Program.cs b/src/Cluckwork.Api/Program.cs index 8e685a362..ae0b590d8 100644 --- a/src/Cluckwork.Api/Program.cs +++ b/src/Cluckwork.Api/Program.cs @@ -58,7 +58,7 @@ builder.Services.AddCluckworkIdentity(builder.Configuration); var rateLimiting = builder.Services.AddCluckworkRateLimiting( - builder.Configuration); + builder.Configuration, processRole); builder.Services.AddCluckworkEdgeSecurity(rateLimiting.TrustedProxies); builder.Services.AddCluckworkFeatures(builder.Configuration); diff --git a/src/Cluckwork.Api/RateLimiting/RateLimitingOptions.cs b/src/Cluckwork.Api/RateLimiting/RateLimitingOptions.cs index 1fe4065f6..ef510280f 100644 --- a/src/Cluckwork.Api/RateLimiting/RateLimitingOptions.cs +++ b/src/Cluckwork.Api/RateLimiting/RateLimitingOptions.cs @@ -59,8 +59,26 @@ public sealed class ConcurrencyPolicy public int QueueLimit { get; init; } } - public IPNetwork[] ParseTrustedProxies() => - [.. TrustedProxies.Select(IPNetwork.Parse)]; + // #347 review — a malformed entry used to surface as a bare + // `FormatException: An invalid IP network was specified.` from deep inside + // IPNetwork.Parse, naming neither the setting nor the value, and (being a + // FormatException) it also slipped past the role filter that spares the + // one-shot verbs. Both problems are the same fix: fail as a named + // InvalidOperationException, like every other configuration guard here. + public IPNetwork[] ParseTrustedProxies() + { + var networks = new IPNetwork[TrustedProxies.Length]; + for (var i = 0; i < TrustedProxies.Length; i++) + { + var entry = TrustedProxies[i]; + if (!IPNetwork.TryParse(entry, out networks[i])) + throw new InvalidOperationException( + $"RateLimiting:TrustedProxies[{i}] is not a valid CIDR network: '{entry}'. " + + "Use an address and prefix length, e.g. '10.0.0.0/8' or '2001:db8::/32'."); + } + + return networks; + } // Fail fast at boot rather than throwing inside the partition factory on the // first auth request (which would surface as a 500 on the login path). @@ -70,7 +88,7 @@ public void Validate() ValidateWindow(nameof(Refresh), Refresh); ValidateWindow(nameof(ClientErrors), ClientErrors); ValidateConcurrency(nameof(ReportsConcurrency), ReportsConcurrency); - ParseTrustedProxies(); // throws FormatException on a bad CIDR + ParseTrustedProxies(); // throws a named InvalidOperationException on a bad CIDR } private static void ValidateWindow(string name, FixedWindow window) diff --git a/tests/Cluckwork.Api.IntegrationTests/ProcessRoleGuardTests.cs b/tests/Cluckwork.Api.IntegrationTests/ProcessRoleGuardTests.cs index 9b6a454bd..e9adfe534 100644 --- a/tests/Cluckwork.Api.IntegrationTests/ProcessRoleGuardTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/ProcessRoleGuardTests.cs @@ -34,8 +34,27 @@ namespace Cluckwork.Api.IntegrationTests; // other, and require the boot to die naming that guard AND NOT naming any // other. Ordering between guards then cannot hide anything, because no arm ever // has two violations to choose between. Adding a fourth guard is a row. -public sealed class ProcessRoleGuardTests +// One Postgres for the whole class. The one-shot arms run `migrate` per guard — +// the first applies the schema, the rest converge on "already current" — so the +// alternative was a container per row for no added coverage, and every container +// is another chance to hit Docker flake on CI. +public sealed class ServingGuardDatabaseFixture : IAsyncLifetime { + private readonly PostgreSqlContainer _container = new PostgreSqlBuilder( + "postgres:18.4-trixie@sha256:3a82e1f56c8f0f5616a11103ac3d47e632c3938698946a7ad26da0df1334744a") + .Build(); + + public string ConnectionString => _container.GetConnectionString(); + + public Task InitializeAsync() => _container.StartAsync(); + + public Task DisposeAsync() => _container.DisposeAsync().AsTask(); +} + +public sealed class ProcessRoleGuardTests(ServingGuardDatabaseFixture database) + : IClassFixture +{ + private readonly ServingGuardDatabaseFixture _database = database; private static readonly string ApiDllPath = typeof(Program).Assembly.Location; // Arm 1 applies a full schema to a fresh database. The serving arms are @@ -45,6 +64,10 @@ public sealed class ProcessRoleGuardTests private static readonly TimeSpan MigrateTimeout = TimeSpan.FromSeconds(120); private static readonly TimeSpan ServingBootTimeout = TimeSpan.FromSeconds(45); + // The serving arms never reach a database — see the call site. + private const string UnreachableDatabase = + "Host=127.0.0.1;Port=1;Database=none;Username=none;Password=none;Timeout=3;Command Timeout=3"; + // A Production SERVING boot must reject each of these, and none of them has // any bearing on what a one-shot verb does. // @@ -63,7 +86,7 @@ private sealed record ServingGuard( // #316 — plaintext OTLP in Production exposes telemetry and Otlp:Headers // credentials in transit. Satisfied by acknowledging the plaintext peer, // which keeps the endpoint set so the same code path still runs. - new("#316", "Otlp:Endpoint", + new("#316 endpoint", "Otlp:Endpoint", Violate: psi => psi.Environment["Otlp__Endpoint"] = "http://collector.invalid:4317", Satisfy: psi => { @@ -71,19 +94,72 @@ private sealed record ServingGuard( psi.Environment["Otlp__AllowInsecureEndpoint"] = "true"; }), + // A SECOND violation of the same subsystem, and its own row because it + // had its own role behaviour: ParseProtocol sat OUTSIDE the role-checked + // catch, so `Otlp:Protocol=bogus` killed `recover-admin` with SIGABRT 134 + // — #331 alive in a second form, three lines above its own fix, and the + // #331 regression test stepped over it by setting a valid protocol + // alongside the bad endpoint it was testing. One row per VIOLATION, not + // per subsystem, is what makes that visible. + new("#316 protocol", "Otlp:Protocol must be", + Violate: psi => psi.Environment["Otlp__Protocol"] = "not-a-protocol", + Satisfy: psi => psi.Environment["Otlp__Protocol"] = "grpc"), + // #260 — an empty trusted-proxy list silently makes HSTS inert and // collapses the per-IP login limiter to one global bucket. - new("#260", "RateLimiting:TrustedProxies", - Violate: _ => { /* the ABSENCE of the key IS the violation */ }, + new("#260 empty", "RateLimiting:TrustedProxies is empty", + // Removes rather than simply not-setting: the "#260 malformed" row + // below keys on the SAME setting, so its Satisfy would otherwise + // supply a valid value and quietly un-violate this row. Violations are + // applied after every Satisfy for exactly this reason (see Start). + Violate: psi => psi.Environment.Remove("RateLimiting__TrustedProxies__0"), Satisfy: psi => psi.Environment["RateLimiting__TrustedProxies__0"] = "10.0.0.0/8"), + // The same key MALFORMED, which used to be classified the opposite way + // from the same key being empty: empty was correctly serving-only, while + // a bad CIDR threw from RateLimitingOptions.Validate at registration and + // took out every verb. + new("#260 malformed", "is not a valid CIDR network", + Violate: psi => psi.Environment["RateLimiting__TrustedProxies__0"] = "not-a-cidr", + Satisfy: psi => psi.Environment["RateLimiting__TrustedProxies__0"] = "10.0.0.0/8"), + + // The rest of RateLimitingOptions.Validate, same story — inbound-HTTP + // machinery that a run-then-exit verb never serves. + new("#311 queue", "ReportsConcurrency:QueueLimit must be 0", + Violate: psi => psi.Environment["RateLimiting__ReportsConcurrency__QueueLimit"] = "5", + Satisfy: psi => psi.Environment["RateLimiting__ReportsConcurrency__QueueLimit"] = "0"), + // #319 — a wildcard AllowedHosts turns Host-header filtering off, so a // forged Host is accepted. - new("#319", "AllowedHosts", + new("#319", "AllowedHosts is missing, blank, or wildcard", Violate: psi => psi.Environment["AllowedHosts"] = "*", Satisfy: psi => psi.Environment["AllowedHosts"] = "cluckwork-test.example"), + + // The two ValidateOnStart upload caps. Serving-only for a different + // REASON than the three above — not a role check but a mechanism: + // .ValidateOnStart() fires from Host.StartAsync, and CliDispatcher + // operates on the BUILT host without ever starting it. They behave + // correctly today and are rows precisely because nothing said so: convert + // either to an eager check inside AddCluckworkFeatures — the shape #316 + // had — and it aborts every verb while the rest of this suite stays green. + // The first derivation missed both because it walked `IsProduction` and + // explicit `throw` sites, which is "list what I thought of" one + // abstraction level up. ServingGuardCoverageTests now enumerates them. + new("FarmLogo", "FarmLogo:MaxUploadBytes", + Violate: psi => psi.Environment["FarmLogo__MaxUploadBytes"] = "0", + Satisfy: psi => psi.Environment["FarmLogo__MaxUploadBytes"] = "2097152"), + + new("FarmBanner", "FarmBanner:MaxUploadBytes", + Violate: psi => psi.Environment["FarmBanner__MaxUploadBytes"] = "0", + Satisfy: psi => psi.Environment["FarmBanner__MaxUploadBytes"] = "5242880"), ]; + // Exposed so ServingGuardCoverageTests can hold this table against the source + // it claims to enumerate — without that, deleting a row silently deletes an + // arm and the suite stays green (#347 review round 3). + internal static IReadOnlyList CoveredGuardTokens => + [.. ServingOnlyGuards.Select(g => g.MessageToken)]; + // Only these are inherited from the test host. Everything else the child sees // is set explicitly below. // @@ -100,7 +176,7 @@ private sealed record ServingGuard( ["PATH", "HOME", "DOTNET_ROOT", "TMPDIR", "LANG", "LC_ALL", "USER"]; private static Process Start( - string verb, string connectionString, ServingGuard? violate) + string verb, string connectionString, ServingGuard violate) { var arguments = verb.Length == 0 ? $"\"{ApiDllPath}\"" : $"\"{ApiDllPath}\" {verb}"; var psi = new ProcessStartInfo("dotnet", arguments) @@ -131,39 +207,44 @@ private static Process Start( // with anything else on the machine. psi.Environment["ASPNETCORE_URLS"] = "http://127.0.0.1:0"; - // violate == null means "violate them all" (arm 1). Otherwise exactly one - // guard is violated and every other is satisfied. - foreach (var guard in ServingOnlyGuards) - { - if (violate is null || ReferenceEquals(guard, violate)) - guard.Violate(psi); - else - guard.Satisfy(psi); - } + // Satisfy every OTHER guard first, then violate the target LAST. Two rows + // key on the same setting (`RateLimiting:TrustedProxies` absent vs + // malformed), so a Satisfy running afterwards would silently un-violate + // the row under test — which is how the first version of this pair failed, + // and it failed loudly, which is the point of the negative assertions. + foreach (var guard in ServingOnlyGuards.Where(g => !ReferenceEquals(g, violate))) + guard.Satisfy(psi); + violate.Violate(psi); return Process.Start(psi)!; } - // ARM 1 — the guarantee. Every serving-only guard is violated at once and the - // one-shot verb still runs to completion. This is what #331 broke, and each - // guard is independently load-bearing here: misclassify any single one as - // applying to OneShot and `migrate` aborts. - [Fact] - public async Task AllServingGuardsViolated_DoesNotAbortAOneShotVerb() + // THE GUARANTEE, per guard. Violating any one of them must not abort the + // one-shot verb — that is what #331 broke. + // + // Per guard rather than all-at-once, and that is not symmetry for its own + // sake: two rows key on the same setting in mutually exclusive states + // (`RateLimiting:TrustedProxies` absent vs malformed), so a single combined + // run structurally CANNOT violate both, and whichever lost would have been + // covered here by nothing. That is the same dead-disjunct shape one level + // further out, and it is why the malformed case — a live bug this PR fixes — + // needs its own run. + [Theory] + [MemberData(nameof(EveryServingGuard))] + public async Task EachServingGuard_DoesNotAbortAOneShotVerb(string issue) { - await using var db = new PostgreSqlBuilder("postgres:18.4-trixie@sha256:3a82e1f56c8f0f5616a11103ac3d47e632c3938698946a7ad26da0df1334744a").Build(); - await db.StartAsync(); + var guard = ServingOnlyGuards.Single(g => g.Issue == issue); var (exitCode, stdout, stderr) = await SeedCommandRunner.RunToCompletionAsync( - Start("migrate", db.GetConnectionString(), violate: null), MigrateTimeout); + Start("migrate", _database.ConnectionString, violate: guard), MigrateTimeout); Assert.True( 0 == exitCode, - "a serving-only boot guard aborted the one-shot `migrate` verb (the #331 regression). " - + $"expected exit 0, got {exitCode}. stdout={stdout} stderr={stderr}"); + $"the serving-only guard {issue} aborted the one-shot `migrate` verb — the #331 " + + $"regression. expected exit 0, got {exitCode}. stdout={stdout} stderr={stderr}"); } - // One arm per guard, so arm 1 above is never vacuous for ANY of them. + // The other direction, so the arms above are never vacuous for ANY guard. // // Each case violates exactly one guard and satisfies the rest, then requires // the boot to die naming THAT guard and no other. The negative half is what @@ -184,19 +265,21 @@ public async Task EachServingGuard_AloneFailsAServingBoot(string issue) { var guard = ServingOnlyGuards.Single(g => g.Issue == issue); - await using var db = new PostgreSqlBuilder("postgres:18.4-trixie@sha256:3a82e1f56c8f0f5616a11103ac3d47e632c3938698946a7ad26da0df1334744a").Build(); - await db.StartAsync(); - var (exitCode, stdout, stderr) = await SeedCommandRunner.RunToCompletionAsync( - Start(string.Empty, db.GetConnectionString(), violate: guard), + // No container here: every one of these guards stops the boot before + // any database work (Production sets Database:MigrateOnStartup=false), + // so an unreachable literal is enough — and it is not a weakening, + // since a process that got far enough to need a database would die + // without this arm's token and go red on the assertion below. + Start(string.Empty, UnreachableDatabase, violate: guard), ServingBootTimeout, $"A serving boot that SUCCEEDS with {issue} violated means {issue} no longer fails a " - + "Production boot, so arm 1's coverage of it proves nothing."); + + "Production boot, so the one-shot arm's coverage of it proves nothing."); Assert.True( exitCode != 0, - $"{issue} alone did not fail a Production serving boot, so arm 1 does not actually " - + $"exercise it. stdout={stdout} stderr={stderr}"); + $"{issue} alone did not fail a Production serving boot, so the one-shot arm does not " + + $"actually exercise it. stdout={stdout} stderr={stderr}"); Assert.True( stderr.Contains(guard.MessageToken, StringComparison.Ordinal), diff --git a/tests/Cluckwork.Api.IntegrationTests/ServingGuardCoverageTests.cs b/tests/Cluckwork.Api.IntegrationTests/ServingGuardCoverageTests.cs new file mode 100644 index 000000000..37c713ec4 --- /dev/null +++ b/tests/Cluckwork.Api.IntegrationTests/ServingGuardCoverageTests.cs @@ -0,0 +1,146 @@ +namespace Cluckwork.Api.IntegrationTests; + +using System.Reflection; +using Cluckwork.Api.Hosting; +using Cluckwork.Api.RateLimiting; +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; + +// #347 review round 3 — makes ProcessRoleGuardTests' table track the source. +// +// That suite derives one arm per serving-only boot guard, which is only as good +// as the table being complete. It was not enforced: deleting the #319 row +// deleted its arm and the suite went from 4 green to 3 green — silently. So the +// v2 defect (a guard proven by nothing) was re-openable by editing the test, and +// any guard added later was covered by nothing at all. "Adding a guard is a row" +// was an invariant living in a comment, which AGENTS.md calls a bug unless a +// line enforces it. +// +// These enumerate the two places a serving-only guard can be added and hold each +// against the table. Neither reads the table's own definition of what a guard is +// — that would be the tautology the registry test was corrected for. +public sealed class ServingGuardCoverageTests +{ + // 1. Guards inside ServingBootGuards. Every private static check method + // there is one, by construction of the class. Reflection rather than a + // list, so a fourth method added tomorrow fails this immediately. + [Fact] + public void EveryServingBootGuardMethod_HasACoveringRow() + { + var guardMethods = typeof(ServingBootGuards) + .GetMethods(BindingFlags.NonPublic | BindingFlags.Static) + .Where(m => m.Name.StartsWith("Ensure", StringComparison.Ordinal)) + .Select(m => m.Name) + .ToArray(); + + // The mapping is deliberately explicit and tiny: a method name cannot be + // derived from a config key, and guessing would let a rename pass. + var covered = new Dictionary(StringComparer.Ordinal) + { + ["EnsureTrustedProxiesConfigured"] = "RateLimiting:TrustedProxies is empty", + ["EnsureAllowedHostsPinned"] = "AllowedHosts is missing, blank, or wildcard", + }; + + Assert.NotEmpty(guardMethods); + + var uncovered = guardMethods.Where(m => !covered.ContainsKey(m)).ToArray(); + Assert.True( + uncovered.Length == 0, + "ServingBootGuards has check method(s) with no row in " + + $"ProcessRoleGuardTests.ServingOnlyGuards: {string.Join(", ", uncovered)}. " + + "A serving-only guard with no row is proven by nothing — add a row (violate it, " + + "satisfy the others, assert the boot dies naming it) and map it here."); + + // The reverse direction, so this mapping cannot rot either: a method + // removed or renamed must not leave a stale entry claiming coverage. + var stale = covered.Keys.Where(k => !guardMethods.Contains(k)).ToArray(); + Assert.True( + stale.Length == 0, + $"this mapping names ServingBootGuards method(s) that no longer exist: " + + $"{string.Join(", ", stale)}. Was one renamed or removed?"); + + foreach (var token in covered.Values) + Assert.Contains(token, ProcessRoleGuardTests.CoveredGuardTokens); + } + + // 2. RateLimitingOptions.Validate — the second place a serving-only guard + // can hide, and the one that produced two live bugs: every check in it + // aborted one-shot verbs at service registration. Reflected for the same + // reason as ServingBootGuards: a check added here tomorrow must not be + // silently uncovered. + [Fact] + public void EveryRateLimitingValidation_HasACoveringRow() + { + var validationMethods = typeof(RateLimitingOptions) + .GetMethods(BindingFlags.NonPublic | BindingFlags.Static) + .Where(m => m.Name.StartsWith("Validate", StringComparison.Ordinal)) + .Select(m => m.Name) + .Concat([nameof(RateLimitingOptions.ParseTrustedProxies)]) + .ToArray(); + + var covered = new Dictionary(StringComparer.Ordinal) + { + // ValidateWindow guards Login/Refresh/ClientErrors permit+window; one + // row stands for the family because they share a single code path and + // a single message shape. + ["ValidateWindow"] = "PermitLimit must be greater than 0", + ["ValidateConcurrency"] = "ReportsConcurrency:QueueLimit must be 0", + ["ParseTrustedProxies"] = "is not a valid CIDR network", + }; + + Assert.NotEmpty(validationMethods); + + var uncovered = validationMethods.Where(m => !covered.ContainsKey(m)).ToArray(); + Assert.True( + uncovered.Length == 0, + "RateLimitingOptions has validation method(s) with no row in " + + $"ProcessRoleGuardTests.ServingOnlyGuards: {string.Join(", ", uncovered)}. " + + "Rate limiting is inbound-HTTP machinery, so its validation must not abort a " + + "one-shot verb — add a row proving that."); + + var stale = covered.Keys.Where(k => !validationMethods.Contains(k)).ToArray(); + Assert.True( + stale.Length == 0, + $"this mapping names RateLimitingOptions method(s) that no longer exist: " + + $"{string.Join(", ", stale)}. Was one renamed or removed?"); + } + + // 3. Guards that are serving-only by MECHANISM rather than by a role check: + // .ValidateOnStart() runs from Host.StartAsync, which the CLI dispatcher + // never reaches. These are the ones the first derivation missed entirely, + // so they are enumerated from the real registration rather than listed. + [Fact] + public void EveryValidateOnStartOption_HasACoveringRow() + { + var services = new ServiceCollection(); + services.AddCluckworkFeatures(new ConfigurationBuilder().Build()); + + var validatedOptionTypes = services + .Where(d => d.ServiceType.IsGenericType + && d.ServiceType.GetGenericTypeDefinition() == typeof(IValidateOptions<>)) + .Select(d => d.ServiceType.GetGenericArguments()[0].Name) + .Distinct(StringComparer.Ordinal) + .ToArray(); + + Assert.NotEmpty(validatedOptionTypes); + + // An options type validated at host start fails a SERVING boot and is + // skipped for every one-shot verb, which is exactly the property the + // table exists to pin. The row's token is the config key the validator + // names, so match on the section prefix the options type is named for. + var uncovered = validatedOptionTypes + .Where(t => !ProcessRoleGuardTests.CoveredGuardTokens.Any(token => + token.StartsWith( + t.Replace("Options", string.Empty, StringComparison.Ordinal) + ":", + StringComparison.Ordinal))) + .ToArray(); + + Assert.True( + uncovered.Length == 0, + "these options types are validated at host start — so they fail a serving boot and " + + "are skipped for one-shot verbs — but have no row in " + + $"ProcessRoleGuardTests.ServingOnlyGuards: {string.Join(", ", uncovered)}. " + + "Add a row so arm 1 proves the verbs survive them."); + } +} From b556f36d521e82497348ee91ec9252e04197ceca Mon Sep 17 00:00:00 2001 From: mforce <> Date: Mon, 10 Aug 2026 19:30:15 -0700 Subject: [PATCH 6/9] fix(api): scope config binding to the serving role and assert the property Codex found the same class twice more, at the layer under the last fix. The config BINDING sat outside the role boundary in both subsystems, so Get() threw on a non-boolean Otlp AllowInsecureEndpoint and Get() threw on a non-numeric Login PermitLimit, before either role-checked try was entered. Both aborted every one-shot verb. Both are now inside the boundary, and the degrade resets the whole bound object rather than the part that threw. That is the fifth instance of one bug class across four review rounds, each found after the previous scope-that-subsystem fix. Enumerating guards was the wrong method: it can only ever cover what someone remembered. So this adds a test that asserts the property instead. Every one-shot verb runs under a Production environment holding only a connection string and its own arguments, and must not crash out of service registration. Every one of the five instances would have failed it on the day it was written. Mutation checked: making the serving guards ignore role reddens three of its four cases. The guard table stays, because it pins which guard fails a serving boot, which the property test does not. It gains rows for both binding failures and for each individual window and concurrency check, since a row per helper left one violation of each pair authoritative for nothing. Two coverage-test gaps codex also found: the rate-limiting enumeration never compared its mapping against the table, so deleting a row kept it green, and the ValidateOnStart enumeration only executed one registration extension. The first now cross-checks; the second is joined by a source-wide sweep so a registration added anywhere else fails loudly. One token needed tightening: the endpoint guard's own message tells the operator to set Otlp AllowInsecureEndpoint, so the bare key was not discriminating and the negative assertion caught it. --- ...RateLimitingServiceCollectionExtensions.cs | 13 ++- ...orkTelemetryServiceCollectionExtensions.cs | 32 +++--- .../OneShotVerbMinimalConfigTests.cs | 102 ++++++++++++++++++ .../ProcessRoleGuardTests.cs | 49 ++++++++- .../ServingGuardCoverageTests.cs | 65 +++++++++-- 5 files changed, 233 insertions(+), 28 deletions(-) create mode 100644 tests/Cluckwork.Api.IntegrationTests/OneShotVerbMinimalConfigTests.cs diff --git a/src/Cluckwork.Api/Hosting/CluckworkRateLimitingServiceCollectionExtensions.cs b/src/Cluckwork.Api/Hosting/CluckworkRateLimitingServiceCollectionExtensions.cs index 5f4be8324..75d4620bd 100644 --- a/src/Cluckwork.Api/Hosting/CluckworkRateLimitingServiceCollectionExtensions.cs +++ b/src/Cluckwork.Api/Hosting/CluckworkRateLimitingServiceCollectionExtensions.cs @@ -25,12 +25,19 @@ public static CluckworkRateLimitingRegistration AddCluckworkRateLimiting( IConfiguration configuration, ProcessRole role = ProcessRole.Serving) { - var rateLimiting = configuration - .GetSection(RateLimitingOptions.SectionName) - .Get() ?? new RateLimitingOptions(); + var rateLimiting = new RateLimitingOptions(); IPNetwork[] trustedProxies; try { + // The BINDING is inside the boundary too, not just the validation: a + // non-numeric `RateLimiting:Login:PermitLimit` throws from Get() + // before any validator runs, and that aborted every verb just as + // surely (#347 review). Same lesson as the OTLP section — scope + // everything that can reject this configuration, or the next + // unscoped part of it is the next #331. + rateLimiting = configuration + .GetSection(RateLimitingOptions.SectionName) + .Get() ?? new RateLimitingOptions(); rateLimiting.Validate(); trustedProxies = rateLimiting.ParseTrustedProxies(); } diff --git a/src/Cluckwork.Api/Hosting/CluckworkTelemetryServiceCollectionExtensions.cs b/src/Cluckwork.Api/Hosting/CluckworkTelemetryServiceCollectionExtensions.cs index 69b366025..fbb312c93 100644 --- a/src/Cluckwork.Api/Hosting/CluckworkTelemetryServiceCollectionExtensions.cs +++ b/src/Cluckwork.Api/Hosting/CluckworkTelemetryServiceCollectionExtensions.cs @@ -58,22 +58,23 @@ public static CluckworkTelemetryRegistration AddCluckworkTelemetry( // OTLP export is config-gated (#214). Endpoint and protocol are validated // eagerly so configuration errors fail at boot. - var otlp = configuration.GetSection(OtlpOptions.SectionName).Get() - ?? new OtlpOptions(); var isProduction = environment.IsProduction(); + var otlp = new OtlpOptions(); var protocol = OtlpOptions.DefaultProtocol; Uri? traceEndpoint = null; Uri? metricsEndpoint = null; try { - // #347 review — ParseProtocol MUST sit inside this try. It used to be - // three lines above it, outside, which left #331 live in a second - // form: `Otlp:Protocol=bogus` killed `recover-admin` with SIGABRT 134, - // the break-glass verb taken out by a telemetry typo. The existing - // #331 regression test stepped over it by setting a valid protocol - // alongside the bad endpoint it was testing. Validate the WHOLE - // telemetry configuration in one place so no part of it can be - // serving-only by accident. + // Everything that can reject this section sits INSIDE this boundary — + // binding, protocol parsing, endpoint resolution — and that + // completeness is the whole point. Two rounds of review each found one + // more part of it outside: first ParseProtocol (so `Otlp:Protocol=bogus` + // killed `recover-admin` with SIGABRT 134, #331 verbatim, three lines + // from its own fix), then the BINDING itself (so a non-boolean + // `Otlp:AllowInsecureEndpoint` threw from Get() before any + // of it ran). Scope the subsystem, not the setting that bit you. + otlp = configuration.GetSection(OtlpOptions.SectionName).Get() + ?? new OtlpOptions(); protocol = otlp.ParseProtocol(); if (otlp.Enabled) { @@ -92,10 +93,13 @@ public static CluckworkTelemetryRegistration AddCluckworkTelemetry( // exporting insecurely: the verb runs, and nothing leaves the process // over an unvalidated endpoint. // - // BOTH endpoints are cleared, not just the one that threw: if the - // trace endpoint resolved and the metrics endpoint did not, keeping - // the first would export half a signal set from a configuration this - // process has already declared unusable. + // Everything is reset, not just the part that threw: if the trace + // endpoint resolved and the metrics endpoint did not, keeping the + // first would export half a signal set from a configuration this + // process has already declared unusable — and a partially bound + // `otlp` would still feed Headers to an exporter. + otlp = new OtlpOptions(); + protocol = OtlpOptions.DefaultProtocol; traceEndpoint = null; metricsEndpoint = null; Console.Error.WriteLine( diff --git a/tests/Cluckwork.Api.IntegrationTests/OneShotVerbMinimalConfigTests.cs b/tests/Cluckwork.Api.IntegrationTests/OneShotVerbMinimalConfigTests.cs new file mode 100644 index 000000000..4eeaac32e --- /dev/null +++ b/tests/Cluckwork.Api.IntegrationTests/OneShotVerbMinimalConfigTests.cs @@ -0,0 +1,102 @@ +namespace Cluckwork.Api.IntegrationTests; + +using System.Diagnostics; +using Cluckwork.Api.IntegrationTests.Infrastructure; + +// #347 — every one-shot verb must run under a Production environment containing +// ONLY the configuration that verb genuinely needs. +// +// This asserts the PROPERTY. ProcessRoleGuardTests enumerates the individual +// serving-only guards and pins which of them fails a serving boot; this pins the +// other half — that none of them, named or unnamed, can abort a verb — and it +// does so without anyone having to remember a table. +// +// It exists because enumerating was not working. Five instances of one bug class +// were found across four review rounds, each after the previous "scope that +// subsystem" fix: the #316 endpoint check (#331 itself), then ParseProtocol +// beside it, then the config BINDING beside that, then all of +// RateLimitingOptions.Validate, then the eager Jwt:PublicKeyPem requirement in a +// file no round had opened. Every one of them was a registration-time throw for +// machinery that only a SERVING process uses, and every one of them would have +// been caught here on the day it was written. +// +// So the rule this encodes is deliberately blunt: a verb gets a connection +// string and its own arguments. Anything else the process demands is a serving +// concern leaking into an operational escape hatch — and `recover-admin` is the +// break-glass path for a farm that is already broken, which is exactly when the +// rest of the configuration is missing. +public sealed class OneShotVerbMinimalConfigTests(ServingGuardDatabaseFixture database) + : IClassFixture +{ + private readonly ServingGuardDatabaseFixture _database = database; + private static readonly string ApiDllPath = typeof(Program).Assembly.Location; + private static readonly TimeSpan VerbTimeout = TimeSpan.FromSeconds(120); + + // Only these are inherited. The child gets no ambient application config at + // all, which is the whole point — an inherited Jwt__* or Otlp__* would hide + // exactly the defect under test. + private static readonly string[] InheritedOsVariables = + ["PATH", "HOME", "DOTNET_ROOT", "TMPDIR", "LANG", "LC_ALL", "USER"]; + + private Process Start(string verbAndArgs, string environment) + { + var psi = new ProcessStartInfo("dotnet", $"\"{ApiDllPath}\" {verbAndArgs}") + { + RedirectStandardOutput = true, + RedirectStandardError = true, + UseShellExecute = false, + }; + + psi.Environment.Clear(); + foreach (var name in InheritedOsVariables) + if (Environment.GetEnvironmentVariable(name) is { } value) + psi.Environment[name] = value; + + psi.Environment["ASPNETCORE_ENVIRONMENT"] = environment; + // The database and the acknowledgement that the test container is + // plaintext. The #261/#262 TLS floor applies to BOTH roles by design, so + // opting out of it is not a serving concern leaking in. + psi.Environment["ConnectionStrings__Default"] = _database.ConnectionString; + psi.Environment["Database__Provider"] = "Postgres"; + psi.Environment["Database__AllowInsecureConnection"] = "true"; + + return Process.Start(psi)!; + } + + public static TheoryData EveryOneShotVerb() => new() + { + // Production on purpose: these run against real production databases. + { "migrate", "Production" }, + { "recover-admin --email nobody@example.test", "Production" }, + { "bootstrap-admin --email owner@example.test", "Production" }, + // The seed profiles are deliberately blocked in Production (#280), so + // their minimal environment is a non-Production one. + { "seed --profile demo", "Testing" }, + }; + + [Theory] + [MemberData(nameof(EveryOneShotVerb))] + public async Task EveryOneShotVerb_RunsWithOnlyTheConfigurationItNeeds(string verbAndArgs, string environment) + { + var (exitCode, stdout, stderr) = await SeedCommandRunner.RunToCompletionAsync( + Start(verbAndArgs, environment), + VerbTimeout, + $"`{verbAndArgs}` did not exit — a one-shot verb must never start the web host."); + + // The verb's OWN failures are fine and are covered by its own suite: an + // unknown email is a clean exit 1 with a message. What must never happen + // is a crash out of service registration, which is what every instance of + // this bug class looked like — an unhandled InvalidOperationException, + // exit 134, before the verb's code ran at all. + Assert.False( + stderr.Contains("Unhandled exception", StringComparison.Ordinal), + $"`{verbAndArgs}` crashed instead of running. Some serving-only configuration is " + + "required of a one-shot verb — that is the #331 class, and this is the fifth " + + $"variant of it. exit={exitCode}\nstderr={stderr}"); + + Assert.True( + exitCode is 0 or 1, + $"`{verbAndArgs}` exited {exitCode}; a one-shot verb exits 0 on success or 1 on its " + + $"own clean failure, never a crash. stdout={stdout}\nstderr={stderr}"); + } +} diff --git a/tests/Cluckwork.Api.IntegrationTests/ProcessRoleGuardTests.cs b/tests/Cluckwork.Api.IntegrationTests/ProcessRoleGuardTests.cs index e9adfe534..fd5623868 100644 --- a/tests/Cluckwork.Api.IntegrationTests/ProcessRoleGuardTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/ProcessRoleGuardTests.cs @@ -87,7 +87,16 @@ private sealed record ServingGuard( // credentials in transit. Satisfied by acknowledging the plaintext peer, // which keeps the endpoint set so the same code path still runs. new("#316 endpoint", "Otlp:Endpoint", - Violate: psi => psi.Environment["Otlp__Endpoint"] = "http://collector.invalid:4317", + Violate: psi => + { + psi.Environment["Otlp__Endpoint"] = "http://collector.invalid:4317"; + // Must REMOVE, not merely not-set: the "#316 binding" row below + // keys on the same setting and its Satisfy runs first, so leaving + // it would acknowledge the plaintext endpoint and un-violate this + // row. Second instance of that hazard in this table; the first is + // the TrustedProxies pair. + psi.Environment.Remove("Otlp__AllowInsecureEndpoint"); + }, Satisfy: psi => { psi.Environment["Otlp__Endpoint"] = "http://collector.invalid:4317"; @@ -105,6 +114,20 @@ private sealed record ServingGuard( Violate: psi => psi.Environment["Otlp__Protocol"] = "not-a-protocol", Satisfy: psi => psi.Environment["Otlp__Protocol"] = "grpc"), + // A THIRD violation of the same subsystem: the config BINDING, which + // throws from Get() before any validator runs. Rows are per + // violation and not per subsystem precisely because each of these three + // was, at some point in this PR's review, outside the role boundary while + // the others were inside it. + // Token includes the binder's quoting (`at 'Otlp:…'`) on purpose: the + // plain key name is NOT discriminating, because the #316 endpoint guard's + // own message tells the operator to "set Otlp:AllowInsecureEndpoint=true". + // The first attempt used the bare key and the endpoint arm went red on the + // negative assertion — which is the assertion earning its place. + new("#316 binding", "at 'Otlp:AllowInsecureEndpoint'", + Violate: psi => psi.Environment["Otlp__AllowInsecureEndpoint"] = "not-a-boolean", + Satisfy: psi => psi.Environment["Otlp__AllowInsecureEndpoint"] = "true"), + // #260 — an empty trusted-proxy list silently makes HSTS inert and // collapses the per-IP login limiter to one global bucket. new("#260 empty", "RateLimiting:TrustedProxies is empty", @@ -123,8 +146,28 @@ private sealed record ServingGuard( Violate: psi => psi.Environment["RateLimiting__TrustedProxies__0"] = "not-a-cidr", Satisfy: psi => psi.Environment["RateLimiting__TrustedProxies__0"] = "10.0.0.0/8"), - // The rest of RateLimitingOptions.Validate, same story — inbound-HTTP - // machinery that a run-then-exit verb never serves. + // The config BINDING for this section, which throws from Get() before + // any validator runs — the same third-violation shape as "#316 binding". + new("rate-limit binding", "RateLimiting:Login:PermitLimit' to type", + Violate: psi => psi.Environment["RateLimiting__Login__PermitLimit"] = "not-a-number", + Satisfy: psi => psi.Environment["RateLimiting__Login__PermitLimit"] = "5"), + + // The rest of RateLimitingOptions.Validate, one row per CHECK rather than + // one per helper: ValidateWindow and ValidateConcurrency each enforce two + // different things, and a row per helper would leave one of each pair + // authoritative for nothing. + new("window permits", "RateLimiting:Login:PermitLimit must be greater than 0", + Violate: psi => psi.Environment["RateLimiting__Login__PermitLimit"] = "0", + Satisfy: psi => psi.Environment["RateLimiting__Login__PermitLimit"] = "5"), + + new("window seconds", "RateLimiting:Login:WindowSeconds must be greater than 0", + Violate: psi => psi.Environment["RateLimiting__Login__WindowSeconds"] = "0", + Satisfy: psi => psi.Environment["RateLimiting__Login__WindowSeconds"] = "60"), + + new("#311 permits", "RateLimiting:ReportsConcurrency:PermitLimit must be greater than 0", + Violate: psi => psi.Environment["RateLimiting__ReportsConcurrency__PermitLimit"] = "0", + Satisfy: psi => psi.Environment["RateLimiting__ReportsConcurrency__PermitLimit"] = "2"), + new("#311 queue", "ReportsConcurrency:QueueLimit must be 0", Violate: psi => psi.Environment["RateLimiting__ReportsConcurrency__QueueLimit"] = "5", Satisfy: psi => psi.Environment["RateLimiting__ReportsConcurrency__QueueLimit"] = "0"), diff --git a/tests/Cluckwork.Api.IntegrationTests/ServingGuardCoverageTests.cs b/tests/Cluckwork.Api.IntegrationTests/ServingGuardCoverageTests.cs index 37c713ec4..e3d7398ac 100644 --- a/tests/Cluckwork.Api.IntegrationTests/ServingGuardCoverageTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/ServingGuardCoverageTests.cs @@ -79,14 +79,23 @@ public void EveryRateLimitingValidation_HasACoveringRow() .Concat([nameof(RateLimitingOptions.ParseTrustedProxies)]) .ToArray(); - var covered = new Dictionary(StringComparer.Ordinal) + // One entry per CHECK, not per helper: ValidateWindow and + // ValidateConcurrency each enforce two different things, and mapping a + // helper to a single token left one of each pair authoritative for + // nothing (#347 review round 3, codex). + var covered = new Dictionary(StringComparer.Ordinal) { - // ValidateWindow guards Login/Refresh/ClientErrors permit+window; one - // row stands for the family because they share a single code path and - // a single message shape. - ["ValidateWindow"] = "PermitLimit must be greater than 0", - ["ValidateConcurrency"] = "ReportsConcurrency:QueueLimit must be 0", - ["ParseTrustedProxies"] = "is not a valid CIDR network", + ["ValidateWindow"] = + [ + "RateLimiting:Login:PermitLimit must be greater than 0", + "RateLimiting:Login:WindowSeconds must be greater than 0", + ], + ["ValidateConcurrency"] = + [ + "RateLimiting:ReportsConcurrency:PermitLimit must be greater than 0", + "ReportsConcurrency:QueueLimit must be 0", + ], + ["ParseTrustedProxies"] = ["is not a valid CIDR network"], }; Assert.NotEmpty(validationMethods); @@ -104,6 +113,13 @@ public void EveryRateLimitingValidation_HasACoveringRow() stale.Length == 0, $"this mapping names RateLimitingOptions method(s) that no longer exist: " + $"{string.Join(", ", stale)}. Was one renamed or removed?"); + + // The half this test was missing: without it, deleting the malformed-CIDR + // or queue-limit ROW removed both of its subprocess arms while this stayed + // green, because the reflected methods still satisfied everything above + // (#347 review round 3, codex). + foreach (var token in covered.Values.SelectMany(t => t)) + Assert.Contains(token, ProcessRoleGuardTests.CoveredGuardTokens); } // 3. Guards that are serving-only by MECHANISM rather than by a role check: @@ -141,6 +157,39 @@ public void EveryValidateOnStartOption_HasACoveringRow() "these options types are validated at host start — so they fail a serving boot and " + "are skipped for one-shot verbs — but have no row in " + $"ProcessRoleGuardTests.ServingOnlyGuards: {string.Join(", ", uncovered)}. " - + "Add a row so arm 1 proves the verbs survive them."); + + "Add a row so the one-shot arms prove the verbs survive them."); + } + + // 4. …and the same question asked of the WHOLE source tree, because the + // enumeration above only executes AddCluckworkFeatures. A `.ValidateOnStart()` + // added tomorrow to persistence, identity, jobs or anywhere else would be a + // new serving-only guard that test cannot see, and `Assert.NotEmpty` would + // stay satisfied by the two existing validators (#347 review round 3, codex). + // + // Deliberately a file-level sweep rather than a cleverer parse: it needs to + // answer "did a new registration site appear", and the boring version of + // that cannot quietly under-report. + [Fact] + public void NoValidateOnStartRegistrationOutsideTheEnumeratedExtension() + { + var sourceRoot = Path.Combine(RepositoryRoot(), "src"); + var sites = Directory + .EnumerateFiles(sourceRoot, "*.cs", SearchOption.AllDirectories) + .Where(f => File.ReadAllText(f).Contains(".ValidateOnStart()", StringComparison.Ordinal)) + .Select(f => Path.GetFileName(f)) + .OrderBy(f => f, StringComparer.Ordinal) + .ToArray(); + + Assert.Equal(["CluckworkFeatureServiceCollectionExtensions.cs"], sites); + } + + private static string RepositoryRoot() + { + var dir = new DirectoryInfo(AppContext.BaseDirectory); + while (dir is not null && !File.Exists(Path.Combine(dir.FullName, "Cluckwork.sln"))) + dir = dir.Parent; + + Assert.NotNull(dir); + return dir!.FullName; } } From cd9302b4502080becfbed642598e0b5507382442 Mon Sep 17 00:00:00 2001 From: mforce <> Date: Mon, 10 Aug 2026 19:49:08 -0700 Subject: [PATCH 7/9] test(api): probe malformed serving-only config, not just its absence A mutation run refuted a claim this PR had already written into the source. OneShotVerbMinimalConfigTests said all five instances of the #331 bug class "would have been caught here"; hoisting either config binding back outside its role-checked try kills the ProcessRoleGuardTests arms but leaves every one of its cases GREEN. The reason is structural, not a gap to patch. A boot guard fires on one of two inputs: ABSENT config #260's empty TrustedProxies, #319's missing AllowedHosts. Caught by minimal config, which supplies nothing. MALFORMED config a bad protocol, an unparseable CIDR, a non-numeric PermitLimit, a non-boolean AllowInsecureEndpoint. Minimal config is well-formed by construction, so binding and validation both succeed and the guard never runs. Four of the five instances are the malformed shape, so minimal config alone would have caught one. Overstating the newer, structurally stronger test is worse than a plain gap: adding it was meant to stop relying on the table, and a reader who believes the claim stops adding rows. NoServingOnlySection_CanAbortAVerbByBeingUnparseable is the other half. One unparseable value per serving-only section, one section per case so no arm hides behind another's throw (the dead-disjunct defect is available here too). The value breaks the BINDING rather than a validation rule, because binding is the outer of the two and that is exactly the shape the last round found. The section set is walked, not listed: every type with a SectionName constant in either assembly must be probed or excluded with a stated reason. Two are excluded -- Database:Resilience (every verb opens the same database, so its retry settings are the verb's own configuration) and Simulation (seed --profile simulation's own input, where a broken value MUST fail the verb). healthcheck joins the verb list: not an ICliCommand, so it is the one name ProcessRoles writes by hand, which is how it came to be classified as a serving process in the first place. Mutation evidence, re-run against the named test rather than the suite: baseline green OTLP Get() outside the try red (arms + Otlp case) rate-limiting Get() outside it red (arms + RateLimiting case) serving guards ignore role red (9 of 12 cases) restored green No hostile section is a live bug: everything but Otlp and RateLimiting binds lazily through Configure/Bind, so it cannot throw at registration. Full suite 1574 passed, 0 failed. --- docs/decisions/347-process-role.md | 23 ++- .../OneShotVerbMinimalConfigTests.cs | 160 ++++++++++++++++-- 2 files changed, 171 insertions(+), 12 deletions(-) diff --git a/docs/decisions/347-process-role.md b/docs/decisions/347-process-role.md index 7d7d134df..238631582 100644 --- a/docs/decisions/347-process-role.md +++ b/docs/decisions/347-process-role.md @@ -67,6 +67,21 @@ Both directions are per-row, which is not symmetry for its own sake: two rows ke **And the table itself is enforced, because it was not.** Deleting the #319 row deleted its arms and the suite went from green to green — so the v2 survivor was re-openable by editing the *test*, and any guard added later was covered by nothing. "Adding a guard is a row" was an invariant living in a comment, which is a bug unless a line enforces it. `ServingGuardCoverageTests` now reflects over the three places a serving-only guard can be added — `ServingBootGuards`' check methods, `RateLimitingOptions`' validation methods, and every `IValidateOptions<>` registered with `.ValidateOnStart()` — and fails when one has no row, in both directions so a rename cannot leave a stale entry claiming coverage. +### Then stop enumerating: assert the property + +The table above is still a list, and it kept being one entry short. Five instances of this bug class were found across four review rounds — the #316 endpoint (#331 itself), `ParseProtocol` beside it, the OTLP config **binding** beside that, all of `RateLimitingOptions.Validate`, and its binding — **each found immediately after the previous "scope that subsystem" fix**. Five is not five unlucky misses; it is the method failing, and the answer is not a sixth row. + +`OneShotVerbMinimalConfigTests` asserts the property instead: **a verb gets a connection string and its own arguments, and nothing else may stop it.** The child environment is built from scratch, so no ambient `Otlp__*`/`Jwt__*` can mask the defect. + +**A guard fires on one of two inputs, and one test catches only one of them.** This was nearly shipped overstated — the file first claimed all five instances "would have been caught here", and a mutation run refuted it: + +- **ABSENT config** — #260's empty `TrustedProxies`, #319's missing `AllowedHosts`. Caught by the minimal-config arms, which supply nothing. +- **MALFORMED config** — a bad protocol, an unparseable CIDR, a non-numeric `PermitLimit`, a non-boolean `AllowInsecureEndpoint`. Minimal config is **well-formed by construction**, so binding and validation both succeed and the guard never runs. Hoisting either binding back outside its `try` kills the table's arms and leaves every minimal-config case **green**. + +Four of the five instances are the malformed shape, so minimal config alone would have caught **one**. `NoServingOnlySection_CanAbortAVerbByBeingUnparseable` is the other half: one deliberately unparseable value per serving-only section, **one section per case** so no arm hides behind another's throw — the dead-disjunct defect is available here too. The value breaks the **binding** rather than a validation rule, because binding is the outer of the two and the round that found the binding bugs found exactly that shape: validation correctly role-scoped while `Get()` was not. + +The section set is **walked, not listed** — every type with a `SectionName` constant in either assembly must be probed or excluded with a stated reason. Two are excluded: `Database:Resilience` (every verb opens the same database, so its retry settings are the verb's own configuration — the one section eagerly bound for both roles on purpose) and `Simulation` (it is `seed --profile simulation`'s own input, where a broken value **must** fail the verb). + The same "list what I thought of" method had also sprung three leaks in how the child process's environment was built. The subprocess inherits the test host's environment and **#260's condition is the ABSENCE of `RateLimiting__*`**, so v2 stripped that prefix — but the strip was `Ordinal` while Linux environment keys are case-sensitive and .NET configuration keys are not (`ratelimiting__…` survived it), `ASPNETCORE_`- and `DOTNET_`-prefixed variables bind to the same configuration keys with the prefix removed, and `Otlp__` was never stripped at all — so an inherited `Otlp__AllowInsecureEndpoint` (a documented sim-harness setting) would have silently voided the #316 arm. The child environment is now **built from scratch**: cleared, then a small allow-list of OS variables, then every application setting stated explicitly. One smaller thing from the same review, same family: `SeedCommandRunner`'s timeout message asserted the seed suites' regression ("falling through into `app.Run()`"), which for the serving arms means the exact opposite — a timeout there means the boot *succeeded* — so the message is now per-caller. @@ -80,7 +95,7 @@ Mutation evidence, run before the claim: | Mutation | Result | |---|---| | baseline, unmutated | **green** | -| `EnsureServingConfiguration` drops its `role` check | **red** (`migrate` aborts on #260) | +| `EnsureServingConfiguration` drops its `role` check | **red** (`migrate` aborts on #260; 9 of 12 property cases) | | #316's degrade filter flipped to `Serving` | **red** — #331 verbatim | | `healthcheck` removed from `OneShotVerbs` | **red** (registry suite) | | the #319 `AllowedHosts` guard deleted outright | **red** — *survived v2 entirely* | @@ -88,13 +103,17 @@ Mutation evidence, run before the claim: | rate-limiting validation's role scope reverted | **red** — the live bug above | | a row deleted from the table | **red** — *was silent before the coverage suite* | | `ParseProtocol` moved back outside the role-checked `try` | **red** — the other live bug above | +| OTLP `Get()` moved back outside the `try` | **red** — table arms **and** the `Otlp` hostile case | +| rate-limiting `Get()` moved back outside it | **red** — table arms **and** the `RateLimiting` hostile case | | `healthcheck`'s early return removed from `Program.cs` | **red** (the assertion fires, exit 134) | | restored | **green** | The marked rows are the ones that matter. The two "survived v2" mutants are how the dead disjuncts were found and are the regression check on the per-guard table. The "row deleted" mutant is the regression check on the coverage suite — before it, deleting a row was green. -Two process notes, because each nearly produced false evidence. +Three process notes, because each nearly produced false evidence. The first mutation script restored with `git checkout --`, which **silently refuses to restore untracked files**. Two of the three targets were new files, so mutants stacked instead of reverting and two "red" results were measured against an already-mutated tree. Snapshot by file copy, and assert the restored tree is green again. And a mutant being red is not the same as being red *for the stated reason*. Each mutant is re-checked against the specific arm it should redden: the two single-guard rows must fail their own guard's arm — the serving process otherwise boots clean, logging `OTLP export enabled` — and not the one-shot arm, which is the whole claim behind deriving an arm per guard. + +Third, and the one that came closest to shipping: **a mutation run refuted a claim already written into the source.** The property test's own header said all five instances "would have been caught here"; the two binding mutants were red overall but left every one of its cases green, because the enumerated arms were doing the killing. Overstating the newer, structurally stronger test is worse than a plain gap — the whole point of adding it was to stop relying on the list, and a reader who believes that claim stops adding rows. It is why the two-shapes distinction above exists at all, and why the hostile-section arms were written. **Run the mutation against the specific test the claim names, not against the suite.** diff --git a/tests/Cluckwork.Api.IntegrationTests/OneShotVerbMinimalConfigTests.cs b/tests/Cluckwork.Api.IntegrationTests/OneShotVerbMinimalConfigTests.cs index 4eeaac32e..b91dad315 100644 --- a/tests/Cluckwork.Api.IntegrationTests/OneShotVerbMinimalConfigTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/OneShotVerbMinimalConfigTests.cs @@ -1,6 +1,7 @@ namespace Cluckwork.Api.IntegrationTests; using System.Diagnostics; +using System.Reflection; using Cluckwork.Api.IntegrationTests.Infrastructure; // #347 — every one-shot verb must run under a Production environment containing @@ -15,16 +16,35 @@ namespace Cluckwork.Api.IntegrationTests; // were found across four review rounds, each after the previous "scope that // subsystem" fix: the #316 endpoint check (#331 itself), then ParseProtocol // beside it, then the config BINDING beside that, then all of -// RateLimitingOptions.Validate, then the eager Jwt:PublicKeyPem requirement in a -// file no round had opened. Every one of them was a registration-time throw for -// machinery that only a SERVING process uses, and every one of them would have -// been caught here on the day it was written. +// RateLimitingOptions.Validate, then `healthcheck` classified as a serving +// process. Every one was a registration-time throw for machinery only a SERVING +// process uses. // -// So the rule this encodes is deliberately blunt: a verb gets a connection -// string and its own arguments. Anything else the process demands is a serving -// concern leaking into an operational escape hatch — and `recover-admin` is the -// break-glass path for a farm that is already broken, which is exactly when the -// rest of the configuration is missing. +// ── The defect has TWO shapes, and one test catches only one of them ──────── +// +// This was nearly shipped overstated. The first version of this file claimed all +// five instances "would have been caught here", and a mutation run refuted it: +// hoisting either config binding back outside its role-checked try kills the +// ProcessRoleGuardTests arms but leaves EVERY case here green. The reason is +// structural, not a gap to patch — a guard fires on one of two inputs: +// +// ABSENT config — #260's empty TrustedProxies, #319's missing AllowedHosts. +// Caught below by MinimalConfig, which supplies nothing. +// MALFORMED config — a bad protocol, an unparseable CIDR, a non-numeric +// PermitLimit, a non-boolean AllowInsecureEndpoint. Minimal +// config is *well-formed by construction*, so binding and +// validation both succeed and the guard never runs. +// +// Four of the five instances are the malformed shape. So MinimalConfig alone +// would have caught one of them, and claiming otherwise made the weaker of the +// two tests read as the stronger. HostileServingOnlySection is the other half: +// one deliberately broken value per serving-only config section, isolated one +// section per case so no arm can hide behind another's throw. +// +// The rule the pair encodes: a verb gets a connection string and its own +// arguments, and nothing in a section it does not consume may stop it — +// `recover-admin` is the break-glass path for a farm that is already broken, +// which is exactly when the rest of the configuration is missing or wrong. public sealed class OneShotVerbMinimalConfigTests(ServingGuardDatabaseFixture database) : IClassFixture { @@ -38,7 +58,10 @@ public sealed class OneShotVerbMinimalConfigTests(ServingGuardDatabaseFixture da private static readonly string[] InheritedOsVariables = ["PATH", "HOME", "DOTNET_ROOT", "TMPDIR", "LANG", "LC_ALL", "USER"]; - private Process Start(string verbAndArgs, string environment) + private Process Start(string verbAndArgs, string environment) => + Start(verbAndArgs, environment, _ => { }); + + private Process Start(string verbAndArgs, string environment, Action configure) { var psi = new ProcessStartInfo("dotnet", $"\"{ApiDllPath}\" {verbAndArgs}") { @@ -59,6 +82,7 @@ private Process Start(string verbAndArgs, string environment) psi.Environment["ConnectionStrings__Default"] = _database.ConnectionString; psi.Environment["Database__Provider"] = "Postgres"; psi.Environment["Database__AllowInsecureConnection"] = "true"; + configure(psi); return Process.Start(psi)!; } @@ -72,6 +96,12 @@ private Process Start(string verbAndArgs, string environment) // The seed profiles are deliberately blocked in Production (#280), so // their minimal environment is a non-Production one. { "seed --profile demo", "Testing" }, + // Not an ICliCommand, and therefore the one verb ProcessRoles has to name + // by hand — which is exactly how it came to be classified as a SERVING + // process before this PR. Nothing here reads configuration (the verb + // returns before the host is built), so its value is the regression, not + // the config coverage. + { "healthcheck", "Production" }, }; [Theory] @@ -99,4 +129,114 @@ public async Task EveryOneShotVerb_RunsWithOnlyTheConfigurationItNeeds(string ve $"`{verbAndArgs}` exited {exitCode}; a one-shot verb exits 0 on success or 1 on its " + $"own clean failure, never a crash. stdout={stdout}\nstderr={stderr}"); } + + // ── The malformed half ──────────────────────────────────────────────────── + // + // One broken value per serving-only section, each in its OWN case. Isolation + // is the point: applying them together would let the first throw hide every + // section behind it, which is the dead-disjunct defect ProcessRoleGuardTests + // was rewritten twice to remove — the same mistake is available here. + // + // The value breaks the BINDING (a type mismatch), not a validation rule, + // because binding is the outer of the two and the round that found bugs 3 + // and 4 found exactly that: a subsystem whose *validation* was correctly + // role-scoped while its Get() was not. A section that survives a value it + // cannot even parse has its whole configuration inside the boundary. + public static TheoryData HostileServingOnlySections() + { + var data = new TheoryData(); + foreach (var (section, key, value) in HostileValues) + data.Add(section, key, value); + return data; + } + + // Keyed by the section's own SectionName constant so EverySection_IsCovered + // can hold this against the real set rather than a remembered one. + private static readonly (string Section, string EnvKey, string Value)[] HostileValues = + [ + ("Otlp", "Otlp__AllowInsecureEndpoint", "not-a-bool"), + ("RateLimiting", "RateLimiting__Login__PermitLimit", "not-a-number"), + ("Idempotency", "Idempotency__LeaseDurationSeconds", "not-a-number"), + ("FarmLogo", "FarmLogo__MaxUploadBytes", "not-a-number"), + ("FarmBanner", "FarmBanner__MaxUploadBytes", "not-a-number"), + ("Jwt", "Jwt__AccessTokenMinutes", "not-a-number"), + ]; + + // Deliberate exclusions, with the reason each is NOT serving-only. A section + // is excluded because a one-shot verb genuinely consumes it — never because + // an arm was inconvenient. + private static readonly Dictionary BothRoleSections = new(StringComparer.Ordinal) + { + ["Database:Resilience"] = + "every verb opens the same database, so its retry settings are the verb's own " + + "configuration; this is the one section bound eagerly for BOTH roles on purpose", + ["Simulation"] = + "it is `seed --profile simulation`'s own input — a broken value there must fail that " + + "verb, which is the opposite of what these arms assert", + }; + + [Theory] + [MemberData(nameof(HostileServingOnlySections))] + public async Task NoServingOnlySection_CanAbortAVerbByBeingUnparseable( + string section, string envKey, string value) + { + // recover-admin, not migrate: it is the break-glass verb #331 actually + // took out, it needs no schema, and its own failure here (no such user) + // is a clean exit 1 that the assertions below deliberately tolerate. + var (exitCode, stdout, stderr) = await SeedCommandRunner.RunToCompletionAsync( + Start("recover-admin --email nobody@example.test", "Production", + psi => psi.Environment[envKey] = value), + VerbTimeout, + $"`recover-admin` did not exit with a broken {section} section."); + + Assert.False( + stderr.Contains("Unhandled exception", StringComparison.Ordinal), + $"a broken `{envKey}` aborted `recover-admin`. The {section} section is serving-only " + + "machinery, so everything that can reject it — binding included — belongs inside a " + + $"role-checked boundary. That is the #331 class. exit={exitCode}\nstderr={stderr}"); + + Assert.True( + exitCode is 0 or 1, + $"`recover-admin` exited {exitCode} with a broken {section} section. " + + $"stdout={stdout}\nstderr={stderr}"); + } + + // Walk every config section that exists and require a decision about it — + // covered above, or excluded with a stated reason. AGENTS.md: prefer "walk + // everything, exclude deliberately" over "list what I thought of". A new + // options type added tomorrow fails here until someone classifies it. + [Fact] + public void EveryConfigSection_IsEitherProbedOrDeliberatelyExcluded() + { + var sections = new[] { typeof(Program).Assembly, typeof(Cluckwork.Infrastructure.Identity.JwtOptions).Assembly } + .SelectMany(a => a.GetTypes()) + .Select(t => t.GetField("SectionName", BindingFlags.Public | BindingFlags.Static)) + .Where(f => f is { IsLiteral: true, FieldType: { } ft } && ft == typeof(string)) + .Select(f => (string)f!.GetRawConstantValue()!) + .Distinct(StringComparer.Ordinal) + .OrderBy(s => s, StringComparer.Ordinal) + .ToArray(); + + Assert.NotEmpty(sections); + + var probed = HostileValues.Select(v => v.Section).ToHashSet(StringComparer.Ordinal); + var unclassified = sections + .Where(s => !probed.Contains(s) && !BothRoleSections.ContainsKey(s)) + .ToArray(); + + Assert.True( + unclassified.Length == 0, + $"config section(s) with no decision: {string.Join(", ", unclassified)}. Either add a " + + "hostile value proving a one-shot verb survives the section being unparseable, or " + + "add it to BothRoleSections with the reason a verb genuinely consumes it."); + + // Both directions, so a renamed or deleted section cannot leave an entry + // claiming coverage it no longer provides. + var stale = probed.Concat(BothRoleSections.Keys) + .Where(s => !sections.Contains(s, StringComparer.Ordinal)) + .ToArray(); + Assert.True( + stale.Length == 0, + $"these names match no SectionName constant: {string.Join(", ", stale)}. Renamed?"); + } } From 098bdd0ddea3963677b6de0ca3816bbdcba0d0e0 Mon Sep 17 00:00:00 2001 From: mforce <> Date: Mon, 10 Aug 2026 20:06:39 -0700 Subject: [PATCH 8/9] test(api): one row per validator branch, and derive the verb cases Round 5 review. Five findings, no live bug among them -- the first round of this PR to find none. Four are real test gaps and are fixed; one is rejected with evidence. Upload-cap ceilings (codex). Both FarmLogoOptionsValidator and FarmBannerOptionsValidator have a floor branch (<= 0) and a distinct ceiling branch (> the domain constant), and the table violated only the floor -- so deleting either ceiling check was green. That is the one-row-per-violation rule broken inside the table that states it. Each cap is now two rows, and the ceiling token carries the violated value so the pair cannot satisfy each other's negative assertion. ServingGuardCoverageTests counted rows per TYPE, which is what let that through: "is this validator represented at all" is satisfied by one row no matter how many ways the validator can fail. It now counts ValidateOptionsResult.Fail sites in the validator's source and requires at least that many rows -- a boring source scan, because reflection cannot see branches and would have to guess. Verb cases derived (codex). EveryOneShotVerb was hand-written, so a new ICliCommand joins ProcessRoles.OneShotVerbs automatically and would be exercised by nothing here. Held against the registry in both directions. Production seed arm (codex). Testing-only bypassed every Production-gated guard, so a new guard checking IsProduction() without respecting ProcessRole would abort seed before it reached its own #280 refusal, with every row still green. ValidateOnStart sweep (codex). The literal `.ValidateOnStart()` match missed a spaced, line-broken, or statically-invoked registration. A sweep whose whole job is "did a registration appear anywhere" must not be defeated by formatting; it is now a whitespace-tolerant regex that does not require the leading dot. REJECTED: Database:Resilience is not a live leak. Codex argued the exclusion blesses one, since Production `seed --profile demo` refuses before resolving AppDbContext. Malformed resilience config does abort every verb with exit 134 -- but so does an intentional TLS-floor violation (#261/#262), which is a documented both-roles guard failing closed. Exit 134 on a both-roles section is this repo's convention, not a leak, and migrate/recover-admin genuinely execute against the configured retry policy. The alternative was priced before rejecting it: degrading to defaults would silently run migrations under a retry policy the operator did not configure, which is worse than failing. Mutation evidence, each against the test whose claim it names: baseline green FarmLogo ceiling check deleted red (arms) FarmLogo ceiling ROW deleted red (coverage) bootstrap-admin case deleted red (verb registry) spaced ValidateOnStart outside the extension red (sweep) restored green Full suite 1580 passed, 0 failed. --- .../OneShotVerbMinimalConfigTests.cs | 39 +++++++++++- .../ProcessRoleGuardTests.cs | 21 ++++++- .../ServingGuardCoverageTests.cs | 59 +++++++++++++++---- 3 files changed, 102 insertions(+), 17 deletions(-) diff --git a/tests/Cluckwork.Api.IntegrationTests/OneShotVerbMinimalConfigTests.cs b/tests/Cluckwork.Api.IntegrationTests/OneShotVerbMinimalConfigTests.cs index b91dad315..9a01f82c2 100644 --- a/tests/Cluckwork.Api.IntegrationTests/OneShotVerbMinimalConfigTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/OneShotVerbMinimalConfigTests.cs @@ -2,6 +2,7 @@ namespace Cluckwork.Api.IntegrationTests; using System.Diagnostics; using System.Reflection; +using Cluckwork.Api.Hosting; using Cluckwork.Api.IntegrationTests.Infrastructure; // #347 — every one-shot verb must run under a Production environment containing @@ -93,9 +94,15 @@ private Process Start(string verbAndArgs, string environment, Action ((string)row[0]).Split(' ')[0]) + .ToHashSet(StringComparer.Ordinal); + + var uncovered = ProcessRoles.OneShotVerbs.Where(v => !tested.Contains(v)).ToArray(); + + Assert.True( + uncovered.Length == 0, + $"verb(s) classified OneShot but never run here: {string.Join(", ", uncovered)}. " + + "Add a case with the minimal configuration that verb genuinely needs — a verb no " + + "arm executes is covered by nothing."); + + // The other direction: a case naming a verb the dispatcher no longer + // classifies is testing a command that cannot be reached. + var stale = tested.Where(v => !ProcessRoles.OneShotVerbs.Contains(v)).ToArray(); + Assert.True( + stale.Length == 0, + $"case(s) name verb(s) absent from ProcessRoles.OneShotVerbs: {string.Join(", ", stale)}."); + } + // ── The malformed half ──────────────────────────────────────────────────── // // One broken value per serving-only section, each in its OWN case. Isolation diff --git a/tests/Cluckwork.Api.IntegrationTests/ProcessRoleGuardTests.cs b/tests/Cluckwork.Api.IntegrationTests/ProcessRoleGuardTests.cs index fd5623868..08c12e1a7 100644 --- a/tests/Cluckwork.Api.IntegrationTests/ProcessRoleGuardTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/ProcessRoleGuardTests.cs @@ -188,13 +188,30 @@ private sealed record ServingGuard( // The first derivation missed both because it walked `IsProduction` and // explicit `throw` sites, which is "list what I thought of" one // abstraction level up. ServingGuardCoverageTests now enumerates them. - new("FarmLogo", "FarmLogo:MaxUploadBytes", + // Each cap is TWO rows, not one. Both validators have a floor branch + // (<= 0) and a distinct CEILING branch (> the domain constant), and the + // first version violated only the floor — so deleting either ceiling + // check left every arm green. That is the one-row-per-violation rule + // broken inside the table that states it (#347 review round 5, codex). + // The ceiling token carries the violated VALUE, so the two rows for one + // setting cannot satisfy each other's negative assertion. + new("FarmLogo floor", "FarmLogo:MaxUploadBytes must be greater than zero", Violate: psi => psi.Environment["FarmLogo__MaxUploadBytes"] = "0", Satisfy: psi => psi.Environment["FarmLogo__MaxUploadBytes"] = "2097152"), - new("FarmBanner", "FarmBanner:MaxUploadBytes", + // ImageSanitizer.MaxByteLengthCeiling is 5 MiB — one byte over it. + new("FarmLogo ceiling", "FarmLogo:MaxUploadBytes (5242881) cannot exceed", + Violate: psi => psi.Environment["FarmLogo__MaxUploadBytes"] = "5242881", + Satisfy: psi => psi.Environment["FarmLogo__MaxUploadBytes"] = "2097152"), + + new("FarmBanner floor", "FarmBanner:MaxUploadBytes must be greater than zero", Violate: psi => psi.Environment["FarmBanner__MaxUploadBytes"] = "0", Satisfy: psi => psi.Environment["FarmBanner__MaxUploadBytes"] = "5242880"), + + // ImageSanitizer.MaxBannerByteLengthCeiling is 15 MiB — one byte over it. + new("FarmBanner ceiling", "FarmBanner:MaxUploadBytes (15728641) cannot exceed", + Violate: psi => psi.Environment["FarmBanner__MaxUploadBytes"] = "15728641", + Satisfy: psi => psi.Environment["FarmBanner__MaxUploadBytes"] = "5242880"), ]; // Exposed so ServingGuardCoverageTests can hold this table against the source diff --git a/tests/Cluckwork.Api.IntegrationTests/ServingGuardCoverageTests.cs b/tests/Cluckwork.Api.IntegrationTests/ServingGuardCoverageTests.cs index e3d7398ac..7a4931b34 100644 --- a/tests/Cluckwork.Api.IntegrationTests/ServingGuardCoverageTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/ServingGuardCoverageTests.cs @@ -1,6 +1,7 @@ namespace Cluckwork.Api.IntegrationTests; using System.Reflection; +using System.Text.RegularExpressions; using Cluckwork.Api.Hosting; using Cluckwork.Api.RateLimiting; using Microsoft.Extensions.Configuration; @@ -145,19 +146,44 @@ public void EveryValidateOnStartOption_HasACoveringRow() // skipped for every one-shot verb, which is exactly the property the // table exists to pin. The row's token is the config key the validator // names, so match on the section prefix the options type is named for. - var uncovered = validatedOptionTypes - .Where(t => !ProcessRoleGuardTests.CoveredGuardTokens.Any(token => - token.StartsWith( - t.Replace("Options", string.Empty, StringComparison.Ordinal) + ":", - StringComparison.Ordinal))) - .ToArray(); + // + // Counted per BRANCH, not per type. Enumerating types only asked "is this + // validator represented at all", and both upload-cap validators have a + // floor branch AND a distinct ceiling branch while the table violated + // only the floor — so deleting either ceiling check was green (#347 + // review round 5, codex). A validator's failure count is the number of + // ValidateOptionsResult.Fail sites in its source, which is a boring + // source scan on purpose: it cannot silently under-report the way + // reflection over branches would have to guess. + foreach (var type in validatedOptionTypes) + { + var prefix = type.Replace("Options", string.Empty, StringComparison.Ordinal) + ":"; + var rows = ProcessRoleGuardTests.CoveredGuardTokens + .Count(token => token.StartsWith(prefix, StringComparison.Ordinal)); + var branches = FailureBranchCount(type); + + Assert.True( + rows >= branches, + $"{type} is validated at host start — so it fails a serving boot and is skipped " + + $"for one-shot verbs — but has {rows} row(s) in " + + $"ProcessRoleGuardTests.ServingOnlyGuards for {branches} failure branch(es). " + + "Rows are per VIOLATION, not per validator: add a row violating each branch, " + + "or a deleted check stays green."); + } + } - Assert.True( - uncovered.Length == 0, - "these options types are validated at host start — so they fail a serving boot and " - + "are skipped for one-shot verbs — but have no row in " - + $"ProcessRoleGuardTests.ServingOnlyGuards: {string.Join(", ", uncovered)}. " - + "Add a row so the one-shot arms prove the verbs survive them."); + // The validator lives beside its options type, named for it. + private static int FailureBranchCount(string optionsTypeName) + { + var file = Directory + .EnumerateFiles(Path.Combine(RepositoryRoot(), "src"), $"{optionsTypeName}.cs", + SearchOption.AllDirectories) + .SingleOrDefault(); + + Assert.NotNull(file); + return Regex.Matches( + File.ReadAllText(file!), + @"ValidateOptionsResult\s*\.\s*Fail\s*\(").Count; } // 4. …and the same question asked of the WHOLE source tree, because the @@ -169,13 +195,20 @@ public void EveryValidateOnStartOption_HasACoveringRow() // Deliberately a file-level sweep rather than a cleverer parse: it needs to // answer "did a new registration site appear", and the boring version of // that cannot quietly under-report. + // + // The match is whitespace-tolerant and does NOT require the leading dot. + // The first version looked for the literal `.ValidateOnStart()`, which a + // line break before the parentheses, a space, or a static call through + // OptionsBuilderExtensions all slip past — a sweep whose whole job is + // "did a registration appear anywhere" must not be defeated by formatting + // (#347 review round 5, codex). [Fact] public void NoValidateOnStartRegistrationOutsideTheEnumeratedExtension() { var sourceRoot = Path.Combine(RepositoryRoot(), "src"); var sites = Directory .EnumerateFiles(sourceRoot, "*.cs", SearchOption.AllDirectories) - .Where(f => File.ReadAllText(f).Contains(".ValidateOnStart()", StringComparison.Ordinal)) + .Where(f => Regex.IsMatch(File.ReadAllText(f), @"ValidateOnStart\s*\(")) .Select(f => Path.GetFileName(f)) .OrderBy(f => f, StringComparer.Ordinal) .ToArray(); From 5710a263af0b3124a6fc0e0cf62b55a5425a6108 Mon Sep 17 00:00:00 2001 From: mforce <> Date: Mon, 10 Aug 2026 20:31:53 -0700 Subject: [PATCH 9/9] fix(api): fail the boot on unusable JWT keys, scoped to the serving role Folds #510 into #347, because #510's fix DEPENDS on #347's mechanism. Two live defects, both reproduced against the real binary. The eager guard could not fire for any real deployment. It read `configuration["Jwt:PublicKeyPem"] ?? throw`, and `??` catches only NULL, while the shipped appsettings.json carries an empty string. The guard fired only when appsettings.json was absent entirely, which is not how the image runs. Now IsNullOrWhiteSpace. The key CONTENT was never checked at boot. ImportFromPem sat inside the AddJwtBearer options delegate, so a corrupt key was a per-request failure: the farm booted, /health/ready went green, the container HEALTHCHECK passed, and every authenticated request 500'd. An orchestrator saw a healthy instance that rejected every login. Both keys are now imported once at registration -- the private one is read per-request by JwtTokenService and StepUpGrantService and had exactly the same defect one file over. WHY THIS COULD NOT LAND BEFORE #347. Making the check eager without a role scope would have been a brand-new instance of the #331 class, in the most security-sensitive file of the set: recover-admin, the break-glass path for a farm that is already broken, would have started demanding a signing key it never uses. The check takes a ProcessRole and is skipped for one-shot verbs, which neither issue nor validate a token. Coverage walk widened. The guard landed in CluckworkIdentityServiceCollectionExtensions, a THIRD file, which a ServingBootGuards-only reflection could not see -- the same miss this PR has now made four times. The walk is over the whole Cluckwork.Api.Hosting namespace. Four new table rows, one per violation: each key, missing or unusable. #370: bootstrap.sh already generates a real keypair and the compose file already passes both, so the harness boots -- confirmed by running it, not assumed. verify-harness.sh gains a bounded check, because the generic unresolved-interpolation check cannot see a variable that IS set to a blank or to deploy/.env.example's `replace-me` armor. Verified in both directions: placeholder -> exit 1 naming the key, restored -> exit 0. Mutation evidence, each against the test whose claim it names: baseline green blank check reverted to null-only red (arms) eager ImportFromPem removed red (arms) key guard made role-blind, #331 reintroduced red (property test) restored green Full suite 1588 passed, 0 failed. Closes #510. --- AGENTS.md | 1 + deploy/.env.example | 7 ++ docs/decisions/347-process-role.md | 12 +++ ...workIdentityServiceCollectionExtensions.cs | 82 ++++++++++++++++++- src/Cluckwork.Api/Program.cs | 2 +- .../ProcessRoleGuardTests.cs | 27 ++++++ .../ServingGuardCoverageTests.cs | 40 ++++++--- tools/simulation/verify-harness.sh | 40 ++++++++- 8 files changed, 195 insertions(+), 16 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index e9930a957..322cc0a97 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -45,6 +45,7 @@ dotnet test Cluckwork.sln # 688 tests as of 2026-07; integratio - **Endpoints:** minimal APIs grouped under `/api/v1/...` via `MapEndpoints`; writes require auth + an `Idempotency-Key` (middleware). - **Multi-tenancy:** every tenant-owned entity has `AccountId`; enforced by an EF **global query filter** + a **`TenantStampInterceptor`** (stamps on insert). `TenantContext` is resolved per-request from the JWT `account_id` claim. At startup it's unresolved → use `IgnoreQueryFilters()` in seeders. - **Auth:** asymmetric JWT + rotating refresh tokens. PEM keys come from config with escaped `\n`; normalize via `PemKey.Normalize` before `ImportFromPem`. Integration tests generate an ephemeral RSA pair at test-process startup (`TestJwtKeys` in `CluckworkWebApplicationFactory.cs`) — no real key material is ever committed to source (`deploy/.env.example` carries PEM armor with a `replace-me` body, not a usable key). +- **Both JWT keys are checked at boot, and the check is serving-only (#510/#347).** `AddCluckworkIdentity` requires `Jwt:PublicKeyPem` **and** `Jwt:PrivateKeyPem` to be non-blank **and to actually import**; a `Serving` process refuses to start otherwise. Two traps this closes, both of which shipped: `configuration[...] ?? throw` catches **only null** while the shipped `appsettings.json` carries `""` — use **`IsNullOrWhiteSpace`**, never `??`, or the guard is decorative for every real deployment; and importing inside the `AddJwtBearer` delegate makes a corrupt key a **per-request** failure, so the farm boots green, `/health/ready` passes, the container `HEALTHCHECK` passes, and every authenticated request 500s — an orchestrator sees a healthy instance that rejects every login. The check takes a `ProcessRole` and is skipped for one-shot verbs, which neither issue nor validate a token; **making it eager without that scope is a fresh #331** in the most security-sensitive file of the set. Pinned from both sides: four rows in `ProcessRoleGuardTests` (each key × missing/unusable) plus `OneShotVerbMinimalConfigTests`, whose environment carries no `Jwt:*` at all. - **Credential epoch revocation (#364).** Every access/refresh token is bound to the user's `CredentialEpoch`; every password-reset path bumps it so issued credentials fail on next use. Epoch **`0` is permanently retired** (users start at `1`, legacy refresh rows at `0`), and a missing/malformed `credential_epoch` claim is **always a mismatch** — never default it to the DB epoch. `CredentialEpochMiddleware` does a **fresh DB read per authenticated request** (the round trip *is* the fail-closed guarantee — do not cache), sits **after `TenantResolutionMiddleware`, before `MustChangePasswordMiddleware`**, and exempts `auth/logout`. Login/step-up snapshot the verified epoch+stamp before clearing failed-access and refuse issuance on a concurrency-loss reload. Distinct from #283/#265/#308 — none substitutes for another. **Break it and a revoked credential keeps working.** → [`docs/decisions/364-credential-epoch-revocation.md`](docs/decisions/364-credential-epoch-revocation.md) - **Base reference data via guarded raw-SQL migrations (#283).** The default account, four assignable roles, default egg grades, and packed-unit conversions are **static reference data baked into the migrations** as hand-written `migrationBuilder.Sql` with `WHERE NOT EXISTS` guards — **never `HasData`/`InsertData`** (which key on the PK and would either collide or emit `UpdateData`/`DeleteData` that silently reverts the farm's own edits). No runtime seeder, no `Seed:*`. The **grades** guard is whole-set (empty catalog, because grades are user-renamable); roles/conversions/account are **per-key** (keys aren't user-mutable). Pinned by `MigrationSecurityReviewTests` + `BaseReferenceDataMigrationTests`. **Regenerate these into `HasData` and a later model-diff reverts a renamed grade.** → [`docs/decisions/283-migrations-base-provisioning.md`](docs/decisions/283-migrations-base-provisioning.md) - **Migrations: `InitialCreate` frozen, one migration per change (#407).** `Persistence/Migrations/` is one squashed `InitialCreate` (recorded id `20260801190854_InitialCreate`); it is **frozen and never regenerated**, and **every schema change gets its own `dotnet ef migrations add`**. EF never re-runs an applied migration, so a column **hand-folded into `InitialCreate` silently does not exist** on any booted DB — it surfaces as broken behaviour (a failed login, #399), not a migration error. `InitialCreate` also carries four un-regenerable `lower("Name")` expression indexes + the base-reference SQL; regenerating mints a new timestamp that desynchronises `__EFMigrationsHistory` everywhere. Pre-squash or pre-#407 dev DBs can't migrate forward — drop and recreate. `MigrationSecurityReviewTests` freezes the id + a portable operation digest. → [`docs/decisions/407-migration-freeze.md`](docs/decisions/407-migration-freeze.md) diff --git a/deploy/.env.example b/deploy/.env.example index 534edc629..ae0d3bbe4 100644 --- a/deploy/.env.example +++ b/deploy/.env.example @@ -23,6 +23,13 @@ POSTGRES_PASSWORD=replace-with-a-strong-password # Database__AllowInsecureConnection=false Jwt__Issuer=cluckwork Jwt__Audience=cluckwork-api +# BOTH keys are checked at boot, and the SERVING process refuses to start unless +# each is present and actually parses (#510). The armor below is a placeholder, +# not a key — copy this file unchanged and the API will tell you so and exit, +# rather than booting green, passing /health/ready, and 500ing every login. Keep +# the surrounding quotes and the escaped \n; that is the format PemKey.Normalize +# expects. The one-shot verbs (migrate / seed / recover-admin / bootstrap-admin / +# healthcheck) never need these — they neither issue nor validate a token (#347). Jwt__PublicKeyPem="-----BEGIN PUBLIC KEY-----\nreplace-me\n-----END PUBLIC KEY-----" Jwt__PrivateKeyPem="-----BEGIN PRIVATE KEY-----\nreplace-me\n-----END PRIVATE KEY-----" diff --git a/docs/decisions/347-process-role.md b/docs/decisions/347-process-role.md index 238631582..d10fa62c7 100644 --- a/docs/decisions/347-process-role.md +++ b/docs/decisions/347-process-role.md @@ -67,6 +67,18 @@ Both directions are per-row, which is not symmetry for its own sake: two rows ke **And the table itself is enforced, because it was not.** Deleting the #319 row deleted its arms and the suite went from green to green — so the v2 survivor was re-openable by editing the *test*, and any guard added later was covered by nothing. "Adding a guard is a row" was an invariant living in a comment, which is a bug unless a line enforces it. `ServingGuardCoverageTests` now reflects over the three places a serving-only guard can be added — `ServingBootGuards`' check methods, `RateLimitingOptions`' validation methods, and every `IValidateOptions<>` registered with `.ValidateOnStart()` — and fails when one has no row, in both directions so a rename cannot leave a stale entry claiming coverage. +### The mechanism's first consumer: #510 + +#510 is the **opposite** failure from everything above — a serving guard that does not fire *at all* — and it is folded in here because its fix **depends on this mechanism**. + +Two halves, both live. `configuration["Jwt:PublicKeyPem"] ?? throw` catches only **null**, and the shipped `appsettings.json` carries `""`, so the guard fired only when `appsettings.json` was absent entirely — which is not how the image runs. And `ImportFromPem` sat inside the `AddJwtBearer` options delegate, so a corrupt key was a **per-request** failure: the farm booted, `/health/ready` went green, the container `HEALTHCHECK` passed, and every authenticated request 500'd. An orchestrator saw a healthy instance that rejected every login. + +Both are now boot-time, fail-closed, for **both** keys — the private one is read per-request by `JwtTokenService` and `StepUpGrantService` and had exactly the same defect one file over. + +**And that is why it could not land before #347.** Making this check eager without a role scope would have made it a brand-new instance of the #331 class, in the most security-sensitive file of the set: `recover-admin` — the break-glass path — would have started demanding a signing key it never uses. It takes `ProcessRole` and is skipped for one-shot verbs. `OneShotVerbMinimalConfigTests` pins that from the other side, since its environment carries no `Jwt:*` at all; the mutation that makes the guard role-blind reddens it. + +It also moved the coverage walk: the guard landed in `CluckworkIdentityServiceCollectionExtensions`, a **third** file, which a `ServingBootGuards`-only reflection could not see. The walk is now over the whole `Cluckwork.Api.Hosting` namespace — the same "walk everything, exclude deliberately" correction, applied one more time to the place that kept producing misses. + ### Then stop enumerating: assert the property The table above is still a list, and it kept being one entry short. Five instances of this bug class were found across four review rounds — the #316 endpoint (#331 itself), `ParseProtocol` beside it, the OTLP config **binding** beside that, all of `RateLimitingOptions.Validate`, and its binding — **each found immediately after the previous "scope that subsystem" fix**. Five is not five unlucky misses; it is the method failing, and the answer is not a sixth row. diff --git a/src/Cluckwork.Api/Hosting/CluckworkIdentityServiceCollectionExtensions.cs b/src/Cluckwork.Api/Hosting/CluckworkIdentityServiceCollectionExtensions.cs index 609721a0d..2b7a72214 100644 --- a/src/Cluckwork.Api/Hosting/CluckworkIdentityServiceCollectionExtensions.cs +++ b/src/Cluckwork.Api/Hosting/CluckworkIdentityServiceCollectionExtensions.cs @@ -13,9 +13,14 @@ namespace Cluckwork.Api.Hosting; internal static class CluckworkIdentityServiceCollectionExtensions { + // role is OneShot for the operator verbs. Nothing here issues or validates a + // token for them — see the key guard below, which is serving-only for that + // reason and would otherwise be a fresh instance of the #331 class this file + // now has to avoid (#347/#510). public static IServiceCollection AddCluckworkIdentity( this IServiceCollection services, - IConfiguration configuration) + IConfiguration configuration, + ProcessRole role = ProcessRole.Serving) { services.AddScoped(); services.AddScoped(sp => @@ -41,15 +46,32 @@ public static IServiceCollection AddCluckworkIdentity( services.Configure( configuration.GetSection(JwtOptions.SectionName)); - var jwtPublicKeyPem = PemKey.Normalize(configuration["Jwt:PublicKeyPem"] - ?? throw new InvalidOperationException( - "Jwt:PublicKeyPem is not configured.")); + // #510 — this guard used to be dead for every real deployment, and the + // key's CONTENT was never checked at boot at all. + // + // `configuration[...] ?? throw` only catches NULL. The shipped + // appsettings.json carries `"PublicKeyPem": ""`, so a deployment that + // loses its environment variable gets an EMPTY STRING and sailed past — + // the guard fired only when appsettings.json was absent entirely, which + // is not how the image runs. And ImportFromPem sat inside the AddJwtBearer + // delegate, so a malformed key was a per-request failure: the farm booted, + // /health/ready went green, the container HEALTHCHECK passed, and every + // authenticated request 500'd. An orchestrator saw a healthy instance that + // rejected every login. + // + // Both halves are fixed here, at boot, matching the fail-closed stance of + // the #261/#262 TLS floor and the #264 tzdata canary — a farm with no + // usable signing key must not pretend to be up. + var jwtPublicKeyPem = EnsureUsablePublicKey(configuration, role); services .AddAuthentication(JwtBearerDefaults.AuthenticationScheme) .AddJwtBearer(options => { var rsa = RSA.Create(); + // Still imported here because RSA is not thread-safe and this + // instance is the one the options hold; the boot-time import + // above is what decides whether the key is usable at all. rsa.ImportFromPem(jwtPublicKeyPem); options.MapInboundClaims = false; @@ -68,6 +90,7 @@ public static IServiceCollection AddCluckworkIdentity( }); services.AddAuthorization(options => options.AddCluckworkPolicies()); + services.AddSingleton< Microsoft.AspNetCore.Authorization.IAuthorizationMiddlewareResultHandler, ForbiddenProblemResultHandler>(); @@ -102,4 +125,55 @@ public static IServiceCollection AddCluckworkIdentity( return services; } + + // Serving-only (#347). A one-shot verb neither issues nor validates a token — + // `recover-admin` resets a password, `bootstrap-admin` creates an Owner, + // `migrate` applies DDL — so requiring a signing key of them would abort the + // break-glass path over configuration it does not use. That is precisely the + // #331 class, and making a NEW guard eager is how it would come back; this is + // why #510's fix belongs with #347 rather than before it. + // + // OneShotVerbMinimalConfigTests pins it from the other side: its environment + // carries no Jwt:* at all, so a verb that started demanding one goes red. + private static string EnsureUsablePublicKey(IConfiguration configuration, ProcessRole role) + { + var publicKeyPem = configuration["Jwt:PublicKeyPem"]; + if (role is not ProcessRole.Serving) + return publicKeyPem ?? string.Empty; + + // BOTH keys, because both fail the same way and only one of them was ever + // looked at here. The private key is read per-request by JwtTokenService + // and StepUpGrantService, so a corrupt one boots green and then 500s the + // login it is needed for — the same defect as the public key, one file + // over. + EnsureUsable("Jwt:PublicKeyPem", publicKeyPem); + EnsureUsable("Jwt:PrivateKeyPem", configuration["Jwt:PrivateKeyPem"]); + return PemKey.Normalize(publicKeyPem!); + } + + private static void EnsureUsable(string key, string? pem) + { + // IsNullOrWhiteSpace, not `?? throw`: the shipped appsettings.json carries + // an EMPTY string for both keys, so a null check is unreachable in any + // real deployment and the guard was decorative (#510). + if (string.IsNullOrWhiteSpace(pem)) + throw new InvalidOperationException( + $"{key} is not configured. The API cannot sign or validate tokens without it, " + + "so it refuses to start rather than accept traffic it must reject."); + + using var rsa = RSA.Create(); + try + { + rsa.ImportFromPem(PemKey.Normalize(pem)); + } + catch (Exception ex) when (ex is ArgumentException or CryptographicException) + { + // Named, and at BOOT. Previously this surfaced per-request from inside + // the AddJwtBearer delegate: /health/ready stayed green, the container + // HEALTHCHECK passed, and every authenticated request 500'd — an + // orchestrator saw a healthy instance that rejected every login. + throw new InvalidOperationException( + $"{key} is not a usable PEM key: {ex.Message}", ex); + } + } } diff --git a/src/Cluckwork.Api/Program.cs b/src/Cluckwork.Api/Program.cs index ae0b590d8..ffacd95c3 100644 --- a/src/Cluckwork.Api/Program.cs +++ b/src/Cluckwork.Api/Program.cs @@ -55,7 +55,7 @@ builder.Configuration, builder.Environment); -builder.Services.AddCluckworkIdentity(builder.Configuration); +builder.Services.AddCluckworkIdentity(builder.Configuration, processRole); var rateLimiting = builder.Services.AddCluckworkRateLimiting( builder.Configuration, processRole); diff --git a/tests/Cluckwork.Api.IntegrationTests/ProcessRoleGuardTests.cs b/tests/Cluckwork.Api.IntegrationTests/ProcessRoleGuardTests.cs index 08c12e1a7..e1f7c06e2 100644 --- a/tests/Cluckwork.Api.IntegrationTests/ProcessRoleGuardTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/ProcessRoleGuardTests.cs @@ -188,6 +188,33 @@ private sealed record ServingGuard( // The first derivation missed both because it walked `IsProduction` and // explicit `throw` sites, which is "list what I thought of" one // abstraction level up. ServingGuardCoverageTests now enumerates them. + // #510, folded in here because its fix DEPENDS on this PR's mechanism: + // making the JWT key check eager without a role scope would have been a + // brand-new instance of the #331 class, in the most security-sensitive + // file of the set. Four rows because there are four violations — each key + // can be missing or unusable, and only one of the four ever failed the + // boot before. + // + // Satisfy supplies a real generated pair (see Start); these rows break one + // half of it at a time. + new("#510 public missing", "Jwt:PublicKeyPem is not configured", + Violate: psi => psi.Environment["Jwt__PublicKeyPem"] = " ", + Satisfy: psi => psi.Environment["Jwt__PublicKeyPem"] = TestJwtKeys.PublicKeyPem), + + new("#510 public unusable", "Jwt:PublicKeyPem is not a usable PEM key", + Violate: psi => psi.Environment["Jwt__PublicKeyPem"] = + "-----BEGIN PUBLIC KEY-----\\nnot-base64\\n-----END PUBLIC KEY-----", + Satisfy: psi => psi.Environment["Jwt__PublicKeyPem"] = TestJwtKeys.PublicKeyPem), + + new("#510 private missing", "Jwt:PrivateKeyPem is not configured", + Violate: psi => psi.Environment["Jwt__PrivateKeyPem"] = " ", + Satisfy: psi => psi.Environment["Jwt__PrivateKeyPem"] = TestJwtKeys.PrivateKeyPem), + + new("#510 private unusable", "Jwt:PrivateKeyPem is not a usable PEM key", + Violate: psi => psi.Environment["Jwt__PrivateKeyPem"] = + "-----BEGIN PRIVATE KEY-----\\nnot-base64\\n-----END PRIVATE KEY-----", + Satisfy: psi => psi.Environment["Jwt__PrivateKeyPem"] = TestJwtKeys.PrivateKeyPem), + // Each cap is TWO rows, not one. Both validators have a floor branch // (<= 0) and a distinct CEILING branch (> the domain constant), and the // first version violated only the floor — so deleting either ceiling diff --git a/tests/Cluckwork.Api.IntegrationTests/ServingGuardCoverageTests.cs b/tests/Cluckwork.Api.IntegrationTests/ServingGuardCoverageTests.cs index 7a4931b34..5e2f0d998 100644 --- a/tests/Cluckwork.Api.IntegrationTests/ServingGuardCoverageTests.cs +++ b/tests/Cluckwork.Api.IntegrationTests/ServingGuardCoverageTests.cs @@ -23,24 +23,44 @@ namespace Cluckwork.Api.IntegrationTests; // — that would be the tautology the registry test was corrected for. public sealed class ServingGuardCoverageTests { - // 1. Guards inside ServingBootGuards. Every private static check method - // there is one, by construction of the class. Reflection rather than a - // list, so a fourth method added tomorrow fails this immediately. + // 1. Guard methods anywhere in Cluckwork.Api.Hosting, not just inside + // ServingBootGuards. + // + // Scoping this to one type was the same "list what I thought of" method + // that produced every other miss in this PR: #510's JWT key guard landed + // in CluckworkIdentityServiceCollectionExtensions, a THIRD file, and a + // ServingBootGuards-only walk could not see it. Walk the namespace and + // exclude deliberately. [Fact] public void EveryServingBootGuardMethod_HasACoveringRow() { - var guardMethods = typeof(ServingBootGuards) - .GetMethods(BindingFlags.NonPublic | BindingFlags.Static) + var guardMethods = typeof(ServingBootGuards).Assembly + .GetTypes() + .Where(t => t.Namespace == typeof(ServingBootGuards).Namespace) + .SelectMany(t => t.GetMethods(BindingFlags.NonPublic | BindingFlags.Static)) .Where(m => m.Name.StartsWith("Ensure", StringComparison.Ordinal)) .Select(m => m.Name) + .Distinct(StringComparer.Ordinal) .ToArray(); // The mapping is deliberately explicit and tiny: a method name cannot be - // derived from a config key, and guessing would let a rename pass. - var covered = new Dictionary(StringComparer.Ordinal) + // derived from a config key, and guessing would let a rename pass. One + // entry per method, listing every token that method is responsible for — + // per VIOLATION, so a method with two branches owes two rows. + var covered = new Dictionary(StringComparer.Ordinal) { - ["EnsureTrustedProxiesConfigured"] = "RateLimiting:TrustedProxies is empty", - ["EnsureAllowedHostsPinned"] = "AllowedHosts is missing, blank, or wildcard", + ["EnsureTrustedProxiesConfigured"] = ["RateLimiting:TrustedProxies is empty"], + ["EnsureAllowedHostsPinned"] = ["AllowedHosts is missing, blank, or wildcard"], + // #510 — the role gate; its rows' one-shot arms are what prove a verb + // still runs with no signing key at all. + ["EnsureUsablePublicKey"] = ["Jwt:PublicKeyPem is not configured"], + // ...and the check itself, run once per key, two branches each. + ["EnsureUsable"] = + [ + "Jwt:PublicKeyPem is not a usable PEM key", + "Jwt:PrivateKeyPem is not configured", + "Jwt:PrivateKeyPem is not a usable PEM key", + ], }; Assert.NotEmpty(guardMethods); @@ -61,7 +81,7 @@ public void EveryServingBootGuardMethod_HasACoveringRow() $"this mapping names ServingBootGuards method(s) that no longer exist: " + $"{string.Join(", ", stale)}. Was one renamed or removed?"); - foreach (var token in covered.Values) + foreach (var token in covered.Values.SelectMany(t => t)) Assert.Contains(token, ProcessRoleGuardTests.CoveredGuardTokens); } diff --git a/tools/simulation/verify-harness.sh b/tools/simulation/verify-harness.sh index c035be8df..bbd6e9ece 100755 --- a/tools/simulation/verify-harness.sh +++ b/tools/simulation/verify-harness.sh @@ -179,7 +179,7 @@ if grep -qi 'variable is not set' "$compose_err"; then fi python3 - "$resolved_json" <<'PY' || exit 1 -import json, sys +import json, re, sys from urllib.parse import urlsplit cfg = json.load(open(sys.argv[1], encoding="utf-8")) @@ -306,6 +306,44 @@ else: ok.append(f"Otlp endpoint OK ({parts.scheme}" f"{', plaintext explicitly acknowledged' if parts.scheme != 'https' else ''})") +# --- #510 JWT signing keys ------------------------------------------------ +# The serving boot now refuses to start unless BOTH PEMs are present and +# actually import. The generic "variable is not set" check above cannot see +# this: a variable set to a BLANK, or to deploy/.env.example's `replace-me` +# armor, is set as far as compose is concerned and dies at boot instead. +# +# On the header's "do not add guards speculatively" rule: this one has NOT +# broken the harness — bootstrap.sh generates a real keypair, so a freshly +# bootstrapped .env.sim satisfies it. It is here because the AGENTS.md #370 rule +# requires a new boot guard to reach all three harness files, and because the +# thing it catches is one this repo actively SHIPS: deploy/.env.example's +# `replace-me` body is a plausible copy-paste source for a hand-edited .env.sim. +# It also cannot produce the failure mode the header is really about — it only +# ever appends to `fail`, so it can turn a green red, never a red green. +# +# Deliberately NOT a reimplementation of ImportFromPem — the claim is bounded to +# "this is a real key rather than a placeholder": armor present, and a body that +# is substantial base64. Chasing app parity in a checker is how a checker drifts +# from the thing it mirrors. +for key in ("Jwt__PublicKeyPem", "Jwt__PrivateKeyPem"): + raw = env.get(key) + if raw is None or not str(raw).strip(): + fail.append(f"{key} is missing or blank on the app service — " + "#510 fails the Production boot") + continue + pem = str(raw).replace("\\n", "\n") + body = "".join( + line for line in pem.splitlines() + if line.strip() and not line.startswith("-----")) + if "-----BEGIN" not in pem or "-----END" not in pem: + fail.append(f"{key} has no PEM armor — #510 fails the Production boot") + elif len(body) < 64 or not re.fullmatch(r"[A-Za-z0-9+/=]+", body): + fail.append(f"{key} carries a placeholder body, not a key " + "(deploy/.env.example ships `replace-me`) — " + "#510 fails the Production boot; regenerate with bootstrap.sh") + else: + ok.append(f"{key} OK (PEM armor, {len(body)}-char base64 body)") + for line in ok: print(f" {line}") for line in fail: