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)
This commit is contained in:
AJ ONeal 2026-04-09 14:22:11 -06:00
parent 0c1eb1f125
commit 13a15b14db
No known key found for this signature in database
3 changed files with 28 additions and 26 deletions

View File

@ -546,20 +546,16 @@ func mustInit(cfg *MainConfig) {
entries, err := os.ReadDir(cfg.migrationsDir) entries, err := os.ReadDir(cfg.migrationsDir)
if err != nil { if err != nil {
if !os.IsNotExist(err) { 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) os.Exit(1)
} }
if err = os.MkdirAll(cfg.migrationsDir, 0755); err != nil { 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) os.Exit(1)
} }
} }
ups, downs := migrationsList(cfg.migrationsDir, entries) 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) mMigratorUpPath := filepath.Join(cfg.migrationsDir, M_MIGRATOR_UP_NAME)
mMigratorDownPath := filepath.Join(cfg.migrationsDir, M_MIGRATOR_DOWN_NAME) mMigratorDownPath := filepath.Join(cfg.migrationsDir, M_MIGRATOR_DOWN_NAME)

View File

@ -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 { 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) cmd := strings.Replace(r.SqlCommand, "%s", path, 1)
syncCmd := strings.Replace(r.SqlCommand, "%s", unclean(r.LogQueryPath), 1) syncCmd := strings.Replace(r.SqlCommand, "%s", shellQuote(unclean(r.LogQueryPath)), 1)
logPath := unclean(r.LogPath) logPath := shellQuote(unclean(r.LogPath))
fmt.Fprintf(r.Writer, "# %s %s\n", label, name) fmt.Fprintf(r.Writer, "# %s %s\n", label, name)
if _, err := os.Stat(filepath.Join(r.MigrationsDir, name+suffix)); err != nil { 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) { if errors.Is(err, fs.ErrNotExist) {
return nil, nil 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() }() 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}) 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 return applied, nil
} }
@ -140,6 +143,12 @@ func (r *Migrator) Reset() {
r.counter = 0 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 // unclean ensures a relative path starts with ./ or ../ so it
// is not interpreted as a command name in shell scripts. // is not interpreted as a command name in shell scripts.
func unclean(path string) string { func unclean(path string) string {

View File

@ -87,36 +87,33 @@ func Collect(fsys fs.FS, subpath string) ([]Script, error) {
ups := map[string]string{} ups := map[string]string{}
downs := map[string]string{} downs := map[string]string{}
err := fs.WalkDir(fsys, ".", func(path string, d fs.DirEntry, err error) error { entries, err := fs.ReadDir(fsys, ".")
if err != nil { if err != nil {
return err return nil, fmt.Errorf("%w: %w", ErrWalkFailed, err)
} }
for _, d := range entries {
if d.IsDir() { if d.IsDir() {
return nil continue
} }
name := d.Name() name := d.Name()
if base, ok := strings.CutSuffix(name, ".up.sql"); ok { if base, ok := strings.CutSuffix(name, ".up.sql"); ok {
b, readErr := fs.ReadFile(fsys, path) b, readErr := fs.ReadFile(fsys, name)
if readErr != nil { if readErr != nil {
return readErr return nil, fmt.Errorf("%w: %w", ErrWalkFailed, readErr)
} }
ups[base] = string(b) ups[base] = string(b)
return nil continue
} }
if base, ok := strings.CutSuffix(name, ".down.sql"); ok { if base, ok := strings.CutSuffix(name, ".down.sql"); ok {
b, readErr := fs.ReadFile(fsys, path) b, readErr := fs.ReadFile(fsys, name)
if readErr != nil { if readErr != nil {
return readErr return nil, fmt.Errorf("%w: %w", ErrWalkFailed, readErr)
} }
downs[base] = string(b) downs[base] = string(b)
return nil continue
} }
return nil
})
if err != nil {
return nil, fmt.Errorf("%w: %w", ErrWalkFailed, err)
} }
var ddls []Script var ddls []Script