chore: Webhooks fixes

2026-01-06 23:23:47 +00:00 · 2026-01-06 23:23:47 +00:00 · c714dc0c62
parent 0368fb10db
commit c714dc0c62
5 changed files with 244 additions and 219 deletions
--- a/service/internal/webhooks/auth.go
+++ b/service/internal/webhooks/auth.go
@ -22,23 +22,47 @@ func NewAuthVerifier(cfg config.WebhookConfig) *AuthVerifier {
 }

 func (v *AuthVerifier) Verify(r *http.Request, payload []byte) bool {
-	switch v.config.AuthType {
-	case "hmac-sha256":
-		return v.verifyHMAC256(r, payload)
-	case "hmac-sha1":
-		return v.verifyHMAC1(r, payload)
-	case "bearer":
-		return v.verifyBearer(r)
-	case "basic":
-		return v.verifyBasic(r)
-	case "none", "":
-		return true
-	default:
-		log.WithFields(log.Fields{
-			"authType": v.config.AuthType,
-		}).Warnf("Unknown auth type, rejecting")
-		return false
+	verifier := v.getVerifier()
+	if verifier == nil {
+		return v.handleUnknownAuthType()
 	}
+	return verifier(r, payload)
+}
+
+type authVerifierFunc func(*http.Request, []byte) bool
+
+func (v *AuthVerifier) getVerifier() authVerifierFunc {
+	if v.config.AuthType == "" || v.config.AuthType == "none" {
+		return func(_ *http.Request, _ []byte) bool {
+			return true
+		}
+	}
+
+	verifierMap := v.buildVerifierMap()
+	if verifier, ok := verifierMap[v.config.AuthType]; ok {
+		return verifier
+	}
+	return nil
+}
+
+func (v *AuthVerifier) buildVerifierMap() map[string]authVerifierFunc {
+	return map[string]authVerifierFunc{
+		"hmac-sha256": v.verifyHMAC256,
+		"hmac-sha1":   v.verifyHMAC1,
+		"bearer": func(r *http.Request, _ []byte) bool {
+			return v.verifyBearer(r)
+		},
+		"basic": func(r *http.Request, _ []byte) bool {
+			return v.verifyBasic(r)
+		},
+	}
+}
+
+func (v *AuthVerifier) handleUnknownAuthType() bool {
+	log.WithFields(log.Fields{
+		"authType": v.config.AuthType,
+	}).Warnf("Unknown auth type, rejecting")
+	return false
 }

 func (v *AuthVerifier) verifyHMAC256(r *http.Request, payload []byte) bool {
@ -123,12 +147,23 @@ func (v *AuthVerifier) verifyBasic(r *http.Request) bool {
 		return false
 	}

+	return v.verifyBasicCredentials(username, password)
+}
+
+func (v *AuthVerifier) verifyBasicCredentials(username, password string) bool {
 	parts := strings.SplitN(v.config.Secret, ":", 2)
 	if len(parts) == 2 {
-		usernameMatch := subtle.ConstantTimeCompare([]byte(username), []byte(parts[0]))
-		passwordMatch := subtle.ConstantTimeCompare([]byte(password), []byte(parts[1]))
-		return usernameMatch == 1 && passwordMatch == 1
+		return v.verifyBasicWithUsername(username, password, parts[0], parts[1])
 	}
+	return v.verifyBasicPasswordOnly(password)
+}

+func (v *AuthVerifier) verifyBasicWithUsername(username, password, expectedUsername, expectedPassword string) bool {
+	usernameMatch := subtle.ConstantTimeCompare([]byte(username), []byte(expectedUsername))
+	passwordMatch := subtle.ConstantTimeCompare([]byte(password), []byte(expectedPassword))
+	return usernameMatch == 1 && passwordMatch == 1
+}
+
+func (v *AuthVerifier) verifyBasicPasswordOnly(password string) bool {
 	return subtle.ConstantTimeCompare([]byte(password), []byte(v.config.Secret)) == 1
 }
--- a/service/internal/webhooks/github.go
+++ b/service/internal/webhooks/github.go
@ -7,158 +7,106 @@ import (
 // ApplyGitHubTemplate applies GitHub-specific template configurations
 // This allows users to use simple template names instead of configuring everything manually
 func ApplyGitHubTemplate(cfg *config.WebhookConfig, template string) {
-	switch template {
-	case "github-push":
-		applyGitHubPushTemplate(cfg)
-	case "github-pr", "github-pull-request":
-		applyGitHubPRTemplate(cfg)
-	case "github-release":
-		applyGitHubReleaseTemplate(cfg)
-	case "github-workflow":
-		applyGitHubWorkflowTemplate(cfg)
+	applier := getTemplateApplier(template)
+	if applier != nil {
+		applier(cfg)
+	}
+}
+
+type templateApplier func(*config.WebhookConfig)
+
+func getTemplateApplier(template string) templateApplier {
+	templateMap := map[string]templateApplier{
+		"github-push":         applyGitHubPushTemplate,
+		"github-pr":           applyGitHubPRTemplate,
+		"github-pull-request": applyGitHubPRTemplate,
+		"github-release":      applyGitHubReleaseTemplate,
+		"github-workflow":     applyGitHubWorkflowTemplate,
+	}
+	return templateMap[template]
+}
+
+func setDefaultAuth(cfg *config.WebhookConfig) {
+	if cfg.AuthHeader == "" {
+		cfg.AuthHeader = "X-Hub-Signature-256"
+	}
+	if cfg.AuthType == "" {
+		cfg.AuthType = "hmac-sha256"
+	}
+}
+
+func ensureMatchHeaders(cfg *config.WebhookConfig) {
+	if len(cfg.MatchHeaders) == 0 {
+		cfg.MatchHeaders = make(map[string]string)
+	}
+}
+
+func ensureExtract(cfg *config.WebhookConfig) {
+	if len(cfg.Extract) == 0 {
+		cfg.Extract = make(map[string]string)
+	}
+}
+
+func setExtractIfMissing(cfg *config.WebhookConfig, key, value string) {
+	if _, exists := cfg.Extract[key]; !exists {
+		cfg.Extract[key] = value
+	}
+}
+
+func setMatchHeaderIfMissing(cfg *config.WebhookConfig, key, value string) {
+	if _, exists := cfg.MatchHeaders[key]; !exists {
+		cfg.MatchHeaders[key] = value
 	}
 }

 func applyGitHubPushTemplate(cfg *config.WebhookConfig) {
-	if cfg.AuthHeader == "" {
-		cfg.AuthHeader = "X-Hub-Signature-256"
-	}
-	if cfg.AuthType == "" {
-		cfg.AuthType = "hmac-sha256"
-	}
-	if len(cfg.MatchHeaders) == 0 {
-		cfg.MatchHeaders = make(map[string]string)
-	}
-	if _, exists := cfg.MatchHeaders["X-GitHub-Event"]; !exists {
-		cfg.MatchHeaders["X-GitHub-Event"] = "push"
-	}
-	if len(cfg.Extract) == 0 {
-		cfg.Extract = make(map[string]string)
-	}
-	if _, exists := cfg.Extract["git_repository"]; !exists {
-		cfg.Extract["git_repository"] = "$.repository.full_name"
-	}
-	if _, exists := cfg.Extract["git_ref"]; !exists {
-		cfg.Extract["git_ref"] = "$.ref"
-	}
-	if _, exists := cfg.Extract["git_commit"]; !exists {
-		cfg.Extract["git_commit"] = "$.head_commit.id"
-	}
-	if _, exists := cfg.Extract["git_branch"]; !exists {
-		cfg.Extract["git_branch"] = "$.ref"
-	}
-	if _, exists := cfg.Extract["git_message"]; !exists {
-		cfg.Extract["git_message"] = "$.head_commit.message"
-	}
-	if _, exists := cfg.Extract["git_author"]; !exists {
-		cfg.Extract["git_author"] = "$.head_commit.author.name"
-	}
+	setDefaultAuth(cfg)
+	ensureMatchHeaders(cfg)
+	setMatchHeaderIfMissing(cfg, "X-GitHub-Event", "push")
+	ensureExtract(cfg)
+	setExtractIfMissing(cfg, "git_repository", "$.repository.full_name")
+	setExtractIfMissing(cfg, "git_ref", "$.ref")
+	setExtractIfMissing(cfg, "git_commit", "$.head_commit.id")
+	setExtractIfMissing(cfg, "git_branch", "$.ref")
+	setExtractIfMissing(cfg, "git_message", "$.head_commit.message")
+	setExtractIfMissing(cfg, "git_author", "$.head_commit.author.name")
 }

 func applyGitHubPRTemplate(cfg *config.WebhookConfig) {
-	if cfg.AuthHeader == "" {
-		cfg.AuthHeader = "X-Hub-Signature-256"
-	}
-	if cfg.AuthType == "" {
-		cfg.AuthType = "hmac-sha256"
-	}
-	if len(cfg.MatchHeaders) == 0 {
-		cfg.MatchHeaders = make(map[string]string)
-	}
-	if _, exists := cfg.MatchHeaders["X-GitHub-Event"]; !exists {
-		cfg.MatchHeaders["X-GitHub-Event"] = "pull_request"
-	}
-	if len(cfg.Extract) == 0 {
-		cfg.Extract = make(map[string]string)
-	}
-	if _, exists := cfg.Extract["pr_number"]; !exists {
-		cfg.Extract["pr_number"] = "$.number"
-	}
-	if _, exists := cfg.Extract["pr_title"]; !exists {
-		cfg.Extract["pr_title"] = "$.pull_request.title"
-	}
-	if _, exists := cfg.Extract["pr_author"]; !exists {
-		cfg.Extract["pr_author"] = "$.pull_request.user.login"
-	}
-	if _, exists := cfg.Extract["pr_action"]; !exists {
-		cfg.Extract["pr_action"] = "$.action"
-	}
-	if _, exists := cfg.Extract["git_repository"]; !exists {
-		cfg.Extract["git_repository"] = "$.repository.full_name"
-	}
-	if _, exists := cfg.Extract["pr_state"]; !exists {
-		cfg.Extract["pr_state"] = "$.pull_request.state"
-	}
-	if _, exists := cfg.Extract["pr_head_sha"]; !exists {
-		cfg.Extract["pr_head_sha"] = "$.pull_request.head.sha"
-	}
+	setDefaultAuth(cfg)
+	ensureMatchHeaders(cfg)
+	setMatchHeaderIfMissing(cfg, "X-GitHub-Event", "pull_request")
+	ensureExtract(cfg)
+	setExtractIfMissing(cfg, "pr_number", "$.number")
+	setExtractIfMissing(cfg, "pr_title", "$.pull_request.title")
+	setExtractIfMissing(cfg, "pr_author", "$.pull_request.user.login")
+	setExtractIfMissing(cfg, "pr_action", "$.action")
+	setExtractIfMissing(cfg, "git_repository", "$.repository.full_name")
+	setExtractIfMissing(cfg, "pr_state", "$.pull_request.state")
+	setExtractIfMissing(cfg, "pr_head_sha", "$.pull_request.head.sha")
 }

 func applyGitHubReleaseTemplate(cfg *config.WebhookConfig) {
-	if cfg.AuthHeader == "" {
-		cfg.AuthHeader = "X-Hub-Signature-256"
-	}
-	if cfg.AuthType == "" {
-		cfg.AuthType = "hmac-sha256"
-	}
-	if len(cfg.MatchHeaders) == 0 {
-		cfg.MatchHeaders = make(map[string]string)
-	}
-	if _, exists := cfg.MatchHeaders["X-GitHub-Event"]; !exists {
-		cfg.MatchHeaders["X-GitHub-Event"] = "release"
-	}
-	if len(cfg.Extract) == 0 {
-		cfg.Extract = make(map[string]string)
-	}
-	if _, exists := cfg.Extract["release_action"]; !exists {
-		cfg.Extract["release_action"] = "$.action"
-	}
-	if _, exists := cfg.Extract["release_tag"]; !exists {
-		cfg.Extract["release_tag"] = "$.release.tag_name"
-	}
-	if _, exists := cfg.Extract["release_name"]; !exists {
-		cfg.Extract["release_name"] = "$.release.name"
-	}
-	if _, exists := cfg.Extract["git_repository"]; !exists {
-		cfg.Extract["git_repository"] = "$.repository.full_name"
-	}
-	if _, exists := cfg.Extract["release_author"]; !exists {
-		cfg.Extract["release_author"] = "$.release.author.login"
-	}
+	setDefaultAuth(cfg)
+	ensureMatchHeaders(cfg)
+	setMatchHeaderIfMissing(cfg, "X-GitHub-Event", "release")
+	ensureExtract(cfg)
+	setExtractIfMissing(cfg, "release_action", "$.action")
+	setExtractIfMissing(cfg, "release_tag", "$.release.tag_name")
+	setExtractIfMissing(cfg, "release_name", "$.release.name")
+	setExtractIfMissing(cfg, "git_repository", "$.repository.full_name")
+	setExtractIfMissing(cfg, "release_author", "$.release.author.login")
 }

 func applyGitHubWorkflowTemplate(cfg *config.WebhookConfig) {
-	if cfg.AuthHeader == "" {
-		cfg.AuthHeader = "X-Hub-Signature-256"
-	}
-	if cfg.AuthType == "" {
-		cfg.AuthType = "hmac-sha256"
-	}
-	if len(cfg.MatchHeaders) == 0 {
-		cfg.MatchHeaders = make(map[string]string)
-	}
-	if _, exists := cfg.MatchHeaders["X-GitHub-Event"]; !exists {
-		cfg.MatchHeaders["X-GitHub-Event"] = "workflow_run"
-	}
-	if len(cfg.Extract) == 0 {
-		cfg.Extract = make(map[string]string)
-	}
-	if _, exists := cfg.Extract["workflow_name"]; !exists {
-		cfg.Extract["workflow_name"] = "$.workflow_run.name"
-	}
-	if _, exists := cfg.Extract["workflow_status"]; !exists {
-		cfg.Extract["workflow_status"] = "$.workflow_run.status"
-	}
-	if _, exists := cfg.Extract["workflow_conclusion"]; !exists {
-		cfg.Extract["workflow_conclusion"] = "$.workflow_run.conclusion"
-	}
-	if _, exists := cfg.Extract["git_repository"]; !exists {
-		cfg.Extract["git_repository"] = "$.repository.full_name"
-	}
-	if _, exists := cfg.Extract["git_commit"]; !exists {
-		cfg.Extract["git_commit"] = "$.workflow_run.head_sha"
-	}
-	if _, exists := cfg.Extract["git_branch"]; !exists {
-		cfg.Extract["git_branch"] = "$.workflow_run.head_branch"
-	}
+	setDefaultAuth(cfg)
+	ensureMatchHeaders(cfg)
+	setMatchHeaderIfMissing(cfg, "X-GitHub-Event", "workflow_run")
+	ensureExtract(cfg)
+	setExtractIfMissing(cfg, "workflow_name", "$.workflow_run.name")
+	setExtractIfMissing(cfg, "workflow_status", "$.workflow_run.status")
+	setExtractIfMissing(cfg, "workflow_conclusion", "$.workflow_run.conclusion")
+	setExtractIfMissing(cfg, "git_repository", "$.repository.full_name")
+	setExtractIfMissing(cfg, "git_commit", "$.workflow_run.head_sha")
+	setExtractIfMissing(cfg, "git_branch", "$.workflow_run.head_branch")
 }
--- a/service/internal/webhooks/handler.go
+++ b/service/internal/webhooks/handler.go
@ -1,7 +1,6 @@
 package webhooks

 import (
-	"encoding/json"
 	"io"
 	"net/http"

@ -34,70 +33,83 @@ func (h *WebhookHandler) HandleWebhook(w http.ResponseWriter, r *http.Request) {
 		return
 	}

+	payload, err := h.readPayload(r)
+	if err != nil {
+		http.Error(w, "Failed to read payload", http.StatusBadRequest)
+		return
+	}
+
+	matchingActions := h.findMatchingActions(r, payload)
+	if len(matchingActions) == 0 {
+		h.writeOKResponse(w, "no matching webhook actions")
+		return
+	}
+
+	processed := h.processMatchingActions(matchingActions, r, payload)
+	log.WithFields(log.Fields{
+		"matched":   len(matchingActions),
+		"processed": processed,
+	}).Infof("Webhook processed")
+
+	h.writeOKResponse(w, "webhook actions")
+}
+
+func (h *WebhookHandler) readPayload(r *http.Request) ([]byte, error) {
 	maxSize := int64(1024 * 1024)
 	payload, err := io.ReadAll(io.LimitReader(r.Body, maxSize))
 	if err != nil {
 		log.WithFields(log.Fields{
 			"error": err,
 		}).Warnf("Failed to read webhook payload")
-		http.Error(w, "Failed to read payload", http.StatusBadRequest)
-		return
+		return nil, err
 	}

-	var bodyData interface{}
-	if err := json.Unmarshal(payload, &bodyData); err != nil {
-		log.WithFields(log.Fields{
-			"error": err,
-		}).Debugf("Webhook payload is not valid JSON")
-	}
-
-	matchingActions := h.findMatchingActions(r, payload, bodyData)
-
-	if len(matchingActions) == 0 {
-		log.WithFields(log.Fields{
-			"path":   r.URL.Path,
-			"method": r.Method,
-		}).Debugf("No matching webhook actions found")
-		w.WriteHeader(http.StatusOK)
-		w.Write([]byte("OK"))
-		return
+	return payload, nil
+}
+
+func (h *WebhookHandler) writeOKResponse(w http.ResponseWriter, context string) {
+	w.WriteHeader(http.StatusOK)
+	if _, err := w.Write([]byte("OK")); err != nil {
+		log.WithError(err).Warnf("Failed to write response for %s", context)
 	}
+}

+func (h *WebhookHandler) processMatchingActions(matchingActions []ActionWebhookConfig, r *http.Request, payload []byte) int {
 	processed := 0
 	for _, actionConfig := range matchingActions {
 		if h.processWebhook(actionConfig, r, payload) {
 			processed++
 		}
 	}
-
-	log.WithFields(log.Fields{
-		"matched":   len(matchingActions),
-		"processed": processed,
-	}).Infof("Webhook processed")
-
-	w.WriteHeader(http.StatusOK)
-	w.Write([]byte("OK"))
+	return processed
 }

-func (h *WebhookHandler) findMatchingActions(r *http.Request, payload []byte, bodyData interface{}) []ActionWebhookConfig {
+func (h *WebhookHandler) findMatchingActions(r *http.Request, payload []byte) []ActionWebhookConfig {
 	var matches []ActionWebhookConfig

 	for _, action := range h.cfg.Actions {
-		for _, webhookConfig := range action.ExecOnWebhook {
-			webhookConfigCopy := webhookConfig
+		matches = append(matches, h.findMatchingWebhooksForAction(action, r, payload)...)
+	}

-			if webhookConfigCopy.Template != "" {
-				ApplyGitHubTemplate(&webhookConfigCopy, webhookConfigCopy.Template)
-			}
+	return matches
+}

-			matcher := NewWebhookMatcher(webhookConfigCopy, r, payload, bodyData)
+func (h *WebhookHandler) findMatchingWebhooksForAction(action *config.Action, r *http.Request, payload []byte) []ActionWebhookConfig {
+	var matches []ActionWebhookConfig

-			if matcher.Matches() {
-				matches = append(matches, ActionWebhookConfig{
-					Action: action,
-					Config: webhookConfigCopy,
-				})
-			}
+	for _, webhookConfig := range action.ExecOnWebhook {
+		webhookConfigCopy := webhookConfig
+
+		if webhookConfigCopy.Template != "" {
+			ApplyGitHubTemplate(&webhookConfigCopy, webhookConfigCopy.Template)
+		}
+
+		matcher := NewWebhookMatcher(webhookConfigCopy, r, payload)
+		if matcher.Matches() {
+			matches = append(matches, ActionWebhookConfig{
+				Action: action,
+				Config: webhookConfigCopy,
+			})
 		}
 	}

@ -114,10 +126,7 @@ func (h *WebhookHandler) processWebhook(actionConfig ActionWebhookConfig, r *htt
 		return false
 	}

-	var bodyData interface{}
-	json.Unmarshal(payload, &bodyData)
-
-	matcher := NewWebhookMatcher(actionConfig.Config, r, payload, bodyData)
+	matcher := NewWebhookMatcher(actionConfig.Config, r, payload)

 	args, err := matcher.ExtractArguments()
 	if err != nil {
--- a/service/internal/webhooks/jsonpath.go
+++ b/service/internal/webhooks/jsonpath.go
@ -25,7 +25,13 @@ func (m *JSONMatcher) MatchPath(pathExpr string, expectedValue string) (bool, er
 		return false, err
 	}

-	valueStr := fmt.Sprintf("%v", value)
+	// Marshal to JSON for consistent string representation
+	jsonBytes, err := json.Marshal(value)
+	if err != nil {
+		return false, fmt.Errorf("failed to marshal extracted value: %w", err)
+	}
+
+	valueStr := string(jsonBytes)
 	return valueStr == expectedValue, nil
 }

@ -34,7 +40,14 @@ func (m *JSONMatcher) ExtractValue(pathExpr string) (string, error) {
 	if err != nil {
 		return "", err
 	}
-	return fmt.Sprintf("%v", value), nil
+
+	// Marshal to JSON for consistent string representation
+	jsonBytes, err := json.Marshal(value)
+	if err != nil {
+		return "", fmt.Errorf("failed to marshal extracted value: %w", err)
+	}
+
+	return string(jsonBytes), nil
 }

 func (m *JSONMatcher) GetPayload() interface{} {
--- a/service/internal/webhooks/matcher.go
+++ b/service/internal/webhooks/matcher.go
@ -15,7 +15,7 @@ type WebhookMatcher struct {
 	bodyBytes []byte
 }

-func NewWebhookMatcher(cfg config.WebhookConfig, r *http.Request, bodyBytes []byte, body interface{}) *WebhookMatcher {
+func NewWebhookMatcher(cfg config.WebhookConfig, r *http.Request, bodyBytes []byte) *WebhookMatcher {
 	return &WebhookMatcher{
 		config:    cfg,
 		req:       r,
@ -83,13 +83,7 @@ func (m *WebhookMatcher) matchPath() bool {
 		return true
 	}

-	parts := strings.SplitN(m.config.MatchPath, "=", 2)
-	jsonPath := parts[0]
-	expectedValue := ""
-	if len(parts) == 2 {
-		expectedValue = parts[1]
-	}
-
+	jsonPath, expectedValue := m.parseMatchPath()
 	matcher, err := NewJSONMatcher(m.bodyBytes)
 	if err != nil {
 		log.WithFields(log.Fields{
@ -98,6 +92,20 @@ func (m *WebhookMatcher) matchPath() bool {
 		return false
 	}

+	return m.matchPathValue(matcher, jsonPath, expectedValue)
+}
+
+func (m *WebhookMatcher) parseMatchPath() (string, string) {
+	parts := strings.SplitN(m.config.MatchPath, "=", 2)
+	jsonPath := parts[0]
+	expectedValue := ""
+	if len(parts) == 2 {
+		expectedValue = parts[1]
+	}
+	return jsonPath, expectedValue
+}
+
+func (m *WebhookMatcher) matchPathValue(matcher *JSONMatcher, jsonPath, expectedValue string) bool {
 	if expectedValue == "" {
 		_, err := matcher.ExtractValue(jsonPath)
 		return err == nil
@ -131,13 +139,21 @@ func (m *WebhookMatcher) compareValues(actual, expected string) bool {
 }

 func (m *WebhookMatcher) ExtractArguments() (map[string]string, error) {
-	args := make(map[string]string)
-
 	matcher, err := NewJSONMatcher(m.bodyBytes)
 	if err != nil {
 		return nil, err
 	}

+	args := m.extractJSONPathValues(matcher)
+	m.addWebhookMetadata(args)
+	m.addWebhookHeaders(args)
+
+	return args, nil
+}
+
+func (m *WebhookMatcher) extractJSONPathValues(matcher *JSONMatcher) map[string]string {
+	args := make(map[string]string)
+
 	for argName, jsonPath := range m.config.Extract {
 		value, err := matcher.ExtractValue(jsonPath)
 		if err != nil {
@ -151,15 +167,19 @@ func (m *WebhookMatcher) ExtractArguments() (map[string]string, error) {
 		args[argName] = value
 	}

+	return args
+}
+
+func (m *WebhookMatcher) addWebhookMetadata(args map[string]string) {
 	args["webhook_method"] = m.req.Method
 	args["webhook_path"] = m.req.URL.Path
 	args["webhook_query"] = m.req.URL.RawQuery
+}

+func (m *WebhookMatcher) addWebhookHeaders(args map[string]string) {
 	for key, values := range m.req.Header {
 		if len(values) > 0 {
 			args["webhook_header_"+strings.ToLower(key)] = values[0]
 		}
 	}
-
-	return args, nil
 }