Repository: rclone/rclone
Pull request #6912 (2023-04-04 14:38:24)

Config file save improvements
#### What is the purpose of this change?

See commit messages and issues linked from those.

The following example should also illustrate the changes.

Given symbolic link:
```
tempconfig.txt -> tempconfig/temp/tempconfig.txt
```
Then:
```
./rclone -vv config create yyy local --config=tempconfig.txt
2023/04/04 16:42:36 DEBUG : rclone: Version "v1.63.0-DEV" starting with parameters ["./rclone" "-vv" "config" "create" "yyy" "local" "--config=tempconfig.txt"]
2023/04/04 16:42:36 DEBUG : Using config file from "/home/user/projects/rclone/tempconfig.txt"
2023/04/04 16:42:36 DEBUG : yyy: config in: state="", result=""
2023/04/04 16:42:36 DEBUG : yyy: config out: out=<nil>, err=<nil>
2023/04/04 16:42:36 DEBUG : Saving config file: /home/user/projects/rclone/tempconfig.txt
2023/04/04 16:42:36 DEBUG : Config path resolved to: tempconfig/temp/tempconfig.txt
2023/04/04 16:42:36 DEBUG : Writing new config to temporary file: tempconfig/temp/tempconfig.txt740963405
2023/04/04 16:42:36 DEBUG : Moving existing config to temporary file: tempconfig/temp/tempconfig.txt.old1388108941
2023/04/04 16:42:36 DEBUG : Moving temporary file with new config to real config path: tempconfig/temp/tempconfig.txt
[yyy]
type = local
2023/04/04 16:42:36 DEBUG : rclone: Version "v1.63.0-DEV" finishing with parameters ["./rclone" "-vv" "config" "create" "yyy" "local" "--config=tempconfig.txt"]
```

#### Was the change discussed in an issue or in the forum before?

<!--
Link issues and relevant forum posts here.
-->

#### Checklist

