Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions app/controllers/application_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,8 @@ def set_locale
else
SiteSetting.default_locale
end

I18n.fallbacks.ensure_loaded!

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking — ensure_loaded! propagates YAML load errors with no graceful handling, turning a single corrupt fallback file into a 500 on every request.

FallbackLocaleList#ensure_loaded! → I18n.ensure_loaded! → load_locale → I18n.backend.load_translations → load_file/load_yml, which re-raises malformed YAML as I18n::InvalidLocaleData (i18n 0.7.0 base.rb). None of load_locale, I18n.ensure_loaded!, or FallbackLocaleList#ensure_loaded! rescue it, and ApplicationController declares no rescue_from for I18n::InvalidLocaleData / Psych::SyntaxError. Because the fallback chain always includes :en and SiteSetting.default_locale, a single corrupt always-present fallback YAML (e.g. en.yml) takes down the entire site on every request — a broader blast radius than the previous lazy single-locale loading, which only loaded config.locale and only for the user's own locale. Consider skipping a broken fallback tier and continuing with the next (degrading gracefully) rather than failing the whole request, plus a log line.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Non-blocking — corrupt fallback YAML → 500 on every request, no circuit breaker.

I18n.fallbacks.ensure_loaded! → I18n.ensure_loaded! → load_locale → backend.load_translations → load_file, which re-raises malformed YAML as I18n::InvalidLocaleData / Psych::SyntaxError. This runs on every request via set_locale. There is no rescue_from for those classes in ApplicationController (verified: only RenderEmpty, RateLimiter::LimitExceeded, PG::ReadOnlySqlTransaction, Discourse::NotLoggedIn/NotFound/InvalidAccess/ReadOnly). Because :en is unconditionally in the chain, a single corrupt en.yml takes down every request for every user and every multisite site. load_locale skips the @loaded_locales << locale push on exception, so there's no circuit breaker — it retries and 500s on every request until the file is fixed.

# Wrap the preload so a corrupt file degrades gracefully:
I18n.fallbacks.ensure_loaded! rescue nil
# or add a dedicated rescue_from for I18n::InvalidLocaleData / Psych::SyntaxError

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Non-blocking — fallbacks only wired into the web request path; jobs / with_locale / rake are silently half-wired.

ensure_loaded! is called only from set_locale. Background jobs (app/jobs/base.rb:151) set I18n.locale = SiteSetting.default_locale but never call ensure_loaded!, and the accelerator's translate (translate_accelerator.rb:67) only load_locales config.locale (the primary) — never the rest of the fallback chain. So in a job, a key missing from the primary locale falls through to :en (or the site default), which was never loaded → MissingTranslationData instead of the fallback string. Email/notification translation regresses vs. the web path. Same gap affects I18n.with_locale blocks and rake tasks.

# Option A — preload in jobs too (app/jobs/base.rb after I18n.locale = ...):
I18n.fallbacks.ensure_loaded!

# Option B — have translate load the whole chain, not just the primary:
# translate_accelerator.rb translate():
I18n.fallbacks[config.locale].each { |l| load_locale(l) unless @loaded_locales.include?(l) }

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Non-blocking — fallbacks are wired into the web request path only.

ensure_loaded! is called solely from set_locale. Background jobs set I18n.locale = SiteSetting.default_locale directly (app/jobs/base.rb:151) and I18n.with_locale blocks (lib/post_destroyer.rb:102) / rake tasks (lib/tasks/db.rake) do the same, without preloading the fallback chain. The accelerator's translate (line 68) on-demand-loads only config.locale (the primary), never the rest of I18n.fallbacks[locale], so a key missing from the primary locale in a job yields a MissingTranslation/'translation missing' string rather than the fallback — emails and notifications regress versus the web path. Either call I18n.fallbacks.ensure_loaded! wherever I18n.locale is set for a non-request context, or have translate load the whole I18n.fallbacks[config.locale] chain.

end

