Add more checks in migration code (#21011) (#21050)

Backport #21011

When migrating add several more important sanity checks:

* SHAs must be SHAs
* Refs must be valid Refs
* URLs must be reasonable

Signed-off-by: Andrew Thornton <[email protected]>

Author: zeripath

models/action.go

@@ -275,7 +275,7 @@ func (a *Action) GetRefLink() string {
 		return a.GetRepoLink() + "/src/branch/" + util.PathEscapeSegments(strings.TrimPrefix(a.RefName, git.BranchPrefix))
 	case strings.HasPrefix(a.RefName, git.TagPrefix):
 		return a.GetRepoLink() + "/src/tag/" + util.PathEscapeSegments(strings.TrimPrefix(a.RefName, git.TagPrefix))
-	case len(a.RefName) == 40 && git.SHAPattern.MatchString(a.RefName):
+	case len(a.RefName) == 40 && git.IsValidSHAPattern(a.RefName):
 		return a.GetRepoLink() + "/src/commit/" + a.RefName
 	default:
 		// FIXME: we will just assume it's a branch - this was the old way - at some point we may want to enforce that there is always a ref here.

modules/git/ref.go

@@ -4,7 +4,10 @@
 
 package git
 
-import "strings"
+import (
+	"regexp"
+	"strings"
+)
 
 const (
 	// RemotePrefix is the base directory of the remotes information of git.
@@ -15,6 +18,29 @@ const (
 	pullLen = len(PullPrefix)
 )
 
+// refNamePatternInvalid is regular expression with unallowed characters in git reference name
+// They cannot have ASCII control characters (i.e. bytes whose values are lower than \040, or \177 DEL), space, tilde ~, caret ^, or colon : anywhere.
+// They cannot have question-mark ?, asterisk *, or open bracket [ anywhere
+var refNamePatternInvalid = regexp.MustCompile(
+	`[\000-\037\177 \\~^:?*[]|` + // No absolutely invalid characters
+		`(?:^[/.])|` + // Not HasPrefix("/") or "."
+		`(?:/\.)|` + // no "/."
+		`(?:\.lock$)|(?:\.lock/)|` + // No ".lock/"" or ".lock" at the end
+		`(?:\.\.)|` + // no ".." anywhere
+		`(?://)|` + // no "//" anywhere
+		`(?:@{)|` + // no "@{"
+		`(?:[/.]$)|` + // no terminal '/' or '.'
+		`(?:^@$)`) // Not "@"
+
+// IsValidRefPattern ensures that the provided string could be a valid reference
+func IsValidRefPattern(name string) bool {
+	return !refNamePatternInvalid.MatchString(name)
+}
+
+func SanitizeRefPattern(name string) string {
+	return refNamePatternInvalid.ReplaceAllString(name, "_")
+}
+
 // Reference represents a Git ref.
 type Reference struct {
 	Name   string

modules/git/repo_commit_nogogit.go

@@ -138,7 +138,7 @@ func (repo *Repository) getCommitFromBatchReader(rd *bufio.Reader, id SHA1) (*Co
 
 // ConvertToSHA1 returns a Hash object from a potential ID string
 func (repo *Repository) ConvertToSHA1(commitID string) (SHA1, error) {
-	if len(commitID) == 40 && SHAPattern.MatchString(commitID) {
+	if len(commitID) == 40 && IsValidSHAPattern(commitID) {
 		sha1, err := NewIDFromString(commitID)
 		if err == nil {
 			return sha1, nil

modules/git/sha1.go

@@ -19,7 +19,12 @@ const EmptySHA = "0000000000000000000000000000000000000000"
 const EmptyTreeSHA = "4b825dc642cb6eb9a060e54bf8d69288fbee4904"
 
 // SHAPattern can be used to determine if a string is an valid sha
-var SHAPattern = regexp.MustCompile(`^[0-9a-f]{4,40}$`)
+var shaPattern = regexp.MustCompile(`^[0-9a-f]{4,40}$`)
+
+// IsValidSHAPattern will check if the provided string matches the SHA Pattern
+func IsValidSHAPattern(sha string) bool {
+	return shaPattern.MatchString(sha)
+}
 
 // MustID always creates a new SHA1 from a [20]byte array with no validation of input.
 func MustID(b []byte) SHA1 {

modules/migration/pullrequest.go

@@ -26,7 +26,7 @@ type PullRequest struct {
 	Updated        time.Time
 	Closed         *time.Time
 	Labels         []*Label
-	PatchURL       string `yaml:"patch_url"`
+	PatchURL       string `yaml:"patch_url"` // SECURITY: This must be safe to download directly from
 	Merged         bool
 	MergedTime     *time.Time `yaml:"merged_time"`
 	MergeCommitSHA string     `yaml:"merge_commit_sha"`
@@ -37,6 +37,7 @@ type PullRequest struct {
 	Reactions      []*Reaction
 	ForeignIndex   int64
 	Context        DownloaderContext `yaml:"-"`
+	EnsuredSafe    bool              `yaml:"ensured_safe"`
 }
 
 func (p *PullRequest) GetLocalIndex() int64          { return p.Number }
@@ -55,9 +56,9 @@ func (p PullRequest) GetGitRefName() string {
 
 // PullRequestBranch represents a pull request branch
 type PullRequestBranch struct {
-	CloneURL  string `yaml:"clone_url"`
-	Ref       string
-	SHA       string
+	CloneURL  string `yaml:"clone_url"` // SECURITY: This must be safe to download from
+	Ref       string // SECURITY: this must be a git.IsValidRefPattern
+	SHA       string // SECURITY: this must be a git.IsValidSHAPattern
 	RepoName  string `yaml:"repo_name"`
 	OwnerName string `yaml:"owner_name"`
 }

modules/migration/release.go

@@ -18,15 +18,16 @@ type ReleaseAsset struct {
 	DownloadCount *int `yaml:"download_count"`
 	Created       time.Time
 	Updated       time.Time
-	DownloadURL   *string `yaml:"download_url"`
+
+	DownloadURL *string `yaml:"download_url"` // SECURITY: It is the responsibility of downloader to make sure this is safe
 	// if DownloadURL is nil, the function should be invoked
-	DownloadFunc func() (io.ReadCloser, error) `yaml:"-"`
+	DownloadFunc func() (io.ReadCloser, error) `yaml:"-"` // SECURITY: It is the responsibility of downloader to make sure this is safe
 }
 
 // Release represents a release
 type Release struct {
-	TagName         string `yaml:"tag_name"`
-	TargetCommitish string `yaml:"target_commitish"`
+	TagName         string `yaml:"tag_name"`         // SECURITY: This must pass git.IsValidRefPattern
+	TargetCommitish string `yaml:"target_commitish"` // SECURITY: This must pass git.IsValidRefPattern
 	Name            string
 	Body            string
 	Draft           bool

modules/migration/repo.go

@@ -12,7 +12,7 @@ type Repository struct {
 	IsPrivate     bool `yaml:"is_private"`
 	IsMirror      bool `yaml:"is_mirror"`
 	Description   string
-	CloneURL      string `yaml:"clone_url"`
+	CloneURL      string `yaml:"clone_url"` // SECURITY: This must be checked to ensure that is safe to be used
 	OriginalURL   string `yaml:"original_url"`
 	DefaultBranch string
 }

modules/validation/binding.go

@@ -9,6 +9,8 @@ import (
 	"regexp"
 	"strings"
 
+	"code.gitea.io/gitea/modules/git"
+
 	"gitea.com/go-chi/binding"
 	"github.com/gobwas/glob"
 )
@@ -24,30 +26,6 @@ const (
 	ErrRegexPattern = "RegexPattern"
 )
 
-// GitRefNamePatternInvalid is regular expression with unallowed characters in git reference name
-// They cannot have ASCII control characters (i.e. bytes whose values are lower than \040, or \177 DEL), space, tilde ~, caret ^, or colon : anywhere.
-// They cannot have question-mark ?, asterisk *, or open bracket [ anywhere
-var GitRefNamePatternInvalid = regexp.MustCompile(`[\000-\037\177 \\~^:?*[]+`)
-
-// CheckGitRefAdditionalRulesValid check name is valid on additional rules
-func CheckGitRefAdditionalRulesValid(name string) bool {
-	// Additional rules as described at https://www.kernel.org/pub/software/scm/git/docs/git-check-ref-format.html
-	if strings.HasPrefix(name, "/") || strings.HasSuffix(name, "/") ||
-		strings.HasSuffix(name, ".") || strings.Contains(name, "..") ||
-		strings.Contains(name, "//") || strings.Contains(name, "@{") ||
-		name == "@" {
-		return false
-	}
-	parts := strings.Split(name, "/")
-	for _, part := range parts {
-		if strings.HasSuffix(part, ".lock") || strings.HasPrefix(part, ".") {
-			return false
-		}
-	}
-
-	return true
-}
-
 // AddBindingRules adds additional binding rules
 func AddBindingRules() {
 	addGitRefNameBindingRule()
@@ -67,16 +45,10 @@ func addGitRefNameBindingRule() {
 		IsValid: func(errs binding.Errors, name string, val interface{}) (bool, binding.Errors) {
 			str := fmt.Sprintf("%v", val)
 
-			if GitRefNamePatternInvalid.MatchString(str) {
+			if !git.IsValidRefPattern(str) {
 				errs.Add([]string{name}, ErrGitRefName, "GitRefName")
 				return false, errs
 			}
-
-			if !CheckGitRefAdditionalRulesValid(str) {
-				errs.Add([]string{name}, ErrGitRefName, "GitRefName")
-				return false, errs
-			}
-
 			return true, errs
 		},
 	})

routers/api/v1/repo/commits.go

@@ -17,7 +17,6 @@ import (
 	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/setting"
 	api "code.gitea.io/gitea/modules/structs"
-	"code.gitea.io/gitea/modules/validation"
 	"code.gitea.io/gitea/routers/api/v1/utils"
 )
 
@@ -53,7 +52,7 @@ func GetSingleCommit(ctx *context.APIContext) {
 	//     "$ref": "#/responses/notFound"
 
 	sha := ctx.Params(":sha")
-	if (validation.GitRefNamePatternInvalid.MatchString(sha) || !validation.CheckGitRefAdditionalRulesValid(sha)) && !git.SHAPattern.MatchString(sha) {
+	if !git.IsValidRefPattern(sha) {
 		ctx.Error(http.StatusUnprocessableEntity, "no valid ref or sha", fmt.Sprintf("no valid ref or sha: %s", sha))
 		return
 	}

routers/api/v1/repo/notes.go

@@ -12,7 +12,6 @@ import (
 	"code.gitea.io/gitea/modules/convert"
 	"code.gitea.io/gitea/modules/git"
 	api "code.gitea.io/gitea/modules/structs"
-	"code.gitea.io/gitea/modules/validation"
 )
 
 // GetNote Get a note corresponding to a single commit from a repository
@@ -47,7 +46,7 @@ func GetNote(ctx *context.APIContext) {
 	//     "$ref": "#/responses/notFound"
 
 	sha := ctx.Params(":sha")
-	if (validation.GitRefNamePatternInvalid.MatchString(sha) || !validation.CheckGitRefAdditionalRulesValid(sha)) && !git.SHAPattern.MatchString(sha) {
+	if !git.IsValidRefPattern(sha) {
 		ctx.Error(http.StatusUnprocessableEntity, "no valid ref or sha", fmt.Sprintf("no valid ref or sha: %s", sha))
 		return
 	}