Security: Turn on protect_from_forgery for admin/

The Rails protect_from_forgery function helps protect against cross-site request forgery attacks, as described on Wikipedia: http://en.wikipedia.org/wiki/Cross-site_request_forgery These attacks involve a hostile site sending requests to a site where the user is logged in, exploiting the user's session cookie to do various bad things. The protect_from_forgery function works by requiring all POST (and PUT and UPDATE) requests to have an authenticity_token parameter that corresponds to a value in the user's session. This is automatically included in generated forms by the various form helpers, and checked in the controller. However, we still need to deal with some cases (specifically Ajax.Request) manually. We make several types of changes to get everything working again: - Some POST requests were changed to GET requests, when appropriate. - The token was added manually to other POST requests. This was done using the new init_mephisto_authenticity_token. - Forgery protection was disabled in the test environment. Note that we still need to review the authentication controller closely, and eliminate various XSS attacks against our application before this protection will do much good. I tested this code by manually using the admin/ interface, editing articles, adding users, and working with assets. There's probably still some breakage somewhere that I missed, so let me know if you have problems. I also updated the TODO list for Rails 2.2 and added security-auditing notes.
emk · Dec 10, 2008 · dd9f41d2d4f168281b2e4eca12a525065b999f61 · dd9f41d
1 parent d558ba1
commit dd9f41d2d4f168281b2e4eca12a525065b999f61
diff --git a/RAILS-2.2-TODO.txt b/RAILS-2.2-TODO.txt
@@ -1,22 +1,11 @@
 This is a list of issues that we need to fix before making a Mephisto
 release based on Rails 2.2.

+/ Try to upgrade to gem version of coderay
+  Make sure both old- and new-style plugins work
+  Security audit--see blow
  Drop TZInfo completely--see app/models/site.rb (fix docs, too)
-  Handle inactive users with named scopes, not acts_as_versioned
-  SECURITY: Don't ship :session_key in environment.rb!
-  Try to upgrade to gem version of coderay
  We need to review our TODO comments
-  Clean out the issue tracker: http://ar-code.lighthouseapp.com/projects/34-mephisto
-  rake rails:update:javascripts => complicated because mephisto/application.js depends on older versions.
-
-== Other issues
-
-When running the unit tests, we need theme directories in themes/site-$ID.
-But this namespace is also used by the development and production
-databases.  Are the test suites clobbering user themes accidentally?  See
-spec/models/membership_spec.rb for one quick-and-dirty workaround.  Perhaps
-for all non-production environments, we should prepend RAILS-ENV to the
-theme name, giving us themes/test-site-$ID?

 == Security

@@ -26,13 +15,39 @@ We need to do a basic security audit.
    Make sure cookies are HTTP-only whenever possible
    Can we restrict admin cookies to /admin ?
    Make sure logging out clears all relevant cookies and tokens
+    Check for session fixation attacks
+    Expire sessions after a while?
  Cross-site scripting
+/   Turn on protect_against_forgery
    Check all fields in comments
+      It looks like the failed comment error form has issues
    Check macro:* bodies and parameters
      filtered_column_code_macro
      filtered_column_flikr_macro
    Do we have trackback support to check?
    Do we need to upgrade to an industrial-strength HTML sanitizer?
    For now, we'll assume that users with access to /admin don't try XSS
+  Password change
+    Make change-password forms safe against CSRF
+    Require the user to enter the old password when changing it
+    Require password to change e-mail address
  Everything else
+/   Don't ship :session_key in environment.rb!
+    Do we need to override verifiable_request_format?
+    Review mass assignment in public controllers
    Review http://guides.rubyonrails.org/security.html
+    Can we use SafeERB?
+
+  Admin only
+    Filter file names for uploads
+    Review mass assignment in admin controllers
+
+== After next release
+
+  Handle inactive users with named scopes, not acts_as_versioned
+  Clean out the issue tracker
+    http://ar-code.lighthouseapp.com/projects/34-mephisto
+  rake rails:update:javascripts
+    (complicated because mephisto/application.js depends on older versions)
+  Fix sidebar tabs to do something sensible with unsaved articles
+
diff --git a/app/controllers/admin/base_controller.rb b/app/controllers/admin/base_controller.rb
@@ -7,6 +7,9 @@ class Admin::BaseController < ApplicationController
  before_filter :login_from_cookie
  before_filter :login_required, :except => :feed

+  # See ActionController::RequestForgeryProtection for details.
+  protect_from_forgery
+  
  protected
    def protect_action
      if request.get?
diff --git a/app/helpers/application_helper.rb b/app/helpers/application_helper.rb
@@ -49,6 +49,12 @@ def relative_url_root
    ActionController::Base.relative_url_root
  end

+  # Make our form_authenticity_token token available to JavaScript.
+  def init_mephisto_authenticity_token
+    return "" unless protect_against_forgery?
+    "Mephisto.token = '#{form_authenticity_token}';"
+  end
+
  if RAILS_ENV == 'development'
    def gravatar_url_for(user, size = 80)
      'mephisto/avatar.gif'
