From c3eca1fca3a4750e55dfa6d9935564a897232858 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E1=B4=8A=E1=B4=8F=E1=B4=87=20=E1=B4=84=CA=9C=E1=B4=87?= =?UTF-8?q?=C9=B4?= Date: Thu, 8 Jan 2026 14:22:04 -0500 Subject: [PATCH] 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 <175728472+Copilot@users.noreply.github.com> --- internal/db/repo_editor.go | 105 +++++++++++++++++++------------------ internal/osutil/osutil.go | 14 ++--- 2 files changed, 59 insertions(+), 60 deletions(-) diff --git a/internal/db/repo_editor.go b/internal/db/repo_editor.go index 750da9cb8..08ae7bab3 100644 --- a/internal/db/repo_editor.go +++ b/internal/db/repo_editor.go @@ -107,6 +107,19 @@ func (repo *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 (repo *Repository) UpdateRepoFile(doer *User, opts UpdateRepoFileOptions) (err error) { +func (repo *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 (repo *Repository) UpdateRepoFile(doer *User, opts UpdateRepoFileOptions) ( repoWorkingPool.CheckIn(com.ToStr(repo.ID)) defer repoWorkingPool.CheckOut(com.ToStr(repo.ID)) - if err = repo.DiscardLocalRepoBranchChanges(opts.OldBranch); err != nil { + if err := repo.DiscardLocalRepoBranchChanges(opts.OldBranch); err != nil { return fmt.Errorf("discard local repo branch[%s] changes: %v", opts.OldBranch, err) } else if err = repo.UpdateLocalCopyBranch(opts.OldBranch); err != nil { return fmt.Errorf("update local copy branch[%s]: %v", opts.OldBranch, err) } - repoPath := repo.RepoPath() localPath := repo.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 := repo.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 (repo *Repository) UpdateRepoFile(doer *User, opts UpdateRepoFileOptions) ( // 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 (repo *Repository) UpdateRepoFile(doer *User, opts UpdateRepoFileOptions) ( } 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 (repo *Repository) UpdateRepoFile(doer *User, opts UpdateRepoFileOptions) ( } // GetDiffPreview produces and returns diff result of a file which is not yet committed. -func (repo *Repository) GetDiffPreview(branch, treePath, content string) (diff *gitutil.Diff, err error) { +func (repo *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,7 +243,7 @@ func (repo *Repository) GetDiffPreview(branch, treePath, content string) (diff * repoWorkingPool.CheckIn(com.ToStr(repo.ID)) defer repoWorkingPool.CheckOut(com.ToStr(repo.ID)) - if err = repo.DiscardLocalRepoBranchChanges(branch); err != nil { + if err := repo.DiscardLocalRepoBranchChanges(branch); err != nil { return nil, fmt.Errorf("discard local repo branch[%s] changes: %v", branch, err) } else if err = repo.UpdateLocalCopyBranch(branch); err != nil { return nil, fmt.Errorf("update local copy branch[%s]: %v", branch, err) @@ -248,14 +252,15 @@ func (repo *Repository) GetDiffPreview(branch, treePath, content string) (diff * localPath := repo.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 (repo *Repository) GetDiffPreview(branch, treePath, content string) (diff * pid := process.Add(fmt.Sprintf("GetDiffPreview [repo_path: %s]", repo.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 (repo *Repository) DeleteRepoFile(doer *User, opts DeleteRepoFileOptions) ( return fmt.Errorf("update local copy branch[%s]: %v", opts.OldBranch, err) } + localPath := repo.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 := repo.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 := repo.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) } diff --git a/internal/osutil/osutil.go b/internal/osutil/osutil.go index 98135e3a8..f9e24f622 100644 --- a/internal/osutil/osutil.go +++ b/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 }