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