From 941fc76884fcc4d6aa5768882a257203b75975df Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Javi=20Mart=C3=ADn?= <javim@elretirao.net>
Date: Sun, 10 Nov 2019 02:03:57 +0100
Subject: [PATCH 1/5] Remove unused query parameters in redirect

These actions are never called with query parameters in our application,
so there's no need to use these parameters in a redirect.

Note in the test I'm using the `get` method because the `patch` method
wouldn't send query parameters. This doesn't mean the action can be
accessed through GET requests, since controller tests don't check route
verbs.
---
 app/controllers/debates_controller.rb       |  4 ++--
 spec/controllers/debates_controller_spec.rb | 11 +++++++++++
 2 files changed, 13 insertions(+), 2 deletions(-)

diff --git a/app/controllers/debates_controller.rb b/app/controllers/debates_controller.rb
index ee2103ee2..4d4eb0e1d 100644
--- a/app/controllers/debates_controller.rb
+++ b/app/controllers/debates_controller.rb
@@ -37,12 +37,12 @@ class DebatesController < ApplicationController
 
   def unmark_featured
     @debate.update!(featured_at: nil)
-    redirect_to request.query_parameters.merge(action: :index)
+    redirect_to debates_path
   end
 
   def mark_featured
     @debate.update!(featured_at: Time.current)
-    redirect_to request.query_parameters.merge(action: :index)
+    redirect_to debates_path
   end
 
   def disable_recommendations
diff --git a/spec/controllers/debates_controller_spec.rb b/spec/controllers/debates_controller_spec.rb
index af108f906..3f8774ce6 100644
--- a/spec/controllers/debates_controller_spec.rb
+++ b/spec/controllers/debates_controller_spec.rb
@@ -50,4 +50,15 @@ describe DebatesController do
       end.not_to change { debate.reload.votes_for.size }
     end
   end
+
+  describe "PUT mark_featured" do
+    it "ignores query parameters" do
+      debate = create(:debate)
+      sign_in create(:administrator).user
+
+      get :mark_featured, params: { id: debate, controller: "proposals" }
+
+      expect(response).to redirect_to debates_path
+    end
+  end
 end

From 667797161b261a833887f71936272bc6700b742f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Javi=20Mart=C3=ADn?= <javim@elretirao.net>
Date: Sun, 10 Nov 2019 13:21:15 +0100
Subject: [PATCH 2/5] Extract method to redirect keeping query params

---
 app/controllers/admin/hidden_budget_investments_controller.rb | 4 ++--
 app/controllers/admin/hidden_comments_controller.rb           | 4 ++--
 app/controllers/admin/hidden_debates_controller.rb            | 4 ++--
 app/controllers/admin/hidden_proposals_controller.rb          | 4 ++--
 app/controllers/admin/hidden_users_controller.rb              | 4 ++--
 app/controllers/admin/organizations_controller.rb             | 4 ++--
 app/controllers/admin/proposal_notifications_controller.rb    | 4 ++--
 app/controllers/application_controller.rb                     | 4 ++++
 app/controllers/concerns/moderate_actions.rb                  | 2 +-
 app/controllers/moderation/users_controller.rb                | 2 +-
 10 files changed, 20 insertions(+), 16 deletions(-)

diff --git a/app/controllers/admin/hidden_budget_investments_controller.rb b/app/controllers/admin/hidden_budget_investments_controller.rb
index c55ab3ef6..6b977cdd3 100644
--- a/app/controllers/admin/hidden_budget_investments_controller.rb
+++ b/app/controllers/admin/hidden_budget_investments_controller.rb
@@ -15,14 +15,14 @@ class Admin::HiddenBudgetInvestmentsController < Admin::BaseController
 
   def confirm_hide
     @investment.confirm_hide
-    redirect_to request.query_parameters.merge(action: :index)
+    redirect_with_query_params_to(action: :index)
   end
 
   def restore
     @investment.restore(recursive: true)
     @investment.ignore_flag
     Activity.log(current_user, :restore, @investment)
-    redirect_to request.query_parameters.merge(action: :index)
+    redirect_with_query_params_to(action: :index)
   end
 
   private
