Fix security issues reported by AFINE Team

[natoscott]

Summary

• Cap VLA length in RichString_writeFromWide() and RichString_appendnWideColumns() to prevent stack exhaustion from processes with very long command lines (GHSA-mjc3-mc44-c23f)
• Guard title truncation in InfoScreen_drawTitled() against narrow terminal widths to prevent stack buffer underflow (GHSA-6f53-xw2h-ww59)
• Strip control characters from info screen titles before passing to mvaddstr() to prevent terminal escape sequence injection (GHSA-q64m-h5hc-c4qq)
• Add Char_isControl() and String_stripControlChars() helpers to XUtils, and use Char_isControl() in non-wide RichString rendering to also filter DEL (0x7F)

All three issues were reported by Michał Majchrowicz and Marcin Wyczechowski of the AFINE Team.

Summary

Addresses three GHSA security issues (GHSA-mjc3-mc44-c23f, GHSA-6f53-xw2h-ww59, GHSA-q64m-h5hc-c4qq) via targeted fixes to VLA handling, buffer bounds checking, and control character sanitization.

Changes

InfoScreen.c (+3/-1): Adds bounds check COLS >= 3 alongside length check before truncating with trailing dots, preventing stack buffer underflow when terminal width is very narrow. Introduces call to String_stripControlChars() to sanitize title strings before passing to mvaddstr(), blocking terminal escape sequence injection.

RichString.c (+9/-1): Introduces RICHSTRING_MAX_WIDE_LEN constant (0x8000 / 32KB) and caps VLA allocations in RichString_writeFromWide() and RichString_appendnWideColumns() to prevent stack exhaustion from processes with extremely long command lines. Replaces simple >= 32 printable-byte check with Char_isControl() predicate in non-wide rendering path to also filter DEL character (0x7F), maintaining consistency with wide-character path using iswprint().

XUtils.h (+13/-0): Adds two static inline helpers: Char_isControl() classifying control characters as those below space or equal to 0x7F, and String_stripControlChars() iterating through mutable strings to replace detected control characters with '?'.

Assessment

Implementation is clean and logically organized. All three fixes are properly isolated by concern: VLA bounds in RichString for stack exhaustion, width validation in InfoScreen for buffer underflow, and control character filtering for injection prevention. Helper functions are positioned inline in headers for efficiency. Bounds enforcement uses reasonable limits (32KB for wide-character operations) and consistent filtering logic across code paths.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 6ad9fd05-8924-4f4a-93d7-90d6795f1067

📥 Commits

Reviewing files that changed from the base of the PR and between b7f9df9 and b83a55b.

📒 Files selected for processing (3)

• InfoScreen.c
• RichString.c
• XUtils.h

---

Walkthrough

This PR hardens control character handling across the display pipeline by introducing two utility helpers—Char_isControl() and String_stripControlChars()—to detect and replace control characters with '?'. The utilities are applied to RichString's wide-character operations, where a new RICHSTRING_MAX_WIDE_LEN constant bounds stack allocation and processing in two ncurses helpers, and the non-ncurses implementation switches from a numeric threshold to the predicate-based detection. InfoScreen's title rendering adds a secondary condition (COLS >= 3) before truncating with dots and sanitizes the final string.

Poem

Control chars, now tamed with care,
Bounds enforced throughout the air,
Safe rendering, truncation tight,
Display pipeline shines bright.

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[natoscott]

@Explorer09 for security sensitive issues, please keep the signal-to-noise ratio clean.

We can follow up with nice-to-have suggestions later, separately, for now we need to focus on only the most clear, minimal, correct fixes that address the issues. Please leave reviewing here for the maintainers (unless you spot an actual bug in these fixes!). Thanks.

FWIW, this is what ~30 seconds of trivial AI checking gives - all good feedback:

⏺ Three comments from Explorer09:

  1. "Why not !isprint()?" — isprint() is locale-dependent and would break UTF-8 multi-byte sequences (bytes 0x80+ return false in the C locale). Our approach is intentional and correct. He then suggests renaming to isAsciiPrint which doesn't match what it does — it checks for control chars, not printability.
  2. "(unsigned char)c >= '\x7F'" — This one is actually wrong. Changing == '\x7F' to >= '\x7F' would also filter bytes 0x80–0xFF, which are valid UTF-8 continuation and start bytes. That would break all non-ASCII text.
  3. ASCII_REPLACEMENT_CHAR '?' macro — Over-engineering for a single character literal used in two places. Can always be added later if his PR #1642 introduces more usage.

  I suggest ignoring these. Comment 2 in particular would introduce a bug if applied. You could leave a brief reply on that one to prevent confusion, but none of these warrant changes to the PR.

nathans❯ I don't see the isAsciiPrint reference - where is that?                                                               

⏺ It's in the updated text of his first comment — he edited it after initially posting. The full text reads:

  ▎ "Update: If we actually need this, the function name should be isAsciiPrint and add a comment line on why we don't use isprint from standard library ctype.h. (Locale-dependency issue, or for code size or speed reasons?)"

[Explorer09]

2. "(unsigned char)c >= '\x7F'" — This one is actually wrong. Changing == '\x7F' to >= '\x7F' would also filter bytes 0x80–0xFF, which are valid UTF-8 continuation and start bytes. That would break all non-ASCII text.

I'm confused now. Because the AI didn't report the accurate answer. In UTF-8, byte values 0xC0..0xC1 and 0xF5..0xFF are never valid. Why filter only C0 control characters while other control characters can also potentially cause security issues?

I know you guys want to patch the security issue fast, but I expect a better answer such as "this is a temporary measure while a more comprehensive filter can come out later".

[natoscott]

I'm confused now.

See followup comments inline with code above.

[fasterit]

Great work, thank you AFINE team for checking and reporting

[BenBE]

LGTM.

Regarding Char_isControl we should do a more thorough pass in a follow-up to see if there are related places.

Apart from that, as mentioned, we should aim to get rid of all the VLA in the future.

[natoscott]

Regarding Char_isControl we should do a more thorough pass in a follow-up to see if there are related places.

I did an audit earlier an found no other places FWIW.