fix(postgres): review feedback — pg_basebackup user, shell-quote DbUser, recovery test, RmdirAll, generic test creds

PR #136 review feedback, dispositions:

1. replication.go (VALIDE): pg_basebackup hardcoded "-U postgres"; now uses
   config.DbUser (fallback "postgres"), matching the superuser created by
   initdb --username=<db-user>. Needed so replication works with a custom
   initial superuser (otherwise the primary has no 'postgres' role).
   exec.Command is argv-based, so no shell-quoting needed here.

2. scripts.go (VALIDE): DbUser was interpolated raw into generated bash
   scripts (use/clear/replication/recovery). Added a local shellQuote helper
   (POSIX single-quoting) applied at all four generation sites; preserves any
   valid PostgreSQL role name while neutralizing shell metacharacters. No new
   dependency (no shlex in go.mod, no repo helper existed).

3. test coverage (VALIDE): added TestGenerateCheckRecoveryScriptDbUser covering
   GenerateCheckRecoveryScript with a custom DbUser; existing replication test
   left untouched.

4. RmdirAll (SKIPPE): common.RmdirAll exists (common/fileutil.go:639) but calls
   ErrCheckExitf -> os.Exit(1) on any error and has $HOME/$PWD guards that also
   exit. The paths I added do 'os.RemoveAll(config.Dir); return wrapped err' to
   surface a clean error to the caller; RmdirAll would hard-exit instead and
   would be inconsistent with the pre-existing os.RemoveAll calls in the same
   function (not in scope of this review). Per the finding's own instruction
   ('si ça change la sémantique, skippe'), skipped.

