Validate HTML entities in StringRangeMarker position mapping

What does this MR do and why?

Fixes a position-mapping bug in Gitlab::StringRangeMarker where a literal ampersand (&) in html_safe rich text causes the entity scanner to overrun to an unrelated semicolon or drop trailing characters from the mapping.

Resolves: #629891

Problem

Gitlab::StringRangeMarker#safe_position_mapping builds a mapping between raw text positions and rich (HTML-escaped) text positions for inline diff highlighting. The entity scan logic treated every & as the start of an HTML entity and scanned forward to the next ; without validating the span.

This caused two failure modes when a literal & appeared in an html_safe rich_line (which bypasses escaping in the constructor):

  1. Mid-line bare &: The & maps to the entire span up to an unrelated ; (e.g., a URL parameter separator or code semicolon), causing incorrect marker placement.
  2. Trailing bare &: The scan reaches end-of-string, advances rich_pos past rich_length, and breaks the outer loop early, dropping all following characters from the mapping.

Solution

The fix validates the span between & and ; before collapsing it. Only well-formed HTML entities (named: &, decimal: ', hex: /) are collapsed to a single raw position. Bare & characters map 1:1.

The forward scan is also bounded: a rejected bare & only advances rich_pos by one, so without a bound, k bare & before a distant or absent ; would each re-scan nearly the same span, giving O(k*n) worst-case behavior on a single line. Capping the scan at the longest possible entity length keeps a bare & O(1).

Changes:

  • Extracted entity_span_end helper: looks ahead from & to ;, returns the index of ; if the span is a valid entity, otherwise nil
  • Extracted valid_entity? helper: validates entity shape with /\A&(?:\w+|#\d+|#x[0-9a-fA-F]+);\z/
  • Updated entity scan logic: only collapse when entity_span_end returns a valid index, otherwise map & as 1:1
  • Bounded the forward scan at MAX_ENTITY_LENGTH (33, the longest HTML5 named entity), eliminating the O(k*n) worst case for lines with many bare &

Test coverage (9 new specs):

  • Mid-line bare & with later unrelated ;
  • Trailing bare & with no ;
  • Valid entity regression guard (& still collapses)
  • Multiple literal & on same line
  • Numeric entities (decimal ', hex /)
  • Empty entity &; (treated as two chars)
  • HTML tag + literal & interaction
  • Multiple marked ranges spanning literal &
  • Many bare & before a distant ; (performance guard for the bounded scan)

How to test

Spec validation

bundle exec rspec spec/lib/gitlab/string_range_marker_spec.rb
# 22 examples, 0 failures

MR acceptance checklist

  • Code review guidelines followed
  • Tests added for new behavior
  • No RuboCop violations
Edited by Kerri Miller

Merge request reports

Loading
Loading