From f42ec6120e8a2830407027020b65391ebf8e7f59 Mon Sep 17 00:00:00 2001 From: Lauris BH Date: Wed, 19 Apr 2017 06:02:20 +0300 Subject: [PATCH] Better URL validation (#1507) * Add correct git branch name validation * Change git refname validation error constant name * Implement URL validation based on GoLang url.Parse method * Backward compatibility with older Go compiler * Add git reference name validation unit tests * Remove unused variable in unit test * Implement URL validation based on GoLang url.Parse method * Backward compatibility with older Go compiler * Add url validation unit tests --- cmd/web.go | 2 + modules/auth/admin.go | 2 +- modules/auth/auth.go | 3 + modules/auth/org.go | 2 +- modules/auth/repo_form.go | 12 +-- modules/auth/user_form.go | 2 +- modules/validation/binding.go | 102 ++++++++++++++++++++ modules/validation/binding_test.go | 62 ++++++++++++ modules/validation/refname_test.go | 142 ++++++++++++++++++++++++++++ modules/validation/validurl_test.go | 111 ++++++++++++++++++++++ options/locale/locale_en-US.ini | 1 + 11 files changed, 432 insertions(+), 9 deletions(-) create mode 100644 modules/validation/binding.go create mode 100644 modules/validation/binding_test.go create mode 100644 modules/validation/refname_test.go create mode 100644 modules/validation/validurl_test.go diff --git a/cmd/web.go b/cmd/web.go index b2cc3959a..411b50d9b 100644 --- a/cmd/web.go +++ b/cmd/web.go @@ -23,6 +23,7 @@ import ( "code.gitea.io/gitea/modules/public" "code.gitea.io/gitea/modules/setting" "code.gitea.io/gitea/modules/templates" + "code.gitea.io/gitea/modules/validation" "code.gitea.io/gitea/routers" "code.gitea.io/gitea/routers/admin" apiv1 "code.gitea.io/gitea/routers/api/v1" @@ -177,6 +178,7 @@ func runWeb(ctx *cli.Context) error { reqSignOut := context.Toggle(&context.ToggleOptions{SignOutRequired: true}) bindIgnErr := binding.BindIgnErr + validation.AddBindingRules() m.Use(user.GetNotificationCount) diff --git a/modules/auth/admin.go b/modules/auth/admin.go index 68bc4f394..0bb7d355c 100644 --- a/modules/auth/admin.go +++ b/modules/auth/admin.go @@ -32,7 +32,7 @@ type AdminEditUserForm struct { FullName string `binding:"MaxSize(100)"` Email string `binding:"Required;Email;MaxSize(254)"` Password string `binding:"MaxSize(255)"` - Website string `binding:"Url;MaxSize(255)"` + Website string `binding:"ValidUrl;MaxSize(255)"` Location string `binding:"MaxSize(50)"` MaxRepoCreation int Active bool diff --git a/modules/auth/auth.go b/modules/auth/auth.go index 33ba77796..89b3e3850 100644 --- a/modules/auth/auth.go +++ b/modules/auth/auth.go @@ -19,6 +19,7 @@ import ( "code.gitea.io/gitea/modules/base" "code.gitea.io/gitea/modules/log" "code.gitea.io/gitea/modules/setting" + "code.gitea.io/gitea/modules/validation" ) // IsAPIPath if URL is an api path @@ -253,6 +254,8 @@ func validate(errs binding.Errors, data map[string]interface{}, f Form, l macaro data["ErrorMsg"] = trName + l.Tr("form.alpha_dash_error") case binding.ERR_ALPHA_DASH_DOT: data["ErrorMsg"] = trName + l.Tr("form.alpha_dash_dot_error") + case validation.ErrGitRefName: + data["ErrorMsg"] = trName + l.Tr("form.git_ref_name_error") case binding.ERR_SIZE: data["ErrorMsg"] = trName + l.Tr("form.size_error", GetSize(field)) case binding.ERR_MIN_SIZE: diff --git a/modules/auth/org.go b/modules/auth/org.go index 8ad2ca6c6..b9b3f981e 100644 --- a/modules/auth/org.go +++ b/modules/auth/org.go @@ -31,7 +31,7 @@ type UpdateOrgSettingForm struct { Name string `binding:"Required;AlphaDashDot;MaxSize(35)" locale:"org.org_name_holder"` FullName string `binding:"MaxSize(100)"` Description string `binding:"MaxSize(255)"` - Website string `binding:"Url;MaxSize(255)"` + Website string `binding:"ValidUrl;MaxSize(255)"` Location string `binding:"MaxSize(50)"` MaxRepoCreation int } diff --git a/modules/auth/repo_form.go b/modules/auth/repo_form.go index dd3fbff7b..c56e76c87 100644 --- a/modules/auth/repo_form.go +++ b/modules/auth/repo_form.go @@ -87,7 +87,7 @@ func (f MigrateRepoForm) ParseRemoteAddr(user *models.User) (string, error) { type RepoSettingForm struct { RepoName string `binding:"Required;AlphaDashDot;MaxSize(100)"` Description string `binding:"MaxSize(255)"` - Website string `binding:"Url;MaxSize(255)"` + Website string `binding:"ValidUrl;MaxSize(255)"` Interval string MirrorAddress string Private bool @@ -143,7 +143,7 @@ func (f WebhookForm) ChooseEvents() bool { // NewWebhookForm form for creating web hook type NewWebhookForm struct { - PayloadURL string `binding:"Required;Url"` + PayloadURL string `binding:"Required;ValidUrl"` ContentType int `binding:"Required"` Secret string WebhookForm @@ -156,7 +156,7 @@ func (f *NewWebhookForm) Validate(ctx *macaron.Context, errs binding.Errors) bin // NewSlackHookForm form for creating slack hook type NewSlackHookForm struct { - PayloadURL string `binding:"Required;Url"` + PayloadURL string `binding:"Required;ValidUrl"` Channel string `binding:"Required"` Username string IconURL string @@ -323,7 +323,7 @@ type EditRepoFileForm struct { CommitSummary string `binding:"MaxSize(100)"` CommitMessage string CommitChoice string `binding:"Required;MaxSize(50)"` - NewBranchName string `binding:"AlphaDashDot;MaxSize(100)"` + NewBranchName string `binding:"GitRefName;MaxSize(100)"` LastCommit string } @@ -356,7 +356,7 @@ type UploadRepoFileForm struct { CommitSummary string `binding:"MaxSize(100)"` CommitMessage string CommitChoice string `binding:"Required;MaxSize(50)"` - NewBranchName string `binding:"AlphaDashDot;MaxSize(100)"` + NewBranchName string `binding:"GitRefName;MaxSize(100)"` Files []string } @@ -387,7 +387,7 @@ type DeleteRepoFileForm struct { CommitSummary string `binding:"MaxSize(100)"` CommitMessage string CommitChoice string `binding:"Required;MaxSize(50)"` - NewBranchName string `binding:"AlphaDashDot;MaxSize(100)"` + NewBranchName string `binding:"GitRefName;MaxSize(100)"` } // Validate validates the fields diff --git a/modules/auth/user_form.go b/modules/auth/user_form.go index bac9622fc..2f767d4c8 100644 --- a/modules/auth/user_form.go +++ b/modules/auth/user_form.go @@ -103,7 +103,7 @@ type UpdateProfileForm struct { FullName string `binding:"MaxSize(100)"` Email string `binding:"Required;Email;MaxSize(254)"` KeepEmailPrivate bool - Website string `binding:"Url;MaxSize(255)"` + Website string `binding:"ValidUrl;MaxSize(255)"` Location string `binding:"MaxSize(50)"` } diff --git a/modules/validation/binding.go b/modules/validation/binding.go new file mode 100644 index 000000000..783499e69 --- /dev/null +++ b/modules/validation/binding.go @@ -0,0 +1,102 @@ +// Copyright 2017 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package validation + +import ( + "fmt" + "net/url" + "regexp" + "strings" + + "github.com/go-macaron/binding" +) + +const ( + // ErrGitRefName is git reference name error + ErrGitRefName = "GitRefNameError" +) + +var ( + // GitRefNamePattern is regular expression wirh unallowed characters in git reference name + GitRefNamePattern = regexp.MustCompile("[^\\d\\w-_\\./]") +) + +// AddBindingRules adds additional binding rules +func AddBindingRules() { + addGitRefNameBindingRule() + addValidURLBindingRule() +} + +func addGitRefNameBindingRule() { + // Git refname validation rule + binding.AddRule(&binding.Rule{ + IsMatch: func(rule string) bool { + return strings.HasPrefix(rule, "GitRefName") + }, + IsValid: func(errs binding.Errors, name string, val interface{}) (bool, binding.Errors) { + str := fmt.Sprintf("%v", val) + + if GitRefNamePattern.MatchString(str) { + errs.Add([]string{name}, ErrGitRefName, "GitRefName") + return false, errs + } + // Additional rules as described at https://www.kernel.org/pub/software/scm/git/docs/git-check-ref-format.html + if strings.HasPrefix(str, "/") || strings.HasSuffix(str, "/") || + strings.HasPrefix(str, ".") || strings.HasSuffix(str, ".") || + strings.HasSuffix(str, ".lock") || + strings.Contains(str, "..") || strings.Contains(str, "//") { + errs.Add([]string{name}, ErrGitRefName, "GitRefName") + return false, errs + } + + return true, errs + }, + }) +} + +func addValidURLBindingRule() { + // URL validation rule + binding.AddRule(&binding.Rule{ + IsMatch: func(rule string) bool { + return strings.HasPrefix(rule, "ValidUrl") + }, + IsValid: func(errs binding.Errors, name string, val interface{}) (bool, binding.Errors) { + str := fmt.Sprintf("%v", val) + if len(str) != 0 { + if u, err := url.ParseRequestURI(str); err != nil || + (u.Scheme != "http" && u.Scheme != "https") || + !validPort(portOnly(u.Host)) { + errs.Add([]string{name}, binding.ERR_URL, "Url") + return false, errs + } + } + + return true, errs + }, + }) +} + +func portOnly(hostport string) string { + colon := strings.IndexByte(hostport, ':') + if colon == -1 { + return "" + } + if i := strings.Index(hostport, "]:"); i != -1 { + return hostport[i+len("]:"):] + } + if strings.Contains(hostport, "]") { + return "" + } + return hostport[colon+len(":"):] +} + +func validPort(p string) bool { + for _, r := range []byte(p) { + if r < '0' || r > '9' { + return false + } + } + return true +} diff --git a/modules/validation/binding_test.go b/modules/validation/binding_test.go new file mode 100644 index 000000000..7bc41ac39 --- /dev/null +++ b/modules/validation/binding_test.go @@ -0,0 +1,62 @@ +// Copyright 2017 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package validation + +import ( + "fmt" + "net/http" + "net/http/httptest" + "testing" + + "github.com/go-macaron/binding" + "github.com/stretchr/testify/assert" + "gopkg.in/macaron.v1" +) + +const ( + testRoute = "/test" +) + +type ( + validationTestCase struct { + description string + data interface{} + expectedErrors binding.Errors + } + + handlerFunc func(interface{}, ...interface{}) macaron.Handler + + modeler interface { + Model() string + } + + TestForm struct { + BranchName string `form:"BranchName" binding:"GitRefName"` + URL string `form:"ValidUrl" binding:"ValidUrl"` + } +) + +func performValidationTest(t *testing.T, testCase validationTestCase) { + httpRecorder := httptest.NewRecorder() + m := macaron.Classic() + + m.Post(testRoute, binding.Validate(testCase.data), func(actual binding.Errors) { + assert.Equal(t, fmt.Sprintf("%+v", testCase.expectedErrors), fmt.Sprintf("%+v", actual)) + }) + + req, err := http.NewRequest("POST", testRoute, nil) + if err != nil { + panic(err) + } + + m.ServeHTTP(httpRecorder, req) + + switch httpRecorder.Code { + case http.StatusNotFound: + panic("Routing is messed up in test fixture (got 404): check methods and paths") + case http.StatusInternalServerError: + panic("Something bad happened on '" + testCase.description + "'") + } +} diff --git a/modules/validation/refname_test.go b/modules/validation/refname_test.go new file mode 100644 index 000000000..b101ffafe --- /dev/null +++ b/modules/validation/refname_test.go @@ -0,0 +1,142 @@ +// Copyright 2017 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package validation + +import ( + "testing" + + "github.com/go-macaron/binding" +) + +var gitRefNameValidationTestCases = []validationTestCase{ + { + description: "Referece contains only characters", + data: TestForm{ + BranchName: "test", + }, + expectedErrors: binding.Errors{}, + }, + { + description: "Reference name contains single slash", + data: TestForm{ + BranchName: "feature/test", + }, + expectedErrors: binding.Errors{}, + }, + { + description: "Reference name contains backslash", + data: TestForm{ + BranchName: "feature\\test", + }, + expectedErrors: binding.Errors{ + binding.Error{ + FieldNames: []string{"BranchName"}, + Classification: ErrGitRefName, + Message: "GitRefName", + }, + }, + }, + { + description: "Reference name starts with dot", + data: TestForm{ + BranchName: ".test", + }, + expectedErrors: binding.Errors{ + binding.Error{ + FieldNames: []string{"BranchName"}, + Classification: ErrGitRefName, + Message: "GitRefName", + }, + }, + }, + { + description: "Reference name ends with dot", + data: TestForm{ + BranchName: "test.", + }, + expectedErrors: binding.Errors{ + binding.Error{ + FieldNames: []string{"BranchName"}, + Classification: ErrGitRefName, + Message: "GitRefName", + }, + }, + }, + { + description: "Reference name starts with slash", + data: TestForm{ + BranchName: "/test", + }, + expectedErrors: binding.Errors{ + binding.Error{ + FieldNames: []string{"BranchName"}, + Classification: ErrGitRefName, + Message: "GitRefName", + }, + }, + }, + { + description: "Reference name ends with slash", + data: TestForm{ + BranchName: "test/", + }, + expectedErrors: binding.Errors{ + binding.Error{ + FieldNames: []string{"BranchName"}, + Classification: ErrGitRefName, + Message: "GitRefName", + }, + }, + }, + { + description: "Reference name ends with .lock", + data: TestForm{ + BranchName: "test.lock", + }, + expectedErrors: binding.Errors{ + binding.Error{ + FieldNames: []string{"BranchName"}, + Classification: ErrGitRefName, + Message: "GitRefName", + }, + }, + }, + { + description: "Reference name contains multiple consecutive dots", + data: TestForm{ + BranchName: "te..st", + }, + expectedErrors: binding.Errors{ + binding.Error{ + FieldNames: []string{"BranchName"}, + Classification: ErrGitRefName, + Message: "GitRefName", + }, + }, + }, + { + description: "Reference name contains multiple consecutive slashes", + data: TestForm{ + BranchName: "te//st", + }, + expectedErrors: binding.Errors{ + binding.Error{ + FieldNames: []string{"BranchName"}, + Classification: ErrGitRefName, + Message: "GitRefName", + }, + }, + }, +} + +func Test_GitRefNameValidation(t *testing.T) { + AddBindingRules() + + for _, testCase := range gitRefNameValidationTestCases { + t.Run(testCase.description, func(t *testing.T) { + performValidationTest(t, testCase) + }) + } +} diff --git a/modules/validation/validurl_test.go b/modules/validation/validurl_test.go new file mode 100644 index 000000000..ba4d7d53d --- /dev/null +++ b/modules/validation/validurl_test.go @@ -0,0 +1,111 @@ +// Copyright 2017 The Gitea Authors. All rights reserved. +// Use of this source code is governed by a MIT-style +// license that can be found in the LICENSE file. + +package validation + +import ( + "testing" + + "github.com/go-macaron/binding" +) + +var urlValidationTestCases = []validationTestCase{ + { + description: "Empty URL", + data: TestForm{ + URL: "", + }, + expectedErrors: binding.Errors{}, + }, + { + description: "URL without port", + data: TestForm{ + URL: "http://test.lan/", + }, + expectedErrors: binding.Errors{}, + }, + { + description: "URL with port", + data: TestForm{ + URL: "http://test.lan:3000/", + }, + expectedErrors: binding.Errors{}, + }, + { + description: "URL with IPv6 address without port", + data: TestForm{ + URL: "http://[::1]/", + }, + expectedErrors: binding.Errors{}, + }, + { + description: "URL with IPv6 address with port", + data: TestForm{ + URL: "http://[::1]:3000/", + }, + expectedErrors: binding.Errors{}, + }, + { + description: "Invalid URL", + data: TestForm{ + URL: "http//test.lan/", + }, + expectedErrors: binding.Errors{ + binding.Error{ + FieldNames: []string{"URL"}, + Classification: binding.ERR_URL, + Message: "Url", + }, + }, + }, + { + description: "Invalid schema", + data: TestForm{ + URL: "ftp://test.lan/", + }, + expectedErrors: binding.Errors{ + binding.Error{ + FieldNames: []string{"URL"}, + Classification: binding.ERR_URL, + Message: "Url", + }, + }, + }, + { + description: "Invalid port", + data: TestForm{ + URL: "http://test.lan:3x4/", + }, + expectedErrors: binding.Errors{ + binding.Error{ + FieldNames: []string{"URL"}, + Classification: binding.ERR_URL, + Message: "Url", + }, + }, + }, + { + description: "Invalid port with IPv6 address", + data: TestForm{ + URL: "http://[::1]:3x4/", + }, + expectedErrors: binding.Errors{ + binding.Error{ + FieldNames: []string{"URL"}, + Classification: binding.ERR_URL, + Message: "Url", + }, + }, + }, +} + +func Test_ValidURLValidation(t *testing.T) { + AddBindingRules() + + for _, testCase := range urlValidationTestCases { + t.Run(testCase.description, func(t *testing.T) { + performValidationTest(t, testCase) + }) + } +} diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index cb39ec857..04b5f44d2 100644 --- a/options/locale/locale_en-US.ini +++ b/options/locale/locale_en-US.ini @@ -233,6 +233,7 @@ Content = Content require_error = ` cannot be empty.` alpha_dash_error = ` must be valid alphanumeric or dash(-_) characters.` alpha_dash_dot_error = ` must be valid alphanumeric, dash(-_) or dot characters.` +git_ref_name_error = ` must be well formed git reference name.` size_error = ` must be size %s.` min_size_error = ` must contain at least %s characters.` max_size_error = ` must contain at most %s characters.`