Skip to content

Ensure validation occurs on clone addresses too #14994

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 2 additions & 5 deletions integrations/api_repo_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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},
}

Expand All @@ -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)
}
Expand Down
54 changes: 27 additions & 27 deletions models/error.go
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
}

// __________ .__
// \______ \____________ ____ ____ | |__
// | | _/\_ __ \__ \ / \_/ ___\| | \
Expand Down
2 changes: 1 addition & 1 deletion models/user.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
22 changes: 2 additions & 20 deletions modules/forms/repo_form.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -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
Expand Down
59 changes: 41 additions & 18 deletions modules/migrations/migrate.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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")) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems subtle, might make sense to separate this condition into its own func so it's less likely to occur again that checks for the same thing are implemented differently

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was in its own function that wasn't being used! The point is to make sure that there really is only place where this is done.

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}
}
}
}
Expand All @@ -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
}
Expand Down
36 changes: 30 additions & 6 deletions modules/migrations/migrate_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 2 additions & 0 deletions options/locale/locale_en-US.ini
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
17 changes: 13 additions & 4 deletions routers/api/v1/repo/migrate.go
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down Expand Up @@ -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)
Expand Down
Loading