Merge branch 'release/17.4' into release/17.5

lib/open_project/confidential_cache.rb

@@ -36,15 +36,20 @@ module OpenProject
     class << self
       delegate :delete, :clear, to: Cache
 
-      def fetch(*, **)
-        ciphertext = Cache.fetch(*, **) { token_encryptor.encrypt_and_sign(yield) }
-
-        token_encryptor.decrypt_and_verify(ciphertext)
+      # Rubocop wants to convert this to ... but we need the first positional args
+      # for the delete case.
+      # rubocop:disable Style/ArgumentsForwarding
+      def fetch(*, **, &)
+        fetch_and_decrypt(*, **, &)
       rescue ActiveSupport::MessageEncryptor::InvalidMessage
-        # Drop values that can't be read, ensuring the cache heals from unreadable values
+        # Drop the unreadable value and recompute once. The recompute is a guaranteed
+        # cache miss, so the second attempt returns the freshly computed plaintext
+        # without decrypting. If it still raises, the error propagates rather than
+        # looping, since this second attempt is not rescued.
         delete(*)
-        retry
+        fetch_and_decrypt(*, **, &)
       end
+      # rubocop:enable Style/ArgumentsForwarding
 
       def read(name, **)
         ciphertext = Cache.read(name, **)
@@ -64,10 +69,31 @@ module OpenProject
 
       private
 
+      # Reads the cached ciphertext and decrypts it, computing and storing an
+      # encrypted value on a cache miss. On a miss we already hold the plaintext,
+      # so we return it directly instead of decrypting what we just encrypted.
+      def fetch_and_decrypt(*, **)
+        recomputed = false
+        value = nil
+
+        ciphertext = Cache.fetch(*, **) do
+          recomputed = true
+          value = yield
+          token_encryptor.encrypt_and_sign(value)
+        end
+
+        return value if recomputed
+
+        token_encryptor.decrypt_and_verify(ciphertext)
+      end
+
       def token_encryptor
         @token_encryptor ||= begin
           key = Rails.application.key_generator.generate_key("op-cache:confidential-values:v1", 32)
-          ActiveSupport::MessageEncryptor.new(key, cipher: "aes-256-gcm", serializer: YAML)
+          # MessagePack avoids YAML's alias emission (which broke decryption for hashes
+          # that reuse the same object for multiple keys, e.g. Saml::Provider#to_h) and,
+          # unlike :message_pack_allow_marshal, never falls back to Marshal on load.
+          ActiveSupport::MessageEncryptor.new(key, cipher: "aes-256-gcm", serializer: :message_pack)
         end
       end
     end

spec/lib/open_project/confidential_cache_spec.rb

@@ -96,5 +96,40 @@ RSpec.describe OpenProject::ConfidentialCache do
 
       expect(OpenProject::Cache.read(cache_key)).not_to be_nil
     end
+
+    it "raises instead of looping when the value stays unreadable across a retry" do
+      # Simulate a value that remains an unreadable cache hit even after delete/retry.
+      allow(OpenProject::Cache).to receive(:fetch).and_return("some clear text")
+
+      expect { described_class.fetch(cache_key) { "value" } }
+        .to raise_error(ActiveSupport::MessageEncryptor::InvalidMessage)
+    end
+  end
+
+  # Regression: a hash that reuses the same object for two keys  was rejected on load
+  # which the strict read-side load rejected, raising InvalidMessage on every decrypt and
+  # sending #fetch into an endless delete/retry loop.
+  describe "with symbol keys and shared object references" do
+    let(:shared) { "https://idp.example.com/slo" }
+    let(:value) do
+      {
+        my_provider: {
+          name: :my_provider,
+          idp_slo_service_url: shared,
+          idp_slo_target_url: shared
+        }
+      }
+    end
+
+    it "roundtrips via #write and #read" do
+      described_class.write(cache_key, value)
+
+      expect(described_class.read(cache_key)).to eq(value)
+    end
+
+    it "decrypts the cached value on a subsequent #fetch without recomputing" do
+      expect(described_class.fetch(cache_key) { value }).to eq(value)
+      expect(described_class.fetch(cache_key) { raise "should not recompute" }).to eq(value)
+    end
   end
 end