From c717a5959e039bd9deac9b012f37e40d0829a765 Mon Sep 17 00:00:00 2001 From: AJ ONeal Date: Thu, 9 Apr 2026 14:22:11 -0600 Subject: [PATCH] fix(sqlmigrate): address high-severity review findings - Collect: replace WalkDir with ReadDir to avoid subdirectory recursion and the risk of duplicate filenames silently overwriting each other - shmigrate: check scanner.Err() after reading migrations log - shmigrate: shell-quote all paths in generated scripts to prevent shell injection via migration filenames - shmigrate: wrap errors with ErrQueryApplied sentinel for consistency with DB backends - mustInit: remove stale err check that could never fire - mustInit: fix swapped error messages (read vs create) --- cmd/sql-migrate/main.go | 8 ++---- database/sqlmigrate/shmigrate/shmigrate.go | 17 ++++++++++--- database/sqlmigrate/sqlmigrate.go | 29 ++++++++++------------ 3 files changed, 28 insertions(+), 26 deletions(-) diff --git a/cmd/sql-migrate/main.go b/cmd/sql-migrate/main.go index 804d91a..ad72dcb 100644 --- a/cmd/sql-migrate/main.go +++ b/cmd/sql-migrate/main.go @@ -546,20 +546,16 @@ func mustInit(cfg *MainConfig) { entries, err := os.ReadDir(cfg.migrationsDir) if err != nil { if !os.IsNotExist(err) { - fmt.Fprintf(os.Stderr, "Error: init failed to create %q: %v\n", cfg.migrationsDir, err) + fmt.Fprintf(os.Stderr, "Error: init failed to read %q: %v\n", cfg.migrationsDir, err) os.Exit(1) } if err = os.MkdirAll(cfg.migrationsDir, 0755); err != nil { - fmt.Fprintf(os.Stderr, "Error: init failed to read %q: %v\n", cfg.migrationsDir, err) + fmt.Fprintf(os.Stderr, "Error: init failed to create %q: %v\n", cfg.migrationsDir, err) os.Exit(1) } } ups, downs := migrationsList(cfg.migrationsDir, entries) - if err != nil { - fmt.Fprintf(os.Stderr, "Error: init couldn't list existing migrations: %v\n", err) - os.Exit(1) - } mMigratorUpPath := filepath.Join(cfg.migrationsDir, M_MIGRATOR_UP_NAME) mMigratorDownPath := filepath.Join(cfg.migrationsDir, M_MIGRATOR_DOWN_NAME) diff --git a/database/sqlmigrate/shmigrate/shmigrate.go b/database/sqlmigrate/shmigrate/shmigrate.go index 2082ab5..2ea61fb 100644 --- a/database/sqlmigrate/shmigrate/shmigrate.go +++ b/database/sqlmigrate/shmigrate/shmigrate.go @@ -73,11 +73,11 @@ func (r *Migrator) ExecDown(ctx context.Context, m sqlmigrate.Migration, sql str } func (r *Migrator) exec(name, suffix, label string) error { - path := unclean(filepath.Join(r.MigrationsDir, name+suffix)) + path := shellQuote(unclean(filepath.Join(r.MigrationsDir, name+suffix))) cmd := strings.Replace(r.SqlCommand, "%s", path, 1) - syncCmd := strings.Replace(r.SqlCommand, "%s", unclean(r.LogQueryPath), 1) - logPath := unclean(r.LogPath) + syncCmd := strings.Replace(r.SqlCommand, "%s", shellQuote(unclean(r.LogQueryPath)), 1) + logPath := shellQuote(unclean(r.LogPath)) fmt.Fprintf(r.Writer, "# %s %s\n", label, name) if _, err := os.Stat(filepath.Join(r.MigrationsDir, name+suffix)); err != nil { @@ -109,7 +109,7 @@ func (r *Migrator) Applied(ctx context.Context) ([]sqlmigrate.Migration, error) if errors.Is(err, fs.ErrNotExist) { return nil, nil } - return nil, fmt.Errorf("reading migrations log: %w", err) + return nil, fmt.Errorf("%w: reading migrations log: %w", sqlmigrate.ErrQueryApplied, err) } defer func() { _ = f.Close() }() @@ -130,6 +130,9 @@ func (r *Migrator) Applied(ctx context.Context) ([]sqlmigrate.Migration, error) applied = append(applied, sqlmigrate.Migration{Name: line}) } } + if err := scanner.Err(); err != nil { + return nil, fmt.Errorf("%w: reading migrations log: %w", sqlmigrate.ErrQueryApplied, err) + } return applied, nil } @@ -140,6 +143,12 @@ func (r *Migrator) Reset() { r.counter = 0 } +// shellQuote wraps a string in single quotes with proper escaping, +// preventing shell injection in generated scripts. +func shellQuote(s string) string { + return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'" +} + // unclean ensures a relative path starts with ./ or ../ so it // is not interpreted as a command name in shell scripts. func unclean(path string) string { diff --git a/database/sqlmigrate/sqlmigrate.go b/database/sqlmigrate/sqlmigrate.go index 58ff60d..3e6e138 100644 --- a/database/sqlmigrate/sqlmigrate.go +++ b/database/sqlmigrate/sqlmigrate.go @@ -87,36 +87,33 @@ func Collect(fsys fs.FS, subpath string) ([]Script, error) { ups := map[string]string{} downs := map[string]string{} - err := fs.WalkDir(fsys, ".", func(path string, d fs.DirEntry, err error) error { - if err != nil { - return err - } + entries, err := fs.ReadDir(fsys, ".") + if err != nil { + return nil, fmt.Errorf("%w: %w", ErrWalkFailed, err) + } + + for _, d := range entries { if d.IsDir() { - return nil + continue } name := d.Name() if base, ok := strings.CutSuffix(name, ".up.sql"); ok { - b, readErr := fs.ReadFile(fsys, path) + b, readErr := fs.ReadFile(fsys, name) if readErr != nil { - return readErr + return nil, fmt.Errorf("%w: %w", ErrWalkFailed, readErr) } ups[base] = string(b) - return nil + continue } if base, ok := strings.CutSuffix(name, ".down.sql"); ok { - b, readErr := fs.ReadFile(fsys, path) + b, readErr := fs.ReadFile(fsys, name) if readErr != nil { - return readErr + return nil, fmt.Errorf("%w: %w", ErrWalkFailed, readErr) } downs[base] = string(b) - return nil + continue } - - return nil - }) - if err != nil { - return nil, fmt.Errorf("%w: %w", ErrWalkFailed, err) } var ddls []Script