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

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

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

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:

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
 }

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

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)"`
 }
 

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
+}

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 + "'")
+	}
+}

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)
+		})
+	}
+}

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)
+		})
+	}
+}

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.`