control: a refused webhook URL is a failure, not a usage error !333

merged merged by cmc on 2026-09-07 19:19 UTC · krz/gitbay:webhook-refusal-exit-187 into main

3 files changed, +48 −4

Layout: unified · split

e2e/webhook_test.go +4 −3

 	recv.waitN(t, prev+1)
 	// SSRF: on a default instance (allow_local off), local targets are
-	// rejected at add time.
+	// rejected at add time. A refused value is exit 1 with the reason;
+	// exit 2 is for the shape of the command line (#187).
 	inst2 := startInstance(t)
 	k2 := inst2.newKey(t, "a2")
 	inst2.admin(t, "admin", "user", "create", "a2", "--key", k2+".pub")
 		t.Fatal("repo create failed")
 	}
 	_, errOut, code := inst2.ssh(t, k2, "", "webhook", "add", "a2/r", "http://127.0.0.1:9/x")
-	if code != 2 || !strings.Contains(errOut, "SSRF") {
+	if code != 1 || !strings.Contains(errOut, "SSRF") {
 		t.Fatalf("local webhook target accepted: exit %d, %s", code, errOut)
 	}
-	if _, _, code := inst2.ssh(t, k2, "", "webhook", "add", "a2/r", "ftp://example.com/x"); code != 2 {
+	if _, _, code := inst2.ssh(t, k2, "", "webhook", "add", "a2/r", "ftp://example.com/x"); code != 1 {
 		t.Fatal("non-http scheme accepted")
 	}
 }

internal/control/webhook.go +3 −1

 		return code
 	}
 	if err := webhook.ValidateURL(url, c.Cfg.Webhooks.AllowLocal); err != nil {
-		return c.failErr(err)
+		// The command line parsed; the value is what the server refuses.
+		// Exit 1 carries the reason to every client verbatim (#187).
+		return c.fail(protocol.ExitFailure, "%v", err)
 	}
 	id, err := c.Store.AddWebhook(repo.ID, url, secret, events)
 	if err != nil {

internal/control/webhook_test.go added +41

@@ -0,0 +1,41 @@
	1	package control
	2
	3	import (
	4	"bytes"
	5	"strings"
	6	"testing"
	7
	8	"gitbay.org/gitbay/internal/protocol"
	9	"gitbay.org/gitbay/internal/store"
	10	)
	11
	12	// TestWebhookAddRefusedURLIsAFailure: a URL the server refuses after
	13	// parsing it — here one that resolves to a loopback address — is the
	14	// caller's value being rejected, not a malformed command line. Exit 1
	15	// carries the sentence verbatim to every client; exit 2 reads as an
	16	// app bug and is shown as a generic error (#187).
	17	func TestWebhookAddRefusedURLIsAFailure(t *testing.T) {
	18	st, repo, uid := newQueueTestRepo(t)
	19	var out, errOut bytes.Buffer
	20	c := &Ctx{
	21	User: store.User{ID: uid, Username: "alice"},
	22	Store: st,
	23	Scope: "full",
	24	Stdin: strings.NewReader(""),
	25	Stdout: &out,
	26	Stderr: &errOut,
	27	}
	28	code := Dispatch(c, []string{"webhook", "add", repo.Path(), "http://127.0.0.1/hook"})
	29	if code != protocol.ExitFailure {
	30	t.Fatalf("exit %d, want %d (%s)", code, protocol.ExitFailure, strings.TrimSpace(errOut.String()))
	31	}
	32	if !strings.Contains(errOut.String(), "private or local address") {
	33	t.Errorf("message %q lacks the server's reason", strings.TrimSpace(errOut.String()))
	34	}
	35	// The shape of the command line is still a usage error.
	36	out.Reset()
	37	errOut.Reset()
	38	if code := Dispatch(c, []string{"webhook", "add", repo.Path()}); code != protocol.ExitUsage {
	39	t.Errorf("missing url: exit %d, want %d", code, protocol.ExitUsage)
	40	}
	41	}