Repository navigation
fix(infra): fail closed when the design-time migration connection is unset or insecure (#318) - #329
Conversation
AppDbContextDesignTimeFactory used to fall back to a predictable Host=localhost;...;Username=postgres;Password=postgres connection when CLUCKWORK_MIGRATIONS_CONNECTION was unset, and always skipped the Production TLS floor. An unset/blank env var now throws immediately naming the variable, and every target is held to the same allow-list TLS floor a Production boot enforces (#261/#262), reused via PostgresConnectionString.NormalizeAndValidate. Plaintext is permitted only via CLUCKWORK_MIGRATIONS_ALLOW_INSECURE_LOOPBACK=true, and only when the connection targets a loopback host.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7296d458a2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| onWarning: warnings.Add); | ||
| foreach (var warning in warnings) | ||
| { | ||
| Console.Error.WriteLine($"warning: {warning}"); |
There was a problem hiding this comment.
When a plaintext loopback connection succeeds, the reused validator produces a warning saying that access was permitted via Database:AllowInsecureConnection, and this loop prints it unchanged. The design-time factory never reads that setting—the relevant acknowledgement is CLUCKWORK_MIGRATIONS_ALLOW_INSECURE_LOOPBACK—so the security warning directs operators to an ineffective control. Emit a design-time-specific warning naming the actual environment variable.
AGENTS.md reference: AGENTS.md:L51-L51
Useful? React with 👍 / 👎.
…warn branch Review of #329 noted the test class carries no [Collection], so it is its own implicit collection and still runs in wall-clock parallel with every other collection in the assembly — while CLUCKWORK_MIGRATIONS_* are process-global. Nothing else reads those names today, so the isolation was accidental, and a future test touching either would have flaked silently. A dedicated DisableParallelization collection makes it structural. Also adds the missing sslmode=Require case. The warn tier had no coverage at all: deleting the warning loop left every test green. The new test pins both sides — that Require is accepted, and that the warning is actually emitted (verified by deleting the loop and watching it go red).
…at works Review of #329: the TLS floor is reused from the boot path, so its warning text names Database:AllowInsecureConnection. Design-time tooling never reads that setting — the acknowledgement here is CLUCKWORK_MIGRATIONS_ALLOW_INSECURE_LOOPBACK — so an operator following the warning would have been sent to a control that does nothing for `dotnet ef`. Rewrite the borrowed name on the way out. Test asserts the emitted warning names the env var and not the boot setting; verified by printing the warning verbatim again and watching it go red.
|
Correct, and it's the cost of reusing the boot-path validator — the substance transferred but the remediation pointer didn't. The borrowed name is now rewritten on the way out, so the warning names Full suite 981/981, 0 warnings. |
1 participant
Closes #318.
Problem
AppDbContextDesignTimeFactory— whatdotnet ef migrations add/database updateuse to build aDbContext— fell back to a predictableHost=localhost;…;Username=postgres;Password=postgresconnection wheneverCLUCKWORK_MIGRATIONS_CONNECTIONwas unset, and always skipped the Production TLS floor. That let an operator typo silently target the wrong database, and normalized a publicly-known credential.This is deliberately sequenced before the pre-launch schema batch (#283, #270, #307) so the design-time path is fail-closed before more migrations get generated through it.
Fix
PostgresConnectionString.NormalizeAndValidatefrom Deploy: support URI-form (postgresql://) connection strings — Npgsql's key-value-only parser rejects them #261/Deploy: enforce TLS (sslmode) on every production Postgres connection #262 — the existing allow-list (VerifyCA/VerifyFullsilent,Requirewarns, everything else fails) plus its libpq-URI translation. No second, divergent validator was written.CLUCKWORK_MIGRATIONS_ALLOW_INSECURE_LOOPBACK=truepermits plaintext, but only when the target host is loopback (localhost/127.0.0.1/::1, checked viaIPAddress.IsLoopbackrather than a hand-rolled CIDR test). Set against any other host it still fails, so the opt-in can't silently widen scope.Scoped to the design-time factory only — the
migrateCLI verb runs against the built host's own configuration and is untouched.Tests
15 tests in
AppDbContextDesignTimeFactoryTests, covering both sides of every boundary: unset/blank rejected; the error leaks no credentials; no fallback target remains; plaintext rejected without the opt-in; plaintext accepted with it on loopback; the opt-in still rejecting a non-loopback host; weak remote TLS rejected via both key-value and URI form;VerifyFull/VerifyCAaccepted.Mutation-checked with three independent mutants:
postgres/postgresfallback → 3 tests RED.Worth recording: mutant 1 initially left one test green — it had been passing for the wrong reason (the TLS floor happened to reject the fallback host independently). That test was strengthened to pin the "no default target" message so it detects the defect on its own. A test that passes with the bug present is a broken test.
Verification
dotnet build Cluckwork.sln— 0 warnings, 0 errorsdotnet test Cluckwork.sln— 979/979 passing (283 Domain + 114 Application + 582 Integration)Docs
AGENTS.mdgains a bullet beside the existing #260/#261/#262 hardening entries, anddeploy/README.mda section documenting the variable and the escape hatch. Commands use placeholders only (<host>,<user>,<password>) — no credentials, no provider names, no concrete deploy values.