Scrub admin description fields.

10 files changed, 49 insertions, 8 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 01c7a46b9..4f475bfa5 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -5,6 +5,7 @@
         - Fix XSS vulnerability in pagination page number.
         - Rotate session ID after successful login.
         - Switch to auto-escaping of all template variables (see below).
+        - Scrub admin description fields.
     - Front end improvements:
         - Improved 403 message, especially for private reports. #2511
         - Mobile users can now filter the pins on the `/around` map view. #2366
diff --git a/cpanfile b/cpanfile
index c4df511fc..495f1beb7 100644
--- a/cpanfile
+++ b/cpanfile
@@ -85,6 +85,7 @@ requires 'Geography::NationalGrid',
 requires 'Getopt::Long::Descriptive';
 requires 'HTML::Entities';
 requires 'HTML::FormHandler::Model::DBIC';
+requires 'HTML::Scrubber';
 requires 'HTTP::Request::Common';
 requires 'Image::Size', '3.300';
 requires 'Image::PNG::QRCode';
diff --git a/cpanfile.snapshot b/cpanfile.snapshot
index 85132bbb6..42c125da6 100644
--- a/cpanfile.snapshot
+++ b/cpanfile.snapshot
@@ -3377,6 +3377,19 @@ DISTRIBUTIONS
       HTML::Tagset 3
       XSLoader 0
       perl 5.008
+  HTML-Scrubber-0.19
+    pathname: N/NI/NIGELM/HTML-Scrubber-0.19.tar.gz
+    provides:
+      HTML::Scrubber 0.19
+    requirements:
+      ExtUtils::MakeMaker 0
+      HTML::Entities 0
+      HTML::Parser 3.47
+      List::Util 1.33
+      Scalar::Util 0
+      perl 5.008
+      strict 0
+      warnings 0
   HTML-Selector-XPath-0.15
     pathname: C/CO/CORION/HTML-Selector-XPath-0.15.tar.gz
     provides:
