fix(sec): only degrade permission check for git push (#7752)

Reviewed-on: https://codeberg.org/forgejo/forgejo/pulls/7752
Reviewed-by: Gusted <gusted@noreply.codeberg.org>

routers/private/serv.go

@@ -296,8 +296,14 @@ func ServCommand(ctx *context.PrivateContext) {
 				return
 			}
 		} else {
-			// Because of the special ref "refs/for" we will need to delay write permission check
+			// We don't know yet which references the Git client wants to write to,
-			if git.SupportProcReceive && unitType == unit.TypeCode {
+			// but for AGit flow we have to degrade this check to a read permission.
+			// So if we support proc-receive (needed for AGit flow) and the unit type
+			// is code and we know the Git client wants to write to us, then degrade
+			// the permission check to read. The pre-receive hook will do another
+			// permission check which ensure for non AGit flow references the write
+			// permission is checked.
+			if git.SupportProcReceive && unitType == unit.TypeCode && ctx.FormString("verb") == "git-receive-pack" {
 				mode = perm.AccessModeRead
 			}
 

tests/integration/git_test.go

@@ -12,9 +12,11 @@ import (
 	"net/http"
 	"net/url"
 	"os"
+	"os/exec"
 	"path"
 	"path/filepath"
 	"strconv"
+	"strings"
 	"testing"
 	"time"
 
@@ -35,6 +37,7 @@ import (
 	files_service "forgejo.org/services/repository/files"
 	"forgejo.org/tests"
 
+	"github.com/kballard/go-shellquote"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
@@ -113,7 +116,10 @@ func testGit(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-"+objectFormat.Name(), keyFile))
+				var publicKeyID int64
+				t.Run("CreateUserKey", doAPICreateUserKey(sshContext, "test-key-"+objectFormat.Name(), keyFile, func(t *testing.T, pk api.PublicKey) {
+					publicKeyID = pk.ID
+				}))
 
 				// Setup remote link
 				// TODO: get url from api
@@ -140,6 +146,7 @@ func testGit(t *testing.T, u *url.URL) {
 				})
 
 				t.Run("PushCreate", doPushCreate(sshContext, sshURL, objectFormat))
+				t.Run("LFS no access", doLFSNoAccess(sshContext, publicKeyID, objectFormat))
 			})
 		})
 	})
@@ -1122,3 +1129,30 @@ func TestDataAsync_Issue29101(t *testing.T) {
 		defer r2.Close()
 	})
 }
+
+func doLFSNoAccess(ctx APITestContext, publicKeyID int64, objectFormat git.ObjectFormat) func(*testing.T) {
+	return func(t *testing.T) {
+		// This is set in withKeyFile
+		sshCommand := os.Getenv("GIT_SSH_COMMAND")
+
+		// Sanity check, because we are going to execute whatever is in here.
+		require.True(t, strings.HasPrefix(sshCommand, "ssh "))
+
+		// We really have to split on the arguments and pass them individually.
+		sshOptions, err := shellquote.Split(strings.TrimPrefix(sshCommand, "ssh "))
+		require.NoError(t, err)
+
+		sshOptions = append(sshOptions, "-p "+strconv.Itoa(setting.SSH.ListenPort), "git@"+setting.SSH.ListenHost)
+
+		cmd := exec.CommandContext(t.Context(), "ssh", append(sshOptions, "git-lfs-authenticate", "user40/repo60.git", "upload")...)
+		stderr := bytes.Buffer{}
+		cmd.Stderr = &stderr
+
+		require.ErrorContains(t, cmd.Run(), "exit status 1")
+		if objectFormat.Name() == "sha1" {
+			assert.Contains(t, stderr.String(), fmt.Sprintf("Forgejo: User: 2:user2 with Key: %d:test-key-sha1 is not authorized to write to user40/repo60.", publicKeyID))
+		} else {
+			assert.Contains(t, stderr.String(), fmt.Sprintf("Forgejo: User: 2:user2 with Key: %d:test-key-sha256 is not authorized to write to user40/repo60.", publicKeyID))
+		}
+	}
+}