Skip to content

[MM-69266] Fix false "No webhook was found" warning when only an org-… #1027

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
nang2049 merged 3 commits into from
Jun 23, 2026
+201 −35
Merged

[MM-69266] Fix false "No webhook was found" warning when only an org-… #1027

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6b4dab0
[MM-69266] Fix false "No webhook was found" warning when only an org-…
Jun 19, 2026
8a96b1b
Coderabbitai feedback
Jun 19, 2026
feb9e9d
PR feedback
Jun 22, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
105 changes: 70 additions & 35 deletions server/plugin/command.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,57 +409,92 @@ 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
}

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)

@avasconcelos114 avasconcelos114 Jun 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may have to skip this org fallback if the repository's owner is a user instead of an org, otherwise we may get a false-positive where the 404 response from the listOrgHooks request cascades into isWebhookListAccessError being true and returning true instead of leaving it to the final return found, nil which was going to be correctly marked as false since the user hadn't set up webhooks for their personal repo

@nang2049 nang2049 Jun 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@avasconcelos114 added

if err != nil {
if isWebhookListAccessError(err) {
return true, nil
}
return false, err
}
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 {
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", cErr.Error())
return false, cErr
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

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 == nil || 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 {
Expand Down
131 changes: 131 additions & 0 deletions server/plugin/command_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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
Expand Down
Loading