Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

[4.1.0.beta1] I18n Missing Translation Handler doesn't work #13429

Open
firedev opened this Issue · 23 comments

8 participants

@firedev

I have this initializers/i18n.rb to make sure english missing translation go through:

module I18n
  class MissingTranslationHandler < ExceptionHandler
    def call(exception, locale, key, options)
      if exception.is_a?(MissingTranslation) && I18n.locale == :en
        key
      else
        super
      end
    end
  end
end

I18n.exception_handler = I18n::MissingTranslationHandler.new

Works in Rails 4.0.1 but in Rails 4.1.0.beta link_to helpers are wrapping the link text in .translation_missing.

Update

After reading the release notes for Rails 4.0.2 I've tried the suggested Error Handler: https://groups.google.com/forum/#!topic/ruby-security-ann/pLrh6DUw998 but it simply doesn't work:

require 'i18n' 

# Override exception handler to more carefully html-escape missing-key results. 
class HtmlSafeI18nExceptionHandler 
  Missing = I18n.const_defined?(:MissingTranslation) ? I18n::MissingTranslation : I18n::MissingTranslationData 

  def initialize(original_exception_handler) 
    @original_exception_handler = original_exception_handler 
  end 

  def call(exception, locale, key, options) 
    if exception.is_a?(Missing) && options[:rescue_format] == :html 
      keys = exception.keys.map { |k| Rack::Utils.escape_html k } 
      key = keys.last.to_s.gsub('_', ' ').gsub(/\b('?[a-z])/) { $1.capitalize } 
      %(<span class="translation_missing" title="translation missing: #{keys.join('.')}">#{key}</span>) 
    else 
      @original_exception_handler.call(exception, locale, key, options) 
    end 
  end 
end 

I18n.exception_handler = HtmlSafeI18nExceptionHandler.new(I18n.exception_handler) 

Even if I remove the body of the call method, so it always returns key, translations are wrapped in span.translation_missing anyway.

I have tried to redefine t() helper like this:

def t key
  I18n.t(key, raise: true)
end

The I18n::MissingTranslationData error is raised, but it's not being intercepted by the handler.

I propose to have a ignore_missing_translation=[:en] setting that would simply ignore missing translations for specified locales. After all this is the point of overriding the error handler.

@carlosantoniodasilva

Have you tested in 4.0.2? Also, which I18n version are you using? Thanks.

@pftg

Confirmed for rails 4.0.2, i18n is 0.6.9, the same issue happen.

@pftg

Found that raise set to true as default, instead of passing it to the handler. Working on patch

@pftg

Sorry, found that this is expected solution, to prevent security bugs. As I got from commit message 0c7ac34:

Stop using i18n's built in HTML error handling.

i18n doesn't depend on active support which means it can't use our html_safe
code to do its escaping when generating the spans.  Rather than try to sanitize
the output from i18n, just revert to our old behaviour of rescuing the error
and constructing the tag ourselves.

Fixes: CVE-2013-4491

You should add option raise: true to handle exception by rails way. I think need to update documentation because of that. So I18n.exception_handler = I18n::MissingTranslationHandler.new is not working from 4.0.2.

@pftg

Please ignore me, above. I've miss-read the commit code. I assume only rescue_format is dropped off.

@firedev

I don't get it, how should I do it then? Edge Rails Guides suggest the old way to do it: http://edgeguides.rubyonrails.org/i18n.html#using-different-exception-handlers

PS. The issue appears in 4.0.2, works in 4.0.1.

@firedev

After reading the release notes I've tried the suggested Error Handler: https://groups.google.com/forum/#!topic/ruby-security-ann/pLrh6DUw998 in 4.0.2 but it simply doesn't work. Even if I remove the body of the call method, so it always returns key, translations are wrapped in span.translation_missing anyway.

Why not make fallback a default behaviour in I18n module?

@pftg

@firedev to override handler you should use t('.missing', raise: true). Then will be raised exception, which you should handle/catch yourselves.

@NZKoz, I think there is a way to support I18n.exception_handler, should we add patch to return supporting of it, or should this feature be removed, and updated docs that I18n.exception_handler is not working anymore?

/cc @carlosantoniodasilva

@firedev

@pftg It doesn't work. I mean the I18n::MissingTranslationData error is raised, but it's not being intercepted by the handler.

The whole point of messing with this is to let missing translation slip for I18n.default_locale. What about having this functionality built-in? There is config.i18n.fallbacks = [:en] now, it could be a similar fallback setting for translations.

@fguillen

Same here:

  • Rails: 4.0.2
  • I18n: 0.6.9
@fguillen

@pftg you suggest to add :raise => true in all our I18n.t calls? this is too ugly because normally is all the translations or none.

Also the I18n.exception_handler system makes easier to configure behaviour depending in the environment

@pftg

@fguillen you are right, I just show that there is for now only this way. I think I saw PR which setup raise: true for all translations. Have you reviewed opened issues/PR?

@fguillen

@pftg Thanks for the suggestion, I'll take a look.

@fguillen

Looks like this is the PR: #13832

But I don't know in which version of Rails is it merged.

@carlosantoniodasilva

It's been merged to master only (next 4.1 version)

@firedev

So is there a way to suppress the missing_translation for english in Rails 4.1?

Ttried to add

config.action_view.raise_on_missing_translations = false

To application.rb, nothing changed.

@canercak

same for me

@firedev

In the end I just got fed up with this and re-translated the whole app including english locale.

@robin850 robin850 added the regression label
@codev

I'm confused by this. Since 4.1 my exception handler has stopped being called.

The PR #13832 and before it #13183 both set options[:raise] to true for all calls to I18n.translate in - what I think is - the mistaken belief that this will cause an exception that can be caught by the exception handler documented in http://guides.rubyonrails.org/i18n.html#using-different-exception-handlers

However setting options[:raise] when calling I18n.translate actually causes an exception to be raised and the handler set in config.exception_handler to be bypassed. You need to call translate with options[:raise] and options[:throw] set to false in order to use the exception handling mechanism in I18n, take a look at line 316 of i18n.rb

For me I'm just monkey patching actionview/lib/action_view/helpers/translation_helper.rb translate to delete these three lines:

        unless raise_error
          options[:raise] = true
        end

Doing that makes the I18n.exception_handler get called when translating and so the guide makes sense again and you can override the annoying span formatting for missing translations.

@codev

Here's the rather ugly patch to make I18n.exception_handler work as per the docs, make sure you haven't set config.action_view.raise_on_missing_translations to true and stick this in config/initializers/i18n_patch_13429.rb

# Patch up the translation helper so that I18n.exception_handler is called as per documentation
# instead of an error being raised that isn't caught by the provided exception handler
# See https://github.com/rails/rails/issues/13429
module ActionView
  module Helpers
    module TranslationHelper
      def translate(key, options = {})
        options[:default] = wrap_translate_defaults(options[:default]) if options[:default]

        # If the user has specified rescue_format then pass it all through, otherwise use
        # raise and do the work ourselves
        options[:raise] ||= ActionView::Base.raise_on_missing_translations

        raise_error = options[:raise] || options.key?(:rescue_format)
        # The next three lines are commented out - this is the only change
        #unless raise_error
        #  options[:raise] = true
        #end

        if html_safe_translation_key?(key)
          html_safe_options = options.dup
          options.except(*I18n::RESERVED_KEYS).each do |name, value|
            unless name == :count && value.is_a?(Numeric)
              html_safe_options[name] = ERB::Util.html_escape(value.to_s)
            end
          end
          translation = I18n.translate(scope_key_by_partial(key), html_safe_options)

          translation.respond_to?(:html_safe) ? translation.html_safe : translation
        else
          I18n.translate(scope_key_by_partial(key), options)
        end
      rescue I18n::MissingTranslationData => e
        raise e if raise_error

        keys = I18n.normalize_keys(e.locale, e.key, e.options[:scope])
        content_tag('span', keys.last.to_s.titleize, :class => 'translation_missing', :title => "translation missing: #{keys.join('.')}")
      end
    end
  end
end
@mockdeep

We're trying to raise exceptions in dev mode and nothing we try works, either. Even calling with raise doesn't raise the exception:

t('bloo.blah', raise: true)

I can see that handling is set to :raise inside of i18n.rb, but the exception seems to be getting swallowed up somewhere. In this case it just renders the text Blah without complaint.

@mockdeep

Using rails 3.2.18.

@phillipoertel phillipoertel referenced this issue from a commit in typus/typus
@phillipoertel phillipoertel Initializer for i18n.
For missing translations it will do locale fallbacks in production,
all other environments will raise an exception (so problems can be fixed quickly).
bab9145
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.