FIX: Always escape setext heading underlines

UPGRADING.md

@@ -1,5 +1,28 @@
 # Upgrading Markbridge
 
+## 0.4.1 — setext underlines are always escaped
+
+A text line that contains only `=` is now always escaped. Before, the
+escaper escaped it only when it saw a paragraph line directly in front
+of it inside the same text node.
+
+That check was too optimistic. The escaper works on one text fragment
+at a time, and the renderer joins the fragments afterwards.
+A fragment that starts with `===` can therefore end up right after a
+paragraph line — after a line break, or after inline markup — and
+Discourse cooks both lines as a heading:
+
+```ruby
+Markbridge.html_to_markdown("<p>Body:{}<br>========</p>").markdown
+# 0.4.0: "Body:{}\n========"          → cooks as an <h1>
+# 0.4.1: "Body:{}\n\\=\\=\\=\\=\\=\\=\\=\\="  → cooks as two lines of text
+```
+
+No public API changed. The cost is cosmetic: a separator line that has
+no paragraph in front of it now reads `\=\=\=\=` in the raw Markdown.
+Discourse renders `\=` as a literal `=`, so the result looks the same
+as before.
+
 ## 0.4.0 — ancestry matching and forced code blocks
 
 ### AST subclasses inherit rules and tags from their base class

lib/markbridge/renderers/discourse/markdown_escaper.rb

@@ -76,7 +76,6 @@ def initialize(escape_hard_line_breaks: false, allow: nil)
         FENCED_CODE_BACKTICK = /\A`{3,}[^`]*$/
         FENCED_CODE_TILDE = /\A~{3,}/
         SETEXT_UNDERLINE_EQUALS = /\A=+[ \t]*$/
-        SETEXT_UNDERLINE_DASH = /\A-+[ \t]*$/
         # Indented code: 4+ spaces, tab at start, or space+tab reaching column 4+
         INDENTED_CODE = /\A(?: {4}|\t| {1,3}\t)/
 
@@ -182,11 +181,11 @@ def escape_text(text)
           # skip the split and its Array + line-String allocations. A lone
           # `\r` without `\n` stays on the line either way — `/\r?\n/`
           # needs the `\n` — so `include?("\n")` alone decides correctly.
-          return escape_line(text, false) unless text.include?("\n")
+          return escape_line(text) unless text.include?("\n")
 
           # On CRLF input, consume `\r` as part of the line terminator instead
           # of leaving it on the line. A trailing `\r` breaks line-end anchored
-          # regexes (e.g. SETEXT_UNDERLINE_*) and the `ws_end >= line_length`
+          # regexes (e.g. SETEXT_UNDERLINE_EQUALS) and the `ws_end >= line_length`
           # early-out in escape_indented_code, leaking NBSPs onto
           # whitespace-only CRLF lines. The `include?` guard keeps the
           # LF-only fast path on a string split (regex split is ~20% slower
@@ -196,22 +195,19 @@ def escape_text(text)
           # Pre-allocate result buffer
           bytesize = text.bytesize
           result = String.new(capacity: bytesize + bytesize / 3, encoding: text.encoding)
-          prev_was_paragraph = false
           first = true
 
           lines.each do |line|
             result << "\n" unless first
             first = false
 
-            escaped = escape_line(line, prev_was_paragraph)
-            result << escaped
-            prev_was_paragraph = paragraph_line?(line)
+            result << escape_line(line)
           end
 
           result
         end
 
-        def escape_line(line, prev_was_paragraph)
+        def escape_line(line)
           # No `line.empty?` early-return: it's redundant with the
           # `line.getbyte(indent_len).nil?` guard below, which catches both
           # empty and whitespace-only lines while also preserving object
@@ -229,7 +225,7 @@ def escape_line(line, prev_was_paragraph)
           has_indent = indent_len > 0
           content = has_indent ? line[indent_len..] : line
 
-          escaped, skip_inline = escape_block_level(content, prev_was_paragraph)
+          escaped, skip_inline = escape_block_level(content)
           escaped = escape_inline(escaped) unless skip_inline
 
           if has_indent
@@ -276,7 +272,7 @@ def escape_indented_code(line)
           "#{nbsp_indent}#{escape_inline(content)}"
         end
 
-        def escape_block_level(content, prev_was_paragraph)
+        def escape_block_level(content)
           first_byte = content.getbyte(0)
 
           case first_byte
@@ -289,7 +285,7 @@ def escape_block_level(content, prev_was_paragraph)
             return pass_first_char_inline(content) if @allow.include?(:block_quote)
             return escape_first_char_inline(content, "\\>")
           when DASH
-            return escape_block_dash(content, prev_was_paragraph)
+            return escape_block_dash(content)
           when PLUS
             if BULLET_LIST.match?(content)
               return pass_first_char_inline(content) if @allow.include?(:bullet_list)
@@ -302,7 +298,17 @@ def escape_block_level(content, prev_was_paragraph)
               return escape_all_chars(content, UNDERSCORE, "\\_"), true
             end
           when EQUALS