diff --git a/app/controllers/admin/hidden_comments_controller.rb b/app/controllers/admin/hidden_comments_controller.rb
index b3f64688f..fc778a1a6 100644
--- a/app/controllers/admin/hidden_comments_controller.rb
+++ b/app/controllers/admin/hidden_comments_controller.rb
@@ -10,14 +10,14 @@ class Admin::HiddenCommentsController < Admin::BaseController
 
   def confirm_hide
     @comment.confirm_hide
-    redirect_to request.query_parameters.merge(action: :index)
+    redirect_with_query_params_to(action: :index)
   end
 
   def restore
     @comment.restore(recursive: true)
     @comment.ignore_flag
     Activity.log(current_user, :restore, @comment)
-    redirect_to request.query_parameters.merge(action: :index)
+    redirect_with_query_params_to(action: :index)
   end
 
   private
diff --git a/app/controllers/admin/hidden_debates_controller.rb b/app/controllers/admin/hidden_debates_controller.rb
index de9610972..b5a4ea502 100644
--- a/app/controllers/admin/hidden_debates_controller.rb
+++ b/app/controllers/admin/hidden_debates_controller.rb
@@ -13,14 +13,14 @@ class Admin::HiddenDebatesController < Admin::BaseController
 
   def confirm_hide
     @debate.confirm_hide
-    redirect_to request.query_parameters.merge(action: :index)
+    redirect_with_query_params_to(action: :index)
   end
 
   def restore
     @debate.restore!(recursive: true)
     @debate.ignore_flag
     Activity.log(current_user, :restore, @debate)
-    redirect_to request.query_parameters.merge(action: :index)
+    redirect_with_query_params_to(action: :index)
   end
 
   private
diff --git a/app/controllers/admin/hidden_proposals_controller.rb b/app/controllers/admin/hidden_proposals_controller.rb
index 9dea18a07..8ee137140 100644
--- a/app/controllers/admin/hidden_proposals_controller.rb
+++ b/app/controllers/admin/hidden_proposals_controller.rb
@@ -14,14 +14,14 @@ class Admin::HiddenProposalsController < Admin::BaseController
 
   def confirm_hide
     @proposal.confirm_hide
-    redirect_to request.query_parameters.merge(action: :index)
+    redirect_with_query_params_to(action: :index)
   end
 
   def restore
     @proposal.restore(recursive: true)
     @proposal.ignore_flag
     Activity.log(current_user, :restore, @proposal)
-    redirect_to request.query_parameters.merge(action: :index)
+    redirect_with_query_params_to(action: :index)
   end
 
   private
diff --git a/app/controllers/admin/hidden_users_controller.rb b/app/controllers/admin/hidden_users_controller.rb
index 31578c94e..fef56a94e 100644
--- a/app/controllers/admin/hidden_users_controller.rb
+++ b/app/controllers/admin/hidden_users_controller.rb
@@ -15,13 +15,13 @@ class Admin::HiddenUsersController < Admin::BaseController
 
   def confirm_hide
     @user.confirm_hide
-    redirect_to request.query_parameters.merge(action: :index)
+    redirect_with_query_params_to(action: :index)
   end
 
   def restore
     @user.restore
     Activity.log(current_user, :restore, @user)
-    redirect_to request.query_parameters.merge(action: :index)
+    redirect_with_query_params_to(action: :index)
   end
 
   private
diff --git a/app/controllers/admin/organizations_controller.rb b/app/controllers/admin/organizations_controller.rb
index a001e6875..1a5367286 100644
--- a/app/controllers/admin/organizations_controller.rb
+++ b/app/controllers/admin/organizations_controller.rb
@@ -19,11 +19,11 @@ class Admin::OrganizationsController < Admin::BaseController
 
   def verify
     @organization.verify
-    redirect_to request.query_parameters.merge(action: :index)
+    redirect_with_query_params_to(action: :index)
   end
 
   def reject
     @organization.reject
-    redirect_to request.query_parameters.merge(action: :index)
+    redirect_with_query_params_to(action: :index)
   end
 end
