FIX: Escape ] in URL label to prevent early link termination#40
Merged
Conversation
MarkdownEscaper deliberately skips escaping ] in normal prose where it's harmless, but ] is structural inside a Markdown link label. UrlTag and EmailTag both wrap already-escaped children in [label](url), so an input like "[ABC-123] some title" was rendered as "[\[ABC-123] some title](…)" — the unescaped ] closed the label early and Discourse rendered the result as literal text. Escape ] at each bracket-emitting site rather than in the shared escaper so prose output stays untouched.
gschlager
force-pushed
the
fix/escape-bracket-in-url-label
branch
from
841d99b to
2efccab
Compare
4 tasks
gschlager
added a commit
that referenced
this pull request
May 12, 2026
The previous fix (#40) escaped ] in UrlTag/EmailTag with an unconditional `text.gsub("]") { "\\]" }`. That regressed nested markdown that legitimately produces unescaped ] — for example an image inside a link: <a><img src="…"/></a> renders the image as ``, and the tag-level gsub turned the label into `[](href)` — broken. The escape rule is contextual to the text content, not to how it gets wrapped. Move the decision to where escaping already happens: - MarkdownEscaper#escape grows an `in_link_label:` kwarg. When set and the result contains ], it post-processes the result to escape every ]. The non-label hot path is unchanged (one boolean check). - Renderer#render_text detects an Url/Email ancestor via has_parent? and passes the flag through. Only plain Text nodes flow through this — nested tag output (ImageTag, UploadTag, etc.) doesn't touch render_text, so its structural ] is preserved. - UrlTag and EmailTag go back to a plain "[#{text}](#{href})" wrapper. Performance: benchmarked the hot path (non-label) against main at 1.79–1.84M i/s for plain text and ~245k i/s for text with specials — no measurable regression. The label path's no-`]` fast-return preserves the input string instance (locked in by a spec).
4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
MarkdownEscaperskips escaping]in prose (harmless there, and a deliberate perf simplification), but]is structural inside a Markdown link labelUrlTagandEmailTagboth wrap already-escaped children in[label](url), so a link with text like[ABC-123] some titlewas rendered as[\[ABC-123] some title](…)— the unescaped]closed the label early and Discourse rendered it as literal text]at the bracket-emitting site in each tag (not in the shared escaper) so prose stays untouchedhtml_modepath (which emits<a>…</a>) is unaffected;]is not structural in HTMLTest plan
bundle exec rspec spec/unit/markbridge/renderers/discourse/tags/url_tag_spec.rb— 17 examples, 0 failuresbundle exec rspec spec/unit/markbridge/renderers/discourse/tags/email_tag_spec.rb— 7 examples, 0 failuresbundle exec rspec— full suite: 3106 examples, 0 failuresbin/lint— no offenses