Skip to content

Commit 229d57e

Browse files
committed
Fixes strings being interpolated multiple times
Similarly to #599, I've observed issues issues where untrusted user input that includes interpolation patterns gets unintentionally interpolated and leads to bogus `I18n::MissingInterpolationArgument` exceptions. This happens when multiple lookups are required for a key to be resolved, which is common when resolving defaults, or resolving a key that itself resolves to a Symbol. As an example let's consider these translations, common for Rails apps: ```yaml en: activerecord: errors: messages: taken: "%{value} has already been taken" ``` If the `value` given to interpolate ends up containing interpolation characters, and Rails specifies default keys (as [described here](https://guides.rubyonrails.org/i18n.html#error-message-scopes)), resolving those defaults will cause a `I18n::MissingInterpolationArgument` to be raised: ```rb I18n.t('activerecord.errors.models.organization.attributes.name.taken', value: '%{dont_interpolate_me}', default: [ :"activerecord.errors.models.organization.taken", :"activerecord.errors.messages.taken" ] ) ``` Raises: ``` I18n::MissingInterpolationArgument: missing interpolation argument :dont_interpolate_me in "%{dont_interpolate_me}" ({:value=>"%{dont_interpolate_me}"} given) ``` Instead of this, we'd expect the translation to resolve to: ``` %{dont_interpolate_me} has already been taken ``` This behaviour is caused by the way that recursive lookups work: whenever a key can't be resolved to a string directly, the `I18n.translate` method is called either to walk through the defaults specified, or if a Symbol is matched, to try to resolve that symbol. This results in interpolation being executed twice for recursive lookups... once on the pass that finally resolves to a string, and again on the original call to `I18n.translate`. A "proper" fix here would likely revolve around decoupling key resolution from interpolation... it feels odd to me that the `resolve_entry` method calls `I18n.translate`... however I see this as a fundamental change beyond the scope of this fix. Instead I'm proposing to add a new reserved key `skip_interpolation` that gets passed down into every recursive call of `I18n.translate` and instructs the method to skip interpolation. This ensures that only the initial `I18n.translate` call is the one that gets its string interpolated.
1 parent eacc34f commit 229d57e

File tree

6 files changed

+30
-3
lines changed

6 files changed

+30
-3
lines changed

lib/i18n.rb

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,6 +19,7 @@ module I18n
1919
RESERVED_KEYS = %i[
2020
cascade
2121
deep_interpolation
22+
skip_interpolation
2223
default
2324
exception_handler
2425
fallback

lib/i18n/backend/base.rb

Lines changed: 10 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -54,8 +54,9 @@ def translate(locale, key, options = EMPTY_HASH)
5454
end
5555

5656
deep_interpolation = options[:deep_interpolation]
57+
skip_interpolation = options[:skip_interpolation]
5758
values = Utils.except(options, *RESERVED_KEYS) unless options.empty?
58-
if values && !values.empty?
59+
if !skip_interpolation && values && !values.empty?
5960
entry = if deep_interpolation
6061
deep_interpolate(locale, entry, values)
6162
else
@@ -151,7 +152,14 @@ def resolve(locale, object, subject, options = EMPTY_HASH)
151152
result = catch(:exception) do
152153
case subject
153154
when Symbol
154-
I18n.translate(subject, **options.merge(:locale => locale, :throw => true))
155+
I18n.translate(
156+
subject,
157+
**options.merge(
158+
:locale => locale,
159+
:throw => true,
160+
:skip_interpolation => true
161+
)
162+
)
155163
when Proc
156164
date_or_time = options.delete(:object) || object
157165
resolve(locale, object, subject.call(date_or_time, **options))

lib/i18n/backend/fallbacks.rb

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -71,7 +71,11 @@ def resolve_entry(locale, object, subject, options = EMPTY_HASH)
7171

7272
case subject
7373
when Symbol
74-
I18n.translate(subject, **options.merge(:locale => options[:fallback_original_locale], :throw => true))
74+
I18n.translate(subject, **options.merge(
75+
:locale => options[:fallback_original_locale],
76+
:throw => true,
77+
:skip_interpolation => true
78+
))
7579
when Proc
7680
date_or_time = options.delete(:object) || object
7781
resolve_entry(options[:fallback_original_locale], object, subject.call(date_or_time, **options))

lib/i18n/tests/defaults.rb

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,13 @@ def setup
4747
I18n.backend.store_translations(:en, { :foo => { :bar => 'bar' } }, { :separator => '|' })
4848
assert_equal 'bar', I18n.t(nil, :default => :'foo|bar', :separator => '|')
4949
end
50+
51+
# Addresses issue: #599
52+
test "defaults: only interpolates once when resolving defaults" do
53+
I18n.backend.store_translations(:en, :greeting => 'hey %{name}')
54+
assert_equal 'hey %{dont_interpolate_me}',
55+
I18n.t(:does_not_exist, :name => '%{dont_interpolate_me}', default: [:greeting])
56+
end
5057
end
5158
end
5259
end

lib/i18n/tests/lookup.rb

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -76,6 +76,12 @@ def setup
7676
test "lookup: a resulting Hash is not frozen" do
7777
assert !I18n.t(:hash).frozen?
7878
end
79+
80+
# Addresses issue: #599
81+
test "lookup: only interpolates once when resolving symbols" do
82+
I18n.backend.store_translations(:en, foo: :bar, bar: '%{value}')
83+
assert_equal '%{dont_interpolate_me}', I18n.t(:foo, value: '%{dont_interpolate_me}')
84+
end
7985
end
8086
end
8187
end

lib/i18n/tests/procs.rb

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,7 @@ def self.filter_args(*args)
5757
if arg.is_a?(Hash)
5858
arg.delete(:fallback_in_progress)
5959
arg.delete(:fallback_original_locale)
60+
arg.delete(:skip_interpolation)
6061
end
6162
arg
6263
end.inspect

0 commit comments

Comments
 (0)