Ensure validation occurs on clone addresses too (go-gitea#14994)

* Ensure validation occurs on clone addresses too Fix go-gitea#14984 Signed-off-by: Andrew Thornton <[email protected]> * fix lint Signed-off-by: Andrew Thornton <[email protected]> * fix test Signed-off-by: Andrew Thornton <[email protected]> * Fix api tests Signed-off-by: Andrew Thornton <[email protected]> Co-authored-by: techknowlogick <[email protected]>
gfxlabs · Mar 15, 2021 · 6e423d5 · 6e423d5
1 parent f268b48
commit 6e423d5
Show file tree

Hide file tree

Showing 10 changed files with 165 additions and 129 deletions.
diff --git a/integrations/api_repo_test.go b/integrations/api_repo_test.go
@@ -309,7 +309,7 @@ func TestAPIRepoMigrate(t *testing.T) {
 		{ctxUserID: 2, userID: 1, cloneURL: "https://github.com/go-gitea/test_repo.git", repoName: "git-bad", expectedStatus: http.StatusForbidden},
 		{ctxUserID: 2, userID: 3, cloneURL: "https://github.com/go-gitea/test_repo.git", repoName: "git-org", expectedStatus: http.StatusCreated},
 		{ctxUserID: 2, userID: 6, cloneURL: "https://github.com/go-gitea/test_repo.git", repoName: "git-bad-org", expectedStatus: http.StatusForbidden},
-		{ctxUserID: 2, userID: 3, cloneURL: "https://localhost:3000/user/test_repo.git", repoName: "local-ip", expectedStatus: http.StatusUnprocessableEntity},
+		{ctxUserID: 2, userID: 3, cloneURL: "https://localhost:3000/user/test_repo.git", repoName: "private-ip", expectedStatus: http.StatusUnprocessableEntity},
 		{ctxUserID: 2, userID: 3, cloneURL: "https://10.0.0.1/user/test_repo.git", repoName: "private-ip", expectedStatus: http.StatusUnprocessableEntity},
 	}
 
@@ -330,11 +330,8 @@ func TestAPIRepoMigrate(t *testing.T) {
 			switch respJSON["message"] {
 			case "Remote visit addressed rate limitation.":
 				t.Log("test hit github rate limitation")
-			case "migrate from '10.0.0.1' is not allowed: the host resolve to a private ip address '10.0.0.1'":
+			case "You are not allowed to import from private IPs.":
 				assert.EqualValues(t, "private-ip", testCase.repoName)
-			case "migrate from 'localhost:3000' is not allowed: the host resolve to a private ip address '::1'",
-				"migrate from 'localhost:3000' is not allowed: the host resolve to a private ip address '127.0.0.1'":
-				assert.EqualValues(t, "local-ip", testCase.repoName)
 			default:
 				t.Errorf("unexpected error '%v' on url '%s'", respJSON["message"], testCase.cloneURL)
 			}

diff --git a/models/error.go b/models/error.go
@@ -855,20 +855,43 @@ func (err ErrRepoRedirectNotExist) Error() string {
 
 // ErrInvalidCloneAddr represents a "InvalidCloneAddr" kind of error.
 type ErrInvalidCloneAddr struct {
+	Host               string
 	IsURLError         bool
 	IsInvalidPath      bool
+	IsProtocolInvalid  bool
 	IsPermissionDenied bool
+	LocalPath          bool
+	NotResolvedIP      bool
+	PrivateNet         string
 }
 
 // IsErrInvalidCloneAddr checks if an error is a ErrInvalidCloneAddr.
 func IsErrInvalidCloneAddr(err error) bool {
-	_, ok := err.(ErrInvalidCloneAddr)
+	_, ok := err.(*ErrInvalidCloneAddr)
 	return ok
 }
 
-func (err ErrInvalidCloneAddr) Error() string {
-	return fmt.Sprintf("invalid clone address [is_url_error: %v, is_invalid_path: %v, is_permission_denied: %v]",
-		err.IsURLError, err.IsInvalidPath, err.IsPermissionDenied)
+func (err *ErrInvalidCloneAddr) Error() string {
+	if err.NotResolvedIP {
+		return fmt.Sprintf("migration/cloning from '%s' is not allowed: unknown hostname", err.Host)
+	}
+	if len(err.PrivateNet) != 0 {
+		return fmt.Sprintf("migration/cloning from '%s' is not allowed: the host resolve to a private ip address '%s'", err.Host, err.PrivateNet)
+	}
+	if err.IsInvalidPath {
+		return fmt.Sprintf("migration/cloning from '%s' is not allowed: the provided path is invalid", err.Host)
+	}
+	if err.IsProtocolInvalid {
+		return fmt.Sprintf("migration/cloning from '%s' is not allowed: the provided url protocol is not allowed", err.Host)
+	}
+	if err.IsPermissionDenied {
+		return fmt.Sprintf("migration/cloning from '%s' is not allowed.", err.Host)
+	}
+	if err.IsURLError {
+		return fmt.Sprintf("migration/cloning from '%s' is not allowed: the provided url is invalid", err.Host)
+	}
+
+	return fmt.Sprintf("migration/cloning from '%s' is not allowed", err.Host)
 }
 
 // ErrUpdateTaskNotExist represents a "UpdateTaskNotExist" kind of error.
@@ -1065,29 +1088,6 @@ func IsErrWontSign(err error) bool {
 	return ok
 }
 
-// ErrMigrationNotAllowed explains why a migration from an url is not allowed
-type ErrMigrationNotAllowed struct {
-	Host          string
-	NotResolvedIP bool
-	PrivateNet    string
-}
-
-func (e *ErrMigrationNotAllowed) Error() string {
-	if e.NotResolvedIP {
-		return fmt.Sprintf("migrate from '%s' is not allowed: unknown hostname", e.Host)
-	}
-	if len(e.PrivateNet) != 0 {
-		return fmt.Sprintf("migrate from '%s' is not allowed: the host resolve to a private ip address '%s'", e.Host, e.PrivateNet)
-	}
-	return fmt.Sprintf("migrate from '%s is not allowed'", e.Host)
-}
-
-// IsErrMigrationNotAllowed checks if an error is a ErrMigrationNotAllowed
-func IsErrMigrationNotAllowed(err error) bool {
-	_, ok := err.(*ErrMigrationNotAllowed)
-	return ok
-}
-
 // __________                             .__
 // \______   \____________    ____   ____ |  |__
 //  |    |  _/\_  __ \__  \  /    \_/ ___\|  |  \

diff --git a/models/user.go b/models/user.go
@@ -296,7 +296,7 @@ func (u *User) CanEditGitHook() bool {
 
 // CanImportLocal returns true if user can migrate repository by local path.
 func (u *User) CanImportLocal() bool {
-	if !setting.ImportLocalPaths {
+	if !setting.ImportLocalPaths || u == nil {
 		return false
 	}
 	return u.IsAdmin || u.AllowImportLocal

diff --git a/modules/forms/repo_form.go b/modules/forms/repo_form.go
@@ -12,10 +12,8 @@ import (
 
 	"code.gitea.io/gitea/models"
 	"code.gitea.io/gitea/modules/context"
-	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/setting"
 	"code.gitea.io/gitea/modules/structs"
-	"code.gitea.io/gitea/modules/util"
 	"code.gitea.io/gitea/modules/web/middleware"
 	"code.gitea.io/gitea/routers/utils"
 
@@ -92,36 +90,20 @@ func (f *MigrateRepoForm) Validate(req *http.Request, errs binding.Errors) bindi
 
 // ParseRemoteAddr checks if given remote address is valid,
 // and returns composed URL with needed username and password.
-// It also checks if given user has permission when remote address
-// is actually a local path.
-func ParseRemoteAddr(remoteAddr, authUsername, authPassword string, user *models.User) (string, error) {
+func ParseRemoteAddr(remoteAddr, authUsername, authPassword string) (string, error) {
 	remoteAddr = strings.TrimSpace(remoteAddr)
 	// Remote address can be HTTP/HTTPS/Git URL or local path.
 	if strings.HasPrefix(remoteAddr, "http://") ||
 		strings.HasPrefix(remoteAddr, "https://") ||
 		strings.HasPrefix(remoteAddr, "git://") {
 		u, err := url.Parse(remoteAddr)
 		if err != nil {
-			return "", models.ErrInvalidCloneAddr{IsURLError: true}
+			return "", &models.ErrInvalidCloneAddr{IsURLError: true}
 		}
 		if len(authUsername)+len(authPassword) > 0 {
 			u.User = url.UserPassword(authUsername, authPassword)
 		}
 		remoteAddr = u.String()
-		if u.Scheme == "git" && u.Port() != "" && (strings.Contains(remoteAddr, "%0d") || strings.Contains(remoteAddr, "%0a")) {
-			return "", models.ErrInvalidCloneAddr{IsURLError: true}
-		}
-	} else if !user.CanImportLocal() {
-		return "", models.ErrInvalidCloneAddr{IsPermissionDenied: true}
-	} else {
-		isDir, err := util.IsDir(remoteAddr)
-		if err != nil {
-			log.Error("Unable to check if %s is a directory: %v", remoteAddr, err)
-			return "", err
-		}
-		if !isDir {
-			return "", models.ErrInvalidCloneAddr{IsInvalidPath: true}
-		}
 	}
 
 	return remoteAddr, nil

diff --git a/modules/migrations/migrate.go b/modules/migrations/migrate.go
@@ -10,13 +10,15 @@ import (
 	"fmt"
 	"net"
 	"net/url"
+	"path/filepath"
 	"strings"
 
 	"code.gitea.io/gitea/models"
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/matchlist"
 	"code.gitea.io/gitea/modules/migrations/base"
 	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/modules/util"
 )
 
 // MigrateOptions is equal to base.MigrateOptions
@@ -34,39 +36,60 @@ func RegisterDownloaderFactory(factory base.DownloaderFactory) {
 	factories = append(factories, factory)
 }
 
-func isMigrateURLAllowed(remoteURL string) error {
+// IsMigrateURLAllowed checks if an URL is allowed to be migrated from
+func IsMigrateURLAllowed(remoteURL string, doer *models.User) error {
+	// Remote address can be HTTP/HTTPS/Git URL or local path.
 	u, err := url.Parse(strings.ToLower(remoteURL))
 	if err != nil {
-		return err
+		return &models.ErrInvalidCloneAddr{IsURLError: true}
 	}
 
-	if strings.EqualFold(u.Scheme, "http") || strings.EqualFold(u.Scheme, "https") {
-		if len(setting.Migrations.AllowedDomains) > 0 {
-			if !allowList.Match(u.Host) {
-				return &models.ErrMigrationNotAllowed{Host: u.Host}
-			}
-		} else {
-			if blockList.Match(u.Host) {
-				return &models.ErrMigrationNotAllowed{Host: u.Host}
-			}
+	if u.Scheme == "file" || u.Scheme == "" {
+		if !doer.CanImportLocal() {
+			return &models.ErrInvalidCloneAddr{Host: "<LOCAL_FILESYSTEM>", IsPermissionDenied: true, LocalPath: true}
+		}
+		isAbs := filepath.IsAbs(u.Host + u.Path)
+		if !isAbs {
+			return &models.ErrInvalidCloneAddr{Host: "<LOCAL_FILESYSTEM>", IsInvalidPath: true, LocalPath: true}
+		}
+		isDir, err := util.IsDir(u.Host + u.Path)
+		if err != nil {
+			log.Error("Unable to check if %s is a directory: %v", u.Host+u.Path, err)
+			return err
+		}
+		if !isDir {
+			return &models.ErrInvalidCloneAddr{Host: "<LOCAL_FILESYSTEM>", IsInvalidPath: true, LocalPath: true}
 		}
+
+		return nil
+	}
+
+	if u.Scheme == "git" && u.Port() != "" && (strings.Contains(remoteURL, "%0d") || strings.Contains(remoteURL, "%0a")) {
+		return &models.ErrInvalidCloneAddr{Host: u.Host, IsURLError: true}
 	}
 
-	if u.Host == "" {
-		if !setting.ImportLocalPaths {
-			return &models.ErrMigrationNotAllowed{Host: "<LOCAL_FILESYSTEM>"}
+	if u.Opaque != "" || u.Scheme != "" && u.Scheme != "http" && u.Scheme != "https" && u.Scheme != "git" {
+		return &models.ErrInvalidCloneAddr{Host: u.Host, IsProtocolInvalid: true, IsPermissionDenied: true, IsURLError: true}
+	}
+
+	if len(setting.Migrations.AllowedDomains) > 0 {
+		if !allowList.Match(u.Host) {
+			return &models.ErrInvalidCloneAddr{Host: u.Host, IsPermissionDenied: true}
+		}
+	} else {
+		if blockList.Match(u.Host) {
+			return &models.ErrInvalidCloneAddr{Host: u.Host, IsPermissionDenied: true}
 		}
-		return nil
 	}
 
 	if !setting.Migrations.AllowLocalNetworks {
 		addrList, err := net.LookupIP(strings.Split(u.Host, ":")[0])
 		if err != nil {
-			return &models.ErrMigrationNotAllowed{Host: u.Host, NotResolvedIP: true}
+			return &models.ErrInvalidCloneAddr{Host: u.Host, NotResolvedIP: true}
 		}
 		for _, addr := range addrList {
 			if isIPPrivate(addr) || !addr.IsGlobalUnicast() {
-				return &models.ErrMigrationNotAllowed{Host: u.Host, PrivateNet: addr.String()}
+				return &models.ErrInvalidCloneAddr{Host: u.Host, PrivateNet: addr.String(), IsPermissionDenied: true}
 			}
 		}
 	}
@@ -76,7 +99,7 @@ func isMigrateURLAllowed(remoteURL string) error {
 
 // MigrateRepository migrate repository according MigrateOptions
 func MigrateRepository(ctx context.Context, doer *models.User, ownerName string, opts base.MigrateOptions) (*models.Repository, error) {
-	err := isMigrateURLAllowed(opts.CloneAddr)
+	err := IsMigrateURLAllowed(opts.CloneAddr, doer)
 	if err != nil {
 		return nil, err
 	}

diff --git a/modules/migrations/migrate_test.go b/modules/migrations/migrate_test.go
@@ -5,41 +5,65 @@
 package migrations
 
 import (
+	"path/filepath"
 	"testing"
 
+	"code.gitea.io/gitea/models"
 	"code.gitea.io/gitea/modules/setting"
 
 	"github.com/stretchr/testify/assert"
 )
 
 func TestMigrateWhiteBlocklist(t *testing.T) {
+	assert.NoError(t, models.PrepareTestDatabase())
+
+	adminUser := models.AssertExistsAndLoadBean(t, &models.User{Name: "user1"}).(*models.User)
+	nonAdminUser := models.AssertExistsAndLoadBean(t, &models.User{Name: "user2"}).(*models.User)
+
 	setting.Migrations.AllowedDomains = []string{"github.com"}
 	assert.NoError(t, Init())
 
-	err := isMigrateURLAllowed("https://gitlab.com/gitlab/gitlab.git")
+	err := IsMigrateURLAllowed("https://gitlab.com/gitlab/gitlab.git", nonAdminUser)
 	assert.Error(t, err)
 
-	err = isMigrateURLAllowed("https://github.com/go-gitea/gitea.git")
+	err = IsMigrateURLAllowed("https://github.com/go-gitea/gitea.git", nonAdminUser)
 	assert.NoError(t, err)
 
 	setting.Migrations.AllowedDomains = []string{}
 	setting.Migrations.BlockedDomains = []string{"github.com"}
 	assert.NoError(t, Init())
 
-	err = isMigrateURLAllowed("https://gitlab.com/gitlab/gitlab.git")
+	err = IsMigrateURLAllowed("https://gitlab.com/gitlab/gitlab.git", nonAdminUser)
 	assert.NoError(t, err)
 
-	err = isMigrateURLAllowed("https://github.com/go-gitea/gitea.git")
+	err = IsMigrateURLAllowed("https://github.com/go-gitea/gitea.git", nonAdminUser)
+	assert.Error(t, err)
+
+	err = IsMigrateURLAllowed("https://10.0.0.1/go-gitea/gitea.git", nonAdminUser)
 	assert.Error(t, err)
 
+	setting.Migrations.AllowLocalNetworks = true
+	err = IsMigrateURLAllowed("https://10.0.0.1/go-gitea/gitea.git", nonAdminUser)
+	assert.NoError(t, err)
+
 	old := setting.ImportLocalPaths
 	setting.ImportLocalPaths = false
 
-	err = isMigrateURLAllowed("/home/foo/bar/goo")
+	err = IsMigrateURLAllowed("/home/foo/bar/goo", adminUser)
 	assert.Error(t, err)
 
 	setting.ImportLocalPaths = true
-	err = isMigrateURLAllowed("/home/foo/bar/goo")
+	abs, err := filepath.Abs(".")
+	assert.NoError(t, err)
+
+	err = IsMigrateURLAllowed(abs, adminUser)
+	assert.NoError(t, err)
+
+	err = IsMigrateURLAllowed(abs, nonAdminUser)
+	assert.Error(t, err)
+
+	nonAdminUser.AllowImportLocal = true
+	err = IsMigrateURLAllowed(abs, nonAdminUser)
 	assert.NoError(t, err)
 
 	setting.ImportLocalPaths = old

diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini
@@ -789,6 +789,8 @@ migrate.clone_address = Migrate / Clone From URL
 migrate.clone_address_desc = The HTTP(S) or Git 'clone' URL of an existing repository
 migrate.clone_local_path = or a local server path
 migrate.permission_denied = You are not allowed to import local repositories.
+migrate.permission_denied_blocked = You are not allowed to import from blocked hosts.
+migrate.permission_denied_private_ip = You are not allowed to import from private IPs.
 migrate.invalid_local_path = "The local path is invalid. It does not exist or is not a directory."
 migrate.failed = Migration failed: %v
 migrate.lfs_mirror_unsupported = Mirroring LFS objects is not supported - use 'git lfs fetch --all' and 'git lfs push --all' instead.

diff --git a/routers/api/v1/repo/migrate.go b/routers/api/v1/repo/migrate.go
@@ -96,15 +96,24 @@ func Migrate(ctx *context.APIContext) {
 		}
 	}
 
-	remoteAddr, err := auth.ParseRemoteAddr(form.CloneAddr, form.AuthUsername, form.AuthPassword, ctx.User)
+	remoteAddr, err := auth.ParseRemoteAddr(form.CloneAddr, form.AuthUsername, form.AuthPassword)
+	if err == nil {
+		err = migrations.IsMigrateURLAllowed(remoteAddr, ctx.User)
+	}
 	if err != nil {
 		if models.IsErrInvalidCloneAddr(err) {
-			addrErr := err.(models.ErrInvalidCloneAddr)
+			addrErr := err.(*models.ErrInvalidCloneAddr)
 			switch {
 			case addrErr.IsURLError:
 				ctx.Error(http.StatusUnprocessableEntity, "", err)
 			case addrErr.IsPermissionDenied:
-				ctx.Error(http.StatusUnprocessableEntity, "", "You are not allowed to import local repositories.")
+				if addrErr.LocalPath {
+					ctx.Error(http.StatusUnprocessableEntity, "", "You are not allowed to import local repositories.")
+				} else if len(addrErr.PrivateNet) == 0 {
+					ctx.Error(http.StatusUnprocessableEntity, "", "You are not allowed to import from blocked hosts.")
+				} else {
+					ctx.Error(http.StatusUnprocessableEntity, "", "You are not allowed to import from private IPs.")
+				}
 			case addrErr.IsInvalidPath:
 				ctx.Error(http.StatusUnprocessableEntity, "", "Invalid local path, it does not exist or not a directory.")
 			default:
@@ -219,7 +228,7 @@ func handleMigrateError(ctx *context.APIContext, repoOwner *models.User, remoteA
 		ctx.Error(http.StatusUnprocessableEntity, "", fmt.Sprintf("The username '%s' contains invalid characters.", err.(models.ErrNameCharsNotAllowed).Name))
 	case models.IsErrNamePatternNotAllowed(err):
 		ctx.Error(http.StatusUnprocessableEntity, "", fmt.Sprintf("The pattern '%s' is not allowed in a username.", err.(models.ErrNamePatternNotAllowed).Pattern))
-	case models.IsErrMigrationNotAllowed(err):
+	case models.IsErrInvalidCloneAddr(err):
 		ctx.Error(http.StatusUnprocessableEntity, "", err)
 	case base.IsErrNotSupported(err):
 		ctx.Error(http.StatusUnprocessableEntity, "", err)