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 280827dd8..eea419126 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 2513c4976..512928331 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