manually backport #55657 into 5.1 (#56219)

Author: peterguy

CHANGELOG.md

@@ -29,10 +29,9 @@ 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)
 - The gRPC implementation for the Symbol service's `LocalCodeIntel` endpoint has been changed to stream its results. [#55242](https://github.com/sourcegraph/sourcegraph/pull/55242)
-- Pressing `Mod-f` will always select the input value in the file view search [#55546](https://github.com/sourcegraph/sourcegraph/pull/55546)
 - When using OpenAI or Azure OpenAI for Cody completions, code completions will be disabled - chat will continue to work. This is because we currently don't support code completions with OpenAI. [#55624](https://github.com/sourcegraph/sourcegraph/pull/55624)
 - Caddy has been updated to version 2.7.3 resolving a number of vulnerabilities. [#55606](https://github.com/sourcegraph/sourcegraph/pull/55606)
-- 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.
+- 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. [#55657](https://github.com/sourcegraph/sourcegraph/pull/55657)
 
 ### Fixed
 

cmd/gitserver/server/patch.go

@@ -227,11 +227,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 uses multiple messages at the moment
+	cmd.Stdin = strings.NewReader(strings.Join(messages, "\n\n"))
 
 	cmd.Dir = tmpRepoDir
 	cmd.Env = append(os.Environ(), []string{
@@ -351,17 +354,6 @@ func (s *Server) createCommitFromPatch(ctx context.Context, req protocol.CreateC
 	return http.StatusOK, resp
 }
 
-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)

internal/gitserver/gitdomain/exec.go

@@ -141,7 +141,18 @@ 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, "-") {
 			// 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
@@ -162,6 +173,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