Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate OAuth Redirect URIs #32643

Merged
merged 12 commits into from
Nov 28, 2024
Merged

Validate OAuth Redirect URIs #32643

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
a814efc
fix: validate RedirectURIs when saving an Oauth2 Application
bohde Aug 14, 2024
6ce43ab
test: avoid loading fixtures in user settings tests
bohde Aug 15, 2024
6c2ae7f
test: add tests to verify that invalid redirects result in an error m…
bohde Aug 15, 2024
3fbfdb2
fix: remove TODO
bohde Aug 15, 2024
1bdfb94
fix: update for main
bohde Nov 25, 2024
ce4145f
test: add missing PrintCurrentTest
bohde Nov 25, 2024
57958d2
fix wrong binding declaration
bohde Nov 26, 2024
e536a9d
add unit tests and fix failing cases
bohde Nov 26, 2024
17b4c93
Merge branch 'main' into rb/validate-redirect-uri
lunny Nov 27, 2024
2861557
standardize on using == for checking validation rules
bohde Nov 27, 2024
3344494
Merge branch 'main' into rb/validate-redirect-uri
GiteaBot Nov 28, 2024
24f5331
Merge branch 'main' into rb/validate-redirect-uri
GiteaBot Nov 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 29 additions & 0 deletions modules/validation/binding.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -31,6 +32,7 @@ const (
// AddBindingRules adds additional binding rules
func AddBindingRules() {
addGitRefNameBindingRule()
addValidURLListBindingRule()
addValidURLBindingRule()
addValidSiteURLBindingRule()
addGlobPatternRule()
Expand Down Expand Up @@ -58,6 +60,33 @@ func addGitRefNameBindingRule() {
})
}

func addValidURLListBindingRule() {
// URL validation rule
binding.AddRule(&binding.Rule{
IsMatch: func(rule string) bool {
return strings.HasPrefix(rule, "ValidUrlList")
lunny marked this conversation as resolved.
Show resolved Hide resolved
},
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) {
wxiaoguang marked this conversation as resolved.
Show resolved Hide resolved
ok = false
errs.Add([]string{"RedirectURIs"}, binding.ERR_URL, u)
}
}

return ok, errs
},
})
}
lunny marked this conversation as resolved.
Show resolved Hide resolved

func addValidURLBindingRule() {
// URL validation rule
binding.AddRule(&binding.Rule{
Expand Down
17 changes: 15 additions & 2 deletions routers/web/user/setting/oauth2_common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"),
Expand Down Expand Up @@ -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"),
Expand Down
2 changes: 1 addition & 1 deletion services/forms/user_form.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"`
}
Expand Down
117 changes: 93 additions & 24 deletions tests/integration/user_settings_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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")
Expand All @@ -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)

Expand All @@ -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)

Expand All @@ -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
Expand All @@ -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")

Expand All @@ -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)

Expand All @@ -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)

Expand All @@ -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)

Expand All @@ -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)

Expand Down Expand Up @@ -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")
Expand All @@ -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")
Expand All @@ -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")
Expand All @@ -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")
Expand All @@ -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)

Expand All @@ -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)

Expand All @@ -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)

Expand Down
Loading