Skip to content

Commit

Permalink
AO3-6732 Add custom cop for calling html_safe on translated strings (o…
Browse files Browse the repository at this point in the history
…twcode#4890)

* AO3-6732 Add custom cop for t().html_safe

* AO3-6732 Add yml hint to ts -> t cop

* AO3-6732 Remove html_safe from mailer greetings

* AO3-6732 Apply suggestions from code review

Co-authored-by: Brian Austin <13002992+brianjaustin@users.noreply.github.com>

---------

Co-authored-by: Brian Austin <13002992+brianjaustin@users.noreply.github.com>
  • Loading branch information
Bilka2 and brianjaustin authored Aug 16, 2024
1 parent 35eadee commit 4942692
Show file tree
Hide file tree
Showing 37 changed files with 151 additions and 39 deletions.
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<% content_for :message do %>
<p><%= t("mailer.general.greeting.formal", name: style_bold(@resource.login)).html_safe %></p>
<p><%= t("mailer.general.greeting.formal_html", name: style_bold(@resource.login)) %></p>
<p><%= t(".intro") %></p>
<p><%= style_link t(".link_title_html"), edit_admin_password_url(reset_password_token: @token) %></p>
<p><%= t(".expiration", count: ArchiveConfig.DAYS_UNTIL_ADMIN_RESET_PASSWORD_LINK_EXPIRES) %></p>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<% content_for :message do %>
<%= t("mailer.general.greeting.formal", name: @resource.login) %>
<%= t("mailer.general.greeting.formal_html", name: @resource.login) %>

<%= t(".intro") %>

Expand Down
2 changes: 1 addition & 1 deletion app/views/admin_mailer/set_password_notification.html.erb
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<% content_for :message do %>
<p><%= t("mailer.general.greeting.informal.addressed", name: style_bold(@admin.login)).html_safe %></p>
<p><%= t("mailer.general.greeting.informal.addressed_html", name: style_bold(@admin.login)) %></p>
<p><%= t(".created") %></p>
<p><%= style_work_metadata_label(t(".username")) %><%= @admin.login %></p>
<p><%= style_work_metadata_label(t(".url")) %><%= style_link(new_admin_session_url, new_admin_session_url) %></p>
Expand Down
2 changes: 1 addition & 1 deletion app/views/admin_mailer/set_password_notification.text.erb
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<% content_for :message do %>
<%= t("mailer.general.greeting.informal.addressed", name: @admin.login) %>
<%= t("mailer.general.greeting.informal.addressed_html", name: @admin.login) %>

<%= t(".created") %>

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<% content_for :message do %>
<p><%= t("mailer.general.greeting.informal.addressed", name: style_bold(t("mailer.general.greeting.tag_wrangler_supervisors"))).html_safe %></p>
<p><%= t("mailer.general.greeting.informal.addressed_html", name: style_bold(t("mailer.general.greeting.tag_wrangler_supervisors"))) %></p>

<p><%= t(".name_changed.html", old_username: style_bold(@old_username), new_username: style_bold(@new_username)) %></p>

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<% content_for :message do %>
<%= t("mailer.general.greeting.informal.addressed", name: t("mailer.general.greeting.tag_wrangler_supervisors")) %>
<%= t("mailer.general.greeting.informal.addressed_html", name: t("mailer.general.greeting.tag_wrangler_supervisors")) %>

<%= t(".name_changed.html", old_username: @old_username, new_username: @new_username) %>

Expand Down
3 changes: 1 addition & 2 deletions app/views/user_mailer/abuse_report.html.erb
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
<% content_for :message do %>
<p>
<% if @username.present? %>
<%= t("mailer.general.greeting.informal.addressed",
name: style_bold(@username)).html_safe %></p>
<%= t("mailer.general.greeting.informal.addressed_html", name: style_bold(@username)) %></p>
<% else %>
<%= t("mailer.general.greeting.informal.unaddressed") %>
</p>
Expand Down
2 changes: 1 addition & 1 deletion app/views/user_mailer/abuse_report.text.erb
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<% content_for :message do %>
<% if @username.present? %>
<%= t("mailer.general.greeting.informal.addressed", name: @username) %>
<%= t("mailer.general.greeting.informal.addressed_html", name: @username) %>
<% else %>
<%= t("mailer.general.greeting.informal.unaddressed") %>
<% end %>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<% content_for :message do %>
<p><%= t("mailer.general.greeting.formal", name: style_bold(@user.login)).html_safe %></p>
<p><%= t("mailer.general.greeting.formal_html", name: style_bold(@user.login)) %></p>

<p><%= t('.deleted.html', title: style_creation_title(@work.title)) %></p>

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<% content_for :message do %>
<%= t("mailer.general.greeting.formal", name: @user.login) %>
<%= t("mailer.general.greeting.formal_html", name: @user.login) %>

<%= t('.deleted.text', title: @work.title) %>

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<% content_for :message do %>
<p><%= t("mailer.general.greeting.formal", name: style_bold(@user.login)).html_safe %></p>
<p><%= t("mailer.general.greeting.formal_html", name: style_bold(@user.login)) %></p>

<p><%= t(".html.hidden", title: style_creation_link(@work.title, @work)).html_safe %></p>

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<% content_for :message do %>
<%= t("mailer.general.greeting.formal", name: @user.login) %>
<%= t("mailer.general.greeting.formal_html", name: @user.login) %>

<%= t(".text.hidden", title: @work.title, work_url: work_url(@work)) %>

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<% content_for :message do %>
<p><%= t("mailer.general.greeting.formal", name: style_bold(@user.login)).html_safe %></p>
<p><%= t("mailer.general.greeting.formal_html", name: style_bold(@user.login)) %></p>

<p>Your work <%= style_creation_link(@work.title, @work) %> has been flagged by our automated system as spam and hidden until it can be reviewed by our Policy & Abuse team. While the work is hidden it can only be accessed by you and AO3 site admins.</p>

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<% content_for :message do %>
<%= t("mailer.general.greeting.formal", name: @user.login) %>
<%= t("mailer.general.greeting.formal_html", name: @user.login) %>

Your work "<%= @work.title.html_safe %>" (<%= work_url(@work) %>) has been flagged by our automated system as spam and hidden until it can be reviewed by our Abuse team. While the work is hidden it can only be accessed by you and AO3 site admins.

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<% content_for :message do %>
<p><%= t("mailer.general.greeting.formal", name: style_bold(@user.login)).html_safe %></p>
<p><%= t("mailer.general.greeting.formal_html", name: style_bold(@user.login)) %></p>

<p><%= t(".changed_status.#{@status}.html",
collection_link: style_link(@collection.title, collection_url(@collection)),
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<% content_for :message do %>
<%= t("mailer.general.greeting.formal", name: @user.login) %>
<%= t("mailer.general.greeting.formal_html", name: @user.login) %>

<%= t(".changed_status.#{@status}.text",
collection_title: @collection.title,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<% content_for :message do %>
<p><%= t("mailer.general.greeting.formal", name: style_bold(@user.login)).html_safe %></p>
<p><%= t("mailer.general.greeting.formal_html", name: style_bold(@user.login)) %></p>

<p>
<%= t(".work_added.html",
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<% content_for :message do %>
<%= t("mailer.general.greeting.formal", name: @user.login) %>
<%= t("mailer.general.greeting.formal_html", name: @user.login) %>

<%= t(".work_added.text",
collection_title: @collection.title,
Expand Down
2 changes: 1 addition & 1 deletion app/views/user_mailer/delete_work_notification.html.erb
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<% content_for :message do %>
<p><%= t("mailer.general.greeting.formal", name: style_bold(@user.login)).html_safe %></p>
<p><%= t("mailer.general.greeting.formal_html", name: style_bold(@user.login)) %></p>
<p><%= (@work.pseuds.count > 1 && @user != User.current_user) ?
t('.deleted_other.html',
title: style_creation_title(@work.title),
Expand Down
2 changes: 1 addition & 1 deletion app/views/user_mailer/delete_work_notification.text.erb
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<% content_for :message do %>
<%= t("mailer.general.greeting.formal", name: @user.login) %>
<%= t("mailer.general.greeting.formal_html", name: @user.login) %>

<%= (@work.pseuds.count > 1 && @user != User.current_user) ?
t('.deleted_other.text',
Expand Down
3 changes: 1 addition & 2 deletions app/views/user_mailer/feedback.html.erb
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
<% content_for :message do %>
<p>
<% if @username.present? %>
<%= t("mailer.general.greeting.informal.addressed",
name: style_bold(@username)).html_safe %>
<%= t("mailer.general.greeting.informal.addressed_html", name: style_bold(@username)) %>
<% else %>
<%= t("mailer.general.greeting.informal.unaddressed") %>
<% end %>
Expand Down
2 changes: 1 addition & 1 deletion app/views/user_mailer/feedback.text.erb
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
<% content_for :message do %>

<% if @username.present? %>
<%= t("mailer.general.greeting.informal.addressed", name: @username) %>
<%= t("mailer.general.greeting.informal.addressed_html", name: @username) %>
<% else %>
<%= t("mailer.general.greeting.informal.unaddressed") %>
<% end %>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<% content_for :message do %>
<p><%= t("mailer.general.greeting.informal.addressed", name: style_bold(@user.login)).html_safe %></p>
<p><%= t("mailer.general.greeting.informal.addressed_html", name: style_bold(@user.login)) %></p>

<p><%= t(".html.body", count: @total, invitation_page_link: style_link(t(".invitation_page_link_text"), user_invitations_url(@user))).html_safe %></p>

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<% content_for :message do %>
<%= t("mailer.general.greeting.informal.addressed", name: @user.login) %>
<%= t("mailer.general.greeting.informal.addressed_html", name: @user.login) %>

<%= t(".text.body", count: @total, invitation_page_url: user_invitations_url(@user)) %>

Expand Down
2 changes: 1 addition & 1 deletion app/views/user_mailer/invite_request_declined.html.erb
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<% content_for :message do %>
<p><%= t("mailer.general.greeting.formal", name: style_bold(@user.login)).html_safe %></p>
<p><%= t("mailer.general.greeting.formal_html", name: style_bold(@user.login)) %></p>

<p><%= t(".main", count: @total) %></p>
<p><%= t(".reason") %></p>
Expand Down
2 changes: 1 addition & 1 deletion app/views/user_mailer/invite_request_declined.text.erb
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<% content_for :message do %>
<%= t("mailer.general.greeting.formal", name: @user.login) %>
<%= t("mailer.general.greeting.formal_html", name: @user.login) %>

<%= t(".main", count: @total) %>

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<% content_for :message do %>
<p><%= t("mailer.general.greeting.formal", name: style_bold(@user.login)).html_safe %></p>
<p><%= t("mailer.general.greeting.formal_html", name: style_bold(@user.login)) %></p>

<p>The collection maintainers of <%= style_link(@collection.title, collection_url(@collection)) %> would
like to include your work <%= style_creation_link(@work.title, work_url(@work)) %> in their collection!</p>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<% content_for :message do %>
<%= t("mailer.general.greeting.formal", name: @user.login) %>
<%= t("mailer.general.greeting.formal_html", name: @user.login) %>

The collection maintainers of "<%= @collection.title %>" (<%= collection_url(@collection) %>) would like to include your work "<%= @work.title.html_safe %>" (<%= work_url(@work) %>) in their collection!

Expand Down
2 changes: 1 addition & 1 deletion app/views/user_mailer/recipient_notification.html.erb
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
<% content_for :message do %>

<p><%= t("mailer.general.greeting.informal.addressed", name: style_bold(@user.login)).html_safe %></p>
<p><%= t("mailer.general.greeting.informal.addressed_html", name: style_bold(@user.login)) %></p>

<p>
<% if @collection %>
Expand Down
2 changes: 1 addition & 1 deletion app/views/user_mailer/recipient_notification.text.erb
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<% content_for :message do %>
<%= t("mailer.general.greeting.informal.addressed", name: @user.login) %>
<%= t("mailer.general.greeting.informal.addressed_html", name: @user.login) %>

<% if @collection %>
<%= t(".text.collection", collection_title: @collection.title, collection_url: collection_url(@collection)) %>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<% content_for :message do %>
<p><%= t("mailer.general.greeting.formal", name: style_bold(@resource.login)).html_safe %></p>
<p><%= t("mailer.general.greeting.formal_html", name: style_bold(@resource.login)) %></p>
<p><%= t(".intro") %></p>
<p><%= style_link t(".link_title"), edit_user_password_url(reset_password_token: @token).html_safe %></p>
<p><%= t(".expiration") %></p>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<% content_for :message do %>
<%= t("mailer.general.greeting.formal", name: @resource.login) %>
<%= t("mailer.general.greeting.formal_html", name: @resource.login) %>

<%= t(".intro") %>

Expand Down
4 changes: 2 additions & 2 deletions config/locales/mailers/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,9 @@ en:
text: If you've received this message in error, please contact Support at %{support_url}.
sent_at: Sent at %{sent_at}.
greeting:
formal: Dear %{name},
formal_html: Dear %{name},
informal:
addressed: Hi, %{name}!
addressed_html: Hi, %{name}!
unaddressed: Hi!
introductory: Hello from the Archive of Our Own!
tag_wrangler_supervisors: Tag Wrangler Supervisors
Expand Down
4 changes: 3 additions & 1 deletion rubocop/cop/i18n/deprecated_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,10 @@ module I18n
# # good
# t(".relative.path.to.translation")
# t(".greeting", name: "world")
# # and in the en.yml locale file:
# greeting: Hello %{name}
class DeprecatedHelper < RuboCop::Cop::Base
MSG = "Prefer Rails built-in `t` helper over `ts`: the latter is not actually translatable. For more information, refer to https://github.com/otwcode/otwarchive/wiki/Internationalization-(i18n)-Standards"
MSG = "Prefer Rails built-in `t` helper over `ts` and move the text into the yml file. `ts` is not actually translatable. For more information, refer to https://github.com/otwcode/otwarchive/wiki/Internationalization-(i18n)-Standards"

RESTRICT_ON_SEND = %i[ts].freeze

Expand Down
39 changes: 39 additions & 0 deletions rubocop/cop/i18n/html_safe_translation.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
# frozen_string_literal: true

module RuboCop
module Cop
module I18n
# Checks for uses of html_safe on strings translated with `t`.
# html_safe does not escape HTML in strings, making it potentially unsafe
# to call on user-generated text like interpolation variables.
# Renaming the locale key to end with `.html` or `_html` will escape interpolation variables
# while keeping HTML from Rails helpers like link_to intact.
#
# @example
# # bad
# t(".has_invited", user_name: style_bold(@user_name)).html_safe
# t(".about.popular", search_tags_link: link_to(t(".search_tags"), search_tags_path)).html_safe
#
# @example
# # good
# t(".has_invited.html", user_name: style_bold(@user_name))
# t(".about.popular_html", search_tags_link: link_to(t(".search_tags"), search_tags_path))
class HtmlSafeTranslation < RuboCop::Cop::Base
MSG = "Prefer t(key) with locale keys ending in `_html` or `.html` over calling t(key).html_safe"

RESTRICT_ON_SEND = %i[html_safe].freeze

# @!method html_safe_translate?(node)
def_node_matcher :html_safe_translate?, <<~PATTERN
(send (send nil? {:t | :translate} ...) :html_safe)
PATTERN

def on_send(node)
return unless html_safe_translate?(node)

add_offense(node)
end
end
end
end
end
4 changes: 2 additions & 2 deletions spec/rubocop/cop/i18n/deprecated_helper_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -7,14 +7,14 @@
it "registers an offense when `ts` is used" do
expect_offense(<<~INVALID)
ts("Some String")
^^^^^^^^^^^^^^^^^ Prefer Rails built-in `t` helper over `ts`: the latter is not actually translatable. For more information, refer to https://github.com/otwcode/otwarchive/wiki/Internationalization-(i18n)-Standards
^^^^^^^^^^^^^^^^^ Prefer Rails built-in `t` helper over `ts` and move the text into the yml file. `ts` is not actually translatable. For more information, refer to https://github.com/otwcode/otwarchive/wiki/Internationalization-(i18n)-Standards
INVALID
end

it "registers an offense when `ts` is used without parentheses" do
expect_offense(<<~INVALID)
ts "Another string"
^^^^^^^^^^^^^^^^^^^ Prefer Rails built-in `t` helper over `ts`: the latter is not actually translatable. For more information, refer to https://github.com/otwcode/otwarchive/wiki/Internationalization-(i18n)-Standards
^^^^^^^^^^^^^^^^^^^ Prefer Rails built-in `t` helper over `ts` and move the text into the yml file. `ts` is not actually translatable. For more information, refer to https://github.com/otwcode/otwarchive/wiki/Internationalization-(i18n)-Standards
INVALID
end

Expand Down
73 changes: 73 additions & 0 deletions spec/rubocop/cop/i18n/html_safe_translation_spec.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
# frozen_string_literal: true

require "rubocop_spec_helper"
require_relative "../../../../rubocop/cop/i18n/html_safe_translation"

describe RuboCop::Cop::I18n::HtmlSafeTranslation do
context "when using translate" do
it "records a violation for calling `html_safe` on it" do
expect_offense(<<~INVALID)
translate(".foo").html_safe
^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer t(key) with locale keys ending in `_html` or `.html` over calling t(key).html_safe
translate(".bar", input: "hello").html_safe
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer t(key) with locale keys ending in `_html` or `.html` over calling t(key).html_safe
INVALID
end

it "does not record a violation when html_safe is not called" do
expect_no_offenses(<<~RUBY)
translate(".foo")
translate(".bar", input: "hello")
RUBY
end
end

context "when using t" do
it "records a violation for calling `html_safe` on it" do
expect_offense(<<~INVALID)
t(".foo").html_safe
^^^^^^^^^^^^^^^^^^^ Prefer t(key) with locale keys ending in `_html` or `.html` over calling t(key).html_safe
t(".bar", input: "hello").html_safe
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ Prefer t(key) with locale keys ending in `_html` or `.html` over calling t(key).html_safe
INVALID
end

it "does not record a violation when html_safe is not called" do
expect_no_offenses(<<~RUBY)
t(".foo")
t(".bar", input: "hello")
RUBY
end
end

# only the helpers in controllers and views support the html suffixes for HTML safe translations
context "when using I18n.t" do
it "does not record a violation for calling `html_safe` on it" do
expect_no_offenses(<<~RUBY)
I18n.t(".foo").html_safe
I18n.t(".bar", input: "hello").html_safe
RUBY
end
end

# only the helpers in controllers and views support the html suffixes for HTML safe translations
context "when using I18n.translate" do
it "does not record a violation for calling `html_safe` on it" do
expect_no_offenses(<<~RUBY)
I18n.translate(".foo").html_safe
I18n.translate(".bar", input: "hello").html_safe
RUBY
end
end

context "when using anther method" do
it "does not record a violation for calling `html_safe` on it" do
expect_no_offenses(<<~RUBY)
cat(".foo").html_safe
cat(".bar", input: "hello").html_safe
not_translate(".foo").html_safe
not_translate(".bar", input: "hello").html_safe
RUBY
end
end
end

0 comments on commit 4942692

Please sign in to comment.