-            if prev_was_paragraph && SETEXT_UNDERLINE_EQUALS.match?(content)
+            # A line of only `=` is a setext heading underline when a
+            # paragraph line comes before it. The escaper sees a single
+            # text fragment, and the renderer can put that fragment
+            # right after a paragraph line — after a line break, or
+            # after inline markup like `[b]Body[/b]\n===` — so the
+            # previous line is not visible here. The line is therefore
+            # escaped in every position, like every other block
+            # construct. Where no paragraph line comes before it,
+            # Discourse renders `\=` as a literal `=`, so the result
+            # looks the same.
+            if SETEXT_UNDERLINE_EQUALS.match?(content)
               return escape_all_chars(content, EQUALS, "\\="), true
             end
           when BACKTICK
@@ -327,11 +333,8 @@ def escape_first_char_inline(content, escaped_char)
           ["#{escaped_char}#{escape_inline(content[1..])}", true]
         end
 
-        def escape_block_dash(content, prev_was_paragraph)
-          if THEMATIC_BREAK_DASH.match?(content) ||
-               (prev_was_paragraph && SETEXT_UNDERLINE_DASH.match?(content))
-            return escape_all_chars(content, DASH, "\\-"), true
-          end
+        def escape_block_dash(content)
+          return escape_all_chars(content, DASH, "\\-"), true if THEMATIC_BREAK_DASH.match?(content)
           if BULLET_LIST.match?(content)
             return pass_first_char_inline(content) if @allow.include?(:bullet_list)
             return escape_first_char_inline(content, "\\-")
@@ -562,56 +565,6 @@ def utf8_char_length(first_byte)
             1
           end
         end
-
-        def paragraph_line?(line)
-          pos = 0
-          line_len = line.bytesize
-          pos += 1 while pos < line_len && line.getbyte(pos) == SPACE
-          first_non_space = pos
-
-          # Empty or whitespace-only lines: getbyte past the end returns nil.
-          return false if line.getbyte(first_non_space).nil?
-
-          # Indented code (4+ spaces or any leading \t) is not a paragraph.
-          # INDENTED_CODE also catches lines where first_non_space > 3, so no
-          # separate numeric boundary check is needed.
-          return false if INDENTED_CODE.match?(line)
-
-          content = first_non_space == 0 ? line : line[first_non_space..]
-
-          # Lines starting with [ are paragraph content (the escaper rewrites [
-          # to \[). block_construct? has no BRACKET_OPEN case arm, so such
-          # lines naturally fall through and !block_construct?(content) == true.
-          !block_construct?(content)
-        end
-
-        # Checks whether content starts with a block-level markdown construct.
-        # Used by both escape_block_level (to decide what to escape) and
-        # paragraph_line? (to decide if setext underlines can follow).
-        def block_construct?(content)
-          case content.getbyte(0)
-          when HASH
-            ATX_HEADING.match?(content)
-          when GT
-            true
-          when DASH
-            BULLET_LIST.match?(content) || THEMATIC_BREAK_DASH.match?(content)
-          when STAR
-            BULLET_LIST.match?(content) || THEMATIC_BREAK_STAR.match?(content)
-          when PLUS
-            BULLET_LIST.match?(content)
-          when UNDERSCORE
-            THEMATIC_BREAK_UNDERSCORE.match?(content)
-          when BACKTICK
-            FENCED_CODE_BACKTICK.match?(content)
-          when TILDE
-            FENCED_CODE_TILDE.match?(content)
-          when DIGIT_0..DIGIT_9
-            ORDERED_LIST.match?(content)
-          else
-            false
-          end
-        end
       end
     end
   end

mutant.yml

@@ -115,7 +115,6 @@ matcher:
     # and surface as timeouts, never alive. All Bucket A.
     - Markbridge::Renderers::Discourse::MarkdownEscaper#escape_line
     - Markbridge::Renderers::Discourse::MarkdownEscaper#escape_indented_code
-    - Markbridge::Renderers::Discourse::MarkdownEscaper#paragraph_line?
 
 # escape_block_* fallthrough `[content, false]` returns. The `false`
     # → drop-second / `true` mutations on the non-match branch are
@@ -401,20 +400,6 @@ mutation:
     - "send{receiver=send{receiver=self selector=class} selector=new}"
     - "index{receiver=lvar{value=new_cache}}"
 
-# MarkdownEscaper#block_construct?'s `when DIGIT_0..DIGIT_9` range.
-    # Mutations `when DIGIT_0..nil` / `when nil..DIGIT_9` extend the
-    # range to unbounded, but the body is `ORDERED_LIST.match?(content)`
-    # which only matches content starting with `\d+\.` — non-digit bytes
-    # like `:` or `<` land in this arm but produce `false` identically
-    # to the `else false` arm. Dropping `else false` also fine: case
-    # returns nil, `!block_construct?(content)` becomes `!nil == true`
-    # — same as `!false == true` for the `paragraph_line?` caller.
-    # Bucket A.
-    - "case{value=send{receiver=lvar{value=content} selector=getbyte}}"
-
-
-
-
 
 # String.new(capacity:, encoding:) calls are preallocation hints —
     # capacity is a tuning knob with no observable effect on output, and
