From 6b4dab09f6b3eef16a5f6a186395f9345873ffbe Mon Sep 17 00:00:00 2001 From: Nevyana Angelova Date: Fri, 19 Jun 2026 16:40:49 +0300 Subject: [PATCH 1/3] [MM-69266] Fix false "No webhook was found" warning when only an org-level webhook exists --- server/plugin/command.go | 86 ++++++++++++++++++++++++---------------- 1 file changed, 51 insertions(+), 35 deletions(-) diff --git a/server/plugin/command.go b/server/plugin/command.go index b8cb89350..8739413ec 100644 --- a/server/plugin/command.go +++ b/server/plugin/command.go @@ -409,57 +409,73 @@ func (p *Plugin) createPost(channelID, userID, message string) error { } func (p *Plugin) checkIfConfiguredWebhookExists(ctx context.Context, githubClient *github.Client, userInfo *GitHubUserInfo, repo, owner string) (bool, error) { - found := false - opt := &github.ListOptions{ - PerPage: PerPageValue, - } siteURL, err := getSiteURL(p.client) if err != nil { return false, err } - for { - var githubHooks []*github.Hook - var githubResponse *github.Response - var err, cErr error - - if repo == "" { - cErr = p.useGitHubClient(userInfo, func(info *GitHubUserInfo, token *oauth2.Token) error { - githubHooks, githubResponse, err = githubClient.Organizations.ListHooks(ctx, owner, opt) - if err != nil { - return err - } - return nil - }) - } else { - cErr = p.useGitHubClient(userInfo, func(info *GitHubUserInfo, token *oauth2.Token) error { - githubHooks, githubResponse, err = githubClient.Repositories.ListHooks(ctx, owner, repo, opt) - if err != nil { - return err - } - return nil - }) + listOrgHooks := func(opt *github.ListOptions) ([]*github.Hook, *github.Response, error) { + return githubClient.Organizations.ListHooks(ctx, owner, opt) + } + + if repo == "" { + return p.anyHookMatchesSiteURL(userInfo, owner, repo, siteURL, listOrgHooks) + } + + found, err := p.anyHookMatchesSiteURL(userInfo, owner, repo, siteURL, func(opt *github.ListOptions) ([]*github.Hook, *github.Response, error) { + return githubClient.Repositories.ListHooks(ctx, owner, repo, opt) + }) + if err != nil { + return false, err + } + if found { + return true, nil + } + + found, err = p.anyHookMatchesSiteURL(userInfo, owner, repo, siteURL, listOrgHooks) + if err != nil { + if isWebhookListAccessError(err) { + return true, nil } + return false, err + } + return found, nil +} +func (p *Plugin) anyHookMatchesSiteURL(userInfo *GitHubUserInfo, owner, repo, siteURL string, list func(opt *github.ListOptions) ([]*github.Hook, *github.Response, error)) (bool, error) { + opt := &github.ListOptions{PerPage: PerPageValue} + for { + var hooks []*github.Hook + var resp *github.Response + var listErr error + cErr := p.useGitHubClient(userInfo, func(_ *GitHubUserInfo, _ *oauth2.Token) error { + hooks, resp, listErr = list(opt) + return listErr + }) if cErr != nil { - p.client.Log.Warn("Not able to get the list of webhooks", "Owner", owner, "Repo", repo, "error", err.Error()) - return found, err + p.client.Log.Warn("Not able to get the list of webhooks", "Owner", owner, "Repo", repo, "error", listErr.Error()) + return false, listErr } - for _, hook := range githubHooks { - if strings.Contains(hook.Config["url"].(string), siteURL) { - found = true - break + for _, hook := range hooks { + if url, ok := hook.Config["url"].(string); ok && strings.Contains(url, siteURL) { + return true, nil } } - if githubResponse.NextPage == 0 { - break + if resp.NextPage == 0 { + return false, nil } - opt.Page = githubResponse.NextPage + opt.Page = resp.NextPage } +} - return found, nil +func isWebhookListAccessError(err error) bool { + if err == nil { + return false + } + msg := err.Error() + return strings.Contains(msg, "404 Not Found") || strings.Contains(msg, "403 Forbidden") } func (p *Plugin) handleSubscribesAdd(_ *plugin.Context, args *model.CommandArgs, parameters []string, userInfo *GitHubUserInfo) string { From 8a96b1bf0112d6430482db4de036c8c27269db51 Mon Sep 17 00:00:00 2001 From: Nevyana Angelova Date: Fri, 19 Jun 2026 17:12:46 +0300 Subject: [PATCH 2/3] Coderabbitai feedback --- server/plugin/command.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/server/plugin/command.go b/server/plugin/command.go index 8739413ec..db4512b60 100644 --- a/server/plugin/command.go +++ b/server/plugin/command.go @@ -453,8 +453,8 @@ func (p *Plugin) anyHookMatchesSiteURL(userInfo *GitHubUserInfo, owner, repo, si return listErr }) if cErr != nil { - p.client.Log.Warn("Not able to get the list of webhooks", "Owner", owner, "Repo", repo, "error", listErr.Error()) - return false, listErr + p.client.Log.Warn("Not able to get the list of webhooks", "Owner", owner, "Repo", repo, "error", cErr.Error()) + return false, cErr } for _, hook := range hooks { @@ -463,7 +463,7 @@ func (p *Plugin) anyHookMatchesSiteURL(userInfo *GitHubUserInfo, owner, repo, si } } - if resp.NextPage == 0 { + if resp == nil || resp.NextPage == 0 { return false, nil } opt.Page = resp.NextPage From feb9e9d44043ca25ea74987d134ffb4736528a3c Mon Sep 17 00:00:00 2001 From: Nevyana Angelova Date: Mon, 22 Jun 2026 18:31:09 +0300 Subject: [PATCH 3/3] PR feedback --- server/plugin/command.go | 19 +++++ server/plugin/command_test.go | 131 ++++++++++++++++++++++++++++++++++ 2 files changed, 150 insertions(+) diff --git a/server/plugin/command.go b/server/plugin/command.go index db4512b60..601cb268b 100644 --- a/server/plugin/command.go +++ b/server/plugin/command.go @@ -432,6 +432,14 @@ func (p *Plugin) checkIfConfiguredWebhookExists(ctx context.Context, githubClien return true, nil } + isOrg, err := p.isOrganization(ctx, githubClient, owner) + if err != nil { + return false, err + } + if !isOrg { + return false, nil + } + found, err = p.anyHookMatchesSiteURL(userInfo, owner, repo, siteURL, listOrgHooks) if err != nil { if isWebhookListAccessError(err) { @@ -442,6 +450,17 @@ func (p *Plugin) checkIfConfiguredWebhookExists(ctx context.Context, githubClien return found, nil } +func (p *Plugin) isOrganization(ctx context.Context, githubClient *github.Client, owner string) (bool, error) { + _, resp, err := githubClient.Organizations.Get(ctx, owner) + if err != nil { + if resp != nil && resp.StatusCode == http.StatusNotFound { + return false, nil + } + return false, err + } + return true, nil +} + func (p *Plugin) anyHookMatchesSiteURL(userInfo *GitHubUserInfo, owner, repo, siteURL string, list func(opt *github.ListOptions) ([]*github.Hook, *github.Response, error)) (bool, error) { opt := &github.ListOptions{PerPage: PerPageValue} for { diff --git a/server/plugin/command_test.go b/server/plugin/command_test.go index 21add9942..f64f76b36 100644 --- a/server/plugin/command_test.go +++ b/server/plugin/command_test.go @@ -4,12 +4,18 @@ package plugin import ( + "context" "fmt" + "net/http" + "net/http/httptest" + "net/url" "os" "path/filepath" + "sync/atomic" "testing" "github.com/golang/mock/gomock" + "github.com/google/go-github/v54/github" "github.com/mattermost/mattermost/server/public/model" "github.com/mattermost/mattermost/server/public/plugin" "github.com/mattermost/mattermost/server/public/plugin/plugintest" @@ -1784,6 +1790,131 @@ func TestFormattedString(t *testing.T) { } } +func TestCheckIfConfiguredWebhookExists(t *testing.T) { + const ( + siteURL = "https://example.com" + owner = "test-owner" + repo = "test-repo" + ) + + newGitHubClient := func(t *testing.T, server *httptest.Server) *github.Client { + t.Helper() + client := github.NewClient(nil) + base, err := url.Parse(server.URL + "/") + require.NoError(t, err) + client.BaseURL = base + client.UploadURL = base + return client + } + + configureSiteURL := func(api *plugintest.API) { + siteURLCopy := siteURL + api.On("GetConfig").Return(&model.Config{ + ServiceSettings: model.ServiceSettings{SiteURL: &siteURLCopy}, + }) + } + + t.Run("user-owned repo skips org-hooks fallback and returns false", func(t *testing.T) { + mockKvStore, mockAPI, _, _, _ := GetTestSetup(t) + p := getPluginTest(mockAPI, mockKvStore) + userInfo, err := GetMockGHUserInfo(p) + require.NoError(t, err) + configureSiteURL(mockAPI) + + var orgHooksCalls int32 + mux := http.NewServeMux() + mux.HandleFunc(fmt.Sprintf("/repos/%s/%s/hooks", owner, repo), func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte("[]")) + }) + mux.HandleFunc(fmt.Sprintf("/orgs/%s", owner), func(w http.ResponseWriter, _ *http.Request) { + http.Error(w, `{"message":"Not Found"}`, http.StatusNotFound) + }) + mux.HandleFunc(fmt.Sprintf("/orgs/%s/hooks", owner), func(w http.ResponseWriter, _ *http.Request) { + atomic.AddInt32(&orgHooksCalls, 1) + http.Error(w, `{"message":"Not Found"}`, http.StatusNotFound) + }) + server := httptest.NewServer(mux) + defer server.Close() + + found, err := p.checkIfConfiguredWebhookExists(context.Background(), newGitHubClient(t, server), userInfo, repo, owner) + require.NoError(t, err) + assert.False(t, found) + assert.Zero(t, atomic.LoadInt32(&orgHooksCalls), "org-hooks endpoint must not be called for user-owned repos") + }) + + t.Run("repo-level webhook matches site URL", func(t *testing.T) { + mockKvStore, mockAPI, _, _, _ := GetTestSetup(t) + p := getPluginTest(mockAPI, mockKvStore) + userInfo, err := GetMockGHUserInfo(p) + require.NoError(t, err) + configureSiteURL(mockAPI) + + mux := http.NewServeMux() + mux.HandleFunc(fmt.Sprintf("/repos/%s/%s/hooks", owner, repo), func(w http.ResponseWriter, _ *http.Request) { + _, _ = fmt.Fprintf(w, `[{"config":{"url":"%s/plugins/github/webhook"}}]`, siteURL) + }) + server := httptest.NewServer(mux) + defer server.Close() + + found, err := p.checkIfConfiguredWebhookExists(context.Background(), newGitHubClient(t, server), userInfo, repo, owner) + require.NoError(t, err) + assert.True(t, found) + }) + + t.Run("org-owned repo falls back to org-hooks and finds a match", func(t *testing.T) { + mockKvStore, mockAPI, _, _, _ := GetTestSetup(t) + p := getPluginTest(mockAPI, mockKvStore) + userInfo, err := GetMockGHUserInfo(p) + require.NoError(t, err) + configureSiteURL(mockAPI) + + mux := http.NewServeMux() + mux.HandleFunc(fmt.Sprintf("/repos/%s/%s/hooks", owner, repo), func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte("[]")) + }) + mux.HandleFunc(fmt.Sprintf("/orgs/%s", owner), func(w http.ResponseWriter, _ *http.Request) { + _, _ = fmt.Fprintf(w, `{"login":"%s","type":"Organization"}`, owner) + }) + mux.HandleFunc(fmt.Sprintf("/orgs/%s/hooks", owner), func(w http.ResponseWriter, _ *http.Request) { + _, _ = fmt.Fprintf(w, `[{"config":{"url":"%s/plugins/github/webhook"}}]`, siteURL) + }) + server := httptest.NewServer(mux) + defer server.Close() + + found, err := p.checkIfConfiguredWebhookExists(context.Background(), newGitHubClient(t, server), userInfo, repo, owner) + require.NoError(t, err) + assert.True(t, found) + }) + + t.Run("org-owned repo treats org-hooks access error as configured", func(t *testing.T) { + mockKvStore, mockAPI, _, _, _ := GetTestSetup(t) + p := getPluginTest(mockAPI, mockKvStore) + userInfo, err := GetMockGHUserInfo(p) + require.NoError(t, err) + configureSiteURL(mockAPI) + mockAPI.On("LogWarn", "Not able to get the list of webhooks", "Owner", owner, "Repo", repo, "error", mock.Anything).Maybe() + mockAPI.On("LogWarn", "Error occurred while using the Github client", "error", mock.Anything).Maybe() + + mux := http.NewServeMux() + mux.HandleFunc(fmt.Sprintf("/repos/%s/%s/hooks", owner, repo), func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write([]byte("[]")) + }) + mux.HandleFunc(fmt.Sprintf("/orgs/%s", owner), func(w http.ResponseWriter, _ *http.Request) { + _, _ = fmt.Fprintf(w, `{"login":"%s","type":"Organization"}`, owner) + }) + mux.HandleFunc(fmt.Sprintf("/orgs/%s/hooks", owner), func(w http.ResponseWriter, _ *http.Request) { + w.WriteHeader(http.StatusForbidden) + _, _ = w.Write([]byte(`{"message":"Forbidden"}`)) + }) + server := httptest.NewServer(mux) + defer server.Close() + + found, err := p.checkIfConfiguredWebhookExists(context.Background(), newGitHubClient(t, server), userInfo, repo, owner) + require.NoError(t, err) + assert.True(t, found, "403 from org-hooks should be treated as access error and assumed configured") + }) +} + func TestToSlice(t *testing.T) { tests := []struct { name string