Use GET for search forms, not POST.

Now that we use global CSRF authenticity checks, searches were logging
logged-in users out as the form is an HTML form, not a Rails-generated
form with a CSRF token. So form submission raised an InvalidAuthenticityToken
error and reset their session.  We could generate the form in Rails, but we
also want to minimise the number of non-logged in people who have a
session cookie, so that varnish can cache pages extensively. So we don't
want to put the CSRF token for the search form in everyone's session.

5 files changed, 32 insertions, 11 deletions

diff --git a/app/views/general/_frontpage_search_box.html.erb b/app/views/general/_frontpage_search_box.html.erb
index f77bd97fc..a58d837f8 100644
--- a/app/views/general/_frontpage_search_box.html.erb
+++ b/app/views/general/_frontpage_search_box.html.erb
@@ -5,7 +5,7 @@
   :number_of_requests => number_with_delimiter(InfoRequest.visible.count),
   :number_of_authorities => number_with_delimiter(PublicBody.visible.count)) %>
 </h2>
-<form id="search_form" method="post" action="<%= search_redirect_path %>">
+<form id="search_form" method="get" action="<%= search_redirect_path %>">
   <div>
     <input id="query" type="text" size="30" name="query" title="type your search term here" >
     <input type="submit" value="<%= _('Search') %>">
diff --git a/app/views/general/_header.html.erb b/app/views/general/_header.html.erb
index 55bf719e2..f465668a5 100644
--- a/app/views/general/_header.html.erb
+++ b/app/views/general/_header.html.erb
@@ -22,7 +22,7 @@
     <% end %>
 
     <div id="navigation_search">
-      <form id="navigation_search_form" method="post" action="<%= search_redirect_path %>">
+      <form id="navigation_search_form" method="get" action="<%= search_redirect_path %>">
          <p>
              <%= text_field_tag 'query', params[:query], { :size => 40, :id => "navigation_search_query", :title => "type your search term here" } %>
              <input id="navigation_search_button" type="submit" value="search">
diff --git a/app/views/general/_responsive_topnav.html.erb b/app/views/general/_responsive_topnav.html.erb
index 0ece0da9a..0af6629c8 100644
--- a/app/views/general/_responsive_topnav.html.erb
+++ b/app/views/general/_responsive_topnav.html.erb
@@ -21,7 +21,7 @@
         </li>
 
         <li id="navigation_search">
-          <form id="navigation_search_form" method="post" action="<%= search_redirect_path %>">
+          <form id="navigation_search_form" method="get" action="<%= search_redirect_path %>">
             <label for="navigation_search_button">
               <img src="/assets/search.png" alt="Search:">
             </label>
diff --git a/spec/integration/alaveteli_dsl.rb b/spec/integration/alaveteli_dsl.rb
index 370628d98..d7485a094 100644
--- a/spec/integration/alaveteli_dsl.rb
+++ b/spec/integration/alaveteli_dsl.rb
@@ -74,5 +74,14 @@ def cache_directories_exist?(request)
     paths.any?{ |path| File.exist?(path) }
 end
 
+def with_forgery_protection
+  orig = ActionController::Base.allow_forgery_protection
+  begin
+    ActionController::Base.allow_forgery_protection = true
+    yield if block_given?
+  ensure
+    ActionController::Base.allow_forgery_protection = orig
+  end
+end
 
 
diff --git a/spec/integration/search_request_spec.rb b/spec/integration/search_request_spec.rb
index 23a62e97b..699eb2c6c 100644
--- a/spec/integration/search_request_spec.rb
+++ b/spec/integration/search_request_spec.rb
@@ -1,4 +1,5 @@
 require File.expand_path(File.dirname(__FILE__) + '/../spec_helper')
+require File.expand_path(File.dirname(__FILE__) + '/alaveteli_dsl')
 
 describe "When searching" do
 
@@ -8,24 +9,35 @@ describe "When searching" do
     end
 
     it "should not strip quotes from quoted query" do
-        request_via_redirect("post", "/search", :query => '"mouse stilton"')
+        request_via_redirect("get", "/search", :query => '"mouse stilton"')
         response.body.should include("&quot;mouse stilton&quot;")
     end
 
     it "should redirect requests with search in query string to URL-based page" do
-        post '/search/all?query=bob'
+        get '/search/all?query=bob'
         response.should redirect_to "/en/search/bob/all"
     end
 
     it "should correctly execute simple search" do
-        request_via_redirect("post", "/search",
+        request_via_redirect("get", "/search",
                              :query => 'bob'
                              )
         response.body.should include("FOI requests")
     end
 
+    it "should not log a logged-in user out" do
+      with_forgery_protection do
+        user = FactoryGirl.create(:user)
+        user_session = login(user)
+        user_session.visit frontpage_path
+        user_session.fill_in "query", :with => 'test'
+        user_session.click_button "Search"
+        user_session.response.body.should include(user.name)
+      end
+    end
+
     it "should correctly filter searches for requests" do
-        request_via_redirect("post", "/search/bob/requests")
+        request_via_redirect("get", "/search/bob/requests")
         response.body.should_not include("One person found")
         n = 4 # The number of requests that contain the word "bob" somewhere
               # in the email text. At present this is:
@@ -39,13 +51,13 @@ describe "When searching" do
         response.body.should include("FOI requests 1 to #{n} of #{n}")
     end
     it "should correctly filter searches for users" do
-        request_via_redirect("post", "/search/bob/users")
+        request_via_redirect("get", "/search/bob/users")
         response.body.should include("One person found")
         response.body.should_not include("FOI requests 1 to")
     end
 
     it "should correctly filter searches for successful requests" do
-        request_via_redirect("post", "/search/requests",
+        request_via_redirect("get", "/search/requests",
                              :query => "bob",
                              :latest_status => ['successful'])
         n = 2 # The number of *successful* requests that contain the word "bob" somewhere
@@ -56,12 +68,12 @@ describe "When searching" do
     end
 
     it "should correctly filter searches for comments" do
-        request_via_redirect("post", "/search/requests",
+        request_via_redirect("get", "/search/requests",
                              :query => "daftest",
                              :request_variety => ['comments'])
         response.body.should include("One FOI request found")
 
-        request_via_redirect("post", "/search/requests",
+        request_via_redirect("get", "/search/requests",
                              :query => "daftest",
                              :request_variety => ['response','sent'])
         response.body.should include("no results matching your query")