def store_preloaded(key, json)
Expand Down
5 changes: 0 additions & 5 deletions config/cloud/cloud66/files/production.rb
Original file line number Diff line number Diff line change
Expand Up @@ -23,11 +23,6 @@
# Specifies the header that your server uses for sending files
config.action_dispatch.x_sendfile_header = 'X-Accel-Redirect' # for nginx

# Enable locale fallbacks for I18n (makes lookups for any locale fall back to
# the I18n.default_locale when a translation can not be found)
config.i18n.fallbacks = true


# you may use other configuration here for mail eg: sendgrid

config.action_mailer.delivery_method = :smtp
Expand Down
4 changes: 0 additions & 4 deletions config/environments/production.rb
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,6 @@

config.log_level = :info

# Enable locale fallbacks for I18n (makes lookups for any locale fall back to
# the I18n.default_locale when a translation can not be found)
config.i18n.fallbacks = true

if GlobalSetting.smtp_address
settings = {
address: GlobalSetting.smtp_address,
Expand Down
4 changes: 0 additions & 4 deletions config/environments/profile.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,6 @@
# Specifies the header that your server uses for sending files
config.action_dispatch.x_sendfile_header = 'X-Accel-Redirect' # for nginx

# Enable locale fallbacks for I18n (makes lookups for any locale fall back to
# the I18n.default_locale when a translation can not be found)
config.i18n.fallbacks = true

# we recommend you use mailcatcher https://github.com/sj26/mailcatcher
config.action_mailer.smtp_settings = { address: "localhost", port: 1025 }

Expand Down
24 changes: 24 additions & 0 deletions config/initializers/i18n.rb
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
# order: after 02-freedom_patches.rb

# Include pluralization module
require 'i18n/backend/pluralization'
I18n::Backend::Simple.send(:include, I18n::Backend::Pluralization)

# Include fallbacks module
require 'i18n/backend/fallbacks'
I18n.backend.class.send(:include, I18n::Backend::Fallbacks)

# Configure custom fallback order
class FallbackLocaleList < Hash

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Non-blocking — FallbackLocaleList drops the I18n::Locale::Fallbacks extension API.

Pre-PR, config.i18n.fallbacks = true gave Rails an I18n::Locale::Fallbacks instance whose documented API includes .map(:ca => :es), .defaults=, and parent-locale computation (e.g. :"es-MX" -> [:"es-MX", :es, :en]). FallbackLocaleList < Hash overrides only []/ensure_loaded!, so I18n.fallbacks.map(...) now raises NoMethodError — notable because this file's own TODO (fallback zh_TW to zh_CN) anticipates exactly that kind of mapping. Core has no current callers (verified by grep), so this is plugin-facing. Subclass and override only [] to preserve the API:

class FallbackLocaleList < I18n::Locale::Fallbacks
  def [](locale)
    [locale.to_sym, SiteSetting.default_locale.to_sym, :en].uniq.compact
  end
  def ensure_loaded!; self[I18n.locale].each { |l| I18n.ensure_loaded! l }; end
end

def [](locale)
# user locale, site locale, english
# TODO - this can be extended to be per-language for a better user experience
# (e.g. fallback zh_TW to zh_CN / vice versa)
[locale, SiteSetting.default_locale.to_sym, :en].uniq.compact

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking — string/symbol mismatch defeats the .uniq dedup, causing a redundant double-load of the site-default locale.

I18n.locale is a String here: set_locale assigns current_user.effective_locale / SiteSetting.default_locale, both strings (i18n 0.7.0's I18n.locale= stores the value verbatim, no to_sym). But SiteSetting.default_locale.to_sym and :en are Symbols. Array#uniq uses eql?/hash, and "en".eql?(:en) is false, so when the user locale equals the site default (or is en) the chain is e.g. ["en", :en, :en].uniq => ["en", :en] rather than [:en].

Downstream, ensure_loaded! (line 21) calls I18n.ensure_loaded! for both "en" and :en; the accelerator guards @loaded_locales with exact == equality (translate_accelerator.rb:48/64), so the same locale's YAML is parsed and loaded twice on the first request after every boot/reload, and @loaded_locales permanently holds mixed-type duplicates. Translations still resolve via the first matching fallback, so this is a perf/dedup defect, not a correctness break. (Note: i18n's own I18n::Locale::Fallbacks#[] normalizes with to_sym — this custom class should too.) Fix: [locale.to_sym, SiteSetting.default_locale.to_sym, :en].uniq.compact.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Blocking — multisite cache poisoning: cross-site translation leakage.

The fallback chain is now per-site ([locale, SiteSetting.default_locale.to_sym, :en]), so translate_no_cache produces site-dependent results for the same (key, locale). But the translate accelerator caches results in a single process-wide LruRedux::ThreadSafeCache on the I18n module singleton, keyed only "#{key}#{config.locale}#{config.backend.object_id}" (translate_accelerator.rb:72). I18n::Config#backend is a class variable (@@backend in i18n 0.7.0) → object_id is identical for every multisite site, and config.locale collides across sites whenever two sites have a user with the same locale.

Result: site A (default fr) warms a cache entry whose fallback resolved to a French string; site B (default es) with a de-locale user hits the same key and gets site A's French fallback served — a tenancy-isolation failure reachable via every no-arg I18n.t('key') call.

Pre-PR this was safe because config.i18n.fallbacks = true fell back to the global I18n.default_locale (:en) — site-independent. This PR introduces the site-dependence that breaks the cache invariant.

# translate_accelerator.rb — include the site discriminator:
k = "#{key}#{config.locale}#{config.backend.object_id}#{SiteSetting.default_locale}"

Or make @cache per-site (keyed by RailsMultisite::ConnectionManagement.current_db).

end

def ensure_loaded!

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Non-blocking — no test coverage for the new fallback feature.

grep -r 'FallbackLocaleList|ensure_loaded|fallbacks' spec/ returns zero matches; the accelerator itself has no existing specs either. A feature that changes translation loading on every request, touches the shared multisite LRU cache, and runs in all environments should have regression guards. Critical flows to cover:

  1. Fallback chain order: FallbackLocaleList[locale] == [locale, default_locale.to_sym, :en] with dedup.
  2. Multisite cache isolation — assert two sites with different default_locale don't share cached fallback translations (the blocking bug above).
  3. ensure_loaded! idempotency — repeated calls don't reload.
  4. Missing-locale-file behavior — ensure_loaded! doesn't crash when a fallback yml is absent.
  5. Job-path fallback — a job that hits a missing key resolves via the fallback chain.

Note: spec/spec_helper.rb sets I18n.locale = :en (Symbol) before each test, so the real request-path locale flow is never exercised — adding tests here would also close that gap.

self[I18n.locale].each { |l| I18n.ensure_loaded! l }
end
end
I18n.fallbacks = FallbackLocaleList.new

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Non-blocking — FallbackLocaleList replaces I18n::Locale::Fallbacks, dropping the .map / .defaults= plugin API.

In i18n 0.7.0 the default I18n.fallbacks is an I18n::Locale::Fallbacks instance whose documented API includes .map(:ca => :"es-ES"), .defaults=, and parent-locale computation. FallbackLocaleList defines only [] and ensure_loaded!. A plugin using the documented I18n.fallbacks.map(...) to register custom mappings (e.g. zh_TW -> zh_CN, which this file's own TODO anticipates) would now raise NoMethodError. Core Discourse doesn't use these methods (verified by grep), so this is a plugin-facing contract change.

# Subclass and override only [] to preserve the extension API:
class FallbackLocaleList < I18n::Locale::Fallbacks
  def [](locale)
    [locale.to_sym, SiteSetting.default_locale.to_sym, :en].uniq.compact
  end
  # ...ensure_loaded!...
end

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Non-blocking — no test coverage for the new fallback feature.

grep -r 'FallbackLocaleList|ensure_loaded|fallbacks' spec/ returns zero matches, and the accelerator has no existing specs. A feature that changes translation loading on every request and touches the shared multisite LRU cache should have regression guards: (1) FallbackLocaleList[locale] == [locale, default_locale.to_sym, :en] with dedup; (2) multisite cache isolation (the blocking bug above); (3) ensure_loaded! idempotency; (4) missing-locale-file behavior doesn't crash; (5) job-path fallback resolves via the chain.

2 changes: 0 additions & 2 deletions config/initializers/pluralization.rb

This file was deleted.

5 changes: 5 additions & 0 deletions lib/freedom_patches/translate_accelerator.rb
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,11 @@ def load_locale(locale)
end

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking — a missing/misnamed fallback locale file is silently marked loaded with zero translations, no observability.

Inside load_locale above this end: I18n.load_path.grep(Regexp.new("\\.#{locale}\\.yml$")) (line 56) returns [] when no file matches, and the overridden load_translations (lines 23-26) is a no-op for an empty list — yet @loaded_locales << locale (line 58) still runs. So a fallback locale whose file isn't on I18n.load_path (a default_locale whose [locale].yml is missing, or a typo'd locale) is permanently recorded as loaded with zero translations, never retried, with no error and no log. The only symptom is unexpected 'translation missing' strings. This PR's new ensure_loaded! (line 62) eagerly drives load_locale for every fallback locale, surfacing this silent degradation for the fallback chain. Consider checking that the grep matched at least one file (or that the locale loaded non-empty translations) and logging a warning when it didn't.

end

def ensure_loaded!(locale)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Non-blocking — missing locale file silently marked loaded with zero translations.

load_locale (line 56) greps I18n.load_path.grep(/\.#{locale}\.yml$/); when no file matches it returns [], and the overridden load_translations (lines 23-26) is a no-op for an empty list — yet @loaded_locales << locale (line 58) still runs. So a fallback locale whose yml is missing (a default_locale with no [locale].yml, or a typo) is permanently recorded as loaded with zero translations, never retried, with no error and no log. The new ensure_loaded! force-loads the full chain on every request, so this silent-empty-load now happens eagerly per request. Only symptom: unexpected 'translation missing' strings with no operator signal.

# In load_locale, log when a locale resolves to no files:
files = I18n.load_path.grep(Regexp.new("\\.#{locale}\\.yml$"))
Rails.logger.warn("I18n: no translation files found for locale #{locale}") if files.empty?
I18n.backend.load_translations(files)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Non-blocking — no circuit breaker: a corrupt fallback YAML 500s every request.

ensure_loaded! -> load_locale -> backend.load_translations -> load_file re-raises malformed YAML as I18n::InvalidLocaleData / Psych::SyntaxError, and set_locale runs this on every request. ApplicationController declares no rescue_from for those classes, and :en is unconditionally in the fallback chain, so a single corrupt en.yml takes down every request for every user (and every multisite site) until the file is fixed. load_locale skips the @loaded_locales << locale push on exception, so there is no caching of the failure — it reloads and 500s on every request. Consider wrapping the preload (I18n.fallbacks.ensure_loaded! rescue nil) or adding a rescue_from so a corrupt file degrades to missing-translation strings rather than a hard 500.

@loaded_locales ||= []
load_locale locale unless @loaded_locales.include?(locale)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Non-blocking — fallbacks silently no-op (and can poison the shared cache) outside the HTTP request path.

This new ensure_loaded! is called only from ApplicationController#set_locale (application_controller.rb:159). Background jobs set I18n.locale = SiteSetting.default_locale directly (app/jobs/base.rb:151) without loading fallbacks, and I18n.with_locale blocks (lib/post_destroyer.rb:102, app/services/post_alerter.rb) and rake tasks do likewise. The accelerator's translate (line 68) on-demand-loads only config.locale, never the fallback chain. In such a context, Fallbacks#translate iterates the chain but each fallback locale's file was never loaded → throw(:exception, MissingTranslation) → the default handler returns the 'translation missing' string, which is .freeze'd and cached under key+config.locale on the shared global LRU. A job/console running before the matching web request thus caches 'translation missing' for that key+locale, defeating fallback resolution for web requests too until LRU eviction.

Fix: eager-load the fallback chain wherever I18n.locale is set for a non-request context (or have translate load I18n.fallbacks[config.locale] instead of only config.locale).

(Note: fallbacks are not bypassed by the translate_no_cache alias — it delegates dynamically to config.backend.translate, which is Fallbacks#translate once the module is included into the backend class. So the mechanism works in web requests once the cache-key issue is fixed.)

end

def translate(key, *args)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Blocking — multisite cache poisoning: cross-site translation leakage.

translate caches the no-arg result in a process-wide LruRedux::ThreadSafeCache (@cache on the I18n singleton, line 71) keyed "#{key}#{config.locale}#{config.backend.object_id}" (line 72). I18n::Config#backend is a class variable (@@backend in i18n 0.7.0), so backend.object_id is identical for every multisite site, and @cache is shared across all threads — the key is effectively key + locale. Pre-PR, config.i18n.fallbacks = true fell back to the global :en, so the cached result was site-independent. This PR makes the fallback target the per-site SiteSetting.default_locale (FallbackLocaleList#[]), so the resolved string for the same (key, locale) now differs per site, but the cache key still doesn't capture the site. Result: site A (default_locale :de) warms "key:pl:<backend>" with a German fallback string; site B (default_locale :fr) with a :pl user hits that entry and is served German instead of French — a tenancy-isolation failure on every I18n.t('key').

# translate_accelerator.rb — include the site discriminator in the key:
k = "#{key}#{config.locale}#{config.backend.object_id}#{SiteSetting.default_locale}"

(Verified against i18n 0.7.0: Config#locale= symbolizes, Config#backend is @@backend, and I18n.translate delegates to backend.translate -> Backend::Fallbacks#translate, so the cached value is the fallback-resolved string.)

load_locale(config.locale) unless @loaded_locales.include?(config.locale)
return translate_no_cache(key, *args) if args.length > 0

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking — cache key omits the per-site fallback target, causing cross-site translation leakage on multisite.

A few lines below this, the LRU key (line 72) is "#{key}#{config.locale}#{config.backend.object_id}". I18n::Config#backend is a class variable (@@backend in i18n 0.7.0), shared across all threads and all multisite sites, so backend.object_id is identical for every site; @cache (line 71) is one shared LruRedux::ThreadSafeCache on the I18n singleton. Pre-PR, config.i18n.fallbacks = true made Rails fall back to the global I18n.default_locale (:en) — the same target for every site — so the key was sufficient. This PR changes the fallback target to the per-site SiteSetting.default_locale.to_sym (via FallbackLocaleList#[]), but the cache key still doesn't capture it.

Concrete failure: Site A (default_locale :de) and Site B (default_locale :fr) both have a Polish user (:pl). A key missing from pl.yml resolves via fallback. Whichever site first translates that key caches the result under "<key>:pl:<backend_id>"; the other site's Polish user then gets a cache hit and receives the wrong language (e.g. Site B's user sees German). The 300-entry LRU only churns on cold keys, so hot UI strings stay wrong until eviction. Separately, I18n.reload! (the only cache clearer, line 41) is wired by Rails to a FileUpdateChecker over I18n.load_path — never to a SiteSetting change — so within a single site a runtime default_locale change also serves stale cached fallback results.

Fix: include the resolved fallback list (or SiteSetting.default_locale, or a site id such as RailsMultisite::ConnectionManagement.current_db) in the cache key, and clear @cache when default_locale changes.

Expand Down