Enable HTML in updates from staff users

This also extends to response templates.

11 files changed, 161 insertions, 23 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index de2f75664..074b48bef 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -39,6 +39,8 @@
         - Add full text index to speed up admin search.
         - Offline process for CSV generation.
         - Allow inspectors to change report asset.
+        - Staff users can use HTML tags in updates. #3143
+        - Response templates can include HTML tags. #3143
     - Development improvements:
         - `#geolocate_link` is now easier to re-style. #3006
         - Links inside `#front-main` can be customised using `$primary_link_*` Sass variables. #3007
diff --git a/docs/_includes/admin-tasks-content.md b/docs/_includes/admin-tasks-content.md
index d0d5e4935..aa1225cfc 100644
--- a/docs/_includes/admin-tasks-content.md
+++ b/docs/_includes/admin-tasks-content.md
@@ -825,6 +825,15 @@ Click on ‘Templates’ in the admin menu. You will see a table of existing tem
 beside the status you wish to change. You may alter any of the fields as described in the section
 above, ‘Creating a template’. Additionally you can delete the template from this page.
 
+
+#### HTML content in templates
+
+HTML tags are permitted in response templates, which makes it possible to include
+images or rich text formatting in the updates which are added to reports.
+
+Refer to the section ["HTML Content in notices"](#html-content-in-notices) above for details of
+what tags and attributes are allowed.
+
 </div>
 
 <div class="admin-task" markdown="1" id="view-statistics">
diff --git a/perllib/FixMyStreet/App/View/Web.pm b/perllib/FixMyStreet/App/View/Web.pm
index 1e1b50094..5e38fc797 100644
--- a/perllib/FixMyStreet/App/View/Web.pm
+++ b/perllib/FixMyStreet/App/View/Web.pm
@@ -25,7 +25,7 @@ __PACKAGE__->config(
     FILTERS => {
         add_links => \&add_links,
         escape_js => \&escape_js,
-        markup => [ \&markup_factory, 1 ],
+        staff_html_markup => [ \&staff_html_markup_factory, 1 ],
     },
     COMPILE_EXT => '.ttc',
     STAT_TTL    => FixMyStreet->config('STAGING_SITE') ? 1 : 86400,
@@ -100,7 +100,7 @@ sub add_links {
     my $text = shift;
     $text = FixMyStreet::Template::conditional_escape($text);
     $text =~ s/\r//g;
-    $text =~ s{(https?://)([^\s]+)}{"<a href=\"$1$2\">$1" . _space_slash($2) . '</a>'}ge;
+    $text =~ s{(?<!["'])(https?://)([^\s]+)}{"<a href=\"$1$2\">$1" . _space_slash($2) . '</a>'}ge;
     return FixMyStreet::Template::SafeString->new($text);
 }
 
@@ -110,20 +110,44 @@ sub _space_slash {
     return $t;
 }
 
-=head2 markup_factory
+=head2 staff_html_markup_factory
 
-This returns a function that will allow updates to have markdown-style italics.
-Pass in the user that wrote the text, so we know whether it can be privileged.
+This returns a function that processes the text body of an update, applying
+HTML sanitization and markdown-style italics if it was made by a staff user.
+
+Pass in the update extra, so we can determine if it was made by a staff user.
 
 =cut
 
-sub markup_factory {
-    my ($c, $user) = @_;
+sub staff_html_markup_factory {
+    my ($c, $extra) = @_;
+
+    my $staff = $extra->{is_superuser} || $extra->{is_body_user};
+
     return sub {
         my $text = shift;
-        return $text unless $user && ($user->from_body || $user->is_superuser);
+        unless ($staff) {
+            return FixMyStreet::Template::html_paragraph(add_links($text));
+        }
+
+        $text = FixMyStreet::Template::sanitize($text);
+
+        # Apply Markdown-style italics
         $text =~ s{\*(\S.*?\S)\*}{<i>$1</i>};
-        FixMyStreet::Template::SafeString->new($text);
+
+        # Mark safe so add_links doesn't escape everything.
+        $text = FixMyStreet::Template::SafeString->new($text);
+
+        $text = add_links($text);
+
+        # If the update already has block-level elements then don't wrap
+        # individual lines in <p> elements, as we assume the user knows what
+        # they're doing.
+        unless ($text =~ /<(p|ol|ul)>/) {
+            $text = FixMyStreet::Template::html_paragraph($text);
+        }
+
+        return $text;
     }
 }
 
diff --git a/perllib/FixMyStreet/Template.pm b/perllib/FixMyStreet/Template.pm
index 6317f7552..ac9e743ff 100644
--- a/perllib/FixMyStreet/Template.pm
+++ b/perllib/FixMyStreet/Template.pm
@@ -141,6 +141,8 @@ sub html_paragraph : Filter('html_para') {
 sub sanitize {
     my $text = shift;
 
+    $text = $$text if UNIVERSAL::isa($text, 'FixMyStreet::Template::SafeString');
+ 
     my %allowed_tags = map { $_ => 1 } qw( p ul ol li br b i strong em );
     my $scrubber = HTML::Scrubber->new(
         rules => [
diff --git a/t/app/controller/my.t b/t/app/controller/my.t
index 673addf0c..85902ae1a 100644
--- a/t/app/controller/my.t
+++ b/t/app/controller/my.t
@@ -14,17 +14,12 @@ my $other_user = FixMyStreet::DB->resultset('User')->find_or_create({ email => '
 my @other = $mech->create_problems_for_body(1, 1234, 'Another Title', { user => $other_user });
 
 my $user = $mech->log_in_ok( 'test@example.com' );
-$mech->get_ok('/my');
-is $mech->uri->path, '/my', "stayed on '/my' page";
-
-$mech->content_contains('Test Title');
-$mech->content_lacks('Another Title');
-
 my @update;
 my $i = 0;
+my $staff_text = '<p>this is <script>how did this happen</script> <strong>an update</strong></p><ul><li>With</li><li>A</li><li>List</li></ul>';
 foreach ($user, $user, $other_user) {
     $update[$i] = FixMyStreet::DB->resultset('Comment')->create({
-        text => 'this is an update',
+        text => $staff_text,
         user => $_,
         state => 'confirmed',
         problem => $problems[0],
@@ -35,6 +30,20 @@ foreach ($user, $user, $other_user) {
     $i++;
 }
 
+subtest 'Check loading of /my page' => sub {
+    $mech->get_ok('/my');
+    is $mech->uri->path, '/my', "stayed on '/my' page";
+
+    $mech->content_contains('Test Title');
+    $mech->content_lacks('Another Title');
+    $mech->content_contains('&lt;p&gt;this is');
+    $mech->content_lacks('<p>this is  <strong>an update</strong></p><ul><li>With');
+
+    $update[0]->update({ extra => { is_superuser => 1 } });
+    $mech->get_ok('/my');
+    $mech->content_contains('<p>this is  <strong>an update</strong></p><ul><li>With');
+};
+
 foreach (
     { type => 'problem', id => 0, result => 404, desc => 'nothing' },
     { type => 'problem', obj => $problems[0], result => 200, desc => 'own report' },
diff --git a/t/app/controller/questionnaire.t b/t/app/controller/questionnaire.t
index b561b271a..592507288 100644
--- a/t/app/controller/questionnaire.t
+++ b/t/app/controller/questionnaire.t
@@ -351,7 +351,7 @@ my $comment = FixMyStreet::DB->resultset('Comment')->find_or_create(
         user_id    => $user->id,
         name       => 'A User',
         mark_fixed => 'false',
-        text       => 'This is some update text',
+        text       => 'This is some <strong>update</strong> text',
         state      => 'confirmed',
         confirmed  => $sent_time,
         anonymous  => 'f',
@@ -360,7 +360,12 @@ my $comment = FixMyStreet::DB->resultset('Comment')->find_or_create(
 subtest 'Check updates are shown correctly on questionnaire page' => sub {
     $mech->get_ok("/Q/" . $token->token);
     $mech->content_contains( 'Show all updates' );
-    $mech->content_contains( 'This is some update text' );
+    $mech->content_contains( 'This is some &lt;strong&gt;update&lt;/strong&gt; text' );
+};
+subtest 'Check staff update is shown correctly on questionnaire page' => sub {
+    $comment->update({ extra => { is_superuser => 1 } });
+    $mech->get_ok("/Q/" . $token->token);
+    $mech->content_contains( 'This is some <strong>update</strong> text' );
 };
 
 for my $test (
diff --git a/t/app/controller/report_updates.t b/t/app/controller/report_updates.t
index 92cbed861..9c44abc54 100644
--- a/t/app/controller/report_updates.t
+++ b/t/app/controller/report_updates.t
@@ -2200,4 +2200,91 @@ subtest 'check disabling of updates per category' => sub {
     $mech->content_lacks('Provide an update');
 };
 
+subtest 'check that only staff can display HTML in updates' => sub {
+    $report->comments->delete;
+    $user->update({ from_body => undef, is_superuser => 0 });
+
+    my @lines = (
+        "This update contains:",
+        "1. <strong>some staff-allowed HTML</strong>",
+        "2. *some Markdown-style italics*",
+        "3. <script>some disallowed HTML</script>",
+        "4. An automatic link: https://myfancylink.fixmystreet.com/",
+        "5. A block-level element: <p>This is its own para</p>",
+        ""
+    );
+    my $comment = FixMyStreet::DB->resultset('Comment')->create(
+        {
+            user          => $user,
+            problem_id    => $report->id,
+            text          => join("\n\n", @lines),
+            confirmed     => DateTime->now( time_zone => 'local'),
+            problem_state => 'confirmed',
+            anonymous     => 0,
+            mark_open     => 0,
+            mark_fixed    => 0,
+            state         => 'confirmed',
+        }
+    );
+
+    # First check that comments from a public user don't receive special treatment
+    $mech->get_ok( "/report/" . $report->id );
+
+    $mech->content_contains("1. &lt;strong&gt;some staff-allowed HTML&lt;/strong&gt;");
+    $mech->content_lacks("<strong>some staff-allowed HTML</strong>");
+
+    $mech->content_contains("2. *some Markdown-style italics*");
+    $mech->content_lacks("<i>some Markdown-style italics</i>");
+    $mech->content_lacks("&lt;i&gt;some Markdown-style italics&lt;/i&gt;");
+
+    $mech->content_contains("3. &lt;script&gt;some disallowed HTML&lt;/script&gt;");
+    $mech->content_lacks("<script>some disallowed HTML</script>");
+
+    $mech->content_contains('4. An automatic link: <a href="https://myfancylink.fixmystreet.com/">https://myfancylink.fixmystreet.com/</a>') or diag $mech->content;
+
+    $mech->content_contains("5. A block-level element: &lt;p&gt;This is its own para&lt;/p&gt;");
+    $mech->content_lacks("5. A block-level element: <p>This is its own para</p>");
+
+    # Now check that comments from a member of staff user do allow HTML/italic markup
+    $comment->set_extra_metadata(is_body_user => $body->id);
+    $comment->update;
+    $mech->get_ok( "/report/" . $report->id );
+
+    $mech->content_contains("1. <strong>some staff-allowed HTML</strong>");
+    $mech->content_lacks("&lt;strong&gt;some staff-allowed HTML&lt;/strong&gt;");
+
+    $mech->content_contains("2. <i>some Markdown-style italics</i>");
+    $mech->content_lacks("*some Markdown-style italics*");
+    $mech->content_lacks("&lt;i&gt;some Markdown-style italics&lt;/i&gt;");
+
+    $mech->content_lacks("some disallowed HTML");
+
+    $mech->content_contains('4. An automatic link: <a href="https://myfancylink.fixmystreet.com/">https://myfancylink.fixmystreet.com/</a>');
+
+    $mech->content_contains("5. A block-level element: <p>This is its own para</p>");
+    $mech->content_lacks("<p>\n5. A block-level element: <p>This is its own para</p></p>");
+
+    # and the same for superusers
+    $comment->unset_extra_metadata('is_body_user');
+    $comment->set_extra_metadata(is_superuser => 1);
+    $comment->update;
+    $mech->get_ok( "/report/" . $report->id );
+
+    $mech->content_contains("1. <strong>some staff-allowed HTML</strong>");
+    $mech->content_lacks("&lt;strong&gt;some staff-allowed HTML&lt;/strong&gt;");
+
+    $mech->content_contains("2. <i>some Markdown-style italics</i>");
+    $mech->content_lacks("*some Markdown-style italics*");
+    $mech->content_lacks("&lt;i&gt;some Markdown-style italics&lt;/i&gt;");
+
+    $mech->content_lacks("some disallowed HTML");
+
+    $mech->content_contains('4. An automatic link: <a href="https://myfancylink.fixmystreet.com/">https://myfancylink.fixmystreet.com/</a>');
+
+    $mech->content_contains("5. A block-level element: <p>This is its own para</p>");
+    $mech->content_lacks("<p>\n5. A block-level element: <p>This is its own para</p></p>");
+
+};
+
+
 done_testing();
diff --git a/templates/web/base/admin/users/log.html b/templates/web/base/admin/users/log.html
index 4b426e0ba..5c3f36321 100644
--- a/templates/web/base/admin/users/log.html
+++ b/templates/web/base/admin/users/log.html
@@ -49,7 +49,7 @@ action_map = {
                 [%- tprintf(loc('Problem %s created on behalf of %s'), mark_safe(report_link), item.obj.name) %], ‘[% item.obj.title | html %]’
             [%~ CASE 'update' %]
                 [% tprintf(loc("Update %s created for problem %d"), mark_safe(report_link), item.obj.problem_id) %]
-                [% item.obj.text | add_links | markup(item.obj.user) | html_para %]
+                [% item.obj.text | staff_html_markup(item.obj.extra) %]
             [%~ CASE 'shortlistAdded' %]
                 [%- tprintf(loc('Problem %s added to shortlist'), mark_safe(report_link)) %]
             [%~ CASE 'shortlistRemoved' %]
diff --git a/templates/web/base/my/my.html b/templates/web/base/my/my.html
index 04c5b6941..d78461cb5 100644
--- a/templates/web/base/my/my.html
+++ b/templates/web/base/my/my.html
@@ -113,7 +113,7 @@ li .my-account-buttons a {
         <div class="item-list__update-wrap">
             [% INCLUDE 'report/photo.html' object=u %]
             <div class="item-list__update-text">
-                [% u.text | add_links | html_para %]
+                [% u.text | staff_html_markup(u.extra) %]
 
                 <p class="meta-2">
                     [% tprintf( loc("Added %s"), prettify_dt( u.confirmed, 'date' ) ) %]
diff --git a/templates/web/base/questionnaire/index.html b/templates/web/base/questionnaire/index.html
index 00911c5b1..416200d25 100644
--- a/templates/web/base/questionnaire/index.html
+++ b/templates/web/base/questionnaire/index.html
@@ -55,7 +55,7 @@
         <strong>[% loc('Last update') %]</strong>
         <a href="/report/[% problem.id %]">[% loc('Show all updates') %]</a>
     </p>
-    <p class="questionnaire-report-reminder__last-update">&ldquo;[% updates.last.text | add_links %]&rdquo;</p>
+    <p class="questionnaire-report-reminder__last-update">&ldquo;[% updates.last.text | staff_html_markup(updates.last.extra) %]&rdquo;</p>
   [% END %]
 </div>
 
diff --git a/templates/web/base/report/update.html b/templates/web/base/report/update.html
index ca9397bd4..3b1f092ef 100644
--- a/templates/web/base/report/update.html
+++ b/templates/web/base/report/update.html
@@ -35,7 +35,7 @@
                 [% INCLUDE 'report/photo.html' object=update %]
                 <div class="item-list__update-text">
                     <div class="moderate-display">
-                        [% update.text | add_links | markup(update.user) | html_para %]
+                        [% update.text | staff_html_markup(update.extra) %]
                     </div>
                     [% IF can_moderate %]
                     <div class="moderate-edit">
@@ -43,7 +43,7 @@
                         <label><input type="checkbox" name="update_revert_text" class="revert-textarea">
                         [% loc('Revert to original') %]</label>
                         [% END %]
-                        <textarea class="form-control" name="update_text">[% update.text | add_links %]</textarea>
+                        <textarea class="form-control" name="update_text">[% update.text %]</textarea>
                     </div>
                     [% END %]