diff --git a/app/controllers/admin/proposal_notifications_controller.rb b/app/controllers/admin/proposal_notifications_controller.rb
index a591eef83..cff3d6f9d 100644
--- a/app/controllers/admin/proposal_notifications_controller.rb
+++ b/app/controllers/admin/proposal_notifications_controller.rb
@@ -12,14 +12,14 @@ class Admin::ProposalNotificationsController < Admin::BaseController
 
   def confirm_hide
     @proposal_notification.confirm_hide
-    redirect_to request.query_parameters.merge(action: :index)
+    redirect_with_query_params_to(action: :index)
   end
 
   def restore
     @proposal_notification.restore
     @proposal_notification.ignore_flag
     Activity.log(current_user, :restore, @proposal_notification)
-    redirect_to request.query_parameters.merge(action: :index)
+    redirect_with_query_params_to(action: :index)
   end
 
   private
diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index 3c0f27c98..553bd2a85 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -120,4 +120,8 @@ class ApplicationController < ActionController::Base
     def current_budget
       Budget.current
     end
+
+    def redirect_with_query_params_to(options, response_status = {})
+      redirect_to request.query_parameters.merge(options), response_status
+    end
 end
diff --git a/app/controllers/concerns/moderate_actions.rb b/app/controllers/concerns/moderate_actions.rb
index d6c35e777..a96c2d6a6 100644
--- a/app/controllers/concerns/moderate_actions.rb
+++ b/app/controllers/concerns/moderate_actions.rb
@@ -28,7 +28,7 @@ module ModerateActions
       User.where(id: author_ids).accessible_by(current_ability, :block).each { |user| block_user user }
     end
 
-    redirect_to request.query_parameters.merge(action: :index)
+    redirect_with_query_params_to(action: :index)
   end
 
   private
diff --git a/app/controllers/moderation/users_controller.rb b/app/controllers/moderation/users_controller.rb
index d0ee29f83..f07268348 100644
--- a/app/controllers/moderation/users_controller.rb
+++ b/app/controllers/moderation/users_controller.rb
@@ -9,7 +9,7 @@ class Moderation::UsersController < Moderation::BaseController
   def hide_in_moderation_screen
     block_user
 
-    redirect_to request.query_parameters.merge(action: :index), notice: I18n.t("moderation.users.notice_hide")
+    redirect_with_query_params_to({ action: :index }, { notice: I18n.t("moderation.users.notice_hide") })
   end
 
   def hide

From 50bdfd5488dffe360501454645bb86fc0990af2a Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Javi=20Mart=C3=ADn?= <javim@elretirao.net>
Date: Sun, 10 Nov 2019 15:27:20 +0100
Subject: [PATCH 3/5] Avoid redirects with unprotected query params

In theory it's possible to add a `host` parameter to a URL, and we could
end up redirecting to that host if we just redirect using query
parameters.

Generating the path using `url_for` with `only_path` solves the issue.

Note in the tests I'm using the `get` method because the `patch` method
wouldn't send query parameters. This doesn't mean the action can be
accessed through GET requests, since controller tests don't check route
verbs. Using feature specs doesn't seem to work because `controller` and
`host` parameters are filtered automatically in feature specs.

Also note I'm not testing every hidden/moderation controller because
they basically use the same code.
---
 app/controllers/application_controller.rb     |  5 +++-
 .../admin/hidden_debates_controller_spec.rb   | 25 +++++++++++++++++++
 .../admin/organizations_controller_spec.rb    | 25 +++++++++++++++++++
 .../budgets/investments_controller_spec.rb    | 15 +++++++++++
 .../moderation/users_controller_spec.rb       | 15 +++++++++++
 5 files changed, 84 insertions(+), 1 deletion(-)
 create mode 100644 spec/controllers/admin/hidden_debates_controller_spec.rb
 create mode 100644 spec/controllers/admin/organizations_controller_spec.rb
 create mode 100644 spec/controllers/moderation/budgets/investments_controller_spec.rb
 create mode 100644 spec/controllers/moderation/users_controller_spec.rb

diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index 553bd2a85..c560ee5ed 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -122,6 +122,9 @@ class ApplicationController < ActionController::Base
     end
 
     def redirect_with_query_params_to(options, response_status = {})
-      redirect_to request.query_parameters.merge(options), response_status
+      path_options = { controller: params[:controller] }.merge(options).merge(only_path: true)
+      path = url_for(request.query_parameters.merge(path_options))
+
+      redirect_to path, response_status
     end
 end
