fix(notify): neutralize markdown injection and redact webhook url from errors#334
fix(notify): neutralize markdown injection and redact webhook url from errors#334TBX3D wants to merge 3 commits into
Conversation
client.Do wraps every transport failure in a *url.Error whose Error() quotes the request url verbatim. for slack/discord/generic webhooks the url is the credential, and telegram's bot token rides the path, so a routine dns/tls/timeout/refused error leaked the secret into the operator log and the returned error. unwrap to the underlying cause (host:port only, never the path) and let the caller prefix the host separately, so an operator can still tell which sink failed.
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #334 +/- ##
=======================================
Coverage ? 64.91%
=======================================
Files ? 86
Lines ? 7648
Branches ? 0
=======================================
Hits ? 4965
Misses ? 2301
Partials ? 382 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
pr summary3 files changed (+187 -9)
|
c173f53 to
a9ab8ff
Compare
slack and discord both wrap rendered findings in a triple-backtick fence, but a finding's title is attacker-controlled (a scanned target's page title, a crawled url, a cms name). a title embedding a closing fence broke out of the block and injected live markdown (mentions, masked links) into the channel. break up any backtick run in the body before wrapping, and for slack additionally entity-escape &, <, > per slack's api docs, since its parser resolves link/mention syntax ahead of code-block boundaries.
a9ab8ff to
7d938ac
Compare
vmfunc
left a comment
vmfunc
left a comment
There was a problem hiding this comment.
sanitizeFence has a bypass. the replacement ` ends on a bare backtick, so any backtick run whose length is ≡2 mod 3 reforms a clean fence: a title of 5 backticks (or 8, 11...) comes out with the trailing three contiguous again. i checked it, n=5/8/11 all give 3 adjacent backticks back. discord takes sanitizeFence with no entity escaping, so that's a live breakout, @everyone and x render outside the block. slack only survives it because you escape <>& separately.
TestNotifyCodeBlockBreakoutNeutralized only exercises n=3 and asserts fences<=2, so it's green while the hole is open. drop in an n=5 case and it fails.
fix is to not leave a trailing bare backtick. simplest is to break every backtick instead of just triples: strings.ReplaceAll(body, "", ""+zwsp). no run of 2+ can survive that.
the rest is right: unwrapping the *url.Error and re-prefixing req.URL.Host keeps host:port and drops the path token for all four sinks, and the slack <>& escaping is correct. close the fence bypass and this is in.
replacing exact triples left a trailing bare backtick, so a run whose length is 2 mod 3 (5, 8, 11...) came back out with three adjacent backticks and reopened the block. discord takes the sanitized body with no entity escaping, so that was a live breakout for mentions and masked links. the breakout test only covered n=3; it now walks 3,4,5,6,8,11 and fails on the old behaviour at n=5.
|
confirmed, and the n=5 case fails exactly as you said. the old INJECTION: run of 5 backticks added 2 extra code fences, breaking out: took your fix, ReplaceAll on the single backtick, so no two are ever adjacent |
There was a problem hiding this comment.
Pull request overview
Note
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Hardens Slack/Discord notifications against attacker-controlled markdown injection and prevents leaking secret webhook URLs in error strings.
Changes:
- Slack: entity-escape control characters (
&,<,>) before wrapping output in a code fence. - Slack/Discord: neutralize closing code-fence injection by breaking contiguous backtick runs with zero-width spaces.
- Redact webhook URLs from
http.Client.Dotransport errors and add regression tests for redaction/injection behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| internal/notify/slack.go | Escapes Slack control chars and sanitizes backticks before code-fencing. |
| internal/notify/message.go | Redacts credential-bearing URLs from transport errors returned by client.Do. |
| internal/notify/notify_test.go | Adds tests for URL redaction and markdown/code-fence breakout neutralization. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // 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()) |
| req, err := http.NewRequestWithContext(ctx, http.MethodPost, endpoint, bytes.NewReader(body)) | ||
| if err != nil { | ||
| return fmt.Errorf("build request: %w", err) | ||
| } |
| func escapeSlackText(body string) string { | ||
| r := strings.NewReplacer("&", "&", "<", "<", ">", ">") | ||
| return r.Replace(body) | ||
| } |
attacker-controlled finding content (a page title, a crawled url) reached
the slack/discord code block verbatim. a title embedding a closing triple-
backtick fence could break out of the wrapping block and inject live
markdown; slack additionally resolves a bare "<...|...>" as a link/mention
independent of code-fence boundaries, so the three slack control
characters are now entity-escaped ahead of fencing.
the second commit redacts the webhook url from transport errors: for
slack/discord/generic sinks the url itself is the credential, and
http.Client wraps every transport failure in a *url.Error that quotes it
verbatim. errors are now unwrapped to the underlying cause and re-prefixed
with just the host, so a failure is still debuggable without leaking the
secret into a log line.
sanitizeFence breaks every backtick rather than exact triples. replacing only
triples leaves a trailing bare backtick, so a run of 5, 8 or 11 backticks
reformed a contiguous triple and reopened the fence; discord takes the sanitized
body with no entity escaping, so that was a live breakout for mentions and
masked links. the breakout test walks 3, 4, 5, 6, 8 and 11.