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

app/controllers/application_controller.rb

@@ -32,7 +32,7 @@ class ApplicationController < ActionController::Base
   before_action :handle_theme
   before_action :set_current_user_for_logs
   before_action :clear_notifications
-  before_action :set_locale
+  around_action :with_resolved_locale
   before_action :set_mobile_view
   before_action :block_if_readonly_mode
   before_action :authorize_mini_profiler
@@ -245,16 +245,19 @@ class ApplicationController < ActionController::Base
       end
     end
 
-    if opts[:custom_message_translated]
+    message = title = nil
-      title = message = opts[:custom_message_translated]
+    with_resolved_locale(check_current_user: false) do
-    elsif opts[:custom_message]
+      if opts[:custom_message_translated]
-      title = message = I18n.t(opts[:custom_message])
+        title = message = opts[:custom_message_translated]
-    else
+      elsif opts[:custom_message]
-      message = I18n.t(type)
+        title = message = I18n.t(opts[:custom_message])
-      if status_code == 403
-        title = I18n.t("page_forbidden.title")
       else
-        title = I18n.t("page_not_found.title")
+        message = I18n.t(type)
+        if status_code == 403
+          title = I18n.t("page_forbidden.title")
+        else
+          title = I18n.t("page_not_found.title")
+        end
       end
     end
 
@@ -263,9 +266,11 @@ class ApplicationController < ActionController::Base
     if show_json_errors
       opts = { type: type, status: status_code }
 
-      # Include error in HTML format for topics#show.
+      with_resolved_locale(check_current_user: false) do
-      if (request.params[:controller] == 'topics' && request.params[:action] == 'show') || (request.params[:controller] == 'categories' && request.params[:action] == 'find_by_slug')
+        # Include error in HTML format for topics#show.
-        opts[:extras] = { html: build_not_found_page(error_page_opts) }
+        if (request.params[:controller] == 'topics' && request.params[:action] == 'show') || (request.params[:controller] == 'categories' && request.params[:action] == 'find_by_slug')
+          opts[:extras] = { html: build_not_found_page(error_page_opts) }
+        end
       end
 
       render_json_error message, opts
@@ -277,9 +282,10 @@ class ApplicationController < ActionController::Base
       rescue Discourse::InvalidAccess
         return render plain: message, status: status_code
       end
-
+      with_resolved_locale do
-      error_page_opts[:layout] = opts[:include_ember] ? 'application' : 'no_ember'
+        error_page_opts[:layout] = opts[:include_ember] ? 'application' : 'no_ember'
-      render html: build_not_found_page(error_page_opts)
+        render html: build_not_found_page(error_page_opts)
+      end
     end
   end
 
@@ -325,19 +331,23 @@ class ApplicationController < ActionController::Base
     end
   end
 
-  def set_locale
+  def with_resolved_locale(check_current_user: true)
-    if !current_user
+    if check_current_user && current_user
+      locale = current_user.effective_locale
+    else
       if SiteSetting.set_locale_from_accept_language_header
         locale = locale_from_header
       else
         locale = SiteSetting.default_locale
       end
-    else
-      locale = current_user.effective_locale
     end
 
-    I18n.locale = I18n.locale_available?(locale) ? locale : SiteSettings::DefaultsProvider::DEFAULT_LOCALE
+    if !I18n.locale_available?(locale)
+      locale = SiteSettings::DefaultsProvider::DEFAULT_LOCALE
+    end
+
     I18n.ensure_all_loaded!
+    I18n.with_locale(locale) { yield }
   end
 
   def store_preloaded(key, json)

spec/requests/admin/site_texts_controller_spec.rb

@@ -533,8 +533,8 @@ RSpec.describe Admin::SiteTextsController do
           }
           expect(response.status).to eq(200)
 
-          expect(Category.find(SiteSetting.staff_category_id).name).to eq(I18n.t("staff_category_name"))
+          expect(Category.find(SiteSetting.staff_category_id).name).to eq(I18n.t("staff_category_name", locale: :de))
-          expect(Topic.find(SiteSetting.guidelines_topic_id).title).to eq(I18n.t("guidelines_topic.title"))
+          expect(Topic.find(SiteSetting.guidelines_topic_id).title).to eq(I18n.t("guidelines_topic.title", locale: :de))
         end
       end
     end

spec/requests/application_controller_spec.rb

@@ -628,4 +628,107 @@ RSpec.describe ApplicationController do
     get "/t/#{topic.slug}/#{topic.id}"
     expect(response.body).to have_tag("link", with: { rel: "canonical", href: "http://test.localhost/t/#{topic.slug}/#{topic.id}" })
   end
