Skip to content

Commit c453210

Browse files
Exgenewxiaoguangsilverwind
authored
fix(webhook): prevent tag events from bypassing branch filters targets #35449 (#35567)
Tag creation/deletion was triggering push webhooks even when branch filters were configured, causing unintended pipeline executions. This change modifies the branch filter logic to check the full ref name directly instead of first determining if it's a "branch" event. Fixes: Tag events now properly respect branch filters - Add getPayloadRef() function to extract full ref names - Update PrepareWebhook() to use direct ref matching - Prevents refs/tags/* from matching refs/heads/* filters Closes #35449 --------- Co-authored-by: wxiaoguang <wxiaoguang@gmail.com> Co-authored-by: silverwind <me@silverwind.io>
1 parent efc48c3 commit c453210

File tree

7 files changed

+83
-34
lines changed

7 files changed

+83
-34
lines changed

options/locale/locale_en-US.ini

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2434,7 +2434,9 @@ settings.event_workflow_job_desc = Gitea Actions Workflow job queued, waiting, i
24342434
settings.event_package = Package
24352435
settings.event_package_desc = Package created or deleted in a repository.
24362436
settings.branch_filter = Branch filter
2437-
settings.branch_filter_desc = Branch whitelist for push, branch creation and branch deletion events, specified as glob pattern. If empty or <code>*</code>, events for all branches are reported. See <a href="%[1]s">%[2]s</a> documentation for syntax. Examples: <code>master</code>, <code>{master,release*}</code>.
2437+
settings.branch_filter_desc_1 = Branch (and ref name) allowlist for push, branch creation and branch deletion events, specified as glob pattern. If empty or <code>*</code>, events for all branches and tags are reported.
2438+
settings.branch_filter_desc_2 = Use <code>refs/heads/</code> or <code>refs/tags/</code> prefix to match full ref names.
2439+
settings.branch_filter_desc_doc = See <a href="%[1]s">%[2]s</a> documentation for syntax.
24382440
settings.authorization_header = Authorization Header
24392441
settings.authorization_header_desc = Will be included as authorization header for requests when present. Examples: %s.
24402442
settings.active = Active

services/webhook/webhook.go

