diff --git a/app/controllers/account_controller.rb b/app/controllers/account_controller.rb index e60913e1..0e830395 100644 --- a/app/controllers/account_controller.rb +++ b/app/controllers/account_controller.rb @@ -43,7 +43,10 @@ def forget def activate self.current_user = site.user_by_token(params[:id]) if logged_in? - current_user.reset_token! + # TODO - See security comments on AuthenticatedSystem#login_from_cookie. + ActiveRecord::Base.with_writable_records do + current_user.reset_token! + end else flash[:error] = "Invalid token. Try resending your forgotten password request." end diff --git a/app/controllers/admin/assets_controller.rb b/app/controllers/admin/assets_controller.rb index 947a339d..96b0f236 100644 --- a/app/controllers/admin/assets_controller.rb +++ b/app/controllers/admin/assets_controller.rb @@ -3,6 +3,7 @@ class Admin::AssetsController < Admin::BaseController skip_before_filter :login_required before_filter :find_asset, :except => [:index, :new, :create, :latest, :search, :upload, :clear_bucket] before_filter :login_required + before_filter :protect_action, :except => [:index, :new, :latest, :search] def index search_assets 24 diff --git a/app/controllers/application.rb b/app/controllers/application.rb index 8bf6868f..dd4f8f8f 100644 --- a/app/controllers/application.rb +++ b/app/controllers/application.rb @@ -3,6 +3,7 @@ class ApplicationController < ActionController::Base include Mephisto::CachingMethods before_filter :set_cache_root + around_filter :get_requests_are_readonly helper_method :site attr_reader :site @@ -95,6 +96,21 @@ def set_cache_root end end + # Much of a web browser's built-in protection against CSRF attacks + # assumes that all GET requests are "safe". Since we don't want to + # rely on getting this 100% right in every controller and plugin, let's + # just enforce this policy globally. You can override this using + # skip_filter :get_requests_are_readonly. + def get_requests_are_readonly + if request.method == :get + ActiveRecord::Base.with_readonly_records do + yield + end + else + yield + end + end + def with_site_timezone old_tz = ENV['TZ'] ENV['TZ'] = site.timezone.name diff --git a/config/initializers/readonly_records.rb b/config/initializers/readonly_records.rb new file mode 100644 index 00000000..d0ff6af2 --- /dev/null +++ b/config/initializers/readonly_records.rb @@ -0,0 +1,35 @@ +module ActiveRecord + class Base + # Are ActiveRecord::Base objects currently readonly? + def self.all_records_are_readonly? + Thread.current[:all_records_are_readonly] + end + + def readonly_with_global_flag? + self.class.all_records_are_readonly? || readonly_without_global_flag? + end + alias_method_chain :readonly?, :global_flag + + # Make all ActiveRecord::Base objects readonly within a block. + def self.with_readonly_records # :yield: + saved = all_records_are_readonly? + begin + Thread.current[:all_records_are_readonly] = true + yield + ensure + Thread.current[:all_records_are_readonly] = saved + end + end + + # Make all ActiveRecord::Base objects writable within a block. + def self.with_writable_records # :yield: + saved = all_records_are_readonly? + begin + Thread.current[:all_records_are_readonly] = false + yield + ensure + Thread.current[:all_records_are_readonly] = saved + end + end + end +end diff --git a/lib/authenticated_system.rb b/lib/authenticated_system.rb index 9ddcb867..cfed36f5 100644 --- a/lib/authenticated_system.rb +++ b/lib/authenticated_system.rb @@ -74,7 +74,17 @@ def self.included(base) def login_from_cookie return unless cookies[:token] && !logged_in? self.current_user = site.user_by_token(cookies[:token]) - cookies[:token] = { :value => self.current_user.reset_token! , :expires => self.current_user.token_expires_at } if logged_in? + # TODO - We allow the token to be changed on GET requests and we log + # the user in. I haven't fully analyzed the consequences of allowing + # session and token updates on hostile GET requests triggered by CSRF + # attacks. If this helps out in some kind of attack, it would affect + # almost every single web application in existence. + ActiveRecord::Base.with_writable_records do + cookies[:token] = { + :value => self.current_user.reset_token!, + :expires => self.current_user.token_expires_at + } if logged_in? + end true end diff --git a/spec/models/readonly_spec.rb b/spec/models/readonly_spec.rb new file mode 100644 index 00000000..b0e9c73f --- /dev/null +++ b/spec/models/readonly_spec.rb @@ -0,0 +1,35 @@ +require File.dirname(__FILE__) + '/../spec_helper' + +# Verify that our readonly_record patches are working. +describe "Any record" do + before :each do + @article = Article.make(:title => "Original title") + end + + it "should be writable by default" do + assert !ActiveRecord::Base.all_records_are_readonly? + @article.title = "Hello!" + @article.save! + end + + it "should not be writable inside with_readonly_records" do + assert_raise ActiveRecord::ReadOnlyRecord do + ActiveRecord::Base.with_readonly_records do + assert ActiveRecord::Base.all_records_are_readonly? + @article.title = "Hello!" + @article.save! + end + end + end + + it "should be writable inside with_writable_records" do + ActiveRecord::Base.with_readonly_records do + ActiveRecord::Base.with_writable_records do + assert !ActiveRecord::Base.all_records_are_readonly? + @article.title = "Hello!" + @article.save! + end + end + end +end +