From 58fcd14a99c2bc0a66bb50d651a5a96fe5eb5884 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 7 Nov 2026 23:44:58 +0100 Subject: [PATCH 01/11] Add a test showing that removing a submodule in a linked worktree deletes the wrong git dir Co-Authored-By: Claude Opus 5.5 --- .../submodule/remove_in_linked_worktree.go | 52 +++++++++++++++++++ pkg/integration/tests/test_list.go | 1 + 2 files changed, 53 insertions(+) create mode 100644 pkg/integration/tests/submodule/remove_in_linked_worktree.go diff --git a/pkg/integration/tests/submodule/remove_in_linked_worktree.go b/pkg/integration/tests/submodule/remove_in_linked_worktree.go new file mode 100644 index 000000000..438416894 --- /dev/null +++ b/pkg/integration/tests/submodule/remove_in_linked_worktree.go @@ -0,0 +1,52 @@ +package submodule + +import ( + "github.com/jesseduffield/lazygit/pkg/config" + . "github.com/jesseduffield/lazygit/pkg/integration/components" +) + +var RemoveInLinkedWorktree = NewIntegrationTest(NewIntegrationTestArgs{ + Description: "Remove a submodule in a linked worktree", + ExtraCmdArgs: []string{}, + Skip: false, + SetupConfig: func(config *config.AppConfig) {}, + SetupRepo: func(shell *Shell) { + shell.EmptyCommit("first commit") + shell.CloneIntoSubmodule("my_submodule_name", "my_submodule_path") + shell.GitAddAll() + shell.Commit("add submodule") + + shell.AddWorktree("HEAD", "../linked-worktree", "mybranch") + shell.Chdir("../linked-worktree") + shell.RunCommand([]string{"git", "submodule", "update", "--init"}) + }, + Run: func(t *TestDriver, keys config.KeybindingConfig) { + // Git keeps the git dirs of submodules separately for each worktree + mainGitDirSubmodulePath := "../repo/.git/modules/my_submodule_name" + linkedGitDirSubmodulePath := "../repo/.git/worktrees/linked-worktree/modules/my_submodule_name" + t.FileSystem(). + PathPresent(mainGitDirSubmodulePath). + PathPresent(linkedGitDirSubmodulePath) + + t.Views().Submodules().Focus(). + Lines( + Contains("my_submodule_name").IsSelected(), + ). + Press(keys.Universal.Remove). + Tap(func() { + t.ExpectPopup().Confirmation(). + Title(Equals("Remove submodule")). + Content(Equals("Are you sure you want to remove submodule 'my_submodule_name' and its corresponding directory? This is irreversible.")). + Confirm() + }). + IsEmpty() + + t.FileSystem(). + /* EXPECTED: + PathPresent(mainGitDirSubmodulePath). + PathNotPresent(linkedGitDirSubmodulePath) + ACTUAL: */ + PathNotPresent(mainGitDirSubmodulePath). + PathPresent(linkedGitDirSubmodulePath) + }, +}) diff --git a/pkg/integration/tests/test_list.go b/pkg/integration/tests/test_list.go index 36b71ea00..282c2436c 100644 --- a/pkg/integration/tests/test_list.go +++ b/pkg/integration/tests/test_list.go @@ -579,6 +579,7 @@ var tests = []*components.IntegrationTest{ submodule.EnterFromDotfileBareRepo, submodule.EnterNested, submodule.Remove, + submodule.RemoveInLinkedWorktree, submodule.RemoveNested, submodule.Reset, submodule.ResetFolder, From 98ca24de23f92a6ce161f0a8e004328beb1b89b6 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 7 Nov 2026 23:46:56 +0100 Subject: [PATCH 02/11] Delete the right git dir when removing a submodule in a linked worktree Removing a submodule in a linked worktree deletes the git dir of that submodule in the main worktree. This breaks the submodule there, and any unpushed commits, local branches and stashes in it are lost. The git dir of the submodule in the linked worktree stays behind. Git keeps the git dirs of submodules separately for each worktree. For a linked worktree, they are in .git/worktrees//modules. Lazygit used the worktree's git dir until 3b723282cb switched it to the repo's git dir, so this has been broken since v0.41.0. Use the worktree's git dir again. Co-Authored-By: Claude Opus 5.5 --- pkg/commands/git_commands/submodule.go | 5 ++--- pkg/commands/models/submodule_config.go | 6 +++--- .../tests/submodule/remove_in_linked_worktree.go | 4 ---- 3 files changed, 5 insertions(+), 10 deletions(-) diff --git a/pkg/commands/git_commands/submodule.go b/pkg/commands/git_commands/submodule.go index 0fa5de9df..79d3e2318 100644 --- a/pkg/commands/git_commands/submodule.go +++ b/pkg/commands/git_commands/submodule.go @@ -269,9 +269,8 @@ func (self *SubmoduleCommands) Delete(submodule *models.SubmoduleConfig) error { self.Log.Error(err) } - // We may in fact want to use the repo's git dir path but git docs say not to - // mix submodules and worktrees anyway. - return os.RemoveAll(submodule.GitDirPath(self.repoPaths.repoGitDirPath)) + // Git keeps the git dirs of submodules separately for each worktree + return os.RemoveAll(submodule.GitDirPath(self.repoPaths.WorktreeGitDirPath())) } func (self *SubmoduleCommands) Add(name string, path string, url string) error { diff --git a/pkg/commands/models/submodule_config.go b/pkg/commands/models/submodule_config.go index 7df0d131a..48d838ff4 100644 --- a/pkg/commands/models/submodule_config.go +++ b/pkg/commands/models/submodule_config.go @@ -38,10 +38,10 @@ func (r *SubmoduleConfig) Description() string { return r.RefName() } -func (r *SubmoduleConfig) GitDirPath(repoGitDirPath string) string { - parentPath := repoGitDirPath +func (r *SubmoduleConfig) GitDirPath(worktreeGitDirPath string) string { + parentPath := worktreeGitDirPath if r.ParentModule != nil { - parentPath = r.ParentModule.GitDirPath(repoGitDirPath) + parentPath = r.ParentModule.GitDirPath(worktreeGitDirPath) } return filepath.Join(parentPath, "modules", r.Name) diff --git a/pkg/integration/tests/submodule/remove_in_linked_worktree.go b/pkg/integration/tests/submodule/remove_in_linked_worktree.go index 438416894..f081faa46 100644 --- a/pkg/integration/tests/submodule/remove_in_linked_worktree.go +++ b/pkg/integration/tests/submodule/remove_in_linked_worktree.go @@ -42,11 +42,7 @@ var RemoveInLinkedWorktree = NewIntegrationTest(NewIntegrationTestArgs{ IsEmpty() t.FileSystem(). - /* EXPECTED: PathPresent(mainGitDirSubmodulePath). PathNotPresent(linkedGitDirSubmodulePath) - ACTUAL: */ - PathNotPresent(mainGitDirSubmodulePath). - PathPresent(linkedGitDirSubmodulePath) }, }) From aa034b1b5e34b046a2d7f06b8fbe6d6d9cccb7e8 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 7 Nov 2026 23:14:48 +0100 Subject: [PATCH 03/11] Parse .gitmodules completely before loading nested submodules So far, GetConfigs loaded the nested submodules of an entry as soon as it saw the entry's path line. Parse the whole file first, and load the nested submodules in a second loop. This prepares for checking the name and path of each entry before using it. This only makes a difference for malformed entries. If the path value is empty, we no longer read the same .gitmodules file over and over. If an entry has several path lines, we no longer load its nested submodules several times. Co-Authored-By: Claude Opus 5.5 --- pkg/commands/git_commands/submodule.go | 37 +++++++++++++++++++------- 1 file changed, 27 insertions(+), 10 deletions(-) diff --git a/pkg/commands/git_commands/submodule.go b/pkg/commands/git_commands/submodule.go index 79d3e2318..80ccbfcdd 100644 --- a/pkg/commands/git_commands/submodule.go +++ b/pkg/commands/git_commands/submodule.go @@ -2,6 +2,7 @@ package git_commands import ( "bufio" + "io" "os" "path/filepath" "regexp" @@ -46,7 +47,28 @@ 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 { + configs = append(configs, config) + + if config.Path != "" { + 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,7 +82,6 @@ func (self *SubmoduleCommands) GetConfigs(parentModule *models.SubmoduleConfig) } configs := []*models.SubmoduleConfig{} - lastConfigIdx := -1 for scanner.Scan() { line := scanner.Text() @@ -68,19 +89,15 @@ func (self *SubmoduleCommands) GetConfigs(parentModule *models.SubmoduleConfig) configs = append(configs, &models.SubmoduleConfig{ Name: name, ParentModule: parentModule, }) - lastConfigIdx = len(configs) - 1 continue } - if lastConfigIdx != -1 { + if len(configs) > 0 { + config := configs[len(configs)-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...) - } + config.Path = path } else if url, ok := firstMatch(line, `\s*url\s*=\s*(.*)\s*`); ok { - configs[lastConfigIdx].Url = url + config.Url = url } } } From 0c46c6261fd0f48a936fc2b1d0062e6e2e683208 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 7 Nov 2026 21:47:45 +0100 Subject: [PATCH 04/11] Add a test showing that removing a submodule can delete a directory outside the repository Co-Authored-By: Claude Opus 5.5 --- .../tests/submodule/ignore_suspicious_name.go | 54 +++++++++++++++++++ pkg/integration/tests/test_list.go | 1 + 2 files changed, 55 insertions(+) create mode 100644 pkg/integration/tests/submodule/ignore_suspicious_name.go 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..497b12134 --- /dev/null +++ b/pkg/integration/tests/submodule/ignore_suspicious_name.go @@ -0,0 +1,54 @@ +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(). + /* EXPECTED: + IsEmpty() + ACTUAL: */ + Lines( + Contains("../../../victim").IsSelected(), + ). + Press(keys.Universal.Remove). + Tap(func() { + t.ExpectPopup().Confirmation(). + Title(Equals("Remove submodule")). + Content(Equals("Are you sure you want to remove submodule '../../../victim' and its corresponding directory? This is irreversible.")). + Confirm() + }). + Lines( + Contains("../../../victim").IsSelected(), + ) + + t.FileSystem(). + /* EXPECTED: + PathPresent("../victim/file") + ACTUAL: */ + PathNotPresent("../victim/file") + }, +}) diff --git a/pkg/integration/tests/test_list.go b/pkg/integration/tests/test_list.go index 282c2436c..ae54d0f6e 100644 --- a/pkg/integration/tests/test_list.go +++ b/pkg/integration/tests/test_list.go @@ -578,6 +578,7 @@ var tests = []*components.IntegrationTest{ submodule.Enter, submodule.EnterFromDotfileBareRepo, submodule.EnterNested, + submodule.IgnoreSuspiciousName, submodule.Remove, submodule.RemoveInLinkedWorktree, submodule.RemoveNested, From 82de59b3ffc11a3324958b5ff61c85081d06e0a6 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 7 Nov 2026 21:50:29 +0100 Subject: [PATCH 05/11] Ignore submodules with a suspicious name in .gitmodules If .gitmodules contains a submodule name like "../../../victim", removing that submodule in lazygit deletes the directory "victim" outside the repository, along with everything in it. With enough "../" components the path becomes "/", and lazygit then deletes all files that the user has permission to delete. An attacker only needs to commit such a .gitmodules file to a repository. If a user of that repository removes the submodule in lazygit, their files are gone. This happens because lazygit deletes the submodule's git dir with os.RemoveAll, and builds its path by joining .git/modules with the name from .gitmodules. filepath.Join resolves the ".." components. Git refuses to use such names. It prints "ignoring suspicious submodule name" and treats the entry as if it didn't exist. Do the same in GetConfigs, using the same rule as git's check_submodule_name(). Co-Authored-By: Claude Opus 5.5 --- pkg/commands/git_commands/submodule.go | 21 +++++++++++++ pkg/commands/git_commands/submodule_test.go | 31 +++++++++++++++++++ .../tests/submodule/ignore_suspicious_name.go | 18 ----------- 3 files changed, 52 insertions(+), 18 deletions(-) diff --git a/pkg/commands/git_commands/submodule.go b/pkg/commands/git_commands/submodule.go index 80ccbfcdd..addff67cb 100644 --- a/pkg/commands/git_commands/submodule.go +++ b/pkg/commands/git_commands/submodule.go @@ -54,6 +54,11 @@ func (self *SubmoduleCommands) GetConfigs(parentModule *models.SubmoduleConfig) configs := []*models.SubmoduleConfig{} for _, config := range parsedConfigs { + if isSuspiciousSubmoduleName(config.Name) { + self.Log.Warnf("Ignoring suspicious submodule name: %s", config.Name) + continue + } + configs = append(configs, config) if config.Path != "" { @@ -109,6 +114,22 @@ func parseGitmodules(reader io.Reader, 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, "..") +} + // 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 diff --git a/pkg/commands/git_commands/submodule_test.go b/pkg/commands/git_commands/submodule_test.go index d449d81de..ab2c10474 100644 --- a/pkg/commands/git_commands/submodule_test.go +++ b/pkg/commands/git_commands/submodule_test.go @@ -11,6 +11,37 @@ import ( "github.com/stretchr/testify/assert" ) +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 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 index 497b12134..fdca074cd 100644 --- a/pkg/integration/tests/submodule/ignore_suspicious_name.go +++ b/pkg/integration/tests/submodule/ignore_suspicious_name.go @@ -28,27 +28,9 @@ var IgnoreSuspiciousName = NewIntegrationTest(NewIntegrationTestArgs{ }, Run: func(t *TestDriver, keys config.KeybindingConfig) { t.Views().Submodules().Focus(). - /* EXPECTED: IsEmpty() - ACTUAL: */ - Lines( - Contains("../../../victim").IsSelected(), - ). - Press(keys.Universal.Remove). - Tap(func() { - t.ExpectPopup().Confirmation(). - Title(Equals("Remove submodule")). - Content(Equals("Are you sure you want to remove submodule '../../../victim' and its corresponding directory? This is irreversible.")). - Confirm() - }). - Lines( - Contains("../../../victim").IsSelected(), - ) t.FileSystem(). - /* EXPECTED: PathPresent("../victim/file") - ACTUAL: */ - PathNotPresent("../victim/file") }, }) From e67bdba1d543353aa6f8273b7485cb32568380da Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 7 Nov 2026 21:51:15 +0100 Subject: [PATCH 06/11] Add a test showing that removing a submodule can delete more than its git dir Co-Authored-By: Claude Opus 5.5 --- pkg/commands/git_commands/submodule_test.go | 50 +++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/pkg/commands/git_commands/submodule_test.go b/pkg/commands/git_commands/submodule_test.go index ab2c10474..d431dfda0 100644 --- a/pkg/commands/git_commands/submodule_test.go +++ b/pkg/commands/git_commands/submodule_test.go @@ -1,10 +1,13 @@ 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" @@ -42,6 +45,53 @@ func TestIsSuspiciousSubmoduleName(t *testing.T) { } } +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) + runner. + ExpectGitArgs([]string{"submodule", "deinit", "--force", "--", "sub"}, "", nil). + ExpectGitArgs([]string{"rm", "--force", "-r", "sub"}, "", nil) + instance := buildSubmoduleCommands(commonDeps{ + runner: runner, + repoPaths: MockRepoPaths(filepath.Join(dir, "repo")), + }) + + err := instance.Delete(&models.SubmoduleConfig{Name: s.name, Path: "sub"}) + /* EXPECTED: + assert.Error(t, err) + assert.FileExists(t, victimFile) + ACTUAL: */ + assert.NoError(t, err) + assert.NoFileExists(t, victimFile) + runner.CheckForMissingCalls() + }) + } +} + func TestSubmoduleGetConflictCommits(t *testing.T) { type scenario struct { testName string From bcbc5ebd6dec639c5ce2d651d89d3ffe1b6e9bf8 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 7 Nov 2026 21:53:08 +0100 Subject: [PATCH 07/11] Refuse to remove a submodule whose git dir isn't below .git/modules Git accepts "." as a submodule name, and so does GetConfigs. Removing such a submodule in lazygit deletes .git/modules, together with the git dirs of all other submodules. Any unpushed commits in those submodules are lost. Check the path of the git dir before running any git command, and refuse to remove the submodule if the path isn't below .git/modules. This also protects against names with ".." components, in case one ever gets past GetConfigs. Co-Authored-By: Claude Opus 5.5 --- pkg/commands/git_commands/submodule.go | 16 ++++++++++++++-- pkg/commands/git_commands/submodule_test.go | 7 ------- 2 files changed, 14 insertions(+), 9 deletions(-) diff --git a/pkg/commands/git_commands/submodule.go b/pkg/commands/git_commands/submodule.go index addff67cb..dacab426e 100644 --- a/pkg/commands/git_commands/submodule.go +++ b/pkg/commands/git_commands/submodule.go @@ -2,6 +2,7 @@ package git_commands import ( "bufio" + "fmt" "io" "os" "path/filepath" @@ -275,6 +276,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(), @@ -307,8 +320,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 d431dfda0..ad7ec53b8 100644 --- a/pkg/commands/git_commands/submodule_test.go +++ b/pkg/commands/git_commands/submodule_test.go @@ -72,21 +72,14 @@ func TestSubmoduleDeleteOutsideModulesDir(t *testing.T) { assert.NoError(t, os.WriteFile(victimFile, []byte("content"), 0o644)) runner := oscommands.NewFakeRunner(t) - runner. - ExpectGitArgs([]string{"submodule", "deinit", "--force", "--", "sub"}, "", nil). - ExpectGitArgs([]string{"rm", "--force", "-r", "sub"}, "", nil) instance := buildSubmoduleCommands(commonDeps{ runner: runner, repoPaths: MockRepoPaths(filepath.Join(dir, "repo")), }) err := instance.Delete(&models.SubmoduleConfig{Name: s.name, Path: "sub"}) - /* EXPECTED: assert.Error(t, err) assert.FileExists(t, victimFile) - ACTUAL: */ - assert.NoError(t, err) - assert.NoFileExists(t, victimFile) runner.CheckForMissingCalls() }) } From a159e24919425cdb813f1e4ec19b8c93c5d7b4bc Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 7 Nov 2026 23:06:56 +0100 Subject: [PATCH 08/11] Add a test showing that nuking the working tree can stash changes in other repositories Co-Authored-By: Claude Opus 5.5 --- .../tests/submodule/ignore_suspicious_path.go | 75 +++++++++++++++++++ pkg/integration/tests/test_list.go | 1 + 2 files changed, 76 insertions(+) create mode 100644 pkg/integration/tests/submodule/ignore_suspicious_path.go 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..0f0d0e953 --- /dev/null +++ b/pkg/integration/tests/submodule/ignore_suspicious_path.go @@ -0,0 +1,75 @@ +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(). + /* EXPECTED: + IsEmpty() + ACTUAL: */ + Lines( + Contains("other"), + Contains("linked"), + ) + + 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(). + /* EXPECTED: + FileContent("../other_repo/file", Equals("modified")). + FileContent("../linked_repo/file", Equals("modified")) + ACTUAL: */ + FileContent("../other_repo/file", Equals("original")). + FileContent("../linked_repo/file", Equals("original")) + }, +}) diff --git a/pkg/integration/tests/test_list.go b/pkg/integration/tests/test_list.go index ae54d0f6e..97a225f06 100644 --- a/pkg/integration/tests/test_list.go +++ b/pkg/integration/tests/test_list.go @@ -579,6 +579,7 @@ var tests = []*components.IntegrationTest{ submodule.EnterFromDotfileBareRepo, submodule.EnterNested, submodule.IgnoreSuspiciousName, + submodule.IgnoreSuspiciousPath, submodule.Remove, submodule.RemoveInLinkedWorktree, submodule.RemoveNested, From 93556c1588b69434cc817caf01cfc032af215122 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 7 Nov 2026 23:19:09 +0100 Subject: [PATCH 09/11] Ignore submodules with a suspicious path in .gitmodules If .gitmodules contains a submodule path like "../other_repo", nuking the working tree in lazygit stashes all changes and untracked files of that other repository. The same happens if the path is a symlink that points to it. The changes can be recovered with "git stash pop" in that repository, but the user has no reason to look there. This happens because lazygit runs some commands in the submodule's directory, and takes that directory from .gitmodules as it is. Git itself only uses a path from .gitmodules if its index has a submodule at that path, and such a path can't lead out of the repository. Ignore a submodule unless its path is a directory below the directory that contains the .gitmodules file, without any symlinks in between. This also keeps GetConfigs from reading the same .gitmodules file again and again for a path of ".". Co-Authored-By: Claude Opus 5.5 --- pkg/commands/git_commands/submodule.go | 43 +++++++++++-- pkg/commands/git_commands/submodule_test.go | 63 +++++++++++++++++++ .../tests/submodule/ignore_suspicious_path.go | 10 --- 3 files changed, 101 insertions(+), 15 deletions(-) diff --git a/pkg/commands/git_commands/submodule.go b/pkg/commands/git_commands/submodule.go index dacab426e..982f9416c 100644 --- a/pkg/commands/git_commands/submodule.go +++ b/pkg/commands/git_commands/submodule.go @@ -60,13 +60,16 @@ func (self *SubmoduleCommands) GetConfigs(parentModule *models.SubmoduleConfig) continue } + if isSuspiciousSubmodulePath(dir, config.Path) { + self.Log.Warnf("Ignoring suspicious submodule path: %s", config.Path) + continue + } + configs = append(configs, config) - if config.Path != "" { - nestedConfigs, err := self.GetConfigs(config) - if err == nil { - configs = append(configs, nestedConfigs...) - } + nestedConfigs, err := self.GetConfigs(config) + if err == nil { + configs = append(configs, nestedConfigs...) } } @@ -131,6 +134,36 @@ func isSuspiciousSubmoduleName(name string) bool { 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 diff --git a/pkg/commands/git_commands/submodule_test.go b/pkg/commands/git_commands/submodule_test.go index ad7ec53b8..2740ecf51 100644 --- a/pkg/commands/git_commands/submodule_test.go +++ b/pkg/commands/git_commands/submodule_test.go @@ -45,6 +45,69 @@ func TestIsSuspiciousSubmoduleName(t *testing.T) { } } +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 diff --git a/pkg/integration/tests/submodule/ignore_suspicious_path.go b/pkg/integration/tests/submodule/ignore_suspicious_path.go index 0f0d0e953..d2cf3416e 100644 --- a/pkg/integration/tests/submodule/ignore_suspicious_path.go +++ b/pkg/integration/tests/submodule/ignore_suspicious_path.go @@ -38,13 +38,7 @@ var IgnoreSuspiciousPath = NewIntegrationTest(NewIntegrationTestArgs{ }, Run: func(t *TestDriver, keys config.KeybindingConfig) { t.Views().Submodules(). - /* EXPECTED: IsEmpty() - ACTUAL: */ - Lines( - Contains("other"), - Contains("linked"), - ) t.Views().Files().Focus(). Lines( @@ -65,11 +59,7 @@ var IgnoreSuspiciousPath = NewIntegrationTest(NewIntegrationTestArgs{ IsEmpty() t.FileSystem(). - /* EXPECTED: FileContent("../other_repo/file", Equals("modified")). FileContent("../linked_repo/file", Equals("modified")) - ACTUAL: */ - FileContent("../other_repo/file", Equals("original")). - FileContent("../linked_repo/file", Equals("original")) }, }) From aecd5e5bb4d1157e732bb3597404a590f5664233 Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 7 Nov 2026 23:27:21 +0100 Subject: [PATCH 10/11] Add a test showing that .gitmodules keys are found in the middle of a line Co-Authored-By: Claude Opus 5.5 --- pkg/commands/git_commands/submodule_test.go | 68 +++++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/pkg/commands/git_commands/submodule_test.go b/pkg/commands/git_commands/submodule_test.go index 2740ecf51..c1bf458c0 100644 --- a/pkg/commands/git_commands/submodule_test.go +++ b/pkg/commands/git_commands/submodule_test.go @@ -14,6 +14,74 @@ import ( "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: + expected: []*models.SubmoduleConfig{ + {Name: "sub", Path: "sub", Url: "https://example.com/repo?path=other"}, + }, + ACTUAL: */ + expected: []*models.SubmoduleConfig{ + {Name: "sub", Path: "other"}, + }, + }, + { + testName: "commented-out section", + lines: []string{ + `[submodule "sub"]`, + "\tpath = sub", + "\turl = ../sub", + `#[submodule "old"]`, + "#\tpath = old", + "#\turl = ../old", + }, + /* EXPECTED: + expected: []*models.SubmoduleConfig{ + {Name: "sub", Path: "sub", Url: "../sub"}, + }, + ACTUAL: */ + expected: []*models.SubmoduleConfig{ + {Name: "sub", Path: "sub", Url: "../sub"}, + {Name: "old", Path: "old", Url: "../old"}, + }, + }, + } + + 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 From bb4c6813306832a583dea12ec698385e5aa9854b Mon Sep 17 00:00:00 2001 From: Stefan Haller Date: Sat, 7 Nov 2026 23:29:08 +0100 Subject: [PATCH 11/11] Only recognize .gitmodules keys and section headers at the start of a line If a submodule's url contains "path=", for example "https://example.com/repo?path=other", lazygit takes "other" for the submodule's path and loses the url. Also, a commented-out section in .gitmodules shows up as a submodule. The regexes that parse .gitmodules aren't anchored, so they find a key or a section header anywhere in a line. Anchor them at the start of the line. Co-Authored-By: Claude Opus 5.5 --- pkg/commands/git_commands/submodule.go | 6 +++--- pkg/commands/git_commands/submodule_test.go | 11 ----------- 2 files changed, 3 insertions(+), 14 deletions(-) diff --git a/pkg/commands/git_commands/submodule.go b/pkg/commands/git_commands/submodule.go index 982f9416c..a31824e95 100644 --- a/pkg/commands/git_commands/submodule.go +++ b/pkg/commands/git_commands/submodule.go @@ -94,7 +94,7 @@ func parseGitmodules(reader io.Reader, parentModule *models.SubmoduleConfig) ([] 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, }) @@ -103,9 +103,9 @@ func parseGitmodules(reader io.Reader, parentModule *models.SubmoduleConfig) ([] if len(configs) > 0 { config := configs[len(configs)-1] - if path, ok := firstMatch(line, `\s*path\s*=\s*(.*)\s*`); ok { + 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 { + } else if url, ok := firstMatch(line, `^\s*url\s*=\s*(.*)\s*`); ok { config.Url = url } } diff --git a/pkg/commands/git_commands/submodule_test.go b/pkg/commands/git_commands/submodule_test.go index c1bf458c0..554d0175f 100644 --- a/pkg/commands/git_commands/submodule_test.go +++ b/pkg/commands/git_commands/submodule_test.go @@ -42,14 +42,9 @@ func TestParseGitmodules(t *testing.T) { "\tpath = sub", "\turl = https://example.com/repo?path=other", }, - /* EXPECTED: expected: []*models.SubmoduleConfig{ {Name: "sub", Path: "sub", Url: "https://example.com/repo?path=other"}, }, - ACTUAL: */ - expected: []*models.SubmoduleConfig{ - {Name: "sub", Path: "other"}, - }, }, { testName: "commented-out section", @@ -61,15 +56,9 @@ func TestParseGitmodules(t *testing.T) { "#\tpath = old", "#\turl = ../old", }, - /* EXPECTED: expected: []*models.SubmoduleConfig{ {Name: "sub", Path: "sub", Url: "../sub"}, }, - ACTUAL: */ - expected: []*models.SubmoduleConfig{ - {Name: "sub", Path: "sub", Url: "../sub"}, - {Name: "old", Path: "old", Url: "../old"}, - }, }, }