Skip to content

Follow-up hardening from CodeRabbit review of #64 (callback identity + split newline) #69

Closed

Description

@eliau2005

Tracking issue for the non-blocking findings CodeRabbit raised on #64. These were intentionally deferred so #64 could merge; none is a correctness blocker for a single-server, small-watch-list deployment, but all are cheap, worthwhile hardening.

1. Preserve the newline at each split boundary (internal/telegram/telegram.go)

In splitMessage, after s = s[cut:] the leading \n is stripped, so a newline chosen as the cut point is discarded entirely. Because each chunk is sent as a separate Telegram message, the practical impact is cosmetic (a lost blank line between messages) rather than the line-merge CodeRabbit described — but dropping the strip is trivial and makes the joined visible text round-trip exactly.

  • Fix: remove the if len(s) > 0 && s[0] == '\n' { s = s[1:] } block so the newline stays at the start of the next chunk.
  • Test: extend TestSplitMessageHTMLPre to assert the joined visible text equals the original visible text (not just strings.Contains).

2. snz: direct/hashed namespace ambiguity (internal/agent/keyboard.go)

resolveSnoozeKey treats snz:h<8 hex digits> as a hash lookup. A direct alert key literally named h+8 hex digits would be misparsed. No current key shape (cpu, mem, docker:<name>, ssl:<host>, disk:<mount>) can hit this, so it is theoretical — but the clean fix is cheap.

  • Fix: use explicit namespaces snz:d:<key> (direct) and snz:h:<digest> (hashed) so the two can never collide.

3. Collision-resistance of callback identities (snoozeCallbackData, watchIndexData) — low priority

Both encoders use CRC32. A CRC32 collision at the same list index could snooze/remove the wrong entry. For a personal single-server bot with a handful of watched items the collision surface is effectively nil, and chat-ID auth still gates every callback, so this is defense-in-depth rather than a real risk.

  • Optional: swap CRC32 for a fixed-size truncated SHA-256 digest at both encoders (and the matching lookup paths), keeping payloads under Telegram's 64-byte limit.

4. Docstring coverage warning

CodeRabbit's pre-merge Docstring Coverage check reported 27% vs an 80% threshold. Mostly noise (the new helpers already carry comments), but worth a pass if we want the gate green.

Refs #64.

Activity

  1. added
    securitySecurity-relevant behaviour or hardening
    reliabilityProduction reliability, silent failures, long-running behaviour
    telegramTelegram bot and client
    on Sep 15, 2026
  2. eliau2005 commented on Sep 15, 2026

    @eliau2005
    OwnerAuthor

    @MrBeldum since you're already deep in this area from #64 — would you like to take this follow-up? All four items are small and well-scoped (see the checklist above). No pressure, but it's yours if you want it. 🙌

    (GitHub won't let me set you as assignee since you're not a repo collaborator yet — happy to send a collaborator invite if you'd prefer that.)

  3. MrBeldum commented on Sep 16, 2026

    @MrBeldum
    Contributor

    Opened follow-up PR: #70 — implements items 1–3 (split newline preserve, snz:d:/snz:h: namespaces, truncated SHA-256 fingerprints). Skipping docstring-coverage noise.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    good first issueGood for newcomersreliabilityProduction reliability, silent failures, long-running behavioursecuritySecurity-relevant behaviour or hardeningtelegramTelegram bot and client

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions