Merge branch 'brakeman_fixes' into rails-3-develop

12 files changed, 89 insertions, 28 deletions

diff --git a/app/controllers/admin_controller.rb b/app/controllers/admin_controller.rb
index 3bf40b8f9..7760c372b 100644
--- a/app/controllers/admin_controller.rb
+++ b/app/controllers/admin_controller.rb
@@ -9,7 +9,6 @@ require 'fileutils'
 class AdminController < ApplicationController
     layout "admin"
     before_filter :authenticate
-    protect_from_forgery # See ActionController::RequestForgeryProtection for details
 
     # action to take if expecting an authenticity token and one isn't received
     def handle_unverified_request
diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index a06fa7098..dbd879a1c 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -14,6 +14,8 @@ class ApplicationController < ActionController::Base
     end
     class RouteNotFound < StandardError
     end
+    protect_from_forgery
+
     # assign our own handler method for non-local exceptions
     rescue_from Exception, :with => :render_exception
 
diff --git a/app/controllers/comment_controller.rb b/app/controllers/comment_controller.rb
index 2c0037577..890e9faaa 100644
--- a/app/controllers/comment_controller.rb
+++ b/app/controllers/comment_controller.rb
@@ -10,7 +10,6 @@ class CommentController < ApplicationController
     before_filter :create_track_thing, :only => [ :new ]
     before_filter :reject_unless_comments_allowed, :only => [ :new ]
     before_filter :reject_if_user_banned, :only => [ :new ]
-    protect_from_forgery :only => [ :new ]
 
     def new
         if params[:comment]
diff --git a/app/controllers/request_controller.rb b/app/controllers/request_controller.rb
index d529f8dbb..413b74cea 100644
--- a/app/controllers/request_controller.rb
+++ b/app/controllers/request_controller.rb
@@ -10,7 +10,6 @@ require 'open-uri'
 
 class RequestController < ApplicationController
     before_filter :check_read_only, :only => [ :new, :show_response, :describe_state, :upload_response ]
-    protect_from_forgery :only => [ :new, :show_response, :describe_state, :upload_response ] # See ActionController::RequestForgeryProtection for details
     before_filter :check_batch_requests_and_user_allowed, :only => [ :select_authorities, :new_batch ]
     MAX_RESULTS = 500
     PER_PAGE = 25
@@ -841,7 +840,15 @@ class RequestController < ApplicationController
         end
 
         # check filename in URL matches that in database (use a censor rule if you want to change a filename)
