Handle edge cases in ActionView::Template::Handlers::ERB.find_offset

The code for finding offsets in the ERB templates outright fails in some
cases, causing 500s in the error template. In other cases it will bail
out early when it's possible to find and highlight correctly. This PR
fixes many of the issues that regularly occur and correctly highlights
more often. Also adds test coverage of the translate_location method on
ActionView::Template, many of which failed before this fix.

Author: martinemde

actionview/CHANGELOG.md

@@ -1,3 +1,9 @@
+*   Improve reliability of ERB template error highlighting.
+    Fix infinite loops and crashes in highlighting and
+    improve tolerance for alternate ERB handlers.
+
+    *Martin Emde*
+
 *   Allow `hidden_field` and `hidden_field_tag` to accept a custom autocomplete value.
 
     *brendon*

actionview/lib/action_view/template/handlers/erb.rb

@@ -105,51 +105,57 @@ def valid_encoding(string, encoding)
           raise WrongEncodingError.new(string, string.encoding)
         end
 
+        # Find which token in the source template spans the byte range that
+        # contains the error_column, then return the offset compared to the
+        # original source template.
+        #
+        # Iterate consecutive pairs of CODE or TEXT tokens, requiring
+        # a match of the first token before matching either token.
+        #
+        # For example, if we want to find tokens A, B, C, we do the following:
+        # 1. Find a match for A: test error_column or advance scanner.
+        # 2. Find a match for B or A:
+        #   a. If B: start over with next token set (B, C).
+        #   b. If A: test error_column or advance scanner.
+        #   c. Otherwise: Advance 1 byte
+        #
+        # Prioritize matching the next token over the current token once
+        # a match for the current token has been found. This is to prevent
+        # the current token from looping past the next token if they both
+        # match (i.e. if the current token is a single space character).
         def find_offset(compiled, source_tokens, error_column)
           compiled = StringScanner.new(compiled)
+          offset_source_tokens(source_tokens).each_cons(2) do |(name, str, offset), (_, next_str, _)|
+            matched_str = false
 
-          passed_tokens = []
+            until compiled.eos?
+              if matched_str && next_str && compiled.match?(next_str)
+                break
+              elsif compiled.match?(str)
+                matched_str = true
 
-          while tok = source_tokens.shift
-            tok_name, str = *tok
-            case tok_name
-            when :TEXT
-              loop do
-                break if compiled.match?(str)
-                compiled.getch
-              end
-              raise LocationParsingError unless compiled.scan(str)
-            when :CODE
-              if compiled.pos > error_column
-                raise LocationParsingError, "We went too far"
-              end
+                if name == :CODE && compiled.pos <= error_column && compiled.pos + str.bytesize >= error_column
+                  return error_column - compiled.pos + offset
+                end
 
-              if compiled.pos + str.bytesize >= error_column
-                offset = error_column - compiled.pos
-                return passed_tokens.map(&:last).join.bytesize + offset
+                compiled.pos += str.bytesize
               else
-                unless compiled.scan(str)
-                  raise LocationParsingError, "Couldn't find code snippet"
-                end
-              end
-            when :OPEN
-              next_tok = source_tokens.first.last
-              loop do
-                break if compiled.match?(next_tok)
-                compiled.getch
+                compiled.pos += 1
               end
-            when :CLOSE
-              next_tok = source_tokens.first.last
-              loop do
-                break if compiled.match?(next_tok)
-                compiled.getch
-              end
-            else
-              raise LocationParsingError, "Not implemented: #{tok.first}"
             end
+          end
+
+          raise LocationParsingError, "Couldn't find code snippet"
+        end
 
-            passed_tokens << tok
+        def offset_source_tokens(source_tokens)
+          source_offset = 0
+          with_offset = source_tokens.filter_map do |(name, str)|
+            result = [name, str, source_offset] if name == :CODE || name == :TEXT
+            source_offset += str.bytesize
+            result
           end
+          with_offset << [:EOS, nil, source_offset]
         end
       end
     end

actionview/test/template/template_test.rb

@@ -65,6 +65,18 @@ def render(implicit_locals: [], **locals)
     @template.render(@context, locals, implicit_locals: implicit_locals)
   end
 
+  def spot_highlight(compiled, highlight, first_column: nil, **options)
+    # rindex by default since our tests usually put the highlight last
+    first_column ||= compiled.rindex(highlight) || 999
+    last_column = first_column + highlight.size
+    spot = {
+      first_column:, last_column:, snippet: compiled,
+      first_lineno: 1, last_lineno: 1, script_lines: compiled.lines,
+    }
+    spot.merge!(options)
+    spot
+  end
+
   def setup
     @context = Context.with_empty_template_cache.empty
     super
@@ -314,4 +326,111 @@ def test_template_inspect
     @template = new_template("hello")
     assert_equal "#<ActionView::Template hello template locals=[]>", @template.inspect
   end
