From 20a46e86914c7f9c603c2fa8f96d9ff06bb27d11 Mon Sep 17 00:00:00 2001 From: Marius van Niekerk Date: Fri, 6 Mar 2026 13:17:20 -0500 Subject: [PATCH 1/6] Add webhook hook support --- README.md | 10 ++++ internal/config/config.go | 3 +- internal/daemon/broadcaster.go | 2 + internal/daemon/event_test.go | 2 + internal/daemon/hooks.go | 50 ++++++++++++++++ internal/daemon/hooks_test.go | 102 +++++++++++++++++++++++++++++++++ 6 files changed, 168 insertions(+), 1 deletion(-) diff --git a/README.md b/README.md index 59d004bf..7aa1bf0e 100644 --- a/README.md +++ b/README.md @@ -176,6 +176,16 @@ command = "notify-send 'Review done for {repo_name} ({sha})'" Template variables: `{job_id}`, `{repo}`, `{repo_name}`, `{sha}`, `{verdict}`, `{error}`. +For webhook destinations like Slack, Discord, or Teams, use the built-in +`webhook` hook type to POST the review event JSON directly: + +```toml +[[hooks]] +event = "review.completed" +type = "webhook" +url = "https://hooks.slack.com/services/T.../B.../xxx" +``` + ### Beads Integration The built-in `beads` hook type creates [beads](https://github.com/steveyegge/beads) issues diff --git a/internal/config/config.go b/internal/config/config.go index 6e641d7c..e83b9307 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -40,7 +40,8 @@ func IsConfigParseError(err error) bool { type HookConfig struct { Event string `toml:"event"` // "review.failed", "review.completed", "review.*" Command string `toml:"command"` // shell command with {var} templates - Type string `toml:"type"` // "beads" for built-in, empty for command + Type string `toml:"type"` // "beads" or "webhook"; empty or "command" runs Command + URL string `toml:"url"` // webhook destination URL when Type is "webhook" } type AdvancedConfig struct { diff --git a/internal/daemon/broadcaster.go b/internal/daemon/broadcaster.go index 724c5ea8..ff88162a 100644 --- a/internal/daemon/broadcaster.go +++ b/internal/daemon/broadcaster.go @@ -120,6 +120,7 @@ func (e Event) MarshalJSON() ([]byte, error) { SHA string `json:"sha"` Agent string `json:"agent,omitempty"` Verdict string `json:"verdict,omitempty"` + Findings string `json:"findings,omitempty"` Error string `json:"error,omitempty"` }{ Type: e.Type, @@ -130,6 +131,7 @@ func (e Event) MarshalJSON() ([]byte, error) { SHA: e.SHA, Agent: e.Agent, Verdict: e.Verdict, + Findings: e.Findings, Error: e.Error, }) } diff --git a/internal/daemon/event_test.go b/internal/daemon/event_test.go index c8ef2d00..2638fdef 100644 --- a/internal/daemon/event_test.go +++ b/internal/daemon/event_test.go @@ -16,6 +16,7 @@ func TestEvent_MarshalJSON(t *testing.T) { SHA: "abc123", Agent: "claude-code", Verdict: "F", + Findings: "Missing input validation", } data, err := event.MarshalJSON() @@ -40,6 +41,7 @@ func TestEvent_MarshalJSON(t *testing.T) { {"sha", "abc123"}, {"agent", "claude-code"}, {"verdict", "F"}, + {"findings", "Missing input validation"}, } if len(decoded) != len(tests) { diff --git a/internal/daemon/hooks.go b/internal/daemon/hooks.go index ad70877c..faced617 100644 --- a/internal/daemon/hooks.go +++ b/internal/daemon/hooks.go @@ -1,13 +1,18 @@ package daemon import ( + "bytes" + "encoding/json" "fmt" + "io" "log" + "net/http" "os/exec" "path/filepath" "runtime" "strings" "sync" + "time" "github.com/roborev-dev/roborev/internal/config" gitpkg "github.com/roborev-dev/roborev/internal/git" @@ -144,6 +149,17 @@ func (hr *HookRunner) handleEvent(event Event) { continue } + if hook.Type == "webhook" { + if hook.URL == "" { + continue + } + + fired++ + hr.wg.Add(1) + go hr.postWebhook(hook.URL, event) + continue + } + cmd := resolveCommand(hook, event) if cmd == "" { continue @@ -270,3 +286,37 @@ func (hr *HookRunner) runHook(command, workDir string) { hr.logger.Printf("Hook output (cmd=%q): %s", command, output) } } + +func (hr *HookRunner) postWebhook(url string, event Event) { + defer hr.wg.Done() + + payload, err := json.Marshal(event) + if err != nil { + hr.logger.Printf("Webhook error (url=%q): marshal event: %v", url, err) + return + } + + req, err := http.NewRequest(http.MethodPost, url, bytes.NewReader(payload)) + if err != nil { + hr.logger.Printf("Webhook error (url=%q): build request: %v", url, err) + return + } + req.Header.Set("Content-Type", "application/json") + + client := &http.Client{Timeout: 5 * time.Second} + resp, err := client.Do(req) + if err != nil { + hr.logger.Printf("Webhook error (url=%q): %v", url, err) + return + } + defer resp.Body.Close() + + if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusMultipleChoices { + body, _ := io.ReadAll(io.LimitReader(resp.Body, 1024)) + if len(body) > 0 { + hr.logger.Printf("Webhook error (url=%q): status %s: %s", url, resp.Status, strings.TrimSpace(string(body))) + return + } + hr.logger.Printf("Webhook error (url=%q): status %s", url, resp.Status) + } +} diff --git a/internal/daemon/hooks_test.go b/internal/daemon/hooks_test.go index 931dad6e..25924cc7 100644 --- a/internal/daemon/hooks_test.go +++ b/internal/daemon/hooks_test.go @@ -2,8 +2,11 @@ package daemon import ( "bytes" + "encoding/json" "fmt" "log" + "net/http" + "net/http/httptest" "os" "path/filepath" "runtime" @@ -512,6 +515,105 @@ func TestHookRunnerNoMatchDoesNotFire(t *testing.T) { } } +func TestHookRunnerWebhookPostsEventJSON(t *testing.T) { + type webhookRequest struct { + header http.Header + event map[string]any + } + + reqCh := make(chan webhookRequest, 1) + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + defer r.Body.Close() + + var payload map[string]any + if err := json.NewDecoder(r.Body).Decode(&payload); err != nil { + t.Errorf("decode webhook payload: %v", err) + w.WriteHeader(http.StatusBadRequest) + return + } + + reqCh <- webhookRequest{header: r.Header.Clone(), event: payload} + w.WriteHeader(http.StatusNoContent) + })) + defer server.Close() + + cfg := &config.Config{ + Hooks: []config.HookConfig{ + {Event: "review.completed", Type: "webhook", URL: server.URL}, + }, + } + + hr, broadcaster := setupRunner(t, cfg) + broadcaster.Broadcast(Event{ + Type: "review.completed", + TS: time.Date(2026, 2, 25, 14, 30, 0, 0, time.UTC), + JobID: 42, + Repo: "/Users/wesm/code/roborev", + RepoName: "roborev", + SHA: "abc123def456", + Agent: "claude-code", + Verdict: "F", + Findings: "Missing validation in handler", + }) + + hr.WaitUntilIdle() + + select { + case req := <-reqCh: + if got := req.header.Get("Content-Type"); got != "application/json" { + t.Fatalf("content-type = %q, want application/json", got) + } + if got := req.event["type"]; got != "review.completed" { + t.Fatalf("type = %v, want review.completed", got) + } + if got := req.event["ts"]; got != "2026-02-25T14:30:00Z" { + t.Fatalf("ts = %v, want 2026-02-25T14:30:00Z", got) + } + if got := req.event["job_id"]; got != float64(42) { + t.Fatalf("job_id = %v, want 42", got) + } + if got := req.event["findings"]; got != "Missing validation in handler" { + t.Fatalf("findings = %v, want webhook payload to include findings", got) + } + case <-time.After(2 * time.Second): + t.Fatal("timed out waiting for webhook request") + } +} + +func TestHookRunnerWebhookLogsHTTPError(t *testing.T) { + var buf bytes.Buffer + logger := log.New(&buf, "", 0) + + server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + http.Error(w, "boom", http.StatusBadGateway) + })) + defer server.Close() + + cfg := &config.Config{ + Hooks: []config.HookConfig{ + {Event: "review.failed", Type: "webhook", URL: server.URL}, + }, + } + + hr := &HookRunner{cfgGetter: NewStaticConfig(cfg), logger: logger} + hr.handleEvent(Event{ + Type: "review.failed", + JobID: 7, + Repo: t.TempDir(), + Error: "agent timeout", + }) + + hr.wg.Wait() + + logOutput := buf.String() + if !strings.Contains(logOutput, "Webhook error") { + t.Fatalf("expected webhook error log, got %q", logOutput) + } + if !strings.Contains(logOutput, "502 Bad Gateway") { + t.Fatalf("expected HTTP status in log, got %q", logOutput) + } +} + func TestHooksSliceNotAliased(t *testing.T) { // Verify that repo hooks don't leak into the global config's Hooks slice From 9b8c7847e04ecbccd78bea9bc790ed356c97b0cf Mon Sep 17 00:00:00 2001 From: Marius van Niekerk Date: Fri, 6 Mar 2026 13:24:58 -0500 Subject: [PATCH 2/6] Address review findings (job 686) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I’m checking the current workspace state around the webhook docs and hook implementation first so I can make the smallest change that resolves the review finding. The implementation is generic JSON, so the smallest correct fix is to narrow the docs rather than add provider-specific payload support. I’m searching for any other Slack/Discord/Teams claims before editing. I found a single bad product claim in the docs. I’m updating the README to describe the webhook hook as generic JSON only, which matches the current implementation and avoids implying Slack/Discord/Teams compatibility. The code path didn’t need changes; only the README claim was incorrect. I’m running the required build and full test pass now to verify the tree is still clean. Build and test are still running. I’m polling both so I can catch failures immediately rather than waiting for a final timeout. The build finished cleanly. Tests are still progressing, so I’m waiting for the final result. Updated [README.md](/var/folders/7d/d85m5sks7r5f05fg4m3tlcb80000gn/T/roborev-worktree-2035613411/README.md) to describe `type = "webhook"` as a generic JSON webhook integration and replaced the Slack-specific example URL with a neutral endpoint, which brings the docs back in line with the current implementation. Verification: `GOCACHE=/tmp/go-build go build ./...` passed and `GOCACHE=/tmp/go-build go test ./...` passed. Changes: - Clarified the webhook hook docs to say it posts generic review event JSON, not Slack/Discord/Teams-specific payloads --- README.md | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/README.md b/README.md index 7aa1bf0e..5d628a1b 100644 --- a/README.md +++ b/README.md @@ -176,14 +176,14 @@ command = "notify-send 'Review done for {repo_name} ({sha})'" Template variables: `{job_id}`, `{repo}`, `{repo_name}`, `{sha}`, `{verdict}`, `{error}`. -For webhook destinations like Slack, Discord, or Teams, use the built-in -`webhook` hook type to POST the review event JSON directly: +For generic JSON webhook endpoints, use the built-in `webhook` hook type to +POST the review event directly: ```toml [[hooks]] event = "review.completed" type = "webhook" -url = "https://hooks.slack.com/services/T.../B.../xxx" +url = "https://example.com/roborev-webhook" ``` ### Beads Integration From 9baf70bce764d21b5b3ce6cad0d55d34ec67c4c7 Mon Sep 17 00:00:00 2001 From: Marius van Niekerk Date: Fri, 6 Mar 2026 17:52:16 -0500 Subject: [PATCH 3/6] Redact webhook URLs in hook logs --- internal/daemon/hooks.go | 39 +++++++++++++++++++++----- internal/daemon/hooks_test.go | 53 ++++++++++++++++++++++++++++++++++- 2 files changed, 84 insertions(+), 8 deletions(-) diff --git a/internal/daemon/hooks.go b/internal/daemon/hooks.go index faced617..ad28194d 100644 --- a/internal/daemon/hooks.go +++ b/internal/daemon/hooks.go @@ -7,6 +7,7 @@ import ( "io" "log" "net/http" + neturl "net/url" "os/exec" "path/filepath" "runtime" @@ -287,18 +288,19 @@ func (hr *HookRunner) runHook(command, workDir string) { } } -func (hr *HookRunner) postWebhook(url string, event Event) { +func (hr *HookRunner) postWebhook(webhookURL string, event Event) { defer hr.wg.Done() + safeURL := redactWebhookURL(webhookURL) payload, err := json.Marshal(event) if err != nil { - hr.logger.Printf("Webhook error (url=%q): marshal event: %v", url, err) + hr.logger.Printf("Webhook error (url=%q): marshal event: %v", safeURL, err) return } - req, err := http.NewRequest(http.MethodPost, url, bytes.NewReader(payload)) + req, err := http.NewRequest(http.MethodPost, webhookURL, bytes.NewReader(payload)) if err != nil { - hr.logger.Printf("Webhook error (url=%q): build request: %v", url, err) + hr.logger.Printf("Webhook error (url=%q): build request: %v", safeURL, err) return } req.Header.Set("Content-Type", "application/json") @@ -306,7 +308,7 @@ func (hr *HookRunner) postWebhook(url string, event Event) { client := &http.Client{Timeout: 5 * time.Second} resp, err := client.Do(req) if err != nil { - hr.logger.Printf("Webhook error (url=%q): %v", url, err) + hr.logger.Printf("Webhook error (url=%q): %v", safeURL, err) return } defer resp.Body.Close() @@ -314,9 +316,32 @@ func (hr *HookRunner) postWebhook(url string, event Event) { if resp.StatusCode < http.StatusOK || resp.StatusCode >= http.StatusMultipleChoices { body, _ := io.ReadAll(io.LimitReader(resp.Body, 1024)) if len(body) > 0 { - hr.logger.Printf("Webhook error (url=%q): status %s: %s", url, resp.Status, strings.TrimSpace(string(body))) + hr.logger.Printf("Webhook error (url=%q): status %s: %s", safeURL, resp.Status, strings.TrimSpace(string(body))) return } - hr.logger.Printf("Webhook error (url=%q): status %s", url, resp.Status) + hr.logger.Printf("Webhook error (url=%q): status %s", safeURL, resp.Status) } } + +func redactWebhookURL(raw string) string { + parsed, err := neturl.Parse(raw) + if err != nil || parsed.Scheme == "" || parsed.Host == "" { + return "" + } + + redacted := &neturl.URL{ + Scheme: parsed.Scheme, + Host: parsed.Host, + } + + path := strings.Trim(parsed.EscapedPath(), "/") + if path != "" { + segments := strings.Split(path, "/") + redacted.Path = "/" + segments[0] + if len(segments) > 1 { + redacted.Path += "/..." + } + } + + return redacted.String() +} diff --git a/internal/daemon/hooks_test.go b/internal/daemon/hooks_test.go index 25924cc7..372b719f 100644 --- a/internal/daemon/hooks_test.go +++ b/internal/daemon/hooks_test.go @@ -7,6 +7,7 @@ import ( "log" "net/http" "net/http/httptest" + neturl "net/url" "os" "path/filepath" "runtime" @@ -589,9 +590,18 @@ func TestHookRunnerWebhookLogsHTTPError(t *testing.T) { })) defer server.Close() + webhookURL, err := neturl.Parse(server.URL) + if err != nil { + t.Fatalf("parse server URL: %v", err) + } + webhookURL.User = neturl.UserPassword("token", "secret") + webhookURL.Path = "/services/team/webhook" + webhookURL.RawQuery = "api_key=12345" + webhookURL.Fragment = "frag" + cfg := &config.Config{ Hooks: []config.HookConfig{ - {Event: "review.failed", Type: "webhook", URL: server.URL}, + {Event: "review.failed", Type: "webhook", URL: webhookURL.String()}, }, } @@ -612,6 +622,47 @@ func TestHookRunnerWebhookLogsHTTPError(t *testing.T) { if !strings.Contains(logOutput, "502 Bad Gateway") { t.Fatalf("expected HTTP status in log, got %q", logOutput) } + if strings.Contains(logOutput, "token") || strings.Contains(logOutput, "secret") { + t.Fatalf("expected credentials to be redacted from log, got %q", logOutput) + } + if strings.Contains(logOutput, "api_key") || strings.Contains(logOutput, "12345") || strings.Contains(logOutput, "frag") { + t.Fatalf("expected query string and fragment to be redacted from log, got %q", logOutput) + } + if !strings.Contains(logOutput, "/services/...") { + t.Fatalf("expected redacted path in log, got %q", logOutput) + } +} + +func TestRedactWebhookURL(t *testing.T) { + tests := []struct { + name string + in string + want string + }{ + { + name: "userinfo query and fragment are removed", + in: "https://token:secret@example.com/services/team/webhook?api_key=123#frag", + want: "https://example.com/services/...", + }, + { + name: "single path segment is preserved", + in: "https://example.com/webhook", + want: "https://example.com/webhook", + }, + { + name: "invalid URL is hidden", + in: "://token@example.com", + want: "", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := redactWebhookURL(tt.in); got != tt.want { + t.Fatalf("redactWebhookURL(%q) = %q, want %q", tt.in, got, tt.want) + } + }) + } } func TestHooksSliceNotAliased(t *testing.T) { From 68301f21d18fec3344f4514cb855fc485df68c9c Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Fri, 6 Mar 2026 17:46:37 -0600 Subject: [PATCH 4/6] Redact entire webhook URL path to prevent secret leakage The previous implementation preserved the first path segment, which leaks the token for webhook URLs where the secret is a single path segment (e.g., https://host/). Co-Authored-By: Claude Opus 4.6 --- internal/daemon/hooks.go | 9 ++------- internal/daemon/hooks_test.go | 11 ++++++++--- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/internal/daemon/hooks.go b/internal/daemon/hooks.go index ad28194d..f1764d26 100644 --- a/internal/daemon/hooks.go +++ b/internal/daemon/hooks.go @@ -334,13 +334,8 @@ func redactWebhookURL(raw string) string { Host: parsed.Host, } - path := strings.Trim(parsed.EscapedPath(), "/") - if path != "" { - segments := strings.Split(path, "/") - redacted.Path = "/" + segments[0] - if len(segments) > 1 { - redacted.Path += "/..." - } + if p := parsed.EscapedPath(); p != "" && p != "/" { + redacted.Path = "/..." } return redacted.String() diff --git a/internal/daemon/hooks_test.go b/internal/daemon/hooks_test.go index 372b719f..ec128ed0 100644 --- a/internal/daemon/hooks_test.go +++ b/internal/daemon/hooks_test.go @@ -642,12 +642,17 @@ func TestRedactWebhookURL(t *testing.T) { { name: "userinfo query and fragment are removed", in: "https://token:secret@example.com/services/team/webhook?api_key=123#frag", - want: "https://example.com/services/...", + want: "https://example.com/...", }, { - name: "single path segment is preserved", + name: "single path segment is fully redacted", in: "https://example.com/webhook", - want: "https://example.com/webhook", + want: "https://example.com/...", + }, + { + name: "no path is preserved as-is", + in: "https://example.com", + want: "https://example.com", }, { name: "invalid URL is hidden", From faf7808c67320ce6f1d907dbd156fc3031a3b08a Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Fri, 6 Mar 2026 17:49:24 -0600 Subject: [PATCH 5/6] Fix stale webhook redaction assertion in integration test The TestHookRunnerWebhookLogsHTTPError test still expected the old "/services/..." output after redactWebhookURL was changed to collapse the entire path to "/...". Co-Authored-By: Claude Opus 4.6 --- internal/daemon/hooks_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/internal/daemon/hooks_test.go b/internal/daemon/hooks_test.go index ec128ed0..14e7aceb 100644 --- a/internal/daemon/hooks_test.go +++ b/internal/daemon/hooks_test.go @@ -628,9 +628,12 @@ func TestHookRunnerWebhookLogsHTTPError(t *testing.T) { if strings.Contains(logOutput, "api_key") || strings.Contains(logOutput, "12345") || strings.Contains(logOutput, "frag") { t.Fatalf("expected query string and fragment to be redacted from log, got %q", logOutput) } - if !strings.Contains(logOutput, "/services/...") { + if !strings.Contains(logOutput, "/...") { t.Fatalf("expected redacted path in log, got %q", logOutput) } + if strings.Contains(logOutput, "/services") { + t.Fatalf("expected path segments to be fully redacted, got %q", logOutput) + } } func TestRedactWebhookURL(t *testing.T) { From 591f13ab373998cceefeb431c23222f266a08d96 Mon Sep 17 00:00:00 2001 From: Wes McKinney Date: Fri, 6 Mar 2026 18:02:11 -0600 Subject: [PATCH 6/6] Address review findings (job 8427, 8428) - Increase bufio.Scanner buffer in stream client to 1MB to handle large findings payloads that can exceed the default 64KB limit - Unwrap *url.Error before logging in webhook error paths to prevent Go's HTTP client from leaking the raw URL in error messages - Mark HookConfig.URL as sensitive and extend collectSensitiveKeys to recurse into slice element types so config display masks webhook URLs Co-Authored-By: Claude Opus 4.6 --- cmd/roborev/stream.go | 5 ++++- internal/config/config.go | 8 ++++---- internal/config/keyval.go | 6 ++++++ internal/config/keyval_test.go | 3 +++ internal/daemon/hooks.go | 16 ++++++++++++++-- internal/daemon/hooks_test.go | 22 ++++++++++++++++++++++ 6 files changed, 53 insertions(+), 7 deletions(-) diff --git a/cmd/roborev/stream.go b/cmd/roborev/stream.go index bb69617a..a6097c74 100644 --- a/cmd/roborev/stream.go +++ b/cmd/roborev/stream.go @@ -82,8 +82,11 @@ Examples: return fmt.Errorf("stream failed: %s", body) } - // Stream events - pass through lines directly to preserve all fields + // Stream events - pass through lines directly to preserve all fields. + // Use a 1MB buffer because review.completed events can include + // large findings payloads that exceed the default 64KB limit. scanner := bufio.NewScanner(resp.Body) + scanner.Buffer(make([]byte, 0, 64*1024), 1024*1024) for scanner.Scan() { if ctx.Err() != nil { return nil diff --git a/internal/config/config.go b/internal/config/config.go index e83b9307..257d466b 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -38,10 +38,10 @@ func IsConfigParseError(err error) bool { // HookConfig defines a hook that runs on review events type HookConfig struct { - Event string `toml:"event"` // "review.failed", "review.completed", "review.*" - Command string `toml:"command"` // shell command with {var} templates - Type string `toml:"type"` // "beads" or "webhook"; empty or "command" runs Command - URL string `toml:"url"` // webhook destination URL when Type is "webhook" + Event string `toml:"event"` // "review.failed", "review.completed", "review.*" + Command string `toml:"command"` // shell command with {var} templates + Type string `toml:"type"` // "beads" or "webhook"; empty or "command" runs Command + URL string `toml:"url" sensitive:"true"` // webhook destination URL when Type is "webhook" } type AdvancedConfig struct { diff --git a/internal/config/keyval.go b/internal/config/keyval.go index f3f30b4f..7c2bad90 100644 --- a/internal/config/keyval.go +++ b/internal/config/keyval.go @@ -44,6 +44,12 @@ func structType(t reflect.Type) (reflect.Type, bool) { if t.Kind() == reflect.Ptr { t = t.Elem() } + if t.Kind() == reflect.Slice { + t = t.Elem() + if t.Kind() == reflect.Ptr { + t = t.Elem() + } + } return t, t.Kind() == reflect.Struct } diff --git a/internal/config/keyval_test.go b/internal/config/keyval_test.go index bca23932..b6ca6205 100644 --- a/internal/config/keyval_test.go +++ b/internal/config/keyval_test.go @@ -625,6 +625,9 @@ func TestIsSensitiveKey(t *testing.T) { if IsSensitiveKey("ci.github_app_id") { t.Error("expected ci.github_app_id to not be sensitive") } + if !IsSensitiveKey("hooks.url") { + t.Error("expected hooks.url to be sensitive") + } } func TestIsGlobalKey(t *testing.T) { diff --git a/internal/daemon/hooks.go b/internal/daemon/hooks.go index f1764d26..87964577 100644 --- a/internal/daemon/hooks.go +++ b/internal/daemon/hooks.go @@ -3,6 +3,7 @@ package daemon import ( "bytes" "encoding/json" + "errors" "fmt" "io" "log" @@ -300,7 +301,7 @@ func (hr *HookRunner) postWebhook(webhookURL string, event Event) { req, err := http.NewRequest(http.MethodPost, webhookURL, bytes.NewReader(payload)) if err != nil { - hr.logger.Printf("Webhook error (url=%q): build request: %v", safeURL, err) + hr.logger.Printf("Webhook error (url=%q): build request: %v", safeURL, redactURLError(err)) return } req.Header.Set("Content-Type", "application/json") @@ -308,7 +309,7 @@ func (hr *HookRunner) postWebhook(webhookURL string, event Event) { client := &http.Client{Timeout: 5 * time.Second} resp, err := client.Do(req) if err != nil { - hr.logger.Printf("Webhook error (url=%q): %v", safeURL, err) + hr.logger.Printf("Webhook error (url=%q): %v", safeURL, redactURLError(err)) return } defer resp.Body.Close() @@ -340,3 +341,14 @@ func redactWebhookURL(raw string) string { return redacted.String() } + +// redactURLError unwraps *url.Error to return only its inner +// error, preventing Go's HTTP client from leaking the raw URL +// (including secret path segments) in log output. +func redactURLError(err error) error { + var ue *neturl.Error + if errors.As(err, &ue) { + return ue.Err + } + return err +} diff --git a/internal/daemon/hooks_test.go b/internal/daemon/hooks_test.go index 14e7aceb..0e3f479d 100644 --- a/internal/daemon/hooks_test.go +++ b/internal/daemon/hooks_test.go @@ -673,6 +673,28 @@ func TestRedactWebhookURL(t *testing.T) { } } +func TestRedactURLError(t *testing.T) { + inner := fmt.Errorf("connection refused") + urlErr := &neturl.Error{ + Op: "Post", + URL: "https://hooks.example.com/secret-token", + Err: inner, + } + + got := redactURLError(urlErr) + if got != inner { + t.Fatalf("expected inner error, got %v", got) + } + if strings.Contains(got.Error(), "secret-token") { + t.Fatal("redacted error still contains secret") + } + + plain := fmt.Errorf("some other error") + if redactURLError(plain) != plain { + t.Fatal("non-url.Error should be returned as-is") + } +} + func TestHooksSliceNotAliased(t *testing.T) { // Verify that repo hooks don't leak into the global config's Hooks slice