From e871865a615720d24bb590b45c324e00b548cb5d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Lo=C3=AFc=20Guitaut?= <loic@discourse.org>
Date: Tue, 22 Feb 2022 12:02:04 +0100
Subject: [PATCH] FIX: Sanitize parameters provided to user actions
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Currently, providing things like `filter[%24acunetix]=1` to
`UserActionsController#index` will throw an exception because instead of
getting a string as expected, we get a hash instead.

This patch simply uses `#permit` from strong parameters properly: first
we apply it on the whole parameters, this way it filters the keys we’re
interested in. By doing this, if the value is a hash for example, the
whole key/value pair will be ignored completely.
---
 app/controllers/user_actions_controller.rb    |  16 +-
 spec/requests/user_actions_controller_spec.rb | 238 +++++++++++-------
 2 files changed, 155 insertions(+), 99 deletions(-)

diff --git a/app/controllers/user_actions_controller.rb b/app/controllers/user_actions_controller.rb
index fba1a6354a6..18efefa7079 100644
--- a/app/controllers/user_actions_controller.rb
+++ b/app/controllers/user_actions_controller.rb
@@ -2,13 +2,12 @@
 
 class UserActionsController < ApplicationController
   def index
-    params.require(:username)
-    params.permit(:filter, :offset, :acting_username, :limit)
+    user_actions_params.require(:username)
 
     user = fetch_user_from_params(include_inactive: current_user.try(:staff?) || (current_user && SiteSetting.show_inactive_accounts))
-    offset = [0, params[:offset].to_i].max
-    action_types = (params[:filter] || "").split(",").map(&:to_i)
-    limit = params.fetch(:limit, 30).to_i
+    offset = [0, user_actions_params[:offset].to_i].max
+    action_types = (user_actions_params[:filter] || "").split(",").map(&:to_i)
+    limit = user_actions_params.fetch(:limit, 30).to_i
 
     raise Discourse::NotFound unless guardian.can_see_profile?(user)
     raise Discourse::NotFound unless guardian.can_see_user_actions?(user, action_types)
@@ -20,7 +19,7 @@ class UserActionsController < ApplicationController
       limit: limit,
       action_types: action_types,
       guardian: guardian,
-      ignore_private_messages: params[:filter] ? false : true,
+      ignore_private_messages: params[:filter].blank?,
       acting_username: params[:acting_username]
     }
 
@@ -38,4 +37,9 @@ class UserActionsController < ApplicationController
     # TODO should preload messages to avoid extra http req
   end
 
+  private
+
+  def user_actions_params
+    @user_actions_params ||= params.permit(:username, :filter, :offset, :acting_username, :limit)
+  end
 end
diff --git a/spec/requests/user_actions_controller_spec.rb b/spec/requests/user_actions_controller_spec.rb
index 1d9c01c1015..714a0bf59a8 100644
--- a/spec/requests/user_actions_controller_spec.rb
+++ b/spec/requests/user_actions_controller_spec.rb
@@ -1,122 +1,174 @@
 # frozen_string_literal: true
 
-require 'rails_helper'
+require "rails_helper"
 
 describe UserActionsController do
-  context 'index' do
+  describe "GET index" do
+    subject(:user_actions) { get "/user_actions.json", params: params }
 
-    it 'fails if username is not specified' do
-      get "/user_actions.json"
-      expect(response.status).to eq(400)
+    context "when 'username' is not specified" do
+      let(:params) { {} }
+
+      it "fails" do
+        user_actions
+        expect(response).to have_http_status :bad_request
+      end
     end
 
-    it 'renders list correctly' do
-      UserActionManager.enable
-      post = create_post
-
-      get "/user_actions.json", params: { username: post.user.username }
-
-      expect(response.status).to eq(200)
-      parsed = response.parsed_body
-      actions = parsed["user_actions"]
-      expect(actions.length).to eq(1)
-      action = actions[0]
-      expect(action["acting_name"]).to eq(post.user.name)
-      expect(action["email"]).to eq(nil)
-      expect(action["post_number"]).to eq(1)
-    end
-
-    it 'can be filtered by acting_username' do
-      UserActionManager.enable
-      PostActionNotifier.enable
-
-      post = Fabricate(:post)
-      user = Fabricate(:user)
-      PostActionCreator.like(user, post)
-
-      get "/user_actions.json", params: {
-        username: post.user.username,
-        acting_username: user.username
-      }
-
-      expect(response.status).to eq(200)
-
-      response_body = response.parsed_body
-
-      expect(response_body["user_actions"].count).to eq(1)
-
-      expect(response_body["user_actions"].first["acting_username"])
-        .to eq(user.username)
-    end
-
-    context 'hidden profiles' do
-      fab!(:post) { Fabricate(:post) }
+    context "when 'username' is specified" do
+      let(:username) { post.user.username }
+      let(:params) { { username: username } }
+      let(:actions) { response.parsed_body["user_actions"] }
+      let(:post) { create_post }
 
       before do
         UserActionManager.enable
-        post.user.user_option.update_column(:hide_profile_and_presence, true)
       end
 
-      it "returns a 404" do
-        get "/user_actions.json", params: { username: post.user.username }
-        expect(response.code).to eq("404")
+      it "renders list correctly" do
+        user_actions
+        expect(response).to have_http_status :ok
+        expect(actions.first).to include "acting_name" => post.user.name,
+                                         "post_number" => 1
+        expect(actions.first).not_to include "email"
       end
 