-        raise ActiveRecord::RecordNotFound.new("please use same filename as original file has, display: '" + @attachment.display_filename + "' old_display: '" + @attachment.old_display_filename + "' original: '" + @original_filename + "'") if @attachment.display_filename != @original_filename && @attachment.old_display_filename != @original_filename
+        if @attachment.display_filename != @original_filename && @attachment.old_display_filename != @original_filename
+            msg = 'please use same filename as original file has, display: '
+            msg += "'#{ @attachment.display_filename }' "
+            msg += 'old_display: '
+            msg += "'#{ @attachment.old_display_filename }' "
+            msg += 'original: '
+            msg += "'#{ @original_filename }'"
+            raise ActiveRecord::RecordNotFound.new(msg)
+        end
 
         @attachment_url = get_attachment_url(:id => @incoming_message.info_request_id,
                 :incoming_message_id => @incoming_message.id, :part => @part_number,
diff --git a/app/controllers/track_controller.rb b/app/controllers/track_controller.rb
index 83700a55b..7018af03c 100644
--- a/app/controllers/track_controller.rb
+++ b/app/controllers/track_controller.rb
@@ -6,9 +6,6 @@
 # Email: hello@mysociety.org; WWW: http://www.mysociety.org/
 
 class TrackController < ApplicationController
-
-    protect_from_forgery # See ActionController::RequestForgeryProtection for details
-
     before_filter :medium_cache
 
     # Track all updates to a particular request
diff --git a/app/controllers/user_controller.rb b/app/controllers/user_controller.rb
index 9798ff8e2..b7c8252f5 100644
--- a/app/controllers/user_controller.rb
+++ b/app/controllers/user_controller.rb
@@ -7,15 +7,8 @@
 require 'set'
 
 class UserController < ApplicationController
-
     layout :select_layout
 
-    protect_from_forgery :only => [ :contact,
-                                    :set_profile_photo,
-                                    :signchangeemail,
-                                    :clear_profile_photo,
-                                    :set_profile_about_me ] # See ActionController::RequestForgeryProtection for details
-
     # Show page about a user
     def show
         long_cache
diff --git a/app/models/info_request.rb b/app/models/info_request.rb
index 20b7ef9af..2b60e13d8 100644
--- a/app/models/info_request.rb
+++ b/app/models/info_request.rb
@@ -1366,9 +1366,9 @@ public
     end
 
     def InfoRequest.find_in_state(state)
-        find(:all, :select => '*, ' + last_event_time_clause + ' as last_event_time',
-                   :conditions => ["described_state = ?", state],
-                   :order => "last_event_time")
+        select("*, #{ last_event_time_clause } as last_event_time").
+            where(:described_state => state).
+                order('last_event_time')
     end
 
     private
diff --git a/app/models/info_request_event.rb b/app/models/info_request_event.rb
index 9dde3ba80..635ba8f58 100644
--- a/app/models/info_request_event.rb
+++ b/app/models/info_request_event.rb
@@ -161,11 +161,10 @@ class InfoRequestEvent < ActiveRecord::Base
     end
 
     def incoming_message_selective_columns(fields)
-        message = IncomingMessage.find(:all,
-                                       :select => fields + ", incoming_messages.info_request_id",
-                                       :joins => "INNER JOIN info_request_events ON incoming_messages.id = incoming_message_id ",
-                                       :conditions => "info_request_events.id = #{self.id}"
-                                       )
+        message = IncomingMessage.select("#{ fields }, incoming_messages.info_request_id").
+            joins('INNER JOIN info_request_events ON incoming_messages.id = incoming_message_id').
+                where('info_request_events.id = ?', id)
+
         message = message[0]
         if !message.nil?
             message.info_request = InfoRequest.find(message.info_request_id)
diff --git a/app/models/user.rb b/app/models/user.rb
index 1c6dc0eb0..c953e52f2 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -264,11 +264,9 @@ class User < ActiveRecord::Base
     # Returns list of requests which the user hasn't described (and last
     # changed more than a day ago)
     def get_undescribed_requests
-        info_requests.find(
-            :all,
-            :conditions => [ 'awaiting_description = ? and ' + InfoRequest.last_event_time_clause + ' < ?',
-                true, Time.now() - 1.day
-            ]
+        info_requests.where(
+            "awaiting_description = ? and #{ InfoRequest.last_event_time_clause } < ?",
+            true, 1.day.ago
         )
     end
 
diff --git a/app/views/user/_signin.html.erb b/app/views/user/_signin.html.erb
index 864951733..e86791aaf 100644
--- a/app/views/user/_signin.html.erb
+++ b/app/views/user/_signin.html.erb
@@ -18,7 +18,7 @@
     </p>
 
     <p class="form_note">
-    <%= link_to _('Forgotten your password?'), signchangepassword_path + "?pretoken=" + h(params[:token]), :tabindex => 30 %>
+    <%= link_to _('Forgotten your password?'), signchangepassword_path(:pretoken => h(params[:token])), :tabindex => 30 %>
     </p>
 
     <p class="form_checkbox">
diff --git a/config/brakeman.ignore b/config/brakeman.ignore
new file mode 100644
index 000000000..391013a5a
--- /dev/null
+++ b/config/brakeman.ignore
@@ -0,0 +1,63 @@
+{
+  "ignored_warnings": [
+    {
+      "location": {
+        "type": "method",
+        "method": "list_all_csv",
+        "class": "PublicBodyController"
+      },
+      "file": "app/controllers/public_body_controller.rb",
+      "warning_code": 16,
+      "render_path": null,
+      "link": "http://brakemanscanner.org/docs/warning_types/file_access/",
+      "warning_type": "File Access",
+      "code": "File.open(Tempfile.new(\"all-authorities.csv\", File.join(InfoRequest.download_zip_dir, \"download\")).path, \"w\")",
+      "line": 211,
+      "confidence": "Weak",
+      "user_input": "InfoRequest.download_zip_dir",
+      "message": "Model attribute used in file name",
+      "fingerprint": "00ce9cdd1d2c3f220bae94cb854393b5072ee1da064ca7a3af693fe2867d51c8",
+      "note": "InfoRequest.download_zip_dir does not contain user input"
+    },
+    {
+      "location": {
+        "type": "method",
+        "method": "list_all_csv",
+        "class": "PublicBodyController"
+      },
+      "file": "app/controllers/public_body_controller.rb",
+      "warning_code": 16,
+      "render_path": null,
+      "link": "http://brakemanscanner.org/docs/warning_types/file_access/",
+      "warning_type": "File Access",
+      "code": "File.rename(Tempfile.new(\"all-authorities.csv\", File.join(InfoRequest.download_zip_dir, \"download\")).path, File.join(File.join(InfoRequest.download_zip_dir, \"download\"), \"all-authorities.csv\"))",
+      "line": 213,
+      "confidence": "Weak",
+      "user_input": "InfoRequest.download_zip_dir",
+      "message": "Model attribute used in file name",
+      "fingerprint": "6078628aa47451d597e211629d80dcea0fdc7600dc066cabf2c0a4b9e07a75cc",
+      "note": "InfoRequest.download_zip_dir does not contain user input"
+    },
+    {
+      "location": {
+        "type": "method",
+        "method": "list_all_csv",
+        "class": "PublicBodyController"
+      },
+      "file": "app/controllers/public_body_controller.rb",
+      "warning_code": 16,
+      "render_path": null,
+      "link": "http://brakemanscanner.org/docs/warning_types/file_access/",
+      "warning_type": "File Access",
+      "code": "FileUtils.mkdir_p(File.join(InfoRequest.download_zip_dir, \"download\"))",
+      "line": 194,
+      "confidence": "Weak",
+      "user_input": "InfoRequest.download_zip_dir",
+      "message": "Model attribute used in file name",
+      "fingerprint": "5ed20f867c17c814cfe117906161a26f37b986d694996c9fd0089d4f971dc1d0",
+      "note": "InfoRequest.download_zip_dir does not contain user input"
+    }
+  ],
+  "updated": "Thu Oct 02 10:43:19 +0000 2014",
+  "brakeman_version": "2.6.2"
+}
diff --git a/config/brakeman.yml b/config/brakeman.yml
new file mode 100644
index 000000000..1f95903fd
--- /dev/null
+++ b/config/brakeman.yml
@@ -0,0 +1,4 @@
+--- 
+:output_files: 
+- tmp/brakeman.html
+- tmp/brakeman.json