validate sort direction (#17439)

* Prefer compact_blank! to delete_if blank

Rubocop suggested this. Why would I argue.

* When sort_direction is anything other than asc or desc, remove it

There is initializer code in the search/query classes that handles nil
sort_direction by adding a default value, so deleting the sort
direction is reasonable here.

* Set a default sort direction for articles of :desc

* Only sort if sort_direction and sort_by are present

* Add test case for article search with invalid parameter

app/controllers/search_controller.rb

@@ -50,6 +50,8 @@ class SearchController < ApplicationController
     },
   ].freeze
 
+  VALID_SORT_DIRECTIONS = %i[asc desc].freeze
+
   def tags
     result = Search::Tag.search_documents(term: params[:name])
     render json: { result: result }
@@ -196,6 +198,14 @@ class SearchController < ApplicationController
   # nil differently. This is a helper method to remove any params that are
   # blank before passing it to Elasticsearch.
   def sanitize_params
-    params.delete_if { |_k, v| v.blank? }
+    params.compact_blank!
+    remove_invalid_sort_directions
+  end
+
+  def remove_invalid_sort_directions
+    return unless params.key?(:sort_direction)
+
+    direction = params[:sort_direction].downcase.to_sym
+    params.delete(:sort_direction) unless direction.in?(VALID_SORT_DIRECTIONS)
   end
 end

app/queries/homepage/articles_query.rb

@@ -17,7 +17,9 @@ module Homepage
     ].freeze
     DEFAULT_PER_PAGE = 60
     MAX_PER_PAGE = 100
+
     SORT_PARAMS = %i[hotness_score public_reactions_count published_at].freeze
+    DEFAULT_SORT_DIRECTION = :desc
 
     def self.call(...)
       new(...).call
@@ -44,7 +46,7 @@ module Homepage
       @tags = tags.presence || []
 
       @sort_by = sort_by
-      @sort_direction = sort_direction
+      @sort_direction = sort_direction || DEFAULT_SORT_DIRECTION
 
       @page = page.to_i + 1
       @per_page = [(per_page || DEFAULT_PER_PAGE).to_i, MAX_PER_PAGE].min

app/services/search/article.rb

@@ -23,7 +23,7 @@ module Search
       # By skipping ordering, we rely on the custom ranking defined in the article's tsvector document
       return relation if term.present? && sort_by.blank?
 
-      return relation.reorder(sort_by => sort_direction) if sort_by&.to_sym == :published_at
+      return relation.reorder(sort_by => sort_direction) if sort_by&.to_sym == :published_at && sort_direction
 
       relation.reorder(DEFAULT_SORT_BY)
     end

spec/requests/search_spec.rb

@@ -157,6 +157,21 @@ RSpec.describe "Search", type: :request, proper_status: true do
 
         expect(Search::Article).to have_received(:search_documents)
       end
+
+      it "nullifies invalid sort directions" do
+        allow(Search::Article).to receive(:search_documents).and_call_original
+        article = create(:article)
+
+        get search_feed_content_path(
+          class_name: "Article", page: 0, per_page: 1, search_fields: article.title,
+          sort_by: :published_at, sort_direction: :invalid
+        )
+
+        expect(response.parsed_body["result"].first["id"]).to eq(article.id)
+        expect(Search::Article)
+          .to have_received(:search_documents)
+          .with(hash_including(sort_direction: nil))
+      end
     end
 
     context "when searching for comments" do