diff --git a/platform/internal/db/postgres.go b/platform/internal/db/postgres.go index bc7039b8..a0d9cb7e 100644 --- a/platform/internal/db/postgres.go +++ b/platform/internal/db/postgres.go @@ -7,6 +7,7 @@ import ( "os" "path/filepath" "sort" + "strings" _ "github.com/lib/pq" ) @@ -29,11 +30,40 @@ func InitPostgres(databaseURL string) error { return nil } +// RunMigrations applies every forward migration file in migrationsDir on +// platform boot. +// +// Issue #211 — DO NOT glob `*.sql`. That matches both `.up.sql` and `.down.sql`, +// and sort.Strings orders "d" before "u", so every boot used to run the +// rollback BEFORE the forward migration for any pair, wiping data from any +// table the pair recreates (020_workspace_auth_tokens was the canary — every +// restart wiped live tokens, regressing AdminAuth to fail-open bypass for +// every subsequent request). +// +// The fix: only run files that are either `.up.sql` or plain `.sql` (legacy +// pre-pair migrations like 009_activity_logs.sql). Never touch `.down.sql` +// — those are intentional rollbacks, only to be run by operators manually +// via psql when a real rollback is required. +// +// NOTE: this runner still re-applies every migration on every boot. That +// works for idempotent `CREATE TABLE IF NOT EXISTS` + `ALTER TABLE ... IF NOT +// EXISTS` statements but means non-idempotent DDL will fail on restart. +// Migration authors must write idempotent SQL. A real schema_migrations +// tracking table would be better; tracked as follow-up. func RunMigrations(migrationsDir string) error { - files, err := filepath.Glob(filepath.Join(migrationsDir, "*.sql")) + allFiles, err := filepath.Glob(filepath.Join(migrationsDir, "*.sql")) if err != nil { return fmt.Errorf("glob migrations: %w", err) } + // Forward-only filter — skip *.down.sql explicitly. + files := make([]string, 0, len(allFiles)) + for _, f := range allFiles { + base := filepath.Base(f) + if strings.HasSuffix(base, ".down.sql") { + continue + } + files = append(files, f) + } sort.Strings(files) for _, f := range files { diff --git a/platform/internal/db/postgres_migrate_test.go b/platform/internal/db/postgres_migrate_test.go new file mode 100644 index 00000000..f575226d --- /dev/null +++ b/platform/internal/db/postgres_migrate_test.go @@ -0,0 +1,79 @@ +package db + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// Issue #211 regression: RunMigrations used to glob *.sql which caught both +// `.up.sql` and `.down.sql`. Alphabetical sort put `.down.sql` first so +// every platform boot ran the rollback followed by the forward, wiping any +// data the pair re-creates (workspace_auth_tokens was the canary). +// +// This test exercises the filter directly via filepath.Glob against a +// tmp dir of staged files. The real RunMigrations opens a DB connection +// so we can't run it end-to-end in a unit test, but the filtering step +// is where the bug was. + +func TestRunMigrations_SkipsDownSqlFiles(t *testing.T) { + tmp := t.TempDir() + + // Stage a realistic mix: legacy plain .sql (migration 009), plus a pair + // (up + down), plus a runaway .down.sql that shouldn't exist alone. + files := map[string]string{ + "009_legacy.sql": "-- legacy forward only\n", + "020_workspace_auth_tokens.up.sql": "CREATE TABLE workspace_auth_tokens ();\n", + "020_workspace_auth_tokens.down.sql": "DROP TABLE workspace_auth_tokens;\n", + "021_other.up.sql": "-- 21 forward\n", + "021_other.down.sql": "-- 21 rollback (must not run)\n", + } + for name, body := range files { + if err := os.WriteFile(filepath.Join(tmp, name), []byte(body), 0o644); err != nil { + t.Fatal(err) + } + } + + // Mirror the filter logic from RunMigrations. + allFiles, err := filepath.Glob(filepath.Join(tmp, "*.sql")) + if err != nil { + t.Fatal(err) + } + forward := make([]string, 0, len(allFiles)) + for _, f := range allFiles { + base := filepath.Base(f) + if strings.HasSuffix(base, ".down.sql") { + continue + } + forward = append(forward, base) + } + + // Assert: exactly 3 forward files, none end in .down.sql + if len(forward) != 3 { + t.Errorf("expected 3 forward migrations, got %d: %v", len(forward), forward) + } + for _, f := range forward { + if strings.HasSuffix(f, ".down.sql") { + t.Errorf("down migration leaked through filter: %s", f) + } + } + // Spot-check the ones that must be present + wantPresent := []string{ + "009_legacy.sql", + "020_workspace_auth_tokens.up.sql", + "021_other.up.sql", + } + for _, w := range wantPresent { + found := false + for _, f := range forward { + if f == w { + found = true + break + } + } + if !found { + t.Errorf("expected forward set to include %q, got %v", w, forward) + } + } +}