webhook: use regexp.QuoteMeta for hostname and path (#5145)

Author: p-se

pkg/webhook/webhook.go

@@ -105,10 +105,8 @@ func (w *Webhook) ServeHTTP(rw http.ResponseWriter, r *http.Request) {
 			w.logAndReturn(rw, err)
 			return
 		}
-
-		path := strings.Replace(u.EscapedPath()[1:], "/_git/", "(/_git)?/", 1)
-
-		regexpStr := `(?i)(http://|https://|\w+@|ssh://(\w+@)?|git@(ssh\.)?)` + u.Hostname() +
+		path := strings.Replace(regexp.QuoteMeta(u.EscapedPath()[1:]), `/_git/`, `(/_git)?/`, 1)
+		regexpStr := `(?i)(http://|https://|\w+@|ssh://(\w+@)?|git@(ssh\.)?)` + regexp.QuoteMeta(u.Hostname()) +
 			"(:[0-9]+|)[:/](v\\d/)?" + path + "(\\.git)?$"
 		repoRegexp, err := regexp.Compile(regexpStr)
 		if err != nil {
@@ -212,7 +210,7 @@ func HandleHooks(ctx context.Context, namespace string, client client.Client, cl
 func (w *Webhook) logAndReturn(rw http.ResponseWriter, err error) {
 	w.log.Error(err, "Webhook processing failed")
 	rw.WriteHeader(getErrorCodeFromErr(err))
-	_, _ = rw.Write([]byte(err.Error()))
+	_, _ = rw.Write([]byte(err.Error())) //nolint:gosec // G705: error message is not user-controlled
 }
 
 func (w *Webhook) getSecret(ctx context.Context, gitrepo fleet.GitRepo) (*corev1.Secret, error) {

pkg/webhook/webhook_test.go

@@ -874,7 +874,7 @@ func TestErrorReadingRequest(t *testing.T) {
 		client:    mockClient,
 		namespace: "default",
 	}
-	testRequest := httptest.NewRequest(http.MethodPost, "/something", errReader(0))
+	testRequest := httptest.NewRequestWithContext(context.Background(), http.MethodPost, "/something", errReader(0))
 	rr := httptest.NewRecorder()
 	w.ServeHTTP(rr, testRequest)
 
@@ -883,3 +883,83 @@ func TestErrorReadingRequest(t *testing.T) {
 		t.Errorf("handler returned wrong status code: got %v want %v", status, http.StatusInternalServerError)
 	}
 }
+
+// TestGitHubWildcardURLDoesNotMatchUnintendedRepo verifies that regex metacharacters
+// in a webhook payload's repo URL (e.g. a hostname or path of ".*") are treated as
+// literals and do not accidentally match GitRepos whose URLs are unrelated to the
+// webhook source.
+func TestGitHubWildcardURLDoesNotMatchUnintendedRepo(t *testing.T) {
+	tests := []struct {
+		name           string
+		gitRepoURL     string // the GitRepo's configured repo
+		webhookRepoURL string // the repo URL embedded in the webhook payload
+	}{
+		{
+			name:           "wildcard hostname does not match unrelated gitrepo",
+			gitRepoURL:     "https://github.com/example/repo",
+			webhookRepoURL: "https://.*/example/repo",
+		},
+		{
+			name:           "wildcard path does not match unrelated gitrepo",
+			gitRepoURL:     "https://github.com/example/repo",
+			webhookRepoURL: "https://github.com/.*/.*",
+		},
+		{
+			name:           "wildcard hostname and path do not match unrelated gitrepo",
+			gitRepoURL:     "https://github.com/example/repo",
+			webhookRepoURL: "https://.*/.*",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			sch := runtime.NewScheme()
+			if err := v1alpha1.AddToScheme(sch); err != nil {
+				t.Fatalf("unable to add to scheme: %v", err)
+			}
+			utilruntime.Must(corev1.AddToScheme(sch))
+
+			gitRepo := &v1alpha1.GitRepo{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test",
+					Namespace: "default",
+				},
+				Spec: v1alpha1.GitRepoSpec{
+					Repo:   tt.gitRepoURL,
+					Branch: "main",
+				},
+			}
+			fakeClient := cfake.NewClientBuilder().
+				WithScheme(sch).
+				WithRuntimeObjects(gitRepo).
+				WithStatusSubresource(gitRepo).
+				Build()
+
+			w := &Webhook{client: fakeClient, namespace: "default"}
+
+			commit := "af69d162de5a276abc86e0686b2b44033cd3f442"
+			jsonBody := fmt.Appendf(nil, `{
+				"ref": "refs/heads/main",
+				"after": "%s",
+				"repository": {"html_url": "%s"}
+			}`, commit, tt.webhookRepoURL)
+
+			req, err := http.NewRequestWithContext(context.Background(), http.MethodPost, "/", bytes.NewReader(jsonBody))
+			if err != nil {
+				t.Fatalf("failed to create request: %v", err)
+			}
+			req.Header.Set("X-GitHub-Event", "push")
+
+			rr := httptest.NewRecorder()
+			w.ServeHTTP(rr, req)
+
+			updated := &v1alpha1.GitRepo{}
+			if err := fakeClient.Get(context.TODO(), types.NamespacedName{Name: gitRepo.Name, Namespace: gitRepo.Namespace}, updated); err != nil {
+				t.Fatalf("failed to get gitrepo: %v", err)
+			}
+			if updated.Status.WebhookCommit != "" {
+				t.Errorf("expected WebhookCommit to remain empty (no match), but got %q", updated.Status.WebhookCommit)
+			}
+		})
+	}
+}