diff --git a/Bugzilla/Markdown.pm b/Bugzilla/Markdown.pm index 2a77b4932c..cafefab23f 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' => ( @@ -36,6 +37,82 @@ 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 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 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 open' => '

', + '/details' => '

', + 'summary' => '

', + '/summary' => '

', +); + +#

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$DISCLOSURE_OPEN_RE?|/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; + + # 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{ + $MARKER_START_RE \Q$nonce\E ($DISCLOSURE_NAME_RE) \Q$nonce\E + $MARKER_END_RE + }x, + stray => qr/$MARKER_START(?!\Q$nonce\E)|(?markdown_parser; @@ -61,8 +138,20 @@ sub render_html { } # 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. + # Spelled out because tr does not interpolate: keep in step with + # $MARKER_START and $MARKER_END. + $markdown =~ tr/\x{FFFD}\x{E000}\x{E001}//d; + + # Mark the raw disclosure tags before the markdown is parsed, so that only + # these occurrences can ever become elements again. + my $disclosure; + if ($markdown =~ $DISCLOSURE_RE) { + $disclosure = _disclosure_markers(); + $markdown =~ s/($DISCLOSURE_RE)/$disclosure->{mark}->($1)/ge; + } + $markdown =~ s{<(?!https?://)}{\x{FFFD}}gs; my @valid_text_parent_tags = ('h1', 'h2', 'h3', 'h4', 'h5', 'h6', 'p', 'li', 'td'); @@ -71,6 +160,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'); @@ -91,8 +188,79 @@ sub render_html { }); return $node; }); - return $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 +# 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, $disclosure) = @_; + my $marker_re = $disclosure->{re}; + + my $found = 0; + $dom->descendant_nodes->each(sub { + my ($node) = @_; + + if ($node->type eq 'tag') { + my $attr = $node->attr; + foreach my $key (keys %$attr) { + next unless defined $attr->{$key}; + $attr->{$key} =~ s/$marker_re/<$1>/g; + } + return; + } + + my $text = $node->content; + return unless $text =~ $marker_re; + if ($node->type eq 'text' && !$node->ancestors('pre, code')->size) { + $found = 1; + return; + } + $text =~ s/$marker_re/<$1>/g; + $node->content($text); + }); + + my $html = $dom->to_string; + return $html unless $found; + + $html =~ s/$marker_re/_disclosure_html($1)/ge; + + # 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; +} + +# 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 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/skins/standard/global.css b/skins/standard/global.css index ce8d31c36e..c1d159293b 100644 --- a/skins/standard/global.css +++ b/skins/standard/global.css @@ -2724,6 +2724,29 @@ 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; +} + +/* 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 f8b541d5b2..6987204314 100644 --- a/t/markdown.t +++ b/t/markdown.t @@ -98,4 +98,196 @@ 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' +); + +# 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' +); + +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}, + '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 +# tag must not be able to close the section early and reveal hidden content. +is( + $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 $marker_start = chr 0xE000; +my $marker_end = chr 0xE001; + +is( + $parser->render_html( + "${marker_start}details${marker_end}${marker_start}summary${marker_end}nope" + ), + "

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' +); + +# 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"), + qr{
\z}, + 'An unclosed disclosure section is closed for us' +); + done_testing;