diff --git a/pkg/commands/git_commands/submodule.go b/pkg/commands/git_commands/submodule.go index 79d3e2318..a31824e95 100644 --- a/pkg/commands/git_commands/submodule.go +++ b/pkg/commands/git_commands/submodule.go @@ -2,6 +2,8 @@ package git_commands import ( "bufio" + "fmt" + "io" "os" "path/filepath" "regexp" @@ -46,7 +48,36 @@ func (self *SubmoduleCommands) GetConfigs(parentModule *models.SubmoduleConfig) } defer file.Close() - scanner := bufio.NewScanner(file) + parsedConfigs, err := parseGitmodules(file, parentModule) + if err != nil { + return nil, err + } + + configs := []*models.SubmoduleConfig{} + for _, config := range parsedConfigs { + if isSuspiciousSubmoduleName(config.Name) { + self.Log.Warnf("Ignoring suspicious submodule name: %s", config.Name) + continue + } + + if isSuspiciousSubmodulePath(dir, config.Path) { + self.Log.Warnf("Ignoring suspicious submodule path: %s", config.Path) + continue + } + + configs = append(configs, config) + + nestedConfigs, err := self.GetConfigs(config) + if err == nil { + configs = append(configs, nestedConfigs...) + } + } + + return configs, nil +} + +func parseGitmodules(reader io.Reader, parentModule *models.SubmoduleConfig) ([]*models.SubmoduleConfig, error) { + scanner := bufio.NewScanner(reader) scanner.Split(bufio.ScanLines) firstMatch := func(str string, regex string) (string, bool) { @@ -60,27 +91,22 @@ func (self *SubmoduleCommands) GetConfigs(parentModule *models.SubmoduleConfig) } configs := []*models.SubmoduleConfig{} - lastConfigIdx := -1 for scanner.Scan() { line := scanner.Text() - if name, ok := firstMatch(line, `\[submodule "(.*)"\]`); ok { + if name, ok := firstMatch(line, `^\s*\[submodule "(.*)"\]`); ok { configs = append(configs, &models.SubmoduleConfig{ Name: name, ParentModule: parentModule, }) - lastConfigIdx = len(configs) - 1 continue } - if lastConfigIdx != -1 { - if path, ok := firstMatch(line, `\s*path\s*=\s*(.*)\s*`); ok { - configs[lastConfigIdx].Path = path - nestedConfigs, err := self.GetConfigs(configs[lastConfigIdx]) - if err == nil { - configs = append(configs, nestedConfigs...) - } - } else if url, ok := firstMatch(line, `\s*url\s*=\s*(.*)\s*`); ok { - configs[lastConfigIdx].Url = url + if len(configs) > 0 { + config := configs[len(configs)-1] + if path, ok := firstMatch(line, `^\s*path\s*=\s*(.*)\s*`); ok { + config.Path = path + } else if url, ok := firstMatch(line, `^\s*url\s*=\s*(.*)\s*`); ok { + config.Url = url } } } @@ -92,6 +118,52 @@ func (self *SubmoduleCommands) GetConfigs(parentModule *models.SubmoduleConfig) return configs, nil } +// isSuspiciousSubmoduleName applies the same rule as git's +// check_submodule_name(); git ignores a submodule whose name fails it. The +// git dir of a submodule is .git/modules/, so a ".." component in the +// name could put it outside the repository. Like git, we treat both slashes +// and backslashes as separators on every platform. +func isSuspiciousSubmoduleName(name string) bool { + if name == "" { + return true + } + + components := strings.FieldsFunc(name, func(r rune) bool { + return r == '/' || r == '\\' + }) + return lo.Contains(components, "..") +} + +// isSuspiciousSubmodulePath reports whether a submodule path from the +// .gitmodules file in parentDir might lead somewhere other than a directory +// below parentDir. We run commands in the submodule's directory and read the +// .gitmodules file in it, so the path must not lead anywhere else. +// +// Git never checks out a file through a symlink, so a path that goes through +// a symlink can't be a submodule in the first place. We reject such paths +// even if the symlink points to a directory below parentDir. +func isSuspiciousSubmodulePath(parentDir string, path string) bool { + if !filepath.IsLocal(path) || filepath.Clean(path) == "." { + return true + } + + dir := parentDir + for _, component := range strings.Split(filepath.Clean(path), string(filepath.Separator)) { + dir = filepath.Join(dir, component) + fileInfo, err := os.Lstat(dir) + if os.IsNotExist(err) { + // Nothing below this point exists (e.g. because the submodule + // isn't checked out), so it can't lead anywhere else either + return false + } + if err != nil || !fileInfo.IsDir() { + return true + } + } + + return false +} + // AnyHaveStageableChanges reports whether any of the given submodule paths has // a checked-out commit that differs from the one recorded in the // superproject's index, i.e. a change that `git add ` would actually @@ -237,6 +309,18 @@ func (self *SubmoduleCommands) runInParentModule(submodule *models.SubmoduleConf func (self *SubmoduleCommands) Delete(submodule *models.SubmoduleConfig) error { // based on https://gist.github.com/myusuf3/7f645819ded92bda6677 + // Git keeps the git dirs of submodules separately for each worktree + gitDirPath := submodule.GitDirPath(self.repoPaths.WorktreeGitDirPath()) + + // The name of the submodule comes from .gitmodules, so it is under the + // control of whoever committed that file. Make sure that we never delete + // anything other than a directory below the "modules" directory of the + // worktree's git dir. + modulesDirPath := filepath.Join(self.repoPaths.WorktreeGitDirPath(), "modules") + if relPath, err := filepath.Rel(modulesDirPath, gitDirPath); err != nil || relPath == "." || !filepath.IsLocal(relPath) { + return fmt.Errorf("refusing to delete %s because it is not inside %s", gitDirPath, modulesDirPath) + } + if err := self.runInParentModule(submodule, self.cmd.New( NewGitCmd("submodule"). Arg("deinit", "--force", "--", submodule.Path).ToArgv(), @@ -269,8 +353,7 @@ func (self *SubmoduleCommands) Delete(submodule *models.SubmoduleConfig) error { self.Log.Error(err) } - // Git keeps the git dirs of submodules separately for each worktree - return os.RemoveAll(submodule.GitDirPath(self.repoPaths.WorktreeGitDirPath())) + return os.RemoveAll(gitDirPath) } func (self *SubmoduleCommands) Add(name string, path string, url string) error { diff --git a/pkg/commands/git_commands/submodule_test.go b/pkg/commands/git_commands/submodule_test.go index d449d81de..554d0175f 100644 --- a/pkg/commands/git_commands/submodule_test.go +++ b/pkg/commands/git_commands/submodule_test.go @@ -1,16 +1,210 @@ package git_commands import ( + "os" + "path/filepath" "strings" "testing" "github.com/go-errors/errors" + "github.com/jesseduffield/lazygit/pkg/commands/models" "github.com/jesseduffield/lazygit/pkg/commands/oscommands" "github.com/jesseduffield/lazygit/pkg/env" "github.com/samber/lo" "github.com/stretchr/testify/assert" ) +func TestParseGitmodules(t *testing.T) { + scenarios := []struct { + testName string + lines []string + expected []*models.SubmoduleConfig + }{ + { + testName: "two submodules", + lines: []string{ + `[submodule "one"]`, + "\tpath = path/to/one", + "\turl = https://example.com/one.git", + `[submodule "two"]`, + "\tpath = two", + "\turl = ../two", + }, + expected: []*models.SubmoduleConfig{ + {Name: "one", Path: "path/to/one", Url: "https://example.com/one.git"}, + {Name: "two", Path: "two", Url: "../two"}, + }, + }, + { + testName: "url that contains path=", + lines: []string{ + `[submodule "sub"]`, + "\tpath = sub", + "\turl = https://example.com/repo?path=other", + }, + expected: []*models.SubmoduleConfig{ + {Name: "sub", Path: "sub", Url: "https://example.com/repo?path=other"}, + }, + }, + { + testName: "commented-out section", + lines: []string{ + `[submodule "sub"]`, + "\tpath = sub", + "\turl = ../sub", + `#[submodule "old"]`, + "#\tpath = old", + "#\turl = ../old", + }, + expected: []*models.SubmoduleConfig{ + {Name: "sub", Path: "sub", Url: "../sub"}, + }, + }, + } + + for _, s := range scenarios { + t.Run(s.testName, func(t *testing.T) { + configs, err := parseGitmodules(strings.NewReader(strings.Join(s.lines, "\n")), nil) + assert.NoError(t, err) + assert.Equal(t, s.expected, configs) + }) + } +} + +func TestIsSuspiciousSubmoduleName(t *testing.T) { + scenarios := []struct { + name string + suspicious bool + }{ + {name: "sub", suspicious: false}, + {name: "path/to/sub", suspicious: false}, + {name: "path\\to\\sub", suspicious: false}, + {name: ".", suspicious: false}, + {name: "...", suspicious: false}, + {name: "..sub", suspicious: false}, + {name: "sub..", suspicious: false}, + {name: "a..b/c", suspicious: false}, + {name: "", suspicious: true}, + {name: "..", suspicious: true}, + {name: "../victim", suspicious: true}, + {name: "../../../victim", suspicious: true}, + {name: "sub/..", suspicious: true}, + {name: "sub/../../victim", suspicious: true}, + {name: "..\\victim", suspicious: true}, + {name: "sub\\..\\..\\victim", suspicious: true}, + {name: "sub/..\\..\\victim", suspicious: true}, + } + + for _, s := range scenarios { + t.Run(s.name, func(t *testing.T) { + assert.Equal(t, s.suspicious, isSuspiciousSubmoduleName(s.name)) + }) + } +} + +func TestIsSuspiciousSubmodulePath(t *testing.T) { + dir := t.TempDir() + parentDir := filepath.Join(dir, "repo") + assert.NoError(t, os.MkdirAll(filepath.Join(parentDir, "sub"), 0o755)) + assert.NoError(t, os.MkdirAll(filepath.Join(parentDir, "path", "to", "sub"), 0o755)) + assert.NoError(t, os.MkdirAll(filepath.Join(dir, "other"), 0o755)) + assert.NoError(t, os.WriteFile(filepath.Join(parentDir, "file"), []byte("content"), 0o644)) + + scenarios := []struct { + path string + suspicious bool + }{ + {path: "sub", suspicious: false}, + {path: "sub/", suspicious: false}, + {path: "path/to/sub", suspicious: false}, + {path: "not_checked_out", suspicious: false}, + {path: "path/not_checked_out/sub", suspicious: false}, + {path: "", suspicious: true}, + {path: ".", suspicious: true}, + {path: "./", suspicious: true}, + {path: "sub/..", suspicious: true}, + {path: "..", suspicious: true}, + {path: "../other", suspicious: true}, + {path: "sub/../../other", suspicious: true}, + {path: filepath.Join(dir, "other"), suspicious: true}, + {path: "file", suspicious: true}, + {path: "file/sub", suspicious: true}, + } + + for _, s := range scenarios { + t.Run(s.path, func(t *testing.T) { + assert.Equal(t, s.suspicious, isSuspiciousSubmodulePath(parentDir, s.path)) + }) + } +} + +func TestIsSuspiciousSubmodulePathWithSymlinks(t *testing.T) { + dir := t.TempDir() + parentDir := filepath.Join(dir, "repo") + assert.NoError(t, os.MkdirAll(filepath.Join(parentDir, "sub"), 0o755)) + assert.NoError(t, os.MkdirAll(filepath.Join(dir, "other", "sub"), 0o755)) + if err := os.Symlink(filepath.Join("..", "other"), filepath.Join(parentDir, "link_outside")); err != nil { + t.Skipf("Can't create symlinks: %v", err) + } + assert.NoError(t, os.Symlink("sub", filepath.Join(parentDir, "link_inside"))) + + scenarios := []struct { + path string + suspicious bool + }{ + {path: "sub", suspicious: false}, + {path: "link_outside", suspicious: true}, + {path: "link_outside/sub", suspicious: true}, + {path: "link_inside", suspicious: true}, + } + + for _, s := range scenarios { + t.Run(s.path, func(t *testing.T) { + assert.Equal(t, s.suspicious, isSuspiciousSubmodulePath(parentDir, s.path)) + }) + } +} + +func TestSubmoduleDeleteOutsideModulesDir(t *testing.T) { + scenarios := []struct { + testName string + name string + // relative to the directory that contains the repo + victimPath string + }{ + { + testName: "modules dir itself", + name: ".", + victimPath: "repo/.git/modules/other_submodule", + }, + { + testName: "outside the repo", + name: "../../../victim", + victimPath: "victim", + }, + } + + for _, s := range scenarios { + t.Run(s.testName, func(t *testing.T) { + dir := t.TempDir() + victimFile := filepath.Join(dir, s.victimPath, "file") + assert.NoError(t, os.MkdirAll(filepath.Dir(victimFile), 0o755)) + assert.NoError(t, os.WriteFile(victimFile, []byte("content"), 0o644)) + + runner := oscommands.NewFakeRunner(t) + instance := buildSubmoduleCommands(commonDeps{ + runner: runner, + repoPaths: MockRepoPaths(filepath.Join(dir, "repo")), + }) + + err := instance.Delete(&models.SubmoduleConfig{Name: s.name, Path: "sub"}) + assert.Error(t, err) + assert.FileExists(t, victimFile) + runner.CheckForMissingCalls() + }) + } +} + func TestSubmoduleGetConflictCommits(t *testing.T) { type scenario struct { testName string diff --git a/pkg/integration/tests/submodule/ignore_suspicious_name.go b/pkg/integration/tests/submodule/ignore_suspicious_name.go new file mode 100644 index 000000000..fdca074cd --- /dev/null +++ b/pkg/integration/tests/submodule/ignore_suspicious_name.go @@ -0,0 +1,36 @@ +package submodule + +import ( + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var IgnoreSuspiciousName = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Ignore a submodule whose name in .gitmodules would put its git dir outside the repository", + ExtraCmdArgs: []string{}, + Skip: false, + SetupConfig: func(config *config.AppConfig) {}, + SetupRepo: func(shell *Shell) { + shell.CreateFile("../victim/file", "content") + + shell.EmptyCommit("first commit") + shell.CloneIntoSubmodule("my_submodule_name", "my_submodule_path") + shell.GitAddAll() + shell.Commit("add submodule") + + // The git dir of a submodule is .git/modules/, so this name + // points at the victim directory next to the repository + shell.UpdateFileAndAdd(".gitmodules", `[submodule "../../../victim"] + path = my_submodule_path + url = ../my_submodule_name +`) + shell.Commit("rename submodule") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Submodules().Focus(). + IsEmpty() + + t.FileSystem(). + PathPresent("../victim/file") + }, +}) diff --git a/pkg/integration/tests/submodule/ignore_suspicious_path.go b/pkg/integration/tests/submodule/ignore_suspicious_path.go new file mode 100644 index 000000000..d2cf3416e --- /dev/null +++ b/pkg/integration/tests/submodule/ignore_suspicious_path.go @@ -0,0 +1,65 @@ +package submodule + +import ( + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var IgnoreSuspiciousPath = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Ignore a submodule whose path in .gitmodules leads outside the repository", + ExtraCmdArgs: []string{}, + Skip: false, + SetupConfig: func(config *config.AppConfig) {}, + SetupRepo: func(shell *Shell) { + // Two unrelated repos next to ours, each with an uncommitted change + for _, otherRepo := range []string{"other_repo", "linked_repo"} { + shell.Chdir("..") + shell.CreateDir(otherRepo) + shell.Chdir(otherRepo) + shell.Init() + shell.CreateFileAndAdd("file", "original") + shell.Commit("initial commit") + shell.UpdateFile("file", "modified") + shell.Chdir("../repo") + } + + shell.EmptyCommit("first commit") + shell.RunCommand([]string{"ln", "-s", "../linked_repo", "link"}) + shell.GitAdd("link") + shell.CreateFileAndAdd(".gitmodules", `[submodule "other"] + path = ../other_repo + url = ../other_repo +[submodule "linked"] + path = link + url = ../linked_repo +`) + shell.Commit("add submodules") + shell.CreateFile("untracked", "content") + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + t.Views().Submodules(). + IsEmpty() + + t.Views().Files().Focus(). + Lines( + Contains("untracked"), + ). + Press(keys.Files.ViewResetOptions). + Tap(func() { + t.ExpectPopup().Menu(). + Title(Equals("")). + Select(Contains("Nuke working tree")). + Confirm() + + t.ExpectPopup().Confirmation(). + Title(Equals("Nuke working tree")). + Content(AnyString()). + Confirm() + }). + IsEmpty() + + t.FileSystem(). + FileContent("../other_repo/file", Equals("modified")). + FileContent("../linked_repo/file", Equals("modified")) + }, +}) diff --git a/pkg/integration/tests/test_list.go b/pkg/integration/tests/test_list.go index 282c2436c..97a225f06 100644 --- a/pkg/integration/tests/test_list.go +++ b/pkg/integration/tests/test_list.go @@ -578,6 +578,8 @@ var tests = []*components.IntegrationTest{ submodule.Enter, submodule.EnterFromDotfileBareRepo, submodule.EnterNested, + submodule.IgnoreSuspiciousName, + submodule.IgnoreSuspiciousPath, submodule.Remove, submodule.RemoveInLinkedWorktree, submodule.RemoveNested,