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