+
+  describe "set_locale" do
+    # Using /bootstrap.json because it returns a locale-dependent value
+    def headers(locale)
+      { HTTP_ACCEPT_LANGUAGE: locale }
+    end
+
+    context "allow_user_locale disabled" do
+      context "accept-language header differs from default locale" do
+        before do
+          SiteSetting.allow_user_locale = false
+          SiteSetting.default_locale = "en"
+        end
+
+        context "with an anonymous user" do
+          it "uses the default locale" do
+            get "/bootstrap.json", headers: headers("fr")
+            expect(response.status).to eq(200)
+            expect(response.parsed_body['bootstrap']['locale_script']).to end_with("en.js")
+          end
+        end
+
+        context "with a logged in user" do
+          it "it uses the default locale" do
+            user = Fabricate(:user, locale: :fr)
+            sign_in(user)
+
+            get "/bootstrap.json", headers: headers("fr")
+            expect(response.status).to eq(200)
+            expect(response.parsed_body['bootstrap']['locale_script']).to end_with("en.js")
+          end
+        end
+      end
+    end
+
+    context "set_locale_from_accept_language_header enabled" do
+      context "accept-language header differs from default locale" do
+        before do
+          SiteSetting.allow_user_locale = true
+          SiteSetting.set_locale_from_accept_language_header = true
+          SiteSetting.default_locale = "en"
+        end
+
+        context "with an anonymous user" do
+          it "uses the locale from the headers" do
+            get "/bootstrap.json", headers: headers("fr")
+            expect(response.status).to eq(200)
+            expect(response.parsed_body['bootstrap']['locale_script']).to end_with("fr.js")
+          end
+
+          it "doesn't leak after requests" do
+            get "/bootstrap.json", headers: headers("fr")
+            expect(response.status).to eq(200)
+            expect(response.parsed_body['bootstrap']['locale_script']).to end_with("fr.js")
+            expect(I18n.locale.to_s).to eq(SiteSettings::DefaultsProvider::DEFAULT_LOCALE)
+          end
+        end
+
+        context "with a logged in user" do
+          before do
+            user = Fabricate(:user, locale: :fr)
+            sign_in(user)
+          end
+
+          it "uses the user's preferred locale" do
+            get "/bootstrap.json", headers: headers("fr")
+            expect(response.status).to eq(200)
+            expect(response.parsed_body['bootstrap']['locale_script']).to end_with("fr.js")
+          end
+
+          it "serves a 404 page in the preferred locale" do
+            get "/missingroute", headers: headers("fr")
+            expect(response.status).to eq(404)
+            expected_title = I18n.t("page_not_found.title", locale: :fr)
+            expect(response.body).to include(CGI.escapeHTML(expected_title))
+          end
+        end
+      end
+
+      context "the preferred locale includes a region" do
+        it "returns the locale and region separated by an underscore" do
+          SiteSetting.allow_user_locale = true
+          SiteSetting.set_locale_from_accept_language_header = true
+          SiteSetting.default_locale = "en"
+
+          get "/bootstrap.json", headers: headers("zh-CN")
+          expect(response.status).to eq(200)
+          expect(response.parsed_body['bootstrap']['locale_script']).to end_with("zh_CN.js")
+        end
+      end
+
+      context 'accept-language header is not set' do
+        it 'uses the site default locale' do
+          SiteSetting.allow_user_locale = true
+          SiteSetting.default_locale = 'en'
+
+          get "/bootstrap.json", headers: headers("")
+          expect(response.status).to eq(200)
+          expect(response.parsed_body['bootstrap']['locale_script']).to end_with("en.js")
+        end
+      end
+    end
+  end
 end

spec/requests/topics_controller_spec.rb

@@ -1993,94 +1993,6 @@ RSpec.describe TopicsController do
       end
     end
 
-    describe "set_locale" do
-      def headers(locale)
-        { HTTP_ACCEPT_LANGUAGE: locale }
-      end
-
-      context "allow_user_locale disabled" do
-        context "accept-language header differs from default locale" do
-          before do
-            SiteSetting.allow_user_locale = false
-            SiteSetting.default_locale = "en"
-          end
-
-          context "with an anonymous user" do
-            it "uses the default locale" do
-              get "/t/#{topic.id}.json", headers: headers("fr")
-
-              expect(response.status).to eq(200)
-              expect(I18n.locale).to eq(:en)
-            end
-          end
-
-          context "with a logged in user" do
-            it "it uses the default locale" do
-              user = Fabricate(:user, locale: :fr)
-              sign_in(user)
-
-              get "/t/#{topic.id}.json", headers: headers("fr")
-
-              expect(response.status).to eq(200)
-              expect(I18n.locale).to eq(:en)
-            end
-          end
-        end
-      end
-
-      context "set_locale_from_accept_language_header enabled" do
-        context "accept-language header differs from default locale" do
-          before do
-            SiteSetting.allow_user_locale = true
-            SiteSetting.set_locale_from_accept_language_header = true
-            SiteSetting.default_locale = "en"
-          end
-
-          context "with an anonymous user" do
-            it "uses the locale from the headers" do
-              get "/t/#{topic.id}.json", headers: headers("fr")
-              expect(response.status).to eq(200)
-              expect(I18n.locale).to eq(:fr)
-            end
-          end
-
-          context "with a logged in user" do
-            it "uses the user's preferred locale" do
-              user = Fabricate(:user, locale: :fr)
-              sign_in(user)
-
-              get "/t/#{topic.id}.json", headers: headers("fr")
-              expect(response.status).to eq(200)
-              expect(I18n.locale).to eq(:fr)
-            end
-          end
-        end
-
-        context "the preferred locale includes a region" do
-          it "returns the locale and region separated by an underscore" do
-            SiteSetting.allow_user_locale = true
-            SiteSetting.set_locale_from_accept_language_header = true
-            SiteSetting.default_locale = "en"
-
-            get "/t/#{topic.id}.json", headers: headers("zh-CN")
-            expect(response.status).to eq(200)
-            expect(I18n.locale).to eq(:zh_CN)
-          end
-        end
-
-        context 'accept-language header is not set' do
-          it 'uses the site default locale' do
-            SiteSetting.allow_user_locale = true
-            SiteSetting.default_locale = 'en'
-
-            get "/t/#{topic.id}.json", headers: headers("")
-            expect(response.status).to eq(200)
-            expect(I18n.locale).to eq(:en)
-          end
-        end
-      end
-    end
-
     describe "read only header" do
       it "returns no read only header by default" do
         get "/t/#{topic.id}.json"