From d6b134aecc97d22dda60209bd71d089b2e618dee Mon Sep 17 00:00:00 2001
From: Scott Bishel <scott.bishel@mattermost.com>
Date: Tue, 24 Sep 2019 06:44:12 -0600
Subject: [PATCH] MM-18344 Honour absolute paths to SAML certificates (#12237)

* MM-18344 Honour absolute paths to SAML certificates

* fixes from code review

* update text
---
 config/file.go      | 21 +++++++++++++---
 config/file_test.go | 61 +++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 79 insertions(+), 3 deletions(-)

diff --git a/config/file.go b/config/file.go
index 718403c6c9..40ef6a0791 100644
--- a/config/file.go
+++ b/config/file.go
@@ -92,6 +92,16 @@ func resolveConfigFilePath(path string) (string, error) {
 	return "", fmt.Errorf("failed to find config file %s", path)
 }
 
+// resolveFilePath uses the name if name is absolute path.
+// otherwise returns the combined path/name
+func (fs *FileStore) resolveFilePath(name string) string {
+	// Absolute paths are explicit and require no resolution.
+	if filepath.IsAbs(name) {
+		return name
+	}
+	return filepath.Join(filepath.Dir(fs.path), name)
+}
+
 // Set replaces the current configuration in its entirety and updates the backing store.
 func (fs *FileStore) Set(newCfg *model.Config) (*model.Config, error) {
 	return fs.commonStore.set(newCfg, true, func(cfg *model.Config) error {
@@ -160,7 +170,7 @@ func (fs *FileStore) Load() (err error) {
 
 // GetFile fetches the contents of a previously persisted configuration file.
 func (fs *FileStore) GetFile(name string) ([]byte, error) {
-	resolvedPath := filepath.Join(filepath.Dir(fs.path), name)
+	resolvedPath := fs.resolveFilePath(name)
 
 	data, err := ioutil.ReadFile(resolvedPath)
 	if err != nil {
@@ -172,7 +182,7 @@ func (fs *FileStore) GetFile(name string) ([]byte, error) {
 
 // SetFile sets or replaces the contents of a configuration file.
 func (fs *FileStore) SetFile(name string, data []byte) error {
-	resolvedPath := filepath.Join(filepath.Dir(fs.path), name)
+	resolvedPath := fs.resolveFilePath(name)
 
 	err := ioutil.WriteFile(resolvedPath, data, 0777)
 	if err != nil {
@@ -188,7 +198,7 @@ func (fs *FileStore) HasFile(name string) (bool, error) {
 		return false, nil
 	}
 
-	resolvedPath := filepath.Join(filepath.Dir(fs.path), name)
+	resolvedPath := fs.resolveFilePath(name)
 
 	_, err := os.Stat(resolvedPath)
 	if err != nil && os.IsNotExist(err) {
@@ -202,6 +212,11 @@ func (fs *FileStore) HasFile(name string) (bool, error) {
 
 // RemoveFile removes a previously persisted configuration file.
 func (fs *FileStore) RemoveFile(name string) error {
+	if filepath.IsAbs(name) {
+		// Don't delete absolute filenames, as may be mounted drive, etc.
+		mlog.Debug("Skipping removal of configuration file with absolute path", mlog.String("filename", name))
+		return nil
+	}
 	resolvedPath := filepath.Join(filepath.Dir(fs.path), name)
 
 	err := os.Remove(resolvedPath)
diff --git a/config/file_test.go b/config/file_test.go
index 2c18d4bf80..e47df06034 100644
--- a/config/file_test.go
+++ b/config/file_test.go
@@ -890,6 +890,17 @@ func TestFileGetFile(t *testing.T) {
 		require.NoError(t, err)
 		require.Equal(t, []byte("test"), data)
 	})
+
+	t.Run("get via absolute path", func(t *testing.T) {
+		err := fs.SetFile("new", []byte("new file"))
+		require.NoError(t, err)
+
+		data, err := fs.GetFile(filepath.Join(filepath.Dir(path), "new"))
+
+		require.NoError(t, err)
+		require.Equal(t, []byte("new file"), data)
+	})
+
 }
 
 func TestFileSetFile(t *testing.T) {
@@ -920,6 +931,18 @@ func TestFileSetFile(t *testing.T) {
 		require.NoError(t, err)
 		require.Equal(t, []byte("overwritten file"), data)
 	})
+
+	t.Run("set via absolute path", func(t *testing.T) {
+		absolutePath := filepath.Join(filepath.Dir(path), "new")
+		err := fs.SetFile(absolutePath, []byte("new file"))
+		require.NoError(t, err)
+
+		data, err := fs.GetFile("new")
+
+		require.NoError(t, err)
+		require.Equal(t, []byte("new file"), data)
+	})
+
 }
 
 func TestFileHasFile(t *testing.T) {
@@ -987,6 +1010,23 @@ func TestFileHasFile(t *testing.T) {
 		require.NoError(t, err)
 		require.False(t, has)
 	})
+
+	t.Run("has via absolute path", func(t *testing.T) {
+		path, tearDown := setupConfigFile(t, minimalConfig)
+		defer tearDown()
+
+		fs, err := config.NewFileStore(path, true)
+		require.NoError(t, err)
+		defer fs.Close()
+
+		err = fs.SetFile("existing", []byte("existing file"))
+		require.NoError(t, err)
+
+		has, err := fs.HasFile(filepath.Join(filepath.Dir(path), "existing"))
+		require.NoError(t, err)
+		require.True(t, has)
+	})
+
 }
 
 func TestFileRemoveFile(t *testing.T) {
@@ -1052,6 +1092,27 @@ func TestFileRemoveFile(t *testing.T) {
 		_, err = fs.GetFile("existing")
 		require.Error(t, err)
 	})
+
+	t.Run("don't remove via absolute path", func(t *testing.T) {
+		path, tearDown := setupConfigFile(t, minimalConfig)
+		defer tearDown()
+
+		fs, err := config.NewFileStore(path, true)
+		require.NoError(t, err)
+		defer fs.Close()
+
+		err = fs.SetFile("existing", []byte("existing file"))
+		require.NoError(t, err)
+
+		filename := filepath.Join(filepath.Dir(path), "existing")
+		err = fs.RemoveFile(filename)
+		require.NoError(t, err)
+
+		has, err := fs.HasFile(filename)
+		require.NoError(t, err)
+		require.True(t, has)
+
+	})
 }
 
 func TestFileStoreString(t *testing.T) {