diff --git a/.github/wiki/Config-Env-Vars.md b/.github/wiki/Config-Env-Vars.md index 3c399734..cf7514f0 100644 --- a/.github/wiki/Config-Env-Vars.md +++ b/.github/wiki/Config-Env-Vars.md @@ -17,6 +17,7 @@ All ɳSelf project configuration lives in `.env` (and optionally `.env.local` fo - [Load Order (Cascade)](#load-order-cascade) - [Core Project Settings](#core-project-settings) - [PostgreSQL](#postgresql) +- [Backup and Restore](#backup-and-restore) - [Hasura (GraphQL API)](#hasura-graphql-api) - [Auth](#auth) - [Nginx and SSL](#nginx-and-ssl) @@ -104,6 +105,36 @@ This value is written to `.env.computed` and should not be set manually. --- +## Backup and Restore + +Credentials may come from these variables or from `rclone.conf` / `RCLONE_CONFIG_*`. +Leaving **both** key variables empty is valid and means "configured elsewhere". +Setting **exactly one** of the pair is always rejected, because it is never a +meaningful state: it is a typo or a partial migration between the two accepted +name pairs, and rclone would otherwise fail silently or send an empty secret. + +| Variable | Type | Default | Required | Description | +|---|---|---|---|---| +| `BACKUP_S3_BUCKET` | string | *(unset)* | No | Destination bucket for off-box backup copies. App-level: consumed by app backup scripts, not read into the CLI's config struct. | +| `BACKUP_S3_ACCESS_KEY_ID` | string | *(unset)* | No | Access key for the backup bucket. Must be set together with `BACKUP_S3_SECRET_ACCESS_KEY`. | +| `BACKUP_S3_SECRET_ACCESS_KEY` | string | *(unset)* | No | Secret key for the backup bucket. Must be set together with `BACKUP_S3_ACCESS_KEY_ID`. | +| `BACKUP_S3_REGION` | string | *(unset)* | No | Region passed to the storage provider. | +| `BACKUP_S3_ENDPOINT` | string | *(unset)* | No | Custom endpoint for S3-compatible providers (Backblaze B2, Cloudflare R2, MinIO). Leave unset for AWS S3. | +| `BACKUP_S3_PREFIX` | string | *(unset)* | No | Key prefix applied to uploaded objects, for sharing one bucket across projects. App-level, like `BACKUP_S3_BUCKET`. | +| `BACKUP_ACCESS_KEY` | string | *(unset)* | No | **Accepted alias** for `BACKUP_S3_ACCESS_KEY_ID`. The canonical name wins when both are set. | +| `BACKUP_SECRET_KEY` | string | *(unset)* | No | **Accepted alias** for `BACKUP_S3_SECRET_ACCESS_KEY`. The canonical name wins when both are set. | + +**Why the aliases exist.** `BACKUP_ACCESS_KEY` / `BACKUP_SECRET_KEY` were already +in use in the wild (they are the names in `ntask/backend/.env.example`) but were +never read into the backup config, so every deployment using them had remote +upload silently disabled while appearing configured. Both spellings are now +accepted. Prefer the canonical `BACKUP_S3_*` names in new configuration. + +See also `BACKUP_CRITICAL_TABLES` under [PostgreSQL](#postgresql), and +[[cmd-backup]] for the commands that consume these. + +--- + ## Hasura (GraphQL API) | Variable | Type | Default | Required | Description | diff --git a/internal/backup/create.go b/internal/backup/create.go index 10dfd796..8f310b70 100644 --- a/internal/backup/create.go +++ b/internal/backup/create.go @@ -14,6 +14,41 @@ import ( "github.com/nself-org/cli/internal/metrics" ) +// requireBackupContainerConfig fails loudly, naming exactly what is missing, +// before any backup type derives a docker container name from cfg. +// +// PROJECT_NAME and POSTGRES_DB both get silent placeholder defaults inside +// config.ApplyDefaults ("myproject" / "nself") so cfg.ProjectName and +// cfg.Postgres.DB are never actually empty by the time Create() runs — that +// is precisely the trap: createFullBackup happily builds +// "myproject_postgres" and hands it to `docker exec`, which fails with a +// generic "No such container" error that names the wrong thing (a container +// that was never expected to exist) instead of the actual problem (the two +// vars that decide the real container name were never set). Checking +// os.Getenv directly here — instead of the already-defaulted cfg fields — +// is what lets this tell "the user configured myproject/nself on purpose" +// apart from "nothing was configured and the default silently took over." +// +// This only guards `nself backup create`, which is the surface the reported +// failure came from; `Backup()` in internal/database (used by other DB +// tooling) is a separate lower-level primitive with its own callers. +func requireBackupContainerConfig() error { + var missing []string + if strings.TrimSpace(os.Getenv("PROJECT_NAME")) == "" { + missing = append(missing, "PROJECT_NAME") + } + if strings.TrimSpace(os.Getenv("POSTGRES_DB")) == "" { + missing = append(missing, "POSTGRES_DB") + } + if len(missing) == 0 { + return nil + } + return fmt.Errorf( + "backup create: required config not set: %s — set these in .env, or run from inside the project directory (without them the backup would target a placeholder container name that cannot exist)", + strings.Join(missing, ", "), + ) +} + // BackupType identifies the kind of backup to create. type BackupType string @@ -48,6 +83,10 @@ func Create(ctx context.Context, cfg *config.Config, opts CreateOptions) error { return nil } + if err := requireBackupContainerConfig(); err != nil { + return err + } + types := []BackupType{opts.Type} if opts.Type == BackupTypeAll { types = []BackupType{BackupTypeFull, BackupTypeMetadata} diff --git a/internal/backup/create_remote.go b/internal/backup/create_remote.go new file mode 100644 index 00000000..4eb43b90 --- /dev/null +++ b/internal/backup/create_remote.go @@ -0,0 +1,82 @@ +// Package backup — create_remote.go: post-create encryption and remote +// (rclone) upload helpers used by createFullBackup. Split out of +// create_targets.go (file-size ratchet, internal/repoqa) as a pure move — +// no behavior change beyond what is documented on requireCompleteS3Credentials +// and uploadToRemote below. +package backup + +import ( + "context" + "fmt" + "os" + "os/exec" + "path/filepath" + + "github.com/nself-org/cli/internal/config" + "github.com/nself-org/cli/internal/errs" +) + +// encryptFile encrypts a file in-place using age with the given recipient public key. +func encryptFile(path, recipient string) error { + encPath := path + ".age" + args := []string{"-r", recipient, "-o", encPath, path} + cmd := exec.Command("age", args...) + if output, err := cmd.CombinedOutput(); err != nil { + return fmt.Errorf("%w: %s", errs.ErrBackupEncryptFailed, string(output)) + } + + // Replace original with encrypted version. + if err := os.Remove(path); err != nil { + return fmt.Errorf("remove unencrypted file: %w", err) + } + if err := os.Rename(encPath, path+".age"); err != nil { + return fmt.Errorf("rename encrypted file: %w", err) + } + + return nil +} + +// requireCompleteS3Credentials refuses a half-configured S3 credential pair. +// Exactly one of BACKUP_S3_ACCESS_KEY_ID/BACKUP_ACCESS_KEY and +// BACKUP_S3_SECRET_ACCESS_KEY/BACKUP_SECRET_KEY being set is never valid: it +// is not "use rclone.conf instead" (that path is both empty) and it is not +// "use these creds" (that path is both set) — it is a typo or partial +// migration between the two accepted name pairs, and rclone/S3 would either +// silently fail or use an empty-string secret. Both-empty is fine: the +// remote may be fully configured via rclone.conf or RCLONE_CONFIG_* env vars +// with no nSelf-side credentials at all. +func requireCompleteS3Credentials(cfg *config.Config) error { + hasAccess := cfg.Backup.S3AccessKeyID != "" + hasSecret := cfg.Backup.S3SecretAccessKey != "" + if hasAccess == hasSecret { + return nil + } + missing := "BACKUP_S3_SECRET_ACCESS_KEY (or BACKUP_SECRET_KEY)" + if hasSecret { + missing = "BACKUP_S3_ACCESS_KEY_ID (or BACKUP_ACCESS_KEY)" + } + return fmt.Errorf("S3 backup credentials are half-configured: %s is set but its counterpart is not", missing) +} + +// uploadToRemote uploads a local file to the configured rclone remote. When +// cfg carries an S3 access/secret key pair (BACKUP_S3_ACCESS_KEY_ID / +// BACKUP_S3_SECRET_ACCESS_KEY, or the BACKUP_ACCESS_KEY / BACKUP_SECRET_KEY +// aliases), it is exported as AWS_ACCESS_KEY_ID / AWS_SECRET_ACCESS_KEY for +// this rclone invocation — the env-var form rclone's :s3 / :r2 remotes read +// when no matching entry exists in rclone.conf. Callers must have already +// checked requireCompleteS3Credentials; this function does not re-check. +func uploadToRemote(ctx context.Context, localPath, remote string, cfg *config.Config) error { + args := []string{"copyto", localPath, remote + "/" + filepath.Base(localPath)} + cmd := exec.CommandContext(ctx, "rclone", args...) + cmd.Env = os.Environ() + if cfg.Backup.S3AccessKeyID != "" && cfg.Backup.S3SecretAccessKey != "" { + cmd.Env = append(cmd.Env, + "AWS_ACCESS_KEY_ID="+cfg.Backup.S3AccessKeyID, + "AWS_SECRET_ACCESS_KEY="+cfg.Backup.S3SecretAccessKey, + ) + } + if output, err := cmd.CombinedOutput(); err != nil { + return fmt.Errorf("%w: %s", errs.ErrBackupRemoteFailed, string(output)) + } + return nil +} diff --git a/internal/backup/create_targets.go b/internal/backup/create_targets.go index ea30b861..ec59520d 100644 --- a/internal/backup/create_targets.go +++ b/internal/backup/create_targets.go @@ -121,7 +121,14 @@ func createFullBackup(ctx context.Context, cfg *config.Config, backupDir, ts, ta remote = opts.Remote } if remote != "" { - if err := uploadToRemote(ctx, outputPath, remote); err != nil { + if err := requireCompleteS3Credentials(cfg); err != nil { + // Half-configured S3 creds (one of access/secret set, not both) + // is worse than none: it looks configured, uploads nothing or + // fails opaquely inside rclone, and nobody notices until a + // restore is needed. Refuse loudly instead of attempting the + // upload with partial credentials. + slog.Error("remote upload skipped: incomplete S3 credentials", "error", err, "path", outputPath) + } else if err := uploadToRemote(ctx, outputPath, remote, cfg); err != nil { slog.Error("remote upload failed", "error", err, "path", outputPath) // Non-fatal: local backup succeeded. } @@ -227,33 +234,3 @@ func triggerWALCheckpoint(ctx context.Context, cfg *config.Config) error { slog.Info("WAL checkpoint triggered") return nil } - -// encryptFile encrypts a file in-place using age with the given recipient public key. -func encryptFile(path, recipient string) error { - encPath := path + ".age" - args := []string{"-r", recipient, "-o", encPath, path} - cmd := exec.Command("age", args...) - if output, err := cmd.CombinedOutput(); err != nil { - return fmt.Errorf("%w: %s", errs.ErrBackupEncryptFailed, string(output)) - } - - // Replace original with encrypted version. - if err := os.Remove(path); err != nil { - return fmt.Errorf("remove unencrypted file: %w", err) - } - if err := os.Rename(encPath, path+".age"); err != nil { - return fmt.Errorf("rename encrypted file: %w", err) - } - - return nil -} - -// uploadToRemote uploads a local file to the configured rclone remote. -func uploadToRemote(ctx context.Context, localPath, remote string) error { - args := []string{"copyto", localPath, remote + "/" + filepath.Base(localPath)} - cmd := exec.CommandContext(ctx, "rclone", args...) - if output, err := cmd.CombinedOutput(); err != nil { - return fmt.Errorf("%w: %s", errs.ErrBackupRemoteFailed, string(output)) - } - return nil -} diff --git a/internal/backup/create_targets_test.go b/internal/backup/create_targets_test.go new file mode 100644 index 00000000..a51a6317 --- /dev/null +++ b/internal/backup/create_targets_test.go @@ -0,0 +1,81 @@ +package backup + +import ( + "strings" + "testing" + + "github.com/nself-org/cli/internal/config" +) + +// TestRequireCompleteS3Credentials_BothEmptyIsFine covers the common case: +// no BACKUP_S3_* / BACKUP_*_KEY vars set at all, remote upload relies +// entirely on rclone.conf or RCLONE_CONFIG_* env vars. This must not be +// treated as "half-configured". +func TestRequireCompleteS3Credentials_BothEmptyIsFine(t *testing.T) { + cfg := &config.Config{} + if err := requireCompleteS3Credentials(cfg); err != nil { + t.Errorf("want nil for both-empty (rclone.conf-only) config, got %v", err) + } +} + +// TestRequireCompleteS3Credentials_BothSetIsFine covers full nSelf-side +// configuration via either the canonical or alias var names — both should +// already have landed in the same struct fields by the time this runs. +func TestRequireCompleteS3Credentials_BothSetIsFine(t *testing.T) { + cfg := &config.Config{Backup: config.BackupConfig{ + S3AccessKeyID: "AKIAEXAMPLE", + S3SecretAccessKey: "supersecret", + }} + if err := requireCompleteS3Credentials(cfg); err != nil { + t.Errorf("want nil for fully-configured S3 credentials, got %v", err) + } +} + +// TestRequireCompleteS3Credentials_HalfConfiguredFails is the regression +// lock for the ntask defect: BACKUP_ACCESS_KEY set without +// BACKUP_SECRET_KEY (or the canonical pair split the same way) must fail +// loudly rather than silently attempt an upload with an empty secret. +func TestRequireCompleteS3Credentials_HalfConfiguredFails(t *testing.T) { + cases := []struct { + name string + cfg config.BackupConfig + }{ + {"access only", config.BackupConfig{S3AccessKeyID: "AKIAEXAMPLE"}}, + {"secret only", config.BackupConfig{S3SecretAccessKey: "supersecret"}}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + cfg := &config.Config{Backup: tc.cfg} + if err := requireCompleteS3Credentials(cfg); err == nil { + t.Errorf("want error for half-configured S3 credentials (%s), got nil", tc.name) + } + }) + } +} + +// TestRequireBackupContainerConfig_MissingVarsFailLoud is the regression +// lock for "nself backup create failed twice with a confusing error when +// PROJECT_NAME/POSTGRES_DB were unset" — the guard must name the missing +// variable(s) instead of letting a placeholder default silently derive a +// container name that cannot exist. +func TestRequireBackupContainerConfig_MissingVarsFailLoud(t *testing.T) { + t.Setenv("PROJECT_NAME", "") + t.Setenv("POSTGRES_DB", "") + err := requireBackupContainerConfig() + if err == nil { + t.Fatal("want error when PROJECT_NAME and POSTGRES_DB are both unset, got nil") + } + if !strings.Contains(err.Error(), "PROJECT_NAME") || !strings.Contains(err.Error(), "POSTGRES_DB") { + t.Errorf("error must name both missing vars, got: %v", err) + } +} + +// TestRequireBackupContainerConfig_SetVarsPass proves the guard is silent +// once both vars are actually configured. +func TestRequireBackupContainerConfig_SetVarsPass(t *testing.T) { + t.Setenv("PROJECT_NAME", "ntask") + t.Setenv("POSTGRES_DB", "ntask") + if err := requireBackupContainerConfig(); err != nil { + t.Errorf("want nil once PROJECT_NAME/POSTGRES_DB are set, got %v", err) + } +} diff --git a/internal/backup/verify.go b/internal/backup/verify.go index 8e7a1718..c0f775fc 100644 --- a/internal/backup/verify.go +++ b/internal/backup/verify.go @@ -229,6 +229,18 @@ func runRestoreTest(ctx context.Context, cfg *config.Config, backupFile string, smokeCmd := exec.CommandContext(ctx, "docker", smokeArgs...) output, err := smokeCmd.CombinedOutput() if err != nil { + // The gate query (user table count) is the one condition this + // whole check exists to enforce. If IT errors out — psql auth + // failure, container gone, whatever — that is not "unknown, + // carry on": failing to even run the assertion is the same + // hollow-gate shape as the drill's zero-row bug (a success + // returned with no positive check behind it). Every other + // smoke query is advisory and may legitimately fail without + // aborting the restore-test. + if sq.Label == smokeQueryCatalog[0].Label { + return fmt.Errorf("%w: could not run the user-table gate query (%s): %s", + errs.ErrBackupVerifyFailed, err, strings.TrimSpace(string(output))) + } slog.Warn("smoke query error", "label", sq.Label, "error", err) failedQueries++ continue diff --git a/internal/config/backup_s3_alias_test.go b/internal/config/backup_s3_alias_test.go new file mode 100644 index 00000000..24ebb2b6 --- /dev/null +++ b/internal/config/backup_s3_alias_test.go @@ -0,0 +1,71 @@ +package config + +// Purpose: Regression tests for the BACKUP_ACCESS_KEY/BACKUP_SECRET_KEY alias +// onto BACKUP_S3_ACCESS_KEY_ID/BACKUP_S3_SECRET_ACCESS_KEY. +// +// Background: ntask/backend/.env.example configures remote backup upload +// with BACKUP_ACCESS_KEY / BACKUP_SECRET_KEY (alongside BACKUP_S3_ENDPOINT), +// but loader_parse_env_ops.go only ever read the longer +// BACKUP_S3_ACCESS_KEY_ID / BACKUP_S3_SECRET_ACCESS_KEY names. The shorter +// names were on loader_known_vars_ops.go's list (so no "unknown env var" +// warning fired) but never reached cfg.Backup.S3AccessKeyID/S3SecretAccessKey +// — remote upload was silently non-functional. Mirrors the same shape as +// minio_alias_test.go's MINIO_ACCESS_KEY/MINIO_SECRET_KEY fix. +// +// Inputs: Simulated .env cascade values via t.Setenv. +// Outputs: none (t.Error/t.Fatal on assertion failure). + +import "testing" + +// TestBackupS3Alias_ShortNamesMapToCanonicalFields verifies BACKUP_ACCESS_KEY +// / BACKUP_SECRET_KEY populate S3AccessKeyID/S3SecretAccessKey when the +// canonical BACKUP_S3_* names are not set. +func TestBackupS3Alias_ShortNamesMapToCanonicalFields(t *testing.T) { + t.Setenv("BACKUP_S3_ACCESS_KEY_ID", "") + t.Setenv("BACKUP_S3_SECRET_ACCESS_KEY", "") + t.Setenv("BACKUP_ACCESS_KEY", "r2-access-key") + t.Setenv("BACKUP_SECRET_KEY", "r2-secret-key") + + cfg := parseEnvToConfig() + if cfg.Backup.S3AccessKeyID != "r2-access-key" { + t.Errorf("Backup.S3AccessKeyID = %q, want %q (aliased from BACKUP_ACCESS_KEY)", cfg.Backup.S3AccessKeyID, "r2-access-key") + } + if cfg.Backup.S3SecretAccessKey != "r2-secret-key" { + t.Errorf("Backup.S3SecretAccessKey = %q, want %q (aliased from BACKUP_SECRET_KEY)", cfg.Backup.S3SecretAccessKey, "r2-secret-key") + } +} + +// TestBackupS3Alias_CanonicalNamesWin verifies BACKUP_S3_ACCESS_KEY_ID / +// BACKUP_S3_SECRET_ACCESS_KEY take priority when both the canonical and +// alias vars are set. +func TestBackupS3Alias_CanonicalNamesWin(t *testing.T) { + t.Setenv("BACKUP_S3_ACCESS_KEY_ID", "canonical-access") + t.Setenv("BACKUP_S3_SECRET_ACCESS_KEY", "canonical-secret") + t.Setenv("BACKUP_ACCESS_KEY", "alias-access") + t.Setenv("BACKUP_SECRET_KEY", "alias-secret") + + cfg := parseEnvToConfig() + if cfg.Backup.S3AccessKeyID != "canonical-access" { + t.Errorf("Backup.S3AccessKeyID = %q, want %q (canonical must win)", cfg.Backup.S3AccessKeyID, "canonical-access") + } + if cfg.Backup.S3SecretAccessKey != "canonical-secret" { + t.Errorf("Backup.S3SecretAccessKey = %q, want %q (canonical must win)", cfg.Backup.S3SecretAccessKey, "canonical-secret") + } +} + +// TestBackupS3Alias_NeitherSet_StaysEmpty verifies no credentials invented +// from thin air when nothing at all is set. +func TestBackupS3Alias_NeitherSet_StaysEmpty(t *testing.T) { + t.Setenv("BACKUP_S3_ACCESS_KEY_ID", "") + t.Setenv("BACKUP_S3_SECRET_ACCESS_KEY", "") + t.Setenv("BACKUP_ACCESS_KEY", "") + t.Setenv("BACKUP_SECRET_KEY", "") + + cfg := parseEnvToConfig() + if cfg.Backup.S3AccessKeyID != "" { + t.Errorf("Backup.S3AccessKeyID = %q, want empty", cfg.Backup.S3AccessKeyID) + } + if cfg.Backup.S3SecretAccessKey != "" { + t.Errorf("Backup.S3SecretAccessKey = %q, want empty", cfg.Backup.S3SecretAccessKey) + } +} diff --git a/internal/config/loader_known_vars_ops.go b/internal/config/loader_known_vars_ops.go index f9bdaebc..c87a7ecc 100644 --- a/internal/config/loader_known_vars_ops.go +++ b/internal/config/loader_known_vars_ops.go @@ -30,8 +30,14 @@ var knownEnvVarsOps = []string{ "BACKUP_S3_REGION", "BACKUP_S3_ENDPOINT", "BACKUP_CRITICAL_TABLES", - // App-level backup credential/target aliases seen in real .env files - // (e.g. ntask). Not read by the CLI loader (app backup scripts use them). + // BACKUP_ACCESS_KEY / BACKUP_SECRET_KEY: shorter aliases for + // BACKUP_S3_ACCESS_KEY_ID / BACKUP_S3_SECRET_ACCESS_KEY, used verbatim by + // real project .env files (e.g. ntask/backend/.env.example, R2-backed). + // These were previously "known" (no warning) but silently unread — remote + // backup upload configured this way did nothing. loader_parse_env_ops.go + // now accepts either name via firstNonEmpty(), canonical name winning if + // both are set. BACKUP_S3_BUCKET / BACKUP_S3_PREFIX remain app-level + // (consumed by app backup scripts, not this CLI's config struct). "BACKUP_ACCESS_KEY", "BACKUP_SECRET_KEY", "BACKUP_S3_BUCKET", diff --git a/internal/config/loader_parse_env_ops.go b/internal/config/loader_parse_env_ops.go index adf339c7..7c1903d0 100644 --- a/internal/config/loader_parse_env_ops.go +++ b/internal/config/loader_parse_env_ops.go @@ -76,11 +76,20 @@ func parseEnvOps(cfg *Config) { RetentionMonthly: getEnvInt("BACKUP_RETENTION_MONTHLY", 0), RestoreTestSchedule: os.Getenv("BACKUP_RESTORE_TEST_SCHEDULE"), AlertOnFailure: getEnvBool("BACKUP_ALERT_ON_FAILURE", true), - S3AccessKeyID: os.Getenv("BACKUP_S3_ACCESS_KEY_ID"), - S3SecretAccessKey: os.Getenv("BACKUP_S3_SECRET_ACCESS_KEY"), - S3Region: os.Getenv("BACKUP_S3_REGION"), - S3Endpoint: os.Getenv("BACKUP_S3_ENDPOINT"), - CriticalTables: os.Getenv("BACKUP_CRITICAL_TABLES"), + // S3AccessKeyID/S3SecretAccessKey: BACKUP_S3_ACCESS_KEY_ID / + // BACKUP_S3_SECRET_ACCESS_KEY is the canonical pair, but real project + // .env files (ntask/backend/.env.example, R2-backed) use the shorter + // BACKUP_ACCESS_KEY / BACKUP_SECRET_KEY names alongside + // BACKUP_S3_ENDPOINT — those were on loader_known_vars_ops.go's list + // (so no "unknown env var" warning fired) but never actually read + // into this struct, so a project configured that way had remote + // backup upload silently do nothing. Both names are accepted here; + // the canonical BACKUP_S3_* pair wins if both happen to be set. + S3AccessKeyID: firstNonEmpty(os.Getenv("BACKUP_S3_ACCESS_KEY_ID"), os.Getenv("BACKUP_ACCESS_KEY")), + S3SecretAccessKey: firstNonEmpty(os.Getenv("BACKUP_S3_SECRET_ACCESS_KEY"), os.Getenv("BACKUP_SECRET_KEY")), + S3Region: os.Getenv("BACKUP_S3_REGION"), + S3Endpoint: os.Getenv("BACKUP_S3_ENDPOINT"), + CriticalTables: os.Getenv("BACKUP_CRITICAL_TABLES"), } cfg.DR = DRConfig{ @@ -142,3 +151,16 @@ func parseEnvOps(cfg *Config) { cfg.SkipHealthChecks = getEnvBool("NSELF_SKIP_HEALTH_CHECKS", false) cfg.StopTimeout = getEnvInt("NSELF_STOP_TIMEOUT", 0) } + +// firstNonEmpty returns the first non-empty string among values, or "" if +// all are empty. Used for accepting an alias env var name alongside a +// canonical one without silently preferring whichever happens to be read +// last. +func firstNonEmpty(values ...string) string { + for _, v := range values { + if v != "" { + return v + } + } + return "" +} diff --git a/internal/database/restore_drill.go b/internal/database/restore_drill.go index 7b5005cc..0d17882a 100644 --- a/internal/database/restore_drill.go +++ b/internal/database/restore_drill.go @@ -110,6 +110,26 @@ func RestoreDrill(ctx context.Context, cfg *config.Config, backupFile string) (R } result.MissingCriticalTables = missing + // Smoke gate: RestoreDrillResult.Success must be impossible to set true + // without a positive assertion behind it — this cannot be left to whoever + // calls RestoreDrill(). Before this check lived only in drill.go's + // smokeCheck, reached exclusively through the Drill() wrapper; anything + // that calls RestoreDrill() directly (cmd/commands/db_pitr_ops.go's + // `nself db restore-drill`, and any future caller) got a RestoreDrillResult + // with Success unconditionally true regardless of TablesVerified/ + // RowsVerified. That is the same hollow-gate shape proven live on staging + // 2026-08-31 (tables_checked=107, rows_observed=0, success=true), just one + // call-frame lower — reusing smokeCheck here so the invariant lives at the + // one place that actually produces the result, not at every caller. + if smokeErr := smokeCheck(result, ResolveCriticalTables(cfg)); smokeErr != nil { + _ = runSQLOnDB(ctx, cfg, "postgres", "DROP DATABASE IF EXISTS "+quotedDrillDB) + result.ErrorMessage = smokeErr.Error() + result.CompletedAt = time.Now() + result.Duration = result.CompletedAt.Sub(result.StartedAt) + _ = RecordDrillResult(".", result) + return result, fmt.Errorf("restore drill smoke check: %w", smokeErr) + } + // Drop the drill database. if err := runSQLOnDB(ctx, cfg, "postgres", "DROP DATABASE IF EXISTS "+quotedDrillDB); err != nil { return result, fmt.Errorf("drop drill database %s: %w", drillDB, err) @@ -126,103 +146,6 @@ func RestoreDrill(ctx context.Context, cfg *config.Config, backupFile string) (R return result, nil } -// VerifyRestoredDatabase connects to a restored database and runs basic -// integrity checks: counts rows in key tables, verifies pg_catalog consistency. -func VerifyRestoredDatabase(ctx context.Context, cfg *config.Config, drillDB string) (tables int, rows int64, err error) { - // Count user tables. - tableCountSQL := `SELECT count(*) FROM information_schema.tables WHERE table_schema NOT IN ('pg_catalog','information_schema')` - out, err := querySQL(ctx, cfg, drillDB, tableCountSQL) - if err != nil { - return 0, 0, fmt.Errorf("count tables in %s: %w", drillDB, err) - } - out = strings.TrimSpace(out) - tableCount := 0 - if out != "" { - if _, scanErr := fmt.Sscanf(out, "%d", &tableCount); scanErr != nil { - return 0, 0, fmt.Errorf("parse table count %q: %w", out, scanErr) - } - } - - // Exact total row count across ALL user tables, in one round trip. - // - // The prior approach sampled up to 10 tables (LIMIT 10, no ORDER BY) and - // summed their row counts. On a schema with many tables that sample could - // land entirely on empty ones and report zero rows for a perfectly good - // restore — a false fail. It could just as easily miss the tables that - // actually hold data and report zero for the opposite reason: a false - // pass. Neither is acceptable once the caller (drill.go) starts asserting - // on this number. - // - // query_to_xml lets Postgres build and run one dynamic COUNT(*) per table - // server-side and return all the results in a single query. format('%I.%I', ...) - // is Postgres's own identifier quoting (SEC-SQL-01: table_schema/table_name - // here are DB-sourced and must never be interpolated by the Go side without - // it — %I does that quoting inside the server, so no client-side - // SanitizeIdentifier call is needed for this query). - rowTotalSQL := `SELECT COALESCE(SUM((xpath('/row/c/text()', ` + - `query_to_xml(format('SELECT count(*) AS c FROM %I.%I', table_schema, table_name), false, true, '')` + - `))[1]::text::bigint), 0) FROM information_schema.tables ` + - `WHERE table_schema NOT IN ('pg_catalog','information_schema')` - rowOut, err := querySQL(ctx, cfg, drillDB, rowTotalSQL) - if err != nil { - return tableCount, 0, fmt.Errorf("count total rows in %s: %w", drillDB, err) - } - rowOut = strings.TrimSpace(rowOut) - var totalRows int64 - if rowOut != "" { - if _, scanErr := fmt.Sscanf(rowOut, "%d", &totalRows); scanErr != nil { - return tableCount, 0, fmt.Errorf("parse row total %q: %w", rowOut, scanErr) - } - } - - // Verify pg_catalog is accessible by probing a relation size. - catalogSQL := `SELECT pg_catalog.pg_relation_size(c.oid) FROM pg_catalog.pg_class c WHERE c.relname = 'pg_class' LIMIT 1` - if _, catErr := querySQL(ctx, cfg, drillDB, catalogSQL); catErr != nil { - return tableCount, totalRows, fmt.Errorf("pg_catalog probe failed in %s: %w", drillDB, catErr) - } - - return tableCount, totalRows, nil -} - -// verifyCriticalTables reports which entries of ResolveCriticalTables(cfg) -// (drill.go — DefaultCriticalTables unless the project sets -// BACKUP_CRITICAL_TABLES) are missing by name, in any schema, from drillDB. -// The resolved names are project config, not DB-sourced input, so they are -// safe to place directly into a SQL literal list here (properly -// single-quote-escaped below) — SEC-SQL-01's "never interpolate DB-sourced -// identifiers without quoting" concerns table_schema/table_name values read -// back FROM the database (handled via format('%I.%I') in -// VerifyRestoredDatabase above), not our own resolved string-literal values. -func verifyCriticalTables(ctx context.Context, cfg *config.Config, drillDB string) ([]string, error) { - criticalTables := ResolveCriticalTables(cfg) - literals := make([]string, len(criticalTables)) - for i, name := range criticalTables { - literals[i] = "'" + strings.ReplaceAll(name, "'", "''") + "'" - } - sqlText := fmt.Sprintf( - `SELECT DISTINCT table_name FROM information_schema.tables WHERE table_name = ANY(ARRAY[%s])`, - strings.Join(literals, ","), - ) - out, err := querySQL(ctx, cfg, drillDB, sqlText) - if err != nil { - return nil, fmt.Errorf("query critical tables in %s: %w", drillDB, err) - } - present := make(map[string]bool, len(criticalTables)) - for _, line := range strings.Split(out, "\n") { - line = strings.TrimSpace(line) - if line != "" { - present[line] = true - } - } - var missing []string - for _, name := range criticalTables { - if !present[name] { - missing = append(missing, name) - } - } - return missing, nil -} - // RecordDrillResult appends the drill result to .nself/restore-drills.log // in JSON format (one JSON object per line). func RecordDrillResult(projectDir string, result RestoreDrillResult) error { diff --git a/internal/database/restore_drill_integration_test.go b/internal/database/restore_drill_integration_test.go new file mode 100644 index 00000000..e30e8957 --- /dev/null +++ b/internal/database/restore_drill_integration_test.go @@ -0,0 +1,155 @@ +//go:build integration + +// Package database integration tests for RestoreDrill against a REAL, +// multi-schema Postgres fixture — the exact shape that hid the production +// defect: a drill that verifies zero rows must never report success, on +// ANY caller of RestoreDrill(), not only the ones that go through Drill()'s +// smokeCheck wrapper. +// +// Background: `nself db restore-drill` (cmd/commands/db_pitr_ops.go) calls +// database.RestoreDrill() directly and trusts RestoreDrillResult.Success. +// Before this fix, RestoreDrill() set Success = true unconditionally once +// the restore + verify + critical-table steps returned no Go error — it +// never looked at RowsVerified itself; only Drill()'s separate smokeCheck +// call did. A drill run through `nself db restore-drill` on a multi-schema +// stack (tables spread beyond `public`, like nself-web: telemetry_events, +// plugin_downloads, provider_requests) could restore genuinely nothing and +// still print "Drill status: PASS". These tests reproduce that shape +// end-to-end against a real Postgres container rather than only unit-testing +// the pure smokeCheck helper. +// +// Run with: +// +// INTEGRATION=1 go test -mod=vendor -tags integration -timeout 180s \ +// ./internal/database/... -run TestRestoreDrill_Integration +package database + +import ( + "context" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +// fixtureSchemaStmts creates a multi-schema layout — tables split across +// `public` and `app_data`, mirroring nself-web's shape where data lives +// outside the `public` schema alone. withData controls whether the tables +// are populated: the zero-row regression test needs an otherwise-identical, +// otherwise-healthy restore (same table count, same schemas) that simply has +// no rows, so the only variable under test is RowsVerified. +func fixtureSchemaStmts(withData bool) []string { + stmts := []string{ + "CREATE SCHEMA IF NOT EXISTS app_data", + "CREATE TABLE public.telemetry_events (id serial primary key, payload text)", + "CREATE TABLE public.plugin_downloads (id serial primary key, name text)", + "CREATE TABLE app_data.provider_requests (id serial primary key, note text)", + "CREATE TABLE app_data.np_users (id serial primary key, email text)", + "CREATE TABLE app_data.np_licenses (id serial primary key, key text)", + "CREATE TABLE app_data.np_audit_log (id serial primary key, action text)", + "CREATE TABLE app_data.np_plugins (id serial primary key, slug text)", + "CREATE TABLE app_data.np_billing (id serial primary key, amount int)", + } + if withData { + stmts = append(stmts, + "INSERT INTO public.telemetry_events (payload) SELECT 'evt-'||g FROM generate_series(1,50) g", + "INSERT INTO public.plugin_downloads (name) SELECT 'plugin-'||g FROM generate_series(1,20) g", + "INSERT INTO app_data.provider_requests (note) SELECT 'req-'||g FROM generate_series(1,10) g", + ) + } + return stmts +} + +// buildFixtureDump applies fixtureSchemaStmts to the running container's +// "nself" database, then pg_dumps it (custom format, matching every real +// backup produced by `nself backup create`) to a file under dir. Returns the +// dump file path RestoreDrill can consume exactly like a real backup. +func buildFixtureDump(t *testing.T, container, dir string, withData bool) string { + t.Helper() + + for _, stmt := range fixtureSchemaStmts(withData) { + out, err := exec.Command("docker", "exec", container, + "psql", "-U", "postgres", "-d", "nself", "-c", stmt).CombinedOutput() + if err != nil { + t.Fatalf("fixture setup %q: %v\n%s", stmt, err, out) + } + } + + dumpPath := filepath.Join(dir, "fixture.dump") + f, err := os.Create(dumpPath) + if err != nil { + t.Fatalf("create dump file: %v", err) + } + defer func() { _ = f.Close() }() + + cmd := exec.Command("docker", "exec", container, "pg_dump", "-U", "postgres", "-d", "nself", "-Fc") + cmd.Stdout = f + var stderr strings.Builder + cmd.Stderr = &stderr + if err := cmd.Run(); err != nil { + t.Fatalf("pg_dump: %v: %s", err, stderr.String()) + } + return dumpPath +} + +// TestRestoreDrill_Integration_MultiSchemaRowsCounted proves the healthy +// case still works: a restore whose data spans more than one schema must be +// recognized (tables + rows both counted across every non-system schema, not +// just `public`), and RestoreDrill must report success. +func TestRestoreDrill_Integration_MultiSchemaRowsCounted(t *testing.T) { + skipUnlessIntegration(t) + cfg := startTestPostgres(t) + container := containerName(cfg) + + dumpPath := buildFixtureDump(t, container, t.TempDir(), true /* withData */) + + result, err := RestoreDrill(context.Background(), cfg, dumpPath) + if err != nil { + t.Fatalf("RestoreDrill: unexpected error for a real multi-schema restore: %v (result: %+v)", err, result) + } + if !result.Success { + t.Fatalf("RestoreDrill.Success = false for a real multi-schema restore, want true (result: %+v)", result) + } + if result.RowsVerified != 80 { // 50 + 20 + 10, per fixtureSchemaStmts + t.Errorf("RestoreDrill.RowsVerified = %d, want 80 (rows must be summed across ALL schemas, not just public)", result.RowsVerified) + } + if result.TablesVerified < 8 { + t.Errorf("RestoreDrill.TablesVerified = %d, want >= 8 (tables span public + app_data)", result.TablesVerified) + } +} + +// TestRestoreDrill_Integration_ZeroRowsNeverSucceeds is the regression lock +// for the production defect: a restore with real tables (well past the +// table-count floor) but zero rows in every one of them must NEVER report +// success — on RestoreDrill() itself, not only on the Drill() wrapper around +// it. This is exactly the code path `nself db restore-drill` +// (cmd/commands/db_pitr_ops.go) exercises directly. +// +// Before the fix in this commit, RestoreDrill() set Success = true +// unconditionally once restore/verify/critical-table steps returned no Go +// error, never consulting RowsVerified — this test fails against that code +// (result.Success == true, err == nil) and passes against the fix. +func TestRestoreDrill_Integration_ZeroRowsNeverSucceeds(t *testing.T) { + skipUnlessIntegration(t) + cfg := startTestPostgres(t) + container := containerName(cfg) + + dumpPath := buildFixtureDump(t, container, t.TempDir(), false /* schema-only, no rows */) + + result, err := RestoreDrill(context.Background(), cfg, dumpPath) + + if result.Success { + t.Fatalf("RestoreDrill.Success = true for a zero-row restore (tables=%d rows=%d) — a drill that verified zero rows must never report success", + result.TablesVerified, result.RowsVerified) + } + if err == nil { + t.Fatalf("RestoreDrill returned err == nil for a zero-row restore; want a non-nil error surfacing the hard fail") + } + if result.RowsVerified != 0 { + t.Errorf("RestoreDrill.RowsVerified = %d, want 0 for a schema-only restore", result.RowsVerified) + } + if result.ErrorMessage == "" { + t.Errorf("RestoreDrillResult.ErrorMessage is empty; want the zero-row smoke-check message recorded on the result") + } +} diff --git a/internal/database/restore_drill_verify.go b/internal/database/restore_drill_verify.go new file mode 100644 index 00000000..e86fc745 --- /dev/null +++ b/internal/database/restore_drill_verify.go @@ -0,0 +1,112 @@ +// Package database — restore_drill_verify.go: post-restore verification +// queries for RestoreDrill (VerifyRestoredDatabase, verifyCriticalTables). +// Split out of restore_drill.go (file-size ratchet, internal/repoqa) — this +// is the self-contained "what does the restored DB actually contain" half of +// the drill, following the same drill_*.go concern-split convention already +// used for drill_critical_tables.go. No behavior change. +package database + +import ( + "context" + "fmt" + "strings" + + "github.com/nself-org/cli/internal/config" +) + +// VerifyRestoredDatabase connects to a restored database and runs basic +// integrity checks: counts rows in key tables, verifies pg_catalog consistency. +func VerifyRestoredDatabase(ctx context.Context, cfg *config.Config, drillDB string) (tables int, rows int64, err error) { + // Count user tables. + tableCountSQL := `SELECT count(*) FROM information_schema.tables WHERE table_schema NOT IN ('pg_catalog','information_schema')` + out, err := querySQL(ctx, cfg, drillDB, tableCountSQL) + if err != nil { + return 0, 0, fmt.Errorf("count tables in %s: %w", drillDB, err) + } + out = strings.TrimSpace(out) + tableCount := 0 + if out != "" { + if _, scanErr := fmt.Sscanf(out, "%d", &tableCount); scanErr != nil { + return 0, 0, fmt.Errorf("parse table count %q: %w", out, scanErr) + } + } + + // Exact total row count across ALL user tables, in one round trip. + // + // The prior approach sampled up to 10 tables (LIMIT 10, no ORDER BY) and + // summed their row counts. On a schema with many tables that sample could + // land entirely on empty ones and report zero rows for a perfectly good + // restore — a false fail. It could just as easily miss the tables that + // actually hold data and report zero for the opposite reason: a false + // pass. Neither is acceptable once the caller (drill.go) starts asserting + // on this number. + // + // query_to_xml lets Postgres build and run one dynamic COUNT(*) per table + // server-side and return all the results in a single query. format('%I.%I', ...) + // is Postgres's own identifier quoting (SEC-SQL-01: table_schema/table_name + // here are DB-sourced and must never be interpolated by the Go side without + // it — %I does that quoting inside the server, so no client-side + // SanitizeIdentifier call is needed for this query). + rowTotalSQL := `SELECT COALESCE(SUM((xpath('/row/c/text()', ` + + `query_to_xml(format('SELECT count(*) AS c FROM %I.%I', table_schema, table_name), false, true, '')` + + `))[1]::text::bigint), 0) FROM information_schema.tables ` + + `WHERE table_schema NOT IN ('pg_catalog','information_schema')` + rowOut, err := querySQL(ctx, cfg, drillDB, rowTotalSQL) + if err != nil { + return tableCount, 0, fmt.Errorf("count total rows in %s: %w", drillDB, err) + } + rowOut = strings.TrimSpace(rowOut) + var totalRows int64 + if rowOut != "" { + if _, scanErr := fmt.Sscanf(rowOut, "%d", &totalRows); scanErr != nil { + return tableCount, 0, fmt.Errorf("parse row total %q: %w", rowOut, scanErr) + } + } + + // Verify pg_catalog is accessible by probing a relation size. + catalogSQL := `SELECT pg_catalog.pg_relation_size(c.oid) FROM pg_catalog.pg_class c WHERE c.relname = 'pg_class' LIMIT 1` + if _, catErr := querySQL(ctx, cfg, drillDB, catalogSQL); catErr != nil { + return tableCount, totalRows, fmt.Errorf("pg_catalog probe failed in %s: %w", drillDB, catErr) + } + + return tableCount, totalRows, nil +} + +// verifyCriticalTables reports which entries of ResolveCriticalTables(cfg) +// (drill.go — DefaultCriticalTables unless the project sets +// BACKUP_CRITICAL_TABLES) are missing by name, in any schema, from drillDB. +// The resolved names are project config, not DB-sourced input, so they are +// safe to place directly into a SQL literal list here (properly +// single-quote-escaped below) — SEC-SQL-01's "never interpolate DB-sourced +// identifiers without quoting" concerns table_schema/table_name values read +// back FROM the database (handled via format('%I.%I') in +// VerifyRestoredDatabase above), not our own resolved string-literal values. +func verifyCriticalTables(ctx context.Context, cfg *config.Config, drillDB string) ([]string, error) { + criticalTables := ResolveCriticalTables(cfg) + literals := make([]string, len(criticalTables)) + for i, name := range criticalTables { + literals[i] = "'" + strings.ReplaceAll(name, "'", "''") + "'" + } + sqlText := fmt.Sprintf( + `SELECT DISTINCT table_name FROM information_schema.tables WHERE table_name = ANY(ARRAY[%s])`, + strings.Join(literals, ","), + ) + out, err := querySQL(ctx, cfg, drillDB, sqlText) + if err != nil { + return nil, fmt.Errorf("query critical tables in %s: %w", drillDB, err) + } + present := make(map[string]bool, len(criticalTables)) + for _, line := range strings.Split(out, "\n") { + line = strings.TrimSpace(line) + if line != "" { + present[line] = true + } + } + var missing []string + for _, name := range criticalTables { + if !present[name] { + missing = append(missing, name) + } + } + return missing, nil +}