diff --git a/spec/controllers/admin/hidden_debates_controller_spec.rb b/spec/controllers/admin/hidden_debates_controller_spec.rb
new file mode 100644
index 000000000..c55617a94
--- /dev/null
+++ b/spec/controllers/admin/hidden_debates_controller_spec.rb
@@ -0,0 +1,25 @@
+require "rails_helper"
+
+describe Admin::HiddenDebatesController do
+  before { sign_in create(:administrator).user }
+
+  describe "PUT confirm_hide" do
+    it "keeps query parameters while using protected redirects" do
+      debate = create(:debate, :hidden)
+
+      get :confirm_hide, params: { id: debate, filter: "all", host: "evil.dev" }
+
+      expect(response).to redirect_to "/admin/hidden_debates?filter=all"
+    end
+  end
+
+  describe "PUT restore" do
+    it "keeps query parameters while using protected redirects" do
+      debate = create(:debate, :hidden, :with_confirmed_hide)
+
+      get :restore, params: { id: debate, filter: "all", host: "evil.dev" }
+
+      expect(response).to redirect_to "/admin/hidden_debates?filter=all"
+    end
+  end
+end
diff --git a/spec/controllers/admin/organizations_controller_spec.rb b/spec/controllers/admin/organizations_controller_spec.rb
new file mode 100644
index 000000000..bafa1819c
--- /dev/null
+++ b/spec/controllers/admin/organizations_controller_spec.rb
@@ -0,0 +1,25 @@
+require "rails_helper"
+
+describe Admin::OrganizationsController do
+  before { sign_in create(:administrator).user }
+
+  describe "PUT verify" do
+    it "keeps query parameters while using protected redirects" do
+      organization = create(:organization)
+
+      get :verify, params: { id: organization, filter: "pending", host: "evil.dev" }
+
+      expect(response).to redirect_to "/admin/organizations?filter=pending"
+    end
+  end
+
+  describe "PUT reject" do
+    it "keeps query parameters while using protected redirects" do
+      organization = create(:organization)
+
+      get :reject, params: { id: organization, filter: "pending", host: "evil.dev" }
+
+      expect(response).to redirect_to "/admin/organizations?filter=pending"
+    end
+  end
+end
diff --git a/spec/controllers/moderation/budgets/investments_controller_spec.rb b/spec/controllers/moderation/budgets/investments_controller_spec.rb
new file mode 100644
index 000000000..eacba5a2e
--- /dev/null
+++ b/spec/controllers/moderation/budgets/investments_controller_spec.rb
@@ -0,0 +1,15 @@
+require "rails_helper"
+
+describe Moderation::Budgets::InvestmentsController do
+  before { sign_in create(:moderator).user }
+
+  describe "PUT moderate" do
+    it "keeps query parameters while using protected redirects" do
+      id = create(:budget_investment).id
+
+      get :moderate, params: { resource_ids: [id], filter: "all", host: "evil.dev" }
+
+      expect(response).to redirect_to "/moderation/budget_investments?filter=all&resource_ids%5B%5D=#{id}"
+    end
+  end
+end
diff --git a/spec/controllers/moderation/users_controller_spec.rb b/spec/controllers/moderation/users_controller_spec.rb
new file mode 100644
index 000000000..424e41c8b
--- /dev/null
+++ b/spec/controllers/moderation/users_controller_spec.rb
@@ -0,0 +1,15 @@
+require "rails_helper"
+
+describe Moderation::UsersController do
+  before { sign_in create(:moderator).user }
+
+  describe "PUT hide_in_moderation_screen" do
+    it "keeps query parameters while using protected redirects" do
+      user = create(:user, email: "user@consul.dev")
+
+      get :hide_in_moderation_screen, params: { id: user, name_or_email: "user@consul.dev", host: "evil.dev" }
+
+      expect(response).to redirect_to "/moderation/users?name_or_email=user%40consul.dev"
+    end
+  end
+end

From 9065683216b87b92276d664a76d13bec45a8ea66 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Javi=20Mart=C3=ADn?= <javim@elretirao.net>
Date: Sun, 10 Nov 2019 16:14:36 +0100
Subject: [PATCH 4/5] Redirect to referer after destroying an image

