From 325bccc1649c928d512ce7c7b11e14566a8c9eeb Mon Sep 17 00:00:00 2001
From: Azul <azul@riseup.net>
Date: Sun, 17 Sep 2017 09:54:55 +0200
Subject: fix: sanity checks on user params

fixes #8801

Includes a test reproducing 500 on lynx

We now make use of ActionController::Parameters require and permit
methods.
---
 app/controllers/api/users_controller.rb      | 11 ++++++++++-
 test/functional/api/users_controller_test.rb | 22 ++++++++++++++++++----
 test/integration/api/srp_test.rb             |  2 +-
 test/integration/api/update_account_test.rb  |  8 ++++++++
 4 files changed, 37 insertions(+), 6 deletions(-)

diff --git a/app/controllers/api/users_controller.rb b/app/controllers/api/users_controller.rb
index 709e076..cb7b7bc 100644
--- a/app/controllers/api/users_controller.rb
+++ b/app/controllers/api/users_controller.rb
@@ -53,7 +53,7 @@ module Api
     end
 
     def update
-      @user.account.update params[:user]
+      @user.account.update user_update_params
       respond_with @user
     end
 
@@ -67,6 +67,15 @@ module Api
 
     private
 
+    def user_update_params
+      params.require(:user).permit :login,
+        :password_verifier,
+        :password_salt,
+        :recovery_code_verifier,
+        :recovery_code_salt,
+        :public_key
+    end
+
     def release_handles
       current_user.is_monitor? || params[:identities] == "destroy"
     end
diff --git a/test/functional/api/users_controller_test.rb b/test/functional/api/users_controller_test.rb
index 88ecae0..ee183f9 100644
--- a/test/functional/api/users_controller_test.rb
+++ b/test/functional/api/users_controller_test.rb
@@ -4,28 +4,42 @@ class Api::UsersControllerTest < ApiControllerTest
 
   test "user can change settings" do
     user = find_record :user
-    changed_attribs = record_attributes_for :user_with_settings
+    attribs = record_attributes_for(:user)
+    changed_attribs = attribs.slice 'login',
+      'password_verifier',
+      'password_salt'
     account_settings = stub
     account_settings.expects(:update).with(changed_attribs)
     Account.expects(:new).with(user).returns(account_settings)
 
     login user
-    api_put :update, :user => changed_attribs, :id => user.id, :format => :json
+    api_put :update, :user => attribs, :id => user.id, :format => :json
 
     assert_equal user, assigns[:user]
     assert_response 204
     assert @response.body.blank?, "Response should be blank"
   end
 
+  test "deal with empty settings" do
+    user = find_record :user
+    login user
+    assert_raises ActionController::ParameterMissing do
+      api_put :update, :id => user.id, :format => :json
+    end
+  end
+
   test "admin can update user" do
     user = find_record :user
-    changed_attribs = record_attributes_for :user_with_settings
+    attribs = record_attributes_for(:user)
+    changed_attribs = attribs.slice 'login',
+      'password_verifier',
+      'password_salt'
     account_settings = stub
     account_settings.expects(:update).with(changed_attribs)
     Account.expects(:new).with(user).returns(account_settings)
 
     login :is_admin? => true
-    api_put :update, :user => changed_attribs, :id => user.id, :format => :json
+    api_put :update, :user => attribs, :id => user.id, :format => :json
 
     assert_equal user, assigns[:user]
     assert_response 204
diff --git a/test/integration/api/srp_test.rb b/test/integration/api/srp_test.rb
index b9605f9..ef5d9b8 100644
--- a/test/integration/api/srp_test.rb
+++ b/test/integration/api/srp_test.rb
@@ -46,7 +46,7 @@ class SrpTest < RackTest
     @password = password
   end
 
-  def update_user(params)
+  def update_user(params = {})
     put api_url("users/#{@user.id}.json"),
       user_params(params),
       auth_headers
diff --git a/test/integration/api/update_account_test.rb b/test/integration/api/update_account_test.rb
index f083dbc..dd28b06 100644
--- a/test/integration/api/update_account_test.rb
+++ b/test/integration/api/update_account_test.rb
@@ -19,6 +19,14 @@ class UpdateAccountTest < SrpTest
     assert_login_required
   end
 
+  test "empty request" do
+    authenticate
+    update_user
+    refute last_response.successful?
+    assert_equal 400, last_response.status
+    assert_equal '', last_response.body
+  end
+
   test "update password via api" do
     authenticate
     update_user password: "No! Verify me instead."
-- 
cgit v1.2.3