Lines changed: 25 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -8,7 +8,6 @@ import (
88
"errors"
99
"fmt"
1010
"net/http"
11-
"strings"
1211

1312
"code.gitea.io/gitea/models/db"
1413
repo_model "code.gitea.io/gitea/models/repo"
@@ -46,21 +45,25 @@ func IsValidHookTaskType(name string) bool {
4645
// hookQueue is a global queue of web hooks
4746
var hookQueue *queue.WorkerPoolQueue[int64]
4847

49-
// getPayloadBranch returns branch for hook event, if applicable.
50-
func getPayloadBranch(p api.Payloader) string {
48+
// getPayloadRef returns the full ref name for hook event, if applicable.
49+
func getPayloadRef(p api.Payloader) git.RefName {
5150
switch pp := p.(type) {
5251
case *api.CreatePayload:
53-
if pp.RefType == "branch" {
54-
return pp.Ref
52+
switch pp.RefType {
53+
case "branch":
54+
return git.RefNameFromBranch(pp.Ref)
55+
case "tag":
56+
return git.RefNameFromTag(pp.Ref)
5557
}
5658
case *api.DeletePayload:
57-
if pp.RefType == "branch" {
58-
return pp.Ref
59+
switch pp.RefType {
60+
case "branch":
61+
return git.RefNameFromBranch(pp.Ref)
62+
case "tag":
63+
return git.RefNameFromTag(pp.Ref)
5964
}
6065
case *api.PushPayload:
61-
if strings.HasPrefix(pp.Ref, git.BranchPrefix) {
62-
return pp.Ref[len(git.BranchPrefix):]
63-
}
66+
return git.RefName(pp.Ref)
6467
}
6568
return ""
6669
}
@@ -108,19 +111,22 @@ func enqueueHookTask(taskID int64) error {
108111
return nil
109112
}
110113

111-
func checkBranch(w *webhook_model.Webhook, branch string) bool {
112-
if w.BranchFilter == "" || w.BranchFilter == "*" {
114+
func checkBranchFilter(branchFilter string, ref git.RefName) bool {
115+
if branchFilter == "" || branchFilter == "*" || branchFilter == "**" {
113116
return true
114117
}
115118

116-
g, err := glob.Compile(w.BranchFilter)
119+
g, err := glob.Compile(branchFilter)
117120
if err != nil {
118121
// should not really happen as BranchFilter is validated
119-
log.Error("CheckBranch failed: %s", err)
122+
log.Debug("checkBranchFilter failed to compile filer %q, err: %s", branchFilter, err)
120123
return false
121124
}
122125

123-
return g.Match(branch)
126+
if ref.IsBranch() && g.Match(ref.BranchName()) {
127+
return true
128+
}
129+
return g.Match(ref.String())
124130
}
125131

126132
// PrepareWebhook creates a hook task and enqueues it for processing.
@@ -144,11 +150,10 @@ func PrepareWebhook(ctx context.Context, w *webhook_model.Webhook, event webhook
144150
return nil
145151
}
146152

147-
// If payload has no associated branch (e.g. it's a new tag, issue, etc.),
148-
// branch filter has no effect.
149-
if branch := getPayloadBranch(p); branch != "" {
150-
if !checkBranch(w, branch) {
151-
log.Info("Branch %q doesn't match branch filter %q, skipping", branch, w.BranchFilter)
153+
// If payload has no associated branch (e.g. it's a new tag, issue, etc.), branch filter has no effect.
154+
if ref := getPayloadRef(p); ref != "" {
155+
// Check the payload's git ref against the webhook's branch filter.
156+
if !checkBranchFilter(w.BranchFilter, ref) {
152157
return nil
153158
}
154159
}

services/webhook/webhook_test.go

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"code.gitea.io/gitea/models/unittest"
1111
user_model "code.gitea.io/gitea/models/user"
1212
webhook_model "code.gitea.io/gitea/models/webhook"
13+
"code.gitea.io/gitea/modules/git"
1314
"code.gitea.io/gitea/modules/setting"
1415
api "code.gitea.io/gitea/modules/structs"
1516
"code.gitea.io/gitea/modules/test"
@@ -90,3 +91,30 @@ func TestWebhookUserMail(t *testing.T) {
9091
assert.Equal(t, user.GetPlaceholderEmail(), convert.ToUser(t.Context(), user, nil).Email)
9192
assert.Equal(t, user.Email, convert.ToUser(t.Context(), user, user).Email)
9293
}
94+
95+
func TestCheckBranchFilter(t *testing.T) {
96+
cases := []struct {
97+
filter string
98+
ref git.RefName
99+
match bool
100+
}{
101+
{"", "any-ref", true},
102+
{"*", "any-ref", true},
103+
{"**", "any-ref", true},
104+
105+
{"main", git.RefNameFromBranch("main"), true},
106+
{"main", git.RefNameFromTag("main"), false},
107+
108+
{"feature/*", git.RefNameFromBranch("feature"), false},
109+
{"feature/*", git.RefNameFromBranch("feature/foo"), true},
110+
{"feature/*", git.RefNameFromTag("feature/foo"), false},
111+
112+
{"{refs/heads/feature/*,refs/tags/release/*}", git.RefNameFromBranch("feature/foo"), true},
113+
{"{refs/heads/feature/*,refs/tags/release/*}", git.RefNameFromBranch("main"), false},
114+
{"{refs/heads/feature/*,refs/tags/release/*}", git.RefNameFromTag("release/bar"), true},
115+
{"{refs/heads/feature/*,refs/tags/release/*}", git.RefNameFromTag("dev"), false},
116+
}
117+
for _, v := range cases {
118+
assert.Equal(t, v.match, checkBranchFilter(v.filter, v.ref), "filter: %q ref: %q", v.filter, v.ref)
119+
}
120+
}

templates/repo/settings/webhook/settings.tmpl

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,16 @@
4444
<div class="field">
4545
<label>{{ctx.Locale.Tr "repo.settings.branch_filter"}}</label>
4646
<input name="branch_filter" type="text" value="{{or .Webhook.BranchFilter "*"}}">
47-
<span class="help">{{ctx.Locale.Tr "repo.settings.branch_filter_desc" "https://pkg.go.dev/github.com/gobwas/glob#Compile" "github.com/gobwas/glob"}}</span>
47+
<span class="help">
48+
{{ctx.Locale.Tr "repo.settings.branch_filter_desc_1"}}
49+
{{ctx.Locale.Tr "repo.settings.branch_filter_desc_2"}}
50+
{{ctx.Locale.Tr "repo.settings.branch_filter_desc_doc" "https://pkg.go.dev/github.com/gobwas/glob#Compile" "github.com/gobwas/glob"}}
51+
<ul>
52+
<li><code>main</code></li>
53+
<li><code>{main,feature/*}</code></li>
54+
<li><code>{refs/heads/feature/*,refs/tags/release/*}</code></li>
55+
</ul>
56+
</span>
4857
</div>
4958

5059
<div class="field">

web_src/css/base.css

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,13 @@ samp,
101101
font-size: 0.95em; /* compensate for monospace fonts being usually slightly larger */
102102
}
103103

104+
code {
105+
padding: 1px 4px;
106+
border-radius: var(--border-radius);
107+
background-color: var(--color-label-bg);
108+
color: var(--color-label-text);
109+
}
110+
104111
b,
105112
strong,
106113
h1,
@@ -177,6 +184,11 @@ table {
177184
border-collapse: collapse;
178185
}
179186

187+
ul {
188+
margin: 0.5em 0;
189+
padding: 0 0 0 1.5em;
190+
}
191+
180192
button {
181193
cursor: pointer;
182194
}
@@ -603,11 +615,6 @@ img.ui.avatar,
603615
text-align: center;
604616
}
605617

606-
.ui .message > ul {
607-
margin: 0;
608-
padding: 0 1em;
609-
}
610-
611618
.ui .header > i + .content {
612619
padding-left: 0.75rem;
613620
vertical-align: middle;

web_src/css/form.css

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -218,11 +218,16 @@ textarea:focus,
218218

219219
.form .help {
220220
color: var(--color-secondary-dark-5);
221+
margin-top: 0.25em;
221222
padding-bottom: 0.6em;
222223
display: inline-block;
223224
text-wrap: balance;
224225
}
225226

227+
.form .help code {
228+
color: var(--color-text-light-1);
229+
}
230+
226231
.m-captcha-style {
227232
width: 100%;
228233
height: 5em;

web_src/css/repo/issue-list.css

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -84,10 +84,3 @@
8484
margin-right: 8px;
8585
text-align: left;
8686
}
87-
88-
.label-filter-exclude-info code {
89-
border: 1px solid var(--color-secondary);
90-
border-radius: var(--border-radius);
91-
padding: 1px 2px;
92-
font-size: 11px;
93-
}

0 commit comments

Comments
 (0)