Refactor legacy strange git operations (#22756)

During the refactoring of the git module, I found there were some
strange operations. This PR tries to fix 2 of them

1. The empty argument `--` in repo_attribute.go, which was introduced by
#16773. It seems unnecessary because nothing else would be added later.
2. The complex git service logic in repo/http.go. 
* Before: the `hasAccess` only allow `service == "upload-pack" ||
service == "receive-pack"`
* After: unrelated code is removed. No need to call ToTrustedCmdArgs
anymore.

Co-authored-by: Lunny Xiao <[email protected]>

Author: wxiaoguang

modules/git/repo_attribute.go

@@ -135,8 +135,7 @@ func (c *CheckAttributeReader) Init(ctx context.Context) error {
 
 	c.env = append(c.env, "GIT_FLUSH=1")
 
-	// The empty "--" comes from #16773 , and it seems unnecessary because nothing else would be added later.
-	c.cmd.AddDynamicArguments(c.Attributes...).AddArguments("--")
+	c.cmd.AddDynamicArguments(c.Attributes...)
 
 	var err error
 

routers/web/repo/http.go

@@ -424,60 +424,40 @@ func (h *serviceHandler) sendFile(contentType, file string) {
 // one or more key=value pairs separated by colons
 var safeGitProtocolHeader = regexp.MustCompile(`^[0-9a-zA-Z]+=[0-9a-zA-Z]+(:[0-9a-zA-Z]+=[0-9a-zA-Z]+)*$`)
 
-func getGitConfig(ctx gocontext.Context, option, dir string) string {
-	out, _, err := git.NewCommand(ctx, "config").AddDynamicArguments(option).RunStdString(&git.RunOpts{Dir: dir})
-	if err != nil {
-		log.Error("%v - %s", err, out)
-	}
-	return out[0 : len(out)-1]
-}
-
-func getConfigSetting(ctx gocontext.Context, service, dir string) bool {
-	service = strings.ReplaceAll(service, "-", "")
-	setting := getGitConfig(ctx, "http."+service, dir)
-
-	if service == "uploadpack" {
-		return setting != "false"
-	}
-
-	return setting == "true"
-}
-
-func hasAccess(ctx gocontext.Context, service string, h serviceHandler, checkContentType bool) bool {
-	if checkContentType {
-		if h.r.Header.Get("Content-Type") != fmt.Sprintf("application/x-git-%s-request", service) {
-			return false
-		}
+func prepareGitCmdWithAllowedService(service string, h *serviceHandler) (*git.Command, error) {
+	if service == "receive-pack" && h.cfg.ReceivePack {
+		return git.NewCommand(h.r.Context(), "receive-pack"), nil
 	}
-
-	if !(service == "upload-pack" || service == "receive-pack") {
-		return false
-	}
-	if service == "receive-pack" {
-		return h.cfg.ReceivePack
-	}
-	if service == "upload-pack" {
-		return h.cfg.UploadPack
+	if service == "upload-pack" && h.cfg.UploadPack {
+		return git.NewCommand(h.r.Context(), "upload-pack"), nil
 	}
 
-	return getConfigSetting(ctx, service, h.dir)
+	return nil, fmt.Errorf("service %q is not allowed", service)
 }
 
-func serviceRPC(ctx gocontext.Context, h serviceHandler, service string) {
+func serviceRPC(h *serviceHandler, service string) {
 	defer func() {
 		if err := h.r.Body.Close(); err != nil {
 			log.Error("serviceRPC: Close: %v", err)
 		}
 	}()
 
-	if !hasAccess(ctx, service, h, true) {
+	expectedContentType := fmt.Sprintf("application/x-git-%s-request", service)
+	if h.r.Header.Get("Content-Type") != expectedContentType {
+		log.Error("Content-Type (%q) doesn't match expected: %q", h.r.Header.Get("Content-Type"), expectedContentType)
+		h.w.WriteHeader(http.StatusUnauthorized)
+		return
+	}
+
+	cmd, err := prepareGitCmdWithAllowedService(service, h)
+	if err != nil {
+		log.Error("Failed to prepareGitCmdWithService: %v", err)
 		h.w.WriteHeader(http.StatusUnauthorized)
 		return
 	}
 
 	h.w.Header().Set("Content-Type", fmt.Sprintf("application/x-git-%s-result", service))
 
-	var err error
 	reqBody := h.r.Body
 
 	// Handle GZIP.
@@ -498,8 +478,7 @@ func serviceRPC(ctx gocontext.Context, h serviceHandler, service string) {
 	}
 
 	var stderr bytes.Buffer
-	// the service is generated by ourselves, so it's safe to trust it
-	cmd := git.NewCommand(h.r.Context(), git.ToTrustedCmdArgs([]string{service})...).AddArguments("--stateless-rpc").AddDynamicArguments(h.dir)
+	cmd.AddArguments("--stateless-rpc").AddDynamicArguments(h.dir)
 	cmd.SetDescription(fmt.Sprintf("%s %s %s [repo_path: %s]", git.GitExecutable, service, "--stateless-rpc", h.dir))
 	if err := cmd.Run(&git.RunOpts{
 		Dir:               h.dir,
@@ -520,15 +499,15 @@ func serviceRPC(ctx gocontext.Context, h serviceHandler, service string) {
 func ServiceUploadPack(ctx *context.Context) {
 	h := httpBase(ctx)
 	if h != nil {
-		serviceRPC(ctx, *h, "upload-pack")
+		serviceRPC(h, "upload-pack")
 	}
 }
 
 // ServiceReceivePack implements Git Smart HTTP protocol
 func ServiceReceivePack(ctx *context.Context) {
 	h := httpBase(ctx)
 	if h != nil {
-		serviceRPC(ctx, *h, "receive-pack")
+		serviceRPC(h, "receive-pack")
 	}
 }
 
@@ -537,7 +516,7 @@ func getServiceType(r *http.Request) string {
 	if !strings.HasPrefix(serviceType, "git-") {
 		return ""
 	}
-	return strings.Replace(serviceType, "git-", "", 1)
+	return strings.TrimPrefix(serviceType, "git-")
 }
 
 func updateServerInfo(ctx gocontext.Context, dir string) []byte {
@@ -563,16 +542,15 @@ func GetInfoRefs(ctx *context.Context) {
 		return
 	}
 	h.setHeaderNoCache()
-	if hasAccess(ctx, getServiceType(h.r), *h, false) {
-		service := getServiceType(h.r)
-
+	service := getServiceType(h.r)
+	cmd, err := prepareGitCmdWithAllowedService(service, h)
+	if err == nil {
 		if protocol := h.r.Header.Get("Git-Protocol"); protocol != "" && safeGitProtocolHeader.MatchString(protocol) {
 			h.environ = append(h.environ, "GIT_PROTOCOL="+protocol)
 		}
 		h.environ = append(os.Environ(), h.environ...)
 
-		// the service is generated by ourselves, so we can trust it
-		refs, _, err := git.NewCommand(ctx, git.ToTrustedCmdArgs([]string{service})...).AddArguments("--stateless-rpc", "--advertise-refs", ".").RunStdBytes(&git.RunOpts{Env: h.environ, Dir: h.dir})
+		refs, _, err := cmd.AddArguments("--stateless-rpc", "--advertise-refs", ".").RunStdBytes(&git.RunOpts{Env: h.environ, Dir: h.dir})
 		if err != nil {
 			log.Error(fmt.Sprintf("%v - %s", err, string(refs)))
 		}