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. (cherry picked from commit 16a7d343d78807e39df124756e5d43a69a2203a3) Conflicts: services/forms/user_form.go tests/integration/user_settings_test.go simple conflicts
This commit is contained in:
Rowan Bohde
Earl Warren
parent
3973f1022d
commit
2e00ae4cdd
6 changed files with 209 additions and 7 deletions
|
|
@ -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")
|
||||
|
||||
|
|
|
|||
|
|
@ -10,6 +10,7 @@ import (
|
|||
|
||||
"code.gitea.io/gitea/modules/auth"
|
||||
"code.gitea.io/gitea/modules/git"
|
||||
"code.gitea.io/gitea/modules/util"
|
||||
|
||||
"code.forgejo.org/go-chi/binding"
|
||||
"github.com/gobwas/glob"
|
||||
|
|
@ -33,6 +34,7 @@ const (
|
|||
// AddBindingRules adds additional binding rules
|
||||
func AddBindingRules() {
|
||||
addGitRefNameBindingRule()
|
||||
addValidURLListBindingRule()
|
||||
addValidURLBindingRule()
|
||||
addValidSiteURLBindingRule()
|
||||
addGlobPatternRule()
|
||||
|
|
@ -47,7 +49,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)
|
||||
|
|
@ -61,11 +63,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)
|
||||
|
|
@ -83,7 +112,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)
|
||||
|
|
@ -174,7 +203,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))
|
||||
|
|
|
|||
|
|
@ -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"`
|
||||
}
|
||||
|
|
|
|||
157
modules/validation/validurllist_test.go
Normal file
157
modules/validation/validurllist_test.go
Normal file
|
|
@ -0,0 +1,157 @@
|
|||
// Copyright 2024 The Gitea Authors. All rights reserved.
|
||||
// SPDX-License-Identifier: MIT
|
||||
|
||||
package validation
|
||||
|
||||
import (
|
||||
"testing"
|
||||
|
||||
"code.forgejo.org/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)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
|
@ -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"),
|
||||
|
|
@ -95,11 +94,25 @@ func (oa *OAuth2CommonHandlers) EditSave(ctx *context.Context) {
|
|||
form := web.GetForm(ctx).(*forms.EditOAuth2ApplicationForm)
|
||||
|
||||
if ctx.HasError() {
|
||||
app, err := auth.GetOAuth2ApplicationByID(ctx, ctx.ParamsInt64("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.ParamsInt64("id"),
|
||||
|
|
|
|||
|
|
@ -388,7 +388,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"`
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue