diff --git a/backend/internal/atomicfile/syncdir.go b/backend/internal/atomicfile/syncdir.go new file mode 100644 index 00000000..f4a2cf78 --- /dev/null +++ b/backend/internal/atomicfile/syncdir.go @@ -0,0 +1,10 @@ +// Package atomicfile contains the platform-specific durability steps used by +// same-directory atomic file replacements. +package atomicfile + +// SyncDirectory makes a completed rename durable on platforms that support +// syncing directory handles. Platforms without that operation may implement +// this as a no-op after the renamed file itself has been synced. +func SyncDirectory(directory string) error { + return syncDirectory(directory) +} diff --git a/backend/internal/atomicfile/syncdir_nonwindows.go b/backend/internal/atomicfile/syncdir_nonwindows.go new file mode 100644 index 00000000..3f8e788c --- /dev/null +++ b/backend/internal/atomicfile/syncdir_nonwindows.go @@ -0,0 +1,18 @@ +//go:build !windows + +package atomicfile + +import "os" + +func syncDirectory(directory string) error { + directoryHandle, err := os.Open(directory) + if err != nil { + return err + } + syncErr := directoryHandle.Sync() + closeErr := directoryHandle.Close() + if syncErr != nil { + return syncErr + } + return closeErr +} diff --git a/backend/internal/atomicfile/syncdir_nonwindows_test.go b/backend/internal/atomicfile/syncdir_nonwindows_test.go new file mode 100644 index 00000000..7fa7ae78 --- /dev/null +++ b/backend/internal/atomicfile/syncdir_nonwindows_test.go @@ -0,0 +1,16 @@ +//go:build !windows + +package atomicfile + +import ( + "os" + "path/filepath" + "testing" +) + +func TestSyncDirectoryReturnsOpenErrors(t *testing.T) { + missing := filepath.Join(t.TempDir(), "missing") + if err := SyncDirectory(missing); !os.IsNotExist(err) { + t.Fatalf("SyncDirectory error = %v, want not-exist error", err) + } +} diff --git a/backend/internal/atomicfile/syncdir_test.go b/backend/internal/atomicfile/syncdir_test.go new file mode 100644 index 00000000..fce1f726 --- /dev/null +++ b/backend/internal/atomicfile/syncdir_test.go @@ -0,0 +1,9 @@ +package atomicfile + +import "testing" + +func TestSyncDirectoryAcceptsExistingDirectory(t *testing.T) { + if err := SyncDirectory(t.TempDir()); err != nil { + t.Fatal(err) + } +} diff --git a/backend/internal/atomicfile/syncdir_windows.go b/backend/internal/atomicfile/syncdir_windows.go new file mode 100644 index 00000000..ce3fc078 --- /dev/null +++ b/backend/internal/atomicfile/syncdir_windows.go @@ -0,0 +1,10 @@ +//go:build windows + +package atomicfile + +func syncDirectory(string) error { + // os.File.Sync uses FlushFileBuffers on Windows. Directory handles opened + // through os.Open cannot be flushed and return ERROR_ACCESS_DENIED, even + // though the preceding rename has already completed. + return nil +} diff --git a/backend/internal/backup/manager.go b/backend/internal/backup/manager.go index 0540e7e3..a91b31de 100644 --- a/backend/internal/backup/manager.go +++ b/backend/internal/backup/manager.go @@ -18,6 +18,7 @@ import ( "sync" "time" + "github.com/video-site/backend/internal/atomicfile" "github.com/video-site/backend/internal/catalog" "github.com/video-site/backend/internal/config" "github.com/video-site/backend/internal/localpath" @@ -1124,19 +1125,7 @@ func writeJSONAtomic(filePath string, value any, mode os.FileMode) error { return err } removeTemporary = false - directoryHandle, err := os.Open(directory) - if err != nil { - return err - } - syncErr := directoryHandle.Sync() - closeErr := directoryHandle.Close() - if syncErr != nil { - return syncErr - } - if closeErr != nil { - return closeErr - } - return nil + return atomicfile.SyncDirectory(directory) } func metaPath(archivePath string) string { diff --git a/backend/internal/backuptransfer/permissions_nonwindows.go b/backend/internal/backuptransfer/permissions_nonwindows.go new file mode 100644 index 00000000..6e02ccf1 --- /dev/null +++ b/backend/internal/backuptransfer/permissions_nonwindows.go @@ -0,0 +1,9 @@ +//go:build !windows + +package backuptransfer + +import "os" + +func stateFilePermissionsTooBroad(mode os.FileMode) bool { + return mode.Perm()&0o077 != 0 +} diff --git a/backend/internal/backuptransfer/permissions_nonwindows_test.go b/backend/internal/backuptransfer/permissions_nonwindows_test.go new file mode 100644 index 00000000..f2ead1ba --- /dev/null +++ b/backend/internal/backuptransfer/permissions_nonwindows_test.go @@ -0,0 +1,23 @@ +//go:build !windows + +package backuptransfer + +import ( + "os" + "testing" +) + +func TestStateFilePermissionsTooBroad(t *testing.T) { + for _, test := range []struct { + mode uint32 + broad bool + }{ + {mode: 0o600, broad: false}, + {mode: 0o640, broad: true}, + {mode: 0o606, broad: true}, + } { + if broad := stateFilePermissionsTooBroad(os.FileMode(test.mode)); broad != test.broad { + t.Fatalf("mode %#o broad = %t, want %t", test.mode, broad, test.broad) + } + } +} diff --git a/backend/internal/backuptransfer/permissions_windows.go b/backend/internal/backuptransfer/permissions_windows.go new file mode 100644 index 00000000..b0cc6dda --- /dev/null +++ b/backend/internal/backuptransfer/permissions_windows.go @@ -0,0 +1,12 @@ +//go:build windows + +package backuptransfer + +import "os" + +func stateFilePermissionsTooBroad(os.FileMode) bool { + // Windows FileMode permission bits are synthetic: writable files report + // 0666, and Chmod only changes the read-only attribute. The state directory + // ACL is the access-control boundary on this platform. + return false +} diff --git a/backend/internal/backuptransfer/permissions_windows_test.go b/backend/internal/backuptransfer/permissions_windows_test.go new file mode 100644 index 00000000..73d46671 --- /dev/null +++ b/backend/internal/backuptransfer/permissions_windows_test.go @@ -0,0 +1,14 @@ +//go:build windows + +package backuptransfer + +import ( + "os" + "testing" +) + +func TestStateFilePermissionsIgnoreSyntheticWindowsBits(t *testing.T) { + if stateFilePermissionsTooBroad(os.FileMode(0o666)) { + t.Fatal("synthetic Windows permission bits were rejected") + } +} diff --git a/backend/internal/backuptransfer/store.go b/backend/internal/backuptransfer/store.go index 454ff878..095f50f1 100644 --- a/backend/internal/backuptransfer/store.go +++ b/backend/internal/backuptransfer/store.go @@ -10,6 +10,7 @@ import ( "strings" "time" + "github.com/video-site/backend/internal/atomicfile" "github.com/video-site/backend/internal/backup" ) @@ -59,7 +60,7 @@ func readJSONFile(path string, destination any) error { if info.Mode()&os.ModeSymlink != 0 || !info.Mode().IsRegular() { return errors.New("backup transfer: state is not a regular file") } - if info.Mode().Perm()&0o077 != 0 { + if stateFilePermissionsTooBroad(info.Mode()) { return errors.New("backup transfer: state file permissions are too broad") } if info.Size() > maxStateFileBytes { @@ -119,16 +120,7 @@ func writeJSONAtomic(path string, value any) error { return err } removeTemporary = false - directoryHandle, err := os.Open(directory) - if err != nil { - return err - } - syncErr := directoryHandle.Sync() - closeErr := directoryHandle.Close() - if syncErr != nil { - return syncErr - } - return closeErr + return atomicfile.SyncDirectory(directory) } func validOpaqueID(value string) bool { diff --git a/backend/internal/config/config.go b/backend/internal/config/config.go index 29113380..051ee156 100644 --- a/backend/internal/config/config.go +++ b/backend/internal/config/config.go @@ -9,6 +9,7 @@ import ( "strings" "time" + "github.com/video-site/backend/internal/atomicfile" "github.com/video-site/backend/internal/localpath" "github.com/video-site/backend/internal/schedule" "gopkg.in/yaml.v3" @@ -493,10 +494,7 @@ func writeFileAtomically(path string, data []byte, mode os.FileMode) error { // Windows) reject syncing directory handles. The rename has already committed // at this point, so returning an error would falsely tell callers that the // save failed even though config.yaml was replaced. - if dirHandle, err := os.Open(dir); err == nil { - _ = dirHandle.Sync() - _ = dirHandle.Close() - } + _ = atomicfile.SyncDirectory(dir) return nil }