Merge pull request #23251 from opf/fix/use-message-pack-cache-serializer

Use MessagePack serializer that treats old marshal cache as miss

config/application.rb

@@ -222,7 +222,7 @@ module OpenProject
     config.log_level = OpenProject::Configuration["log_level"].to_sym
 
     # Enable the Rails 7 cache format
-    config.active_support.cache_format_version = 7.0
+    config.active_support.cache_format_version = 7.1
 
     config.after_initialize do
       Settings::Definition.add_all

lib_static/open_project/configuration.rb

@@ -37,6 +37,25 @@ module OpenProject
 
     TRUE_VALUES = ["true", true, "1"].freeze
 
+    # Reject cache payloads that do not use MessagePack's own framing.
+    # This prevents SerializerWithFallback from invoking Marshal on attacker-controlled bytes.
+    module StrictMessagePackCoder
+      extend self
+
+      BASE = ActiveSupport::Cache::SerializerWithFallback::MessagePackWithFallback
+
+      def dump(entry) = BASE.dump(entry)
+      def dump_compressed(entry, threshold) = BASE.dump_compressed(entry, threshold)
+
+      def load(dumped)
+        return nil unless dumped.is_a?(String) && BASE.dumped?(dumped)
+        
+        # Avoid using BASE#load as that goes through all the fallbacks again
+        # https://github.com/rails/rails/blob/v8.1.3/activesupport/lib/active_support/cache/serializer_with_fallback.rb#L17
+        BASE._load(dumped)
+      end
+    end
+
     class << self
       # Returns a configuration setting
       def [](name)
@@ -67,7 +86,16 @@ module OpenProject
           end
 
         parameters = cache_store_parameters
-        cache_config << parameters unless parameters.empty?
+        if %i[memcache dalli_store redis].include?(cache_store)
+          parameters[:serializer] ||= StrictMessagePackCoder
+        end
+        unless parameters.empty?
+          if cache_config.last.is_a?(Hash)
+            cache_config[-1] = cache_config.last.merge(parameters)
+          else
+            cache_config << parameters
+          end
+        end
 
         cache_config
       end

spec/lib/open_project/configuration_spec.rb

@@ -82,12 +82,51 @@ RSpec.describe OpenProject::Configuration, :settings_reset do
           it "sets the cache to :redis_cache_store" do
             expect(subject.first).to eq(:redis_cache_store)
           end
+
+          it "uses a strict serializer that rejects Marshal payloads" do
+            serializer_options = subject.grep(Hash).find { |options| options[:serializer].present? }
+            expect(serializer_options).to include(serializer: described_class::StrictMessagePackCoder)
+          end
+
+          it "merges serializer into redis options hash" do
+            expect(subject).to have_attributes(length: 2)
+            expect(subject.second).to include(:url, :error_handler, serializer: described_class::StrictMessagePackCoder)
+          end
         end
 
         it "raises an error trying to set redis without an URL" do
           expect { subject }.to raise_error(ArgumentError, /CACHE_REDIS_URL is not set/)
         end
       end
+
+      context "setting rails cache to memcache",
+              with_config: { "rails_cache_store" => "memcache", "cache_memcache_server" => "localhost:11211" } do
+        it "sets the cache to :mem_cache_store" do
+          expect(subject.first).to eq(:mem_cache_store)
+        end
+
+        it "uses a strict serializer that rejects Marshal payloads" do
+          serializer_options = subject.grep(Hash).find { |options| options[:serializer].present? }
+          expect(serializer_options).to include(serializer: described_class::StrictMessagePackCoder)
+        end
+      end
+    end
+  end
+
+  describe OpenProject::Configuration::StrictMessagePackCoder do
+    subject(:coder) { described_class }
+
+    it "roundtrips MessagePack-serialized data" do
+      value = { "key" => "value" }
+      dumped = coder.dump(value)
+
+      expect(coder.load(dumped)).to eq(value)
+    end
+
+    it "treats Marshal payloads as cache misses" do
+      dumped = Marshal.dump({ "key" => "value" })
+
+      expect(coder.load(dumped)).to be_nil
     end
   end