From 017ee7c2da1168a26c3fa3959e038ea31465b9a2 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9gis=20Hanol?= <regis@hanol.fr>
Date: Tue, 30 Apr 2013 02:34:19 +0200
Subject: [PATCH] FIX: [security bug] XHR check bypass

---
 app/controllers/application_controller.rb     |  6 +----
 .../application_controller_spec.rb            | 24 ++++++++++---------
 2 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index 280827dd81c..eea419126fb 100644
--- a/app/controllers/application_controller.rb
+++ b/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
-        end
-
+        return if !request.get? && request["api_key"] && SiteSetting.api_key_valid?(request["api_key"])
         raise RenderEmpty.new unless ((request.format && request.format.json?) || request.xhr?)
       end
     end
diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb
index 2513c49762d..51292833112 100644
--- a/spec/controllers/application_controller_spec.rb
+++ b/spec/controllers/application_controller_spec.rb
@@ -1,7 +1,7 @@
 require 'spec_helper'
 
-describe 'api' do 
-  before do 
+describe 'api' 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)
-      put :bookmark, bookmarked: "true" ,post_id: post.id , api_key: SiteSetting.api_key, api_username: user.username
+      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, format: :json
     end
 
     it 'disallows phonies to bookmark posts' do
-      lambda do 
-        put :bookmark, bookmarked: "true" ,post_id: post.id , api_key: SecureRandom.hex(32), api_username: user.username
+      PostAction.expects(:act).with(user, post, PostActionType.types[:bookmark]).never
+      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 
-        put :bookmark, bookmarked: "true" ,post_id: post.id , api_key: "", api_username: user.username
+      PostAction.expects(:act).with(user, post, PostActionType.types[:bookmark]).never
+      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