FIX: Make set_locale an around_action to avoid leaking between requests (PR #10282)

GitHub

Instead of adding another middleware, perhaps we can use an around_action for set_locale or even an after_action to reset the locale?

@tgxworld yes I would prefer to keep this inside rails if possible. The problem is, what do we ‘reset the locale’ to at the end of a request? The default may vary per-site on a multisite cluster, and we don’t know which site will be requested next.

I guess one option is ‘reset to english’ after every request, but that means all 404 pages will always be served in English, regardless of the site locale.

Shouldn’t we always reset the locale to the default when the app is booted? Setting of the request locale is done per locale. I’m not quite familiar with the code but is there something special about our rails 404 page that prevents us from setting the locale before rendering?

Shouldn’t we always reset the locale to the default when the app is booted?

The problem is that the default locale varies per-site. On boot it is en, then it is changed during a before_action (and also when running sidekiq jobs)

is there something special about our rails 404 page that prevents us from setting the locale before rendering?

It seems like when an ActionController::RoutingError is raised, it does not run any before_actions in application_controller

Although, looking again, I see we manually run current_user and handle_theme for the 404 page. I will try that approach and see how it goes. Thanks @tgxworld

  • Updated to be an around_action
  • manually called that action when rendering errors
  • moved locale specs from topic_controller_spec to application_controller_spec
  • cleaned up some specs which were relying on the leak

The title of this pull request changed from “FIX: Reset locale before each request to avoid leaking on 404 pages” to "FIX: Make set_locale an around_action to avoid leaking between requests

Do we need to check if SiteSetting.allow_user_locale is true here? I see it being set to true in the tests but it doesn’t seem to be used in the controller.

We have to set it true in the tests because set_locale_from_accept_language_header has a validation for it.

SiteSetting.allow_user_locale is not used in the controller directly, but it’s checked in User#effective_locale

Got it :slight_smile: thanks for clarifying.

PR looks good to me :slight_smile: