From de02ea74c9cb3f19f8968e9d82749277c3db6562 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lauris=20Buk=C5=A1is-Haberkorns?= Date: Mon, 17 Apr 2017 02:27:06 +0300 Subject: [PATCH 1/4] Add correct git branch name validation --- cmd/web.go | 2 ++ modules/auth/auth.go | 3 ++ modules/auth/repo_form.go | 6 ++-- modules/validation/binding.go | 55 +++++++++++++++++++++++++++++++++ options/locale/locale_en-US.ini | 1 + 5 files changed, 64 insertions(+), 3 deletions(-) create mode 100644 modules/validation/binding.go diff --git a/cmd/web.go b/cmd/web.go index b2cc3959a29e6..411b50d9bf0e2 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/auth.go b/modules/auth/auth.go index 33ba777966612..f41d40760b63f 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.ERR_GIT_REF_NAME: + 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/repo_form.go b/modules/auth/repo_form.go index dd3fbff7bf468..629999987e60e 100644 --- a/modules/auth/repo_form.go +++ b/modules/auth/repo_form.go @@ -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/validation/binding.go b/modules/validation/binding.go new file mode 100644 index 0000000000000..e6b5ef0ce6910 --- /dev/null +++ b/modules/validation/binding.go @@ -0,0 +1,55 @@ +// 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" + "regexp" + "strings" + + "github.com/go-macaron/binding" +) + +const ( + // ERR_GIT_REF_NAME is git reference name error + ERR_GIT_REF_NAME = "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() +} + +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}, ERR_GIT_REF_NAME, "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}, ERR_GIT_REF_NAME, "GitRefName") + return false, errs + } + + return true, errs + }, + }) +} diff --git a/options/locale/locale_en-US.ini b/options/locale/locale_en-US.ini index cb39ec857129b..04b5f44d2c25a 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.` From 1a049bf715cabccdf30b64234fcae6115c6f4441 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lauris=20Buk=C5=A1is-Haberkorns?= Date: Mon, 17 Apr 2017 02:38:18 +0300 Subject: [PATCH 2/4] Change git refname validation error constant name --- modules/auth/auth.go | 2 +- modules/validation/binding.go | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/modules/auth/auth.go b/modules/auth/auth.go index f41d40760b63f..89b3e385099e7 100644 --- a/modules/auth/auth.go +++ b/modules/auth/auth.go @@ -254,7 +254,7 @@ 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.ERR_GIT_REF_NAME: + 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)) diff --git a/modules/validation/binding.go b/modules/validation/binding.go index e6b5ef0ce6910..c1880797b0555 100644 --- a/modules/validation/binding.go +++ b/modules/validation/binding.go @@ -13,8 +13,8 @@ import ( ) const ( - // ERR_GIT_REF_NAME is git reference name error - ERR_GIT_REF_NAME = "GitRefNameError" + // ErrGitRefName is git reference name error + ErrGitRefName = "GitRefNameError" ) var ( @@ -37,7 +37,7 @@ func addGitRefNameBindingRule() { str := fmt.Sprintf("%v", val) if GitRefNamePattern.MatchString(str) { - errs.Add([]string{name}, ERR_GIT_REF_NAME, "GitRefName") + 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 @@ -45,7 +45,7 @@ func addGitRefNameBindingRule() { strings.HasPrefix(str, ".") || strings.HasSuffix(str, ".") || strings.HasSuffix(str, ".lock") || strings.Contains(str, "..") || strings.Contains(str, "//") { - errs.Add([]string{name}, ERR_GIT_REF_NAME, "GitRefName") + errs.Add([]string{name}, ErrGitRefName, "GitRefName") return false, errs } From d23c65793902408434cf37c131afb75f4ffce8d6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lauris=20Buk=C5=A1is-Haberkorns?= Date: Tue, 18 Apr 2017 02:22:52 +0300 Subject: [PATCH 3/4] Add git reference name validation unit tests --- modules/validation/binding_test.go | 62 +++++++++++++ modules/validation/refname_test.go | 142 +++++++++++++++++++++++++++++ 2 files changed, 204 insertions(+) create mode 100644 modules/validation/binding_test.go create mode 100644 modules/validation/refname_test.go diff --git a/modules/validation/binding_test.go b/modules/validation/binding_test.go new file mode 100644 index 0000000000000..e726a145b92ca --- /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" + formContentType = "application/x-www-form-urlencoded" +) + +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"` + } +) + +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 0000000000000..b101ffafef082 --- /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) + }) + } +} From 5fa98bb016d427b05281dccf92418bc39b4ac912 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Lauris=20Buk=C5=A1is-Haberkorns?= Date: Tue, 18 Apr 2017 02:24:24 +0300 Subject: [PATCH 4/4] Remove unused variable in unit test --- modules/validation/binding_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/modules/validation/binding_test.go b/modules/validation/binding_test.go index e726a145b92ca..e2bf924134e5c 100644 --- a/modules/validation/binding_test.go +++ b/modules/validation/binding_test.go @@ -16,8 +16,7 @@ import ( ) const ( - testRoute = "/test" - formContentType = "application/x-www-form-urlencoded" + testRoute = "/test" ) type (