+
+  def test_template_translate_location
+    highlight = "nomethoderror"
+    source = "<%= nomethoderror %>"
+    compiled = "'.freeze; @output_buffer.append=  nomethoderror ; @output_buffer.safe_append='\n"
+
+    backtrace_location = Data.define(:lineno).new(lineno: 1)
+    spot = spot_highlight(compiled, highlight)
+    expected = spot_highlight(source, highlight, snippet: compiled)
+
+    assert_equal expected, new_template(source).translate_location(backtrace_location, spot)
+  end
+
+  def test_template_translate_location_lineno_offset
+    highlight = "nomethoderror"
+    source = "<%= nomethoderror %>"
+    compiled = "\n'.freeze; @output_buffer.append=  nomethoderror ; @output_buffer.safe_append='\n"
+
+    backtrace_location = Data.define(:lineno).new(lineno: 1)
+    spot = spot_highlight(compiled, highlight, first_lineno: 2, last_lineno: 2)
+    expected = spot_highlight(source, highlight, snippet: compiled)
+
+    assert_equal expected, new_template(source).translate_location(backtrace_location, spot)
+  end
+
+  def test_template_translate_location_with_multibye_string_before_highlight
+    highlight = "nomethoderror"
+    source = String.new("\u{a5}<%= nomethoderror %>", encoding: Encoding::UTF_8) # yen symbol
+    compiled = String.new("\u{a5}'.freeze; @output_buffer.append=  nomethoderror ; @output_buffer.safe_append='\n", encoding: Encoding::UTF_8)
+
+    backtrace_location = Data.define(:lineno).new(lineno: 1)
+    spot = spot_highlight(compiled, highlight)
+    expected = spot_highlight(source, highlight, snippet: compiled)
+
+    assert_equal expected, new_template(source).translate_location(backtrace_location, spot)
+  end
+
+  def test_template_translate_location_no_match_in_compiled
+    highlight = "nomatch"
+    source = "<%= nomatch %>"
+    compiled = "this source does not contain the highlight, so the original spot is returned"
+
+    backtrace_location = Data.define(:lineno).new(lineno: 1)
+    spot = spot_highlight(compiled, highlight, first_column: 50)
+
+    assert_equal spot, new_template(source).translate_location(backtrace_location, spot)
+  end
+
+  def test_template_translate_location_text_includes_highlight
+    highlight = "nomethoderror"
+    source = " nomethoderror <%= nomethoderror %>"
+    compiled = " nomethoderror '.freeze; @output_buffer.append=  nomethoderror ; @output_buffer.safe_append='\n"
+
+    backtrace_location = Data.define(:lineno).new(lineno: 1)
+    spot = spot_highlight(compiled, highlight)
+    expected = spot_highlight(source, highlight, snippet: compiled)
+
+    assert_equal expected, new_template(source).translate_location(backtrace_location, spot)
+  end
+
+  def test_template_translate_location_space_separated_erb_tags
+    highlight = "nomethoderror"
+    source = "<%= goodcode %> <%= nomethoderror %>"
+    compiled = "'.freeze; @output_buffer.append=  goodcode ; @output_buffer.safe_append=' '.freeze; @output_buffer.append=  nomethoderror ; @output_buffer.safe_append='\n"
+
+    backtrace_location = Data.define(:lineno).new(lineno: 1)
+    spot = spot_highlight(compiled, highlight)
+    expected = spot_highlight(source, highlight, snippet: compiled)
+
+    assert_equal expected, new_template(source).translate_location(backtrace_location, spot)
+  end
+
+  def test_template_translate_location_consecutive_erb_tags
+    highlight = "nomethoderror"
+    source = "<%= goodcode %><%= nomethoderror %>"
+    compiled = "'.freeze; @output_buffer.append=  goodcode ; @output_buffer.append=  nomethoderror ; @output_buffer.safe_append='\n"
+
+    backtrace_location = Data.define(:lineno).new(lineno: 1)
+    spot = spot_highlight(compiled, highlight)
+    expected = spot_highlight(source, highlight, snippet: compiled)
+
+    assert_equal expected, new_template(source).translate_location(backtrace_location, spot)
+  end
+
+  def test_template_translate_location_repeated_highlight_in_compiled_template
+    highlight = "nomethoderror"
+    source = "<%= nomethoderror %>"
+    compiled = "ValidatedOutputBuffer.wrap(@output_buffer, ({}), ' nomethoderror '.freeze, true).safe_none_append=  nomethoderror ; @output_buffer.safe_append='\n"
+
+    backtrace_location = Data.define(:lineno).new(lineno: 1)
+    spot = spot_highlight(compiled, highlight)
+    expected = spot_highlight(source, highlight, snippet: compiled)
+
+    assert_equal expected, new_template(source).translate_location(backtrace_location, spot)
+  end
+
+  def test_template_translate_location_flaky_pathological_template
+    highlight = "flakymethod"
+    source = "<%= flakymethod %> flakymethod <%= flakymethod " # fails on second call, no tailing %>
+    compiled = "ValidatedOutputBuffer.wrap(@output_buffer, ({}), ' flakymethod '.freeze, true).safe_none_append=( flakymethod );@output_buffer.safe_append=' flakymethod '.freeze;ValidatedOutputBuffer.wrap(@output_buffer, ({}), ' flakymethod '.freeze, true).safe_none_append=( flakymethod "
+
+    backtrace_location = Data.define(:lineno).new(lineno: 1)
+    spot = spot_highlight(compiled, highlight)
+    expected = spot_highlight(source, highlight, snippet: compiled)
+
+    assert_equal expected, new_template(source).translate_location(backtrace_location, spot)
+  end
 end