fix(storage): create config.db owner-only and secure database backups

internal/storage/bbolt.go

@@ -39,8 +39,10 @@ type BoltDB struct {
 func NewBoltDB(dataDir string, logger *zap.SugaredLogger) (*BoltDB, error) {
 	dbPath := filepath.Join(dataDir, "config.db")
 
-	// Try to open with timeout, if it fails, immediately return database locked error
-	db, err := bbolt.Open(dbPath, 0644, &bbolt.Options{
+	// Try to open with timeout, if it fails, immediately return database locked error.
+	// The database holds OAuth tokens and DCR client secrets, so it is created
+	// owner-only (0600).
+	db, err := bbolt.Open(dbPath, 0600, &bbolt.Options{
 		Timeout: 10 * time.Second,
 	})
 	if err != nil {
@@ -59,6 +61,10 @@ func NewBoltDB(dataDir string, logger *zap.SugaredLogger) (*BoltDB, error) {
 		return nil, fmt.Errorf("failed to open bolt database: %w", err)
 	}
 
+	// The mode passed to bbolt.Open only applies when the file is created, so
+	// databases created by older builds keep their 0644 mode. Tighten them here.
+	tightenFilePermissions(dbPath, logger)
+
 	boltDB := &BoltDB{
 		db:     db,
 		logger: logger,
@@ -73,6 +79,34 @@ func NewBoltDB(dataDir string, logger *zap.SugaredLogger) (*BoltDB, error) {
 	return boltDB, nil
 }
 
+// tightenFilePermissions clears group and other permission bits from path,
+// leaving owner bits untouched so a deliberately stricter mode is never widened.
+// It is best-effort and never prevents the proxy from starting: a mode that
+// cannot be tightened (read-only mount, exotic filesystem, Windows) is logged
+// at warn level (the default log level is info) rather than blocking startup,
+// because the file holds OAuth tokens and DCR client secrets and a failure
+// here means it silently stays group/world readable - that must be visible
+// to an operator without enabling debug logging.
+func tightenFilePermissions(path string, logger *zap.SugaredLogger) {
+	info, err := os.Stat(path)
+	if err != nil {
+		logger.Warnf("Could not stat %s to check file permissions: %v", path, err)
+		return
+	}
+
+	perm := info.Mode().Perm()
+	if perm&0o077 == 0 {
+		return
+	}
+
+	if err := os.Chmod(path, perm&^0o077); err != nil {
+		logger.Warnf("Could not tighten permissions on %s: %v", path, err)
+		return
+	}
+
+	logger.Infof("Tightened permissions on %s from %#o to %#o", path, perm, perm&^0o077)
+}
+
 // Close closes the database
 func (b *BoltDB) Close() error {
 	return b.db.Close()
@@ -663,11 +697,54 @@ func (b *BoltDB) DeleteServerPromptApprovals(serverName string) error {
 
 // Generic operations
 
-// Backup creates a backup of the database
+// Backup creates a backup of the database.
+// The copy carries the same secrets as the live database and the destination is
+// caller-chosen (potentially outside the 0700 data directory), so it is written
+// owner-only.
 func (b *BoltDB) Backup(destPath string) error {
-	return b.db.View(func(tx *bbolt.Tx) error {
-		return tx.CopyFile(destPath, 0644)
-	})
+	// bbolt's Tx.CopyFile opens the destination with O_CREATE|O_TRUNC, so its
+	// mode argument is ignored when the file already exists: writing straight to
+	// destPath would pour the database into whatever permissions a stale backup
+	// happened to carry, and a chmod afterwards comes too late. Stage the copy in
+	// an owner-only temporary file (os.CreateTemp creates it 0600) next to the
+	// destination, then rename it into place - the rename keeps the temp file's
+	// inode and so carries the 0600 mode with it (atomically on POSIX; Go makes
+	// no atomicity promise on Windows).
+	tmpFile, err := os.CreateTemp(filepath.Dir(destPath), ".config.db.backup-*")
+	if err != nil {
+		return fmt.Errorf("failed to create temporary backup file: %w", err)
+	}
+
+	tmpPath := tmpFile.Name()
+	renamed := false
+	defer func() {
+		tmpFile.Close() //nolint:errcheck // best-effort cleanup; Close below is the checked one
+		if !renamed {
+			os.Remove(tmpPath) //nolint:errcheck // best-effort cleanup
+		}
+	}()
+
+	if err := b.db.View(func(tx *bbolt.Tx) error {
+		_, writeErr := tx.WriteTo(tmpFile)
+		return writeErr
+	}); err != nil {
+		return fmt.Errorf("failed to write backup: %w", err)
+	}
+
+	if err := tmpFile.Sync(); err != nil {
+		return fmt.Errorf("failed to flush backup: %w", err)
+	}
+
+	if err := tmpFile.Close(); err != nil {
+		return fmt.Errorf("failed to close backup: %w", err)
+	}
+
+	if err := os.Rename(tmpPath, destPath); err != nil {
+		return fmt.Errorf("failed to move backup into place: %w", err)
+	}
+	renamed = true
+
+	return nil
 }
 
 // Stats returns database statistics

internal/storage/bbolt_file_mode_test.go

@@ -0,0 +1,204 @@
+package storage
+
+import (
+	"os"
+	"os/exec"
+	"path/filepath"
+	"runtime"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/require"
+	"go.etcd.io/bbolt"
+	"go.uber.org/zap"
+	"go.uber.org/zap/zapcore"
+	"go.uber.org/zap/zaptest/observer"
+)
+
+// skipOnWindows guards the permission assertions below: os.Chmod on Windows only
+// toggles the read-only bit and os.Stat reports 0666, so POSIX mode bits are
+// meaningless there. CI runs the unit tests on Windows too.
+func skipOnWindows(t *testing.T) {
+	t.Helper()
+	if runtime.GOOS == "windows" {
+		t.Skip("POSIX file mode bits are not meaningful on Windows")
+	}
+}
+
+// TestNewBoltDBCreatesDatabaseOwnerOnly pins that a freshly created config.db is
+// owner-only: it holds OAuth access/refresh tokens and DCR client secrets.
+func TestNewBoltDBCreatesDatabaseOwnerOnly(t *testing.T) {
+	skipOnWindows(t)
+
+	dir := t.TempDir()
+	db, err := NewBoltDB(dir, zap.NewNop().Sugar())
+	require.NoError(t, err)
+	defer db.Close()
+
+	info, err := os.Stat(filepath.Join(dir, "config.db"))
+	require.NoError(t, err)
+	require.Equal(t, os.FileMode(0o600), info.Mode().Perm(),
+		"a freshly created config.db must not be group/world readable")
+}
+
+// TestNewBoltDBTightensWorldReadableDatabaseOnOpen covers existing installs:
+// bbolt.Open's mode argument applies only at creation, so an already-created
+// 0644 database must be chmod-migrated on open.
+func TestNewBoltDBTightensWorldReadableDatabaseOnOpen(t *testing.T) {
+	skipOnWindows(t)
+
+	dir := t.TempDir()
+	dbPath := filepath.Join(dir, "config.db")
+
+	db, err := NewBoltDB(dir, zap.NewNop().Sugar())
+	require.NoError(t, err)
+	require.NoError(t, db.Close())
+
+	// Simulate a database created by an older build.
+	require.NoError(t, os.Chmod(dbPath, 0o644))
+
+	db2, err := NewBoltDB(dir, zap.NewNop().Sugar())
+	require.NoError(t, err)
+	require.NoError(t, db2.Close())
+
+	info, err := os.Stat(dbPath)
+	require.NoError(t, err)
+	require.Equal(t, os.FileMode(0o600), info.Mode().Perm(),
+		"an existing world-readable config.db must be tightened on open")
+}
+
+// TestNewBoltDBPreservesOwnerBitsWhenTightening makes sure the migration only
+// clears group/other bits instead of assigning 0600 outright, so it can never
+// widen a mode the owner chose.
+func TestNewBoltDBPreservesOwnerBitsWhenTightening(t *testing.T) {
+	skipOnWindows(t)
+
+	dir := t.TempDir()
+	dbPath := filepath.Join(dir, "config.db")
+
+	db, err := NewBoltDB(dir, zap.NewNop().Sugar())
+	require.NoError(t, err)
+	require.NoError(t, db.Close())
+
+	require.NoError(t, os.Chmod(dbPath, 0o744))
+
+	db2, err := NewBoltDB(dir, zap.NewNop().Sugar())
+	require.NoError(t, err)
+	require.NoError(t, db2.Close())
+
+	info, err := os.Stat(dbPath)
+	require.NoError(t, err)
+	require.Equal(t, os.FileMode(0o700), info.Mode().Perm(),
+		"only group/other bits should be cleared; owner bits must be preserved")
+}
+
+// TestBackupWritesOwnerOnlyCopy pins the backup copy: the destination is an
+// operator-chosen path that may sit outside the 0700 data directory, where a
+// 0644 copy really would be world-readable.
+func TestBackupWritesOwnerOnlyCopy(t *testing.T) {
+	skipOnWindows(t)
+
+	db, err := NewBoltDB(t.TempDir(), zap.NewNop().Sugar())
+	require.NoError(t, err)
+	defer db.Close()
+
+	backupPath := filepath.Join(t.TempDir(), "backup.db")
+	require.NoError(t, db.Backup(backupPath))
+
+	info, err := os.Stat(backupPath)
+	require.NoError(t, err)
+	require.Equal(t, os.FileMode(0o600), info.Mode().Perm(),
+		"database backups must not be group/world readable")
+
+	// The copy must still be a usable database, not just a well-permissioned file.
+	restored, err := bbolt.Open(backupPath, 0600, &bbolt.Options{
+		ReadOnly: true,
+		Timeout:  5 * time.Second,
+	})
+	require.NoError(t, err)
+	require.NoError(t, restored.Close())
+}
+
+// TestBackupTightensExistingDestination covers overwriting an earlier backup:
+// bbolt's CopyFile opens the destination with O_CREATE|O_TRUNC, so its mode
+// argument applies only when the destination does not already exist. The backup
+// must not be written into a pre-existing world-readable file at all, so it is
+// staged in an owner-only temporary file and renamed into place - which means
+// the old destination inode is replaced rather than truncated and rewritten.
+func TestBackupTightensExistingDestination(t *testing.T) {
+	skipOnWindows(t)
+
+	db, err := NewBoltDB(t.TempDir(), zap.NewNop().Sugar())
+	require.NoError(t, err)
+	defer db.Close()
+
+	destDir := t.TempDir()
+	backupPath := filepath.Join(destDir, "backup.db")
+	// A backup left behind by an older build.
+	require.NoError(t, os.WriteFile(backupPath, []byte("stale"), 0o644))
+
+	before, err := os.Stat(backupPath)
+	require.NoError(t, err)
+
+	require.NoError(t, db.Backup(backupPath))
+
+	after, err := os.Stat(backupPath)
+	require.NoError(t, err)
+	require.Equal(t, os.FileMode(0o600), after.Mode().Perm(),
+		"overwriting an existing backup must also tighten its permissions")
+	require.False(t, os.SameFile(before, after),
+		"database bytes must never be written into the pre-existing, possibly world-readable file; "+
+			"stage in an owner-only temp file and rename over the destination")
+
+	entries, err := os.ReadDir(destDir)
+	require.NoError(t, err)
+	require.Len(t, entries, 1, "backup must not leave temporary files behind: %v", entries)
+}
+
+// TestTightenFilePermissionsLogsStatFailureAtWarn covers SEC-03 follow-up
+// review: the database holds OAuth tokens and DCR client secrets, so an
+// operator running at the project's default log level (Info) must be able to
+// see that the permission-tightening migration did not run, instead of the
+// failure disappearing into Debug output nobody has enabled.
+func TestTightenFilePermissionsLogsStatFailureAtWarn(t *testing.T) {
+	core, logs := observer.New(zapcore.DebugLevel)
+	logger := zap.New(core).Sugar()
+
+	tightenFilePermissions(filepath.Join(t.TempDir(), "does-not-exist.db"), logger)
+
+	entries := logs.FilterMessageSnippet("Could not stat").All()
+	require.Len(t, entries, 1, "a stat failure must be logged")
+	require.Equal(t, zapcore.WarnLevel, entries[0].Level,
+		"a stat failure must be visible at the project's default (Info) log level")
+}
+
+// TestTightenFilePermissionsLogsChmodFailureAtWarn reproduces one of the
+// review's named real-world causes (macOS uchg/schg flag) for a chmod that
+// cannot succeed even though the process owns the file: darwin's immutable
+// flag makes chmod fail with EPERM regardless of ownership.
+func TestTightenFilePermissionsLogsChmodFailureAtWarn(t *testing.T) {
+	skipOnWindows(t)
+	if runtime.GOOS != "darwin" {
+		t.Skip("uses chflags uchg to force an owner-proof chmod failure; darwin-only")
+	}
+
+	dir := t.TempDir()
+	path := filepath.Join(dir, "config.db")
+	require.NoError(t, os.WriteFile(path, []byte("x"), 0o644))
+
+	require.NoError(t, exec.Command("chflags", "uchg", path).Run())
+	defer func() {
+		_ = exec.Command("chflags", "nouchg", path).Run()
+	}()
+
+	core, logs := observer.New(zapcore.DebugLevel)
+	logger := zap.New(core).Sugar()
+
+	tightenFilePermissions(path, logger)
+
+	entries := logs.FilterMessageSnippet("Could not tighten permissions").All()
+	require.Len(t, entries, 1, "a chmod failure must be logged")
+	require.Equal(t, zapcore.WarnLevel, entries[0].Level,
+		"a chmod failure must be visible at the project's default (Info) log level, "+
+			"since it means the DB is left silently world-readable")
+}