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")) }, })