Validate OAuth Redirect URIs (#32643)

This fixes a TODO in the code to validate the RedirectURIs when adding
or editing an OAuth application in user settings.

This also includes a refactor of the user settings tests to only create
the DB once per top-level test to avoid reloading fixtures.

modules/util/truncate.go

@@ -41,6 +41,8 @@ func SplitStringAtByteN(input string, n int) (left, right string) {
 
 // SplitTrimSpace splits the string at given separator and trims leading and trailing space
 func SplitTrimSpace(input, sep string) []string {
+	// Trim initial leading & trailing space
+	input = strings.TrimSpace(input)
 	// replace CRLF with LF
 	input = strings.ReplaceAll(input, "\r\n", "\n")
 

modules/validation/binding.go

@@ -10,6 +10,7 @@ import (
 
 	"code.gitea.io/gitea/modules/auth"
 	"code.gitea.io/gitea/modules/git"
+	"code.gitea.io/gitea/modules/util"
 
 	"gitea.com/go-chi/binding"
 	"github.com/gobwas/glob"
@@ -31,6 +32,7 @@ const (
 // AddBindingRules adds additional binding rules
 func AddBindingRules() {
 	addGitRefNameBindingRule()
+	addValidURLListBindingRule()
 	addValidURLBindingRule()
 	addValidSiteURLBindingRule()
 	addGlobPatternRule()
@@ -44,7 +46,7 @@ func addGitRefNameBindingRule() {
 	// Git refname validation rule
 	binding.AddRule(&binding.Rule{
 		IsMatch: func(rule string) bool {
-			return strings.HasPrefix(rule, "GitRefName")
+			return rule == "GitRefName"
 		},
 		IsValid: func(errs binding.Errors, name string, val any) (bool, binding.Errors) {
 			str := fmt.Sprintf("%v", val)
@@ -58,11 +60,38 @@ func addGitRefNameBindingRule() {
 	})
 }
 
+func addValidURLListBindingRule() {
+	// URL validation rule
+	binding.AddRule(&binding.Rule{
+		IsMatch: func(rule string) bool {
+			return rule == "ValidUrlList"
+		},
+		IsValid: func(errs binding.Errors, name string, val any) (bool, binding.Errors) {
+			str := fmt.Sprintf("%v", val)
+			if len(str) == 0 {
+				errs.Add([]string{name}, binding.ERR_URL, "Url")
+				return false, errs
+			}
+
+			ok := true
+			urls := util.SplitTrimSpace(str, "\n")
+			for _, u := range urls {
+				if !IsValidURL(u) {
+					ok = false
+					errs.Add([]string{name}, binding.ERR_URL, u)
+				}
+			}
+
+			return ok, errs
+		},
+	})
+}
+
 func addValidURLBindingRule() {
 	// URL validation rule
 	binding.AddRule(&binding.Rule{
 		IsMatch: func(rule string) bool {
-			return strings.HasPrefix(rule, "ValidUrl")
+			return rule == "ValidUrl"
 		},
 		IsValid: func(errs binding.Errors, name string, val any) (bool, binding.Errors) {
 			str := fmt.Sprintf("%v", val)
@@ -80,7 +109,7 @@ func addValidSiteURLBindingRule() {
 	// URL validation rule
 	binding.AddRule(&binding.Rule{
 		IsMatch: func(rule string) bool {
-			return strings.HasPrefix(rule, "ValidSiteUrl")
+			return rule == "ValidSiteUrl"
 		},
 		IsValid: func(errs binding.Errors, name string, val any) (bool, binding.Errors) {
 			str := fmt.Sprintf("%v", val)
@@ -171,7 +200,7 @@ func addUsernamePatternRule() {
 func addValidGroupTeamMapRule() {
 	binding.AddRule(&binding.Rule{
 		IsMatch: func(rule string) bool {
-			return strings.HasPrefix(rule, "ValidGroupTeamMap")
+			return rule == "ValidGroupTeamMap"
 		},
 		IsValid: func(errs binding.Errors, name string, val any) (bool, binding.Errors) {
 			_, err := auth.UnmarshalGroupTeamMapping(fmt.Sprintf("%v", val))

modules/validation/binding_test.go

@@ -27,6 +27,7 @@ type (
 	TestForm struct {
 		BranchName   string `form:"BranchName" binding:"GitRefName"`
 		URL          string `form:"ValidUrl" binding:"ValidUrl"`
+		URLs         string `form:"ValidUrls" binding:"ValidUrlList"`
 		GlobPattern  string `form:"GlobPattern" binding:"GlobPattern"`
 		RegexPattern string `form:"RegexPattern" binding:"RegexPattern"`
 	}

modules/validation/validurllist_test.go

@@ -0,0 +1,157 @@
+// Copyright 2024 The Gitea Authors. All rights reserved.
+// SPDX-License-Identifier: MIT
+
+package validation
+
+import (
+	"testing"
+
+	"gitea.com/go-chi/binding"
+)
+
+// This is a copy of all the URL tests cases, plus additional ones to
+// account for multiple URLs
+var urlListValidationTestCases = []validationTestCase{
+	{
+		description: "Empty URL",
+		data: TestForm{
+			URLs: "",
+		},
+		expectedErrors: binding.Errors{},
+	},
+	{
+		description: "URL without port",
+		data: TestForm{
+			URLs: "http://test.lan/",
+		},
+		expectedErrors: binding.Errors{},
+	},
+	{
+		description: "URL with port",
+		data: TestForm{
+			URLs: "http://test.lan:3000/",
+		},
+		expectedErrors: binding.Errors{},
+	},
+	{
+		description: "URL with IPv6 address without port",
+		data: TestForm{
+			URLs: "http://[::1]/",
+		},
+		expectedErrors: binding.Errors{},
+	},
+	{
+		description: "URL with IPv6 address with port",
+		data: TestForm{
+			URLs: "http://[::1]:3000/",
+		},
+		expectedErrors: binding.Errors{},
+	},
+	{
+		description: "Invalid URL",
+		data: TestForm{
+			URLs: "http//test.lan/",
+		},
+		expectedErrors: binding.Errors{
+			binding.Error{
+				FieldNames:     []string{"URLs"},
+				Classification: binding.ERR_URL,
+				Message:        "http//test.lan/",
+			},
+		},
+	},
+	{
+		description: "Invalid schema",
+		data: TestForm{
+			URLs: "ftp://test.lan/",
+		},
+		expectedErrors: binding.Errors{
+			binding.Error{
+				FieldNames:     []string{"URLs"},
+				Classification: binding.ERR_URL,
+				Message:        "ftp://test.lan/",
+			},
+		},
+	},
+	{
+		description: "Invalid port",
+		data: TestForm{
+			URLs: "http://test.lan:3x4/",
+		},
+		expectedErrors: binding.Errors{
+			binding.Error{
+				FieldNames:     []string{"URLs"},
+				Classification: binding.ERR_URL,
+				Message:        "http://test.lan:3x4/",
+			},
+		},
+	},
+	{
+		description: "Invalid port with IPv6 address",
+		data: TestForm{
+			URLs: "http://[::1]:3x4/",
+		},
+		expectedErrors: binding.Errors{
+			binding.Error{
+				FieldNames:     []string{"URLs"},
+				Classification: binding.ERR_URL,
+				Message:        "http://[::1]:3x4/",
+			},
+		},
+	},
+	{
+		description: "Multi URLs",
+		data: TestForm{
+			URLs: "http://test.lan:3000/\nhttp://test.local/",
+		},
+		expectedErrors: binding.Errors{},
+	},
+	{
+		description: "Multi URLs with newline",
+		data: TestForm{
+			URLs: "http://test.lan:3000/\nhttp://test.local/\n",
+		},
+		expectedErrors: binding.Errors{},
+	},
+	{
+		description: "List with invalid entry",
+		data: TestForm{
+			URLs: "http://test.lan:3000/\nhttp://[::1]:3x4/",
+		},
+		expectedErrors: binding.Errors{
+			binding.Error{
+				FieldNames:     []string{"URLs"},
+				Classification: binding.ERR_URL,
+				Message:        "http://[::1]:3x4/",
+			},
+		},
+	},
+	{
+		description: "List with two invalid entries",
+		data: TestForm{
+			URLs: "ftp://test.lan:3000/\nhttp://[::1]:3x4/\n",
+		},
+		expectedErrors: binding.Errors{
+			binding.Error{
+				FieldNames:     []string{"URLs"},
+				Classification: binding.ERR_URL,
+				Message:        "ftp://test.lan:3000/",
+			},
+			binding.Error{
+				FieldNames:     []string{"URLs"},
+				Classification: binding.ERR_URL,
+				Message:        "http://[::1]:3x4/",
+			},
+		},
+	},
+}
+
+func Test_ValidURLListValidation(t *testing.T) {
+	AddBindingRules()
+
+	for _, testCase := range urlListValidationTestCases {
+		t.Run(testCase.description, func(t *testing.T) {
+			performValidationTest(t, testCase)
+		})
+	}
+}

routers/web/user/setting/oauth2_common.go

@@ -47,7 +47,6 @@ func (oa *OAuth2CommonHandlers) AddApp(ctx *context.Context) {
 		return
 	}
 
-	// TODO validate redirect URI
 	app, err := auth.CreateOAuth2Application(ctx, auth.CreateOAuth2ApplicationOptions{
 		Name:                       form.Name,
 		RedirectURIs:               util.SplitTrimSpace(form.RedirectURIs, "\n"),
@@ -96,11 +95,25 @@ func (oa *OAuth2CommonHandlers) EditSave(ctx *context.Context) {
 	form := web.GetForm(ctx).(*forms.EditOAuth2ApplicationForm)
 
 	if ctx.HasError() {
+		app, err := auth.GetOAuth2ApplicationByID(ctx, ctx.PathParamInt64("id"))
+		if err != nil {
+			if auth.IsErrOAuthApplicationNotFound(err) {
+				ctx.NotFound("Application not found", err)
+				return
+			}
+			ctx.ServerError("GetOAuth2ApplicationByID", err)
+			return
+		}
+		if app.UID != oa.OwnerID {
+			ctx.NotFound("Application not found", nil)
+			return
+		}
+		ctx.Data["App"] = app
+
 		oa.renderEditPage(ctx)
 		return
 	}
 
-	// TODO validate redirect URI
 	var err error
 	if ctx.Data["App"], err = auth.UpdateOAuth2Application(ctx, auth.UpdateOAuth2ApplicationOptions{
 		ID:                         ctx.PathParamInt64("id"),

services/forms/user_form.go

@@ -366,7 +366,7 @@ func (f *NewAccessTokenForm) GetScope() (auth_model.AccessTokenScope, error) {
 // EditOAuth2ApplicationForm form for editing oauth2 applications
 type EditOAuth2ApplicationForm struct {
 	Name                       string `binding:"Required;MaxSize(255)" form:"application_name"`
-	RedirectURIs               string `binding:"Required" form:"redirect_uris"`
+	RedirectURIs               string `binding:"Required;ValidUrlList" form:"redirect_uris"`
 	ConfidentialClient         bool   `form:"confidential_client"`
 	SkipSecondaryAuthorization bool   `form:"skip_secondary_authorization"`
 }

tests/integration/user_settings_test.go

@@ -10,6 +10,8 @@ import (
 	"code.gitea.io/gitea/modules/container"
 	"code.gitea.io/gitea/modules/setting"
 	"code.gitea.io/gitea/tests"
+
+	"github.com/stretchr/testify/assert"
 )
 
 // Validate that each navbar setting is correct. This checks that the
@@ -51,9 +53,11 @@ func WithDisabledFeatures(t *testing.T, features ...string) {
 }
 
 func TestUserSettingsAccount(t *testing.T) {
-	t.Run("all features enabled", func(t *testing.T) {
 	defer tests.PrepareTestEnv(t)()
 
+	t.Run("all features enabled", func(t *testing.T) {
+		defer tests.PrintCurrentTest(t)()
+
 		session := loginUser(t, "user2")
 		req := NewRequest(t, "GET", "/user/settings/account")
 		resp := session.MakeRequest(t, req, http.StatusOK)
@@ -68,7 +72,7 @@ func TestUserSettingsAccount(t *testing.T) {
 	})
 
 	t.Run("credentials disabled", func(t *testing.T) {
-		defer tests.PrepareTestEnv(t)()
+		defer tests.PrintCurrentTest(t)()
 
 		WithDisabledFeatures(t, setting.UserFeatureManageCredentials)
 
@@ -85,7 +89,7 @@ func TestUserSettingsAccount(t *testing.T) {
 	})
 
 	t.Run("deletion disabled", func(t *testing.T) {
-		defer tests.PrepareTestEnv(t)()
+		defer tests.PrintCurrentTest(t)()
 
 		WithDisabledFeatures(t, setting.UserFeatureDeletion)
 
@@ -102,7 +106,7 @@ func TestUserSettingsAccount(t *testing.T) {
 	})
 
 	t.Run("deletion, credentials and email notifications are disabled", func(t *testing.T) {
-		defer tests.PrepareTestEnv(t)()
+		defer tests.PrintCurrentTest(t)()
 
 		mail := setting.Service.EnableNotifyMail
 		setting.Service.EnableNotifyMail = false
@@ -119,9 +123,11 @@ func TestUserSettingsAccount(t *testing.T) {
 }
 
 func TestUserSettingsUpdatePassword(t *testing.T) {
-	t.Run("enabled", func(t *testing.T) {
 	defer tests.PrepareTestEnv(t)()
 
+	t.Run("enabled", func(t *testing.T) {
+		defer tests.PrintCurrentTest(t)()
+
 		session := loginUser(t, "user2")
 
 		req := NewRequest(t, "GET", "/user/settings/account")
@@ -138,7 +144,7 @@ func TestUserSettingsUpdatePassword(t *testing.T) {
 	})
 
 	t.Run("credentials disabled", func(t *testing.T) {
-		defer tests.PrepareTestEnv(t)()
+		defer tests.PrintCurrentTest(t)()
 
 		WithDisabledFeatures(t, setting.UserFeatureManageCredentials)
 
@@ -156,9 +162,11 @@ func TestUserSettingsUpdatePassword(t *testing.T) {
 }
 
 func TestUserSettingsUpdateEmail(t *testing.T) {
-	t.Run("credentials disabled", func(t *testing.T) {
 	defer tests.PrepareTestEnv(t)()
 
+	t.Run("credentials disabled", func(t *testing.T) {
+		defer tests.PrintCurrentTest(t)()
+
 		WithDisabledFeatures(t, setting.UserFeatureManageCredentials)
 
 		session := loginUser(t, "user2")
@@ -175,9 +183,11 @@ func TestUserSettingsUpdateEmail(t *testing.T) {
 }
 
 func TestUserSettingsDeleteEmail(t *testing.T) {
-	t.Run("credentials disabled", func(t *testing.T) {
 	defer tests.PrepareTestEnv(t)()
 
+	t.Run("credentials disabled", func(t *testing.T) {
+		defer tests.PrintCurrentTest(t)()
+
 		WithDisabledFeatures(t, setting.UserFeatureManageCredentials)
 
 		session := loginUser(t, "user2")
@@ -194,9 +204,11 @@ func TestUserSettingsDeleteEmail(t *testing.T) {
 }
 
 func TestUserSettingsDelete(t *testing.T) {
-	t.Run("deletion disabled", func(t *testing.T) {
 	defer tests.PrepareTestEnv(t)()
 
+	t.Run("deletion disabled", func(t *testing.T) {
+		defer tests.PrintCurrentTest(t)()
+
 		WithDisabledFeatures(t, setting.UserFeatureDeletion)
 
 		session := loginUser(t, "user2")
@@ -224,9 +236,10 @@ func TestUserSettingsAppearance(t *testing.T) {
 }
 
 func TestUserSettingsSecurity(t *testing.T) {
-	t.Run("credentials disabled", func(t *testing.T) {
 	defer tests.PrepareTestEnv(t)()
 
+	t.Run("credentials disabled", func(t *testing.T) {
+		defer tests.PrintCurrentTest(t)()
 		WithDisabledFeatures(t, setting.UserFeatureManageCredentials)
 
 		session := loginUser(t, "user2")
@@ -240,8 +253,7 @@ func TestUserSettingsSecurity(t *testing.T) {
 	})
 
 	t.Run("mfa disabled", func(t *testing.T) {
-		defer tests.PrepareTestEnv(t)()
+		defer tests.PrintCurrentTest(t)()
-
 		WithDisabledFeatures(t, setting.UserFeatureManageMFA)
 
 		session := loginUser(t, "user2")
@@ -255,8 +267,7 @@ func TestUserSettingsSecurity(t *testing.T) {
 	})
 
 	t.Run("credentials and mfa disabled", func(t *testing.T) {
-		defer tests.PrepareTestEnv(t)()
+		defer tests.PrintCurrentTest(t)()
-
 		WithDisabledFeatures(t, setting.UserFeatureManageCredentials, setting.UserFeatureManageMFA)
 
 		session := loginUser(t, "user2")
@@ -268,18 +279,76 @@ func TestUserSettingsSecurity(t *testing.T) {
 func TestUserSettingsApplications(t *testing.T) {
 	defer tests.PrepareTestEnv(t)()
 
+	t.Run("Applications", func(t *testing.T) {
+		defer tests.PrintCurrentTest(t)()
+
 		session := loginUser(t, "user2")
 		req := NewRequest(t, "GET", "/user/settings/applications")
 		resp := session.MakeRequest(t, req, http.StatusOK)
 		doc := NewHTMLParser(t, resp.Body)
 
 		assertNavbar(t, doc)
+	})
+
+	t.Run("OAuth2", func(t *testing.T) {
+		defer tests.PrintCurrentTest(t)()
+
+		session := loginUser(t, "user2")
+
+		t.Run("OAuth2ApplicationShow", func(t *testing.T) {
+			defer tests.PrintCurrentTest(t)()
+
+			req := NewRequest(t, "GET", "/user/settings/applications/oauth2/2")
+			resp := session.MakeRequest(t, req, http.StatusOK)
+			doc := NewHTMLParser(t, resp.Body)
+
+			assertNavbar(t, doc)
+		})
+
+		t.Run("OAuthApplicationsEdit", func(t *testing.T) {
+			defer tests.PrintCurrentTest(t)()
+
+			req := NewRequest(t, "GET", "/user/settings/applications/oauth2/2")
+			resp := session.MakeRequest(t, req, http.StatusOK)
+			doc := NewHTMLParser(t, resp.Body)
+
+			t.Run("Invalid URL", func(t *testing.T) {
+				defer tests.PrintCurrentTest(t)()
+
+				req := NewRequestWithValues(t, "POST", "/user/settings/applications/oauth2/2", map[string]string{
+					"_csrf":               doc.GetCSRF(),
+					"application_name":    "Test native app",
+					"redirect_uris":       "ftp://127.0.0.1",
+					"confidential_client": "false",
+				})
+				resp := session.MakeRequest(t, req, http.StatusOK)
+				doc := NewHTMLParser(t, resp.Body)
+
+				msg := doc.Find(".flash-error p").Text()
+				assert.Equal(t, `form.RedirectURIs"ftp://127.0.0.1" is not a valid URL.`, msg)
+			})
+
+			t.Run("OK", func(t *testing.T) {
+				defer tests.PrintCurrentTest(t)()
+
+				req := NewRequestWithValues(t, "POST", "/user/settings/applications/oauth2/2", map[string]string{
+					"_csrf":               doc.GetCSRF(),
+					"application_name":    "Test native app",
+					"redirect_uris":       "http://127.0.0.1",
+					"confidential_client": "false",
+				})
+				session.MakeRequest(t, req, http.StatusSeeOther)
+			})
+		})
+	})
 }
 
 func TestUserSettingsKeys(t *testing.T) {
-	t.Run("all enabled", func(t *testing.T) {
 	defer tests.PrepareTestEnv(t)()
 
+	t.Run("all enabled", func(t *testing.T) {
+		defer tests.PrintCurrentTest(t)()
+
 		session := loginUser(t, "user2")
 		req := NewRequest(t, "GET", "/user/settings/keys")
 		resp := session.MakeRequest(t, req, http.StatusOK)
@@ -292,7 +361,7 @@ func TestUserSettingsKeys(t *testing.T) {
 	})
 
 	t.Run("ssh keys disabled", func(t *testing.T) {
-		defer tests.PrepareTestEnv(t)()
+		defer tests.PrintCurrentTest(t)()
 
 		WithDisabledFeatures(t, setting.UserFeatureManageSSHKeys)
 
@@ -308,7 +377,7 @@ func TestUserSettingsKeys(t *testing.T) {
 	})
 
 	t.Run("gpg keys disabled", func(t *testing.T) {
-		defer tests.PrepareTestEnv(t)()
+		defer tests.PrintCurrentTest(t)()
 
 		WithDisabledFeatures(t, setting.UserFeatureManageGPGKeys)
 
@@ -324,7 +393,7 @@ func TestUserSettingsKeys(t *testing.T) {
 	})
 
 	t.Run("ssh & gpg keys disabled", func(t *testing.T) {
-		defer tests.PrepareTestEnv(t)()
+		defer tests.PrintCurrentTest(t)()
 
 		WithDisabledFeatures(t, setting.UserFeatureManageSSHKeys, setting.UserFeatureManageGPGKeys)