From 53210841c88a12af537154d942b07d2f72cf6f88 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Lo=C3=AFc=20Guitaut?= <loic@discourse.org>
Date: Thu, 25 Jul 2024 16:56:08 +0200
Subject: [PATCH] FIX: Validate MF strings when adding overrides
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Currently, when adding translation overrides, values aren’t validated
for MF strings. This results in being able to add invalid plural keys or
even strings containing invalid syntax.

This patch addresses this issue by compiling the string when saving an
override if the key is detected as an MF one.

If there’s an error from the compiler, it’s added to the model errors,
which in turn is displayed to the user in the admin UI, helping them to
understand what went wrong.
---
 app/models/translation_override.rb       | 13 ++++++++
 spec/lib/js_locale_helper_spec.rb        | 22 +++++--------
 spec/models/translation_override_spec.rb | 42 ++++++++++++++++++++++++
 3 files changed, 64 insertions(+), 13 deletions(-)

diff --git a/app/models/translation_override.rb b/app/models/translation_override.rb
index 5862dc5dc7c..5d693ff8534 100644
--- a/app/models/translation_override.rb
+++ b/app/models/translation_override.rb
@@ -44,6 +44,7 @@ class TranslationOverride < ActiveRecord::Base
   validates_presence_of :locale, :translation_key, :value
 
   validate :check_interpolation_keys
+  validate :check_MF_string, if: :message_format?
 
   attribute :status, :integer
   enum status: { up_to_date: 0, outdated: 1, invalid_interpolation_keys: 2, deprecated: 3 }
@@ -165,6 +166,10 @@ class TranslationOverride < ActiveRecord::Base
     I18n.overrides_disabled { I18n.t(transformed_key, locale: :en) }
   end
 
+  def message_format?
+    translation_key.to_s.end_with?("_MF")
+  end
+
   private
 
   def transformed_key
@@ -185,6 +190,14 @@ class TranslationOverride < ActiveRecord::Base
       ),
     )
   end
+
+  def check_MF_string
+    require "messageformat"
+
+    MessageFormat.compile(locale, { key: value }, strict: true)
+  rescue MessageFormat::Compiler::CompileError => e
+    errors.add(:base, e.cause.message)
+  end
 end
 
 # == Schema Information
diff --git a/spec/lib/js_locale_helper_spec.rb b/spec/lib/js_locale_helper_spec.rb
index b918468f5cb..76d77d99e78 100644
--- a/spec/lib/js_locale_helper_spec.rb
+++ b/spec/lib/js_locale_helper_spec.rb
@@ -155,20 +155,10 @@ RSpec.describe JsLocaleHelper do
       )
     end
     fab!(:overriden_translation_ja) do
-      Fabricate(
-        :translation_override,
-        locale: "ja",
-        translation_key: "js.posts_likes_MF",
-        value: "{ count, plural, one {返信 # 件、} other {返信 # 件、} }",
-      )
+      Fabricate(:translation_override, locale: "ja", translation_key: "js.posts_likes_MF")
     end
     fab!(:overriden_translation_he) do
-      Fabricate(
-        :translation_override,
-        locale: "he",
-        translation_key: "js.posts_likes_MF",
-        value: "{ count, plural, ",
-      )
+      Fabricate(:translation_override, locale: "he", translation_key: "js.posts_likes_MF")
     end
     let(:output) { described_class.output_MF(locale).gsub(/^import.*$/, "") }
     let(:generated_locales) { v8_ctx.eval("Object.keys(I18n._mfMessages._data)") }
@@ -176,7 +166,13 @@ RSpec.describe JsLocaleHelper do
       v8_ctx.eval("I18n._mfMessages.get('posts_likes_MF', {count: 3, ratio: 'med'})")
     end
 
-    before { v8_ctx.eval(output) }
+    before do
+      overriden_translation_ja.update_columns(
+        value: "{ count, plural, one {返信 # 件、} other {返信 # 件、} }",
+      )
+      overriden_translation_he.update_columns(value: "{ count, plural, ")
+      v8_ctx.eval(output)
+    end
 
     context "when locale is 'en'" do
       let(:locale) { "en" }
diff --git a/spec/models/translation_override_spec.rb b/spec/models/translation_override_spec.rb
index 4d5ff7d5365..a8da3b88b4a 100644
--- a/spec/models/translation_override_spec.rb
+++ b/spec/models/translation_override_spec.rb
@@ -148,6 +148,32 @@ RSpec.describe TranslationOverride do
         end
       end
     end
+
+    describe "MessageFormat translations" do
+      subject(:override) do
+        described_class.new(
+          translation_key: "admin_js.admin.user.delete_all_posts_confirm_MF",
+          locale: "en",
+        )
+      end
+
+      it do
+        is_expected.to allow_value(
+          "This has {COUNT, plural, one{one member} other{# members}}.",
+        ).for(:value).against(:base)
+      end
+      it do
+        is_expected.not_to allow_value(
+          "This has {COUNT, plural, one{one member} many{# members} other{# members}}.",
+        ).for(:value).with_message(/plural case many is not valid/, against: :base)
+      end
+      it do
+        is_expected.not_to allow_value("This has {COUNT, ").for(:value).with_message(
+          /invalid syntax/,
+          against: :base,
+        )
+      end
+    end
   end
 
   it "upserts values" do
@@ -340,4 +366,20 @@ RSpec.describe TranslationOverride do
       expect(translation.invalid_interpolation_keys).to contain_exactly("foo")
     end
   end
+
+  describe "#message_format?" do
+    subject(:override) { described_class.new(translation_key: key) }
+
+    context "when override is for a MessageFormat translation" do
+      let(:key) { "admin_js.admin.user.delete_all_posts_confirm_MF" }
+
+      it { is_expected.to be_a_message_format }
+    end
+
+    context "when override is not for a MessageFormat translation" do
+      let(:key) { "admin_js.type_to_filter" }
+
+      it { is_expected.not_to be_a_message_format }
+    end
+  end
 end