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):
- 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. - Trailing bare
&: The scan reaches end-of-string, advancesrich_pospastrich_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_endhelper: looks ahead from&to;, returns the index of;if the span is a valid entity, otherwisenil - 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_endreturns a valid index, otherwise map&as 1:1 - Bounded the forward scan at
MAX_ENTITY_LENGTH(33, the longest HTML5 named entity), eliminating theO(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 failuresMR acceptance checklist
- Code review guidelines followed
- Tests added for new behavior
- No RuboCop violations