From 50111c71c3d8feedb170a0122c97237f78d1751f Mon Sep 17 00:00:00 2001 From: wxiaoguang Date: Mon, 6 Feb 2023 10:23:17 +0800 Subject: [PATCH] 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 --- modules/git/repo_attribute.go | 3 +- routers/web/repo/http.go | 72 ++++++++++++----------------------- 2 files changed, 26 insertions(+), 49 deletions(-) diff --git a/modules/git/repo_attribute.go b/modules/git/repo_attribute.go index e7d5fb68068bb..2b34f117f72f7 100644 --- a/modules/git/repo_attribute.go +++ b/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 diff --git a/routers/web/repo/http.go b/routers/web/repo/http.go index 89c86e764e3cf..e82b94b9e847e 100644 --- a/routers/web/repo/http.go +++ b/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,7 +499,7 @@ 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") } } @@ -528,7 +507,7 @@ func ServiceUploadPack(ctx *context.Context) { 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))) }