diff --git a/perllib/FixMyStreet/App/Controller/Admin.pm b/perllib/FixMyStreet/App/Controller/Admin.pm
index 64cc9eaaf..c1afccdfd 100644
--- a/perllib/FixMyStreet/App/Controller/Admin.pm
+++ b/perllib/FixMyStreet/App/Controller/Admin.pm
@@ -557,7 +557,8 @@ sub update_extra_fields : Private {
         if ($behaviour eq 'question') {
             $meta->{required} = $c->get_param("metadata[$i].required") ? 'true' : 'false';
             $meta->{variable} = 'true';
-            $meta->{description} = $c->get_param("metadata[$i].description");
+            my $desc = $c->get_param("metadata[$i].description");
+            $meta->{description} = FixMyStreet::Template::sanitize($desc);
             $meta->{datatype} = $c->get_param("metadata[$i].datatype");
 
             if ( $meta->{datatype} eq "singlevaluelist" ) {
@@ -579,7 +580,8 @@ sub update_extra_fields : Private {
             }
         } elsif ($behaviour eq 'notice') {
             $meta->{variable} = 'false';
-            $meta->{description} = $c->get_param("metadata[$i].description");
+            my $desc = $c->get_param("metadata[$i].description");
+            $meta->{description} = FixMyStreet::Template::sanitize($desc);
             $meta->{disable_form} = $c->get_param("metadata[$i].disable_form") ? 'true' : 'false';
         } elsif ($behaviour eq 'hidden') {
             $meta->{automated} = 'hidden_field';
diff --git a/perllib/FixMyStreet/App/Controller/Admin/Bodies.pm b/perllib/FixMyStreet/App/Controller/Admin/Bodies.pm
index ea03b146f..3b7739966 100644
--- a/perllib/FixMyStreet/App/Controller/Admin/Bodies.pm
+++ b/perllib/FixMyStreet/App/Controller/Admin/Bodies.pm
@@ -286,6 +286,7 @@ sub update_contact : Private {
     # Special form disabling form
     if ($c->get_param('disable')) {
         my $msg = $c->get_param('disable_message');
+        $msg = FixMyStreet::Template::sanitize($msg);
         $errors{category} = _('Please enter a message') unless $msg;
         my $meta = {
             code => '_fms_disable_',
diff --git a/perllib/FixMyStreet/Template.pm b/perllib/FixMyStreet/Template.pm
index 84faeb562..afab83e41 100644
--- a/perllib/FixMyStreet/Template.pm
+++ b/perllib/FixMyStreet/Template.pm
@@ -6,6 +6,7 @@ use warnings;
 use FixMyStreet;
 use mySociety::Locale;
 use Attribute::Handlers;
+use HTML::Scrubber;
 use FixMyStreet::Template::SafeString;
 use FixMyStreet::Template::Context;
 use FixMyStreet::Template::Stash;
@@ -135,4 +136,20 @@ sub html_paragraph : Filter('html_para') {
     return FixMyStreet::Template::SafeString->new($text);
 }
 
+sub sanitize {
+    my $text = shift;
+
+    my %allowed_tags = map { $_ => 1 } qw( p ul ol li br b i strong em );
+    my $scrubber = HTML::Scrubber->new(
+        rules => [
+            %allowed_tags,
+            a => { href => qr{^(http|/|tel)}i, style => 1, target => qr/^_blank$/, title => 1 },
+            font => { color => 1 },
+            span => { style => 1 },
+        ]
+    );
+    $text = $scrubber->scrub($text);
+    return $text;
+}
+
 1;
diff --git a/perllib/Open311/PopulateServiceList.pm b/perllib/Open311/PopulateServiceList.pm
index 7c4337b1a..3e987b7dd 100644
--- a/perllib/Open311/PopulateServiceList.pm
+++ b/perllib/Open311/PopulateServiceList.pm
@@ -246,7 +246,13 @@ sub _add_meta_to_contact {
     # turn the data into something a bit more friendly to use
     @meta =
         # remove trailing colon as we add this when we display so we don't want 2
-        map { $_->{description} =~ s/:\s*$// if $_->{description}; $_ }
+        map {
+            if ($_->{description}) {
+                $_->{description} =~ s/:\s*$//;
+                $_->{description} = FixMyStreet::Template::sanitize($_->{description});
+            }
+            $_
+        }
         # there is a display order and we only want to sort once
         sort { ($a->{order} || 0) <=> ($b->{order} || 0) }
         @meta;
diff --git a/t/app/controller/admin/bodies.t b/t/app/controller/admin/bodies.t
index 2d09ee85e..340351473 100644
--- a/t/app/controller/admin/bodies.t
+++ b/t/app/controller/admin/bodies.t
@@ -236,13 +236,13 @@ subtest 'disable form message editing' => sub {
     $mech->get_ok('/admin/body/' . $body->id . '/test%20category');
     $mech->submit_form_ok( { with_fields => {
         disable => 1,
-        disable_message => 'Please ring us!',
+        disable_message => '<em>Please</em> <u>ring</u> us on <a href="tel:01234">01234</a>, click <a href="javascript:bad">bad</a>',
         note => 'Adding emergency message',
     } } );
     $mech->content_contains('Values updated');
     my $contact = $body->contacts->find({ category => 'test category' });
     is_deeply $contact->get_extra_fields, [{
-        description => 'Please ring us!',
+        description => '<em>Please</em> ring us on <a href="tel:01234">01234</a>, click <a>bad</a>',
         code => '_fms_disable_',
         protected => 'true',
         variable => 'false',
diff --git a/t/app/controller/admin/reportextrafields.t b/t/app/controller/admin/reportextrafields.t
index 388cc3c95..dede25207 100644
--- a/t/app/controller/admin/reportextrafields.t
+++ b/t/app/controller/admin/reportextrafields.t
@@ -68,7 +68,7 @@ FixMyStreet::override_config {
             "metadata[9999].code" => "string_test",
             "metadata[9999].required" => 1,
             "metadata[9999].behaviour" => "question",
-            "metadata[9999].description" => "this is a test description",
+            "metadata[9999].description" => "<div style='foo'>this is a test description</div>",
             "metadata[9999].datatype" => "string",
             "note" => "Added extra field",
         }});
diff --git a/t/open311/populate-service-list.t b/t/open311/populate-service-list.t
index 75c468666..40e124d03 100644
--- a/t/open311/populate-service-list.t
+++ b/t/open311/populate-service-list.t
@@ -430,7 +430,7 @@ for my $test (
                 required => 'true',
                 datatype_description => 'Type of bin',
                 order => 1,
-                description => 'Type of bin'
+                description => 'Type of <b>bin</b>'
 
         } ],
         meta_xml => '<?xml version="1.0" encoding="utf-8"?>
@@ -444,7 +444,7 @@ for my $test (
                 <required>true</required>
                 <datatype_description>Type of bin</datatype_description>
                 <order>1</order>
-                <description>Type of bin</description>
+                <description>&lt;type&gt;Type&lt;/type&gt; of &lt;b&gt;bin&lt;/b&gt;</description>
             </attribute>
         </attributes>
     </service_definition>