From 84a0a89f4bd1c13d0b02dc95a373f78639677907 Mon Sep 17 00:00:00 2001
From: evazion <noizave@gmail.com>
Date: Wed, 7 Feb 2018 17:11:52 -0600
Subject: [PATCH] Fix #3539: Open redirect vulnerabilities.

---
 app/controllers/application_controller.rb | 14 ++++++++------
 app/helpers/pagination_helper.rb          | 15 ++++++++-------
 app/helpers/posts_helper.rb               |  9 +++++----
 test/functional/tags_controller_test.rb   |  8 ++++++++
 4 files changed, 29 insertions(+), 17 deletions(-)

diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index 928398ed4..b37a9cbd8 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -196,17 +196,19 @@ class ApplicationController < ActionController::Base
     @page_title = Danbooru.config.app_name + "/#{params[:controller]}"
   end
 
+  # Remove blank `search` params from the url.
+  #
+  # /tags?search[name]=touhou&search[category]=&search[order]=
+  # => /tags?search[name]=touhou
   def normalize_search
     if request.get?
       if params[:search].blank?
-        params[:search] = {}
+        params[:search] = ActionController::Parameters.new
       end
 
-      if params[:search].is_a?(Hash)
-        changed = params[:search].reject! {|k,v| v.blank?}
-        unless changed.nil?
-          redirect_to url_for(params)
-        end
+      if params[:search].is_a?(ActionController::Parameters) && params[:search].values.any?(&:blank?)
+        params[:search].reject! {|k,v| v.blank?}
+        redirect_to url_for(params: params.except(:controller, :action, :index).permit!)
       end
     end
   end
diff --git a/app/helpers/pagination_helper.rb b/app/helpers/pagination_helper.rb
index 0f5cd7ac2..b5ec4c6a4 100644
--- a/app/helpers/pagination_helper.rb
+++ b/app/helpers/pagination_helper.rb
@@ -4,11 +4,11 @@ module PaginationHelper
 
     if records.any?
       if params[:page] =~ /[ab]/ && !records.is_first_page?
-        html << '<li>' + link_to("< Previous", nav_params.merge(:page => "a#{records[0].id}"), :rel => "prev") + '</li>'
+        html << '<li>' + link_to("< Previous", nav_params_for("a#{records[0].id}"), :rel => "prev") + '</li>'
       end
 
       unless records.is_last_page?
-        html << '<li>' + link_to("Next >", nav_params.merge(:page => "b#{records[-1].id}"), :rel => "next") + '</li>'
+        html << '<li>' + link_to("Next >", nav_params_for("b#{records[-1].id}"), :rel => "next") + '</li>'
       end
     end
 
@@ -29,7 +29,7 @@ module PaginationHelper
     window = 4
 
     if records.current_page >= 2
-      html << "<li class='arrow'>" + link_to("<<", nav_params.merge(:page => records.current_page - 1), :rel => "prev") + "</li>"
+      html << "<li class='arrow'>" + link_to("<<", nav_params_for(records.current_page - 1), :rel => "prev") + "</li>"
     else
       html << "<li class='arrow'><span>" + "&lt;&lt;" + "</span></li>"
     end
@@ -69,7 +69,7 @@ module PaginationHelper
     end
 
     if records.current_page < records.total_pages && records.size > 0
-      html << "<li class='arrow'>" + link_to(">>", nav_params.merge(:page => records.current_page + 1), :rel => "next") + "</li>"
+      html << "<li class='arrow'>" + link_to(">>", nav_params_for(records.current_page + 1), :rel => "next") + "</li>"
     else
       html << "<li class='arrow'><span>" + "&gt;&gt;" + "</span></li>"
     end
@@ -100,7 +100,7 @@ module PaginationHelper
       html << "</li>"
     else
       html << "<li class='numbered-page'>"
-      html << link_to(page, nav_params.merge(:page => page)) # XXX
+      html << link_to(page, nav_params_for(page))
       html << "</li>"
     end
     html.join.html_safe
@@ -108,7 +108,8 @@ module PaginationHelper
 
   private
 
-  def nav_params
-    params.to_unsafe_h # XXX
+  def nav_params_for(page)
+    query_params = params.except(:controller, :action, :id).merge(page: page).permit!
+    { params: query_params }
   end
 end
diff --git a/app/helpers/posts_helper.rb b/app/helpers/posts_helper.rb
index 07b058739..15a4e34d1 100644
--- a/app/helpers/posts_helper.rb
+++ b/app/helpers/posts_helper.rb
@@ -5,13 +5,13 @@ module PostsHelper
 
   def next_page_url
     current_page = (params[:page] || 1).to_i
-    url_for(nav_params.merge(page: current_page + 1)).html_safe
+    url_for(nav_params_for(current_page + 1)).html_safe
   end
 
   def prev_page_url
     current_page = (params[:page] || 1).to_i
     if current_page >= 2
-      url_for(nav_params.merge(page: current_page - 1)).html_safe
+      url_for(nav_params_for(current_page - 1)).html_safe
     else
       nil
     end
@@ -134,7 +134,8 @@ module PostsHelper
 
   private
 
-  def nav_params
-    params.to_unsafe_h # XXX
+  def nav_params_for(page)
+    query_params = params.except(:controller, :action, :id).merge(page: page).permit!
+    { params: query_params }
   end
 end
diff --git a/test/functional/tags_controller_test.rb b/test/functional/tags_controller_test.rb
index 7bfc3f624..a57e36460 100644
--- a/test/functional/tags_controller_test.rb
+++ b/test/functional/tags_controller_test.rb
@@ -40,6 +40,14 @@ class TagsControllerTest < ActionController::TestCase
           assert_response :success
         end
       end
+
+      context "with blank search parameters" do
+        should "strip the blank parameters with a redirect" do
+          get :index, { search: { name: "touhou", category: "" } }
+
+          assert_redirected_to tags_path(search: { name: "touhou" })
+        end
+      end
     end
 
     context "autocomplete action" do