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
| @@ -239,7 +239,8 @@ func TestWebhooks(t *testing.T) { |
| 239 | 239 | recv.waitN(t, prev+1) |
| 240 | 240 | |
| 241 | 241 | // SSRF: on a default instance (allow_local off), local targets are |
| 242 | | // rejected at add time. |
| 242 | // rejected at add time. A refused value is exit 1 with the reason; |
| 243 | // exit 2 is for the shape of the command line (#187). |
| 243 | 244 | inst2 := startInstance(t) |
| 244 | 245 | k2 := inst2.newKey(t, "a2") |
| 245 | 246 | inst2.admin(t, "admin", "user", "create", "a2", "--key", k2+".pub") |
| @@ -247,10 +248,10 @@ func TestWebhooks(t *testing.T) { |
| 247 | 248 | t.Fatal("repo create failed") |
| 248 | 249 | } |
| 249 | 250 | _, errOut, code := inst2.ssh(t, k2, "", "webhook", "add", "a2/r", "http://127.0.0.1:9/x") |
| 250 | | if code != 2 || !strings.Contains(errOut, "SSRF") { |
| 251 | if code != 1 || !strings.Contains(errOut, "SSRF") { |
| 251 | 252 | t.Fatalf("local webhook target accepted: exit %d, %s", code, errOut) |
| 252 | 253 | } |
| 253 | | if _, _, code := inst2.ssh(t, k2, "", "webhook", "add", "a2/r", "ftp://example.com/x"); code != 2 { |
| 254 | if _, _, code := inst2.ssh(t, k2, "", "webhook", "add", "a2/r", "ftp://example.com/x"); code != 1 { |
| 254 | 255 | t.Fatal("non-http scheme accepted") |
| 255 | 256 | } |
| 256 | 257 | } |
internal/control/webhook.go
+3 −1
| @@ -53,7 +53,9 @@ func runWebhookAdd(c *Ctx, args []string) int { |
| 53 | 53 | return code |
| 54 | 54 | } |
| 55 | 55 | if err := webhook.ValidateURL(url, c.Cfg.Webhooks.AllowLocal); err != nil { |
| 56 | | return c.failErr(err) |
| 56 | // The command line parsed; the value is what the server refuses. |
| 57 | // Exit 1 carries the reason to every client verbatim (#187). |
| 58 | return c.fail(protocol.ExitFailure, "%v", err) |
| 57 | 59 | } |
| 58 | 60 | id, err := c.Store.AddWebhook(repo.ID, url, secret, events) |
| 59 | 61 | 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 | } |