Merge pull request rails#53657 from martinemde/martinemde/find_offset-test-and-fix

Redesign ActionView::Template::Handlers::ERB.find_offset to handle edge cases

Author: byroot

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