Only mark email_subject_request as HTML safe when used in email subject.

It's also used in the web interface and needs to be escaped there.

7 files changed, 26 insertions, 20 deletions

diff --git a/app/mailers/outgoing_mailer.rb b/app/mailers/outgoing_mailer.rb
index 797bf9fdd..19054b4e2 100644
--- a/app/mailers/outgoing_mailer.rb
+++ b/app/mailers/outgoing_mailer.rb
@@ -21,7 +21,7 @@ class OutgoingMailer < ApplicationMailer
 
         mail(:from => info_request.incoming_name_and_email,
              :to => info_request.recipient_name_and_email,
-             :subject => info_request.email_subject_request)
+             :subject => info_request.email_subject_request(:html => false))
     end
 
     # Later message to public body regarding existing request
@@ -32,7 +32,7 @@ class OutgoingMailer < ApplicationMailer
 
         mail(:from => info_request.incoming_name_and_email,
              :to => OutgoingMailer.name_and_email_for_followup(info_request, incoming_message_followup),
-             :subject => OutgoingMailer.subject_for_followup(info_request, outgoing_message))
+             :subject => OutgoingMailer.subject_for_followup(info_request, outgoing_message, :html => false))
     end
 
     # TODO: the condition checking valid_to_reply_to? also appears in views/request/_followup.html.erb,
@@ -67,11 +67,12 @@ class OutgoingMailer < ApplicationMailer
         end
     end
     # Subject to use for followup
-    def OutgoingMailer.subject_for_followup(info_request, outgoing_message)
+    def OutgoingMailer.subject_for_followup(info_request, outgoing_message, options = {})
         if outgoing_message.what_doing == 'internal_review'
-            return "Internal review of " + info_request.email_subject_request
+            return "Internal review of " + info_request.email_subject_request(:html => options[:html])
         else
-            return info_request.email_subject_followup(outgoing_message.incoming_message_followup)
+            return info_request.email_subject_followup(:incoming_message => outgoing_message.incoming_message_followup,
+                                                       :html => options[:html])
         end
     end
     # Whether we have a valid email address for a followup
diff --git a/app/mailers/request_mailer.rb b/app/mailers/request_mailer.rb
index 768257ba8..89b76fe97 100644
--- a/app/mailers/request_mailer.rb
+++ b/app/mailers/request_mailer.rb
@@ -20,7 +20,7 @@ class RequestMailer < ApplicationMailer
 
         mail(:from => from_user.name_and_email,
              :to => info_request.incoming_name_and_email,
-             :subject => info_request.email_subject_followup)
+             :subject => info_request.email_subject_followup(:html => false))
     end
 
     # Used when a response is uploaded using the API
diff --git a/app/models/info_request.rb b/app/models/info_request.rb
index d0052603a..dcd16878b 100644
--- a/app/models/info_request.rb
+++ b/app/models/info_request.rb
@@ -292,13 +292,18 @@ public
     end
 
     # Subject lines for emails about the request
-    def email_subject_request
-        _('{{law_used_full}} request - {{title}}',:law_used_full=>self.law_used_full,:title=>self.title.html_safe)
+    def email_subject_request(opts = {})
+        html = opts.fetch(:html, true)
+        _('{{law_used_full}} request - {{title}}',
+            :law_used_full => self.law_used_full,
+            :title => (html ? title : title.html_safe))
     end
 
-    def email_subject_followup(incoming_message = nil)
+    def email_subject_followup(opts = {})
+        incoming_message = opts.fetch(:incoming_message, nil)
+        html = opts.fetch(:html, true)
         if incoming_message.nil? || !incoming_message.valid_to_reply_to? || !incoming_message.subject
-            'Re: ' + self.email_subject_request
+            'Re: ' + self.email_subject_request(:html => html)
         else
             if incoming_message.subject.match(/^Re:/i)
                 incoming_message.subject
diff --git a/app/views/request/followup_preview.html.erb b/app/views/request/followup_preview.html.erb
index 55afc0245..83978a2f5 100644
--- a/app/views/request/followup_preview.html.erb
+++ b/app/views/request/followup_preview.html.erb
@@ -3,7 +3,7 @@
 <div id="followup">
 
 <%= form_for(@outgoing_message, :html => { :id => 'preview_form' }, :url => (@incoming_message.nil? ? show_response_no_followup_url(:id => @info_request.id) : show_response_url(:id => @info_request.id, :incoming_message_id => @incoming_message.id)) + "#followup" ) do |o| %>
-    
+
     <% if @internal_review %>
         <h1><%= _('Now preview your message asking for an internal review') %></h1>
     <% else %>
@@ -20,7 +20,7 @@
         <div class="correspondence" id="outgoing-0">
             <p class="preview_subject">
                 <strong><%= _('To:') %></strong> <%=h OutgoingMailer.name_for_followup(@info_request, @incoming_message) %>
-                <br><strong><%= _('Subject:') %></strong> <%=h OutgoingMailer.subject_for_followup(@info_request, @outgoing_message) %>
+                <br><strong><%= _('Subject:') %></strong> <%= OutgoingMailer.subject_for_followup(@info_request, @outgoing_message, :html => true) %>
             </p>
 
             <div class="correspondence_text">
