Fixes strings being interpolated multiple times#699
Merged
radar merged 1 commit intoruby-i18n:masterfrom Sep 15, 2024
Merged
Conversation
Similarly to ruby-i18n#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.
Collaborator
|
LGTM! I'd welcome a patch that would undertake that decoupling if you wish to tackle it. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Similarly to #599, I've observed issues issues where untrusted user input that includes interpolation patterns gets unintentionally interpolated and leads to bogus
I18n::MissingInterpolationArgumentexceptions.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:
If the
valuegiven to interpolate ends up containing interpolation characters, and Rails specifies multiple default keys (as described here) aI18n::MissingInterpolationArgumentwill be raised:Instead of this, we'd expect the translation to resolve to:
This behaviour is caused by the way that recursive lookups work: whenever a key can't be resolved to a string directly the
I18n.translatemethod 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.translatepass that finally resolves to a string, and again on the original call toI18n.translate.A "proper" fix here would likely revolve around decoupling key resolution from interpolation. It feels odd to me that the
resolve_entrymethod callsI18n.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_interpolationthat gets passed down into every recursive call ofI18n.translateand instructs the method to skip interpolation.This ensures that only the initial
I18n.translatecall is the one that gets its string interpolated.