refactor: make git http respond error message (#39390)

* Refactor some bad smells in legacy code
* Fix #37999: instead of creating an empty (undesired) wiki page, just tell users to create a wiki page first

routers/common/errpage.go

@@ -68,7 +68,7 @@ func renderPanicErrorPage(w http.ResponseWriter, req *http.Request, recovered an
 	// This recovery handler could be called without Gitea's web context, so we shouldn't touch that context too much.
 	// Otherwise, the 500-page may cause new panics, eg: cache.GetContextWithData, it makes the developer&users couldn't find the original panic.
 	user, _ := ctxData[middleware.ContextDataKeySignedUser].(*user_model.User)
-	if !setting.IsProd || (user != nil && user.IsAdmin) {
+	if !setting.IsProd || setting.IsInTesting || (user != nil && user.IsAdmin) {
 		plainMsg = "PANIC: " + combinedErr.Error()
 		ctxData["ErrorMsg"] = plainMsg
 	}

routers/web/repo/githttp.go

@@ -7,9 +7,9 @@ package repo
 import (
 	"compress/gzip"
 	"fmt"
+	"io"
 	"net/http"
 	"os"
-	"path"
 	"regexp"
 	"slices"
 	"strconv"
@@ -25,6 +25,7 @@ import (
 	"gitea.dev/modules/git"
 	"gitea.dev/modules/git/gitcmd"
 	"gitea.dev/modules/git/gitrepo"
+	"gitea.dev/modules/httplib"
 	"gitea.dev/modules/log"
 	repo_module "gitea.dev/modules/repository"
 	"gitea.dev/modules/setting"
@@ -232,8 +233,8 @@ func httpBase(ctx *context.Context, optGitService ...string) *serviceHandler {
 
 		repo, err = repo_service.PushCreateRepo(ctx, ctx.Doer, owner, repoName)
 		if err != nil {
-			log.Error("pushCreateRepo: %v", err)
+			log.Debug("PushCreateRepo: %v", err)
-			ctx.Status(http.StatusNotFound)
+			ctx.Status(http.StatusNotFound) // TODO: need to refactor PushCreateRepo and its returned errors
 			return nil
 		}
 	}
@@ -281,7 +282,7 @@ func dummyInfoRefs(ctx *context.Context) {
 			WithDir(tmpEmptyRepoDir).
 			RunStdBytes(ctx)
 		if err != nil {
-			log.Error(fmt.Sprintf("%v - %s", err, string(refs)))
+			log.Error("Failed to prepare git-receive-pack cache: %v", err)
 		}
 
 		log.Debug("populating infoRefsCache: \n%s", string(refs))
@@ -292,9 +293,9 @@ func dummyInfoRefs(ctx *context.Context) {
 	ctx.RespHeader().Set("Pragma", "no-cache")
 	ctx.RespHeader().Set("Cache-Control", "no-cache, max-age=0, must-revalidate")
 	ctx.RespHeader().Set("Content-Type", "application/x-git-receive-pack-advertisement")
-	_, _ = ctx.Write(packetWrite("# service=git-receive-pack\n"))
+	_ = pktLineWriteText(ctx.Resp, "# service=git-receive-pack")
-	_, _ = ctx.Write([]byte("0000"))
+	_ = pktLineWriteFlush(ctx.Resp)
-	_, _ = ctx.Write(infoRefsCache)
+	_, _ = ctx.Resp.Write(infoRefsCache)
 }
 
 type serviceHandler struct {
@@ -326,29 +327,27 @@ func setHeaderCacheForever(ctx *context.Context) {
 	ctx.Resp.Header().Set("Cache-Control", "public, max-age=31536000")
 }
 
-func containsParentDirectorySeparator(v string) bool {
-	if !strings.Contains(v, "..") {
-		return false
-	}
-	return slices.Contains(strings.FieldsFunc(v, isSlashRune), "..")
-}
-
-func isSlashRune(r rune) bool { return r == '/' || r == '\\' }
-
 func (h *serviceHandler) sendFile(ctx *context.Context, contentType, file string) {
-	if containsParentDirectorySeparator(file) {
-		log.Debug("request file path contains invalid path: %v", file)
-		ctx.Resp.WriteHeader(http.StatusBadRequest)
-		return
-	}
-
 	fs := gitrepo.RepoLocalFS(h.getStorageRepo())
 	ctx.Resp.Header().Set("Content-Type", contentType)
-	http.ServeFileFS(ctx.Resp, ctx.Req, fs, path.Clean(file))
+	relPath := util.PathJoinRelX(file)
+	http.ServeFileFS(ctx.Resp, ctx.Req, fs, relPath)
 }
 
 // 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]+)*$`)
+var safeGitProtocolHeader = sync.OnceValue(func() *regexp.Regexp {
+	return regexp.MustCompile(`^[0-9a-zA-Z]+=[0-9a-zA-Z]+(:[0-9a-zA-Z]+=[0-9a-zA-Z]+)*$`)
+})
+
+func prepareGitCmdEnvs(ctx *context.Context, h *serviceHandler, more ...string) []string {
+	envs := slices.Clone(os.Environ())
+	envs = append(envs, h.environ...)
+	envs = append(envs, more...)
+	if protocol := ctx.Req.Header.Get("Git-Protocol"); protocol != "" && safeGitProtocolHeader().MatchString(protocol) {
+		envs = append(envs, "GIT_PROTOCOL="+protocol)
+	}
+	return envs
+}
 
 func prepareGitCmdWithAllowedService(service string, allowedServices []string) *gitcmd.Command {
 	if !slices.Contains(allowedServices, service) {
@@ -404,23 +403,15 @@ func serviceRPC(ctx *context.Context, service string) {
 		}
 	}
 
-	// set this for allow pre-receive and post-receive execute
+	// set SSH_ORIGINAL_COMMAND to allow pre-receive and post-receive hooks
-	h.environ = append(h.environ, "SSH_ORIGINAL_COMMAND="+service)
+	gitCmdEnvs := prepareGitCmdEnvs(ctx, h, "SSH_ORIGINAL_COMMAND="+service)
-
-	if protocol := ctx.Req.Header.Get("Git-Protocol"); protocol != "" && safeGitProtocolHeader.MatchString(protocol) {
-		h.environ = append(h.environ, "GIT_PROTOCOL="+protocol)
-	}
-
 	err := cmd.AddArguments(".").
-		WithRepo(h.getStorageRepo()).WithEnv(append(os.Environ(), h.environ...)).
+		WithRepo(h.getStorageRepo()).WithEnv(gitCmdEnvs).
 		WithStdinCopy(reqBody).
 		WithStdoutCopy(ctx.Resp).
 		RunWithStderr(ctx)
-	if err != nil {
+	if err != nil && !gitcmd.IsErrorCanceledOrKilled(err) && !httplib.IsClientOrNetworkError(ctx, err) {
-		if !gitcmd.IsErrorCanceledOrKilled(err) {
+		log.Error("Fail to serve RPC(%s) for repo %s: %v", service, h.getStorageRepo().LogString(), err)
-			repoLogName := h.repo.FullName() + util.Iif(h.isWiki, ".wiki", "")
-			log.Error("Fail to serve RPC(%s) for repo %s: %v", service, repoLogName, err)
-		}
 	}
 }
 
@@ -444,25 +435,43 @@ func ServiceUploadArchive(ctx *context.Context) {
 	serviceRPC(ctx, ServiceTypeUploadArchive)
 }
 
-func packetWrite(str string) []byte {
+func pktLineWriteText(w io.Writer, str string) error {
-	s := strconv.FormatInt(int64(len(str)+4), 16)
+	// https://git-scm.com/docs/gitprotocol-common
-	if len(s)%4 != 0 {
+	prefix := strconv.FormatInt(int64(len(str)+4+1), 16)
-		s = strings.Repeat("0", 4-len(s)%4) + s
+	if len(prefix)%4 != 0 {
+		prefix = "0000" + prefix
+		prefix = prefix[len(prefix)-4:]
 	}
-	return []byte(s + str)
+	if _, err := io.WriteString(w, prefix); err != nil {
+		return err
+	}
+	if _, err := io.WriteString(w, str); err != nil {
+		return err
+	}
+	_, err := io.WriteString(w, "\n")
+	return err
+}
+
+func pktLineWriteFlush(w io.Writer) error {
+	_, err := io.WriteString(w, "0000")
+	return err
 }
 
 // GetInfoRefs implements Git dumb HTTP
+// ref: https://git-scm.com/docs/gitprotocol-http , https://git-scm.com/docs/gitprotocol-v2
 func GetInfoRefs(ctx *context.Context) {
 	h := httpBase(ctx, ctx.FormString("service")) // git http protocol: "?service=git-<service>"
 	if h == nil {
 		return
 	}
+
+	repo := h.getStorageRepo()
 	setHeaderNoCache(ctx)
+
 	if h.serviceType == "" {
 		// it's said that some legacy git clients will send requests to "/info/refs" without "service" parameter,
 		// although there should be no such case client in the modern days. TODO: not quite sure why we need this UpdateServerInfo logic
-		if err := git.UpdateServerInfo(ctx, h.getStorageRepo()); err != nil {
+		if err := git.UpdateServerInfo(ctx, repo); err != nil {
 			ctx.ServerError("UpdateServerInfo", err)
 			return
 		}
@@ -470,28 +479,43 @@ func GetInfoRefs(ctx *context.Context) {
 		return
 	}
 
+	gitCmdEnvs := prepareGitCmdEnvs(ctx, h)
 	cmd := prepareGitCmdWithAllowedService(h.serviceType, []string{ServiceTypeUploadPack, ServiceTypeReceivePack})
 	if cmd == nil {
 		ctx.Resp.WriteHeader(http.StatusBadRequest)
 		return
 	}
 
-	if protocol := ctx.Req.Header.Get("Git-Protocol"); protocol != "" && safeGitProtocolHeader.MatchString(protocol) {
+	ctx.Resp.Header().Set("Content-Type", fmt.Sprintf("application/x-git-%s-advertisement", h.serviceType))
-		h.environ = append(h.environ, "GIT_PROTOCOL="+protocol)
-	}
-	h.environ = append(os.Environ(), h.environ...)
 
-	cmd = cmd.AddArguments("--stateless-rpc", "--advertise-refs", ".").WithEnv(h.environ)
+	repoExists, err := git.IsRepositoryExist(ctx, repo)
-	refs, _, err := cmd.WithRepo(h.getStorageRepo()).RunStdBytes(ctx)
+	if err != nil {
+		ctx.ServerError("IsRepositoryExist", err)
+		return
+	}
+
+	if !repoExists {
+		ctx.Resp.WriteHeader(http.StatusOK)
+		// error-line = PKT-LINE("ERR" SP explanation-text)
+		errMsg := "repository doesn't exist"
+		if h.isWiki {
+			errMsg = "wiki doesn't exist, please initialize the wiki by creating a new page first"
+		}
+		_ = pktLineWriteText(ctx.Resp, "ERR "+errMsg)
+		return
+	}
+
+	cmd = cmd.AddArguments("--stateless-rpc", "--advertise-refs", ".").WithEnv(gitCmdEnvs)
+	refs, _, err := cmd.WithRepo(repo).RunStdBytes(ctx)
 	if err != nil {
 		ctx.ServerError("RunGitServiceAdvertiseRefs", err)
 		return
 	}
 
-	ctx.Resp.Header().Set("Content-Type", fmt.Sprintf("application/x-git-%s-advertisement", h.serviceType))
+	// https://git-scm.com/docs/gitprotocol-pack
 	ctx.Resp.WriteHeader(http.StatusOK)
-	_, _ = ctx.Resp.Write(packetWrite("# service=git-" + h.serviceType + "\n"))
+	_ = pktLineWriteText(ctx.Resp, "# service=git-"+h.serviceType)
-	_, _ = ctx.Resp.Write([]byte("0000"))
+	_ = pktLineWriteFlush(ctx.Resp)
 	_, _ = ctx.Resp.Write(refs)
 }
 

routers/web/repo/githttp_test.go

@@ -1,42 +0,0 @@
-// Copyright 2021 The Gitea Authors. All rights reserved.
-// SPDX-License-Identifier: MIT
-
-package repo
-
-import (
-	"testing"
-
-	"github.com/stretchr/testify/assert"
-)
-
-func TestContainsParentDirectorySeparator(t *testing.T) {
-	tests := []struct {
-		v string
-		b bool
-	}{
-		{
-			v: `user2/repo1/info/refs`,
-			b: false,
-		},
-		{
-			v: `user2/repo1/HEAD`,
-			b: false,
-		},
-		{
-			v: `user2/repo1/some.../strange_file...mp3`,
-			b: false,
-		},
-		{
-			v: `user2/repo1/../../custom/conf/app.ini`,
-			b: true,
-		},
-		{
-			v: `user2/repo1/objects/info/..\..\..\..\custom\conf\app.ini`,
-			b: true,
-		},
-	}
-
-	for i := range tests {
-		assert.Equal(t, tests[i].b, containsParentDirectorySeparator(tests[i].v))
-	}
-}

services/context/context_response.go

@@ -155,7 +155,7 @@ func (ctx *Context) notFoundInternal(skip int, logMsg string, logErr error) {
 
 func (ctx *Context) buildUserErrorMessage(msg string, err error) (userErrorMsg string) {
 	// it's safe to show internal error to admin users, and it helps
-	if !setting.IsProd || (ctx.Doer != nil && ctx.Doer.IsAdmin) {
+	if !setting.IsProd || setting.IsInTesting || (ctx.Doer != nil && ctx.Doer.IsAdmin) {
 		userErrorMsg = msg
 		if err != nil {
 			userErrorMsg += ", error: " + err.Error()

tests/integration/git_smart_http_test.go

@@ -34,9 +34,10 @@ func TestGitSmartHTTP(t *testing.T) {
 }
 
 func testGitSmartHTTP(t *testing.T, u *url.URL) {
-	kases := []struct {
+	cases := []struct {
 		method, path string
 		code         int
+		contains     string
 	}{
 		{
 			path: "user2/repo1/info/refs",
@@ -51,6 +52,11 @@ func testGitSmartHTTP(t *testing.T, u *url.URL) {
 			path: "user2/repo1/HEAD",
 			code: http.StatusOK,
 		},
+		{
+			path:     "user2/repo2.wiki/info/refs?service=git-upload-pack",
+			code:     http.StatusOK,
+			contains: "ERR wiki doesn't exist",
+		},
 		{
 			path: "user2/repo1/objects/info/alternates",
 			code: http.StatusNotFound,
@@ -73,17 +79,20 @@ func testGitSmartHTTP(t *testing.T, u *url.URL) {
 		},
 	}
 
-	for _, kase := range kases {
+	for _, tc := range cases {
-		t.Run(kase.path, func(t *testing.T) {
+		t.Run(tc.path, func(t *testing.T) {
-			req, err := http.NewRequest(util.IfZero(kase.method, "GET"), u.String()+kase.path, nil)
+			req, err := http.NewRequest(util.IfZero(tc.method, "GET"), u.String()+tc.path, nil)
 			require.NoError(t, err)
 			req.SetBasicAuth("user2", userPassword)
 			resp, err := http.DefaultClient.Do(req)
 			require.NoError(t, err)
 			defer resp.Body.Close()
-			assert.Equal(t, kase.code, resp.StatusCode)
+			assert.Equal(t, tc.code, resp.StatusCode)
-			_, err = io.ReadAll(resp.Body)
+			respBody, err := io.ReadAll(resp.Body)
 			require.NoError(t, err)
+			if tc.contains != "" {
+				assert.Contains(t, string(respBody), tc.contains)
+			}
 		})
 	}
 }