diff --git a/app/views/request/preview.html.erb b/app/views/request/preview.html.erb
index 0265d0328..ddd5ab30c 100644
--- a/app/views/request/preview.html.erb
+++ b/app/views/request/preview.html.erb
@@ -23,7 +23,7 @@
                 <% else %>
                   <%=h(@info_request.public_body.name)%>
                 <% end %>
-                <br><strong><%= _('Subject:') %></strong> <%=h @info_request.email_subject_request %>
+                <br><strong><%= _('Subject:') %></strong> <%= @info_request.email_subject_request %>
             </p>
 
             <div class="correspondence_text">
diff --git a/spec/mailers/outgoing_mailer_spec.rb b/spec/mailers/outgoing_mailer_spec.rb
index a11d56dd3..3df5018fe 100644
--- a/spec/mailers/outgoing_mailer_spec.rb
+++ b/spec/mailers/outgoing_mailer_spec.rb
@@ -75,14 +75,14 @@ describe OutgoingMailer, "when working out follow up subjects" do
         ir = info_requests(:fancy_dog_request)
         im = ir.incoming_messages[0]
 
-        ir.email_subject_request.should == "Freedom of Information request - Why do you have & such a fancy dog?"
+        ir.email_subject_request(:html => false).should == "Freedom of Information request - Why do you have & such a fancy dog?"
     end
 
     it "should use 'Re:' and inital request subject for followups which aren't replies to particular messages" do
         ir = info_requests(:fancy_dog_request)
         om = outgoing_messages(:useless_outgoing_message)
 
-        OutgoingMailer.subject_for_followup(ir, om).should == "Re: Freedom of Information request - Why do you have & such a fancy dog?"
+        OutgoingMailer.subject_for_followup(ir, om, :html => false).should == "Re: Freedom of Information request - Why do you have & such a fancy dog?"
     end
 
     it "should prefix with Re: the subject of the message being replied to" do
@@ -91,7 +91,7 @@ describe OutgoingMailer, "when working out follow up subjects" do
         om = outgoing_messages(:useless_outgoing_message)
         om.incoming_message_followup = im
 
-        OutgoingMailer.subject_for_followup(ir, om).should == "Re: Geraldine FOI Code AZXB421"
+        OutgoingMailer.subject_for_followup(ir, om, :html => false).should == "Re: Geraldine FOI Code AZXB421"
     end
 
     it "should not add Re: prefix if there already is such a prefix" do
@@ -101,7 +101,7 @@ describe OutgoingMailer, "when working out follow up subjects" do
         om.incoming_message_followup = im
 
         im.raw_email.data = im.raw_email.data.sub("Subject: Geraldine FOI Code AZXB421", "Subject: Re: Geraldine FOI Code AZXB421")
-        OutgoingMailer.subject_for_followup(ir, om).should == "Re: Geraldine FOI Code AZXB421"
+        OutgoingMailer.subject_for_followup(ir, om, :html => false).should == "Re: Geraldine FOI Code AZXB421"
     end
 
     it "should not add Re: prefix if there already is a lower case re: prefix" do
@@ -113,7 +113,7 @@ describe OutgoingMailer, "when working out follow up subjects" do
         im.raw_email.data = im.raw_email.data.sub("Subject: Geraldine FOI Code AZXB421", "Subject: re: Geraldine FOI Code AZXB421")
         im.parse_raw_email! true
 
-        OutgoingMailer.subject_for_followup(ir, om).should == "re: Geraldine FOI Code AZXB421"
+        OutgoingMailer.subject_for_followup(ir, om, :html => false).should == "re: Geraldine FOI Code AZXB421"
     end
 
     it "should use 'Re:' and initial request subject when replying to failed delivery notifications" do
@@ -126,7 +126,7 @@ describe OutgoingMailer, "when working out follow up subjects" do
         im.raw_email.data = im.raw_email.data.sub("Subject: Geraldine FOI Code AZXB421", "Subject: Delivery Failed")
         im.parse_raw_email! true
 
-        OutgoingMailer.subject_for_followup(ir, om).should == "Re: Freedom of Information request - Why do you have & such a fancy dog?"
+        OutgoingMailer.subject_for_followup(ir, om, :html => false).should == "Re: Freedom of Information request - Why do you have & such a fancy dog?"
     end
 end
 
diff --git a/spec/models/info_request_spec.rb b/spec/models/info_request_spec.rb
index 9ad616ea5..70947584b 100644
--- a/spec/models/info_request_spec.rb
+++ b/spec/models/info_request_spec.rb
@@ -824,7 +824,7 @@ describe InfoRequest do
             im = mock_model(IncomingMessage,
                             :subject => nil,
                             :valid_to_reply_to? => true)
-            subject = ir.email_subject_followup im
+            subject = ir.email_subject_followup(:incoming_message => im, :html => false)
             subject.should match(/^Re: Freedom of Information request.*fancy dog/)
         end