forked from molecule-ai/molecule-core
a507961f22
Closes #211 HIGH ops/security. RunMigrations globbed \`*.sql\` which matches both \`.up.sql\` AND \`.down.sql\`. Alphabetical sort puts \"d\" before \"u\", so every platform boot ran the rollback BEFORE the forward migration for any pair starting with migration 018. Net effect: every restart wiped workspace_auth_tokens (the 020 pair), which in turn regressed AdminAuth to its fail-open bootstrap bypass for every route protected by it — the live server was effectively unauthenticated from restart until the next workspace re-registered. Also wiped 018_secrets_encryption_version and 019_workspace_access pairs silently. Fix is a 3-line filter: skip files whose base name ends in \`.down.sql\`. Down migrations remain on disk for operator-driven rollback via psql, but are never picked up by the auto-run loop. Added unit test against a tmp dir to lock the filter behaviour so this can never regress: stages a mix of legacy plain .sql, matched up/down pairs, asserts only forward files survive. Follow-up (not in this PR): the runner still re-applies every migration on every boot. Migrations must be idempotent. A proper schema_migrations tracking table is tracked as a future cleanup. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
82 lines
2.4 KiB
Go
82 lines
2.4 KiB
Go
package db
|
|
|
|
import (
|
|
"database/sql"
|
|
"fmt"
|
|
"log"
|
|
"os"
|
|
"path/filepath"
|
|
"sort"
|
|
"strings"
|
|
|
|
_ "github.com/lib/pq"
|
|
)
|
|
|
|
var DB *sql.DB
|
|
|
|
func InitPostgres(databaseURL string) error {
|
|
var err error
|
|
DB, err = sql.Open("postgres", databaseURL)
|
|
if err != nil {
|
|
return fmt.Errorf("open postgres: %w", err)
|
|
}
|
|
DB.SetMaxOpenConns(25)
|
|
DB.SetMaxIdleConns(5)
|
|
|
|
if err := DB.Ping(); err != nil {
|
|
return fmt.Errorf("ping postgres: %w", err)
|
|
}
|
|
log.Println("Connected to Postgres")
|
|
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 {
|
|
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 {
|
|
log.Printf("Applying migration: %s", filepath.Base(f))
|
|
content, err := os.ReadFile(f)
|
|
if err != nil {
|
|
return fmt.Errorf("read %s: %w", f, err)
|
|
}
|
|
if _, err := DB.Exec(string(content)); err != nil {
|
|
return fmt.Errorf("exec %s: %w", filepath.Base(f), err)
|
|
}
|
|
}
|
|
log.Printf("Applied %d migrations", len(files))
|
|
return nil
|
|
}
|