Slack webhook channel name cannot be empty or just contain an hashtag (#4786)

Author: adelowo

modules/auth/repo_form.go

@@ -10,6 +10,8 @@ import (
 	"strings"
 
 	"code.gitea.io/gitea/models"
+	"code.gitea.io/gitea/routers/utils"
+
 	"github.com/Unknwon/com"
 	"github.com/go-macaron/binding"
 	"gopkg.in/macaron.v1"
@@ -225,6 +227,11 @@ func (f *NewSlackHookForm) Validate(ctx *macaron.Context, errs binding.Errors) b
 	return validate(errs, ctx.Data, f, ctx.Locale)
 }
 
+// HasInvalidChannel validates the channel name is in the right format
+func (f NewSlackHookForm) HasInvalidChannel() bool {
+	return !utils.IsValidSlackChannel(f.Channel)
+}
+
 // NewDiscordHookForm form for creating discord hook
 type NewDiscordHookForm struct {
 	PayloadURL string `binding:"Required;ValidUrl"`

options/locale/locale_en-US.ini

@@ -1044,6 +1044,7 @@ settings.search_user_placeholder = Search user…
 settings.org_not_allowed_to_be_collaborator = Organizations cannot be added as a collaborator.
 settings.user_is_org_member = The user is an organization member who cannot be added as a collaborator.
 settings.add_webhook = Add Webhook
+settings.add_webhook.invalid_channel_name = Webhook channel name cannot be empty and cannot contain only a # character.
 settings.hooks_desc = Webhooks automatically make HTTP POST requests to a server when certain Gitea events trigger. Read more in the <a target="_blank" rel="noopener noreferrer" href="%s">webhooks guide</a>.
 settings.webhook_deletion = Remove Webhook
 settings.webhook_deletion_desc = Removing a webhook deletes its settings and delivery history. Continue?

routers/api/v1/utils/hook.go

@@ -5,14 +5,16 @@
 package utils
 
 import (
-	api "code.gitea.io/sdk/gitea"
-
 	"encoding/json"
 	"net/http"
+	"strings"
 
 	"code.gitea.io/gitea/models"
 	"code.gitea.io/gitea/modules/context"
 	"code.gitea.io/gitea/routers/api/v1/convert"
+	"code.gitea.io/gitea/routers/utils"
+	api "code.gitea.io/sdk/gitea"
+
 	"github.com/Unknwon/com"
 )
 
@@ -119,8 +121,14 @@ func addHook(ctx *context.APIContext, form *api.CreateHookOption, orgID, repoID
 			ctx.Error(422, "", "Missing config option: channel")
 			return nil, false
 		}
+
+		if !utils.IsValidSlackChannel(channel) {
+			ctx.Error(400, "", "Invalid slack channel name")
+			return nil, false
+		}
+
 		meta, err := json.Marshal(&models.SlackMeta{
-			Channel:  channel,
+			Channel:  strings.TrimSpace(channel),
 			Username: form.Config["username"],
 			IconURL:  form.Config["icon_url"],
 			Color:    form.Config["color"],

routers/repo/webhook.go

@@ -332,8 +332,14 @@ func SlackHooksNewPost(ctx *context.Context, form auth.NewSlackHookForm) {
 		return
 	}
 
+	if form.HasInvalidChannel() {
+		ctx.Flash.Error(ctx.Tr("repo.settings.add_webhook.invalid_channel_name"))
+		ctx.Redirect(orCtx.Link + "/settings/hooks/slack/new")
+		return
+	}
+
 	meta, err := json.Marshal(&models.SlackMeta{
-		Channel:  form.Channel,
+		Channel:  strings.TrimSpace(form.Channel),
 		Username: form.Username,
 		IconURL:  form.IconURL,
 		Color:    form.Color,
@@ -515,8 +521,14 @@ func SlackHooksEditPost(ctx *context.Context, form auth.NewSlackHookForm) {
 		return
 	}
 
+	if form.HasInvalidChannel() {
+		ctx.Flash.Error(ctx.Tr("repo.settings.add_webhook.invalid_channel_name"))
+		ctx.Redirect(fmt.Sprintf("%s/settings/hooks/%d", orCtx.Link, w.ID))
+		return
+	}
+
 	meta, err := json.Marshal(&models.SlackMeta{
-		Channel:  form.Channel,
+		Channel:  strings.TrimSpace(form.Channel),
 		Username: form.Username,
 		IconURL:  form.IconURL,
 		Color:    form.Color,

routers/utils/utils.go

@@ -15,3 +15,22 @@ func RemoveUsernameParameterSuffix(name string) string {
 	}
 	return name
 }
+
+// IsValidSlackChannel validates a channel name conforms to what slack expects.
+// It makes sure a channel name cannot be empty and invalid ( only an # )
+func IsValidSlackChannel(channelName string) bool {
+	switch len(strings.TrimSpace(channelName)) {
+	case 0:
+		return false
+	case 1:
+		// Keep default behaviour where a channel name is still
+		// valid without an #
+		// But if it contains only an #, it should be regarded as
+		// invalid
+		if channelName[0] == '#' {
+			return false
+		}
+	}
+
+	return true
+}

routers/utils/utils_test.go

@@ -15,3 +15,20 @@ func TestRemoveUsernameParameterSuffix(t *testing.T) {
 	assert.Equal(t, "foobar", RemoveUsernameParameterSuffix("foobar"))
 	assert.Equal(t, "", RemoveUsernameParameterSuffix(""))
 }
+
+func TestIsValidSlackChannel(t *testing.T) {
+	tt := []struct {
+		channelName string
+		expected    bool
+	}{
+		{"gitea", true},
+		{"  ", false},
+		{"#", false},
+		{"gitea   ", true},
+		{"  gitea", true},
+	}
+
+	for _, v := range tt {
+		assert.Equal(t, v.expected, IsValidSlackChannel(v.channelName))
+	}
+}