notify/telegram: wrap Send error with notify.RedactURL - #5254
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThis PR redacts Telegram bot tokens from send errors before classification and return. Tests cover transport-level failures and Telegram API error responses. ChangesTelegram notifier error redaction
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to Telegram transport errors now redact bot tokens before reaching logs while preserving existing retry behavior. No actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Description checkExplanation The description is complete and directly explains the security issue, implementation, affected error paths, tests, and release note. It also records the bugfix test and commit sign-off checklist items.
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@notify/telegram/telegram_test.go`:
- Line 288: There is an unmatched closing brace in
notify/telegram/telegram_test.go (a stray "}" at the end of the file) that
prevents the tests from compiling; remove that extra brace so the file's braces
properly match and the surrounding test functions (e.g., the test function
blocks in this file) close correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a883295a-cfc2-45c6-97ac-6a546fb58436
📒 Files selected for processing (2)
notify/telegram/telegram.gonotify/telegram/telegram_test.go
| require.NotContains(t, err.Error(), token, "bot token leaked in API error") | ||
| }) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
python - <<'PY'
from pathlib import Path
path = Path("notify/telegram/telegram_test.go")
src = path.read_text()
i = 0
line = 1
stack = 0
state = "code"
while i < len(src):
c = src[i]
n = src[i + 1] if i + 1 < len(src) else ""
if state == "code":
if c == "/" and n == "/":
state = "line_comment"; i += 1
elif c == "/" and n == "*":
state = "block_comment"; i += 1
elif c == '"':
state = "double_quote"
elif c == "'":
state = "single_quote"
elif c == "`":
state = "raw_quote"
elif c == "{":
stack += 1
elif c == "}":
stack -= 1
if stack < 0:
print(f"unmatched closing brace near line {line}")
break
elif state == "line_comment":
if c == "\n":
state = "code"
elif state == "block_comment":
if c == "*" and n == "/":
state = "code"; i += 1
elif state == "double_quote":
if c == "\\":
i += 1
elif c == '"':
state = "code"
elif state == "single_quote":
if c == "\\":
i += 1
elif c == "'":
state = "code"
elif state == "raw_quote":
if c == "`":
state = "code"
if c == "\n":
line += 1
i += 1
else:
print(f"brace_balance={stack}")
PYRepository: prometheus/alertmanager
Length of output: 105
Remove the stray closing brace at Line 288 in notify/telegram/telegram_test.go
The file contains an unmatched }, so it will fail to parse/compile.
💡 Proposed fix
@@
-}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@notify/telegram/telegram_test.go` at line 288, There is an unmatched closing
brace in notify/telegram/telegram_test.go (a stray "}" at the end of the file)
that prevents the tests from compiling; remove that extra brace so the file's
braces properly match and the surrounding test functions (e.g., the test
function blocks in this file) close correctly.
telegram.go was the only notifier not applying notify.RedactURL to errors from the underlying HTTP client. When telebot's Send call fails for transport reasons (e.g. dial timeout), it returns a *url.Error that includes the full API URL, which contains the bot token as a path segment (https://api.telegram.org/bot<TOKEN>/...). This error propagated verbatim into the slog attributes logged by notify.go and dispatch.go, leaking the token. All other notifiers (webhook, slack, discord, msteams, etc.) already call notify.RedactURL on their HTTP errors following PR prometheus#3887. Apply the same pattern to the one remaining call site in telegram.go. Note: getBotToken errors are local file-read errors with no URL, and createTelegramClient / telebot.NewBot runs in Offline mode and never contacts the API, so neither site needs wrapping. Adds a unit test covering both error paths: - transport error (*url.Error): token redacted, URL shows <redacted> - Telegram API error (non-*url.Error): RedactURL is a no-op, but telebot's own error message does not include the URL or token. Signed-off-by: otmyards-crypto <245061101+otmyards-crypto@users.noreply.github.com> Co-Authored-By: Paperclip <noreply@paperclip.ing> Signed-off-by: Solomon Jacobs <solomonjacobs@protonmail.com>
4653ffc to
f253fcd
Compare
|
Thanks! |
3 participants
Pull Request Checklist
This PR addresses a small inconsistency left over from #3887.
Which user-facing changes does this PR introduce?
Context
#3887 introduced
notify.RedactURLand applied it to every notifier's HTTP error return path so that bot tokens, webhook URLs, and other secrets embedded in*url.Errorare stripped before the error is logged.notify/telegram/telegram.gowas the only notifier that was missed. Whentelebot.Bot.Sendfails for transport reasons (dial timeout, connection refused, TLS handshake error, etc.), the underlyinghttp.Clientreturns a*url.ErrorwhoseURLfield is the full Telegram API URL — which contains the bot token as a path segment:That error is returned verbatim from
Notify, then logged by the surroundingnotify.RetryStage(l.Warn("Notify attempt failed, will retry later", "err", err)) anddispatch.aggrGroup(logger.Error("Notify for alerts failed")). The result is that the bot token ends up in operator logs on every transport failure — observed in production with alertmanager v0.28.1 going through a flaky outbound link.Change
One-line fix at the only relevant call site:
if err != nil { - return true, err + return true, notify.RedactURL(err) }Other return paths in
Notifywere checked and do not need wrapping:getBotTokenandgetChatIDerrors come fromos.ReadFileon local files and contain no URL.createTelegramClientrunstelebot.NewBotwithOffline: true, so it never contacts the API and never returns a*url.Error. (Constructor errors don't go throughNotifyanyway.)Tests
TestTelegramNotifyRedactURLcovers both error paths:*url.Error): ahttptest.Serveris started and immediately closed so the nextSendcall fails to dial. The test asserts the bot token is absent from the returned error and that the URL is replaced with<redacted>.*url.Error): a fake server returns{"ok":false,"description":"Bad Request: chat not found"}.telebotwraps this in its own error type that does not contain*url.Error, sonotify.RedactURLis a no-op — but the token is also not present in the resulting error string (telebot's API error formatter doesn't include the URL). The test asserts the token is absent in this path too.Notes
notify.RedactURLuseserrors.As(err, *url.Error)and is a no-op when the error chain doesn't include*url.Error, so this change is safe for the API-error path and does not depend on assumptions about telebot's internal error wrapping.