Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 18 additions & 1 deletion internal/gitvolume/sync.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Comment on lines +110 to +113

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-medium medium

Regression: Broken Idempotency for Directory Sync

The check targetInfo.Mode()&os.ModeSymlink == 0 prevents overwriting any existing directory that is not a symlink. However, when git-volume is used in ModeCopy for a directory, it creates a real directory at the target path.

On subsequent runs of the sync command, this check will identify the previously created directory as "not a symlink" and fail with an error. This breaks the idempotency of the sync operation, which is a core design principle of the tool (as noted in the comments on line 117). This can lead to partial sync states where some volumes are updated while others are skipped due to this error, potentially leaving the workspace in an inconsistent or insecure configuration.

Severity: Medium
Vulnerability Type: Logic Error

}
}
Comment on lines +101 to +115

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

security-medium medium

This section introduces a critical Time-of-Check to Time-of-Use (TOCTOU) race condition. The os.Lstat check (lines 101-115) for existing files/directories, followed by os.RemoveAll (line 124), creates a window where an attacker could replace a symlink with a regular directory or file. This could lead to os.RemoveAll recursively deleting attacker-supplied content, potentially causing significant data loss and undermining the intended overwrite protection. Additionally, consider defining a specific error type for this scenario to allow callers to handle it programmatically, rather than relying on string matching of the error message "target %s already exists and is not a symlink (skipping to prevent data loss)".


// 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.
Expand Down Expand Up @@ -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)
}
Expand Down
45 changes: 28 additions & 17 deletions internal/gitvolume/sync_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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()

Expand All @@ -82,47 +82,57 @@ 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()

// Create existing file at target
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()

Expand All @@ -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) {
Expand Down