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

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

diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index eaaa515..e142ac0 100644
--- a/app/controllers/application_controller.rb
+++ b/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]
-      title = message = opts[:custom_message_translated]
-    elsif opts[:custom_message]
-      title = message = I18n.t(opts[:custom_message])
-    else
-      message = I18n.t(type)
-      if status_code == 403
-        title = I18n.t("page_forbidden.title")
+    message = title = nil
+    with_resolved_locale(check_current_user: false) do
+      if opts[:custom_message_translated]
+        title = message = opts[:custom_message_translated]
+      elsif opts[:custom_message]
+        title = message = I18n.t(opts[:custom_message])
       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.
-      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) }
+      with_resolved_locale(check_current_user: false) do
+        # Include error in HTML format for topics#show.
+        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
-
-      error_page_opts[:layout] = opts[:include_ember] ? 'application' : 'no_ember'
-      render html: build_not_found_page(error_page_opts)
+      with_resolved_locale do
+        error_page_opts[:layout] = opts[:include_ember] ? 'application' : 'no_ember'
+        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
-    if !current_user
+  def with_resolved_locale(check_current_user: true)
+    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)
diff --git a/spec/requests/admin/site_texts_controller_spec.rb b/spec/requests/admin/site_texts_controller_spec.rb
index aea6653..22fbe7a 100644
--- a/spec/requests/admin/site_texts_controller_spec.rb
+++ b/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(Topic.find(SiteSetting.guidelines_topic_id).title).to eq(I18n.t("guidelines_topic.title"))
+          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", locale: :de))
         end
       end
     end
diff --git a/spec/requests/application_controller_spec.rb b/spec/requests/application_controller_spec.rb
index 0e3fe9a..4d832e7 100644
--- a/spec/requests/application_controller_spec.rb
+++ b/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)

[... diff too long, it was truncated ...]

GitHub sha: bcb0e623

This commit appears in #10282 which was merged by davidtaylorhq.