fix(migrations): preserve SHA-256 pull request commit IDs (#39343)

* Fixes #39339

---------

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

modules/git/blame.go

@@ -72,12 +72,14 @@ func (r *BlameReader) NextPart() (*BlamePart, error) {
 		}
 
 		var objectID string
-		objectFormatLength := r.objectFormat.FullLength()
+		if lineField1, _, ok := bytes.Cut(lineBytes, []byte(" ")); ok {
-
+			lineFieldStr := string(lineField1)
-		if len(lineBytes) > objectFormatLength && lineBytes[objectFormatLength] == ' ' && r.objectFormat.IsValid(string(lineBytes[0:objectFormatLength])) {
+			if IsStringValidObjectID(r.objectFormat, lineFieldStr) {
-			objectID = string(lineBytes[0:objectFormatLength])
+				objectID = lineFieldStr
+			}
 		}
-		if len(objectID) > 0 {
+
+		if objectID != "" {
 			if blamePart == nil {
 				blamePart = &BlamePart{
 					Sha:   objectID,
@@ -99,6 +101,7 @@ func (r *BlameReader) NextPart() (*BlamePart, error) {
 		} else if lineBytes[0] == '\t' {
 			blamePart.Lines = append(blamePart.Lines, string(lineBytes[1:]))
 		} else if bytes.HasPrefix(lineBytes, []byte(previousHeader)) {
+			objectFormatLength := r.objectFormat.FullLength()
 			offset := len(previousHeader) // already includes a space
 			blamePart.PreviousSha = string(lineBytes[offset : offset+objectFormatLength])
 			offset += objectFormatLength + 1 // +1 for space

modules/git/blame_sha256_test.go

@@ -4,7 +4,6 @@
 package git
 
 import (
-	"context"
 	"testing"
 
 	"gitea.dev/modules/setting"
@@ -14,8 +13,7 @@ import (
 
 func TestReadingBlameOutputSha256(t *testing.T) {
 	setting.AppDataPath = t.TempDir()
-	ctx, cancel := context.WithCancel(t.Context())
+	ctx := t.Context()
-	defer cancel()
 
 	if DefaultFeatures().UsingGogit {
 		t.Skip("Skipping test since gogit does not support sha256")

modules/git/blame_test.go

@@ -4,7 +4,6 @@
 package git
 
 import (
-	"context"
 	"testing"
 
 	"gitea.dev/modules/setting"
@@ -14,8 +13,7 @@ import (
 
 func TestReadingBlameOutput(t *testing.T) {
 	setting.AppDataPath = t.TempDir()
-	ctx, cancel := context.WithCancel(t.Context())
+	ctx := t.Context()
-	defer cancel()
 
 	t.Run("Without .git-blame-ignore-revs", func(t *testing.T) {
 		storage := mockRepository("repo5_pulls")

modules/git/commit.go

@@ -248,25 +248,3 @@ func GetFullCommitID(ctx context.Context, repo RepositoryFacade, shortID string)
 	}
 	return strings.TrimSpace(commitID), nil
 }
-
-func IsStringLikelyCommitID(objFmt ObjectFormat, s string, minLength ...int) bool {
-	maxLen := 64 // sha256
-	if objFmt != nil {
-		maxLen = objFmt.FullLength()
-	}
-	minLen := util.OptionalArg(minLength, maxLen)
-	if len(s) < minLen || len(s) > maxLen {
-		return false
-	}
-	return isStringLowerHex(s)
-}
-
-func isStringLowerHex(s string) bool {
-	for _, c := range s {
-		isHex := (c >= '0' && c <= '9') || (c >= 'a' && c <= 'f')
-		if !isHex {
-			return false
-		}
-	}
-	return len(s) > 0 // it accepts odd length because "shorten commit id" can be 7-chars
-}

modules/git/commit_test.go

@@ -195,10 +195,3 @@ func Test_GetCommitBranchStart(t *testing.T) {
 	assert.NotEmpty(t, startCommitID)
 	assert.Equal(t, "95bb4d39648ee7e325106df01a621c530863a653", startCommitID)
 }
-
-func TestIsStringLikelyCommitID(t *testing.T) {
-	assert.True(t, IsStringLikelyCommitID(nil, "abc", 3))
-	assert.False(t, IsStringLikelyCommitID(nil, "abc", 4))
-	assert.True(t, IsStringLikelyCommitID(nil, strings.Repeat("a", 64), 4))
-	assert.False(t, IsStringLikelyCommitID(nil, strings.Repeat("a", 65), 4))
-}

modules/git/object_format.go

@@ -6,16 +6,9 @@ package git
 import (
 	"crypto/sha1"
 	"crypto/sha256"
-	"regexp"
 	"strconv"
 )
 
-// sha1Pattern can be used to determine if a string is a valid sha
-var sha1Pattern = regexp.MustCompile(`^[0-9a-f]{4,40}$`)
-
-// sha256Pattern can be used to determine if a string is a valid sha
-var sha256Pattern = regexp.MustCompile(`^[0-9a-f]{4,64}$`)
-
 type ObjectFormat interface {
 	// Name returns the name of the object format
 	Name() string
@@ -25,8 +18,6 @@ type ObjectFormat interface {
 	EmptyTree() ObjectID
 	// FullLength is the length of the hash's hex string
 	FullLength() int
-	// IsValid returns true if the input is a valid hash
-	IsValid(input string) bool
 	// MustID creates a new ObjectID from a byte slice
 	MustID(b []byte) ObjectID
 	// ComputeHash compute the hash for a given ObjectType and content
@@ -41,19 +32,10 @@ var (
 	emptySha1Tree = &Sha1Hash{0x4b, 0x82, 0x5d, 0xc6, 0x42, 0xcb, 0x6e, 0xb9, 0xa0, 0x60, 0xe5, 0x4b, 0xf8, 0xd6, 0x92, 0x88, 0xfb, 0xee, 0x49, 0x04}
 )
 
-func (Sha1ObjectFormatImpl) Name() string { return "sha1" }
+func (Sha1ObjectFormatImpl) Name() string            { return "sha1" }
-func (Sha1ObjectFormatImpl) EmptyObjectID() ObjectID {
+func (Sha1ObjectFormatImpl) EmptyObjectID() ObjectID { return emptySha1ObjectID }
-	return emptySha1ObjectID
+func (Sha1ObjectFormatImpl) EmptyTree() ObjectID     { return emptySha1Tree }
-}
+func (Sha1ObjectFormatImpl) FullLength() int         { return 40 }
-
-func (Sha1ObjectFormatImpl) EmptyTree() ObjectID {
-	return emptySha1Tree
-}
-func (Sha1ObjectFormatImpl) FullLength() int { return 40 }
-func (Sha1ObjectFormatImpl) IsValid(input string) bool {
-	return sha1Pattern.MatchString(input)
-}
-
 func (Sha1ObjectFormatImpl) MustID(b []byte) ObjectID {
 	var id Sha1Hash
 	copy(id[0:20], b)
@@ -83,19 +65,10 @@ var (
 	}
 )
 
-func (Sha256ObjectFormatImpl) Name() string { return "sha256" }
+func (Sha256ObjectFormatImpl) Name() string            { return "sha256" }
-func (Sha256ObjectFormatImpl) EmptyObjectID() ObjectID {
+func (Sha256ObjectFormatImpl) EmptyObjectID() ObjectID { return emptySha256ObjectID }
-	return emptySha256ObjectID
+func (Sha256ObjectFormatImpl) EmptyTree() ObjectID     { return emptySha256Tree }
-}
+func (Sha256ObjectFormatImpl) FullLength() int         { return 64 }
-
-func (Sha256ObjectFormatImpl) EmptyTree() ObjectID {
-	return emptySha256Tree
-}
-func (Sha256ObjectFormatImpl) FullLength() int { return 64 }
-func (Sha256ObjectFormatImpl) IsValid(input string) bool {
-	return sha256Pattern.MatchString(input)
-}
-
 func (Sha256ObjectFormatImpl) MustID(b []byte) ObjectID {
 	var id Sha256Hash
 	copy(id[0:32], b)
@@ -115,34 +88,13 @@ func (h Sha256ObjectFormatImpl) ComputeHash(t ObjectType, content []byte) Object
 
 type invalidObjectFormatImpl struct{}
 
-var emptyInvalidObjectID = &Sha1Hash{}
+func (h invalidObjectFormatImpl) Name() string             { return "invalid-object-format" }
-
+func (h invalidObjectFormatImpl) EmptyObjectID() ObjectID  { return &Sha1Hash{} }
-func (h invalidObjectFormatImpl) Name() string {
+func (h invalidObjectFormatImpl) EmptyTree() ObjectID      { return h.EmptyObjectID() }
-	return "invalid-object-format"
+func (h invalidObjectFormatImpl) FullLength() int          { return len(h.EmptyObjectID().RawValue()) * 2 }
-}
+func (h invalidObjectFormatImpl) MustID(b []byte) ObjectID { return h.EmptyObjectID() }
-
-func (h invalidObjectFormatImpl) EmptyObjectID() ObjectID {
-	return emptyInvalidObjectID
-}
-
-func (h invalidObjectFormatImpl) EmptyTree() ObjectID {
-	return emptyInvalidObjectID
-}
-
-func (h invalidObjectFormatImpl) FullLength() int {
-	return len(emptyInvalidObjectID) * 2
-}
-
-func (h invalidObjectFormatImpl) IsValid(input string) bool {
-	return false
-}
-
-func (h invalidObjectFormatImpl) MustID(b []byte) ObjectID {
-	return emptyInvalidObjectID
-}
-
 func (h invalidObjectFormatImpl) ComputeHash(t ObjectType, content []byte) ObjectID {
-	return emptyInvalidObjectID
+	return h.EmptyObjectID()
 }
 
 var (

modules/git/object_id.go

@@ -7,6 +7,8 @@ import (
 	"bytes"
 	"encoding/hex"
 	"fmt"
+
+	"gitea.dev/modules/util"
 )
 
 type ObjectID interface {
@@ -104,3 +106,37 @@ func IsEmptyCommitID(commitID string) bool {
 func ComputeBlobHash(hashType ObjectFormat, content []byte) ObjectID {
 	return hashType.ComputeHash(ObjectBlob, content)
 }
+
+func IsStringValidObjectID(objFmt ObjectFormat, s string, optMinLen ...int) bool {
+	if objFmt == invalidObjectFormat {
+		return false
+	}
+	var minLen, maxLen int
+	if objFmt != nil {
+		maxLen = objFmt.FullLength()
+		minLen = util.OptionalArg(optMinLen, maxLen)
+	} else {
+		if len(optMinLen) == 0 {
+			// if no "min length" is applied, then the length must exactly match one of the formats
+			if len(s) != Sha1ObjectFormat.FullLength() && len(s) != Sha256ObjectFormat.FullLength() {
+				return false
+			}
+		}
+		maxLen = Sha256ObjectFormat.FullLength()
+		minLen = util.OptionalArg(optMinLen, Sha1ObjectFormat.FullLength())
+	}
+	if len(s) < minLen || len(s) > maxLen {
+		return false
+	}
+	return isStringLowerHex(s)
+}
+
+func isStringLowerHex(s string) bool {
+	for _, c := range s {
+		isHex := (c >= '0' && c <= '9') || (c >= 'a' && c <= 'f')
+		if !isHex {
+			return false
+		}
+	}
+	return len(s) > 0 // it accepts odd length because "shorten commit id" can be 7-chars
+}

modules/git/object_id_test.go

@@ -4,19 +4,13 @@
 package git
 
 import (
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 )
 
 func TestIsValidSHAPattern(t *testing.T) {
-	assert.True(t, Sha1ObjectFormat.IsValid("fee1"))
-	assert.True(t, Sha1ObjectFormat.IsValid("abc000"))
-	assert.True(t, Sha1ObjectFormat.IsValid("9023902390239023902390239023902390239023"))
-	assert.False(t, Sha1ObjectFormat.IsValid("90239023902390239023902390239023902390239023"))
-	assert.False(t, Sha1ObjectFormat.IsValid("abc"))
-	assert.False(t, Sha1ObjectFormat.IsValid("123g"))
-	assert.False(t, Sha1ObjectFormat.IsValid("some random text"))
 	assert.Equal(t, "e69de29bb2d1d6434b8b29ae775ad8c2e48c5391", ComputeBlobHash(Sha1ObjectFormat, nil).String())
 	assert.Equal(t, "2e65efe2a145dda7ee51d1741299f848e5bf752e", ComputeBlobHash(Sha1ObjectFormat, []byte("a")).String())
 	assert.Equal(t, "473a0f4c3be8a93681a267e3b1e9a7dcda1185436fe141f7749120a303721813", ComputeBlobHash(Sha256ObjectFormat, nil).String())
@@ -30,3 +24,30 @@ func TestInvalidObjectFormat(t *testing.T) {
 	assert.NotNil(t, of)
 	assert.False(t, IsValidObjectFormat("no-such"))
 }
+
+func TestIsStringValidObjectID(t *testing.T) {
+	assert.True(t, IsStringValidObjectID(nil, "abc", 3))
+	assert.False(t, IsStringValidObjectID(nil, "abg", 3))
+
+	assert.False(t, IsStringValidObjectID(nil, "abc", 4))
+	assert.True(t, IsStringValidObjectID(nil, "0123456789abcdef", 4))
+
+	assert.False(t, IsStringValidObjectID(nil, strings.Repeat("a", 3), 4))
+	assert.True(t, IsStringValidObjectID(nil, strings.Repeat("a", 4), 4))
+	assert.True(t, IsStringValidObjectID(nil, strings.Repeat("a", 5), 4))
+	assert.True(t, IsStringValidObjectID(nil, strings.Repeat("a", 64), 4))
+	assert.False(t, IsStringValidObjectID(nil, strings.Repeat("a", 65), 4))
+
+	assert.False(t, IsStringValidObjectID(nil, strings.Repeat("a", 39)))
+	assert.True(t, IsStringValidObjectID(nil, strings.Repeat("a", 40)))
+	assert.False(t, IsStringValidObjectID(nil, strings.Repeat("a", 41)))
+	assert.True(t, IsStringValidObjectID(nil, strings.Repeat("a", 64)))
+	assert.False(t, IsStringValidObjectID(nil, strings.Repeat("a", 65)))
+
+	assert.False(t, IsStringValidObjectID(Sha1ObjectFormat, strings.Repeat("a", 39)))
+	assert.True(t, IsStringValidObjectID(Sha1ObjectFormat, strings.Repeat("a", 40)))
+	assert.False(t, IsStringValidObjectID(Sha1ObjectFormat, strings.Repeat("a", 41)))
+
+	assert.False(t, IsStringValidObjectID(invalidObjectFormat, strings.Repeat("a", 40)))
+	assert.False(t, IsStringValidObjectID(invalidObjectFormat, strings.Repeat("a", 40), 4))
+}

modules/git/ref.go

@@ -224,7 +224,7 @@ func (ref RefName) RefType() RefType {
 		return RefTypeBranch
 	case ref.IsTag():
 		return RefTypeTag
-	case IsStringLikelyCommitID(nil, string(ref), 6):
+	case IsStringValidObjectID(nil, string(ref), 6):
 		return RefTypeCommit
 	}
 	return ""

modules/git/repo_commit.go

@@ -203,8 +203,8 @@ func (repo *Repository) searchCommits(ctx context.Context, id ObjectID, opts Sea
 	// if there are any keywords (ie not committer:, author:, time:)
 	// then let's iterate over them
 	for _, v := range opts.Keywords {
-		// ignore anything not matching a valid sha pattern
+		// ignore anything not matching a valid sha pattern (TODO: the legacy min-length is pretty small, is it right or fine?)
-		if id.Type().IsValid(v) {
+		if IsStringValidObjectID(id.Type(), v, 4) {
 			// create new git log command with 1 commit limit
 			hashCmd := gitcmd.NewCommand("log", "-1", prettyLogFormat)
 			// add previous arguments except for --grep and --all

modules/git/repo_commit_gogit.go

@@ -13,7 +13,6 @@ import (
 	"gitea.dev/modules/git/gitcmd"
 
 	"github.com/go-git/go-git/v5/plumbing"
-	"github.com/go-git/go-git/v5/plumbing/hash"
 	"github.com/go-git/go-git/v5/plumbing/object"
 )
 
@@ -48,7 +47,7 @@ func (repo *Repository) ConvertToGitID(ctx context.Context, commitID string) (Ob
 	if err != nil {
 		return nil, err
 	}
-	if len(commitID) == hash.HexSize && objectFormat.IsValid(commitID) {
+	if IsStringValidObjectID(objectFormat, commitID) {
 		ID, err := NewIDFromString(commitID)
 		if err == nil {
 			return ID, nil

modules/git/repo_commit_nogogit.go

@@ -116,7 +116,7 @@ func (repo *Repository) ConvertToGitID(ctx context.Context, ref string) (ObjectI
 	if err != nil {
 		return nil, err
 	}
-	if len(ref) == objectFormat.FullLength() && objectFormat.IsValid(ref) {
+	if IsStringValidObjectID(objectFormat, ref) {
 		id, err := NewIDFromString(ref)
 		if err == nil {
 			return id, nil

routers/api/v1/repo/pull.go

@@ -1157,8 +1157,8 @@ func parseCompareInfo(ctx *context.APIContext, compareParam string) (result *git
 
 	log.Trace("Repo: %q, base ref: %q->%q, head ref: %q->%q", ctx.Repo.Repository.FullName(), compareReq.BaseOriRef+compareReq.BaseOriRefSuffix, baseRef, compareReq.HeadOriRef+compareReq.HeadOriRefSuffix, headRef)
 
-	baseRefValid := baseRef.IsBranch() || baseRef.IsTag() || git.IsStringLikelyCommitID(git.ObjectFormatFromName(ctx.Repo.Repository.ObjectFormatName), baseRef.ShortName())
+	baseRefValid := baseRef.IsBranch() || baseRef.IsTag() || git.IsStringValidObjectID(git.ObjectFormatFromName(ctx.Repo.Repository.ObjectFormatName), baseRef.ShortName())
-	headRefValid := headRef.IsBranch() || headRef.IsTag() || git.IsStringLikelyCommitID(git.ObjectFormatFromName(headRepo.ObjectFormatName), headRef.ShortName())
+	headRefValid := headRef.IsBranch() || headRef.IsTag() || git.IsStringValidObjectID(git.ObjectFormatFromName(headRepo.ObjectFormatName), headRef.ShortName())
 	// Check if base&head ref are valid.
 	if !baseRefValid || !headRefValid {
 		ctx.APIErrorNotFound()

routers/api/v1/utils/git.go

@@ -52,7 +52,7 @@ func ResolveRefCommit(ctx reqctx.RequestContext, repo *repo_model.Repository, in
 		}
 	}
 	if refCommit.RefName == "" {
-		if git.IsStringLikelyCommitID(git.ObjectFormatFromName(repo.ObjectFormatName), inputRef, minCommitIDLen...) {
+		if git.IsStringValidObjectID(git.ObjectFormatFromName(repo.ObjectFormatName), inputRef, minCommitIDLen...) {
 			refCommit.RefName = git.RefNameFromCommit(inputRef)
 		}
 	}

services/context/repo.go

@@ -849,7 +849,7 @@ func getRefNameLegacy(ctx *Base, repo *Repository, reqPath, extraRef string) (re
 	if refName := getRefName(ctx, repo, reqRefPath, git.RefTypeTag); refName != "" {
 		return refName, git.RefTypeTag, false
 	}
-	if git.IsStringLikelyCommitID(git.ObjectFormatFromName(repo.Repository.ObjectFormatName), reqRefPathParts[0]) {
+	if git.IsStringValidObjectID(git.ObjectFormatFromName(repo.Repository.ObjectFormatName), reqRefPathParts[0]) {
 		// FIXME: this logic is different from other types. Ideally, it should also try to GetCommit to check if it exists
 		repo.TreePath = strings.Join(reqRefPathParts[1:], "/")
 		return reqRefPathParts[0], git.RefTypeCommit, false
@@ -900,7 +900,7 @@ func getRefName(ctx *Base, repo *Repository, path string, refType git.RefType) s
 		})
 	case git.RefTypeCommit:
 		parts := strings.Split(path, "/")
-		if git.IsStringLikelyCommitID(repo.GetObjectFormat(), parts[0], 7) {
+		if git.IsStringValidObjectID(repo.GetObjectFormat(), parts[0], 7) {
 			// FIXME: this logic is different from other types. Ideally, it should also try to GetCommit to check if it exists
 			repo.TreePath = strings.Join(parts[1:], "/")
 			return parts[0]
@@ -1032,7 +1032,7 @@ func RepoRefByType(detectRefType git.RefType) func(*Context) {
 					return
 				}
 				ctx.Repo.CommitID = ctx.Repo.Commit.ID.String()
-			} else if git.IsStringLikelyCommitID(ctx.Repo.GetObjectFormat(), refShortName, 7) {
+			} else if git.IsStringValidObjectID(ctx.Repo.GetObjectFormat(), refShortName, 7) {
 				ctx.Repo.RefFullName = git.RefNameFromCommit(refShortName)
 				ctx.Repo.CommitID = refShortName
 

services/migrations/common.go

@@ -50,19 +50,18 @@ func CheckAndEnsureSafePR(pr *base.PullRequest, commonCloneBaseURL string, g bas
 		valid = false
 	}
 
-	// SECURITY: SHAs Must be a SHA
+	// SECURITY: SHAs must be valid Git object IDs.
-	// FIXME: hash only a SHA1
+	// The repository object format is not yet available at this stage.
-	CommitType := git.Sha1ObjectFormat
+	if pr.MergeCommitSHA != "" && !git.IsStringValidObjectID(nil, pr.MergeCommitSHA) {
-	if pr.MergeCommitSHA != "" && !CommitType.IsValid(pr.MergeCommitSHA) {
 		WarnAndNotice("PR #%d in %s has invalid MergeCommitSHA: %s", pr.Number, g, pr.MergeCommitSHA)
 		pr.MergeCommitSHA = ""
 	}
-	if pr.Head.SHA != "" && !CommitType.IsValid(pr.Head.SHA) {
+	if pr.Head.SHA != "" && !git.IsStringValidObjectID(nil, pr.Head.SHA) {
 		WarnAndNotice("PR #%d in %s has invalid HeadSHA: %s", pr.Number, g, pr.Head.SHA)
 		pr.Head.SHA = ""
 		valid = false
 	}
-	if pr.Base.SHA != "" && !CommitType.IsValid(pr.Base.SHA) {
+	if pr.Base.SHA != "" && !git.IsStringValidObjectID(nil, pr.Base.SHA) {
 		WarnAndNotice("PR #%d in %s has invalid BaseSHA: %s", pr.Number, g, pr.Base.SHA)
 		pr.Base.SHA = ""
 		valid = false

services/migrations/gitea_uploader.go

@@ -918,7 +918,7 @@ func (g *GiteaLocalUploader) CreateReviews(ctx context.Context, reviews ...*base
 			}
 
 			objectFormat := git.ObjectFormatFromName(g.repo.ObjectFormatName)
-			if !objectFormat.IsValid(comment.CommitID) {
+			if !git.IsStringValidObjectID(objectFormat, comment.CommitID) {
 				log.Warn("Invalid comment CommitID[%s] on comment[%d] in PR #%d of %s/%s replaced with %s", comment.CommitID, pr.Index, g.repoOwner, g.repoName, headCommitID)
 				comment.CommitID = headCommitID
 			}