Merge pull request #790 from ZogStriP/fix-security-bug-XHR-check-bypass

FIX: [security bug] XHR check bypass

app/controllers/application_controller.rb

@@ -38,7 +38,6 @@ class ApplicationController < ActionController::Base
 
   # Some exceptions
   class RenderEmpty < Exception; end
-  class NotLoggedIn < Exception; end
 
   # Render nothing unless we are an xhr request
   rescue_from RenderEmpty do
@@ -246,10 +245,7 @@ class ApplicationController < ActionController::Base
     def check_xhr
       unless (controller_name == 'forums' || controller_name == 'user_open_ids')
         # bypass xhr check on PUT / POST / DELETE provided api key is there, otherwise calling api is annoying
-        if !request.get? && request["api_key"]
+        return if !request.get? && request["api_key"] && SiteSetting.api_key_valid?(request["api_key"])
-          return
-        end
-
         raise RenderEmpty.new unless ((request.format && request.format.json?) || request.xhr?)
       end
     end

spec/controllers/application_controller_spec.rb

@@ -1,7 +1,7 @@
 require 'spec_helper'
 
-describe 'api' do 
+describe 'api' do
-  before do 
+  before do
     fake_key = SecureRandom.hex(32)
     SiteSetting.stubs(:api_key).returns(fake_key)
   end
@@ -11,26 +11,28 @@ describe 'api' do
       Fabricate(:user)
     end
 
-    let(:post) do 
+    let(:post) do
       Fabricate(:post)
     end
-    
+
     # choosing an arbitrarily easy to mock trusted activity
     it 'allows users with api key to bookmark posts' do
-      PostAction.expects(:act).with(user,post,PostActionType.types[:bookmark]).returns(true)
+      PostAction.expects(:act).with(user, post, PostActionType.types[:bookmark]).once
-      put :bookmark, bookmarked: "true" ,post_id: post.id , api_key: SiteSetting.api_key, api_username: user.username
+      put :bookmark, bookmarked: "true", post_id: post.id, api_key: SiteSetting.api_key, api_username: user.username, format: :json
     end
 
     it 'disallows phonies to bookmark posts' do
-      lambda do 
+      PostAction.expects(:act).with(user, post, PostActionType.types[:bookmark]).never
-        put :bookmark, bookmarked: "true" ,post_id: post.id , api_key: SecureRandom.hex(32), api_username: user.username
+      lambda do
+        put :bookmark, bookmarked: "true", post_id: post.id, api_key: SecureRandom.hex(32), api_username: user.username, format: :json
       end.should raise_error Discourse::NotLoggedIn
     end
-    
+
     it 'disallows blank api' do
       SiteSetting.stubs(:api_key).returns("")
-      lambda do 
+      PostAction.expects(:act).with(user, post, PostActionType.types[:bookmark]).never
-        put :bookmark, bookmarked: "true" ,post_id: post.id , api_key: "", api_username: user.username
+      lambda do
+        put :bookmark, bookmarked: "true", post_id: post.id, api_key: "", api_username: user.username, format: :json
       end.should raise_error Discourse::NotLoggedIn
     end
   end