Fix a bug when uploading file via lfs ssh command (#34408) (#34416)

Backport #34408 by @lunny

Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>

cmd/serv.go

@@ -11,7 +11,6 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
-	"regexp"
 	"strconv"
 	"strings"
 	"time"
@@ -20,7 +19,7 @@ import (
 	asymkey_model "code.gitea.io/gitea/models/asymkey"
 	git_model "code.gitea.io/gitea/models/git"
 	"code.gitea.io/gitea/models/perm"
-	"code.gitea.io/gitea/modules/container"
+	"code.gitea.io/gitea/models/repo"
 	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/json"
 	"code.gitea.io/gitea/modules/lfstransfer"
@@ -37,14 +36,6 @@ import (
 	"github.com/urfave/cli/v2"
 )
 
-const (
-	verbUploadPack      = "git-upload-pack"
-	verbUploadArchive   = "git-upload-archive"
-	verbReceivePack     = "git-receive-pack"
-	verbLfsAuthenticate = "git-lfs-authenticate"
-	verbLfsTransfer     = "git-lfs-transfer"
-)
-
 // CmdServ represents the available serv sub-command.
 var CmdServ = &cli.Command{
 	Name:        "serv",
@@ -78,22 +69,6 @@ func setup(ctx context.Context, debug bool) {
 	}
 }
 
-var (
-	// keep getAccessMode() in sync
-	allowedCommands = container.SetOf(
-		verbUploadPack,
-		verbUploadArchive,
-		verbReceivePack,
-		verbLfsAuthenticate,
-		verbLfsTransfer,
-	)
-	allowedCommandsLfs = container.SetOf(
-		verbLfsAuthenticate,
-		verbLfsTransfer,
-	)
-	alphaDashDotPattern = regexp.MustCompile(`[^\w-\.]`)
-)
-
 // fail prints message to stdout, it's mainly used for git serv and git hook commands.
 // The output will be passed to git client and shown to user.
 func fail(ctx context.Context, userMessage, logMsgFmt string, args ...any) error {
@@ -139,19 +114,20 @@ func handleCliResponseExtra(extra private.ResponseExtra) error {
 
 func getAccessMode(verb, lfsVerb string) perm.AccessMode {
 	switch verb {
-	case verbUploadPack, verbUploadArchive:
+	case git.CmdVerbUploadPack, git.CmdVerbUploadArchive:
 		return perm.AccessModeRead
-	case verbReceivePack:
+	case git.CmdVerbReceivePack:
 		return perm.AccessModeWrite
-	case verbLfsAuthenticate, verbLfsTransfer:
+	case git.CmdVerbLfsAuthenticate, git.CmdVerbLfsTransfer:
 		switch lfsVerb {
-		case "upload":
+		case git.CmdSubVerbLfsUpload:
 			return perm.AccessModeWrite
-		case "download":
+		case git.CmdSubVerbLfsDownload:
 			return perm.AccessModeRead
 		}
 	}
 	// should be unreachable
+	setting.PanicInDevOrTesting("unknown verb: %s %s", verb, lfsVerb)
 	return perm.AccessModeNone
 }
 
@@ -230,12 +206,12 @@ func runServ(c *cli.Context) error {
 		log.Debug("SSH_ORIGINAL_COMMAND: %s", os.Getenv("SSH_ORIGINAL_COMMAND"))
 	}
 
-	words, err := shellquote.Split(cmd)
+	sshCmdArgs, err := shellquote.Split(cmd)
 	if err != nil {
 		return fail(ctx, "Error parsing arguments", "Failed to parse arguments: %v", err)
 	}
 
-	if len(words) < 2 {
+	if len(sshCmdArgs) < 2 {
 		if git.DefaultFeatures().SupportProcReceive {
 			// for AGit Flow
 			if cmd == "ssh_info" {
@@ -246,25 +222,21 @@ func runServ(c *cli.Context) error {
 		return fail(ctx, "Too few arguments", "Too few arguments in cmd: %s", cmd)
 	}
 
-	verb := words[0]
-	repoPath := strings.TrimPrefix(words[1], "/")
-
-	var lfsVerb string
-
-	rr := strings.SplitN(repoPath, "/", 2)
-	if len(rr) != 2 {
+	repoPath := strings.TrimPrefix(sshCmdArgs[1], "/")
+	repoPathFields := strings.SplitN(repoPath, "/", 2)
+	if len(repoPathFields) != 2 {
 		return fail(ctx, "Invalid repository path", "Invalid repository path: %v", repoPath)
 	}
 
-	username := rr[0]
-	reponame := strings.TrimSuffix(rr[1], ".git")
+	username := repoPathFields[0]
+	reponame := strings.TrimSuffix(repoPathFields[1], ".git") // “the-repo-name" or "the-repo-name.wiki"
 
 	// LowerCase and trim the repoPath as that's how they are stored.
 	// This should be done after splitting the repoPath into username and reponame
 	// so that username and reponame are not affected.
 	repoPath = strings.ToLower(strings.TrimSpace(repoPath))
 
-	if alphaDashDotPattern.MatchString(reponame) {
+	if !repo.IsValidSSHAccessRepoName(reponame) {
 		return fail(ctx, "Invalid repo name", "Invalid repo name: %s", reponame)
 	}
 
@@ -286,21 +258,22 @@ func runServ(c *cli.Context) error {
 		}()
 	}
 
-	if allowedCommands.Contains(verb) {
-		if allowedCommandsLfs.Contains(verb) {
+	verb, lfsVerb := sshCmdArgs[0], ""
+	if !git.IsAllowedVerbForServe(verb) {
+		return fail(ctx, "Unknown git command", "Unknown git command %s", verb)
+	}
+
+	if git.IsAllowedVerbForServeLfs(verb) {
 		if !setting.LFS.StartServer {
 			return fail(ctx, "LFS Server is not enabled", "")
 		}
-			if verb == verbLfsTransfer && !setting.LFS.AllowPureSSH {
+		if verb == git.CmdVerbLfsTransfer && !setting.LFS.AllowPureSSH {
 			return fail(ctx, "LFS SSH transfer is not enabled", "")
 		}
-			if len(words) > 2 {
-				lfsVerb = words[2]
+		if len(sshCmdArgs) > 2 {
+			lfsVerb = sshCmdArgs[2]
 		}
 	}
-	} else {
-		return fail(ctx, "Unknown git command", "Unknown git command %s", verb)
-	}
 
 	requestedMode := getAccessMode(verb, lfsVerb)
 
@@ -310,7 +283,7 @@ func runServ(c *cli.Context) error {
 	}
 
 	// LFS SSH protocol
-	if verb == verbLfsTransfer {
+	if verb == git.CmdVerbLfsTransfer {
 		token, err := getLFSAuthToken(ctx, lfsVerb, results)
 		if err != nil {
 			return err
@@ -319,7 +292,7 @@ func runServ(c *cli.Context) error {
 	}
 
 	// LFS token authentication
-	if verb == verbLfsAuthenticate {
+	if verb == git.CmdVerbLfsAuthenticate {
 		url := fmt.Sprintf("%s%s/%s.git/info/lfs", setting.AppURL, url.PathEscape(results.OwnerName), url.PathEscape(results.RepoName))
 
 		token, err := getLFSAuthToken(ctx, lfsVerb, results)

models/repo/repo.go

@@ -67,15 +67,15 @@ type globalVarsStruct struct {
 	validRepoNamePattern     *regexp.Regexp
 	invalidRepoNamePattern   *regexp.Regexp
 	reservedRepoNames        []string
-	reservedRepoPatterns   []string
+	reservedRepoNamePatterns []string
 }
 
 var globalVars = sync.OnceValue(func() *globalVarsStruct {
 	return &globalVarsStruct{
-		validRepoNamePattern:   regexp.MustCompile(`[-.\w]+`),
+		validRepoNamePattern:     regexp.MustCompile(`^[-.\w]+$`),
 		invalidRepoNamePattern:   regexp.MustCompile(`[.]{2,}`),
 		reservedRepoNames:        []string{".", "..", "-"},
-		reservedRepoPatterns:   []string{"*.git", "*.wiki", "*.rss", "*.atom"},
+		reservedRepoNamePatterns: []string{"*.wiki", "*.git", "*.rss", "*.atom"},
 	}
 })
 
@@ -86,7 +86,16 @@ func IsUsableRepoName(name string) error {
 		// Note: usually this error is normally caught up earlier in the UI
 		return db.ErrNameCharsNotAllowed{Name: name}
 	}
-	return db.IsUsableName(vars.reservedRepoNames, vars.reservedRepoPatterns, name)
+	return db.IsUsableName(vars.reservedRepoNames, vars.reservedRepoNamePatterns, name)
+}
+
+// IsValidSSHAccessRepoName is like IsUsableRepoName, but it allows "*.wiki" because wiki repo needs to be accessed in SSH code
+func IsValidSSHAccessRepoName(name string) bool {
+	vars := globalVars()
+	if !vars.validRepoNamePattern.MatchString(name) || vars.invalidRepoNamePattern.MatchString(name) {
+		return false
+	}
+	return db.IsUsableName(vars.reservedRepoNames, vars.reservedRepoNamePatterns[1:], name) == nil
 }
 
 // TrustModelType defines the types of trust model for this repository

models/repo/repo_test.go

@@ -216,8 +216,23 @@ func TestIsUsableRepoName(t *testing.T) {
 
 	assert.Error(t, IsUsableRepoName("-"))
 	assert.Error(t, IsUsableRepoName("🌞"))
+	assert.Error(t, IsUsableRepoName("the/repo"))
 	assert.Error(t, IsUsableRepoName("the..repo"))
 	assert.Error(t, IsUsableRepoName("foo.wiki"))
 	assert.Error(t, IsUsableRepoName("foo.git"))
 	assert.Error(t, IsUsableRepoName("foo.RSS"))
 }
+
+func TestIsValidSSHAccessRepoName(t *testing.T) {
+	assert.True(t, IsValidSSHAccessRepoName("a"))
+	assert.True(t, IsValidSSHAccessRepoName("-1_."))
+	assert.True(t, IsValidSSHAccessRepoName(".profile"))
+	assert.True(t, IsValidSSHAccessRepoName("foo.wiki"))
+
+	assert.False(t, IsValidSSHAccessRepoName("-"))
+	assert.False(t, IsValidSSHAccessRepoName("🌞"))
+	assert.False(t, IsValidSSHAccessRepoName("the/repo"))
+	assert.False(t, IsValidSSHAccessRepoName("the..repo"))
+	assert.False(t, IsValidSSHAccessRepoName("foo.git"))
+	assert.False(t, IsValidSSHAccessRepoName("foo.RSS"))
+}

modules/git/cmdverb.go

@@ -0,0 +1,36 @@
+// Copyright 2025 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package git
+
+const (
+	CmdVerbUploadPack      = "git-upload-pack"
+	CmdVerbUploadArchive   = "git-upload-archive"
+	CmdVerbReceivePack     = "git-receive-pack"
+	CmdVerbLfsAuthenticate = "git-lfs-authenticate"
+	CmdVerbLfsTransfer     = "git-lfs-transfer"
+
+	CmdSubVerbLfsUpload   = "upload"
+	CmdSubVerbLfsDownload = "download"
+)
+
+func IsAllowedVerbForServe(verb string) bool {
+	switch verb {
+	case CmdVerbUploadPack,
+		CmdVerbUploadArchive,
+		CmdVerbReceivePack,
+		CmdVerbLfsAuthenticate,
+		CmdVerbLfsTransfer:
+		return true
+	}
+	return false
+}
+
+func IsAllowedVerbForServeLfs(verb string) bool {
+	switch verb {
+	case CmdVerbLfsAuthenticate,
+		CmdVerbLfsTransfer:
+		return true
+	}
+	return false
+}

modules/private/serv.go

@@ -46,18 +46,16 @@ type ServCommandResults struct {
 }
 
 // ServCommand preps for a serv call
-func ServCommand(ctx context.Context, keyID int64, ownerName, repoName string, mode perm.AccessMode, verbs ...string) (*ServCommandResults, ResponseExtra) {
+func ServCommand(ctx context.Context, keyID int64, ownerName, repoName string, mode perm.AccessMode, verb, lfsVerb string) (*ServCommandResults, ResponseExtra) {
 	reqURL := setting.LocalURL + fmt.Sprintf("api/internal/serv/command/%d/%s/%s?mode=%d",
 		keyID,
 		url.PathEscape(ownerName),
 		url.PathEscape(repoName),
 		mode,
 	)
-	for _, verb := range verbs {
-		if verb != "" {
 	reqURL += "&verb=" + url.QueryEscape(verb)
-		}
-	}
+	// reqURL += "&lfs_verb=" + url.QueryEscape(lfsVerb) // TODO: actually there is no use of this parameter. In the future, the URL construction should be more flexible
+	_ = lfsVerb
 	req := newInternalRequestAPI(ctx, reqURL, "GET")
 	return requestJSONResp(req, &ServCommandResults{})
 }

routers/private/serv.go

@@ -81,6 +81,7 @@ func ServCommand(ctx *context.PrivateContext) {
 	ownerName := ctx.PathParam("owner")
 	repoName := ctx.PathParam("repo")
 	mode := perm.AccessMode(ctx.FormInt("mode"))
+	verb := ctx.FormString("verb")
 
 	// Set the basic parts of the results to return
 	results := private.ServCommandResults{
@@ -295,8 +296,11 @@ func ServCommand(ctx *context.PrivateContext) {
 				return
 			}
 		} else {
-			// Because of the special ref "refs/for" we will need to delay write permission check
-			if git.DefaultFeatures().SupportProcReceive && unitType == unit.TypeCode {
+			// Because of the special ref "refs/for" (AGit) we will need to delay write permission check,
+			// AGit flow needs to write its own ref when the doer has "reader" permission (allowing to create PR).
+			// The real permission check is done in HookPreReceive (routers/private/hook_pre_receive.go).
+			// Here it should relax the permission check for "git push (git-receive-pack)", but not for others like LFS operations.
+			if git.DefaultFeatures().SupportProcReceive && unitType == unit.TypeCode && verb == git.CmdVerbReceivePack {
 				mode = perm.AccessModeRead
 			}
 

tests/integration/git_general_test.go

@@ -11,8 +11,10 @@ import (
 	"net/http"
 	"net/url"
 	"os"
+	"os/exec"
 	"path"
 	"path/filepath"
+	"slices"
 	"strconv"
 	"testing"
 	"time"
@@ -30,6 +32,7 @@ import (
 	api "code.gitea.io/gitea/modules/structs"
 	"code.gitea.io/gitea/tests"
 
+	"github.com/kballard/go-shellquote"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -105,7 +108,12 @@ func testGitGeneral(t *testing.T, u *url.URL) {
 
 		// Setup key the user ssh key
 		withKeyFile(t, keyname, func(keyFile string) {
-			t.Run("CreateUserKey", doAPICreateUserKey(sshContext, "test-key", keyFile))
+			var keyID int64
+			t.Run("CreateUserKey", doAPICreateUserKey(sshContext, "test-key", keyFile, func(t *testing.T, key api.PublicKey) {
+				keyID = key.ID
+			}))
+			assert.NotZero(t, keyID)
+			t.Run("LFSAccessTest", doSSHLFSAccessTest(sshContext, keyID))
 
 			// Setup remote link
 			// TODO: get url from api
@@ -136,6 +144,36 @@ func testGitGeneral(t *testing.T, u *url.URL) {
 	})
 }
 
+func doSSHLFSAccessTest(_ APITestContext, keyID int64) func(*testing.T) {
+	return func(t *testing.T) {
+		sshCommand := os.Getenv("GIT_SSH_COMMAND")       // it is set in withKeyFile
+		sshCmdParts, err := shellquote.Split(sshCommand) // and parse the ssh command to construct some mocked arguments
+		require.NoError(t, err)
+
+		t.Run("User2AccessOwned", func(t *testing.T) {
+			sshCmdUser2Self := append(slices.Clone(sshCmdParts),
+				"-p", strconv.Itoa(setting.SSH.ListenPort), "git@"+setting.SSH.ListenHost,
+				"git-lfs-authenticate", "user2/repo1.git", "upload", // accessible to own repo
+			)
+			cmd := exec.CommandContext(t.Context(), sshCmdUser2Self[0], sshCmdUser2Self[1:]...)
+			_, err := cmd.Output()
+			assert.NoError(t, err) // accessible, no error
+		})
+
+		t.Run("User2AccessOther", func(t *testing.T) {
+			sshCmdUser2Other := append(slices.Clone(sshCmdParts),
+				"-p", strconv.Itoa(setting.SSH.ListenPort), "git@"+setting.SSH.ListenHost,
+				"git-lfs-authenticate", "user5/repo4.git", "upload", // inaccessible to other's (user5/repo4)
+			)
+			cmd := exec.CommandContext(t.Context(), sshCmdUser2Other[0], sshCmdUser2Other[1:]...)
+			_, err := cmd.Output()
+			var errExit *exec.ExitError
+			require.ErrorAs(t, err, &errExit) // inaccessible, error
+			assert.Contains(t, string(errExit.Stderr), fmt.Sprintf("User: 2:user2 with Key: %d:test-key is not authorized to write to user5/repo4.", keyID))
+		})
+	}
+}
+
 func ensureAnonymousClone(t *testing.T, u *url.URL) {
 	dstLocalPath := t.TempDir()
 	t.Run("CloneAnonymous", doGitClone(dstLocalPath, u))