From cce3311b5705457909db21514b852c6b6143f65b Mon Sep 17 00:00:00 2001 From: laggu91 Date: Thu, 19 Feb 2026 01:27:27 +0900 Subject: [PATCH] fix: prevent sync from overwriting existing files (#027) * fix: prevent sync from overwriting existing non-symlink files * test: add regression tests for sync overwrite protection --- internal/gitvolume/sync.go | 19 +++++++++++++- internal/gitvolume/sync_test.go | 45 ++++++++++++++++++++------------- 2 files changed, 46 insertions(+), 18 deletions(-) diff --git a/internal/gitvolume/sync.go b/internal/gitvolume/sync.go index c41cad8..4f35be5 100644 --- a/internal/gitvolume/sync.go +++ b/internal/gitvolume/sync.go @@ -97,6 +97,23 @@ func (g *GitVolume) sync(vol Volume, srcInfo os.FileInfo, opts SyncOptions) erro return nil } + // Check if target exists and is safe to overwrite + if _, err := os.Lstat(vol.TargetPath); err == nil { + // Target exists, check if it's a symlink (safe to remove) + // We allow overwriting symlinks because they are likely created by us (or user wants to replace them) + // But we DO NOT overwrite regular files or directories to prevent data loss. + targetInfo, err := os.Lstat(vol.TargetPath) + if err != nil { + return fmt.Errorf("failed to check target %s: %w", vol.Target, err) + } + + if targetInfo.Mode()&os.ModeSymlink == 0 { + // It is NOT a symlink (regular file or directory) + // For safety, we skip this volume and report an error. + return fmt.Errorf("target %s already exists and is not a symlink (skipping to prevent data loss)", vol.Target) + } + } + // Delete-and-Recreate strategy (Idempotency): // ensuring the target state exactly matches the source state. // We always remove the target path before syncing to guarantees a clean slate. @@ -163,7 +180,7 @@ func (g *GitVolume) syncCopy(src, dst string, srcInfo os.FileInfo) error { // syncLink handles link mode synchronization func (g *GitVolume) syncLink(src, dst string, relativeLink bool) error { - // Ensure parent directory exists + // Ensure parent directory exists if err := os.MkdirAll(filepath.Dir(dst), DefaultDirPerm); err != nil { return fmt.Errorf("failed to create parent directory: %w", err) } diff --git a/internal/gitvolume/sync_test.go b/internal/gitvolume/sync_test.go index da2bdc6..1d61087 100644 --- a/internal/gitvolume/sync_test.go +++ b/internal/gitvolume/sync_test.go @@ -71,7 +71,7 @@ func TestGitVolume_Sync_CopyDirectory(t *testing.T) { assert.Equal(t, "A=1", string(data)) } -func TestGitVolume_Sync_CopyDirectory_ExistingTargetOverwrites(t *testing.T) { +func TestGitVolume_Sync_CopyDirectory_ExistingTargetProtection(t *testing.T) { sourceDir, targetDir, cleanup := setupTestEnv(t) defer cleanup() @@ -82,24 +82,30 @@ func TestGitVolume_Sync_CopyDirectory_ExistingTargetOverwrites(t *testing.T) { targetConfigDir := filepath.Join(targetDir, "config") require.NoError(t, os.MkdirAll(targetConfigDir, 0755)) - require.NoError(t, os.WriteFile(filepath.Join(targetConfigDir, "app.env"), []byte("OLD"), 0644)) + // Create an existing file that should be protected + existingFile := filepath.Join(targetConfigDir, "app.env") + require.NoError(t, os.WriteFile(existingFile, []byte("OLD"), 0644)) volumes := []Volume{ {Source: "config", Target: "config", Mode: ModeCopy}, } gv := createTestGitVolume(sourceDir, targetDir, "", volumes) - // Should succeed and overwrite because we always overwrite now + // Should fail because target "config" directory exists and contains files not managed by us + // Wait, if target is a directory, do we check if it's a symlink? + // The code checks: `if targetInfo.Mode()&os.ModeSymlink == 0` + // "config" directory is NOT a symlink. So it should return error. err := gv.Sync(SyncOptions{}) - require.NoError(t, err) + require.Error(t, err) + assert.Contains(t, err.Error(), "already exists and is not a symlink") - // Verify content was updated - data, err := os.ReadFile(filepath.Join(targetDir, "config", "app.env")) + // Verify content was NOT updated (Protected) + data, err := os.ReadFile(existingFile) require.NoError(t, err) - assert.Equal(t, "A=1", string(data)) + assert.Equal(t, "OLD", string(data)) } -func TestGitVolume_Sync_Link_ExistingTargetOverwrites(t *testing.T) { +func TestGitVolume_Sync_Link_ExistingTargetProtection(t *testing.T) { sourceDir, targetDir, cleanup := setupTestEnv(t) defer cleanup() @@ -107,22 +113,26 @@ func TestGitVolume_Sync_Link_ExistingTargetOverwrites(t *testing.T) { targetPath := filepath.Join(targetDir, "link1.txt") require.NoError(t, os.WriteFile(targetPath, []byte("existing content"), 0644)) - // Try sync - should succeed (rename/remove logic) + // Try sync - should FAIL (protection) volumes := []Volume{ {Source: "source1.txt", Target: "link1.txt", Mode: ModeLink}, } gv := createTestGitVolume(sourceDir, targetDir, "", volumes) err := gv.Sync(SyncOptions{}) - assert.NoError(t, err) + assert.Error(t, err) + assert.Contains(t, err.Error(), "already exists and is not a symlink") - // Verify it's now a symlink + // Verify it's still the original file info, err := os.Lstat(targetPath) require.NoError(t, err) - assert.True(t, info.Mode()&os.ModeSymlink != 0, "target should be replaced with symlink") + assert.True(t, info.Mode()&os.ModeSymlink == 0, "target should remain a regular file") + + content, _ := os.ReadFile(targetPath) + assert.Equal(t, "existing content", string(content)) } -func TestGitVolume_Sync_Link_ExistingDirectoryOverwrites(t *testing.T) { +func TestGitVolume_Sync_Link_ExistingDirectoryProtection(t *testing.T) { sourceDir, targetDir, cleanup := setupTestEnv(t) defer cleanup() @@ -131,19 +141,20 @@ func TestGitVolume_Sync_Link_ExistingDirectoryOverwrites(t *testing.T) { require.NoError(t, os.Mkdir(targetPath, 0755)) require.NoError(t, os.WriteFile(filepath.Join(targetPath, "subfile.txt"), []byte("sub"), 0644)) - // Sync - should succeed (remove directory and symlink) + // Sync - should FAIL (protection) volumes := []Volume{ {Source: "source1.txt", Target: "link1.txt", Mode: ModeLink}, } gv := createTestGitVolume(sourceDir, targetDir, "", volumes) err := gv.Sync(SyncOptions{}) - assert.NoError(t, err) + assert.Error(t, err) + assert.Contains(t, err.Error(), "already exists and is not a symlink") - // Verify it's now a symlink + // Verify it's still a directory info, err := os.Lstat(targetPath) require.NoError(t, err) - assert.True(t, info.Mode()&os.ModeSymlink != 0, "target directory should be replaced with symlink") + assert.True(t, info.IsDir(), "target should remain a directory") } func TestGitVolume_Sync_Transition_LinkToCopy(t *testing.T) {