Sanitise filter parameter values

We occasionally (mostly through pentests) get values for filters that
contain random odd Unicode characters. Google's filter logic has failing
validations in those cases, causing an error to be raised on the
request.

This adds basic, allowlist based sanitisation for all incoming filter
values. We know that the values come from a limited character set
anyway (slugs and UUIDs), as they are not direct user input.

- Add value parameter sanitisation for filters
- Remove existing escape logic in `FilterExpressionHelpers` (as there
  should never be any quotes or backslashes in the values to begin with,
  and we will now have removed them anyway)

app/services/discovery_engine/query/filter_expression_helpers.rb

@@ -6,7 +6,7 @@ module FilterExpressionHelpers
     # values in string_value_or_values
     def filter_any_string(string_or_array_field, string_value_or_values)
       Array(string_value_or_values)
-        .map { escape_and_quote(_1) }
+        .map { quote(_1) }
         .join(",")
         .then { "#{string_or_array_field}: ANY(#{_1})" }
     end
@@ -69,9 +69,8 @@ def parenthesize(expression)
       "(#{expression})"
     end
 
-    def escape_and_quote(string_value)
-      escaped_string = string_value.gsub(/(["\\])/, '\\\\\1')
-      "\"#{escaped_string}\""
+    def quote(string_value)
+      "\"#{string_value}\""
     end
   end
 end

app/services/discovery_engine/query/filters.rb

@@ -1,6 +1,7 @@
 module DiscoveryEngine::Query
   class Filters
     FILTER_PARAM_KEY_REGEX = /\A(filter_all|filter|reject)_(.+)\z/
+    ACCEPTABLE_VALUE_REGEX = /\A[a-zA-Z0-9_:,\-\/]+\z/
 
     FILTERABLE_STRING_FIELDS = %w[
       content_purpose_supergroup
@@ -70,9 +71,11 @@ def valid_filter_value?(value)
       # for the time being. This ensures that occasionally observed garbage parameters such as:
       #
       #   filter_world_locations[\\\\]=all
+      #   filter_something=blah%00blah
       #
-      # are ignored by checking that the value is either a string or an array of strings.
-      Array(value).all? { _1.is_a?(String) }
+      # are ignored by checking that the value is either a string or an array of strings, and
+      # matches an allowlist of permissible characters.
+      Array(value).all? { _1.is_a?(String) && _1.match?(ACCEPTABLE_VALUE_REGEX) }
     end
   end
 end

spec/services/discovery_engine/query/filters_spec.rb

@@ -32,6 +32,12 @@
 
         it { is_expected.to be_nil }
       end
+
+      context "with invalid characters in the filter value" do
+        let(:query_params) { { q: "garden centres", reject_link: "foo\u0000bar" } }
+
+        it { is_expected.to be_nil }
+      end
     end
 
     context "with an 'any' string filter" do
@@ -65,6 +71,12 @@
         it { is_expected.to be_nil }
       end
 
+      context "with invalid characters in the filter value" do
+        let(:query_params) { { q: "garden centres", filter_link: "foo\u0000bar" } }
+
+        it { is_expected.to be_nil }
+      end
+
       context "with an unknown field" do
         let(:query_params) { { q: "garden centres", filter_foo: "bar" } }
 
@@ -101,6 +113,12 @@
         it { is_expected.to be_nil }
       end
 
+      context "with invalid characters in the filter value" do
+        let(:query_params) { { q: "garden centres", filter_all_link: "foo\u0000bar" } }
+
+        it { is_expected.to be_nil }
+      end
+
       context "with an unknown field" do
         let(:query_params) { { q: "garden centres", filter_all_foo: "bar" } }
 
@@ -177,11 +195,5 @@
 
       it { is_expected.to eq('(NOT link: ANY("/foo")) AND (content_purpose_supergroup: ANY("services")) AND ((part_of_taxonomy_tree: ANY("cafe-1234")) AND (part_of_taxonomy_tree: ANY("face-5678"))) AND (public_timestamp: IN(629510400,629596799))') }
     end
-
-    context "with filters containing escapable characters" do
-      let(:query_params) { { q: "garden centres", filter_content_purpose_supergroup: "foo\"\\bar" } }
-
-      it { is_expected.to eq('content_purpose_supergroup: ANY("foo\\"\\\\bar")') }
-    end
   end
 end