Skip to content

Commit 16a7d34

Browse files
bohdebohde
authored
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.
1 parent 68d9f36 commit 16a7d34

File tree

7 files changed

+302
-31
lines changed

7 files changed

+302
-31
lines changed

‎modules/util/truncate.go

+2
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,8 @@ func SplitStringAtByteN(input string, n int) (left, right string) {
4141

4242
// SplitTrimSpace splits the string at given separator and trims leading and trailing space
4343
func SplitTrimSpace(input, sep string) []string {
44+
// Trim initial leading & trailing space
45+
input = strings.TrimSpace(input)
4446
// replace CRLF with LF
4547
input = strings.ReplaceAll(input, "\r\n", "\n")
4648

‎modules/validation/binding.go

+33-4
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010

1111
"code.gitea.io/gitea/modules/auth"
1212
"code.gitea.io/gitea/modules/git"
13+
"code.gitea.io/gitea/modules/util"
1314

1415
"gitea.com/go-chi/binding"
1516
"github.com/gobwas/glob"
@@ -31,6 +32,7 @@ const (
3132
// AddBindingRules adds additional binding rules
3233
func AddBindingRules() {
3334
addGitRefNameBindingRule()
35+
addValidURLListBindingRule()
3436
addValidURLBindingRule()
3537
addValidSiteURLBindingRule()
3638
addGlobPatternRule()
@@ -44,7 +46,7 @@ func addGitRefNameBindingRule() {
4446
// Git refname validation rule
4547
binding.AddRule(&binding.Rule{
4648
IsMatch: func(rule string) bool {
47-
return strings.HasPrefix(rule, "GitRefName")
49+
return rule == "GitRefName"
4850
},
4951
IsValid: func(errs binding.Errors, name string, val any) (bool, binding.Errors) {
5052
str := fmt.Sprintf("%v", val)
@@ -58,11 +60,38 @@ func addGitRefNameBindingRule() {
5860
})
5961
}
6062

63+
func addValidURLListBindingRule() {
64+
// URL validation rule
65+
binding.AddRule(&binding.Rule{
66+
IsMatch: func(rule string) bool {
67+
return rule == "ValidUrlList"
68+
},
69+
IsValid: func(errs binding.Errors, name string, val any) (bool, binding.Errors) {
70+
str := fmt.Sprintf("%v", val)
71+
if len(str) == 0 {
72+
errs.Add([]string{name}, binding.ERR_URL, "Url")
73+
return false, errs
74+
}
75+
76+
ok := true
77+
urls := util.SplitTrimSpace(str, "\n")
78+
for _, u := range urls {
79+
if !IsValidURL(u) {
80+
ok = false
81+
errs.Add([]string{name}, binding.ERR_URL, u)
82+
}
83+
}
84+
85+
return ok, errs
86+
},
87+
})
88+
}
89+
6190
func addValidURLBindingRule() {
6291
// URL validation rule
6392
binding.AddRule(&binding.Rule{
6493
IsMatch: func(rule string) bool {
65-
return strings.HasPrefix(rule, "ValidUrl")
94+
return rule == "ValidUrl"
6695
},
6796
IsValid: func(errs binding.Errors, name string, val any) (bool, binding.Errors) {
6897
str := fmt.Sprintf("%v", val)
@@ -80,7 +109,7 @@ func addValidSiteURLBindingRule() {
80109
// URL validation rule
81110
binding.AddRule(&binding.Rule{
82111
IsMatch: func(rule string) bool {
83-
return strings.HasPrefix(rule, "ValidSiteUrl")
112+
return rule == "ValidSiteUrl"
84113
},
85114
IsValid: func(errs binding.Errors, name string, val any) (bool, binding.Errors) {
86115
str := fmt.Sprintf("%v", val)
@@ -171,7 +200,7 @@ func addUsernamePatternRule() {
171200
func addValidGroupTeamMapRule() {
172201
binding.AddRule(&binding.Rule{
173202
IsMatch: func(rule string) bool {
174-
return strings.HasPrefix(rule, "ValidGroupTeamMap")
203+
return rule == "ValidGroupTeamMap"
175204
},
176205
IsValid: func(errs binding.Errors, name string, val any) (bool, binding.Errors) {
177206
_, err := auth.UnmarshalGroupTeamMapping(fmt.Sprintf("%v", val))

‎modules/validation/binding_test.go

+1
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@ type (
2727
TestForm struct {
2828
BranchName string `form:"BranchName" binding:"GitRefName"`
2929
URL string `form:"ValidUrl" binding:"ValidUrl"`
30+
URLs string `form:"ValidUrls" binding:"ValidUrlList"`
3031
GlobPattern string `form:"GlobPattern" binding:"GlobPattern"`
3132
RegexPattern string `form:"RegexPattern" binding:"RegexPattern"`
3233
}

‎modules/validation/validurllist_test.go

+157
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,157 @@
1+
// Copyright 2024 The Gitea Authors. All rights reserved.
2+
// SPDX-License-Identifier: MIT
3+
4+
package validation
5+
6+
import (
7+
"testing"
8+
9+
"gitea.com/go-chi/binding"
10+
)
11+
12+
// This is a copy of all the URL tests cases, plus additional ones to
13+
// account for multiple URLs
14+
var urlListValidationTestCases = []validationTestCase{
15+
{
16+
description: "Empty URL",
17+
data: TestForm{
18+
URLs: "",
19+
},
20+
expectedErrors: binding.Errors{},
21+
},
22+
{
23+
description: "URL without port",
24+
data: TestForm{
25+
URLs: "http://test.lan/",
26+
},
27+
expectedErrors: binding.Errors{},
28+
},
29+
{
30+
description: "URL with port",
31+
data: TestForm{
32+
URLs: "http://test.lan:3000/",
33+
},
34+
expectedErrors: binding.Errors{},
35+
},
36+
{
37+
description: "URL with IPv6 address without port",
38+
data: TestForm{
39+
URLs: "http://[::1]/",
40+
},
41+
expectedErrors: binding.Errors{},
42+
},
43+
{
44+
description: "URL with IPv6 address with port",
45+
data: TestForm{
46+
URLs: "http://[::1]:3000/",
47+
},
48+
expectedErrors: binding.Errors{},
49+
},
50+
{
51+
description: "Invalid URL",
52+
data: TestForm{
53+
URLs: "http//test.lan/",
54+
},
55+
expectedErrors: binding.Errors{
56+
binding.Error{
57+
FieldNames: []string{"URLs"},
58+
Classification: binding.ERR_URL,
59+
Message: "http//test.lan/",
60+
},
61+
},
62+
},
63+
{
64+
description: "Invalid schema",
65+
data: TestForm{
66+
URLs: "ftp://test.lan/",
67+
},
68+
expectedErrors: binding.Errors{
69+
binding.Error{
70+
FieldNames: []string{"URLs"},
71+
Classification: binding.ERR_URL,
72+
Message: "ftp://test.lan/",
73+
},
74+
},
75+
},
76+
{
77+
description: "Invalid port",
78+
data: TestForm{
79+
URLs: "http://test.lan:3x4/",
80+
},
81+
expectedErrors: binding.Errors{
82+
binding.Error{
83+
FieldNames: []string{"URLs"},
84+
Classification: binding.ERR_URL,
85+
Message: "http://test.lan:3x4/",
86+
},
87+
},
88+
},
89+
{
90+
description: "Invalid port with IPv6 address",
91+
data: TestForm{
92+
URLs: "http://[::1]:3x4/",
93+
},
94+
expectedErrors: binding.Errors{
95+
binding.Error{
96+
FieldNames: []string{"URLs"},
97+
Classification: binding.ERR_URL,
98+
Message: "http://[::1]:3x4/",
99+
},
100+
},
101+
},
102+
{
103+
description: "Multi URLs",
104+
data: TestForm{
105+
URLs: "http://test.lan:3000/\nhttp://test.local/",
106+
},
107+
expectedErrors: binding.Errors{},
108+
},
109+
{
110+
description: "Multi URLs with newline",
111+
data: TestForm{
112+
URLs: "http://test.lan:3000/\nhttp://test.local/\n",
113+
},
114+
expectedErrors: binding.Errors{},
115+
},
116+
{
117+
description: "List with invalid entry",
118+
data: TestForm{
119+
URLs: "http://test.lan:3000/\nhttp://[::1]:3x4/",
120+
},
121+
expectedErrors: binding.Errors{
122+
binding.Error{
123+
FieldNames: []string{"URLs"},
124+
Classification: binding.ERR_URL,
125+
Message: "http://[::1]:3x4/",
126+
},
127+
},
128+
},
129+
{
130+
description: "List with two invalid entries",
131+
data: TestForm{
132+
URLs: "ftp://test.lan:3000/\nhttp://[::1]:3x4/\n",
133+
},
134+
expectedErrors: binding.Errors{
135+
binding.Error{
136+
FieldNames: []string{"URLs"},
137+
Classification: binding.ERR_URL,
138+
Message: "ftp://test.lan:3000/",
139+
},
140+
binding.Error{
141+
FieldNames: []string{"URLs"},
142+
Classification: binding.ERR_URL,
143+
Message: "http://[::1]:3x4/",
144+
},
145+
},
146+
},
147+
}
148+
149+
func Test_ValidURLListValidation(t *testing.T) {
150+
AddBindingRules()
151+
152+
for _, testCase := range urlListValidationTestCases {
153+
t.Run(testCase.description, func(t *testing.T) {
154+
performValidationTest(t, testCase)
155+
})
156+
}
157+
}

‎routers/web/user/setting/oauth2_common.go

+15-2
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,6 @@ func (oa *OAuth2CommonHandlers) AddApp(ctx *context.Context) {
4747
return
4848
}
4949

50-
// TODO validate redirect URI
5150
app, err := auth.CreateOAuth2Application(ctx, auth.CreateOAuth2ApplicationOptions{
5251
Name: form.Name,
5352
RedirectURIs: util.SplitTrimSpace(form.RedirectURIs, "\n"),
@@ -96,11 +95,25 @@ func (oa *OAuth2CommonHandlers) EditSave(ctx *context.Context) {
9695
form := web.GetForm(ctx).(*forms.EditOAuth2ApplicationForm)
9796

9897
if ctx.HasError() {
98+
app, err := auth.GetOAuth2ApplicationByID(ctx, ctx.PathParamInt64("id"))
99+
if err != nil {
100+
if auth.IsErrOAuthApplicationNotFound(err) {
101+
ctx.NotFound("Application not found", err)
102+
return
103+
}
104+
ctx.ServerError("GetOAuth2ApplicationByID", err)
105+
return
106+
}
107+
if app.UID != oa.OwnerID {
108+
ctx.NotFound("Application not found", nil)
109+
return
110+
}
111+
ctx.Data["App"] = app
112+
99113
oa.renderEditPage(ctx)
100114
return
101115
}
102116

103-
// TODO validate redirect URI
104117
var err error
105118
if ctx.Data["App"], err = auth.UpdateOAuth2Application(ctx, auth.UpdateOAuth2ApplicationOptions{
106119
ID: ctx.PathParamInt64("id"),

‎services/forms/user_form.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -366,7 +366,7 @@ func (f *NewAccessTokenForm) GetScope() (auth_model.AccessTokenScope, error) {
366366
// EditOAuth2ApplicationForm form for editing oauth2 applications
367367
type EditOAuth2ApplicationForm struct {
368368
Name string `binding:"Required;MaxSize(255)" form:"application_name"`
369-
RedirectURIs string `binding:"Required" form:"redirect_uris"`
369+
RedirectURIs string `binding:"Required;ValidUrlList" form:"redirect_uris"`
370370
ConfidentialClient bool `form:"confidential_client"`
371371
SkipSecondaryAuthorization bool `form:"skip_secondary_authorization"`
372372
}

0 commit comments

Comments
 (0)