From 16a7d343d78807e39df124756e5d43a69a2203a3 Mon Sep 17 00:00:00 2001
From: Rowan Bohde <rowan.bohde@gmail.com>
Date: Wed, 27 Nov 2024 20:50:27 -0600
Subject: [PATCH] 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 | 2 +
modules/validation/binding.go | 37 ++++-
modules/validation/binding_test.go | 1 +
modules/validation/validurllist_test.go | 157 ++++++++++++++++++++++
routers/web/user/setting/oauth2_common.go | 17 ++-
services/forms/user_form.go | 2 +-
tests/integration/user_settings_test.go | 117 ++++++++++++----
7 files changed, 302 insertions(+), 31 deletions(-)
create mode 100644 modules/validation/validurllist_test.go
diff --git a/modules/util/truncate.go b/modules/util/truncate.go
index 77b116eeff..f2edbdc673 100644
--- a/modules/util/truncate.go
+++ b/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")
diff --git a/modules/validation/binding.go b/modules/validation/binding.go
index cb0a5063e5..75190e31e1 100644
--- a/modules/validation/binding.go
+++ b/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))
diff --git a/modules/validation/binding_test.go b/modules/validation/binding_test.go
index 01ff4e3435..28d0f57b5c 100644
--- a/modules/validation/binding_test.go
+++ b/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"`
}
diff --git a/modules/validation/validurllist_test.go b/modules/validation/validurllist_test.go
new file mode 100644
index 0000000000..c6f862a962
--- /dev/null
+++ b/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)
+ })
+ }
+}
diff --git a/routers/web/user/setting/oauth2_common.go b/routers/web/user/setting/oauth2_common.go
index 6029d7bedb..e93e9e1954 100644
--- a/routers/web/user/setting/oauth2_common.go
+++ b/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"),
diff --git a/services/forms/user_form.go b/services/forms/user_form.go
index 5b7a43642a..ed79936add 100644
--- a/services/forms/user_form.go
+++ b/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"`
}
diff --git a/tests/integration/user_settings_test.go b/tests/integration/user_settings_test.go
index 2103c92d58..d8402eb25f 100644
--- a/tests/integration/user_settings_test.go
+++ b/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,8 +53,10 @@ func WithDisabledFeatures(t *testing.T, features ...string) {
}
func TestUserSettingsAccount(t *testing.T) {
+ defer tests.PrepareTestEnv(t)()
+
t.Run("all features enabled", func(t *testing.T) {
- defer tests.PrepareTestEnv(t)()
+ defer tests.PrintCurrentTest(t)()
session := loginUser(t, "user2")
req := NewRequest(t, "GET", "/user/settings/account")
@@ -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,8 +123,10 @@ func TestUserSettingsAccount(t *testing.T) {
}
func TestUserSettingsUpdatePassword(t *testing.T) {
+ defer tests.PrepareTestEnv(t)()
+
t.Run("enabled", func(t *testing.T) {
- defer tests.PrepareTestEnv(t)()
+ defer tests.PrintCurrentTest(t)()
session := loginUser(t, "user2")
@@ -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,8 +162,10 @@ func TestUserSettingsUpdatePassword(t *testing.T) {
}
func TestUserSettingsUpdateEmail(t *testing.T) {
+ defer tests.PrepareTestEnv(t)()
+
t.Run("credentials disabled", func(t *testing.T) {
- defer tests.PrepareTestEnv(t)()
+ defer tests.PrintCurrentTest(t)()
WithDisabledFeatures(t, setting.UserFeatureManageCredentials)
@@ -175,8 +183,10 @@ func TestUserSettingsUpdateEmail(t *testing.T) {
}
func TestUserSettingsDeleteEmail(t *testing.T) {
+ defer tests.PrepareTestEnv(t)()
+
t.Run("credentials disabled", func(t *testing.T) {
- defer tests.PrepareTestEnv(t)()
+ defer tests.PrintCurrentTest(t)()
WithDisabledFeatures(t, setting.UserFeatureManageCredentials)
@@ -194,8 +204,10 @@ func TestUserSettingsDeleteEmail(t *testing.T) {
}
func TestUserSettingsDelete(t *testing.T) {
+ defer tests.PrepareTestEnv(t)()
+
t.Run("deletion disabled", func(t *testing.T) {
- defer tests.PrepareTestEnv(t)()
+ defer tests.PrintCurrentTest(t)()
WithDisabledFeatures(t, setting.UserFeatureDeletion)
@@ -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)()
+ 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,17 +279,75 @@ func TestUserSettingsSecurity(t *testing.T) {
func TestUserSettingsApplications(t *testing.T) {
defer tests.PrepareTestEnv(t)()
- session := loginUser(t, "user2")
- req := NewRequest(t, "GET", "/user/settings/applications")
- resp := session.MakeRequest(t, req, http.StatusOK)
- doc := NewHTMLParser(t, resp.Body)
+ t.Run("Applications", func(t *testing.T) {
+ defer tests.PrintCurrentTest(t)()
- assertNavbar(t, doc)
+ 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) {
+ defer tests.PrepareTestEnv(t)()
+
t.Run("all enabled", func(t *testing.T) {
- defer tests.PrepareTestEnv(t)()
+ defer tests.PrintCurrentTest(t)()
session := loginUser(t, "user2")
req := NewRequest(t, "GET", "/user/settings/keys")
@@ -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)