ruby-i18n / i18n

Internationalization (i18n) library for Ruby
MIT License
986 stars 411 forks source link

Fixes strings being interpolated multiple times #699

Closed alexpls closed 1 month ago

alexpls commented 2 months ago

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:

en:
  activerecord:
    errors:
      messages:
        taken: "%{value} has already been taken"

If the value given to interpolate ends up containing interpolation characters, and Rails specifies multiple default keys (as described here) a I18n::MissingInterpolationArgument will be raised:

I18n.t('activerecord.errors.models.organization.attributes.name.taken',
  value: '%{dont_interpolate_me}',
  default: [
    :"activerecord.errors.models.organization.taken",
    :"activerecord.errors.messages.taken"
  ]
)

# => 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 I18n.translate 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.

radar commented 1 month ago

LGTM! I'd welcome a patch that would undertake that decoupling if you wish to tackle it.