fix

Author: wxiaoguang

cmd/serv.go

@@ -246,11 +246,7 @@ 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 {
 		return fail(ctx, "Invalid repository path", "Invalid repository path: %v", repoPath)
@@ -286,22 +282,25 @@ func runServ(c *cli.Context) error {
 		}()
 	}
 
-	if allowedCommands.Contains(verb) {
-		if allowedCommandsLfs.Contains(verb) {
-			if !setting.LFS.StartServer {
-				return fail(ctx, "LFS Server is not enabled", "")
-			}
-			if verb == verbLfsTransfer && !setting.LFS.AllowPureSSH {
-				return fail(ctx, "LFS SSH transfer is not enabled", "")
-			}
-			if len(words) > 2 {
-				lfsVerb = words[2]
-			}
-		}
-	} else {
+	verb := words[0]
+	var lfsVerb string
+
+	if !allowedCommands.Contains(verb) {
 		return fail(ctx, "Unknown git command", "Unknown git command %s", verb)
 	}
 
+	if allowedCommandsLfs.Contains(verb) {
+		if !setting.LFS.StartServer {
+			return fail(ctx, "LFS Server is not enabled", "")
+		}
+		if verb == verbLfsTransfer && !setting.LFS.AllowPureSSH {
+			return fail(ctx, "LFS SSH transfer is not enabled", "")
+		}
+		if len(words) > 2 {
+			lfsVerb = words[2]
+		}
+	}
+
 	requestedMode := getAccessMode(verb, lfsVerb)
 
 	results, extra := private.ServCommand(ctx, keyID, username, reponame, requestedMode, verb, lfsVerb)

modules/private/serv.go

@@ -46,17 +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 += "&verb=" + url.QueryEscape(verb)
+	if lfsVerb != "" {
+		reqURL += "&lfs_verb=" + url.QueryEscape(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 && verb == "git-receive-pack" && unitType == unit.TypeCode {
 				mode = perm.AccessModeRead
 			}
 

tests/integration/git_general_test.go

@@ -11,6 +11,7 @@ import (
 	"net/http"
 	"net/url"
 	"os"
+	"os/exec"
 	"path"
 	"path/filepath"
 	"strconv"
@@ -30,6 +31,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 +107,11 @@ 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)
 
 			// Setup remote link
 			// TODO: get url from api
@@ -132,10 +138,30 @@ func testGitGeneral(t *testing.T, u *url.URL) {
 			})
 
 			t.Run("PushCreate", doPushCreate(sshContext, sshURL))
+			t.Run("LFSUploadTest", doLFSUploadTest(sshContext, keyID))
 		})
 	})
 }
 
+func doLFSUploadTest(_ APITestContext, keyID int64) func(*testing.T) {
+	return func(t *testing.T) {
+		sshCommand := os.Getenv("GIT_SSH_COMMAND")       // it is set in withKeyFile and ensure correctly
+		sshCmdParts, err := shellquote.Split(sshCommand) // and parse the ssh command to construct some mocked arguments
+		require.NoError(t, err)
+
+		t.Run("User2AccessUser5Repo4", func(t *testing.T) {
+			sshCmdParts = append(sshCmdParts,
+				"-p", strconv.Itoa(setting.SSH.ListenPort), "git@"+setting.SSH.ListenHost,
+				"git-lfs-authenticate", "user5/repo4.git", "upload", // user2's key upload lfs file to user5/repo4
+			)
+			cmd := exec.CommandContext(t.Context(), sshCmdParts[0], sshCmdParts[1:]...)
+			out, err := cmd.Output()
+			assert.ErrorContains(t, err, "exit status 1")
+			assert.Contains(t, string(out), 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))