diff --git a/CHANGELOG.md b/CHANGELOG.md index 28c35196f919..1a880b4774be 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,6 +27,7 @@ All notable changes to Sourcegraph are documented in this file. - OpenTelemetry Collector has been upgraded to v0.81, and OpenTelemetry packages have been upgraded to v1.16. [#54969](https://github.com/sourcegraph/sourcegraph/pull/54969), [#54999](https://github.com/sourcegraph/sourcegraph/pull/54999) - Bitbucket Cloud code host connections no longer automatically syncs the repository of the username used. The appropriate workspace name will have to be added to the `teams` list if repositories for that account need to be synced. [#55095](https://github.com/sourcegraph/sourcegraph/pull/55095) - Pressing `Mod-f` will always select the input value in the file view search [#55546](https://github.com/sourcegraph/sourcegraph/pull/55546) +- The commit message defined in a batch spec will now be passed to `git commit` on stdin using `--file=-` instead of being included inline with `git commit -m` to improve how the message is interpreted by `git` in certain edge cases, such as when the commit message begins with a dash, and to prevent extra quotes being added to the message. This may mean that previous escaping strategies will behave differently. ### Fixed diff --git a/cmd/gitserver/server/patch.go b/cmd/gitserver/server/patch.go index f11d55e211b6..0f289612fd86 100644 --- a/cmd/gitserver/server/patch.go +++ b/cmd/gitserver/server/patch.go @@ -17,6 +17,7 @@ import ( "time" "github.com/sourcegraph/log" + "github.com/sourcegraph/sourcegraph/internal/api" "github.com/sourcegraph/sourcegraph/internal/gitserver/gitdomain" @@ -227,11 +228,14 @@ func (s *Server) createCommitFromPatch(ctx context.Context, req protocol.CreateC committerEmail = authorEmail } - gitCommitArgs := []string{"commit"} - for _, m := range messages { - gitCommitArgs = append(gitCommitArgs, "-m", stylizeCommitMessage(m)) - } - cmd = exec.CommandContext(ctx, "git", gitCommitArgs...) + // Commit messages can be arbitrary strings, so using `-m` runs into problems. + // Instead, feed the commit messages to stdin. + cmd = exec.CommandContext(ctx, "git", "commit", "-F", "-") + // NOTE: join messages with a blank line in between ("\n\n") + // because the previous behavior was to use multiple -m arguments, + // which concatenate with a blank line in between. + // Gerrit is the only code host that usees multiple messages at the moment + cmd.Stdin = strings.NewReader(strings.Join(messages, "\n\n")) cmd.Dir = tmpRepoDir cmd.Env = append(os.Environ(), []string{ @@ -395,17 +399,6 @@ func (s *Server) repoRemoteRefs(ctx context.Context, remoteURL *vcs.URL, repoNam return refs, nil } -func stylizeCommitMessage(message string) string { - if styleMessage(message) { - return fmt.Sprintf("%q", message) - } - return message -} - -func styleMessage(message string) bool { - return !strings.HasPrefix(message, "Change-Id: I") -} - func (s *Server) shelveChangelist(ctx context.Context, req protocol.CreateCommitFromPatchRequest, patchCommit string, remoteURL *vcs.URL, tmpGitPathEnv, altObjectsEnv string) (string, error) { repo := string(req.Repo) diff --git a/internal/gitserver/gitdomain/exec.go b/internal/gitserver/gitdomain/exec.go index 8d77657cb370..572a29443765 100644 --- a/internal/gitserver/gitdomain/exec.go +++ b/internal/gitserver/gitdomain/exec.go @@ -1,6 +1,7 @@ package gitdomain import ( + "fmt" "os" "strconv" "strings" @@ -141,8 +142,20 @@ func IsAllowedGitCmd(logger log.Logger, args []string) bool { logger.Warn("command not allowed", log.String("cmd", cmd)) return false } + + // I hate state machines, but I hate them less than complicated multi-argument checking + checkFileInput := false for i, arg := range args[1:] { + if checkFileInput { + if arg == "-" { + checkFileInput = false + continue + } + logger.Warn("IsAllowedGitCmd: unallowed file input for `git commit`", log.String("cmd", cmd), log.String("arg", arg)) + return false + } if strings.HasPrefix(arg, "-") { + fmt.Printf("EVALUATING ARGUMENT %d: %s\n", i, arg) // Special-case `git log -S` and `git log -G`, which interpret any characters // after their 'S' or 'G' as part of the query. There is no long form of this // flags (such as --something=query), so if we did not special-case these, there @@ -162,6 +175,24 @@ func IsAllowedGitCmd(logger log.Logger, args []string) bool { continue // this arg is OK } + // For `git commit`, allow reading the commit message from stdin + // but don't just blindly accept the `--file` or `-F` args + // because they could be used to read arbitrary files. + // Instead, accept only the forms that read from stdin. + if cmd == "commit" { + if arg == "--file=-" { + continue + } + // checking `-F` requires a second check for `-` in the next argument + // Instead of an obtuse check of next and previous arguments, set state and check it the next time around + // Here's the alternative obtuse check of previous and next arguments: + // (arg == "-F" && len(args) > i+2 && args[i+2] == "-") || (arg == "-" && args[i] == "-F") + if arg == "-F" { + checkFileInput = true + continue + } + } + if !isAllowedGitArg(allowedArgs, arg) { logger.Warn("IsAllowedGitCmd.isAllowedGitArgcmd", log.String("cmd", cmd), log.String("arg", arg)) return false