diff options
22 files changed, 196 insertions, 65 deletions
diff --git a/.gitignore b/.gitignore index 8fabc641a..e8317395f 100644 --- a/.gitignore +++ b/.gitignore @@ -14,3 +14,4 @@ moo.txt TAGS /vendor/plugins/*theme /locale/model_attributes.rb +/files/
\ No newline at end of file diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 405327952..5f18be2e5 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -14,10 +14,32 @@ class ApplicationController < ActionController::Base layout "default" include FastGettext::Translation # make functions like _, n_, N_ etc available) before_filter :set_gettext_locale - + before_filter :set_vary_header # scrub sensitive parameters from the logs filter_parameter_logging :password + def set_vary_header + response.headers['Vary'] = 'Cookie' + end + + helper_method :anonymous_cache, :short_cache, :medium_cache, :long_cache + def anonymous_cache(time) + if session[:user_id].nil? + expires_in time, :public => true + end + end + + def short_cache + anonymous_cache(60.seconds) + end + + def medium_cache + anonymous_cache(60.minutes) + end + + def long_cache + anonymous_cache(24.hours) + end def set_gettext_locale requested_locale = params[:locale] || session[:locale] || cookies[:locale] || request.env['HTTP_ACCEPT_LANGUAGE'] @@ -46,12 +68,17 @@ class ApplicationController < ActionController::Base # egrep "CONSUME MEMORY: [0-9]{7} KB" production.log around_filter :record_memory def record_memory - File.read("/proc/#{Process.pid}/status").match(/VmRSS:\s+(\d+)/) - rss_before_action = $1.to_i - yield - File.read("/proc/#{Process.pid}/status").match(/VmRSS:\s+(\d+)/) - rss_after_action = $1.to_i - logger.info("PID: #{Process.pid}\tCONSUME MEMORY: #{rss_after_action - rss_before_action} KB\tNow: #{rss_after_action} KB\t#{request.url}") + record_memory = MySociety::Config.get('DEBUG_RECORD_MEMORY', false) + if record_memory + File.read("/proc/#{Process.pid}/status").match(/VmRSS:\s+(\d+)/) + rss_before_action = $1.to_i + yield + File.read("/proc/#{Process.pid}/status").match(/VmRSS:\s+(\d+)/) + rss_after_action = $1.to_i + logger.info("PID: #{Process.pid}\tCONSUME MEMORY: #{rss_after_action - rss_before_action} KB\tNow: #{rss_after_action} KB\t#{request.url}") + else + yield + end end # Set cookie expiry according to "remember me" checkbox, as per "An easier diff --git a/app/controllers/general_controller.rb b/app/controllers/general_controller.rb index 6e5c8c3fd..ffc97237a 100644 --- a/app/controllers/general_controller.rb +++ b/app/controllers/general_controller.rb @@ -20,9 +20,8 @@ class GeneralController < ApplicationController # New, improved front page! def frontpage - + medium_cache behavior_cache do - # get some example searches and public bodies to display # either from config, or based on a (slow!) query if not set body_short_names = MySociety::Config.get('FRONTPAGE_PUBLICBODY_EXAMPLES', '').split(/\s*;\s*/).map{|s| "'%s'" % s.gsub(/'/, "''") }.join(", ") @@ -66,6 +65,7 @@ class GeneralController < ApplicationController # Display WhatDoTheyKnow category from mySociety blog def blog + medium_cache @feed_autodetect = [] feed_url = MySociety::Config.get('BLOG_FEED', '') if not feed_url.empty? @@ -176,6 +176,7 @@ class GeneralController < ApplicationController end def custom_css + long_cache @locale = self.locale_from_params() render(:layout => false, :content_type => 'text/css') end diff --git a/app/controllers/help_controller.rb b/app/controllers/help_controller.rb index ab1ef5c5f..c6d246b4c 100644 --- a/app/controllers/help_controller.rb +++ b/app/controllers/help_controller.rb @@ -10,6 +10,8 @@ class HelpController < ApplicationController # we don't even have a control subroutine for most help pages, just see their templates + before_filter :long_cache + def unhappy @info_request = nil if params[:url_title] diff --git a/app/controllers/public_body_controller.rb b/app/controllers/public_body_controller.rb index c74959b17..05acf4868 100644 --- a/app/controllers/public_body_controller.rb +++ b/app/controllers/public_body_controller.rb @@ -11,6 +11,7 @@ require 'csv' class PublicBodyController < ApplicationController # XXX tidy this up with better error messages, and a more standard infrastructure for the redirect to canonical URL def show + long_cache if MySociety::Format.simplify_url_part(params[:url_name], 'body') != params[:url_name] redirect_to :url_name => MySociety::Format.simplify_url_part(params[:url_name], 'body'), :status => :moved_permanently return @@ -80,6 +81,7 @@ class PublicBodyController < ApplicationController end def list + long_cache # XXX move some of these tag SQL queries into has_tag_string.rb @tag = params[:tag] @locale = self.locale_from_params() diff --git a/app/controllers/request_controller.rb b/app/controllers/request_controller.rb index 81dffaa80..472f18f6e 100644 --- a/app/controllers/request_controller.rb +++ b/app/controllers/request_controller.rb @@ -19,10 +19,11 @@ class RequestController < ApplicationController include RequestControllerCustomStates @@custom_states_loaded = true end - rescue MissingSourceFile + rescue MissingSourceFile, NameError end def show + medium_cache @locale = self.locale_from_params() PublicBody.with_locale(@locale) do @@ -95,6 +96,7 @@ class RequestController < ApplicationController # Extra info about a request, such as event history def details + long_cache @info_request = InfoRequest.find_by_url_title(params[:url_title]) if !@info_request.user_can_view?(authenticated_user) render :template => 'request/hidden', :status => 410 # gone @@ -106,6 +108,7 @@ class RequestController < ApplicationController # Requests similar to this one def similar + short_cache @per_page = 25 @page = (params[:page] || "1").to_i @info_request = InfoRequest.find_by_url_title(params[:url_title]) @@ -124,6 +127,7 @@ class RequestController < ApplicationController end def list + medium_cache @view = params[:view] if @view.nil? diff --git a/app/controllers/track_controller.rb b/app/controllers/track_controller.rb index 10b3418bd..e06701a5f 100644 --- a/app/controllers/track_controller.rb +++ b/app/controllers/track_controller.rb @@ -11,6 +11,8 @@ class TrackController < ApplicationController protect_from_forgery # See ActionController::RequestForgeryProtection for details + before_filter :medium_cache + # Track all updates to a particular request def track_request @info_request = InfoRequest.find_by_url_title(params[:url_title]) diff --git a/app/controllers/user_controller.rb b/app/controllers/user_controller.rb index 2e3f6c9e0..d3c42c7f1 100644 --- a/app/controllers/user_controller.rb +++ b/app/controllers/user_controller.rb @@ -16,6 +16,7 @@ class UserController < ApplicationController # Show page about a user def show + long_cache if MySociety::Format.simplify_url_part(params[:url_name], 'user', 32) != params[:url_name] redirect_to :url_name => MySociety::Format.simplify_url_part(params[:url_name], 'user', 32), :status => :moved_permanently return diff --git a/app/models/info_request.rb b/app/models/info_request.rb index 9182b6ae7..209954b16 100644 --- a/app/models/info_request.rb +++ b/app/models/info_request.rb @@ -112,7 +112,7 @@ class InfoRequest < ActiveRecord::Base include InfoRequestCustomStates @@custom_states_loaded = true end - rescue MissingSourceFile + rescue MissingSourceFile, NameError end # only check on create, so existing models with mixed case are allowed @@ -347,14 +347,7 @@ public # XXX this *should* also check outgoing message joined to is an initial # request (rather than follow up) def InfoRequest.find_by_existing_request(title, public_body_id, body) - # XXX can add other databases here which have regexp_replace - if ActiveRecord::Base.connection.adapter_name == "PostgreSQL" - # Exclude spaces from the body comparison using regexp_replace - return InfoRequest.find(:first, :conditions => [ "title = ? and public_body_id = ? and regexp_replace(outgoing_messages.body, '[[:space:]]', '', 'g') = regexp_replace(?, '[[:space:]]', '', 'g')", title, public_body_id, body ], :include => [ :outgoing_messages ] ) - else - # For other databases (e.g. SQLite) not the end of the world being space-sensitive for this check - return InfoRequest.find(:first, :conditions => [ "title = ? and public_body_id = ? and outgoing_messages.body = ?", title, public_body_id, body ], :include => [ :outgoing_messages ] ) - end + return InfoRequest.find(:first, :conditions => [ "title = ? and public_body_id = ? and outgoing_messages.body = ?", title, public_body_id, body ], :include => [ :outgoing_messages ] ) end def find_existing_outgoing_message(body) diff --git a/app/models/raw_email.rb b/app/models/raw_email.rb index 7a57399d5..4e1a69456 100644 --- a/app/models/raw_email.rb +++ b/app/models/raw_email.rb @@ -6,7 +6,6 @@ # id :integer not null, primary key # data_text :text # data_binary :binary -# - prepared to 277k. # models/raw_email.rb: # The fat part of models/incoming_message.rb diff --git a/app/models/track_mailer.rb b/app/models/track_mailer.rb index 1b37709b9..6901a834d 100644 --- a/app/models/track_mailer.rb +++ b/app/models/track_mailer.rb @@ -26,7 +26,11 @@ class TrackMailer < ApplicationMailer @body = { :user => user, :email_about_things => email_about_things, :unsubscribe_url => unsubscribe_url } end - # Send email alerts for tracked things. + # Send email alerts for tracked things. Never more than one email + # a day, nor about events which are more than a week old, nor + # events about which emails have been sent within the last two + # weeks. + # Useful query to run by hand to see how many alerts are due: # User.find(:all, :conditions => [ "last_daily_track_email < ?", Time.now - 1.day ]).size def self.alert_tracks diff --git a/config/environment.rb b/config/environment.rb index ec6a4096f..a40c2df4e 100644 --- a/config/environment.rb +++ b/config/environment.rb @@ -61,15 +61,6 @@ Rails::Initializer.run do |config| config.gem 'routing-filter' config.gem 'will_paginate', :version => '~> 2.3.11', :source => 'http://gemcutter.org' #GettextI18nRails.translations_are_html_safe = true - # Your secret key for verifying cookie session data integrity. - # If you change this key, all old sessions will become invalid! - # Make sure the secret is at least 30 characters and all random, - # no regular words or you'll be exposed to dictionary attacks. - config.action_controller.session = { - :key => '_wdtk_cookie_session', - :secret => MySociety::Config.get("COOKIE_STORE_SESSION_SECRET", 'this default is insecure as code is open source, please override for live sites in config/general; this will do for local development') - } - config.action_controller.session_store = :cookie_store # Use SQL instead of Active Record's schema dumper when creating the test database. # This is necessary if your schema can't be completely dumped by the schema dumper, diff --git a/config/general.yml-example b/config/general.yml-example index 60a527302..fb2afd336 100644 --- a/config/general.yml-example +++ b/config/general.yml-example @@ -87,3 +87,9 @@ STAGING_SITE: 1 RECAPTCHA_PUBLIC_KEY: 'x' RECAPTCHA_PRIVATE_KEY: 'x' +# For debugging memory problems. If true, the app logs +# the memory use increase of the Ruby process due to the +# request (Linux only). Since Ruby never returns memory to the OS, if the +# existing process previously served a larger request, this won't +# show any consumption for the later request. +DEBUG_RECORD_MEMORY: false
\ No newline at end of file diff --git a/config/initializers/session_store.rb b/config/initializers/session_store.rb new file mode 100644 index 000000000..9ef2dddc1 --- /dev/null +++ b/config/initializers/session_store.rb @@ -0,0 +1,17 @@ +# Be sure to restart your server when you modify this file. + +# Your secret key for verifying cookie session data integrity. +# If you change this key, all old sessions will become invalid! +# Make sure the secret is at least 30 characters and all random, +# no regular words or you'll be exposed to dictionary attacks. + +ActionController::Base.session = { + :key => '_wdtk_cookie_session', + :secret => MySociety::Config.get("COOKIE_STORE_SESSION_SECRET", 'this default is insecure as code is open source, please override for live sites in config/general; this will do for local development') +} +ActionController::Base.session_store = :cookie_store + +# Insert a bit of middleware code to prevent uneeded cookie setting. +require "#{RAILS_ROOT}/lib/whatdotheyknow/strip_empty_sessions" +ActionController::Dispatcher.middleware.insert_before ActionController::Base.session_store, WhatDoTheyKnow::StripEmptySessions, :key => '_wdtk_cookie_session', :path => "/", :httponly => true + diff --git a/db/migrate/099_move_raw_email_to_filesystem.rb b/db/migrate/099_move_raw_email_to_filesystem.rb index 991ef55d7..ea77580e1 100644 --- a/db/migrate/099_move_raw_email_to_filesystem.rb +++ b/db/migrate/099_move_raw_email_to_filesystem.rb @@ -6,15 +6,15 @@ class MoveRawEmailToFilesystem < ActiveRecord::Migration if !File.exists?(raw_email.filepath) STDERR.puts "converting raw_email " + raw_email.id.to_s raw_email.data = raw_email.dbdata - raw_email.dbdata = nil - raw_email.save! + #raw_email.dbdata = nil + #raw_email.save! end end end end def self.down - #raise "safer not to have reverse migration scripts, and we never use them" + raise "safer not to have reverse migration scripts, and we never use them" end end diff --git a/db/migrate/100_remove_redundant_raw_email_columns.rb b/db/migrate/100_remove_redundant_raw_email_columns.rb new file mode 100644 index 000000000..2ebffba34 --- /dev/null +++ b/db/migrate/100_remove_redundant_raw_email_columns.rb @@ -0,0 +1,12 @@ +class RemoveRedundantRawEmailColumns < ActiveRecord::Migration + def self.up + remove_column :raw_emails, :data_text + end + def self.down + add_column :raw_emails, :data_text, :text + end +end + + + + diff --git a/lib/whatdotheyknow/strip_empty_sessions.rb b/lib/whatdotheyknow/strip_empty_sessions.rb new file mode 100644 index 000000000..9c87a4bbc --- /dev/null +++ b/lib/whatdotheyknow/strip_empty_sessions.rb @@ -0,0 +1,29 @@ +module WhatDoTheyKnow + + class StripEmptySessions + ENV_SESSION_KEY = "rack.session".freeze + HTTP_SET_COOKIE = "Set-Cookie".freeze + STRIPPABLE_KEYS = [:session_id, :_csrf_token] + + def initialize(app, options = {}) + @app = app + @options = options + end + + def call(env) + status, headers, body = @app.call(env) + session_data = env[ENV_SESSION_KEY] + set_cookie = headers[HTTP_SET_COOKIE] + if session_data + if (session_data.keys - STRIPPABLE_KEYS).empty? + if set_cookie.is_a? Array + set_cookie.reject! {|c| c.match(/^\n?#{@options[:key]}=/)} + elsif set_cookie.is_a? String + headers[HTTP_SET_COOKIE].gsub!( /(^|\n)#{@options[:key]}=.*?(\n|$)/, "" ) + end + end + end + [status, headers, body] + end + end +end diff --git a/script/mailin b/script/mailin index 738034c26..733eaff3d 100755 --- a/script/mailin +++ b/script/mailin @@ -22,7 +22,7 @@ then # send error to administators (use mutt for MIME encoding) SUBJ="Mail import error for $OPTION_DOMAIN" BODY="There was an error code $ERROR_CODE returned by the RequestMailer.receive command in script/mailin. See attached for details. This might be quite serious, such as the database was down, or might be an email with corrupt headers that Rails is choking on. The email was returned with an exit code 75, which for Exim at least means the MTA will try again later. A well configured installation of this code will separately have had Exim make a backup copy of the email in a separate mailbox, just in case." - echo "$BODY" | /usr/bin/mutt -s "$SUBJ" -a $OUTPUT -a $INPUT $OPTION_CONTACT_EMAIL + echo "$BODY" | /usr/bin/mutt -s "$SUBJ" -a $OUTPUT $INPUT -- $OPTION_CONTACT_EMAIL # tell exim error was temporary, so try again later (no point bouncing message to authority) rm -f $OUTPUT diff --git a/spec/controllers/request_controller_spec.rb b/spec/controllers/request_controller_spec.rb index 8232a7398..9d91bf8c2 100644 --- a/spec/controllers/request_controller_spec.rb +++ b/spec/controllers/request_controller_spec.rb @@ -376,20 +376,6 @@ describe RequestController, "when creating a new request" do response.should render_template('new') end - it "should give an error if the same request is submitted twice with extra whitespace in the body" do - # This only works for PostgreSQL databases which have regexp_replace - - # see model method InfoRequest.find_by_existing_request for more info - if ActiveRecord::Base.connection.adapter_name == "PostgreSQL" - session[:user_id] = @user.id - - post :new, :info_request => { :public_body_id => info_requests(:fancy_dog_request).public_body_id, - :title => info_requests(:fancy_dog_request).title }, - :outgoing_message => { :body => "\n" + info_requests(:fancy_dog_request).outgoing_messages[0].body + " "}, - :submitted_new_request => 1, :preview => 0, :mouse_house => 1 - response.should render_template('new') - end - end - it "should let you submit another request with the same title" do session[:user_id] = @user.id diff --git a/spec/lib/whatdotheyknow/strip_empty_sessions_spec.rb b/spec/lib/whatdotheyknow/strip_empty_sessions_spec.rb new file mode 100644 index 000000000..cbe1feea6 --- /dev/null +++ b/spec/lib/whatdotheyknow/strip_empty_sessions_spec.rb @@ -0,0 +1,55 @@ +require 'spec_helper' +describe WhatDoTheyKnow::StripEmptySessions do + + def make_response(session_data, response_headers) + app = lambda do |env| + env['rack.session'] = session_data + return [200, response_headers, ['content']] + end + strip_empty_sessions = WhatDoTheyKnow::StripEmptySessions + app = strip_empty_sessions.new(app, {:key => 'mykey', :path => '', :httponly => true}) + response = Rack::MockRequest.new(app).get('/', 'HTTP_ACCEPT' => 'text/html') + end + + + it 'should not prevent a cookie being set if there is data in the session' do + session_data = { :some_real_data => 'important', + :session_id => 'my_session_id', + :_csrf_token => 'hi_there' } + application_response_headers = { 'Content-Type' => 'text/html', + 'Set-Cookie' => 'mykey=f274c61a35320c52d45e9f8d7d4e2649; path=/; HttpOnly'} + response = make_response(session_data, application_response_headers) + response.headers['Set-Cookie'].should == 'mykey=f274c61a35320c52d45e9f8d7d4e2649; path=/; HttpOnly' + end + + describe 'if there is no meaningful data in the session' do + + before do + @session_data = { :session_id => 'my_session_id', + :_csrf_token => 'hi_there' } + end + + it 'should not strip any other header' do + application_response_headers = { 'Content-Type' => 'text/html', + 'Set-Cookie' => 'mykey=f274c61a35320c52d45e9f8d7d4e2649; path=/; HttpOnly'} + response = make_response(@session_data, application_response_headers) + response.headers['Content-Type'].should == 'text/html' + end + + it 'should strip the session cookie setting header ' do + application_response_headers = { 'Content-Type' => 'text/html', + 'Set-Cookie' => 'mykey=f274c61a35320c52d45e9f8d7d4e2649; path=/; HttpOnly'} + response = make_response(@session_data, application_response_headers) + response.headers['Set-Cookie'].should == "" + end + + it 'should strip the session cookie setting header (but no other cookie setting header) if there is more than one' do + application_response_headers = { 'Content-Type' => 'text/html', + 'Set-Cookie' => ['mykey=f274c61a35320c52d45e9f8d7d4e2649; path=/; HttpOnly', + 'other=mydata']} + response = make_response(@session_data, application_response_headers) + response.headers['Set-Cookie'].should == ['other=mydata'] + end + + end +end diff --git a/spec/models/outgoing_mailer_spec.rb b/spec/models/outgoing_mailer_spec.rb index 83da7a553..24143fc9b 100644 --- a/spec/models/outgoing_mailer_spec.rb +++ b/spec/models/outgoing_mailer_spec.rb @@ -6,23 +6,6 @@ describe OutgoingMailer, " when working out follow up addresses" do # mocks. Put parts of the tests in spec/lib/tmail_extensions.rb fixtures :info_requests, :incoming_messages, :raw_emails, :public_bodies, :public_body_translations - before do - # XXX this is a hack around the fact that our raw_email model - # is in transition to something that doesn't actually live in - # the database at all. The raw_email fixture saves to the - # model, the model then needs to be told to save itself on the - # filesystem. - raw_email = raw_emails(:useless_raw_email) - raw_email.data=raw_email.dbdata - end - - after do - # And this is a hack around the fact that Rails fixtures don't - # have teardowns happen on them; we need to ensure no emails - # are left lying around - raw_emails(:useless_raw_email).destroy_file_representation! - end - it "should parse them right" do ir = info_requests(:fancy_dog_request) im = ir.incoming_messages[0] diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index bbcc9aa23..086def32a 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -30,6 +30,22 @@ Spec::Runner.configure do |config| # in your config/boot.rb config.fixture_path = RAILS_ROOT + '/spec/fixtures/' + + config.before(:each) do + # XXX this is a hack around the fact that our raw_email model + # is in transition to something that doesn't actually live in + # the database at all. The raw_email *fixture* saves to the + # model, the model then needs to be told to save itself on the + # filesystem. + begin + raw_email = raw_emails(:useless_raw_email) + raw_email.data=raw_email.dbdata + rescue NoMethodError + # only do it in tests with raw_emails fixtures + end + end + + # == Fixtures # # You can declare fixtures for each example_group like this: |