postgres.Migrate writes the database password to the pod log on a malformed DATABASE_URL

[mrautela365]

A malformed DATABASE_URL causes the full DSN, including the password, to be written to the pod log at startup and again on every background retry. Verified end-to-end on origin/main, not in a PR branch.

Mechanism

(*url.Error).Error() renders as fmt.Sprintf("%s %q: %s", Op, URL, Err) — it embeds the URL it was given.

The subtle part: no format string in our code names the URL. migrate.NewWithSourceInstance → database.Open parses the DSN internally and returns a *url.Error already carrying it. internal/infrastructure/postgres/pool.go:138 then wraps it:

return fmt.Errorf("init migrator: %w", err)

and internal/container/container.go:948 calls postgres.Migrate(dsn), surfacing the result through slog.Warn at startup and on each retry.

So a grep for url.Parse does not find this site — the parse happens inside a dependency.

Why unwrapping is not sufficient

The obvious remedy — unwrap to drop the outer URL — still leaks. net/url causes quote the fragment they choked on, so:

postgres://user:<password>@host/db     ->  invalid URL escape "%zz"

where the quoted fragment is part of the password itself. This was found by mutation testing on #159: a fix that unwrapped instead of discarding survived the mutation.

The form that holds is to discard the cause entirely for *url.Error and substitute a sentinel, which is what internal/platform/llm already does under its own no-echo rule. Note safeErrSummary normalises and truncates — it is not a redactor and must not be used as one here.

Four net/url failure modes were confirmed to leak: invalid percent-escape, invalid port, unclosed IPv6 bracket, and a control character in the URL.

Scope

#159 fixed 9 test-path sites (dbtest.go, invalid_index_live_test.go, the migrate-down harness). This production path was deliberately left out: turning an approved, test-only PR into a production change was not a trade worth making mid-review.

Suggested fix

Route the Migrate error through the same discard-the-cause treatment, and add a test that renders the error for a password-bearing DSN and asserts the password is absent. Mutation-verify it: reverting the fix must make the test fail, and an unwrap-only variant must also fail — otherwise the test is not pinning the real behaviour.

Keep diagnosability: an operator must still be able to distinguish a malformed DSN from a connection failure, so replace the cause with a sentinel rather than dropping the error.

cc @dealako

[mrautela365]

The fix shape is now established, and production is the easier case

#159 has landed a working redaction for the test paths. Recording the mechanism here because the obvious approach fails and it took four mutation survivors to find that out.

What does not work: scrubbing shapes

redact.URLUserinfo only understands URL-shaped user:pass@ text. pgx emits keyword/value DSNs, so a host=... user=... password=... string passes through completely unredacted — password included, not just the username as first reported.

Worse, two facts make shape-scrubbing a losing bet here:

1. pgx errors never reach the *url.Error sentinel arm at all — errors.As returns false for every one of them, so they all land on the fallback.
2. PostgreSQL itself quotes identifiers back (SQLSTATE 28000 / 3D000 / 28P01). That channel is generated by the server, so no amount of pattern-matching pgx's own two templates covers it.

Scrubbing is a denylist. This redaction has now been defeated twice by a shape nobody enumerated; a third denylist would be a third bet.

What works: sentinel substitution keyed on VALUES

pgconn.ParseConfig parses both DSN forms into one Config. Any message containing the configured user, password, database or host is replaced wholesale. It mirrors config.redactDatabaseURL, which is already this repo's answer for this value class.

Production is strictly easier than the test path: postgres.Migrate already holds the DSN it dialled, so it can do the same value-keyed comparison without reading the environment.

Four mutation survivors worth knowing about before you write this

All four were real gaps in the first version of the fix:

• dropping cfg.Host from the comparison — realistic messages name several identifiers at once, so no single comparison is individually load-bearing and dropping one survives
• dropping cfg.Password — same reason
• failing open on an unparseable DSN
• dropping the id != "" guard — this is the dangerous one. strings.Contains(msg, "") is true for every string, so a peer-auth DSN (no password configured) would have withheld every error the package prints. A redaction that hides everything is a different outage, not a safer default.

Preserve diagnosability, and assert it

There is already a test requiring non-url.Error causes to keep their driver text, so an operator can distinguish a malformed DSN from a connection failure. A redaction that leaks nothing and diagnoses nothing trades one problem for another — whatever lands here should carry the same assertion.

One incidental finding from that work, relevant to any test written for this: a test can be environment-dependent by accident. One documented itself as needing no database yet used host=localhost fixtures, so on a runner whose own DSN said localhost the helper correctly withheld the message and the test failed. Pin a DSN sharing no identifier with the fixtures — and verify the pin does not weaken the mutation the test exists to catch.

Scope note: #159 is test-only and deliberately touched no production code. This issue remains open for pool.go:138 / container.go:948.

cc @dealako

[mrautela365]

Correction: the fix shape recorded above was necessary but not sufficient

Amending my earlier comment. Sentinel-substitution-keyed-on-values is still the right mechanism, but a later round found a leak that survived it — and the failure mode transfers directly to the production fix this issue tracks.

The leaking site called the redactor.

// leaked
fmt.Errorf("migrate %s: %s", EnvDatabaseURL, SafeDSNErr(err))
// correct
fmt.Errorf("migrate %s: %s", EnvDatabaseURL, SafeDSNErrFor(dsn, err))

SafeDSNErr redacts against the environment's DSN(). That arm is handed an explicit dsn, so it compared the message against the wrong secret and passed the right one through. A real rendered failure produced failed to connect to `user=leakuser database=leakdb` verbatim.

Every grep and every review pass saw SafeDSNErr(...) at the site and read it as fixed.

Two things made it look impossible:

1. A comment asserted the *url.Error sentinel arm applied. It does not — golang-migrate wraps pgx's ConnectError, so control reached the string arm, the one arm whose correctness depends on which DSN it is given.
2. The source guard written to catch exactly this class could not see it, because it matched SafeDSNErr textually. Two line-window versions failed before it was rewritten with go/parser + go/ast. Parsing then found two DSN-bearing sites the guard had never listed at all — the call set was too narrow independently of the line matching.

What this means for the production fix

postgres.Migrate is handed its DSN explicitly, exactly like the arm that leaked here. So:

• Pass the DSN you were given, not an ambient one. If a convenience wrapper exists that reads the environment, it is correct only where the environment is the right source — and Migrate is not that place.
• Render the output with a known-secret input and read it. A redactor call at the site is not evidence; only the rendered string is.
• If you add a guard, use the AST. Line proximity approximates the question and failed twice here.
• Mutation-test the guard with a differently-shaped violation than the one it was written for — wrong argument, argument on a continuation line, wrapped in another call. The wrong-argument shape is precisely what slipped through.

Still unpinned, stated plainly

One arm on #159 is fixed by inspection only — reaching it needs a DSN that migrates successfully then fails ping, and Migrate connects first. Reverting that fix leaves the suite green; that was verified, and the call site now says so. Any production fix will likely have the same property, and it is worth saying out loud rather than folding into a passing test count.

cc @dealako