notify: fix panic when truncating multi-byte messages - #5544
Open
MaxFreedomPollard wants to merge 1 commit into
Open
notify: fix panic when truncating multi-byte messages#5544MaxFreedomPollard wants to merge 1 commit into
MaxFreedomPollard wants to merge 1 commit into
Conversation
TruncateInBytes indexed the rune slice with a byte count. In notify/util.go it built the starting slice as r[:truncationTarget], where r is []rune(s) and truncationTarget is n-3, a number of bytes. A string holding multi-byte characters has fewer runes than bytes, so whenever the input was longer than n bytes but held fewer than n-3 runes the index ran past the end of the rune slice and panicked with "slice bounds out of range". The Webex notifier is the only caller. It truncates the rendered message to maxMessageSize (7439 bytes) in notify/webex/webex.go:89, so a notification whose markdown was 7440 bytes or more of non-ASCII text crashed Alertmanager: 3720 Cyrillic characters, 2480 CJK characters or 1860 emoji are enough. The notification pipeline runs in a goroutine started by the dispatcher and nothing recovers, so the process exits. Clamp the starting index to the number of runes available. The loop that follows already trims the slice down until it fits the byte budget, so the result is unchanged for every input that did not panic. Add a case to TestTruncate covering a short string whose byte budget exceeds its rune count, TestTruncateInBytesFewerRunesThanBytes covering two-, three- and four-byte runes at the Webex limit, and TestWebexTruncatesMultiByteMessage covering the notifier end to end. Signed-off-by: Max Freedom Pollard <272618364+MaxFreedomPollard@users.noreply.github.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthrough
ChangesMultibyte truncation handling
Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to Multibyte Webex messages exceeding the byte limit now truncate safely instead of panicking, while preserving the configured payload bound. The focused utility and Webex coverage supports merge readiness. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TruncateInBytesinnotify/util.goindexes the rune slice with a byte count. Onmain, line 139 builds the starting slice asr[:truncationTarget], whereris[]rune(s)andtruncationTargetisn-3, a number of bytes. A string holding multi-byte characters has fewer runes than bytes, so whenever the input is longer thannbytes while holding fewer thann-3runes, the index runs past the end of the rune slice and panics.The Webex notifier is the only caller in this repository.
notify/webex/webex.go:89truncates the rendered markdown tomaxMessageSize, which is 7439 bytes, so a notification whose message reaches 7440 bytes of non-ASCII text crashes Alertmanager. 3720 Cyrillic characters, 2480 CJK characters or 1860 emoji are enough. The notification pipeline runs in a goroutine started by the dispatcher atdispatch/dispatch.go:578, and neithernotifynordispatchrecovers, so the process exits.The line arrived in f51f51e, "Use the new truncation in bytes functions to ensure strings are not butchered", part of #3132, which replaced the byte slicing that #3135 had introduced a day earlier.
TruncateInRunesis not affected: it returns early whenlen(r) <= n, so its ownr[:n-1]is always in range.What changed
truncatedRunesnow starts atr[:min(truncationTarget, len(r))]. The loop that follows already trims the slice down until it fits, andtruncationTargetis still the byte budget it trims to, so the returned string is unchanged for every input that did not panic. The existingTestTruncatetable passes untouched.How tested
Two new tests fail on unmodified
mainand pass with the fix.TestTruncateInBytesFewerRunesThanBytesinnotify/util_test.gocovers two-byte, three-byte and four-byte runes at the Webex limit. It asserts the result stays within the limit, ends with the truncation marker, is a prefix of the input, and keeps as many whole runes as the byte budget allows.TestWebexTruncatesMultiByteMessageinnotify/webex/webex_test.godrivesNotifyend to end with a 3720 character Cyrillic annotation against anhttptestserver, and asserts the posted markdown fitsmaxMessageSize.On unmodified
main, with only the two test files applied:The third test change is one more case in the existing
TestTruncatetable,TruncateInBytes("😀😀", 7). Two runes, eight bytes: the byte budget left for the text is larger than the number of runes to pick it from. That case passes onmainas well, because the rune slice happens to have enough spare capacity for the out of length index to be legal, and the loop then trims the zero runes it picked up back off. It is here to pin the small end of the same mistake.With the fix applied, on Go 1.26.4 and golangci-lint v2.13.1, the version pinned in
Makefile.common:I also ran
go testacross the 76 Go packages that build without the generated UI assets. They all pass exceptTestDefaultConfigFilesOthersWithXDGConfigHomeincli, which fails identically on unmodifiedmainon macOS, becauseos.UserConfigDirreturns~/Library/Application Supportthere and ignoresXDG_CONFIG_HOME.go build ./...needsui/app/dist, which a fresh clone does not have, so I built and tested by package list instead.Pull Request Checklist
Please check all the applicable boxes.
benchstatto compare benchmarksCHANGELOG.mdasks for behaviour notes in the pull request description, so the entry is in the release notes block below. No configuration surface changes.Which user-facing changes does this PR introduce?