Merge pull request #334 from TBX3D/harden-notify-clean

fix(notify): neutralize markdown injection and redact webhook url from errors

Author: vmfunc

internal/notify/message.go

@@ -16,8 +16,10 @@ import (
 	"bytes"
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"net/http"
+	"net/url"
 	"strings"
 
 	"github.com/vmfunc/sif/internal/finding"
@@ -46,24 +48,25 @@ func renderFindings(findings []finding.Finding) string {
 	return b.String()
 }
 
-// postJSON marshals payload and POSTs it to url through the shared client. it
-// drains+closes the response so the conn returns to httpx's pool, and treats any
-// non-2xx as a delivery failure so a 4xx from a bad webhook surfaces loudly.
-func postJSON(ctx context.Context, client *http.Client, url string, payload any) error {
+// postJSON marshals payload and POSTs it to endpoint through the shared
+// client. it drains+closes the response so the conn returns to httpx's pool,
+// and treats any non-2xx as a delivery failure so a 4xx from a bad webhook
+// surfaces loudly.
+func postJSON(ctx context.Context, client *http.Client, endpoint string, payload any) error {
 	body, err := json.Marshal(payload)
 	if err != nil {
 		return fmt.Errorf("marshal payload: %w", err)
 	}
 
-	req, err := http.NewRequestWithContext(ctx, http.MethodPost, url, bytes.NewReader(body))
+	req, err := http.NewRequestWithContext(ctx, http.MethodPost, endpoint, bytes.NewReader(body))
 	if err != nil {
 		return fmt.Errorf("build request: %w", err)
 	}
 	req.Header.Set("Content-Type", contentTypeJSON)
 
 	resp, err := client.Do(req) //nolint:bodyclose // drained and closed via httpx.DrainClose
 	if err != nil {
-		return fmt.Errorf("post: %w", err)
+		return fmt.Errorf("post to %s: %w", req.URL.Host, redactTransportErr(err))
 	}
 	defer httpx.DrainClose(resp)
 
@@ -72,3 +75,16 @@ func postJSON(ctx context.Context, client *http.Client, url string, payload any)
 	}
 	return nil
 }
+
+// redactTransportErr strips the webhook url out of a client.Do failure. for
+// these providers the url IS the credential, and http.Client wraps every
+// transport failure in a *url.Error whose Error() quotes it verbatim; unwrap
+// to the underlying cause (which only ever mentions host:port) and let the
+// caller prefix the host separately.
+func redactTransportErr(err error) error {
+	var urlErr *url.Error
+	if errors.As(err, &urlErr) {
+		return urlErr.Err
+	}
+	return err
+}

internal/notify/notify_test.go

@@ -222,3 +222,140 @@ func assertPostJSON(t *testing.T, c capture) {
 		t.Errorf("content-type = %q, want %q", c.contentType, contentTypeJSON)
 	}
 }