-      it "succeeds when `allow_users_to_hide_profile` is false" do
-        SiteSetting.allow_users_to_hide_profile = false
-        get "/user_actions.json", params: { username: post.user.username }
-        expect(response.code).to eq("200")
-      end
-    end
+      context "when 'acting_username' is provided" do
+        let(:user) { Fabricate(:user) }
 
-    context "other users' activity" do
-      fab!(:another_user) { Fabricate(:user) }
+        before do
+          PostActionNotifier.enable
+          PostActionCreator.like(user, post)
+          params[:acting_username] = user.username
+        end
 
-      UserAction.private_types.each do |action_type|
-        action_name = UserAction.types.key(action_type)
-        it "anonymous users cannot list other users' actions of type: #{action_name}" do
-          list_and_check(action_type, 404)
+        it "filters its results" do
+          user_actions
+          expect(response).to have_http_status :ok
+          expect(actions.first).to include "acting_username" => user.username
         end
       end
 
-      UserAction.private_types.each do |action_type|
+      context "when user's profile is hidden" do
+        fab!(:post) { Fabricate(:post) }
+
+        before do
+          post.user.user_option.update_column(:hide_profile_and_presence, true)
+        end
+
+        context "when `allow_users_to_hide_profile` is disabled" do
+          before do
+            SiteSetting.allow_users_to_hide_profile = false
+          end
+
+          it "succeeds" do
+            user_actions
+            expect(response).to have_http_status :ok
+          end
+        end
+
+        context "when `allow_users_to_hide_profile` is enabled" do
+          it "returns a 404" do
+            user_actions
+            expect(response).to have_http_status :not_found
+          end
+        end
+      end
+
+      context "when checking other users' activity" do
+        fab!(:another_user) { Fabricate(:user) }
+
+        context "when user is anonymous" do
+          UserAction.private_types.each do |action_type|
+            action_name = UserAction.types.key(action_type)
+            it "cannot list other users' actions of type: #{action_name}" do
+              list_and_check(action_type, 404)
+            end
+          end
+        end
+
+        context "when user is logged in" do
+          fab!(:user) { Fabricate(:user) }
+
+          before do
+            sign_in(user)
+          end
+
+          UserAction.private_types.each do |action_type|
+            action_name = UserAction.types.key(action_type)
+            it "cannot list other users' actions of type: #{action_name}" do
+              list_and_check(action_type, 404)
+            end
+          end
+        end
+
+        context "when user is a moderator" do
+          fab!(:moderator) { Fabricate(:moderator) }
+
+          before do
+            sign_in(moderator)
+          end
+
+          UserAction.private_types.each do |action_type|
+            action_name = UserAction.types.key(action_type)
+            it "cannot list other users' actions of type: #{action_name}" do
+              list_and_check(action_type, 404)
+            end
+          end
+        end
+
+        context "when user is an admin" do
+          fab!(:admin) { Fabricate(:admin) }
+
+          before do
+            sign_in(admin)
+          end
+
+          UserAction.private_types.each do |action_type|
+            action_name = UserAction.types.key(action_type)
+            it "can list other users' actions of type: #{action_name}" do
+              list_and_check(action_type, 200)
+            end
+          end
+        end
+
+        def list_and_check(action_type, expected_response)
+          get "/user_actions.json", params: {
+            filter: action_type,
+            username: another_user.username
+          }
+
+          expect(response.status).to eq(expected_response)
+        end
+      end
+
+      context "when bad data is provided" do
         fab!(:user) { Fabricate(:user) }
-        action_name = UserAction.types.key(action_type)
 
-        it "logged in users cannot list other users' actions of type: #{action_name}" do
-          sign_in(user)
-          list_and_check(action_type, 404)
+        let(:params) do
+          {
+            filter: filter,
+            username: username,
+            offset: offset,
+            limit: limit
+          }
         end
-      end
+        let(:filter) { "1,2" }
+        let(:username) { user.username }
+        let(:offset) { "0" }
+        let(:limit) { "10" }
 
-      UserAction.private_types.each do |action_type|
-        fab!(:moderator) { Fabricate(:moderator) }
-        action_name = UserAction.types.key(action_type)
+        %i[filter username offset limit].each do |parameter|
+          context "when providing bad data for '#{parameter}'" do
+            let(parameter) { { bad: "data" } }
 
-        it "moderators cannot list other users' actions of type: #{action_name}" do
-          sign_in(moderator)
-          list_and_check(action_type, 404)
+            it "doesn't raise an error" do
+              user_actions
+              expect(response).not_to have_http_status :error
+            end
+          end
         end
       end
-
-      UserAction.private_types.each do |action_type|
-        fab!(:admin) { Fabricate(:admin) }
-        action_name = UserAction.types.key(action_type)
-
-        it "admins can list other users' actions of type: #{action_name}" do
-          sign_in(admin)
-          list_and_check(action_type, 200)
-        end
-      end
-
-      def list_and_check(action_type, expected_response)
-        get "/user_actions.json", params: {
-          filter: action_type,
-          username: another_user.username
-        }
-
-        expect(response.status).to eq(expected_response)
-      end
     end
   end
 end