diff --git a/config/application.rb b/config/application.rb index 9b23c86aac0..9b098e9ea44 100644 --- a/config/application.rb +++ b/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 diff --git a/lib_static/open_project/configuration.rb b/lib_static/open_project/configuration.rb index 85f78a64f5a..d2a6ec56618 100644 --- a/lib_static/open_project/configuration.rb +++ b/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 diff --git a/spec/lib/open_project/configuration_spec.rb b/spec/lib/open_project/configuration_spec.rb index 7a481c4da09..4446d9c2bec 100644 --- a/spec/lib/open_project/configuration_spec.rb +++ b/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