diff --git a/app/views/layouts/application.rhtml b/app/views/layouts/application.rhtml
@@ -6,7 +6,7 @@
    <title><%= site.title %>: Admin <%= controller.controller_name %></title>
    <%= stylesheet_link_tag 'mephisto/mephisto' %>
    <%= javascript_include_tag 'mephisto/prototype', 'mephisto/effects', 'mephisto/dragdrop', 'mephisto/lowpro', 'mephisto/application' %>
-    <script type="text/javascript">Mephisto.root = '<%= relative_url_root %>';</script>
+    <script type="text/javascript">Mephisto.root = '<%= relative_url_root %>'; <%= init_mephisto_authenticity_token %></script>
    <%= yield :head %>
  </head>
  <body>
diff --git a/config/environments/test.rb b/config/environments/test.rb
@@ -3,4 +3,7 @@
 config.action_controller.consider_all_requests_local = true
 config.action_controller.perform_caching             = true
 config.action_controller.page_cache_directory        = File.join(RAILS_ROOT, 'tmp/cache')
-config.action_mailer.delivery_method = :test
+config.action_mailer.delivery_method = :test
+
+# Disable request forgery protection in test environment
+config.action_controller.allow_forgery_protection    = false
diff --git a/lib/mephisto/routing.rb b/lib/mephisto/routing.rb
@@ -28,7 +28,7 @@ def self.connect_with(map)
      map.overview 'admin/overview.xml', :controller => 'admin/overview', :action => 'feed'
      map.admin    'admin', :controller => 'admin/overview', :action => 'index'
      map.resources :assets, :path_prefix => '/admin', :controller => 'admin/assets', :member => { :add_bucket => :post },
-        :collection => { :latest => :post, :search => :post, :upload => :post, :clear_bucket => :post }
+        :collection => { :latest => [:get, :post], :search => [:get, :post], :upload => :post, :clear_bucket => :post }
      
      # Where oh where is my xmlrpc code?
      # map.connect 'xmlrpc', :controller => 'backend', :action => 'xmlrpc' 
diff --git a/public/javascripts/mephisto/application.js b/public/javascripts/mephisto/application.js
@@ -157,7 +157,7 @@ TinyTab.callbacks ={
  'search-files': function(q) {
    if(!q) return;
    $('spinner').show();
-    new Ajax.Request(Mephisto.root + '/admin/assets/search', {parameters: 'q=' + escape(q)});
+    new Ajax.Request(Mephisto.root + '/admin/assets/search', {method: 'get', parameters: 'q=' + escape(q)});
  }
 };

@@ -300,23 +300,27 @@ var ArticleForm = {
    var articleId = location.href.match(/\/([0-9]+)\/(edit|upload)/)[1];
    var attached  = $('attached-widget-' + assetId);
    if(attached) return;
-    new Ajax.Request('/admin/articles/attach/' + articleId + '/' + assetId);
+    new Ajax.Request('/admin/articles/attach/' + articleId + '/' + assetId,
+                     { parameters: 'authenticity_token=' + Mephisto.token });
    $$('.widget').each(function(asset) { if(assetId == asset.getAttribute('id').match(/-(\d+)$/)[1]) asset.addClassName('selected-widget'); });
  },

  labelAsset: function(assetId) {
    var articleId = location.href.match(/\/([0-9]+)\/(edit|upload)/)[1];
    var attached  = $('attached-widget-' + assetId);
    var label     = $('attached-widget-version-' + assetId);
-    new Ajax.Request('/admin/articles/label/' + articleId + '/' + assetId + '?label=' + escape(label.value));
+    new Ajax.Request('/admin/articles/label/' + articleId + '/' + assetId,
+                     { parameters: 'authenticity_token=' + Mephisto.token +
+                         '&label=' + escape(label.value) });
    if(attached) return;
  },

  detachAsset: function(assetId) {
    var articleId = location.href.match(/\/([0-9]+)\/(edit|upload)/)[1];
    var attached  = $('attached-widget-' + assetId);
    if(!attached) return;
-    new Ajax.Request('/admin/articles/detach/' + articleId + '/' + assetId);
+    new Ajax.Request('/admin/articles/detach/' + articleId + '/' + assetId,
+                     { parameters: 'authenticity_token=' + Mephisto.token });
    new Effect.DropOut(attached, {afterFinish: function() { attached.remove(); }});
    $$('.widget').each(function(asset) { if(assetId == asset.getAttribute('id').match(/-(\d+)$/)[1]) asset.removeClassName('selected-widget'); });
  },
diff --git a/spec/controllers/admin/articles_controller_spec.rb b/spec/controllers/admin/articles_controller_spec.rb
@@ -0,0 +1,15 @@
+require File.dirname(__FILE__) + '/../../spec_helper'
+
+describe Admin::ArticlesController do
+  controller_name "admin/articles"
+  integrate_views
+
+  it "should route /admin/articles/attach and friends correctly" do
+    params = { :controller => "admin/articles", :action => "attach",
+               :id => '1', :version => "2" }
+    params_from(:post, "/admin/articles/attach/1/2").should == params
+    params = { :controller => "admin/articles", :action => "detach",
+               :id => '1', :version => "2" }
+    params_from(:post, "/admin/articles/detach/1/2").should == params
+  end
+end