Generic test creds: replaced project-specific 'wse' with 'testuser' in
TestGenerateScriptsDbUserPropagation (DbUser value + quoted assertions) and in
the new recovery test. No password value was used in these script-generation
tests, so only the username was renamed. No 'wse' appears in provider code or
comments (only in this commit's predecessor example, which is git history).

Author: nnosal

providers/postgresql/postgresql_test.go

@@ -214,24 +214,24 @@ func TestGenerateScripts(t *testing.T) {
 }
 
 // TestGenerateScriptsDbUserPropagation ensures the configured db-user is embedded
-// in the generated psql/initdb invocations, and that an empty DbUser falls back
-// to the historical "postgres" default.
+// (shell-quoted) in the generated psql/initdb invocations, and that an empty
+// DbUser falls back to the historical "postgres" default.
 func TestGenerateScriptsDbUserPropagation(t *testing.T) {
-	// Explicit user: scripts must reference it and not the default.
+	// Explicit user: scripts must reference it (shell-quoted) and not the default.
 	custom := GenerateScripts(ScriptOptions{
 		BinDir:  "/opt/postgresql/16.13/bin",
 		DataDir: "/tmp/pg/data",
 		Port:    16613,
-		DbUser:  "wse",
+		DbUser:  "testuser",
 	})
-	if !strings.Contains(custom["use"], "-U wse") {
-		t.Errorf("use script must embed -U wse; got: %q", custom["use"])
+	if !strings.Contains(custom["use"], "-U 'testuser'") {
+		t.Errorf("use script must embed -U 'testuser'; got: %q", custom["use"])
 	}
 	if strings.Contains(custom["use"], "-U postgres") {
 		t.Errorf("use script must not reference -U postgres; got: %q", custom["use"])
 	}
-	if !strings.Contains(custom["clear"], "--username=wse") {
-		t.Errorf("clear script must embed --username=wse; got: %q", custom["clear"])
+	if !strings.Contains(custom["clear"], "--username='testuser'") {
+		t.Errorf("clear script must embed --username='testuser'; got: %q", custom["clear"])
 	}
 
 	// Empty user: falls back to "postgres" (backward compatible).
@@ -240,20 +240,41 @@ func TestGenerateScriptsDbUserPropagation(t *testing.T) {
 		DataDir: "/tmp/pg/data",
 		Port:    16613,
 	})
-	if !strings.Contains(def["use"], "-U postgres") {
-		t.Errorf("default use script must embed -U postgres; got: %q", def["use"])
+	if !strings.Contains(def["use"], "-U 'postgres'") {
+		t.Errorf("default use script must embed -U 'postgres'; got: %q", def["use"])
 	}
-	if !strings.Contains(def["clear"], "--username=postgres") {
-		t.Errorf("default clear script must embed --username=postgres; got: %q", def["clear"])
+	if !strings.Contains(def["clear"], "--username='postgres'") {
+		t.Errorf("default clear script must embed --username='postgres'; got: %q", def["clear"])
 	}
 
 	// Replication scripts honor DbUser too.
 	repl := GenerateCheckReplicationScript(ScriptOptions{
 		BinDir: "/opt/postgresql/16.13/bin",
 		Port:   16613,
-		DbUser: "wse",
+		DbUser: "testuser",
 	})
-	if !strings.Contains(repl, "-U wse") {
-		t.Errorf("replication script must embed -U wse; got: %q", repl)
+	if !strings.Contains(repl, "-U 'testuser'") {
+		t.Errorf("replication script must embed -U 'testuser'; got: %q", repl)
+	}
+}
+
+// TestGenerateCheckRecoveryScriptDbUser ensures the recovery-check script embeds
+// the configured db-user (shell-quoted), complementing the existing
+// TestGenerateCheckRecoveryScript which only checks the query text and ports.
+func TestGenerateCheckRecoveryScriptDbUser(t *testing.T) {
+	ports := []int{16614, 16615}
+	script := GenerateCheckRecoveryScript(ScriptOptions{
+		BinDir: "/opt/postgresql/16.13/bin",
+		Port:   16613,
+		DbUser: "testuser",
+	}, ports)
+	if !strings.Contains(script, "pg_is_in_recovery") {
+		t.Error("missing pg_is_in_recovery query")
+	}
+	if !strings.Contains(script, "-U 'testuser'") {
+		t.Errorf("recovery script must embed -U 'testuser'; got: %q", script)
+	}
+	if !strings.Contains(script, "16614") || !strings.Contains(script, "16615") {
+		t.Error("recovery script missing replica ports")
 	}
 }

providers/postgresql/replication.go

@@ -22,12 +22,20 @@ func (p *PostgreSQLProvider) CreateReplica(primary providers.SandboxInfo, config
 	dataDir := filepath.Join(config.Dir, "data")
 	logFile := filepath.Join(config.Dir, "postgresql.log")
 
+	// The replica connects to the primary as the configured superuser, which
+	// matches the role created by initdb --username=<db-user>. Fall back to
+	// "postgres" to preserve the historical behavior when no user is set.
+	dbUser := config.DbUser
+	if dbUser == "" {
+		dbUser = "postgres"
+	}
+
 	// pg_basebackup from the running primary
 	pgBasebackup := filepath.Join(binDir, "pg_basebackup")
 	bbCmd := exec.Command(pgBasebackup,
 		"-h", "127.0.0.1",
 		"-p", fmt.Sprintf("%d", primary.Port),
-		"-U", "postgres",
+		"-U", dbUser,
 		"-D", dataDir,
 		"-Fp", "-Xs", "-R",
 	)

providers/postgresql/scripts.go

@@ -23,6 +23,14 @@ export LD_LIBRARY_PATH="%s"
 unset PGDATA PGPORT PGHOST PGUSER PGDATABASE
 `
 
+// shellQuote wraps s in POSIX single quotes (escaping any embedded single
+// quotes) so it is safe to interpolate into a generated bash script. This
+// preserves any valid PostgreSQL role name while neutralizing shell
+// metacharacters in user-supplied values.
+func shellQuote(s string) string {
+	return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'"
+}
+
 func GenerateScripts(opts ScriptOptions) map[string]string {
 	preamble := fmt.Sprintf(envPreamble, opts.LibDir)
 	dbUser := opts.DbUser
@@ -44,10 +52,10 @@ func GenerateScripts(opts ScriptOptions) map[string]string {
 			preamble, opts.BinDir, opts.DataDir, opts.LogFile),
 
 		"use": fmt.Sprintf("%s%s/psql -h 127.0.0.1 -p %d -U %s \"$@\"\n",
-			preamble, opts.BinDir, opts.Port, dbUser),
+			preamble, opts.BinDir, opts.Port, shellQuote(dbUser)),
 
 		"clear": fmt.Sprintf("%s%s/pg_ctl -D %s stop -m fast 2>/dev/null\nrm -rf %s\n%s/initdb -D %s --auth=trust --username=%s\necho \"Sandbox cleared.\"\n",
-			preamble, opts.BinDir, opts.DataDir, opts.DataDir, opts.BinDir, opts.DataDir, dbUser),
+			preamble, opts.BinDir, opts.DataDir, opts.DataDir, opts.BinDir, opts.DataDir, shellQuote(dbUser)),
 	}
 }
 
@@ -59,7 +67,7 @@ func GenerateCheckReplicationScript(opts ScriptOptions) string {
 	}
 	return fmt.Sprintf(`%s%s/psql -h 127.0.0.1 -p %d -U %s -c \
   "SELECT client_addr, state, sent_lsn, write_lsn, flush_lsn, replay_lsn FROM pg_stat_replication;"
-`, preamble, opts.BinDir, opts.Port, dbUser)
+`, preamble, opts.BinDir, opts.Port, shellQuote(dbUser))
 }
 
 func GenerateCheckRecoveryScript(opts ScriptOptions, replicaPorts []int) string {
@@ -72,7 +80,7 @@ func GenerateCheckRecoveryScript(opts ScriptOptions, replicaPorts []int) string
 	b.WriteString(preamble)
 	for _, port := range replicaPorts {
 		b.WriteString(fmt.Sprintf("echo \"=== Replica port %d ===\"\n", port))
-		b.WriteString(fmt.Sprintf("%s/psql -h 127.0.0.1 -p %d -U %s -c \"SELECT pg_is_in_recovery();\"\n", opts.BinDir, port, dbUser))
+		b.WriteString(fmt.Sprintf("%s/psql -h 127.0.0.1 -p %d -U %s -c \"SELECT pg_is_in_recovery();\"\n", opts.BinDir, port, shellQuote(dbUser)))
 	}
 	return b.String()
 }