repository: reject any updates that has symlink in path hierarchy (#8082)

Co-authored-by: deepsource-autofix[bot] <62050782+deepsource-autofix[bot]@users.noreply.github.com>
Co-authored-by: Copilot <[email protected]>

internal/database/repo_editor.go

@@ -107,6 +107,19 @@ func (r *Repository) CheckoutNewBranch(oldBranch, newBranch string) error {
 	return nil
 }
 
+// hasSymlinkInPath returns true if there is any symlink in path hierarchy using
+// the given base and relative path.
+func hasSymlinkInPath(base, relPath string) bool {
+	parts := strings.Split(filepath.ToSlash(relPath), "/")
+	for i := range parts {
+		filePath := path.Join(append([]string{base}, parts[:i+1]...)...)
+		if osutil.IsSymlink(filePath) {
+			return true
+		}
+	}
+	return false
+}
+
 type UpdateRepoFileOptions struct {
 	OldBranch   string
 	NewBranch   string
@@ -118,7 +131,7 @@ type UpdateRepoFileOptions struct {
 }
 
 // UpdateRepoFile adds or updates a file in repository.
-func (r *Repository) UpdateRepoFile(doer *User, opts UpdateRepoFileOptions) (err error) {
+func (r *Repository) UpdateRepoFile(doer *User, opts UpdateRepoFileOptions) error {
 	// 🚨 SECURITY: Prevent uploading files into the ".git" directory.
 	if isRepositoryGitPath(opts.NewTreeName) {
 		return errors.Errorf("bad tree path %q", opts.NewTreeName)
@@ -127,15 +140,21 @@ func (r *Repository) UpdateRepoFile(doer *User, opts UpdateRepoFileOptions) (err
 	repoWorkingPool.CheckIn(com.ToStr(r.ID))
 	defer repoWorkingPool.CheckOut(com.ToStr(r.ID))
 
-	if err = r.DiscardLocalRepoBranchChanges(opts.OldBranch); err != nil {
-		return fmt.Errorf("discard local r branch[%s] changes: %v", opts.OldBranch, err)
+	if err := r.DiscardLocalRepoBranchChanges(opts.OldBranch); err != nil {
+		return fmt.Errorf("discard local repo branch[%s] changes: %v", opts.OldBranch, err)
 	} else if err = r.UpdateLocalCopyBranch(opts.OldBranch); err != nil {
 		return fmt.Errorf("update local copy branch[%s]: %v", opts.OldBranch, err)
 	}
 
-	repoPath := r.RepoPath()
 	localPath := r.LocalCopyPath()
 
+	// 🚨 SECURITY: Prevent touching files in surprising places, reject operations
+	// that involve symlinks.
+	if hasSymlinkInPath(localPath, opts.OldTreeName) || hasSymlinkInPath(localPath, opts.NewTreeName) {
+		return errors.New("cannot update file with symbolic link in path")
+	}
+
+	repoPath := r.RepoPath()
 	if opts.OldBranch != opts.NewBranch {
 		// Directly return error if new branch already exists in the server
 		if git.RepoHasBranch(repoPath, opts.NewBranch) {
@@ -144,7 +163,7 @@ func (r *Repository) UpdateRepoFile(doer *User, opts UpdateRepoFileOptions) (err
 
 		// Otherwise, delete branch from local copy in case out of sync
 		if git.RepoHasBranch(localPath, opts.NewBranch) {
-			if err = git.DeleteBranch(localPath, opts.NewBranch, git.DeleteBranchOptions{
+			if err := git.DeleteBranch(localPath, opts.NewBranch, git.DeleteBranchOptions{
 				Force: true,
 			}); err != nil {
 				return fmt.Errorf("delete branch %q: %v", opts.NewBranch, err)
@@ -157,46 +176,31 @@ func (r *Repository) UpdateRepoFile(doer *User, opts UpdateRepoFileOptions) (err
 	}
 
 	oldFilePath := path.Join(localPath, opts.OldTreeName)
-	filePath := path.Join(localPath, opts.NewTreeName)
-	if err = os.MkdirAll(path.Dir(filePath), os.ModePerm); err != nil {
-		return err
+	newFilePath := path.Join(localPath, opts.NewTreeName)
+
+	// Prompt the user if the meant-to-be new file already exists.
+	if osutil.IsExist(newFilePath) && opts.IsNewFile {
+		return ErrRepoFileAlreadyExist{newFilePath}
 	}
 
-	// If it's meant to be a new file, make sure it doesn't exist.
-	if opts.IsNewFile {
-		// 🚨 SECURITY: Prevent updating files in surprising place, check if the file is
-		// a symlink.
-		if osutil.IsSymlink(filePath) {
-			return fmt.Errorf("cannot update symbolic link: %s", opts.NewTreeName)
-		}
-		if osutil.IsExist(filePath) {
-			return ErrRepoFileAlreadyExist{filePath}
-		}
+	if err := os.MkdirAll(path.Dir(newFilePath), os.ModePerm); err != nil {
+		return errors.Wrapf(err, "create parent directories of %q", newFilePath)
 	}
 
-	// Ignore move step if it's a new file under a directory.
-	// Otherwise, move the file when name changed.
 	if osutil.IsFile(oldFilePath) && opts.OldTreeName != opts.NewTreeName {
-		// 🚨 SECURITY: Prevent updating files in surprising place, check if the file is
-		// a symlink.
-		if osutil.IsSymlink(oldFilePath) {
-			return fmt.Errorf("cannot move symbolic link: %s", opts.OldTreeName)
-		}
-
-		if err = git.Move(localPath, opts.OldTreeName, opts.NewTreeName); err != nil {
-			return fmt.Errorf("git mv %q %q: %v", opts.OldTreeName, opts.NewTreeName, err)
+		if err := git.Move(localPath, opts.OldTreeName, opts.NewTreeName); err != nil {
+			return errors.Wrapf(err, "git mv %q %q", opts.OldTreeName, opts.NewTreeName)
 		}
 	}
 
-	if err = os.WriteFile(filePath, []byte(opts.Content), 0600); err != nil {
+	if err := os.WriteFile(newFilePath, []byte(opts.Content), 0o600); err != nil {
 		return fmt.Errorf("write file: %v", err)
 	}
 
-	if err = git.Add(localPath, git.AddOptions{All: true}); err != nil {
+	if err := git.Add(localPath, git.AddOptions{All: true}); err != nil {
 		return fmt.Errorf("git add --all: %v", err)
 	}
-
-	err = git.CreateCommit(
+	err := git.CreateCommit(
 		localPath,
 		&git.Signature{
 			Name:  doer.DisplayName(),
@@ -230,7 +234,7 @@ func (r *Repository) UpdateRepoFile(doer *User, opts UpdateRepoFileOptions) (err
 }
 
 // GetDiffPreview produces and returns diff result of a file which is not yet committed.
-func (r *Repository) GetDiffPreview(branch, treePath, content string) (diff *gitutil.Diff, err error) {
+func (r *Repository) GetDiffPreview(branch, treePath, content string) (*gitutil.Diff, error) {
 	// 🚨 SECURITY: Prevent uploading files into the ".git" directory.
 	if isRepositoryGitPath(treePath) {
 		return nil, errors.Errorf("bad tree path %q", treePath)
@@ -239,8 +243,8 @@ func (r *Repository) GetDiffPreview(branch, treePath, content string) (diff *git
 	repoWorkingPool.CheckIn(com.ToStr(r.ID))
 	defer repoWorkingPool.CheckOut(com.ToStr(r.ID))
 
-	if err = r.DiscardLocalRepoBranchChanges(branch); err != nil {
-		return nil, fmt.Errorf("discard local r branch[%s] changes: %v", branch, err)
+	if err := r.DiscardLocalRepoBranchChanges(branch); err != nil {
+		return nil, fmt.Errorf("discard local repo branch[%s] changes: %v", branch, err)
 	} else if err = r.UpdateLocalCopyBranch(branch); err != nil {
 		return nil, fmt.Errorf("update local copy branch[%s]: %v", branch, err)
 	}
@@ -248,14 +252,15 @@ func (r *Repository) GetDiffPreview(branch, treePath, content string) (diff *git
 	localPath := r.LocalCopyPath()
 	filePath := path.Join(localPath, treePath)
 
-	// 🚨 SECURITY: Prevent updating files in surprising place, check if the target is
-	// a symlink.
-	if osutil.IsSymlink(filePath) {
-		return nil, fmt.Errorf("cannot get diff preview for symbolic link: %s", treePath)
+	// 🚨 SECURITY: Prevent touching files in surprising places, reject operations
+	// that involve symlinks.
+	if hasSymlinkInPath(localPath, treePath) {
+		return nil, errors.New("cannot update file with symbolic link in path")
 	}
-	if err = os.MkdirAll(filepath.Dir(filePath), os.ModePerm); err != nil {
-		return nil, err
-	} else if err = os.WriteFile(filePath, []byte(content), 0600); err != nil {
+
+	if err := os.MkdirAll(path.Dir(filePath), os.ModePerm); err != nil {
+		return nil, errors.Wrap(err, "create parent directories")
+	} else if err = os.WriteFile(filePath, []byte(content), 0o600); err != nil {
 		return nil, fmt.Errorf("write file: %v", err)
 	}
 
@@ -276,15 +281,13 @@ func (r *Repository) GetDiffPreview(branch, treePath, content string) (diff *git
 	pid := process.Add(fmt.Sprintf("GetDiffPreview [repo_path: %s]", r.RepoPath()), cmd)
 	defer process.Remove(pid)
 
-	diff, err = gitutil.ParseDiff(stdout, conf.Git.MaxDiffFiles, conf.Git.MaxDiffLines, conf.Git.MaxDiffLineChars)
+	diff, err := gitutil.ParseDiff(stdout, conf.Git.MaxDiffFiles, conf.Git.MaxDiffLines, conf.Git.MaxDiffLineChars)
 	if err != nil {
 		return nil, fmt.Errorf("parse diff: %v", err)
 	}
-
 	if err = cmd.Wait(); err != nil {
 		return nil, fmt.Errorf("wait: %v", err)
 	}
-
 	return diff, nil
 }
 
@@ -319,21 +322,21 @@ func (r *Repository) DeleteRepoFile(doer *User, opts DeleteRepoFileOptions) (err
 		return fmt.Errorf("update local copy branch[%s]: %v", opts.OldBranch, err)
 	}
 
+	localPath := r.LocalCopyPath()
+
+	// 🚨 SECURITY: Prevent touching files in surprising places, reject operations
+	// that involve symlinks.
+	if hasSymlinkInPath(localPath, opts.TreePath) {
+		return errors.New("cannot update file with symbolic link in path")
+	}
+
 	if opts.OldBranch != opts.NewBranch {
 		if err := r.CheckoutNewBranch(opts.OldBranch, opts.NewBranch); err != nil {
 			return fmt.Errorf("checkout new branch[%s] from old branch[%s]: %v", opts.NewBranch, opts.OldBranch, err)
 		}
 	}
 
-	localPath := r.LocalCopyPath()
 	filePath := path.Join(localPath, opts.TreePath)
-
-	// 🚨 SECURITY: Prevent updating files in surprising place, check if the file is
-	// a symlink.
-	if osutil.IsSymlink(filePath) {
-		return fmt.Errorf("cannot delete symbolic link: %s", opts.TreePath)
-	}
-
 	if err = os.Remove(filePath); err != nil {
 		return fmt.Errorf("remove file %q: %v", opts.TreePath, err)
 	}

internal/osutil/osutil.go

@@ -9,7 +9,8 @@ import (
 	"os/user"
 )
 
-// IsFile returns true if given path exists as a file (i.e. not a directory).
+// IsFile returns true if given path exists as a file (i.e. not a directory)
+// following any symlinks.
 func IsFile(path string) bool {
 	f, e := os.Stat(path)
 	if e != nil {
@@ -18,8 +19,8 @@ func IsFile(path string) bool {
 	return !f.IsDir()
 }
 
-// IsDir returns true if given path is a directory, and returns false when it's
-// a file or does not exist.
+// IsDir returns true if given path is a directory following any symlinks, and
+// returns false when it's a file or does not exist.
 func IsDir(dir string) bool {
 	f, e := os.Stat(dir)
 	if e != nil {
@@ -28,7 +29,7 @@ func IsDir(dir string) bool {
 	return f.IsDir()
 }
 
-// IsExist returns true if a file or directory exists.
+// IsExist returns true if a file or directory exists following any symlinks.
 func IsExist(path string) bool {
 	_, err := os.Stat(path)
 	return err == nil || os.IsExist(err)
@@ -36,15 +37,10 @@ func IsExist(path string) bool {
 
 // IsSymlink returns true if given path is a symbolic link.
 func IsSymlink(path string) bool {
-	if !IsExist(path) {
-		return false
-	}
-
 	fileInfo, err := os.Lstat(path)
 	if err != nil {
 		return false
 	}
-
 	return fileInfo.Mode()&os.ModeSymlink != 0
 }