From d1787905ca4acb6259a654adfee2203e4f548b00 Mon Sep 17 00:00:00 2001 From: Alex Plescan Date: Tue, 20 Aug 2024 17:11:50 +1000 Subject: [PATCH] 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" ] ) ``` 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. --- lib/i18n.rb | 1 + lib/i18n/backend/base.rb | 12 ++++++++++-- lib/i18n/backend/fallbacks.rb | 6 +++++- lib/i18n/tests/defaults.rb | 7 +++++++ lib/i18n/tests/lookup.rb | 6 ++++++ lib/i18n/tests/procs.rb | 1 + 6 files changed, 30 insertions(+), 3 deletions(-) diff --git a/lib/i18n.rb b/lib/i18n.rb index 06f7bfa6..4ade2c1b 100644 --- a/lib/i18n.rb +++ b/lib/i18n.rb @@ -19,6 +19,7 @@ module I18n RESERVED_KEYS = %i[ cascade deep_interpolation + skip_interpolation default exception_handler fallback diff --git a/lib/i18n/backend/base.rb b/lib/i18n/backend/base.rb index 7ca9c28a..d8a8bd68 100644 --- a/lib/i18n/backend/base.rb +++ b/lib/i18n/backend/base.rb @@ -54,8 +54,9 @@ def translate(locale, key, options = EMPTY_HASH) end deep_interpolation = options[:deep_interpolation] + skip_interpolation = options[:skip_interpolation] values = Utils.except(options, *RESERVED_KEYS) unless options.empty? - if values && !values.empty? + if !skip_interpolation && values && !values.empty? entry = if deep_interpolation deep_interpolate(locale, entry, values) else @@ -151,7 +152,14 @@ def resolve(locale, object, subject, options = EMPTY_HASH) result = catch(:exception) do case subject when Symbol - I18n.translate(subject, **options.merge(:locale => locale, :throw => true)) + I18n.translate( + subject, + **options.merge( + :locale => locale, + :throw => true, + :skip_interpolation => true + ) + ) when Proc date_or_time = options.delete(:object) || object resolve(locale, object, subject.call(date_or_time, **options)) diff --git a/lib/i18n/backend/fallbacks.rb b/lib/i18n/backend/fallbacks.rb index a88a4fca..caa4e66e 100644 --- a/lib/i18n/backend/fallbacks.rb +++ b/lib/i18n/backend/fallbacks.rb @@ -71,7 +71,11 @@ def resolve_entry(locale, object, subject, options = EMPTY_HASH) case subject when Symbol - I18n.translate(subject, **options.merge(:locale => options[:fallback_original_locale], :throw => true)) + I18n.translate(subject, **options.merge( + :locale => options[:fallback_original_locale], + :throw => true, + :skip_interpolation => true + )) when Proc date_or_time = options.delete(:object) || object resolve_entry(options[:fallback_original_locale], object, subject.call(date_or_time, **options)) diff --git a/lib/i18n/tests/defaults.rb b/lib/i18n/tests/defaults.rb index 31fdb469..e5db3365 100644 --- a/lib/i18n/tests/defaults.rb +++ b/lib/i18n/tests/defaults.rb @@ -47,6 +47,13 @@ def setup I18n.backend.store_translations(:en, { :foo => { :bar => 'bar' } }, { :separator => '|' }) assert_equal 'bar', I18n.t(nil, :default => :'foo|bar', :separator => '|') end + + # Addresses issue: #599 + test "defaults: only interpolates once when resolving defaults" do + I18n.backend.store_translations(:en, :greeting => 'hey %{name}') + assert_equal 'hey %{dont_interpolate_me}', + I18n.t(:does_not_exist, :name => '%{dont_interpolate_me}', default: [:greeting]) + end end end end diff --git a/lib/i18n/tests/lookup.rb b/lib/i18n/tests/lookup.rb index bbd775f0..f1bee792 100644 --- a/lib/i18n/tests/lookup.rb +++ b/lib/i18n/tests/lookup.rb @@ -76,6 +76,12 @@ def setup test "lookup: a resulting Hash is not frozen" do assert !I18n.t(:hash).frozen? end + + # Addresses issue: #599 + test "lookup: only interpolates once when resolving symbols" do + I18n.backend.store_translations(:en, foo: :bar, bar: '%{value}') + assert_equal '%{dont_interpolate_me}', I18n.t(:foo, value: '%{dont_interpolate_me}') + end end end end diff --git a/lib/i18n/tests/procs.rb b/lib/i18n/tests/procs.rb index 6abd8612..db99c766 100644 --- a/lib/i18n/tests/procs.rb +++ b/lib/i18n/tests/procs.rb @@ -57,6 +57,7 @@ def self.filter_args(*args) if arg.is_a?(Hash) arg.delete(:fallback_in_progress) arg.delete(:fallback_original_locale) + arg.delete(:skip_interpolation) end arg end.inspect