@@ -464,24 +449,23 @@ mutation:
     - "lvasgn{name=has_indent}"
     - "if{condition=lvar{value=has_indent}}"
 
-    # Nested `line.getbyte(i).<selector>` guards in escape_line and
-    # paragraph_line? (`.nil?` / `!=`). Mutations on selector equality
-    # variants and drop-guard are equivalent because getbyte returns
-    # nil past the end and Integer equality is the same for Fixnum.
+    # Nested `line.getbyte(i).<selector>` guards in escape_line
+    # (`.nil?` / `!=`). Mutations on selector equality variants and
+    # drop-guard are equivalent because getbyte returns nil past the
+    # end and Integer equality is the same for Fixnum.
     - "if{condition=send{receiver=send{receiver=lvar{value=line} selector=getbyte} selector=(nil?,!=)}}"
 
     # Allocation-saving ternary `content = <cond> ? line[N..] : line`
-    # in escape_line and paragraph_line?. Output bytes identical; only
-    # object identity differs, which is an internal contract.
+    # in escape_line. Output bytes identical; only object identity
+    # differs, which is an internal contract.
     - "lvasgn{name=content value=if}"
 
     # escape_block_level's `case first_byte` dispatch. Mutations on
     # `when` conditions (`when STAR` → `when nil`, etc.) make the
     # branch unreachable. The fallthrough `[content, false]` + inline
     # escaping produces byte-identical output for STAR/UNDERSCORE/
     # BACKTICK/TILDE/BRACKET_OPEN/PIPE inputs because inline escape
-    # wraps the same characters. block_construct?'s different value
-    # shape (`case content.getbyte(0)`) is unaffected.
+    # wraps the same characters.
     - "case{value=lvar{value=first_byte}}"
 
     # escape_regular_char's `if byte < 128` ASCII fast-path. ASCII

spec/markbridge_spec.rb

@@ -297,6 +297,15 @@ def render(_e, _i)
 
       expect(result.markdown).to eq("**hi** extra")
     end
+
+    # The `=` line lands in its own text fragment after the <br>, so the
+    # escaper cannot see the paragraph line in front of it. Without the
+    # escape, Discourse cooks the two lines as an <h1>.
+    it "escapes a =-only line that follows a line break" do
+      result = described_class.html_to_markdown("<p>Body:{}<br>========</p>")
+
+      expect(result.markdown).to eq("Body:{}\n\\=\\=\\=\\=\\=\\=\\=\\=")
+    end
   end
 
   describe ".parse_text_formatter_xml" do

spec/system/bbcode_to_markdown_spec.rb

@@ -534,6 +534,19 @@
       expect(result.markdown).to eq("**bold text**")
     end
 
+    # The `=` line starts its own text node, so the escaper cannot see the
+    # paragraph line that the inline markup produced before it. Without the
+    # escape, Discourse cooks both lines as an <h1>.
+    it "escapes a =-only line after a bold tag" do
+      result = Markbridge.bbcode_to_markdown("[b]Body[/b]\n===")
+      expect(result.markdown).to eq("**Body**\n\\=\\=\\=")
+    end
+
+    it "escapes a =-only line after text with inline markup" do
+      result = Markbridge.bbcode_to_markdown("Body [i]x[/i]\n===")
+      expect(result.markdown).to eq("Body *x*\n\\=\\=\\=")
+    end
+
     it "inserts an HTML comment to break colliding emphasis delimiters between siblings" do
       # After reorder-with-reopen the Bold ends with *** and the reopened
       # Italic starts with * — adjacent they would form **** and parse

spec/unit/markbridge/renderers/discourse/markdown_escaper/setext_headings_spec.rb

@@ -39,10 +39,20 @@
       end
     end
 
-    context "when = is standalone (MAY escape - false positives OK)" do
-      it "may or may not escape standalone ===" do
-        result = escaper.escape("===")
-        expect(result).to eq("===").or eq("\\=\\=\\=")
+    context "when the = line has no paragraph before it (MUST escape)" do
+      # The escaper only sees one text fragment. The renderer can place
+      # that fragment after a paragraph line, so a `=`-only line is always
+      # escaped. Discourse shows `\=` as a literal `=`.
+      it "escapes a standalone ===" do
+        expect(escaper.escape("===")).to eq("\\=\\=\\=")
+      end
+
+      it "escapes === after a blank line" do
+        expect(escaper.escape("Text\n\n===")).to eq("Text\n\n\\=\\=\\=")
+      end
+
+      it "escapes === after a list item" do
+        expect(escaper.escape("- item\n===")).to eq("\\- item\n\\=\\=\\=")
       end
     end