Fix git commit committer parsing and add some tests (go-gitea#35007)

* Fix go-gitea#34991
* Fix go-gitea#34882

---------

Co-authored-by: wxiaoguang <[email protected]>

Author: lunny

Dockerfile

@@ -39,7 +39,6 @@ RUN chmod 755 /tmp/local/usr/bin/entrypoint \
               /tmp/local/etc/s6/.s6-svscan/* \
               /go/src/code.gitea.io/gitea/gitea \
               /go/src/code.gitea.io/gitea/environment-to-ini
-RUN chmod 644 /go/src/code.gitea.io/gitea/contrib/autocompletion/bash_autocomplete
 
 FROM docker.io/library/alpine:3.22
 LABEL maintainer="[email protected]"
@@ -83,4 +82,3 @@ CMD ["/usr/bin/s6-svscan", "/etc/s6"]
 COPY --from=build-env /tmp/local /
 COPY --from=build-env /go/src/code.gitea.io/gitea/gitea /app/gitea/gitea
 COPY --from=build-env /go/src/code.gitea.io/gitea/environment-to-ini /usr/local/bin/environment-to-ini
-COPY --from=build-env /go/src/code.gitea.io/gitea/contrib/autocompletion/bash_autocomplete /etc/profile.d/gitea_bash_autocomplete.sh

Dockerfile.rootless

@@ -37,7 +37,6 @@ RUN chmod 755 /tmp/local/usr/local/bin/docker-entrypoint.sh \
               /tmp/local/usr/local/bin/gitea \
               /go/src/code.gitea.io/gitea/gitea \
               /go/src/code.gitea.io/gitea/environment-to-ini
-RUN chmod 644 /go/src/code.gitea.io/gitea/contrib/autocompletion/bash_autocomplete
 
 FROM docker.io/library/alpine:3.22
 LABEL maintainer="[email protected]"
@@ -72,7 +71,6 @@ RUN chown git:git /var/lib/gitea /etc/gitea
 COPY --from=build-env /tmp/local /
 COPY --from=build-env --chown=root:root /go/src/code.gitea.io/gitea/gitea /app/gitea/gitea
 COPY --from=build-env --chown=root:root /go/src/code.gitea.io/gitea/environment-to-ini /usr/local/bin/environment-to-ini
-COPY --from=build-env /go/src/code.gitea.io/gitea/contrib/autocompletion/bash_autocomplete /etc/profile.d/gitea_bash_autocomplete.sh
 
 # git:git
 USER 1000:1000

models/asymkey/gpg_key_commit_verification.go

@@ -15,34 +15,15 @@ import (
 	"github.com/ProtonMail/go-crypto/openpgp/packet"
 )
 
-//   __________________  ________   ____  __.
-//  /  _____/\______   \/  _____/  |    |/ _|____ ___.__.
-// /   \  ___ |     ___/   \  ___  |      <_/ __ <   |  |
-// \    \_\  \|    |   \    \_\  \ |    |  \  ___/\___  |
-//  \______  /|____|    \______  / |____|__ \___  > ____|
-//         \/                  \/          \/   \/\/
-// _________                        .__  __
-// \_   ___ \  ____   _____   _____ |__|/  |_
-// /    \  \/ /  _ \ /     \ /     \|  \   __\
-// \     \___(  <_> )  Y Y  \  Y Y  \  ||  |
-//  \______  /\____/|__|_|  /__|_|  /__||__|
-//         \/             \/      \/
-// ____   ____           .__  _____.__               __  .__
-// \   \ /   /___________|__|/ ____\__| ____ _____ _/  |_|__| ____   ____
-//  \   Y   // __ \_  __ \  \   __\|  |/ ___\\__  \\   __\  |/  _ \ /    \
-//   \     /\  ___/|  | \/  ||  |  |  \  \___ / __ \|  | |  (  <_> )   |  \
-//    \___/  \___  >__|  |__||__|  |__|\___  >____  /__| |__|\____/|___|  /
-//               \/                        \/     \/                    \/
-
 // This file provides functions relating commit verification
 
 // CommitVerification represents a commit validation of signature
 type CommitVerification struct {
 	Verified       bool
 	Warning        bool
 	Reason         string
-	SigningUser    *user_model.User
-	CommittingUser *user_model.User
+	SigningUser    *user_model.User // if Verified, then SigningUser is non-nil
+	CommittingUser *user_model.User // if Verified, then CommittingUser is non-nil
 	SigningEmail   string
 	SigningKey     *GPGKey
 	SigningSSHKey  *PublicKey

models/user/user.go

@@ -1166,12 +1166,6 @@ func ValidateCommitsWithEmails(ctx context.Context, oldCommits []*git.Commit) ([
 
 	for _, c := range oldCommits {
 		user := emailUserMap.GetByEmail(c.Author.Email) // FIXME: why ValidateCommitsWithEmails uses "Author", but ParseCommitsWithSignature uses "Committer"?
-		if user == nil {
-			user = &User{
-				Name:  c.Author.Name,
-				Email: c.Author.Email,
-			}
-		}
 		newCommits = append(newCommits, &UserCommit{
 			User:   user,
 			Commit: c,
@@ -1195,12 +1189,14 @@ func GetUsersByEmails(ctx context.Context, emails []string) (*EmailUserMap, erro
 
 	needCheckEmails := make(container.Set[string])
 	needCheckUserNames := make(container.Set[string])
+	noReplyAddressSuffix := "@" + strings.ToLower(setting.Service.NoReplyAddress)
 	for _, email := range emails {
-		if strings.HasSuffix(email, "@"+setting.Service.NoReplyAddress) {
-			username := strings.TrimSuffix(email, "@"+setting.Service.NoReplyAddress)
-			needCheckUserNames.Add(strings.ToLower(username))
+		emailLower := strings.ToLower(email)
+		if noReplyUserNameLower, ok := strings.CutSuffix(emailLower, noReplyAddressSuffix); ok {
+			needCheckUserNames.Add(noReplyUserNameLower)
+			needCheckEmails.Add(emailLower)
 		} else {
-			needCheckEmails.Add(strings.ToLower(email))
+			needCheckEmails.Add(emailLower)
 		}
 	}
 

models/user/user_test.go

@@ -85,6 +85,11 @@ func TestUserEmails(t *testing.T) {
 				testGetUserByEmail(t, c.Email, c.UID)
 			})
 		}
+
+		t.Run("NoReplyConflict", func(t *testing.T) {
+			setting.Service.NoReplyAddress = "example.com"
+			testGetUserByEmail(t, "[email protected]", 1)
+		})
 	})
 }
 

modules/git/commit.go

@@ -22,9 +22,9 @@ import (
 type Commit struct {
 	Tree // FIXME: bad design, this field can be nil if the commit is from "last commit cache"
 
-	ID            ObjectID // The ID of this commit object
-	Author        *Signature
-	Committer     *Signature
+	ID            ObjectID
+	Author        *Signature // never nil
+	Committer     *Signature // never nil
 	CommitMessage string
 	Signature     *CommitSignature
 

services/asymkey/commit.go

@@ -24,47 +24,43 @@ import (
 
 // ParseCommitWithSignature check if signature is good against keystore.
 func ParseCommitWithSignature(ctx context.Context, c *git.Commit) *asymkey_model.CommitVerification {
-	var committer *user_model.User
-	if c.Committer != nil {
-		var err error
-		// Find Committer account
-		committer, err = user_model.GetUserByEmail(ctx, c.Committer.Email) // This finds the user by primary email or activated email so commit will not be valid if email is not
-		if err != nil {                                                    // Skipping not user for committer
-			committer = &user_model.User{
-				Name:  c.Committer.Name,
-				Email: c.Committer.Email,
-			}
-			// We can expect this to often be an ErrUserNotExist. in the case
-			// it is not, however, it is important to log it.
-			if !user_model.IsErrUserNotExist(err) {
-				log.Error("GetUserByEmail: %v", err)
-				return &asymkey_model.CommitVerification{
-					CommittingUser: committer,
-					Verified:       false,
-					Reason:         "gpg.error.no_committer_account",
-				}
-			}
+	committer, err := user_model.GetUserByEmail(ctx, c.Committer.Email)
+	if err != nil && !user_model.IsErrUserNotExist(err) {
+		log.Error("GetUserByEmail: %v", err)
+		return &asymkey_model.CommitVerification{
+			Verified: false,
+			Reason:   "gpg.error.no_committer_account", // this error is not right, but such error should seldom happen
 		}
 	}
-
 	return ParseCommitWithSignatureCommitter(ctx, c, committer)
 }
 
+// ParseCommitWithSignatureCommitter parses a commit's GPG or SSH signature.
+// If the commit is singed by an instance key, then committer can be nil.
+// If the signature exists, even if committer is nil, the returned CommittingUser will be a non-nil fake user.
 func ParseCommitWithSignatureCommitter(ctx context.Context, c *git.Commit, committer *user_model.User) *asymkey_model.CommitVerification {
-	// If no signature just report the committer
+	// If no signature, just report the committer
 	if c.Signature == nil {
 		return &asymkey_model.CommitVerification{
 			CommittingUser: committer,
-			Verified:       false,                         // Default value
-			Reason:         "gpg.error.not_signed_commit", // Default value
+			Verified:       false,
+			Reason:         "gpg.error.not_signed_commit",
+		}
+	}
+	// to support instance key, we need a fake committer user (not really needed, but legacy code accesses the committer without nil-check)
+	if committer == nil {
+		committer = &user_model.User{
+			Name:  c.Committer.Name,
+			Email: c.Committer.Email,
 		}
 	}
-
-	// If this a SSH signature handle it differently
 	if strings.HasPrefix(c.Signature.Signature, "-----BEGIN SSH SIGNATURE-----") {
-		return ParseCommitWithSSHSignature(ctx, c, committer)
+		return parseCommitWithSSHSignature(ctx, c, committer)
 	}
+	return parseCommitWithGPGSignature(ctx, c, committer)
+}
 
+func parseCommitWithGPGSignature(ctx context.Context, c *git.Commit, committer *user_model.User) *asymkey_model.CommitVerification {
 	// Parsing signature
 	sig, err := asymkey_model.ExtractSignature(c.Signature.Signature)
 	if err != nil { // Skipping failed to extract sign
@@ -165,7 +161,7 @@ func ParseCommitWithSignatureCommitter(ctx context.Context, c *git.Commit, commi
 		}
 		if err := gpgSettings.LoadPublicKeyContent(); err != nil {
 			log.Error("Error getting default signing key: %s %v", gpgSettings.KeyID, err)
-		} else if commitVerification := VerifyWithGPGSettings(ctx, &gpgSettings, sig, c.Signature.Payload, committer, keyID); commitVerification != nil {
+		} else if commitVerification := verifyWithGPGSettings(ctx, &gpgSettings, sig, c.Signature.Payload, committer, keyID); commitVerification != nil {
 			if commitVerification.Reason == asymkey_model.BadSignature {
 				defaultReason = asymkey_model.BadSignature
 			} else {
@@ -180,7 +176,7 @@ func ParseCommitWithSignatureCommitter(ctx context.Context, c *git.Commit, commi
 	} else if defaultGPGSettings == nil {
 		log.Warn("Unable to get defaultGPGSettings for unattached commit: %s", c.ID.String())
 	} else if defaultGPGSettings.Sign {
-		if commitVerification := VerifyWithGPGSettings(ctx, defaultGPGSettings, sig, c.Signature.Payload, committer, keyID); commitVerification != nil {
+		if commitVerification := verifyWithGPGSettings(ctx, defaultGPGSettings, sig, c.Signature.Payload, committer, keyID); commitVerification != nil {
 			if commitVerification.Reason == asymkey_model.BadSignature {
 				defaultReason = asymkey_model.BadSignature
 			} else {
@@ -295,7 +291,7 @@ func HashAndVerifyForKeyID(ctx context.Context, sig *packet.Signature, payload s
 	}
 }
 
-func VerifyWithGPGSettings(ctx context.Context, gpgSettings *git.GPGSettings, sig *packet.Signature, payload string, committer *user_model.User, keyID string) *asymkey_model.CommitVerification {
+func verifyWithGPGSettings(ctx context.Context, gpgSettings *git.GPGSettings, sig *packet.Signature, payload string, committer *user_model.User, keyID string) *asymkey_model.CommitVerification {
 	// First try to find the key in the db
 	if commitVerification := HashAndVerifyForKeyID(ctx, sig, payload, committer, gpgSettings.KeyID, gpgSettings.Name, gpgSettings.Email); commitVerification != nil {
 		return commitVerification
@@ -375,8 +371,8 @@ func verifySSHCommitVerificationByInstanceKey(c *git.Commit, committerUser, sign
 	return verifySSHCommitVerification(c.Signature.Signature, c.Signature.Payload, sshPubKey, committerUser, signerUser, committerGitEmail)
 }
 
-// ParseCommitWithSSHSignature check if signature is good against keystore.
-func ParseCommitWithSSHSignature(ctx context.Context, c *git.Commit, committerUser *user_model.User) *asymkey_model.CommitVerification {
+// parseCommitWithSSHSignature check if signature is good against keystore.
+func parseCommitWithSSHSignature(ctx context.Context, c *git.Commit, committerUser *user_model.User) *asymkey_model.CommitVerification {
 	// Now try to associate the signature with the committer, if present
 	if committerUser.ID != 0 {
 		keys, err := db.Find[asymkey_model.PublicKey](ctx, asymkey_model.FindPublicKeyOptions{

services/asymkey/commit_test.go

@@ -41,7 +41,7 @@ Initial commit with signed file
 			Name:  "User Two",
 			Email: "[email protected]",
 		}
-		ret := ParseCommitWithSSHSignature(t.Context(), commit, committingUser)
+		ret := parseCommitWithSSHSignature(t.Context(), commit, committingUser)
 		require.NotNil(t, ret)
 		assert.True(t, ret.Verified)
 		assert.False(t, ret.Warning)

services/git/commit.go

@@ -35,13 +35,6 @@ func ParseCommitsWithSignature(ctx context.Context, repo *repo_model.Repository,
 
 	for _, c := range oldCommits {
 		committerUser := emailUsers.GetByEmail(c.Committer.Email) // FIXME: why ValidateCommitsWithEmails uses "Author", but ParseCommitsWithSignature uses "Committer"?
-		if committerUser == nil {
-			committerUser = &user_model.User{
-				Name:  c.Committer.Name,
-				Email: c.Committer.Email,
-			}
-		}
-
 		signCommit := &asymkey_model.SignCommit{
 			UserCommit:   c,
 			Verification: asymkey_service.ParseCommitWithSignatureCommitter(ctx, c.Commit, committerUser),

templates/repo/commit_page.tmpl

@@ -147,7 +147,7 @@
 			<div class="flex-text-inline">
 				{{if or (ne .Commit.Committer.Name .Commit.Author.Name) (ne .Commit.Committer.Email .Commit.Author.Email)}}
 					<span class="text grey">{{ctx.Locale.Tr "repo.diff.committed_by"}}</span>
-					{{if ne .Verification.CommittingUser.ID 0}}
+					{{if and .Verification.CommittingUser .Verification.CommittingUser.ID}}
 						{{ctx.AvatarUtils.Avatar .Verification.CommittingUser 20}}
 						<a href="{{.Verification.CommittingUser.HomeLink}}"><strong>{{.Commit.Committer.Name}}</strong></a>
 					{{else}}