The same way we do for documents. This way we avoid a possible
unprotected redirect.
---
 app/controllers/images_controller.rb            |  2 +-
 .../investments/_investment_show.html.erb       |  2 +-
 spec/controllers/images_controller_spec.rb      | 17 +++++++++++++++++
 3 files changed, 19 insertions(+), 2 deletions(-)
 create mode 100644 spec/controllers/images_controller_spec.rb

diff --git a/app/controllers/images_controller.rb b/app/controllers/images_controller.rb
index 665c7ce64..2e1093f0b 100644
--- a/app/controllers/images_controller.rb
+++ b/app/controllers/images_controller.rb
@@ -11,7 +11,7 @@ class ImagesController < ApplicationController
         else
           flash[:alert] = t "images.actions.destroy.alert"
         end
-        redirect_to params[:from]
+        redirect_to request.referer
       end
       format.js do
         if @image.destroy
diff --git a/app/views/budgets/investments/_investment_show.html.erb b/app/views/budgets/investments/_investment_show.html.erb
index 98d18e655..e112342b7 100644
--- a/app/views/budgets/investments/_investment_show.html.erb
+++ b/app/views/budgets/investments/_investment_show.html.erb
@@ -26,7 +26,7 @@
           <div class="sidebar-divider"></div>
           <h2><%= t("budgets.investments.show.author") %></h2>
           <div class="show-actions-menu">
-            <%= link_to image_path(investment.image, from: request.url),
+            <%= link_to image_path(investment.image),
                         method: :delete,
                         class: "button hollow alert expanded" do %>
                 <span class="icon-image"></span>
diff --git a/spec/controllers/images_controller_spec.rb b/spec/controllers/images_controller_spec.rb
new file mode 100644
index 000000000..5b83ac1c8
--- /dev/null
+++ b/spec/controllers/images_controller_spec.rb
@@ -0,0 +1,17 @@
+require "rails_helper"
+
+describe ImagesController do
+  let(:user) { create(:user) }
+  before { sign_in user }
+
+  describe "DELETE destroy" do
+    it "redirects to the referer URL" do
+      image = create(:image, imageable: create(:proposal, author: user))
+      request.env["HTTP_REFERER"] = "/proposals"
+
+      delete :destroy, params: { id: image, from: "http://evil.dev" }
+
+      expect(response).to redirect_to "/proposals"
+    end
+  end
+end

From 31c21ddd42018ab223ed482ec73a592c3587fae0 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Javi=20Mart=C3=ADn?= <javim@elretirao.net>
Date: Sun, 10 Nov 2019 16:56:56 +0100
Subject: [PATCH 5/5] Keep current host in links to current path

This way we avoid a possible unprotected redirect.
---
 app/helpers/application_helper.rb  | 2 +-
 spec/features/localization_spec.rb | 9 +++++++++
 2 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb
index 255534a88..44b5410cc 100644
--- a/app/helpers/application_helper.rb
+++ b/app/helpers/application_helper.rb
@@ -3,7 +3,7 @@ module ApplicationHelper
   # notice: if query_params have a param which also exist in current path,
   # it "overrides" (query_params is merged last)
   def current_path_with_query_params(query_parameters)
-    url_for(request.query_parameters.merge(query_parameters))
+    url_for(request.query_parameters.merge(query_parameters).merge(only_path: true))
   end
 
   def markdown(text)
diff --git a/spec/features/localization_spec.rb b/spec/features/localization_spec.rb
index 47cf53528..4056cb8dd 100644
--- a/spec/features/localization_spec.rb
+++ b/spec/features/localization_spec.rb
@@ -40,6 +40,15 @@ describe "Localization" do
     expect(page).to have_select("locale-switcher", selected: "Español")
   end
 
+  scenario "Keeps query parameters while using protected redirects", :js do
+    visit "/debates?order=created_at&host=evil.dev"
+
+    select("Español", from: "locale-switcher")
+
+    expect(current_host).to eq "http://127.0.0.1"
+    expect(page).to have_current_path "/debates?locale=es&order=created_at"
+  end
+
   context "Only one locale" do
     before do
       allow(I18n).to receive(:available_locales).and_return([:en])