Use CSV escaping for categories in URLs.

Categories could contain commas, so splitting on comma is not good enough.
Let’s escape the fields as if it’s a line in CSV. Fixes #2166.

10 files changed, 44 insertions, 10 deletions

diff --git a/CHANGELOG.md b/CHANGELOG.md
index a2b4de887..7698b8736 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -11,6 +11,7 @@
         - Remove small border to left of Fixed banner. #2156
         - Fix issue displaying admin timeline. #2159
         - Send details of unresponsive bodies to mobile app #2164
+        - Fix issue with category filter when category contains comma #2166
 
 * v2.3.4 (7th June 2018)
     - Bugfixes:
diff --git a/perllib/FixMyStreet/App.pm b/perllib/FixMyStreet/App.pm
index 82fcce508..160d2851e 100644
--- a/perllib/FixMyStreet/App.pm
+++ b/perllib/FixMyStreet/App.pm
@@ -14,6 +14,7 @@ use Utils;
 
 use Path::Tiny 'path';
 use Try::Tiny;
+use Text::CSV;
 use URI;
 use URI::QueryParam;
 
@@ -517,7 +518,11 @@ sub get_param_list {
     my $value = $c->req->params->{$param};
     return () unless defined $value;
     my @value = ref $value ? @$value : ($value);
-    return map { split /,/, $_ } @value if $allow_commas;
+    if ($allow_commas) {
+        my $csv = Text::CSV->new;
+        $csv->parse(join ',', @value);
+        @value = $csv->fields;
+    }
     return @value;
 }
 