- [x] I have read the [contribution guidelines](https://github.com/rclone/rclone/blob/master/CONTRIBUTING.md#submitting-a-new-feature-or-bug-fix).
- [ ] I have added tests for all changes in this PR if appropriate.
- [x] I have added documentation for the changes if appropriate.
- [x] All commit messages are in [house style](https://github.com/rclone/rclone/blob/master/CONTRIBUTING.md#commit-messages).
- [x] I'm done, this Pull Request is ready for review :-)


albertony: Thanks.

> I think you are right - those extra Debugs are going to be very noisy :-)

Definitely. Useful for testing at this stage, so I'll keep them for now and (hopefully ;)) remember to remove them in time...

> I put some notes inline about resolving symlinks (another thing which is surprisingly hard!).

Yes, maybe I stepped right into a classic "how hard can it be" trap... and opened a can of worms! If so, perhaps the first commit (overwriting/removing .old files) should be a separate PR after all... 
ncw: > Pushed a couple of smaller improvements according to your comments, but not so important until the main question regarding how "deep" to handle symlink (EvalSymlinks) is decided..

I think just the last one is fine! Hence your original code with a small mod - see above.
albertony: I think I have it fixed according to our discussions, unless I've misunderstood you? And cleaned up (removed the debug output). And documented, but wasn't too sure how to explain the symlink part so feel free to suggest an alternative paragraph.
albertony: Argh... On testing with symlink on windows I got exactly what I was fearing: 11 backup (.old) config files remaining in my directory, as a result of "Failed to save config after 10 tries".

Test case:
```
mklink configlink.txt temp\config.txt
rclone --config=configlink.txt -vv config create zzz local
```
When directory temp does not exist the empty file created by os.CreateTemp was left behind, after error exit on "failed to move newly written config from ... to final location" before backup file cleanup. Such an empty backup file is of no use, so I've changed the code to take this into account and delete the backup even on write error of the new file if the backup was simply an empty file.

Now what if one comes into a similar situation, with "Failed to save config after 10 tries", but there was an existing config file? It should then leave behind the renamed backup of the existing file on the first failed save, and on the following failed saves it will not find an existing config file (since it has been renamed to .old) and the result should be as above. End result is one backup file left behind containing the actual original config file.
albertony: Back from some Easter holiday (hope you're enjoying some time off too!); fixed what was really an underlying bug in the case from my previous comment (introduced after I refactored away the configPathReal used initially). It will now actually create the directory `temp` and the config create command will succeed.
albertony: Great, thanks. I've pushed an update according to your last comment.

> Maybe we should try to test all the failure branches in the test code - that will be hard though!

Sounds like a good idea, but would definitely not be easy - at least to cover all of them! I could give it a try, but will probably be several days or even weeks before I get enough time to get anything out of it.
albertony: Actually I got some extra time today, so I wrote a few tests around the configfile save. Covering symlinks (but not on Windows, as it seem to require admin), creation of directories in the path, and a few FileMode/read-only related cases. Some failure branches are covered with this, but not nearly all of them.

Patch:
diff --git a/docs/content/docs.md b/docs/content/docs.md
index 7eb415d4e..941c9c842 100644
--- a/docs/content/docs.md
+++ b/docs/content/docs.md
@@ -952,15 +952,17 @@ To reduce risk of corrupting an existing configuration file, rclone
 will not write directly to it when saving changes. Instead it will
 first write to a new, temporary, file. If a configuration file already
 existed, it will (on Unix systems) try to mirror its permissions to
-the new file. Then it will rename the existing file by adding suffix
-".old" to its name (e.g. `rclone.conf.old`), if a file with the same
-name already exists it will simply be replaced. Next, rclone will rename
-the new file to the correct name, before finally cleaning up by deleting
-the original file now which has now ".old" suffix to its name. Note that
-one side-effect of this is that if you happen to have a file in the same
-directory and with the same name as your configuration file, but with
-suffix ".old", then rclone will end up deleting this file next time it
-updates its configuration file!
+the new file. Then it will rename the existing file to a temporary
+name as backup. Next, rclone will rename the new file to the correct name,
+before finally cleaning up by deleting the backup file.
+
+If the configuration file path used by rclone is a symbolic link, then
+this will be evaluated and rclone will write to the resolved path, instead
+of overwriting the symbolic link. Temporary files used in the process
+(described above) will be written to the same parent directory as that
+of the resolved configuration file, but if this directory is also a
+symbolic link it will not be resolved and the temporary files will be
+written to the location of the directory symbolic link.
 
 ### --contimeout=TIME ###
 
diff --git a/fs/config/configfile/configfile.go b/fs/config/configfile/configfile.go
index b242206a8..659df705d 100644
--- a/fs/config/configfile/configfile.go
+++ b/fs/config/configfile/configfile.go
@@ -106,20 +106,37 @@ func (s *Storage) Save() error {
 	if configPath == "" {
 		return fmt.Errorf("failed to save config file, path is empty")
 	}
+	configDir, configName := filepath.Split(configPath)
 
-	dir, name := filepath.Split(configPath)
-	err := file.MkdirAll(dir, os.ModePerm)
+	info, err := os.Lstat(configPath)
+	if err != nil {
+		if !os.IsNotExist(err) {
+			return fmt.Errorf("failed to resolve config file path: %w", err)
+		}
+	} else {
+		if info.Mode()&os.ModeSymlink != 0 {
+			configPath, err = os.Readlink(configPath)
+			if err != nil {
+				return fmt.Errorf("failed to resolve config file symbolic link: %w", err)
+			}
+			if !filepath.IsAbs(configPath) {
+				configPath = filepath.Join(configDir, configPath)
+			}
+			configDir = filepath.Dir(configPath)
+		}
+	}
+	err = file.MkdirAll(configDir, os.ModePerm)
 	if err != nil {
 		return fmt.Errorf("failed to create config directory: %w", err)
 	}
-	f, err := os.CreateTemp(dir, name)
+	f, err := os.CreateTemp(configDir, configName)
 	if err != nil {
 		return fmt.Errorf("failed to create temp file for new config: %w", err)
 	}
 	defer func() {
 		_ = f.Close()
 		if err := os.Remove(f.Name()); err != nil && !os.IsNotExist(err) {
-			fs.Errorf(nil, "failed to remove temp config file: %v", err)
+			fs.Errorf(nil, "Failed to remove temp file for new config: %v", err)
 		}
 	}()
 
@@ -139,7 +156,7 @@ func (s *Storage) Save() error {
 	}
 
 	var fileMode os.FileMode = 0600
-	info, err := os.Stat(configPath)
+	info, err = os.Stat(configPath)
 	if err != nil {
 		fs.Debugf(nil, "Using default permissions for config file: %v", fileMode)
 	} else if info.Mode() != fileMode {
@@ -154,15 +171,33 @@ func (s *Storage) Save() error {
 		fs.Errorf(nil, "Failed to set permissions on config file: %v", err)
 	}
 
-	if err = os.Rename(configPath, configPath+".old"); err != nil && !os.IsNotExist(err) {
-		return fmt.Errorf("failed to move previous config to backup location: %w", err)
+	fbackup, err := os.CreateTemp(configDir, configName+".old")
+	if err != nil {
+		return fmt.Errorf("failed to create temp file for old config backup: %w", err)
+	}
+	err = fbackup.Close()
+	if err != nil {
+		return fmt.Errorf("failed to close temp file for old config backup: %w", err)
+	}
+	keepBackup := true
+	defer func() {
+		if !keepBackup {
+			if err := os.Remove(fbackup.Name()); err != nil && !os.IsNotExist(err) {
+				fs.Errorf(nil, "Failed to remove temp file for old config backup: %v", err)
+			}
+		}
+	}()
+
+	if err = os.Rename(configPath, fbackup.Name()); err != nil {
+		if !os.IsNotExist(err) {
+			return fmt.Errorf("failed to move previous config to backup location: %w", err)
+		}
+		keepBackup = false // no existing file, no need to keep backup even if writing of new file fails
 	}
 	if err = os.Rename(f.Name(), configPath); err != nil {
 		return fmt.Errorf("failed to move newly written config from %s to final location: %v", f.Name(), err)
 	}
-	if err := os.Remove(configPath + ".old"); err != nil && !os.IsNotExist(err) {
-		fs.Errorf(nil, "Failed to remove backup config file: %v", err)
-	}
+	keepBackup = false // new file was written, no need to keep backup
 
 	// Update s.fi with the newly written file
 	s.fi, _ = os.Stat(configPath)


Test patch:
diff --git a/fs/config/configfile/configfile_test.go b/fs/config/configfile/configfile_test.go
index c7a325cce..2c2ba7e27 100644
--- a/fs/config/configfile/configfile_test.go
+++ b/fs/config/configfile/configfile_test.go
@@ -3,6 +3,7 @@ package configfile
 import (
 	"fmt"
 	"os"
+	"path/filepath"
 	"runtime"
 	"strings"
 	"testing"
@@ -242,3 +243,121 @@ func TestConfigFileNoConfig(t *testing.T) {
 		testConfigFileNoConfig(t, "/notfound")
 	})
 }
+
+func TestConfigFileSave(t *testing.T) {
+	testDir := t.TempDir()
+	configPath := filepath.Join(testDir, "a", "b", "c", "configfile")
+
+	assert.NoError(t, config.SetConfigPath(configPath))
+	data := &Storage{}
+	require.Error(t, data.Load(), config.ErrorConfigFileNotFound)
+
+	t.Run("CreatesDirsAndFile", func(t *testing.T) {
+		err := data.Save()
+		require.NoError(t, err)
+		info, err := os.Stat(configPath)
+		require.NoError(t, err)
+		assert.False(t, info.IsDir())
+	})
+	t.Run("KeepsFileMode", func(t *testing.T) {
+		if runtime.GOOS != "linux" {
+			t.Skip("this is a Linux only test")
+		}
+		assert.NoError(t, os.Chmod(configPath, 0400)) // -r--------
+		defer func() {
+			_ = os.Chmod(configPath, 0644) // -rw-r--r--
+		}()
+		err := data.Save()
+		require.NoError(t, err)
+		info, err := os.Stat(configPath)
+		require.NoError(t, err)
+		assert.Equal(t, os.FileMode(0400), info.Mode().Perm())
+	})
+	t.Run("SucceedsEvenIfReadOnlyFile", func(t *testing.T) {
+		// Save succeeds even if file is read-only since it does not write directly to the file.
+		assert.NoError(t, os.Chmod(configPath, 0400)) // -r--------
+		defer func() {
+			_ = os.Chmod(configPath, 0644) // -rw-r--r--
+		}()
+		err := data.Save()
+		assert.NoError(t, err)
+	})
+	t.Run("FailsIfNotAccessToDir", func(t *testing.T) {
+		// Save fails if no access to the directory.
+		if runtime.GOOS != "linux" {
+			// On Windows the os.Chmod only affects the read-only attribute of files)
+			t.Skip("this is a Linux only test")
+		}
+		configDir := filepath.Dir(configPath)
+		assert.NoError(t, os.Chmod(configDir, 0400)) // -r--------
+		defer func() {
+			_ = os.Chmod(configDir, 0755) // -rwxr-xr-x
+		}()
+		err := data.Save()
+		require.Error(t, err)
+		assert.True(t, strings.HasPrefix(err.Error(), "failed to resolve config file path"))
+	})
+	t.Run("FailsIfNotAllowedToCreateNewFiles", func(t *testing.T) {
+		// Save fails if read-only access to the directory, since it needs to create temporary files in there.
+		if runtime.GOOS != "linux" {
+			// On Windows the os.Chmod only affects the read-only attribute of files)
+			t.Skip("this is a Linux only test")
+		}
+		configDir := filepath.Dir(configPath)
+		assert.NoError(t, os.Chmod(configDir, 0544)) // -r-xr--r--
+		defer func() {
+			_ = os.Chmod(configDir, 0755) // -rwxr-xr-x
+		}()
+		err := data.Save()
+		require.Error(t, err)
+		assert.True(t, strings.HasPrefix(err.Error(), "failed to create temp file for new config"))
+	})
+}
+
+func TestConfigFileSaveSymlinkAbsolute(t *testing.T) {
+	if runtime.GOOS != "linux" {
+		// Symlinks may require admin privileges on Windows and os.Symlink will then
+		// fail with "A required privilege is not held by the client."
+		t.Skip("this is a Linux only test")
+	}
+	testDir := t.TempDir()
+	linkDir := filepath.Join(testDir, "a")
+	err := os.Mkdir(linkDir, os.ModePerm)
+	require.NoError(t, err)
+
+	testSymlink := func(t *testing.T, link string, target string, resolvedTarget string) {
+		err = os.Symlink(target, link)
+		require.NoError(t, err)
+		defer func() {
+			_ = os.Remove(link)
+		}()
+
+		assert.NoError(t, config.SetConfigPath(link))
+		data := &Storage{}
+		require.Error(t, data.Load(), config.ErrorConfigFileNotFound)
+
+		err = data.Save()
+		require.NoError(t, err)
+
+		info, err := os.Lstat(link)
+		require.NoError(t, err)
+		assert.True(t, info.Mode()&os.ModeSymlink != 0)
+		assert.False(t, info.IsDir())
+
+		info, err = os.Lstat(resolvedTarget)
+		require.NoError(t, err)
+		assert.False(t, info.IsDir())
+	}
+
+	t.Run("Absolute", func(t *testing.T) {
+		link := filepath.Join(linkDir, "configfilelink")
+		target := filepath.Join(testDir, "b", "configfiletarget")
+		testSymlink(t, link, target, target)
+	})
+	t.Run("Relative", func(t *testing.T) {
+		link := filepath.Join(linkDir, "configfilelink")
+		target := filepath.Join("b", "c", "configfiletarget")
+		resolvedTarget := filepath.Join(filepath.Dir(link), target)
+		testSymlink(t, link, target, resolvedTarget)
+	})
+}