Skip to content

Commit

Permalink
Replace Text::Markdown and HTML::Restrict with CommonMark for much be…
Browse files Browse the repository at this point in the history 
…tter comment rendering speed and security
  • Loading branch information
@kraih
kraih committed Jul 31, 2019
1 parent cb3b0fc commit 48fc500
Show file tree
Hide file tree
Showing 9 changed files with 162 additions and 105 deletions.
2 changes: 0 additions & 2 deletions cpanfile
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,8 +76,6 @@ requires 'Scalar::Util';
requires 'Sub::Install';
requires 'Sub::Name';
requires 'Text::Diff';
requires 'Text::Markdown';
requires 'HTML::Restrict';
requires 'CommonMark';
requires 'Time::HiRes';
requires 'Time::ParseDate';
Expand Down
2 changes: 0 additions & 2 deletions docker/travis_test/Dockerfile
View file
Original file line number Diff line number Diff line change
Expand Up @@ -121,8 +121,6 @@ RUN zypper in -y -C \
'perl(Socket::MsgHdr)' \
'perl(Test::Warnings)' \
'perl(Text::Diff)' \
'perl(Text::Markdown)' \
'perl(HTML::Restrict)' \
'perl(CommonMark)' \
'perl(Time::ParseDate)' \
'perl(XSLoader) >= 0.24' \
Expand Down
71 changes: 36 additions & 35 deletions lib/OpenQA/Markdown.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,55 +17,56 @@ use Mojo::Base -strict;

use Exporter 'import';
use Regexp::Common 'URI';
use OpenQA::Utils 'bugref_to_href';
use Text::Markdown;
use HTML::Restrict;
use OpenQA::Utils qw(bugref_regex bugurl);
use CommonMark;

our @EXPORT_OK = qw(markdown_to_html);
our @EXPORT_OK = qw(bugref_to_markdown is_light_color markdown_to_html);

# Limit tags to a safe subset
my $RULES = {
a => [qw(href)],
blockquote => [],
code => [],
em => [],
img => [qw(src alt)],
h1 => [],
h2 => [],
h3 => [],
h4 => [],
h5 => [],
h6 => [],
hr => [],
li => [],
ol => [],
p => [],
strong => [],
ul => []};
my $RE = bugref_regex;

# Only allow "href=/...", "href=http://..." and "href=https://..."
my $SCHEMES = [undef, 'http', 'https'];
sub bugref_to_markdown {
my $text = shift;
$text =~ s/$RE/"[$+{match}](" . bugurl($+{match}) . ')'/geio;
return $text;
}

my $RESTRICT = HTML::Restrict->new(rules => $RULES, uri_schemes => $SCHEMES);
my $MARKDOWN = Text::Markdown->new;
sub is_light_color {
my $color = shift;

return undef unless $color =~ m/^#([a-f0-9]{2})([a-f0-9]{2})([a-f0-9]{2})$/;
my ($R, $G, $B) = ($1, $2, $3);
$_ = hex $_ for ($R, $G, $B);
my $sum = $R + $G + $B;
return $sum > 380;
}

