From e1eae610c08d45fab509fd0d3c2e475cf0327bd3 Mon Sep 17 00:00:00 2001 From: Suhaib Mujahid Date: Tue, 25 Aug 2026 12:35:07 -0400 Subject: [PATCH 1/5] Bug 2060932 - Support GitHub-style collapsible sections in comments Comments render markdown with cmark's safe option and every `<` escaped beforehand, so raw HTML never reaches the parser. Instead of weakening that, convert the four disclosure tags back to real elements after rendering: mark the escaped tags in text nodes (skipping pre/code so the syntax can still be documented), then re-parse so the block level elements are lifted out of the paragraph markdown wrapped them in. Only those four exact tags are recognized and they never carry attributes, so no other markup can be smuggled in. The marker characters are stripped from the input so they cannot be forged, and unbalanced tags cannot leak an unclosed element into the page. --- Bugzilla/Markdown.pm | 65 +++++++++++++++++++++++++++++++++++++-- skins/standard/global.css | 12 ++++++++ t/markdown.t | 63 +++++++++++++++++++++++++++++++++++++ 3 files changed, 137 insertions(+), 3 deletions(-) diff --git a/Bugzilla/Markdown.pm b/Bugzilla/Markdown.pm index 2a77b4932c..efc67daad9 100644 --- a/Bugzilla/Markdown.pm +++ b/Bugzilla/Markdown.pm @@ -36,6 +36,30 @@ sub _build_markdown_parser { } my $MARKDOWN_OFF = quotemeta '#[markdown(off)]'; + +# The only raw HTML allowed in comments: GitHub-style collapsible sections. +# Markdown rendering escapes all tags, so the escaped text is swapped back to +# real elements afterwards. Only these exact tags are recognized and they never +# carry attributes, so no other markup can be smuggled in. +my %DISCLOSURE_MARKER = ( + '
' => "\x{E000}", + '
' => "\x{E001}", + '' => "\x{E002}", + '' => "\x{E003}", +); + +# Markdown wraps the tags in a paragraph. Closing and reopening it lets the +# HTML parser lift the block level disclosure elements out of the paragraph; +# the empty paragraphs left behind are dropped afterwards. +my %DISCLOSURE_HTML = ( + "\x{E000}" => '

', + "\x{E001}" => '

', + "\x{E002}" => '

', + "\x{E003}" => '

', +); + +my $DISCLOSURE_RE = qr{}i; + sub render_html { my ($self, $markdown, $bug, $comment, $user) = @_; my $parser = $self->markdown_parser; @@ -60,9 +84,12 @@ sub render_html { return $html; } + my $has_disclosure = $markdown =~ $DISCLOSURE_RE; + # Replace < with \x{FFFD} (special unicode replacement character), - # and remove \x{FFFD} later. - $markdown =~ tr/\x{FFFD}//d; + # and remove \x{FFFD} later. The private use characters reserved for the + # disclosure markers are dropped too, so they can't be forged in a comment. + $markdown =~ tr/\x{FFFD}\x{E000}-\x{E003}//d; $markdown =~ s{<(?!https?://)}{\x{FFFD}}gs; my @valid_text_parent_tags = ('h1', 'h2', 'h3', 'h4', 'h5', 'h6', 'p', 'li', 'td'); @@ -91,8 +118,40 @@ sub render_html { }); return $node; }); - return $dom->to_string; + return $has_disclosure ? _expand_disclosure_tags($dom) : $dom->to_string; +} + +# Turn the escaped

