diff options
| -rw-r--r-- | CHANGELOG.md | 2 | ||||
| -rw-r--r-- | docs/_includes/admin-tasks-content.md | 9 | ||||
| -rw-r--r-- | perllib/FixMyStreet/App/View/Web.pm | 42 | ||||
| -rw-r--r-- | perllib/FixMyStreet/Template.pm | 2 | ||||
| -rw-r--r-- | t/app/controller/my.t | 23 | ||||
| -rw-r--r-- | t/app/controller/questionnaire.t | 9 | ||||
| -rw-r--r-- | t/app/controller/report_updates.t | 87 | ||||
| -rw-r--r-- | templates/web/base/admin/users/log.html | 2 | ||||
| -rw-r--r-- | templates/web/base/my/my.html | 2 | ||||
| -rw-r--r-- | templates/web/base/questionnaire/index.html | 2 | ||||
| -rw-r--r-- | templates/web/base/report/update.html | 4 |
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('<p>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 <strong>update</strong> 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. <strong>some staff-allowed HTML</strong>"); + $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("<i>some Markdown-style italics</i>"); + + $mech->content_contains("3. <script>some disallowed HTML</script>"); + $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: <p>This is its own para</p>"); + $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("<strong>some staff-allowed HTML</strong>"); + + $mech->content_contains("2. <i>some Markdown-style italics</i>"); + $mech->content_lacks("*some Markdown-style italics*"); + $mech->content_lacks("<i>some Markdown-style italics</i>"); + + $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("<strong>some staff-allowed HTML</strong>"); + + $mech->content_contains("2. <i>some Markdown-style italics</i>"); + $mech->content_lacks("*some Markdown-style italics*"); + $mech->content_lacks("<i>some Markdown-style italics</i>"); + + $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">“[% updates.last.text | add_links %]”</p> + <p class="questionnaire-report-reminder__last-update">“[% updates.last.text | staff_html_markup(updates.last.extra) %]”</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 %] |