sub markdown_to_html {
my $text = shift;

# Replace bugrefs with links
$text = bugref_to_href($text);
$text = bugref_to_markdown($text);

# Turn all remaining URLs into links
$text =~ s@(?<!['"(<>])($RE{URI})@<$1>@gi;
$text =~ s/(?<!['"(<>])($RE{URI})/<$1>/gio;

# Turn references to test modules and needling steps into links
$text =~ s{\b(t#([\w/]+))}{<a href="/tests/$2">$1</a>}gi;
$text =~ s!\b(t#([\w/]+))![$1](/tests/$2)!gi;

# Markdown -> HTML
my $html = $MARKDOWN->markdown($text);
my $html = CommonMark->markdown_to_html($text);

# Custom markup "{{color:#ff0000|Some text}}"
$html =~ s/(\{\{([^|]+?)\|(.*?)\}\})/_custom($1, $2, $3)/ge;

return $html;
}

# Unsafe -> safe
return $RESTRICT->process($html);
sub _custom {
my ($full, $rules, $text) = @_;
if ($rules =~ /^color:(#[a-f0-9]{6})$/) {
my $color = $1;
my $bg = is_light_color($color) ? 'black' : 'white';
return qq{<span style="color:$color;background-color:$bg">$text</span>};
}
return $full;
}

1;
1 change: 1 addition & 0 deletions lib/OpenQA/Utils.pm
View file
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ our @EXPORT = qw(
&parse_assets_from_settings
&find_bugref
&find_bugrefs
bugref_regex
&bugurl
&bugref_to_href
&href_to_bugref
Expand Down
2 changes: 1 addition & 1 deletion openQA.spec
View file
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
%else
%define python_scripts_requires %{nil}
%endif
%define t_requires perl(DBD::Pg) perl(DBIx::Class) perl(Config::IniFiles) perl(SQL::Translator) perl(Date::Format) perl(File::Copy::Recursive) perl(DateTime::Format::Pg) perl(Net::OpenID::Consumer) perl(Mojolicious::Plugin::RenderFile) perl(Mojolicious::Plugin::AssetPack) perl(aliased) perl(Config::Tiny) perl(DBIx::Class::DeploymentHandler) perl(DBIx::Class::DynamicDefault) perl(DBIx::Class::Schema::Config) perl(DBIx::Class::Storage::Statistics) perl(IO::Socket::SSL) perl(Data::Dump) perl(DBIx::Class::OptimisticLocking) perl(Module::Pluggable) perl(Text::Diff) perl(Text::Markdown) perl(HTML::Restrict) perl(CommonMark) perl(JSON::Validator) perl(YAML::XS) perl(IPC::Run) perl(Archive::Extract) perl(CSS::Minifier::XS) perl(JavaScript::Minifier::XS) perl(Time::ParseDate) perl(Sort::Versions) perl(Mojo::RabbitMQ::Client) perl(BSD::Resource) perl(Cpanel::JSON::XS) perl(Pod::POM) perl(Mojo::IOLoop::ReadWriteProcess) perl(Minion) perl(Mojo::Pg) perl(Mojo::SQLite) perl(Minion::Backend::SQLite) %python_scripts_requires
%define t_requires perl(DBD::Pg) perl(DBIx::Class) perl(Config::IniFiles) perl(SQL::Translator) perl(Date::Format) perl(File::Copy::Recursive) perl(DateTime::Format::Pg) perl(Net::OpenID::Consumer) perl(Mojolicious::Plugin::RenderFile) perl(Mojolicious::Plugin::AssetPack) perl(aliased) perl(Config::Tiny) perl(DBIx::Class::DeploymentHandler) perl(DBIx::Class::DynamicDefault) perl(DBIx::Class::Schema::Config) perl(DBIx::Class::Storage::Statistics) perl(IO::Socket::SSL) perl(Data::Dump) perl(DBIx::Class::OptimisticLocking) perl(Module::Pluggable) perl(Text::Diff) perl(CommonMark) perl(JSON::Validator) perl(YAML::XS) perl(IPC::Run) perl(Archive::Extract) perl(CSS::Minifier::XS) perl(JavaScript::Minifier::XS) perl(Time::ParseDate) perl(Sort::Versions) perl(Mojo::RabbitMQ::Client) perl(BSD::Resource) perl(Cpanel::JSON::XS) perl(Pod::POM) perl(Mojo::IOLoop::ReadWriteProcess) perl(Minion) perl(Mojo::Pg) perl(Mojo::SQLite) perl(Minion::Backend::SQLite) %python_scripts_requires
Name: openQA
Version: 4.6
Release: 0
Expand Down
136 changes: 98 additions & 38 deletions t/16-markdown.t
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
#!/usr/bin/env perl -w

# Copyright (C) 2016 SUSE LLC
# Copyright (C) 2019 SUSE LLC
#
# This program is free software; you can redistribute it and/or modify
# it under the terms of the GNU General Public License as published by
Expand All @@ -18,71 +18,131 @@

use Mojo::Base -strict;

BEGIN { unshift @INC, 'lib' }

use Test::More;
use OpenQA::Markdown 'markdown_to_html';
use OpenQA::Markdown qw(bugref_to_markdown is_light_color markdown_to_html);

subtest 'standard markdown' => sub {
is markdown_to_html('Test'), '<p>Test</p>', 'HTML rendered';
is markdown_to_html('# Test'), '<h1>Test</h1>', 'HTML rendered';
is markdown_to_html('## Test'), '<h2>Test</h2>', 'HTML rendered';
is markdown_to_html('### Test'), '<h3>Test</h3>', 'HTML rendered';
is markdown_to_html('#### Test'), '<h4>Test</h4>', 'HTML rendered';
is markdown_to_html('##### Test'), '<h5>Test</h5>', 'HTML rendered';
is markdown_to_html('###### Test'), '<h6>Test</h6>', 'HTML rendered';
is markdown_to_html("Test\n123\n\n456 789 tset\n"), qq{<p>Test\n123</p>\n\n<p>456 789 tset</p>}, 'HTML rendered';
is markdown_to_html('*Test*'), '<p><em>Test</em></p>', 'HTML rendered';
is markdown_to_html('**Test**'), '<p><strong>Test</strong></p>', 'HTML rendered';
is markdown_to_html("1. a\n2. b\n3. c\n"), qq{<ol>\n<li>a</li>\n<li>b</li>\n<li>c</li>\n</ol>}, 'HTML rendered';
is markdown_to_html("* a\n* b\n* c\n"), qq{<ul>\n<li>a</li>\n<li>b</li>\n<li>c</li>\n</ul>}, 'HTML rendered';
is markdown_to_html('[Test](http://test.com)'), qq{<p><a href="http://test.com">Test</a></p>}, 'HTML rendered';
is markdown_to_html('[Test](/test.html)'), qq{<p><a href="/test.html">Test</a></p>}, 'HTML rendered';
is markdown_to_html('![Test](http://test.com)'), qq{<p><img src="http://test.com" alt="Test"></p>}, 'HTML rendered';
is markdown_to_html('Test `123` 123'), '<p>Test <code>123</code> 123</p>', 'HTML rendered';
is markdown_to_html("> test\n> 123"), "<blockquote>\n <p>test\n 123</p>\n</blockquote>", 'HTML rendered';
is markdown_to_html('---'), '<hr>', 'HTML rendered';
is markdown_to_html('Test'), "<p>Test</p>\n", 'HTML rendered';
is markdown_to_html('# Test #'), "<h1>Test</h1>\n", 'HTML rendered';
is markdown_to_html('# Test'), "<h1>Test</h1>\n", 'HTML rendered';
is markdown_to_html('## Test'), "<h2>Test</h2>\n", 'HTML rendered';
is markdown_to_html('### Test'), "<h3>Test</h3>\n", 'HTML rendered';
is markdown_to_html('#### Test'), "<h4>Test</h4>\n", 'HTML rendered';
is markdown_to_html('##### Test'), "<h5>Test</h5>\n", 'HTML rendered';
is markdown_to_html('###### Test'), "<h6>Test</h6>\n", 'HTML rendered';
is markdown_to_html("Test\n123\n\n456 789 tset\n"), qq{<p>Test\n123</p>\n<p>456 789 tset</p>\n}, 'HTML rendered';
is markdown_to_html('*Test*'), "<p><em>Test</em></p>\n", 'HTML rendered';
is markdown_to_html('**Test**'), "<p><strong>Test</strong></p>\n", 'HTML rendered';
is markdown_to_html("1. a\n2. b\n3. c\n"), qq{<ol>\n<li>a</li>\n<li>b</li>\n<li>c</li>\n</ol>\n}, 'HTML rendered';
is markdown_to_html("* a\n* b\n* c\n"), qq{<ul>\n<li>a</li>\n<li>b</li>\n<li>c</li>\n</ul>\n}, 'HTML rendered';
is markdown_to_html('[Test](http://test.com)'), qq{<p><a href="http://test.com">Test</a></p>\n}, 'HTML rendered';
is markdown_to_html('[Test](/test.html)'), qq{<p><a href="/test.html">Test</a></p>\n}, 'HTML rendered';
is markdown_to_html('![Test](http://test.com)'), qq{<p><img src="http://test.com" alt="Test" /></p>\n},
'HTML rendered';
is markdown_to_html('Test `123` 123'), "<p>Test <code>123</code> 123</p>\n", 'HTML rendered';
is markdown_to_html("> test\n> 123"), "<blockquote>\n<p>test\n123</p>\n</blockquote>\n", 'HTML rendered';
is markdown_to_html('---'), "<hr />\n", 'HTML rendered';
};

subtest 'bugrefs' => sub {
is markdown_to_html('boo#123'),
qq{<p><a href="https://bugzilla.opensuse.org/show_bug.cgi?id=123">boo#123</a></p>}, 'bugref expanded';
qq{<p><a href="https://bugzilla.opensuse.org/show_bug.cgi?id=123">boo#123</a></p>\n}, 'bugref expanded';
is markdown_to_html('testing boo#123 123'),
qq{<p>testing <a href="https://bugzilla.opensuse.org/show_bug.cgi?id=123">boo#123</a> 123</p>},
qq{<p>testing <a href="https://bugzilla.opensuse.org/show_bug.cgi?id=123">boo#123</a> 123</p>\n},
'bugref expanded';
is markdown_to_html('testing boo#123 123 boo#321'),
qq{<p>testing <a href="https://bugzilla.opensuse.org/show_bug.cgi?id=123">boo#123</a> 123}
. qq{ <a href="https://bugzilla.opensuse.org/show_bug.cgi?id=321">boo#321</a></p>},
. qq{ <a href="https://bugzilla.opensuse.org/show_bug.cgi?id=321">boo#321</a></p>\n},
'bugref expanded';
is markdown_to_html("testing boo#123 \n123\n boo#321"),
qq{<p>testing <a href="https://bugzilla.opensuse.org/show_bug.cgi?id=123">boo#123</a> \n123\n}
. qq{ <a href="https://bugzilla.opensuse.org/show_bug.cgi?id=321">boo#321</a></p>},
is markdown_to_html("testing boo#123\n123\n boo#321"),
qq{<p>testing <a href="https://bugzilla.opensuse.org/show_bug.cgi?id=123">boo#123</a>\n123\n}
. qq{<a href="https://bugzilla.opensuse.org/show_bug.cgi?id=321">boo#321</a></p>\n},
'bugref expanded';
is markdown_to_html("boo\ntesting boo#123 123\n123"),
qq{<p>boo\ntesting <a href="https://bugzilla.opensuse.org/show_bug.cgi?id=123">boo#123</a> 123\n123</p>},
qq{<p>boo\ntesting <a href="https://bugzilla.opensuse.org/show_bug.cgi?id=123">boo#123</a> 123\n123</p>\n},
'bugref expanded';
};

subtest 'openQA additions' => sub {
is markdown_to_html('https://example.com'),
qq{<p><a href="https://example.com">https://example.com</a></p>}, 'URL turned into a link';
qq{<p><a href="https://example.com">https://example.com</a></p>\n}, 'URL turned into a link';
is markdown_to_html('testing https://example.com 123'),
qq{<p>testing <a href="https://example.com">https://example.com</a> 123</p>}, 'URL turned into a link';
qq{<p>testing <a href="https://example.com">https://example.com</a> 123</p>\n}, 'URL turned into a link';
is markdown_to_html("t\ntesting https://example.com 123\n123"),
qq{<p>t\ntesting <a href="https://example.com">https://example.com</a> 123\n123</p>}, 'URL turned into a link';
is markdown_to_html('t#123'), qq{<p><a href="/tests/123">t#123</a></p>}, 'testref expanded';
is markdown_to_html('testing t#123 123'), qq{<p>testing <a href="/tests/123">t#123</a> 123</p>}, 'testref expanded';
is markdown_to_html("t\ntesting t#123 123\n123"), qq{<p>t\ntesting <a href="/tests/123">t#123</a> 123\n123</p>},
qq{<p>t\ntesting <a href="https://example.com">https://example.com</a> 123\n123</p>\n}, 'URL turned into a link';

is markdown_to_html('t#123'), qq{<p><a href="/tests/123">t#123</a></p>\n}, 'testref expanded';
is markdown_to_html('testing t#123 123'), qq{<p>testing <a href="/tests/123">t#123</a> 123</p>\n},
'testref expanded';
is markdown_to_html("t\ntesting t#123 123\n123"), qq{<p>t\ntesting <a href="/tests/123">t#123</a> 123\n123</p>\n},
'testref expanded';

is markdown_to_html(qq{{{color:#ffffff|"Text"}}}),
qq{<p><span style="color:#ffffff;background-color:black">&quot;Text&quot;</span></p>\n},
'White text';
is markdown_to_html("test {{color:#ff0000|Text}} 123"),
qq{<p>test <span style="color:#ff0000;background-color:white">Text</span> 123</p>\n}, 'Red text';
is markdown_to_html("test {{color:#ffffff|Text}} 123"),
qq{<p>test <span style="color:#ffffff;background-color:black">Text</span> 123</p>\n}, 'White text';
is markdown_to_html("test {{color:#00ff00|Some Text}} 123"),
qq{<p>test <span style="color:#00ff00;background-color:white">Some Text</span> 123</p>\n}, 'Green text';
is markdown_to_html("test {{color:#00ff00|Some Text}} 123 {{color:#0000ff|Also {w}orks}}"),
qq{<p>test <span style="color:#00ff00;background-color:white">Some Text</span> 123}
. qq{ <span style="color:#0000ff;background-color:white">Also {w}orks</span></p>\n},
'Green and blue text';
is markdown_to_html("test {{ color: #00ff00 | Some Text }} 123"),
"<p>test {{ color: #00ff00 | Some Text }} 123</p>\n", 'Extra whitespace is not allowed';
};

subtest 'unsafe HTML filtered out' => sub {
is markdown_to_html('Test <script>alert("boom!");</script> 123'), '<p>Test 123</p>', 'unsafe HTML filtered';
is markdown_to_html('<font>Test</font>'), '<p>Test</p>', 'unsafe HTML filtered';
is markdown_to_html('Test [Boom!](javascript:alert("boom!")) 123'), '<p>Test <a>Boom!</a> 123</p>',
is markdown_to_html('Test <script>alert("boom!");</script> 123'),
"<p>Test <!-- raw HTML omitted -->alert(&quot;boom!&quot;);<!-- raw HTML omitted --> 123</p>\n",
'unsafe HTML filtered';
is markdown_to_html('<font>Test</font>'), "<p><!-- raw HTML omitted -->Test<!-- raw HTML omitted --></p>\n",
'unsafe HTML filtered';
is markdown_to_html('Test [Boom!](javascript:alert("boom!")) 123'), qq{<p>Test <a href="">Boom!</a> 123</p>\n},
'unsafe HTML filtered';
is markdown_to_html('<a href="/" onclick="someFunction()">Totally safe</a>'),
'<p><a href="/">Totally safe</a></p>', 'unsafe HTML filtered';
"<p><!-- raw HTML omitted -->Totally safe<!-- raw HTML omitted --></p>\n", 'unsafe HTML filtered';
is markdown_to_html(qq{> hello <a name="n"\n> href="javascript:alert('boom!')">*you*</a>}),
qq{<blockquote>\n <p>hello <a><em>you</em></a></p>\n</blockquote>}, 'unsafe HTML filtered';
qq{<blockquote>\n<p>hello <!-- raw HTML omitted --><em>you</em><!-- raw HTML omitted --></p>\n</blockquote>\n},
'unsafe HTML filtered';
is markdown_to_html('{{color:#0000ff|<a>Test</a>}}'),
qq{<p><span style="color:#0000ff;background-color:white">}
. qq{<!-- raw HTML omitted -->Test<!-- raw HTML omitted --></span></p>\n},
'unsafe HTML filtered';
};

subtest 'bugrefs to markdown' => sub {
is bugref_to_markdown('bnc#9876'), '[bnc#9876](https://bugzilla.suse.com/show_bug.cgi?id=9876)', 'right markdown';
is bugref_to_markdown('bsc#9876'), '[bsc#9876](https://bugzilla.suse.com/show_bug.cgi?id=9876)', 'right markdown';
is bugref_to_markdown('boo#9876'), '[boo#9876](https://bugzilla.opensuse.org/show_bug.cgi?id=9876)',
'right markdown';
is bugref_to_markdown('bgo#9876'), '[bgo#9876](https://bugzilla.gnome.org/show_bug.cgi?id=9876)', 'right markdown';
is bugref_to_markdown('brc#9876'), '[brc#9876](https://bugzilla.redhat.com/show_bug.cgi?id=9876)', 'right markdown';
is bugref_to_markdown('bko#9876'), '[bko#9876](https://bugzilla.kernel.org/show_bug.cgi?id=9876)', 'right markdown';
is bugref_to_markdown('poo#9876'), '[poo#9876](https://progress.opensuse.org/issues/9876)', 'right markdown';
is bugref_to_markdown('gh#foo/bar#1234'), '[gh#foo/bar#1234](https://github.com/foo/bar/issues/1234)',
'right markdown';
is bugref_to_markdown('kde#9876'), '[kde#9876](https://bugs.kde.org/show_bug.cgi?id=9876)', 'right markdown';
is bugref_to_markdown('fdo#9876'), '[fdo#9876](https://bugs.freedesktop.org/show_bug.cgi?id=9876)',
'right markdown';
is bugref_to_markdown('jsc#9876'), '[jsc#9876](https://jira.suse.de/browse/9876)', 'right markdown';
is bugref_to_markdown("boo#9876\n\ntest boo#211\n"),
"[boo#9876](https://bugzilla.opensuse.org/show_bug.cgi?id=9876)\n\n"
. "test [boo#211](https://bugzilla.opensuse.org/show_bug.cgi?id=211)\n",
'right markdown';
};

subtest 'color detection' => sub {
ok !is_light_color('#000000'), 'dark';
ok !is_light_color('#ff0000'), 'dark';
ok !is_light_color('#00ff00'), 'dark';
ok !is_light_color('#0000ff'), 'dark';
ok is_light_color('#ffffff'), 'light';
ok !is_light_color('test'), 'not a color at all';
};

done_testing;
2 changes: 1 addition & 1 deletion t/api/10-jobgroups.t
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ subtest 'list job groups' => sub() {
keep_important_logs_in_days => 120,
default_priority => 50,
carry_over_bugrefs => 1,
description => "##Test description\n\nwith bugref bsc#1234",
description => "## Test description\n\nwith bugref bsc#1234",
template => undef,
keep_results_in_days => 365,
keep_important_results_in_days => 0,
Expand Down
2 changes: 1 addition & 1 deletion t/fixtures/01-jobs.pl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@
id => 1001,
name => 'opensuse',
sort_order => 0,
description => "##Test description\n\nwith bugref bsc#1234",
description => "## Test description\n\nwith bugref bsc#1234",
},
JobGroups => {
id => 1002,
Expand Down
Loading

0 comments on commit 48fc500

Please sign in to comment.