Discovered while auditing PR #520 (which closes #440, #441, #444, #446).
PR #520's regression test for issue #440 (TestEnsureAdminUserWithPassword_LogsToStderr_NotStdout in internal/database/postgres/migrations/migrate_security_test.go) does not actually invoke the production functions ensureAdminUserWithPassword or ensureAdminUser. Instead it re-types the expected log.Printf(...) calls inline and asserts on those:
// Simulate the log call that the fixed code makes:
log.Printf("Ensuring admin user exists with password: %s", "admin@example.com")
log.Printf("Admin user created/activated: %s", "admin@example.com")Because the test simulates the calls rather than exercising the real code path, a regression that reverts log.Printf back to fmt.Printf in migrate.go would NOT fail this test. The test guards a copy of the behaviour, not the behaviour itself. The other three fixes in #520 (#441, #444, #446) do have tests that call the real functions and are sound.
The obstacle the PR worked around is that ensureAdminUserWithPassword needs a live *pgxpool.Pool and panics on nil. The fix needs the function refactored so the log emission is testable without a DB, or an integration-tagged test against a real container (as PR #533 does for the same package).
Remediation
- Refactor so the stdout-vs-stderr decision is exercised by the real function, or add an integration test (
//go:build integration) that runs ensureAdminUserWithPassword against a postgres container and asserts nothing password-related lands on stdout.
- Keep the assertion that the actual password value never appears in any log sink.
Acceptance
Discovered while auditing PR #520 (which closes #440, #441, #444, #446).
PR #520's regression test for issue #440 (
TestEnsureAdminUserWithPassword_LogsToStderr_NotStdoutininternal/database/postgres/migrations/migrate_security_test.go) does not actually invoke the production functionsensureAdminUserWithPasswordorensureAdminUser. Instead it re-types the expectedlog.Printf(...)calls inline and asserts on those:Because the test simulates the calls rather than exercising the real code path, a regression that reverts
log.Printfback tofmt.Printfinmigrate.gowould NOT fail this test. The test guards a copy of the behaviour, not the behaviour itself. The other three fixes in #520 (#441, #444, #446) do have tests that call the real functions and are sound.The obstacle the PR worked around is that
ensureAdminUserWithPasswordneeds a live*pgxpool.Pooland panics on nil. The fix needs the function refactored so the log emission is testable without a DB, or an integration-tagged test against a real container (as PR #533 does for the same package).Remediation
//go:build integration) that runsensureAdminUserWithPasswordagainst a postgres container and asserts nothing password-related lands on stdout.Acceptance
log.Printfin migrate.go is reverted tofmt.Printf.