Skip to content

Commit

Permalink
Fix XSS vulnerability when using HTML-safe translations and interpola…
Browse files Browse the repository at this point in the history
…tion arguments
  • Loading branch information
camertron committed Mar 2, 2022
1 parent d4678fd commit 5f41f38
Show file tree
Hide file tree
Showing 4 changed files with 43 additions and 5 deletions.
7 changes: 2 additions & 5 deletions Gemfile.lock
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
PATH
remote: .
specs:
view_component (2.48.0)
view_component (2.49.1)
activesupport (>= 5.0.0, < 8.0)
method_source (~> 1.0)

Expand Down Expand Up @@ -274,11 +274,8 @@ DEPENDENCIES
haml (~> 5)
jbuilder (~> 2)
minitest (= 5.6.0)
net-imap
net-pop
net-smtp
pry (~> 0.13)
rails (~> 7.0.0)
rails (= 7.0.1)
rake (~> 13.0)
rubocop-github (~> 0.16.1)
simplecov (~> 0.18.0)
Expand Down
30 changes: 30 additions & 0 deletions lib/view_component/translatable.rb
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
# frozen_string_literal: true

require "erb"
require "set"
require "i18n"
require "action_view/helpers/translation_helper"
Expand Down Expand Up @@ -70,6 +71,10 @@ def translate(key = nil, **options)
key = key&.to_s unless key.is_a?(String)
key = "#{i18n_scope}#{key}" if key.start_with?(".")

if HTML_SAFE_TRANSLATION_KEY.match?(key)
html_escape_translation_options!(options)
end

if key.start_with?(i18n_scope + ".")
translated =
catch(:exception) do
Expand All @@ -96,5 +101,30 @@ def translate(key = nil, **options)
def i18n_scope
self.class.i18n_scope
end

def html_safe_translation(translation)
if translation.respond_to?(:map)
translation.map { |element| html_safe_translation(element) }
else
# It's assumed here that objects loaded by the i18n backend will respond to `#html_safe?`.
# It's reasonable that if we're in Rails, `active_support/core_ext/string/output_safety.rb`
# will provide this to `Object`.
translation.html_safe # rubocop:disable Rails/OutputSafety
end
end

private

def html_escape_translation_options!(options)
options.each do |name, value|
unless i18n_option?(name) || (name == :count && value.is_a?(Numeric))
options[name] = ERB::Util.html_escape(value.to_s)
end
end
end

def i18n_option?(name)
(@i18n_option_names ||= I18n::RESERVED_KEYS.to_set).include?(name)
end
end
end
2 changes: 2 additions & 0 deletions test/sandbox/app/components/translatable_component.yml
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ en:

hello_html: "Hello from <strong>sidecar translations</strong>!"

interpolated_html: "There are %{horse_count} horses in the <strong>barn</strong>!"

html: "hello <em>world</em>!"

from:
Expand Down
9 changes: 9 additions & 0 deletions test/view_component/translatable_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,15 @@ def test_translate_marks_translations_with_a_html_suffix_as_safe_html
assert_predicate translate(".hello_html"), :html_safe?
end

def test_translate_with_html_suffix_escapes_interpolated_arguments
translation = translate(".interpolated_html", horse_count: "<script type='text/javascript'>alert('foo');</script>")
assert_equal(
"There are &lt;script type=&#39;text/javascript&#39;&gt;alert(&#39;foo&#39;);&lt;/script&gt; horses in the "\
"<strong>barn</strong>!",
translation
)
end

def test_translate_uses_the_helper_when_no_sidecar_file_is_provided
# The cache needs to be kept clean for TranslatableComponent, otherwise it will rely on the
# already created i18n_backend.
Expand Down

0 comments on commit 5f41f38

Please sign in to comment.