/ text left by the markdown renderer back +# into real elements. Text inside code blocks is skipped so the syntax can +# still be documented in a comment. +sub _expand_disclosure_tags { + my ($dom) = @_; + + my $found = 0; + $dom->descendant_nodes->each(sub { + my ($node) = @_; + return unless $node->type eq 'text'; + return if $node->ancestors('pre, code')->size; + my $text = $node->content; + return unless $text =~ s/($DISCLOSURE_RE)/$DISCLOSURE_MARKER{lc $1}/g; + $found = 1; + $node->content($text); + }); + + my $html = $dom->to_string; + return $html unless $found; + + $html =~ s/([\x{E000}-\x{E003}])/$DISCLOSURE_HTML{$1}/g; + + # Drop the line breaks and empty paragraphs the rewrite leaves behind. + $html =~ s{\s*\s*(?=

)}{}g; + $html =~ s{(?<=

)\s*\s*}{}g; + + my $expanded = Mojo::DOM->new($html); + $expanded->find('p') + ->grep(sub { !$_->children->size && $_->all_text !~ /\S/ })->map('remove'); + return $expanded->to_string; } sub _is_external_link { diff --git a/skins/standard/global.css b/skins/standard/global.css index ce8d31c36e..564a110412 100644 --- a/skins/standard/global.css +++ b/skins/standard/global.css @@ -2724,6 +2724,18 @@ div.bz_comment_text pre { margin: 0; } +.markdown-body details { + margin-bottom: 10px; +} + +.markdown-body details > *:last-child { + margin-bottom: 0; +} + +.markdown-body summary { + cursor: pointer; +} + .markdown-body ul, .markdown-body ol { padding-left: 0; diff --git a/t/markdown.t b/t/markdown.t index f8b541d5b2..30d10c6496 100644 --- a/t/markdown.t +++ b/t/markdown.t @@ -98,4 +98,67 @@ is($ahref->attr('href'), 'https://searchfox.org/mozilla-central/rev/76fe4bb38534 is($parser->render_html(''), "

<foo>

\n", "literal tags work"); +# Bug 2060932: collapsible sections via
/. +is( + $parser->render_html('
Text to click' + . 'Text hidden by default
'), + '
Text to click' + . "

Text hidden by default

\n", + 'Disclosure tags on a single line' +); + +my $details_block = <<'MARKDOWN'; +
+Click **me** + +Hidden content + +
+MARKDOWN + +is( + $parser->render_html($details_block), + "
Click me\n" + . "

Hidden content

\n
\n", + 'Disclosure tags as their own blocks, with markdown in the summary' +); + +is( + $parser->render_html("
Uphidden
"), + "
Up

hidden

\n", + 'Disclosure tags are case insensitive' +); + +is( + $parser->render_html("```\n
xy
\n```"), + "
<details><summary>x</summary>"
+    . "y</details>\n
\n", + 'Disclosure tags in a code block stay literal' +); + +is( + $parser->render_html('Use `
` to fold.'), + "

Use <details> to fold.

\n", + 'Disclosure tags in a code span stay literal' +); + +like( + $parser->render_html('
nope'), + qr{<details open onclick="x">nope}, + 'Only the bare disclosure tags are recognized' +); + +is( + $parser->render_html("\x{E000}\x{E002}nope\x{E003}\x{E001}"), + "

nope

\n", + 'The internal disclosure markers cannot be forged in a comment' +); + +# An unbalanced tag must not leak an unclosed element into the page. +like( + $parser->render_html("
\noops\n\nrest\n"), + qr{
\z}, + 'An unclosed disclosure section is closed for us' +); + done_testing; From bfab908547a4d7954849ea25f60654e814a76c58 Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Wed, 16 Sep 2026 10:54:29 -0400 Subject: [PATCH 2/5] Copilot fixes and sanity test fix --- Bugzilla/Markdown.pm | 67 +++++++++++++++++++++++++++++--------------- t/markdown.t | 27 +++++++++++++++++- 2 files changed, 71 insertions(+), 23 deletions(-) diff --git a/Bugzilla/Markdown.pm b/Bugzilla/Markdown.pm index efc67daad9..937e23a502 100644 --- a/Bugzilla/Markdown.pm +++ b/Bugzilla/Markdown.pm @@ -38,27 +38,33 @@ sub _build_markdown_parser { my $MARKDOWN_OFF = quotemeta '#[markdown(off)]'; # The only raw HTML allowed in comments: GitHub-style collapsible sections. -# Markdown rendering escapes all tags, so the escaped text is swapped back to -# real elements afterwards. Only these exact tags are recognized and they never -# carry attributes, so no other markup can be smuggled in. +# The raw tags are swapped for private use characters before the markdown is +# parsed, and those markers are turned into real elements afterwards. Marking +# them up front is what keeps a raw tag distinct from text that merely renders +# as one, such as an entity-encoded tag. Only these exact tags are recognized +# and they never carry attributes, so no other markup can be smuggled in. my %DISCLOSURE_MARKER = ( - '
' => "\x{E000}", - '
' => "\x{E001}", - '' => "\x{E002}", - '' => "\x{E003}", + '
' => chr 0xE000, + '
' => chr 0xE001, + '' => chr 0xE002, + '' => chr 0xE003, ); +# Marker back to the tag it stands for, for markers that cannot be expanded. +my %DISCLOSURE_TAG = reverse %DISCLOSURE_MARKER; + # Markdown wraps the tags in a paragraph. Closing and reopening it lets the # HTML parser lift the block level disclosure elements out of the paragraph; # the empty paragraphs left behind are dropped afterwards. my %DISCLOSURE_HTML = ( - "\x{E000}" => '

', - "\x{E001}" => '

', - "\x{E002}" => '

', - "\x{E003}" => '

', + $DISCLOSURE_MARKER{'

'} => '

', + $DISCLOSURE_MARKER{'

'} => '

', + $DISCLOSURE_MARKER{'

'} => '

', + $DISCLOSURE_MARKER{''} => '

', ); -my $DISCLOSURE_RE = qr{}i; +my $DISCLOSURE_RE = qr{}i; +my $DISCLOSURE_MARKER_RE = qr{[\x{E000}-\x{E003}]}; sub render_html { my ($self, $markdown, $bug, $comment, $user) = @_; @@ -84,12 +90,16 @@ sub render_html { return $html; } - my $has_disclosure = $markdown =~ $DISCLOSURE_RE; - # Replace < with \x{FFFD} (special unicode replacement character), # and remove \x{FFFD} later. The private use characters reserved for the # disclosure markers are dropped too, so they can't be forged in a comment. $markdown =~ tr/\x{FFFD}\x{E000}-\x{E003}//d; + + # Mark the raw disclosure tags before the markdown is parsed, so that only + # these occurrences can ever become elements again. + my $has_disclosure + = $markdown =~ s/($DISCLOSURE_RE)/$DISCLOSURE_MARKER{lc $1}/g; + $markdown =~ s{<(?!https?://)}{\x{FFFD}}gs; my @valid_text_parent_tags = ('h1', 'h2', 'h3', 'h4', 'h5', 'h6', 'p', 'li', 'td'); @@ -121,27 +131,40 @@ sub render_html { return $has_disclosure ? _expand_disclosure_tags($dom) : $dom->to_string; } -# Turn the escaped

/ text left by the markdown renderer back -# into real elements. Text inside code blocks is skipped so the syntax can -# still be documented in a comment. +# Turn the markers left in place of the raw
/ tags into real +# elements. A marker that ended up somewhere it cannot be expanded is restored +# as literal text: inside a code block, so the syntax can still be documented +# in a comment, or inside an attribute value, where only text belongs. sub _expand_disclosure_tags { my ($dom) = @_; my $found = 0; $dom->descendant_nodes->each(sub { my ($node) = @_; - return unless $node->type eq 'text'; - return if $node->ancestors('pre, code')->size; + + if ($node->type eq 'tag') { + my $attr = $node->attr; + foreach my $key (keys %$attr) { + next unless defined $attr->{$key}; + $attr->{$key} =~ s/($DISCLOSURE_MARKER_RE)/$DISCLOSURE_TAG{$1}/g; + } + return; + } + my $text = $node->content; - return unless $text =~ s/($DISCLOSURE_RE)/$DISCLOSURE_MARKER{lc $1}/g; - $found = 1; + return unless $text =~ $DISCLOSURE_MARKER_RE; + if ($node->type eq 'text' && !$node->ancestors('pre, code')->size) { + $found = 1; + return; + } + $text =~ s/($DISCLOSURE_MARKER_RE)/$DISCLOSURE_TAG{$1}/g; $node->content($text); }); my $html = $dom->to_string; return $html unless $found; - $html =~ s/([\x{E000}-\x{E003}])/$DISCLOSURE_HTML{$1}/g; + $html =~ s/($DISCLOSURE_MARKER_RE)/$DISCLOSURE_HTML{$1}/g; # Drop the line breaks and empty paragraphs the rewrite leaves behind. $html =~ s{\s*\s*(?=

)}{}g; diff --git a/t/markdown.t b/t/markdown.t index 30d10c6496..21bf35dcd8 100644 --- a/t/markdown.t +++ b/t/markdown.t @@ -148,8 +148,33 @@ like( 'Only the bare disclosure tags are recognized' ); +# Only the raw tags in the comment are expanded; text that merely renders as a +# tag must not be able to close the section early and reveal hidden content. is( - $parser->render_html("\x{E000}\x{E002}nope\x{E003}\x{E001}"), + $parser->render_html('
x' + . '</details>hidden
'), + '
x' + . "

</details>hidden

\n", + 'Entity encoded disclosure tags are not expanded' +); + +is( + $parser->render_html('Use `
` for <summary>a</summary>'), + '

Use <details> for ' + . "<summary>a</summary>

\n", + 'A raw tag in a code span does not expand entity encoded tags elsewhere' +); + +# Spelled with chr() rather than \x escapes, which perlcritic flags. +my $details_open = chr 0xE000; +my $details_close = chr 0xE001; +my $summary_open = chr 0xE002; +my $summary_close = chr 0xE003; + +is( + $parser->render_html( + $details_open . $summary_open . 'nope' . $summary_close . $details_close + ), "

nope

\n", 'The internal disclosure markers cannot be forged in a comment' ); From bc108547589350bfc43dfdf15add6a480741d3b5 Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Wed, 16 Sep 2026 11:39:38 -0400 Subject: [PATCH 3/5] Copilot review fixes --- Bugzilla/Markdown.pm | 110 +++++++++++++++++++++++++++++++------------ t/markdown.t | 69 ++++++++++++++++++++++++--- 2 files changed, 142 insertions(+), 37 deletions(-) diff --git a/Bugzilla/Markdown.pm b/Bugzilla/Markdown.pm index 937e23a502..dc8f4d2b19 100644 --- a/Bugzilla/Markdown.pm +++ b/Bugzilla/Markdown.pm @@ -14,6 +14,7 @@ use Mojo::DOM; use Mojo::Util qw(trim); use HTML::Escape qw(escape_html); use List::MoreUtils qw(any); +use Bugzilla::Util qw(generate_random_password); has 'markdown_parser' => (is => 'lazy'); has 'bugzilla_shorthand' => ( @@ -38,33 +39,67 @@ sub _build_markdown_parser { my $MARKDOWN_OFF = quotemeta '#[markdown(off)]'; # The only raw HTML allowed in comments: GitHub-style collapsible sections. -# The raw tags are swapped for private use characters before the markdown is -# parsed, and those markers are turned into real elements afterwards. Marking -# them up front is what keeps a raw tag distinct from text that merely renders -# as one, such as an entity-encoded tag. Only these exact tags are recognized -# and they never carry attributes, so no other markup can be smuggled in. -my %DISCLOSURE_MARKER = ( - '
' => chr 0xE000, - '
' => chr 0xE001, - '' => chr 0xE002, - '' => chr 0xE003, -); - -# Marker back to the tag it stands for, for markers that cannot be expanded. -my %DISCLOSURE_TAG = reverse %DISCLOSURE_MARKER; +# The raw tags are swapped for markers before the markdown is parsed, and +# those markers are turned into real elements afterwards. Marking them up +# front is what keeps a raw tag distinct from text that merely renders as one, +# such as an entity-encoded tag. Only these exact tags are recognized and they +# never carry attributes, so no other markup can be smuggled in. # Markdown wraps the tags in a paragraph. Closing and reopening it lets the # HTML parser lift the block level disclosure elements out of the paragraph; -# the empty paragraphs left behind are dropped afterwards. +# the empty paragraphs left behind are dropped afterwards. Keyed by tag name, +# as a marker carries the name rather than the whole tag. my %DISCLOSURE_HTML = ( - $DISCLOSURE_MARKER{'
'} => '

', - $DISCLOSURE_MARKER{'

'} => '

', - $DISCLOSURE_MARKER{'

'} => '

', - $DISCLOSURE_MARKER{''} => '

', + 'details' => '

', + '/details' => '

', + 'summary' => '

', + '/summary' => '

', ); -my $DISCLOSURE_RE = qr{}i; -my $DISCLOSURE_MARKER_RE = qr{[\x{E000}-\x{E003}]}; +my $DISCLOSURE_RE = qr{}i; + +# A marker wraps the tag name as the comment spelled it, so the marker is self +# describing: a tag that turns out to be a code literal is restored to the +# comment's own spelling, while one that becomes an element is named in +# canonical lower case. +my $DISCLOSURE_NAME_RE = qr{/?(?:details|summary)}i; + +# The private use characters a marker starts and ends with. +my $MARKER_START = chr 0xE000; +my $MARKER_END = chr 0xE001; + +# The markdown parser percent encodes those characters in a link destination, +# so a marker that ended up in one has to be recognized in that form too. +my $MARKER_START_RE = qr{(?:$MARKER_START|%EE%80%80)}; +my $MARKER_END_RE = qr{(?:$MARKER_END|%EE%80%81)}; +my $MARKER_CHARS_RE = qr{[$MARKER_START$MARKER_END]}; + +# The markers for one comment: how to mark a tag, a regex capturing the name +# out of this comment's markers, and a regex for the marker characters that +# are not part of one. +# +# The characters on their own cannot mark a tag, because the markdown parser +# decodes character references: a comment containing  hands back that +# character after the input has been scrubbed of it, forging a marker it never +# wrote as a tag. Wrapping the name in a token that is random per comment is +# what keeps the markers ours, as nothing in the comment can predict it. +sub _disclosure_markers { + my $nonce = generate_random_password(16); + + return { + mark => sub { + # The tag without its angle brackets, which a marker cannot contain: + # every remaining < in the comment is escaped before it is parsed. + my $name = substr $_[0], 1, -1; + return $MARKER_START . $nonce . $name . $nonce . $MARKER_END; + }, + re => qr{ + $MARKER_START_RE \Q$nonce\E ($DISCLOSURE_NAME_RE) \Q$nonce\E + $MARKER_END_RE + }x, + stray => qr/$MARKER_START(?!\Q$nonce\E)|(?{mark}->($1)/ge; + } $markdown =~ s{<(?!https?://)}{\x{FFFD}}gs; @@ -108,6 +148,14 @@ sub render_html { my $html = decode('UTF-8', $parser->render_html($markdown)); $html =~ s/\x{FFFD}/</g; + + # A character reference decodes to the character it names, so the comment + # can still hand back one of the characters the markers are built from: the + # scrub above only cleared the ones it wrote as characters. Drop anything + # left in that range which is not a marker of ours. + my $stray_marker_re = $disclosure ? $disclosure->{stray} : $MARKER_CHARS_RE; + $html =~ s/$stray_marker_re//g; + my $dom = Mojo::DOM->new($html); $dom->find(join(', ', @bad_tags))->map('remove'); @@ -128,7 +176,8 @@ sub render_html { }); return $node; }); - return $has_disclosure ? _expand_disclosure_tags($dom) : $dom->to_string; + return $dom->to_string unless $disclosure; + return _expand_disclosure_tags($dom, $disclosure); } # Turn the markers left in place of the raw

/ tags into real @@ -136,7 +185,8 @@ sub render_html { # as literal text: inside a code block, so the syntax can still be documented # in a comment, or inside an attribute value, where only text belongs. sub _expand_disclosure_tags { - my ($dom) = @_; + my ($dom, $disclosure) = @_; + my $marker_re = $disclosure->{re}; my $found = 0; $dom->descendant_nodes->each(sub { @@ -146,25 +196,25 @@ sub _expand_disclosure_tags { my $attr = $node->attr; foreach my $key (keys %$attr) { next unless defined $attr->{$key}; - $attr->{$key} =~ s/($DISCLOSURE_MARKER_RE)/$DISCLOSURE_TAG{$1}/g; + $attr->{$key} =~ s/$marker_re/<$1>/g; } return; } my $text = $node->content; - return unless $text =~ $DISCLOSURE_MARKER_RE; + return unless $text =~ $marker_re; if ($node->type eq 'text' && !$node->ancestors('pre, code')->size) { $found = 1; return; } - $text =~ s/($DISCLOSURE_MARKER_RE)/$DISCLOSURE_TAG{$1}/g; + $text =~ s/$marker_re/<$1>/g; $node->content($text); }); my $html = $dom->to_string; return $html unless $found; - $html =~ s/($DISCLOSURE_MARKER_RE)/$DISCLOSURE_HTML{$1}/g; + $html =~ s/$marker_re/$DISCLOSURE_HTML{lc $1}/g; # Drop the line breaks and empty paragraphs the rewrite leaves behind. $html =~ s{\s*\s*(?=

)}{}g; diff --git a/t/markdown.t b/t/markdown.t index 21bf35dcd8..6c0a279132 100644 --- a/t/markdown.t +++ b/t/markdown.t @@ -142,6 +142,22 @@ is( 'Disclosure tags in a code span stay literal' ); +# A code literal is the comment's own text, so marking the tags must not +# normalize the spelling of the ones that turn out to be literals. Only the +# tags that become elements are named in canonical lower case. +is( + $parser->render_html("```\n
xy
\n```"), + "
<DETAILS><SUMMARY>x</SUMMARY>"
+    . "y</DETAILS>\n
\n", + 'A literal disclosure tag keeps the spelling the comment used' +); + +is( + $parser->render_html('Use `
` to fold.'), + "

Use <DeTaIlS> to fold.

\n", + 'A disclosure tag in a code span keeps the spelling the comment used' +); + like( $parser->render_html('
nope'), qr{<details open onclick="x">nope}, @@ -166,17 +182,56 @@ is( ); # Spelled with chr() rather than \x escapes, which perlcritic flags. -my $details_open = chr 0xE000; -my $details_close = chr 0xE001; -my $summary_open = chr 0xE002; -my $summary_close = chr 0xE003; +my $marker_start = chr 0xE000; +my $marker_end = chr 0xE001; is( $parser->render_html( - $details_open . $summary_open . 'nope' . $summary_close . $details_close + "${marker_start}details${marker_end}${marker_start}summary${marker_end}nope" ), - "

nope

\n", - 'The internal disclosure markers cannot be forged in a comment' + "

detailssummarynope

\n", + 'The marker characters cannot be forged in a comment' +); + +# A character reference is decoded by the markdown parser, after the comment +# has been scrubbed of the marker characters, so it must not be able to hand +# back a marker. Otherwise a comment with one real disclosure tag could close +# the section early and reveal the content hidden in it, or open a section of +# its own and hide what follows. +foreach my $reference ('', '', '') { + is( + $parser->render_html( + "
x${reference}visible
" + ), + "
x

visible

\n", + "A $reference character reference cannot forge a disclosure marker" + ); +} + +is( + $parser->render_html( + '
xy
hidden?' + ), + '
x

y

' + . "

hidden?

\n", + 'A character reference cannot open a section of its own' +); + +# The characters the markers are built from are never content, so a reference +# to one leaves nothing behind, whether or not the comment has a real tag. +is( + $parser->render_html('abc'), + "

abc

\n", + 'References to the reserved characters are dropped' +); + +# A marker is percent encoded when it lands in a link destination, where it +# has to be recognized too: otherwise the token meant to stay internal is +# served as part of the URL. +like( + $parser->render_html('[a](http://x/
)'), + qr{href="http://x/<DETAILS>"}, + 'A marker in a link destination is restored, not served as the URL' ); # An unbalanced tag must not leak an unclosed element into the page. From 4c2ab1cd0ed1ed4b2645721305729d96fdfcd1be Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Wed, 16 Sep 2026 11:48:01 -0400 Subject: [PATCH 4/5] kyoshino review fixes --- Bugzilla/Markdown.pm | 37 ++++++++++++++++++++++++++++--------- skins/standard/global.css | 11 +++++++++++ t/markdown.t | 34 +++++++++++++++++++++++++++++++++- 3 files changed, 72 insertions(+), 10 deletions(-) diff --git a/Bugzilla/Markdown.pm b/Bugzilla/Markdown.pm index dc8f4d2b19..77e81d41ef 100644 --- a/Bugzilla/Markdown.pm +++ b/Bugzilla/Markdown.pm @@ -42,27 +42,35 @@ my $MARKDOWN_OFF = quotemeta '#[markdown(off)]'; # The raw tags are swapped for markers before the markdown is parsed, and # those markers are turned into real elements afterwards. Marking them up # front is what keeps a raw tag distinct from text that merely renders as one, -# such as an entity-encoded tag. Only these exact tags are recognized and they -# never carry attributes, so no other markup can be smuggled in. +# such as an entity-encoded tag. Only these exact tags are recognized, and the +# bare `open` attribute on
is the only attribute they may carry, so +# no other markup can be smuggled in. # Markdown wraps the tags in a paragraph. Closing and reopening it lets the # HTML parser lift the block level disclosure elements out of the paragraph; # the empty paragraphs left behind are dropped afterwards. Keyed by tag name, # as a marker carries the name rather than the whole tag. my %DISCLOSURE_HTML = ( - 'details' => '

', - '/details' => '

', - 'summary' => '

', - '/summary' => '

', + 'details' => '

', + 'details open' => '

', + '/details' => '

', + 'summary' => '

', + '/summary' => '

', ); -my $DISCLOSURE_RE = qr{}i; +#

starts a section already expanded. Only the bare attribute is +# recognized, so a marker never has to carry a quoted value, and the whitespace +# around it has to stay horizontal: a marker spanning a line break would be +# split by the hard break the parser puts there. +my $DISCLOSURE_OPEN_RE = qr{\h+open\h*}i; + +my $DISCLOSURE_RE = qr{|
|}i; # A marker wraps the tag name as the comment spelled it, so the marker is self # describing: a tag that turns out to be a code literal is restored to the # comment's own spelling, while one that becomes an element is named in # canonical lower case. -my $DISCLOSURE_NAME_RE = qr{/?(?:details|summary)}i; +my $DISCLOSURE_NAME_RE = qr{details$DISCLOSURE_OPEN_RE?|/details|/?summary}i; # The private use characters a marker starts and ends with. my $MARKER_START = chr 0xE000; @@ -214,7 +222,7 @@ sub _expand_disclosure_tags { my $html = $dom->to_string; return $html unless $found; - $html =~ s/$marker_re/$DISCLOSURE_HTML{lc $1}/g; + $html =~ s/$marker_re/_disclosure_html($1)/ge; # Drop the line breaks and empty paragraphs the rewrite leaves behind. $html =~ s{\s*\s*(?=

)}{}g; @@ -227,6 +235,17 @@ sub _expand_disclosure_tags { return $expanded->to_string; } +# The markup a marker expands to. A marker carries the tag name as the comment +# spelled it, so the lookup normalizes the case and the whitespace an `open` +# attribute was written with. +sub _disclosure_html { + my ($name) = @_; + + $name = lc $name; + $name =~ s/$DISCLOSURE_OPEN_RE\z/ open/; + return $DISCLOSURE_HTML{$name}; +} + sub _is_external_link { # the urlbase, without the trailing / state $urlbase = substr(Bugzilla->localconfig->urlbase, 0, -1); diff --git a/skins/standard/global.css b/skins/standard/global.css index 564a110412..c1d159293b 100644 --- a/skins/standard/global.css +++ b/skins/standard/global.css @@ -2736,6 +2736,17 @@ div.bz_comment_text pre { cursor: pointer; } +/* A heading is a block, which would drop it below the disclosure triangle. */ +.markdown-body summary h1, +.markdown-body summary h2, +.markdown-body summary h3, +.markdown-body summary h4, +.markdown-body summary h5, +.markdown-body summary h6 { + display: inline-block; + margin: 0; +} + .markdown-body ul, .markdown-body ol { padding-left: 0; diff --git a/t/markdown.t b/t/markdown.t index 6c0a279132..d1633b304d 100644 --- a/t/markdown.t +++ b/t/markdown.t @@ -158,10 +158,42 @@ is( 'A disclosure tag in a code span keeps the spelling the comment used' ); +is( + $parser->render_html('
xy
'), + "
x

y

\n", + 'The open attribute starts a section expanded' +); + +is( + $parser->render_html('
xy
'), + "
x

y

\n", + 'The open attribute is case insensitive and tolerates whitespace' +); + +is( + $parser->render_html('Use `
` to fold.'), + "

Use <DETAILS OPEN> to fold.

\n", + 'An open disclosure tag in a code span keeps the spelling the comment used' +); + +is( + $parser->render_html('nope'), + "

<summary open>nope

\n", + 'The open attribute is only recognized on
' +); + like( $parser->render_html('
nope'), qr{<details open onclick="x">nope}, - 'Only the bare disclosure tags are recognized' + 'The open attribute is the only attribute recognized' +); + +# A marker must not span a line break, or the hard break the parser puts there +# would split it and spill its innards into the page. +like( + $parser->render_html("hidden?"), + qr{\A

<details
\nopen>hidden\?

\n\z}, + 'A disclosure tag split over two lines is not recognized' ); # Only the raw tags in the comment are expanded; text that merely renders as a From 7aaadd92b49cd637071edea9410771f7b99583d8 Mon Sep 17 00:00:00 2001 From: David Lawrence Date: Wed, 16 Sep 2026 11:54:14 -0400 Subject: [PATCH 5/5] Copilot review fixes --- Bugzilla/Markdown.pm | 27 ++++++++++++++++++++++----- t/markdown.t | 17 +++++++++++++++++ 2 files changed, 39 insertions(+), 5 deletions(-) diff --git a/Bugzilla/Markdown.pm b/Bugzilla/Markdown.pm index 77e81d41ef..cafefab23f 100644 --- a/Bugzilla/Markdown.pm +++ b/Bugzilla/Markdown.pm @@ -99,6 +99,10 @@ sub _disclosure_markers { # The tag without its angle brackets, which a marker cannot contain: # every remaining < in the comment is escaped before it is parsed. my $name = substr $_[0], 1, -1; + + # A name the lookup has no markup for is left as the comment wrote it, + # rather than marked and expanded to nothing later on. + return $_[0] unless defined _disclosure_key($name); return $MARKER_START . $nonce . $name . $nonce . $MARKER_END; }, re => qr{ @@ -235,15 +239,28 @@ sub _expand_disclosure_tags { return $expanded->to_string; } -# The markup a marker expands to. A marker carries the tag name as the comment -# spelled it, so the lookup normalizes the case and the whitespace an `open` -# attribute was written with. -sub _disclosure_html { +# The %DISCLOSURE_HTML key a tag name belongs to, or nothing when it has no +# markup. A marker carries the tag name as the comment spelled it, so the case +# and the whitespace an `open` attribute was written with are normalized here. +# +# The name is checked against the keys rather than assumed to be one of them, +# because a case insensitive match is not the same thing as lc: Perl folds +# Unicode, so (U+017F) matches the tags while lc leaves that +# spelling alone. +sub _disclosure_key { my ($name) = @_; $name = lc $name; $name =~ s/$DISCLOSURE_OPEN_RE\z/ open/; - return $DISCLOSURE_HTML{$name}; + return exists $DISCLOSURE_HTML{$name} ? $name : undef; +} + +# The markup a marker expands to. The name was checked when the marker was +# made, so the key is always there. +sub _disclosure_html { + my ($name) = @_; + + return $DISCLOSURE_HTML{_disclosure_key($name)}; } sub _is_external_link { diff --git a/t/markdown.t b/t/markdown.t index d1633b304d..6987204314 100644 --- a/t/markdown.t +++ b/t/markdown.t @@ -266,6 +266,23 @@ like( 'A marker in a link destination is restored, not served as the URL' ); +# Perl's case insensitive match folds Unicode, so a spelling like +# matches the disclosure tags while lc does not turn it into a tag name. Such a +# tag has no markup to expand to and must be left as the comment wrote it, +# rather than marked and then dropped, deleting the text. +my $long_s = chr 0x17F; + +foreach my $tag ("", "", "", + "<$long_s" . 'ummary>', "') +{ + my $name = substr $tag, 1, -1; + is( + $parser->render_html("${tag}text"), + "

<${name}>text

\n", + "A $tag Unicode case fold of a disclosure tag is kept literally" + ); +} + # An unbalanced tag must not leak an unclosed element into the page. like( $parser->render_html("
\noops\n\nrest\n"),