Use hostmatcher to replace matchlist to improve blocking of bad hosts in Webhooks

integrations/api_repo_lfs_migrate_test.go

@@ -14,6 +14,7 @@ import (
 	"code.gitea.io/gitea/modules/lfs"
 	"code.gitea.io/gitea/modules/setting"
 	api "code.gitea.io/gitea/modules/structs"
+	"code.gitea.io/gitea/services/migrations"
 
 	"github.com/stretchr/testify/assert"
 )
@@ -25,6 +26,7 @@ func TestAPIRepoLFSMigrateLocal(t *testing.T) {
 	oldAllowLocalNetworks := setting.Migrations.AllowLocalNetworks
 	setting.ImportLocalPaths = true
 	setting.Migrations.AllowLocalNetworks = true
+	assert.NoError(t, migrations.Init())
 
 	user := unittest.AssertExistsAndLoadBean(t, &models.User{ID: 1}).(*models.User)
 	session := loginUser(t, user.Name)
@@ -47,4 +49,5 @@ func TestAPIRepoLFSMigrateLocal(t *testing.T) {
 
 	setting.ImportLocalPaths = oldImportLocalPaths
 	setting.Migrations.AllowLocalNetworks = oldAllowLocalNetworks
+	assert.NoError(t, migrations.Init()) // reset old migration settings
 }

integrations/api_repo_test.go

@@ -331,10 +331,10 @@ func TestAPIRepoMigrate(t *testing.T) {
 			switch respJSON["message"] {
 			case "Remote visit addressed rate limitation.":
 				t.Log("test hit github rate limitation")
-			case "You are not allowed to import from private IPs.":
+			case "You can not import from disallowed hosts.":
 				assert.EqualValues(t, "private-ip", testCase.repoName)
 			default:
-				t.Errorf("unexpected error '%v' on url '%s'", respJSON["message"], testCase.cloneURL)
+				assert.Fail(t, "unexpected error '%v' on url '%s'", respJSON["message"], testCase.cloneURL)
 			}
 		} else {
 			assert.EqualValues(t, testCase.expectedStatus, resp.Code)

integrations/mirror_pull_test.go

@@ -47,7 +47,7 @@ func TestMirrorPull(t *testing.T) {
 
 	ctx := context.Background()
 
-	mirror, err := repository.MigrateRepositoryGitData(ctx, user, mirrorRepo, opts)
+	mirror, err := repository.MigrateRepositoryGitData(ctx, user, mirrorRepo, opts, nil)
 	assert.NoError(t, err)
 
 	gitRepo, err := git.OpenRepository(repoPath)

integrations/mirror_push_test.go

@@ -16,6 +16,7 @@ import (
 	"code.gitea.io/gitea/modules/git"
 	"code.gitea.io/gitea/modules/repository"
 	"code.gitea.io/gitea/modules/setting"
+	"code.gitea.io/gitea/services/migrations"
 	mirror_service "code.gitea.io/gitea/services/mirror"
 
 	"github.com/stretchr/testify/assert"
@@ -29,6 +30,7 @@ func testMirrorPush(t *testing.T, u *url.URL) {
 	defer prepareTestEnv(t)()
 
 	setting.Migrations.AllowLocalNetworks = true
+	assert.NoError(t, migrations.Init())
 
 	user := unittest.AssertExistsAndLoadBean(t, &models.User{ID: 2}).(*models.User)
 	srcRepo := unittest.AssertExistsAndLoadBean(t, &models.Repository{ID: 1}).(*models.Repository)

models/error.go

@@ -797,7 +797,6 @@ type ErrInvalidCloneAddr struct {
 	IsPermissionDenied bool
 	LocalPath          bool
 	NotResolvedIP      bool
-	PrivateNet         string
 }
 
 // IsErrInvalidCloneAddr checks if an error is a ErrInvalidCloneAddr.
@@ -810,9 +809,6 @@ 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)
 	}

modules/hostmatcher/hostmatcher.go

@@ -13,15 +13,18 @@ import (
 )
 
 // HostMatchList is used to check if a host or IP is in a list.
-// If you only need to do wildcard matching, consider to use modules/matchlist
 type HostMatchList struct {
-	hosts  []string
+	SettingKeyHint string
+	SettingValue   string
+
+	// builtins networks
+	builtins []string
+	// patterns for host names (with wildcard support)
+	patterns []string
+	// ipNets is the CIDR network list
 	ipNets []*net.IPNet
 }
 
-// MatchBuiltinAll all hosts are matched
-const MatchBuiltinAll = "*"
-
 // MatchBuiltinExternal A valid non-private unicast IP, all hosts on public internet are matched
 const MatchBuiltinExternal = "external"
 
@@ -31,9 +34,13 @@ const MatchBuiltinPrivate = "private"
 // MatchBuiltinLoopback 127.0.0.0/8 for IPv4 and ::1/128 for IPv6, localhost is included.
 const MatchBuiltinLoopback = "loopback"
 
+func isBuiltin(s string) bool {
+	return s == MatchBuiltinExternal || s == MatchBuiltinPrivate || s == MatchBuiltinLoopback
+}
+
 // ParseHostMatchList parses the host list HostMatchList
-func ParseHostMatchList(hostList string) *HostMatchList {
-	hl := &HostMatchList{}
+func ParseHostMatchList(settingKeyHint string, hostList string) *HostMatchList {
+	hl := &HostMatchList{SettingKeyHint: settingKeyHint, SettingValue: hostList}
 	for _, s := range strings.Split(hostList, ",") {
 		s = strings.ToLower(strings.TrimSpace(s))
 		if s == "" {
@@ -42,53 +49,106 @@ func ParseHostMatchList(hostList string) *HostMatchList {
 		_, ipNet, err := net.ParseCIDR(s)
 		if err == nil {
 			hl.ipNets = append(hl.ipNets, ipNet)
+		} else if isBuiltin(s) {
+			hl.builtins = append(hl.builtins, s)
 		} else {
-			hl.hosts = append(hl.hosts, s)
+			hl.patterns = append(hl.patterns, s)
 		}
 	}
 	return hl
 }
 
-// MatchesHostOrIP checks if the host or IP matches an allow/deny(block) list
-func (hl *HostMatchList) MatchesHostOrIP(host string, ip net.IP) bool {
-	var matched bool
-	host = strings.ToLower(host)
-	ipStr := ip.String()
-loop:
-	for _, hostInList := range hl.hosts {
-		switch hostInList {
-		case "":
+// ParseSimpleMatchList parse a simple matchlist (no built-in networks, no CIDR support, only wildcard pattern match)
+func ParseSimpleMatchList(settingKeyHint string, matchList string) *HostMatchList {
+	hl := &HostMatchList{
+		SettingKeyHint: settingKeyHint,
+		SettingValue:   matchList,
+	}
+	for _, s := range strings.Split(matchList, ",") {
+		s = strings.ToLower(strings.TrimSpace(s))
+		if s == "" {
 			continue
-		case MatchBuiltinAll:
-			matched = true
-			break loop
+		}
+		// we keep the same result as old `matchlist`, so no builtin/CIDR support here, we only match wildcard patterns
+		hl.patterns = append(hl.patterns, s)
+	}
+	return hl
+}
+
+// AppendBuiltin appends more builtins to match
+func (hl *HostMatchList) AppendBuiltin(builtin string) {
+	hl.builtins = append(hl.builtins, builtin)
+}
+
+// IsEmpty checks if the checklist is empty
+func (hl *HostMatchList) IsEmpty() bool {
+	return hl == nil || (len(hl.builtins) == 0 && len(hl.patterns) == 0 && len(hl.ipNets) == 0)
+}
+
+func (hl *HostMatchList) checkPattern(host string) bool {
+	host = strings.ToLower(strings.TrimSpace(host))
+	for _, pattern := range hl.patterns {
+		if matched, _ := filepath.Match(pattern, host); matched {
+			return true
+		}
+	}
+	return false
+}
+
+func (hl *HostMatchList) checkIP(ip net.IP) bool {
+	for _, pattern := range hl.patterns {
+		if pattern == "*" {
+			return true
+		}
+	}
+	for _, builtin := range hl.builtins {
+		switch builtin {
 		case MatchBuiltinExternal:
-			if matched = ip.IsGlobalUnicast() && !util.IsIPPrivate(ip); matched {
-				break loop
+			if ip.IsGlobalUnicast() && !util.IsIPPrivate(ip) {
+				return true
 			}
 		case MatchBuiltinPrivate:
-			if matched = util.IsIPPrivate(ip); matched {
-				break loop
+			if util.IsIPPrivate(ip) {
+				return true
 			}
 		case MatchBuiltinLoopback:
-			if matched = ip.IsLoopback(); matched {
-				break loop
-			}
-		default:
-			if matched, _ = filepath.Match(hostInList, host); matched {
-				break loop
-			}
-			if matched, _ = filepath.Match(hostInList, ipStr); matched {
-				break loop
+			if ip.IsLoopback() {
+				return true
 			}
 		}
 	}
-	if !matched {
-		for _, ipNet := range hl.ipNets {
-			if matched = ipNet.Contains(ip); matched {
-				break
-			}
+	for _, ipNet := range hl.ipNets {
+		if ipNet.Contains(ip) {
+			return true
 		}
 	}
-	return matched
+	return false
+}
+
+// MatchHostName checks if the host matches an allow/deny(block) list
+func (hl *HostMatchList) MatchHostName(host string) bool {
+	if hl == nil {
+		return false
+	}
+	if hl.checkPattern(host) {
+		return true
+	}
+	if ip := net.ParseIP(host); ip != nil {
+		return hl.checkIP(ip)
+	}
+	return false
+}
+
+// MatchIPAddr checks if the IP matches an allow/deny(block) list, it's safe to pass `nil` to `ip`
+func (hl *HostMatchList) MatchIPAddr(ip net.IP) bool {
+	if hl == nil {
+		return false
+	}
+	host := ip.String() // nil-safe, we will get "<nil>" if ip is nil
+	return hl.checkPattern(host) || hl.checkIP(ip)
+}
+
+// MatchHostOrIP checks if the host or IP matches an allow/deny(block) list
+func (hl *HostMatchList) MatchHostOrIP(host string, ip net.IP) bool {
+	return hl.MatchHostName(host) || hl.MatchIPAddr(ip)
 }

modules/hostmatcher/hostmatcher_test.go

@@ -20,17 +20,28 @@ func TestHostOrIPMatchesList(t *testing.T) {
 
 	// for IPv6: "::1" is loopback, "fd00::/8" is private
 
-	hl := ParseHostMatchList("private, External, *.myDomain.com, 169.254.1.0/24")
+	hl := ParseHostMatchList("", "private, External, *.myDomain.com, 169.254.1.0/24")
+
+	test := func(cases []tc) {
+		for _, c := range cases {
+			assert.Equalf(t, c.expected, hl.MatchHostOrIP(c.host, c.ip), "case domain=%s, ip=%v, expected=%v", c.host, c.ip, c.expected)
+		}
+	}
+
 	cases := []tc{
 		{"", net.IPv4zero, false},
 		{"", net.IPv6zero, false},
 
 		{"", net.ParseIP("127.0.0.1"), false},
+		{"127.0.0.1", nil, false},
 		{"", net.ParseIP("::1"), false},
 
 		{"", net.ParseIP("10.0.1.1"), true},
+		{"10.0.1.1", nil, true},
 		{"", net.ParseIP("192.168.1.1"), true},
+		{"192.168.1.1", nil, true},
 		{"", net.ParseIP("fd00::1"), true},
+		{"fd00::1", nil, true},
 
 		{"", net.ParseIP("8.8.8.8"), true},
 		{"", net.ParseIP("1001::1"), true},
@@ -39,13 +50,13 @@ func TestHostOrIPMatchesList(t *testing.T) {
 		{"sub.mydomain.com", net.IPv4zero, true},
 
 		{"", net.ParseIP("169.254.1.1"), true},
+		{"169.254.1.1", nil, true},
 		{"", net.ParseIP("169.254.2.2"), false},
+		{"169.254.2.2", nil, false},
 	}
-	for _, c := range cases {
-		assert.Equalf(t, c.expected, hl.MatchesHostOrIP(c.host, c.ip), "case %s(%v)", c.host, c.ip)
-	}
+	test(cases)
 
-	hl = ParseHostMatchList("loopback")
+	hl = ParseHostMatchList("", "loopback")
 	cases = []tc{
 		{"", net.IPv4zero, false},
 		{"", net.ParseIP("127.0.0.1"), true},
@@ -59,11 +70,9 @@ func TestHostOrIPMatchesList(t *testing.T) {
 
 		{"mydomain.com", net.IPv4zero, false},
 	}
-	for _, c := range cases {
-		assert.Equalf(t, c.expected, hl.MatchesHostOrIP(c.host, c.ip), "case %s(%v)", c.host, c.ip)
-	}
+	test(cases)
 
-	hl = ParseHostMatchList("private")
+	hl = ParseHostMatchList("", "private")
 	cases = []tc{
 		{"", net.IPv4zero, false},
 		{"", net.ParseIP("127.0.0.1"), false},
@@ -77,11 +86,9 @@ func TestHostOrIPMatchesList(t *testing.T) {
 
 		{"mydomain.com", net.IPv4zero, false},
 	}
-	for _, c := range cases {
-		assert.Equalf(t, c.expected, hl.MatchesHostOrIP(c.host, c.ip), "case %s(%v)", c.host, c.ip)
-	}
+	test(cases)
 
-	hl = ParseHostMatchList("external")
+	hl = ParseHostMatchList("", "external")
 	cases = []tc{
 		{"", net.IPv4zero, false},
 		{"", net.ParseIP("127.0.0.1"), false},
@@ -95,11 +102,9 @@ func TestHostOrIPMatchesList(t *testing.T) {
 
 		{"mydomain.com", net.IPv4zero, false},
 	}
-	for _, c := range cases {
-		assert.Equalf(t, c.expected, hl.MatchesHostOrIP(c.host, c.ip), "case %s(%v)", c.host, c.ip)
-	}
+	test(cases)
 
-	hl = ParseHostMatchList("*")
+	hl = ParseHostMatchList("", "*")
 	cases = []tc{
 		{"", net.IPv4zero, true},
 		{"", net.ParseIP("127.0.0.1"), true},
@@ -113,7 +118,43 @@ func TestHostOrIPMatchesList(t *testing.T) {
 
 		{"mydomain.com", net.IPv4zero, true},
 	}
-	for _, c := range cases {
-		assert.Equalf(t, c.expected, hl.MatchesHostOrIP(c.host, c.ip), "case %s(%v)", c.host, c.ip)
+	test(cases)
+
+	// built-in network names can be escaped (warping the first char with `[]`) to be used as a real host name
+	// this mechanism is reversed for internal usage only (maybe for some rare cases), it's not supposed to be used by end users
+	// a real user should never use loopback/private/external as their host names
+	hl = ParseHostMatchList("", "loopback, [p]rivate")
+	cases = []tc{
+		{"loopback", nil, false},
+		{"", net.ParseIP("127.0.0.1"), true},
+		{"private", nil, true},
+		{"", net.ParseIP("192.168.1.1"), false},
+	}
+	test(cases)
+
+	hl = ParseSimpleMatchList("", "loopback, *.domain.com")
+	cases = []tc{
+		{"loopback", nil, true},
+		{"", net.ParseIP("127.0.0.1"), false},
+		{"sub.domain.com", nil, true},
+		{"other.com", nil, false},
+		{"", net.ParseIP("1.1.1.1"), false},
+	}
+	test(cases)
+
+	hl = ParseSimpleMatchList("", "external")
+	cases = []tc{
+		{"", net.ParseIP("192.168.1.1"), false},
+		{"", net.ParseIP("1.1.1.1"), false},
+		{"external", nil, true},
+	}
+	test(cases)
+
+	hl = ParseSimpleMatchList("", "")
+	cases = []tc{
+		{"", net.ParseIP("192.168.1.1"), false},
+		{"", net.ParseIP("1.1.1.1"), false},
+		{"external", nil, false},
 	}
+	test(cases)
 }