FIX: Sanitize parameters provided to user actions

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

@@ -2,13 +2,12 @@
 
 class UserActionsController < ApplicationController
   def index
-    params.require(:username)
+    user_actions_params.require(:username)
-    params.permit(:filter, :offset, :acting_username, :limit)
 
     user = fetch_user_from_params(include_inactive: current_user.try(:staff?) || (current_user && SiteSetting.show_inactive_accounts))
-    offset = [0, params[:offset].to_i].max
+    offset = [0, user_actions_params[:offset].to_i].max
-    action_types = (params[:filter] || "").split(",").map(&:to_i)
+    action_types = (user_actions_params[:filter] || "").split(",").map(&:to_i)
-    limit = params.fetch(:limit, 30).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

spec/requests/user_actions_controller_spec.rb

@@ -1,113 +1,136 @@
 # 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
+    context "when 'username' is not specified" do
-      get "/user_actions.json"
+      let(:params) { {} }
-      expect(response.status).to eq(400)
+
+      it "fails" do
+        user_actions
+        expect(response).to have_http_status :bad_request
+      end
     end
 
-    it 'renders list correctly' do
+    context "when 'username' is specified" do
-      UserActionManager.enable
+      let(:username) { post.user.username }
-      post = create_post
+      let(:params) { { username: username } }
-
+      let(:actions) { response.parsed_body["user_actions"] }
-      get "/user_actions.json", params: { username: post.user.username }
+      let(:post) { create_post }
-
-      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) }
 
       before do
         UserActionManager.enable
+      end
+
+      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
+
+      context "when 'acting_username' is provided" do
+        let(:user) { Fabricate(:user) }
+
+        before do
+          PostActionNotifier.enable
+          PostActionCreator.like(user, post)
+          params[:acting_username] = user.username
+        end
+
+        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
+
+      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
 
-      it "returns a 404" do
+        context "when `allow_users_to_hide_profile` is disabled" do
-        get "/user_actions.json", params: { username: post.user.username }
+          before do
-        expect(response.code).to eq("404")
-      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 }
+          end
-        expect(response.code).to eq("200")
+
+          it "succeeds" do
+            user_actions
+            expect(response).to have_http_status :ok
           end
         end
 
-    context "other users' activity" do
+        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 "anonymous users cannot list other users' actions of type: #{action_name}" do
+            it "cannot list other users' actions of type: #{action_name}" do
               list_and_check(action_type, 404)
             end
           end
+        end
 
-      UserAction.private_types.each do |action_type|
+        context "when user is logged in" 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
+          before do
             sign_in(user)
-          list_and_check(action_type, 404)
-        end
           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) }
-        action_name = UserAction.types.key(action_type)
 
-        it "moderators cannot list other users' actions of type: #{action_name}" do
+          before do
             sign_in(moderator)
-          list_and_check(action_type, 404)
-        end
           end
 
           UserAction.private_types.each do |action_type|
-        fab!(:admin) { Fabricate(:admin) }
             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
 
-        it "admins can list other users' actions of type: #{action_name}" do
+        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: {
@@ -118,5 +141,34 @@ describe UserActionsController do
           expect(response.status).to eq(expected_response)
         end
       end
+
+      context "when bad data is provided" do
+        fab!(:user) { Fabricate(:user) }
+
+        let(:params) do
+          {
+            filter: filter,
+            username: username,
+            offset: offset,
+            limit: limit
+          }
+        end
+        let(:filter) { "1,2" }
+        let(:username) { user.username }
+        let(:offset) { "0" }
+        let(:limit) { "10" }
+
+        %i[filter username offset limit].each do |parameter|
+          context "when providing bad data for '#{parameter}'" do
+            let(parameter) { { bad: "data" } }
+
+            it "doesn't raise an error" do
+              user_actions
+              expect(response).not_to have_http_status :error
+            end
+          end
+        end
+      end
+    end
   end
 end