+
+// deadURL returns a URL that will refuse connection: bind a listener, close
+// it, reuse the address. good enough to force a transport-level error out of
+// client.Do without touching the network.
+func deadURL(t *testing.T) string {
+	t.Helper()
+	srv := httptest.NewServer(http.HandlerFunc(func(http.ResponseWriter, *http.Request) {}))
+	u := srv.URL
+	srv.Close()
+	return u
+}
+
+// see redactTransportErr's doc comment (message.go) for why this matters.
+func TestNotifyErrorRedactsSecretWebhookURL(t *testing.T) {
+	host := deadURL(t)
+	secret := host + "/services/T00000000/B11111111/SUPERSECRETTOKEN"
+	p := &slackProvider{webhook: secret}
+	err := p.send(context.Background(), http.DefaultClient, sampleFindings())
+	if err == nil {
+		t.Fatal("expected transport error")
+	}
+	if strings.Contains(err.Error(), "SUPERSECRETTOKEN") {
+		t.Fatalf("LEAK: secret webhook token present in error: %v", err)
+	}
+	if !strings.Contains(err.Error(), strings.TrimPrefix(host, "http://")) {
+		t.Errorf("error dropped the host too, operator can't debug: %v", err)
+	}
+}
+
+func TestNotifyErrorRedactsTelegramToken(t *testing.T) {
+	orig := telegramAPIBase
+	host := deadURL(t)
+	telegramAPIBase = host
+	t.Cleanup(func() { telegramAPIBase = orig })
+
+	p := &telegramProvider{token: "123456:AAHsupersecretbottoken", chatID: "42"}
+	err := p.send(context.Background(), http.DefaultClient, sampleFindings())
+	if err == nil {
+		t.Fatal("expected transport error")
+	}
+	if strings.Contains(err.Error(), "AAHsupersecretbottoken") {
+		t.Fatalf("LEAK: telegram bot token present in error: %v", err)
+	}
+	if !strings.Contains(err.Error(), strings.TrimPrefix(host, "http://")) {
+		t.Errorf("error dropped the host too, operator can't debug: %v", err)
+	}
+}
+
+// attacker-controlled finding content (a scanned target's page title, a
+// crawled url, a cms name) reaches the slack/discord code block verbatim. a
+// title that embeds a closing fence used to break out of our wrapping block
+// and inject live markdown (mentions, masked links) into the channel.
+func TestNotifyCodeBlockBreakoutNeutralized(t *testing.T) {
+	var c capture
+	srv := captureServer(t, &c)
+
+	// a backtick run of any length has to come out broken; 5, 8 and 11 are the
+	// lengths that reformed a fence when only exact triples were replaced.
+	for _, n := range []int{3, 4, 5, 6, 8, 11} {
+		run := strings.Repeat("`", n)
+		evil := []finding.Finding{{
+			Target:   "https://evil.test",
+			Module:   "probe",
+			Severity: finding.SeverityHigh,
+			Key:      "probe:x",
+			Title:    run + "\n@everyone pwned <https://evil.test|click>\n" + run,
+		}}
+		p := &discordProvider{webhook: srv.URL}
+		if err := p.send(context.Background(), srv.Client(), evil); err != nil {
+			t.Fatalf("run of %d: send: %v", n, err)
+		}
+		var payload discordPayload
+		if err := json.Unmarshal(c.body, &payload); err != nil {
+			t.Fatalf("run of %d: unmarshal: %v", n, err)
+		}
+		// a clean payload has exactly the 2 fences we added (open+close); any more
+		// means attacker content broke out.
+		if fences := strings.Count(payload.Content, "```"); fences > 2 {
+			t.Fatalf("INJECTION: run of %d backticks added %d extra code fences, breaking out: %q", n, fences-2, payload.Content)
+		}
+	}
+}
+
+// slack resolves a bare "<...|...>" as a link/mention independent of code-block
+// boundaries, so the fence fix alone isn't enough for slack: the control
+// characters (&, <, >) must be entity-escaped too.
+func TestSlackEscapesControlChars(t *testing.T) {
+	var c capture
+	srv := captureServer(t, &c)
+
+	evil := []finding.Finding{{
+		Target:   "https://evil.test",
+		Module:   "probe",
+		Severity: finding.SeverityHigh,
+		Key:      "probe:x",
+		Title:    "<https://evil.test|click> & <!everyone>",
+	}}
+	p := &slackProvider{webhook: srv.URL}
+	if err := p.send(context.Background(), srv.Client(), evil); err != nil {
+		t.Fatalf("send: %v", err)
+	}
+	var payload slackPayload
+	if err := json.Unmarshal(c.body, &payload); err != nil {
+		t.Fatalf("unmarshal: %v", err)
+	}
+	if strings.Contains(payload.Text, "<https://evil.test|click>") {
+		t.Fatalf("INJECTION: unescaped slack link syntax reached the payload: %q", payload.Text)
+	}
+	if !strings.Contains(payload.Text, "&lt;https://evil.test|click&gt;") || !strings.Contains(payload.Text, "&amp;") {
+		t.Fatalf("expected slack control chars entity-escaped, got: %q", payload.Text)
+	}
+}
+
+// robustness sanity: confirm a zero http.Client.Timeout would mean an
+// unbounded client. not a bug in notify per se, but documents that ctx, not
+// Timeout, is what bounds a hung endpoint here.
+func TestNotifyZeroTimeoutIsUnbounded(t *testing.T) {
+	blocked := make(chan struct{})
+	srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+		<-blocked
+	}))
+	t.Cleanup(func() { close(blocked); srv.Close() })
+
+	ctx, cancel := context.WithTimeout(context.Background(), 300*time.Millisecond)
+	defer cancel()
+	p := &slackProvider{webhook: srv.URL}
+	done := make(chan error, 1)
+	go func() { done <- p.send(ctx, srv.Client(), sampleFindings()) }()
+	select {
+	case err := <-done:
+		if err == nil {
+			t.Fatal("expected ctx-cancel error from hung endpoint")
+		}
+	case <-time.After(3 * time.Second):
+		t.Fatal("send did not honor ctx cancellation on hung endpoint")
+	}
+}

internal/notify/slack.go

@@ -15,6 +15,7 @@ package notify
 import (
 	"context"
 	"net/http"
+	"strings"
 
 	"github.com/vmfunc/sif/internal/finding"
 )
@@ -34,12 +35,36 @@ type slackPayload struct {
 }
 
 func (s *slackProvider) send(ctx context.Context, client *http.Client, findings []finding.Finding) error {
-	payload := slackPayload{Text: codeBlock(renderFindings(findings))}
+	payload := slackPayload{Text: codeBlock(escapeSlackText(renderFindings(findings)))}
 	return postJSON(ctx, client, s.webhook, payload)
 }
 
+// escapeSlackText entity-escapes slack's three control characters (& first, so
+// the later replacements don't double-escape). slack resolves a bare
+// "<...|...>" as a link/mention regardless of surrounding code-fence text, so
+// an unescaped title would otherwise render as a live masked link.
+func escapeSlackText(body string) string {
+	r := strings.NewReplacer("&", "&amp;", "<", "&lt;", ">", "&gt;")
+	return r.Replace(body)
+}
+
 // codeBlock wraps body in a triple-backtick fence; both slack and discord render
-// it fixed-width, which preserves the column-aligned finding lines.
+// it fixed-width, which preserves the column-aligned finding lines. body runs
+// through sanitizeFence first so attacker-controlled finding content (a title
+// pulled from the scanned target) can't close the fence early and inject
+// markdown/mentions outside it.
 func codeBlock(body string) string {
-	return "```\n" + body + "```"
+	return "```\n" + sanitizeFence(body) + "```"
+}
+
+// sanitizeFence separates every backtick in body from the next with a
+// zero-width space. the text still reads as backticks to a human but no two
+// are ever adjacent, so neither slack nor discord sees a fence boundary and
+// attacker content can't close the code block we wrap it in.
+//
+// breaking exact triples instead would leave a trailing bare backtick, and any
+// run of length \u2261 2 mod 3 (5, 8, 11...) would reform a contiguous triple.
+func sanitizeFence(body string) string {
+	const zwsp = "\u200b" // zero-width space
+	return strings.ReplaceAll(body, "`", "`"+zwsp)
 }