From aaebc8635123537f60db62763e6d80489098060d Mon Sep 17 00:00:00 2001 From: Martin Emde Date: Sun, 17 Nov 2024 10:41:47 -0800 Subject: [PATCH] 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. --- actionview/CHANGELOG.md | 6 + .../lib/action_view/template/handlers/erb.rb | 76 +++++------ actionview/test/template/template_test.rb | 119 ++++++++++++++++++ 3 files changed, 166 insertions(+), 35 deletions(-) diff --git a/actionview/CHANGELOG.md b/actionview/CHANGELOG.md index 11e7cfd9dae3c..d389af1d531e2 100644 --- a/actionview/CHANGELOG.md +++ b/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* diff --git a/actionview/lib/action_view/template/handlers/erb.rb b/actionview/lib/action_view/template/handlers/erb.rb index 781217fc93b05..4791d5629995f 100644 --- a/actionview/lib/action_view/template/handlers/erb.rb +++ b/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 diff --git a/actionview/test/template/template_test.rb b/actionview/test/template/template_test.rb index 7876849044cf8..346aed8b91a8c 100644 --- a/actionview/test/template/template_test.rb +++ b/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 "#", @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