diff --git a/perllib/FixMyStreet/App/Controller/Auth.pm b/perllib/FixMyStreet/App/Controller/Auth.pm
index 533e6a9be..fa3403f6d 100644
--- a/perllib/FixMyStreet/App/Controller/Auth.pm
+++ b/perllib/FixMyStreet/App/Controller/Auth.pm
@@ -314,7 +314,7 @@ categories this user has been assigned to.
 sub redirect_to_categories : Private {
     my ( $self, $c ) = @_;
 
-    my $categories = join(',', @{ $c->user->categories });
+    my $categories = $c->user->categories_string;
     my $body_short = $c->cobrand->short_name( $c->user->from_body );
 
     $c->res->redirect( $c->uri_for( "/reports/" . $body_short, { filter_category => $categories } ) );
diff --git a/perllib/FixMyStreet/App/Controller/Report.pm b/perllib/FixMyStreet/App/Controller/Report.pm
index 799985f8e..f5d7db069 100644
--- a/perllib/FixMyStreet/App/Controller/Report.pm
+++ b/perllib/FixMyStreet/App/Controller/Report.pm
@@ -508,7 +508,7 @@ sub inspect : Private {
             # shortlist is always a single click away, being on the main nav.
             if ($c->user->has_body_permission_to('planned_reports')) {
                 unless ($redirect_uri = $c->get_param("post_inspect_url")) {
-                    my $categories = join(',', @{ $c->user->categories });
+                    my $categories = $c->user->categories_string;
                     my $params = {
                         lat => $problem->latitude,
                         lon => $problem->longitude,
diff --git a/perllib/FixMyStreet/App/Controller/Report/New.pm b/perllib/FixMyStreet/App/Controller/Report/New.pm
index 06885d566..9172de5b6 100644
--- a/perllib/FixMyStreet/App/Controller/Report/New.pm
+++ b/perllib/FixMyStreet/App/Controller/Report/New.pm
@@ -13,6 +13,7 @@ use Path::Class;
 use Utils;
 use mySociety::EmailUtil;
 use JSON::MaybeXS;
+use Text::CSV;
 use FixMyStreet::SMS;
 
 =head1 NAME
@@ -1509,8 +1510,11 @@ sub redirect_to_around : Private {
     foreach (qw(pc zoom)) {
         $params->{$_} = $c->get_param($_);
     }
+
+    my $csv = Text::CSV->new;
     foreach (qw(status filter_category)) {
-        $params->{$_} = join(',', $c->get_param_list($_, 1));
+        $csv->combine($c->get_param_list($_, 1));
+        $params->{$_} = $csv->string;
     }
 
     # delete empty values
diff --git a/perllib/FixMyStreet/DB/Result/User.pm b/perllib/FixMyStreet/DB/Result/User.pm
index 8b539f85d..5ba597f74 100644
--- a/perllib/FixMyStreet/DB/Result/User.pm
+++ b/perllib/FixMyStreet/DB/Result/User.pm
@@ -131,6 +131,7 @@ __PACKAGE__->load_components("+FixMyStreet::DB::RABXColumn");
 __PACKAGE__->rabx_column('extra');
 
 use Moo;
+use Text::CSV;
 use FixMyStreet::SMS;
 use mySociety::EmailUtil;
 use namespace::clean -except => [ 'meta' ];
@@ -544,6 +545,17 @@ has categories => (
     },
 );
 
+has categories_string => (
+    is => 'ro',
+    lazy => 1,
+    default => sub {
+        my $self = shift;
+        my $csv = Text::CSV->new;
+        $csv->combine(@{$self->categories});
+        return $csv->string;
+    },
+);
+
 sub set_last_active {
     my $self = shift;
     my $time = shift;
diff --git a/templates/web/base/around/postcode_form.html b/templates/web/base/around/postcode_form.html
index 52aa177a9..1293028d0 100644
--- a/templates/web/base/around/postcode_form.html
+++ b/templates/web/base/around/postcode_form.html
@@ -20,7 +20,7 @@
             [% END %]
 
             [% IF c.user_exists AND c.user.categories.size %]
-              <input type="hidden" name="filter_category" value="[% c.user.categories.join(",") | html %]">
+              <input type="hidden" name="filter_category" value="[% c.user.categories_string | html %]">
             [% END %]
         </form>
         <a href="[% c.uri_for('/around') %]" id="geolocate_link">&hellip; [% loc('or locate me automatically') %]</a>
diff --git a/templates/web/base/main_nav_items.html b/templates/web/base/main_nav_items.html
index 84657444e..618aa8328 100644
--- a/templates/web/base/main_nav_items.html
+++ b/templates/web/base/main_nav_items.html
@@ -14,7 +14,7 @@
 [%~ UNLESS hide_all_reports_link ~%]
   [%~
     IF c.user_exists AND c.user.categories.size;
-      categories = c.user.categories.join(",") | uri;
+      categories = c.user.categories_string | uri;
       cat_suffix = "?filter_category=" _ categories;
     END;
 
diff --git a/templates/web/borsetshire/around/postcode_form.html b/templates/web/borsetshire/around/postcode_form.html
index 6a569dbc5..bda1c1f1d 100644
--- a/templates/web/borsetshire/around/postcode_form.html
+++ b/templates/web/borsetshire/around/postcode_form.html
@@ -28,7 +28,7 @@
             [% END %]
 
             [% IF c.user_exists AND c.user.categories.size %]
-              <input type="hidden" name="filter_category" value="[% c.user.categories.join(",") | html %]">
+              <input type="hidden" name="filter_category" value="[% c.user.categories_string | html %]">
             [% END %]
         </form>
     </div>
diff --git a/web/js/map-OpenLayers.js b/web/js/map-OpenLayers.js
index 8f84e5c94..645e5114e 100644
--- a/web/js/map-OpenLayers.js
+++ b/web/js/map-OpenLayers.js
@@ -15,6 +15,18 @@ var fixmystreet = fixmystreet || {};
 fixmystreet.utils = fixmystreet.utils || {};
 
 $.extend(fixmystreet.utils, {
+    array_to_csv_line: function(arr) {
+        var out = [], s;
+        for (var i=0; i<arr.length; i++) {
+            s = arr[i];
+            if (/[",]/.test(s)) {
+                s = '"' + s.replace('"', '""') + '"';
+            }
+            out.push(s);
+        }
+        return out.join(',');
+    },
+
     parse_query_string: function() {
         var qs = {};
         if (!location.search) {
@@ -350,7 +362,7 @@ $.extend(fixmystreet.utils, {
     function replace_query_parameter(qs, id, key) {
         var value = $('#' + id).val();
         if (value) {
-            qs[key] = (typeof value === 'string') ? value : value.join(',');
+            qs[key] = (typeof value === 'string') ? value : fixmystreet.utils.array_to_csv_line(value);
         } else {
             delete qs[key];
         }
@@ -898,8 +910,8 @@ OpenLayers.Protocol.FixMyStreet = OpenLayers.Class(OpenLayers.Protocol.HTTP, {
         options.params = options.params || {};
         $.each({ filter_category: 'filter_categories', status: 'statuses', sort: 'sort' }, function(key, id) {
             var val = $('#' + id).val();
-            if (val !== undefined) {
-                options.params[key] = val;
+            if (val && val.length) {
+                options.params[key] = val.join ? fixmystreet.utils.array_to_csv_line(val) : val;
             }
         });
         var page;