[pro#549] apostrophe encoding #4807

lizconlan · 2018-08-14T13:38:08Z

What does this do?

Prevents appending text to the outgoing message body from triggering HTML escaping of the existing text (only applies to logged in users)
Alters the preview templates on the pro side to bring them in line with the standard ones (the HTML was not being escaped before display before)

Prevent apostrophe's being displayed as ' (and the whole message being wrapped in an HTML template) when an admin is sending an email as part of marking a request as vexatious/not_foii
Fixes the display of user names with apostrophes in the admins' user list

Why was this needed?

Fixes some of the inconsistencies of how apostrophes (and other special characters such as ampersands) are displayed across the site.

Notes to reviewer

Are the pro request preview changes correct or was the behaviour difference intentional?
Is removing the HTML wrapper from the admin email textarea correct? (4410611)

Relevant issue(s)

Fixes mysociety/alaveteli-professional#549
Fixes #4793

houndci-bot · 2018-08-14T13:38:18Z

spec/integration/create_request_spec.rb

+    end
+
+    it 'appends the user name' do
+       using_session(user_session) do


Use 2 (not 3) spaces for indentation.

garethrees · 2018-08-21T16:29:29Z

app/views/admin_user/_user_table.html.erb

@@ -7,7 +7,7 @@
        <span class="user-labels">
          <%= user_labels(user) %>
        </span>
-        <%= link_to("#{ h(user.name) }", admin_user_path(user)) %>
+        <%= link_to("#{ h(user.name.html_safe) }", admin_user_path(user)) %>


👎 Rails escapes content by default.

The solution here is to remove the h call, which is causing the double escape.

<%= link_to user.name, admin_user_path(user) %>

garethrees · 2018-08-22T09:34:02Z

app/models/outgoing_message.rb

@@ -94,7 +94,7 @@ def get_default_message
  def set_signature_name(name)
    # TODO: We use raw_body here to get unstripped one
    if raw_body == get_default_message
-      self.body = raw_body + name
+      self.body = "#{raw_body}#{name}".html_safe


This feels like the wrong place to be doing this.

The escaping happens much further down the chain:

t = OutgoingMessage::Template::InitialRequest.new # => #<OutgoingMessage::Template::InitialRequest:0x0000000a34c8d0> t.body(public_body_name: "Department for '& Humpadinking") # => "Dear Department for '& Humpadinking,\n\n\n\nYours faithfully,\n\n"

_("hi {{name}}", name: '&') # => "hi &"

Translations are explicitly html_safe.

_("hi {{name}}", name: '&').html_safe? # => true

It looks like this should all "just work":

diff --git a/app/controllers/admin_spam_addresses_controller.rb b/app/controllers/admin_spam_addresses_controller.rb index 357d9e594..35370f90e 100644 --- a/app/controllers/admin_spam_addresses_controller.rb +++ b/app/controllers/admin_spam_addresses_controller.rb @@ -6,6 +6,7 @@ class AdminSpamAddressesController < AdminController def index @spam_addresses = SpamAddress.all @spam_address = SpamAddress.new + @thing = _("Hi {{name}}", name: "bo&'b") end def create diff --git a/app/views/admin_spam_addresses/index.html.erb b/app/views/admin_spam_addresses/index.html.erb index f445e35aa..2630115d2 100644 --- a/app/views/admin_spam_addresses/index.html.erb +++ b/app/views/admin_spam_addresses/index.html.erb @@ -2,6 +2,9 @@ <h1><%= @title %></h1> +<p><%= @thing %></p> +<%= text_area_tag :thing, @thing %> + <div class="row"> <div class="span12"> <p>

The actual problem we have is not that HTML is escaped, but that it is double escaped:

_("hi {{name}}", name: '&') # => "hi &" CGI.escapeHTML(_("hi {{name}}", name: '&')) # => "hi &amp;" CGI.escapeHTML(_("hi {{name}}", name: '&').html_safe) # => "hi &amp;"

We think a better solution is:

diff --git a/app/models/outgoing_message.rb b/app/models/outgoing_message.rb index 560a23a59..1cf33c71d 100644 --- a/app/models/outgoing_message.rb +++ b/app/models/outgoing_message.rb @@ -94,7 +94,7 @@ class OutgoingMessage < ActiveRecord::Base def set_signature_name(name) # TODO: We use raw_body here to get unstripped one if raw_body == get_default_message - self.body = raw_body + name + self.body = get_default_message + name end end

garethrees · 2018-08-22T11:09:02Z

app/views/admin_request/hidden_user_explanation.html.erb

@@ -1,4 +1,4 @@
-<%= _("Dear {{name}},", :name => name_to) %>
+<%= _("Dear {{name}},", :name => name_to.html_safe) %>


We're going to try making this a text template rather than html.

The "right" way to deal with this is probably to pass in a SafeBuffer as this is supposed to be text/plain output and it puts the decision of how to treat the information in the calling controller rather than the reusable template. (Just passing name_to in still causes it to be escaped in a text.erb template - ~~unless I've missed something, will double-check that~~ because it's happening when we translate the phrase - which is irritating). oh, presumably the same problem will apply to {{site_name}} as well (I half remember having to fix this for email templates ages ago) sigh

Immediate downside of renaming the template to text.erb from html.erb is that without adding e.g. response.content_type = :html after calling render_to_string (stealth bug!) the admin page is sent as plain text as well /o\

However it doesn't look as though the version of the templated text carefully constructed in the controller is ever used as the textarea is unreachable with javascript disabled and the javascript version (from ServicesController#hidden_user_explanation) will immediately replace the text with javascript enabled so it's probably safe to remove it. Although that's going to make it hard to write a test for...

Oh also the textarea text is used as-is to construct a plain/text email to the user (sent by AdminRequestController#hide via ContactMailer#from_admin_message).

Waiting to see if the changes I've made so far break anything I'd not thought about. Then I think the best place to test the textarea/email to user to say their request has been hidden as vexatious content is the ServicesController spec

garethrees · 2018-08-22T11:14:43Z

app/controllers/admin_request_controller.rb

-    @request_hidden_user_explanation = render_to_string(:template => "admin_request/hidden_user_explanation",
-                                                        :locals => vars_for_explanation)
+    @request_hidden_user_explanation =
+      render_to_string(template: "admin_request/hidden_user_explanation",


If we make the template a text template rather than html we probably won't need this change.

houndci-bot · 2018-08-23T09:41:36Z

app/controllers/services_controller.rb

@@ -58,11 +58,10 @@ def hidden_user_explanation
    render :template => "admin_request/hidden_user_explanation",
      :content_type => "text/plain",
      :layout => false,
-      :locals => {:name_to => info_request.user_name,
-                  :name_from => AlaveteliConfiguration.contact_name,
+      :locals => {:name_to => info_request.user_name.html_safe,


Align the elements of a hash literal if they span more than one line.

houndci-bot · 2018-08-23T11:24:35Z

app/controllers/services_controller.rb

+           :locals => {
+              :name_to => info_request.user_name.html_safe,
+              :info_request => info_request, :reason => params[:reason],
+              :info_request_url => 'http://' + AlaveteliConfiguration.domain + request_path(info_request),


Line is too long. [106/80]

houndci-bot · 2018-08-23T13:12:29Z

app/controllers/services_controller.rb

+           locals: {
+             name_to: info_request.user_name.html_safe,
+             info_request: info_request, :reason => params[:reason],
+             info_request_url: 'http://' + AlaveteliConfiguration.domain + request_path(info_request),


Line is too long. [102/80]

Should be able to change this to request_url(info_request) to clean this up and fix the line-length issue. I'd make a new commit for that.

The test failed but I'm willing to bet that's a problem with my test rather than the Rails helper method.

houndci-bot · 2018-08-23T13:33:51Z

app/controllers/services_controller.rb

+    # https links in emails if forcing SSL
+    protocol = if AlaveteliConfiguration.force_ssl
+      "https://"
+    else


Align else with if.

houndci-bot · 2018-08-23T13:33:51Z

app/controllers/services_controller.rb

+  def request_link(request_path)
+    # https links in emails if forcing SSL
+    protocol = if AlaveteliConfiguration.force_ssl
+      "https://"


Use 2 (not -9) spaces for indentation.

garethrees

I think this is good in principle.

Inline comments to address, but the gist of the fix looks okay to me.

garethrees · 2018-08-29T14:24:29Z

app/views/admin_user/_user_table.html.erb

@@ -7,7 +7,7 @@
        <span class="user-labels">
          <%= user_labels(user) %>
        </span>
-        <%= link_to("#{ h(user.name) }", admin_user_path(user)) %>
+        <%= link_to("#{ user.name }", admin_user_path(user)) %>


This doesn't need to be interpolated in to a string.

garethrees · 2018-08-29T14:26:00Z

spec/views/admin_user/_user_table.html.erb_spec.rb

+    render partial: 'admin_user/user_table.html.erb',
+           locals: { users: users,
+                     banned_column: false }
+    expect(rendered).not_to match("O&amp;#39;Toole")


Would be clearer and more robust to expect(rendered).to match("O'Toole"). That way the spec would fail if the behaviour was incorrect, with a diff of expected vs actual (and the actual would be the incorrect double escaped string).

(added more to remind myself) the expected match is "O'Toole" as it escapes the ' but avoids the double escape (which catches the ampersand)

spec/controllers/services_controller_spec.rb

app/controllers/services_controller.rb

garethrees · 2018-08-29T14:42:04Z

app/controllers/services_controller.rb

+           locals: {
+             name_to: info_request.user_name.html_safe,
+             info_request: info_request, :reason => params[:reason],
+             info_request_url: 'http://' + AlaveteliConfiguration.domain + request_path(info_request),


Should be able to change this to request_url(info_request) to clean this up and fix the line-length issue. I'd make a new commit for that.

garethrees · 2018-08-29T14:42:33Z

app/controllers/services_controller.rb

+           locals: {
+             name_to: info_request.user_name,
+             name_from: AlaveteliConfiguration.contact_name,
+             info_request: info_request, :reason => params[:reason],


You missed :reason => params[:reason] – should drop that on to a new line and make it a 1.9-style hash

app/controllers/services_controller.rb

garethrees · 2018-08-29T14:53:12Z

app/controllers/services_controller.rb

@@ -115,4 +115,14 @@ def ask_the_eu_link
    %q(<a href="http://asktheeu.org">Ask The EU</a>)
  end

+  def request_link(request_path)


👎 – pretty sure the following would fix this:

diff --git a/app/controllers/services_controller.rb b/app/controllers/services_controller.rb index 079f69d7f..76771a6c0 100644 --- a/app/controllers/services_controller.rb +++ b/app/controllers/services_controller.rb @@ -61,7 +61,7 @@ class ServicesController < ApplicationController locals: { name_to: info_request.user_name.html_safe, info_request: info_request, :reason => params[:reason], - info_request_url: request_link(request_path(info_request)), + info_request_url: request_url(info_request), site_name: site_name.html_safe } end

The specs do fail, but that's because config.force_ssl is set in the environment initialiser so can't be stubbed.

Its worth checking that I'm not talking rubbish here, but I'm pretty sure the helpers take protocol in to account…

garethrees

Need to address #4807 (comment), but good to merge after that

So that the new text is added to the html_safe version of the existing text to avoid the original text (including the authority name) from being HTML escaped twice.

Use the same `OutgoingMessage#get_body_for_html_display` used by the standard preview template.

The form fields for dealing with hiding a request as either not_foi or vexatious are hidden until the radio button is selected (requiring javascript). And with javascript enabled, the pre-populated message is immediately replaced with a holding message then a fresh copy of the text as generated by ServicesController#hidden_user_explanation

* Simplify `require` of spec helper; can just reference the file by name as its already in the load path * Make the top level `describe` block describe only the class we're testing * Group specs under `describe '#method_name' do` block * Remove the Ruby 1.9 encoding comment line * Remove line breaks from spec descriptions * Reduce line lengths

Prior to Rails 4.0, the template format was set from the MIME type (as originally set here via the `:content_type` option). This has now flipped so that content type is derived from the supplied format (which defaults to `:html`). If we don't pass in `formats: [:text]` it assumes HTML and fails to find the renamed template (raising an `ActionView::MissingTemplate` error). See: rails/rails@67b2404

This template is only going to be rendered as `text/plain` so malicious user input (e.g. <script> tags) is not going to be rendered as HTML, allowing us to treat the text as safe. We only need to do this because we're going to use these arguments in translation strings and we don't currently have a way to disable GetText's HTML escaping of interpolated strings.

houndci-bot reviewed Aug 14, 2018

View reviewed changes

lizconlan force-pushed the pro549-apostrophe-encoding branch 4 times, most recently from ca74840 to f5a4253 Compare August 16, 2018 17:13

lizconlan changed the title ~~[WIP] [pro#549] apostrophe encoding~~ [pro#549] apostrophe encoding Aug 17, 2018

lizconlan added the awaiting-review label Aug 17, 2018

garethrees requested changes Aug 22, 2018

View reviewed changes

garethrees assigned lizconlan Aug 22, 2018

garethrees added 3 - current sprint and removed awaiting-review labels Aug 22, 2018

lizconlan force-pushed the pro549-apostrophe-encoding branch from f5a4253 to 5ad2024 Compare August 22, 2018 15:31

lizconlan added 4 - now and removed 3 - current sprint labels Aug 23, 2018

houndci-bot reviewed Aug 23, 2018

View reviewed changes

lizconlan force-pushed the pro549-apostrophe-encoding branch from 8e1899b to 288b6cf Compare August 23, 2018 10:38

houndci-bot reviewed Aug 23, 2018

View reviewed changes

lizconlan force-pushed the pro549-apostrophe-encoding branch from 43a9e2f to da3a5f0 Compare August 23, 2018 13:12

houndci-bot reviewed Aug 23, 2018

View reviewed changes

lizconlan force-pushed the pro549-apostrophe-encoding branch from da3a5f0 to 336b828 Compare August 23, 2018 13:13

houndci-bot reviewed Aug 23, 2018

View reviewed changes

lizconlan force-pushed the pro549-apostrophe-encoding branch from a0f40b5 to e0aa4b5 Compare August 23, 2018 13:35

lizconlan added awaiting-review and removed 4 - now labels Aug 23, 2018

lizconlan assigned garethrees and unassigned lizconlan Aug 23, 2018

garethrees requested changes Aug 29, 2018

View reviewed changes

garethrees added 3 - current sprint and removed awaiting-review labels Aug 29, 2018

garethrees removed their assignment Aug 29, 2018

garethrees assigned lizconlan Aug 29, 2018

lizconlan force-pushed the pro549-apostrophe-encoding branch 3 times, most recently from 36eedf2 to 7a7d30d Compare August 31, 2018 14:54

lizconlan added awaiting-review and removed 3 - current sprint labels Aug 31, 2018

lizconlan assigned garethrees and unassigned lizconlan Aug 31, 2018

garethrees approved these changes Sep 12, 2018

View reviewed changes

garethrees added 3 - current sprint and removed awaiting-review labels Sep 12, 2018

garethrees assigned lizconlan and unassigned garethrees Sep 12, 2018

lizconlan added 10 commits September 18, 2018 10:10

Tidy up spec

9a42c63

Append the signature to get_default_message instead of raw_body

6315a4f

So that the new text is added to the html_safe version of the existing text to avoid the original text (including the authority name) from being HTML escaped twice.

Escape HTML in the pro request preview templates

6696423

Use the same `OutgoingMessage#get_body_for_html_display` used by the standard preview template.

Don't double escape HTML in the admin user list

433f7bd

Remove unused param

edd5f4c

Use the request_url helper to construct the link to the request page

9562015

lizconlan force-pushed the pro549-apostrophe-encoding branch from 7a7d30d to 9562015 Compare September 18, 2018 09:10

lizconlan merged commit 9562015 into develop Sep 18, 2018

lizconlan mentioned this pull request Sep 18, 2018

HTML escaped single quote in authority name mysociety/belgium-theme#72

Closed

lizconlan deleted the pro549-apostrophe-encoding branch October 9, 2018 12:28

mdeuk mentioned this pull request Jul 23, 2022

Encoding issues with user and body names #4793

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[pro#549] apostrophe encoding #4807

[pro#549] apostrophe encoding #4807

lizconlan commented Aug 14, 2018 •

edited

Loading

houndci-bot Aug 14, 2018

garethrees Aug 21, 2018

garethrees Aug 22, 2018

garethrees Aug 22, 2018

garethrees Aug 22, 2018

lizconlan Aug 22, 2018 •

edited

Loading

lizconlan Aug 23, 2018

garethrees Aug 22, 2018

houndci-bot Aug 23, 2018

houndci-bot Aug 23, 2018

houndci-bot Aug 23, 2018

garethrees Aug 29, 2018

lizconlan Aug 31, 2018

houndci-bot Aug 23, 2018

houndci-bot Aug 23, 2018

garethrees left a comment

garethrees Aug 29, 2018

garethrees Aug 29, 2018

lizconlan Aug 31, 2018

garethrees Aug 29, 2018

garethrees Aug 29, 2018

garethrees Aug 29, 2018

garethrees left a comment

		@@ -1,4 +1,4 @@
		<%= _("Dear {{name}},", :name => name_to) %>
		<%= _("Dear {{name}},", :name => name_to.html_safe) %>

[pro#549] apostrophe encoding #4807

[pro#549] apostrophe encoding #4807

Conversation

lizconlan commented Aug 14, 2018 • edited Loading

What does this do?

Why was this needed?

Notes to reviewer

Relevant issue(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lizconlan Aug 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garethrees left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garethrees left a comment

Choose a reason for hiding this comment

lizconlan commented Aug 14, 2018 •

edited

Loading

lizconlan Aug 22, 2018 •

edited

Loading