Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[BUG] Symbol resolving interpolates the string multiple times #599

Open
movermeyer opened this issue Jan 7, 2022 · 2 comments
Open

[BUG] Symbol resolving interpolates the string multiple times #599

movermeyer opened this issue Jan 7, 2022 · 2 comments

Comments

@movermeyer
Copy link
Contributor

movermeyer commented Jan 7, 2022 •
edited
Loading

What I tried to do

# frozen_string_literal: true

require "i18n"

I18n.backend = I18n::Backend::Simple.new
I18n.backend.store_translations(:en, foo: :bar, bar: "%{baz}")
puts(I18n.t("foo", baz: "%{qux}"))

What I expected to happen

"%{qux}"

i.e., the resolved Symbol's value is only interpolated once and returned.

What actually happened

I18n::MissingInterpolationArgument: missing interpolation argument :qux in "%{qux}" ({:baz=>"%{qux}"} given)

This is a result of %{baz} interpolating to %{qux}, and then the code trying to interpolate %{qux}.

Note that removing the Symbol resolving works:

# frozen_string_literal: true

require "i18n"

I18n.backend = I18n::Backend::Simple.new
I18n.backend.store_translations(:en, foo: "%{baz}")
puts(I18n.t("foo", baz: "%{qux}"))

Note that interpolation occurs for each layer of Symbol resolving that is done:

e.g.,

# frozen_string_literal: true

require "i18n"

I18n.backend = I18n::Backend::Simple.new
I18n.backend.store_translations(:en, foo: :bar, bar: :baz, baz: "%{baz}")
puts(I18n.t("foo", baz: "%{qux}", qux: "%{cat}"))

Interpolates the string three times.

Versions of i18n, rails, and anything else you think is necessary

i18n: v1.8.11
Ruby: v3.0.3
@movermeyer movermeyer changed the title [BUG] Symbol resolving interpolates the string twice [BUG] Symbol resolving interpolates the string multiple times Jan 10, 2022
@movermeyer
Copy link
Contributor Author

This can also happen as a result of Symbol resolving via default parameters:

# frozen_string_literal: true

require "i18n"

I18n.backend = I18n::Backend::Simple.new
I18n.backend.store_translations(:en, foo: "%{baz}")
puts(I18n.t("non-existent", baz: "%{qux}", default: [:foo]))

@ruby-i18n ruby-i18n deleted a comment Apr 8, 2023
alexpls added a commit to alexpls/i18n that referenced this issue Aug 23, 2024
@alexpls
Fixes strings being interpolated multiple times
e72eb53 
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 multiple keys (as [described here](https://guides.rubyonrails.org/i18n.html#error-message-scopes))
a `I18n::MissingInterpolationArgument` will 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 I'm not
comfortable with implemeting as my first contribution to this library.

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.
alexpls added a commit to alexpls/i18n that referenced this issue Aug 23, 2024
@alexpls
Fixes strings being interpolated multiple times
d178790 
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"
  ]
)

```

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.
alexpls added a commit to alexpls/i18n that referenced this issue Aug 23, 2024
@alexpls
Fixes strings being interpolated multiple times
b1cb20d 
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.
alexpls added a commit to alexpls/i18n that referenced this issue Aug 23, 2024
@alexpls
Fixes strings being interpolated multiple times
229d57e 
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.
@alexpls alexpls mentioned this issue Aug 23, 2024
Merged
@alexpls
Copy link
Contributor

alexpls commented Sep 16, 2024

@movermeyer I reckon this'll have been resolved now with #699 being merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@alexpls @movermeyer