fix(config): make a settings save durable, and keep it private

.changeset/durable-private-config-save.md

@@ -0,0 +1,5 @@
+---
+"ftw": patch
+---
+
+A settings save now survives a power cut, and stops leaving the config world-readable. `config.yaml` is the file the gateway boots from, and every save from the UI rewrote it with no fsync of the temp file and no fsync of the containing directory — a rename is only atomic for bytes that already reached the disk, so losing power mid-save could publish a truncated or zero-length config and leave an unattended gateway unbootable. The save now fsyncs the temp file, renames, then fsyncs the directory, and reports a failure instead of claiming a config was saved that the next power cut can still take away. The file is also written 0600 rather than 0644: it holds MQTT passwords, API keys and OAuth refresh tokens.

go/internal/config/config.go

@@ -13,7 +13,9 @@ import (
 	"net/url"
 	"os"
 	"path/filepath"
+	"runtime"
 	"strings"
+	"sync"
 	"time"
 
 	"github.com/srcfl/ftw/go/internal/optimizercontract"
@@ -1889,8 +1891,51 @@ func (c *Config) SiteMeterDriver() string {
 	return ""
 }
 
-// SaveAtomic writes config to disk via tmp-file + rename. Safe from partial writes.
+// configFileMode is owner-only because config.yaml carries MQTT passwords,
+// API keys and OAuth refresh tokens. Rename replaces the destination inode, so
+// whatever mode the temp file has is the mode the saved config ends up with.
+const configFileMode os.FileMode = 0o600
+
+// saveMu serializes config saves. The settings handlers do not hold a write
+// lock across a save, so two overlapping requests would otherwise both write
+// the shared temp path and rename half of each other's bytes over config.yaml.
+var saveMu sync.Mutex
+
+// durableWriter holds the two sync calls that make a save survive power loss.
+// They are fields so a test can prove the ordering and force a sync failure;
+// production always uses defaultDurableWriter.
+type durableWriter struct {
+	syncFile func(*os.File) error
+	syncDir  func(string) error
+}
+
+var defaultDurableWriter = durableWriter{
+	syncFile: (*os.File).Sync,
+	syncDir:  syncDir,
+}
+
+// syncDir fsyncs a directory so a completed rename survives power loss.
+// Best-effort on platforms where directories can't be fsynced (Windows).
+func syncDir(dir string) error {
+	d, err := os.Open(dir)
+	if err != nil {
+		return err
+	}
+	defer d.Close()
+	if err := d.Sync(); err != nil && runtime.GOOS != "windows" {
+		return err
+	}
+	return nil
+}
+
+// SaveAtomic writes config to disk via tmp-file + rename. Safe from partial
+// writes and from power loss: the temp file is fsynced before the rename and
+// the containing directory is fsynced after it.
 func SaveAtomic(path string, c *Config) error {
+	return saveAtomic(defaultDurableWriter, path, c)
+}
+
+func saveAtomic(w durableWriter, path string, c *Config) error {
 	// Driver paths are resolved to absolute-ish paths at Load() time.
 	// Convert them back to config-relative before writing so that
 	// repeated save cycles don't accumulate extra "../" prefixes.
@@ -1908,11 +1953,50 @@ func SaveAtomic(path string, c *Config) error {
 	if err != nil {
 		return fmt.Errorf("yaml marshal: %w", err)
 	}
+	saveMu.Lock()
+	defer saveMu.Unlock()
+
 	tmp := path + ".tmp"
-	if err := os.WriteFile(tmp, data, 0644); err != nil {
+	// Clear any temp left by an interrupted save, then create with O_EXCL.
+	// OpenFile only applies the mode when it creates the file, so reusing a
+	// stale 0644 temp would hand the secrets in config.yaml to every user on
+	// the box; O_EXCL also refuses to follow a symlink planted at that path.
+	if err := os.Remove(tmp); err != nil && !errors.Is(err, os.ErrNotExist) {
+		return fmt.Errorf("clear stale tmp: %w", err)
+	}
+	f, err := os.OpenFile(tmp, os.O_WRONLY|os.O_CREATE|os.O_EXCL, configFileMode)
+	if err != nil {
+		return fmt.Errorf("create tmp: %w", err)
+	}
+	if _, err := f.Write(data); err != nil {
+		f.Close()
+		os.Remove(tmp)
 		return fmt.Errorf("write tmp: %w", err)
 	}
-	return os.Rename(tmp, path)
+	// fsync before rename: a rename is only atomic for bytes that have already
+	// reached the disk. Without this, a power cut mid-save can publish a
+	// truncated or zero-length config.yaml — the file the gateway boots from,
+	// on a device that is expected to come back up unattended.
+	if err := w.syncFile(f); err != nil {
+		f.Close()
+		os.Remove(tmp)
+		return fmt.Errorf("sync tmp: %w", err)
+	}
+	if err := f.Close(); err != nil {
+		os.Remove(tmp)
+		return fmt.Errorf("close tmp: %w", err)
+	}
+	if err := os.Rename(tmp, path); err != nil {
+		os.Remove(tmp)
+		return fmt.Errorf("rename tmp: %w", err)
+	}
+	// fsync the directory so the rename itself survives power loss. The
+	// caller's contract is "the config is now saved", so this failure is
+	// reported rather than swallowed.
+	if err := w.syncDir(filepath.Dir(path)); err != nil {
+		return fmt.Errorf("sync config dir: %w", err)
+	}
+	return nil
 }
 
 func relDriverPath(baseDir, p string) string {

go/internal/config/config_test.go

@@ -1,6 +1,7 @@
 package config
 
 import (
+	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -675,6 +676,168 @@ func TestSaveAtomicKeepsOutOfTreeDriverPathAbsolute(t *testing.T) {
 	}
 }
 
+// config.yaml holds MQTT passwords, API keys and OAuth refresh tokens. Rename
+// replaces the destination inode, so the temp file's mode is the mode the
+// operator ends up with — including when the config on disk was already
+// world-readable, or when an interrupted save left a world-readable temp
+// behind for the next save to reuse.
+func TestSaveAtomicWritesOwnerOnlyMode(t *testing.T) {
+	tests := []struct {
+		name string
+		prep func(t *testing.T, path string)
+	}{
+		{
+			name: "new config file",
+			prep: func(*testing.T, string) {},
+		},
+		{
+			name: "replacing a world-readable config",
+			prep: func(t *testing.T, path string) {
+				if err := os.WriteFile(path, []byte("site:\n  name: old\n"), 0o644); err != nil {
+					t.Fatal(err)
+				}
+			},
+		},
+		{
+			name: "stale world-readable tmp from an interrupted save",
+			prep: func(t *testing.T, path string) {
+				if err := os.WriteFile(path+".tmp", []byte("half a config"), 0o644); err != nil {
+					t.Fatal(err)
+				}
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			dir := t.TempDir()
+			path := filepath.Join(dir, "c.yaml")
+			c, err := Parse([]byte(minimalYAML), dir)
+			if err != nil {
+				t.Fatal(err)
+			}
+			tt.prep(t, path)
+			if err := SaveAtomic(path, c); err != nil {
+				t.Fatal(err)
+			}
+			info, err := os.Stat(path)
+			if err != nil {
+				t.Fatal(err)
+			}
+			if got := info.Mode().Perm(); got != 0o600 {
+				t.Errorf("saved config mode = %04o, want 0600 — the file holds MQTT passwords and OAuth refresh tokens", got)
+			}
+			if _, err := os.Stat(path + ".tmp"); !os.IsNotExist(err) {
+				t.Errorf("tmp file survived the save: %v", err)
+			}
+		})
+	}
+}
+
+// A rename is only atomic for bytes that already reached the disk, and the
+// rename itself only survives power loss once the directory entry is synced.
+// Both syncs must happen, and they must straddle the rename in that order.
+func TestSaveAtomicSyncsFileBeforeRenameAndDirAfter(t *testing.T) {
+	dir := t.TempDir()
+	path := filepath.Join(dir, "c.yaml")
+	c, err := Parse([]byte(minimalYAML), dir)
+	if err != nil {
+		t.Fatal(err)
+	}
+	saved := func() bool {
+		_, err := os.Stat(path)
+		return err == nil
+	}
+	var order []string
+	var savedAtFileSync, savedAtDirSync bool
+	w := durableWriter{
+		syncFile: func(f *os.File) error {
+			order = append(order, "file")
+			savedAtFileSync = saved()
+			return f.Sync()
+		},
+		syncDir: func(d string) error {
+			order = append(order, "dir")
+			savedAtDirSync = saved()
+			if d != dir {
+				t.Errorf("syncDir got %q, want the config's directory %q", d, dir)
+			}
+			return syncDir(d)
+		},
+	}
+	if err := saveAtomic(w, path, c); err != nil {
+		t.Fatal(err)
+	}
+	if got := strings.Join(order, ","); got != "file,dir" {
+		t.Fatalf("sync order = [%s], want [file,dir]", got)
+	}
+	if savedAtFileSync {
+		t.Error("the temp file was fsynced after the rename; a power cut could publish a truncated config")
+	}
+	if !savedAtDirSync {
+		t.Error("the directory was fsynced before the rename; the rename itself would not be durable")
+	}
+}
+
+// The caller's contract is "the config is now saved". A sync that fails must
+// not report success, or the settings UI tells the operator a change landed
+// that the next power cut can still take away.
+func TestSaveAtomicReportsSyncFailure(t *testing.T) {
+	syncFailed := errors.New("no space left on device")
+	const oldConfig = "site:\n  name: previous\n"
+	tests := []struct {
+		name        string
+		writer      durableWriter
+		keepsOldCfg bool
+	}{
+		{
+			name: "temp file sync fails",
+			writer: durableWriter{
+				syncFile: func(*os.File) error { return syncFailed },
+				syncDir:  syncDir,
+			},
+			// The rename never ran, so the config the gateway boots from is
+			// still the one that was there before.
+			keepsOldCfg: true,
+		},
+		{
+			name: "directory sync fails",
+			writer: durableWriter{
+				syncFile: (*os.File).Sync,
+				syncDir:  func(string) error { return syncFailed },
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			dir := t.TempDir()
+			path := filepath.Join(dir, "c.yaml")
+			c, err := Parse([]byte(minimalYAML), dir)
+			if err != nil {
+				t.Fatal(err)
+			}
+			if err := os.WriteFile(path, []byte(oldConfig), 0o600); err != nil {
+				t.Fatal(err)
+			}
+			err = saveAtomic(tt.writer, path, c)
+			if !errors.Is(err, syncFailed) {
+				t.Fatalf("saveAtomic error = %v, want it to report %v", err, syncFailed)
+			}
+			if _, err := os.Stat(path + ".tmp"); !os.IsNotExist(err) {
+				t.Errorf("tmp file survived a failed save: %v", err)
+			}
+			if tt.keepsOldCfg {
+				got, err := os.ReadFile(path)
+				if err != nil {
+					t.Fatal(err)
+				}
+				if string(got) != oldConfig {
+					t.Errorf("config on disk = %q, want the previous config left untouched", got)
+				}
+			}
+		})
+	}
+}
+
 func pretty(f float64) string {
 	return fmt.Sprintf("%g", f)
 }