Prevent cache tampering by freezing cached value

[david-uhlig]

This PR freezes the merged class String object before writing it to the cache.

Currently, the merge method returns the mutable String object of the cached class string after merging. Here is an example test case that fails:

# frozen_string_literal: true

require "test_helper"

class TestMutateCachedValue < Minitest::Test
  def setup
    @merger = TailwindMerge::Merger.new
  end

  def test_cache_is_immutable
    merged_classes = @merger.merge("font-medium font-bold")
    assert_equal("font-bold", merged_classes)
    merged_classes << " text-white"
    assert_equal("font-bold", @merger.merge("font-medium font-bold"))
  end
end

with

Minitest::Assertion: Expected: "font-bold"
  Actual: "font-bold text-white"

This causes some very confusing and hard to debug bugs. Initially, the merger computes the correct value and saves it to the cache. Then the output is modified externally, e.g. because you want to append a class in some special case. For all in-place operations, like <<, this changes the cached value. The cache can never recover because the value is not recalculated.

The obvious candidates for fixing this are either .freeze on the cache value, like in this PR, or .dup on the return value of the merge method. I picked .freeze because it comes with almost no additional computational penalty. I imagine that in the vast majority of cases, the consumer won't need to modify the merged classes. So spawning a new String object on every request seems wasteful. Also Ruby is moving forward to freeze all strings by default in 3.5. The downside is, that .freeze is a breaking change. But then again, it probably mostly breaks code that had unnoticed bugs, so it seems justified.

Results of my test suite indicate, that .freeze doesn't affect the computation speed. .dub performs 10% worse when the cache isn't overflowing and equally to .freeze once the cache size is exceeded. Here are some results:

--------------- 500 samples ---------------
ruby 3.3.6 (2024-11-05 revision 75015d4c1f) [x86_64-linux]
Warming up --------------------------------------
              Merger   131.978k i/100ms
        FreezeMerger   129.696k i/100ms
           DupMerger   117.562k i/100ms
Calculating -------------------------------------
              Merger      1.190M (± 2.0%) i/s  (840.43 ns/i) -      6.071M in   5.104444s
        FreezeMerger      1.153M (± 2.3%) i/s  (867.32 ns/i) -      5.836M in   5.064749s
           DupMerger      1.068M (± 3.3%) i/s  (936.47 ns/i) -      5.408M in   5.070382s

Comparison:
              Merger:  1189862.4 i/s
        FreezeMerger:  1152977.3 i/s - same-ish: difference falls within error
           DupMerger:  1067845.3 i/s - 1.11x  slower

--------------- 800 samples ---------------
ruby 3.3.6 (2024-11-05 revision 75015d4c1f) [x86_64-linux]
Warming up --------------------------------------
              Merger     9.264k i/100ms
        FreezeMerger     9.400k i/100ms
           DupMerger     9.163k i/100ms
Calculating -------------------------------------
              Merger     86.642k (± 4.9%) i/s   (11.54 μs/i) -    435.408k in   5.038346s
        FreezeMerger     88.249k (± 2.2%) i/s   (11.33 μs/i) -    441.800k in   5.008778s
           DupMerger     87.516k (± 2.3%) i/s   (11.43 μs/i) -    439.824k in   5.028285s

Comparison:
        FreezeMerger:    88248.9 i/s
           DupMerger:    87516.5 i/s - same-ish: difference falls within error
              Merger:    86642.3 i/s - same-ish: difference falls within error

This is the suite:

require 'securerandom'
require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  gem "benchmark-ips"
  gem "tailwind_merge"
end

module TailwindMerge
  class FreezeMerger < Merger
    def merge(class_names)
      if class_names.is_a?(Array)
        class_names = class_names.compact.join(" ")
      end

      @cache.getset(class_names) do
        merge_class_list(class_names).freeze
      end
    end
  end

  class DupMerger < Merger
    def merge(class_names)
      if class_names.is_a?(Array)
        class_names = class_names.compact.join(" ")
      end

      @cache.getset(class_names) do
        merge_class_list(class_names)
      end.dup
    end
  end

  class Sampler
    TAILWIND_CLASSES = %w[
      text-left text-center text-right text-justify
      bg-red-500 bg-green-500 bg-blue-500 bg-yellow-500
      border border-0 border-2 border-dashed
      m-0 m-1 m-2 m-4 m-8
      p-0 p-1 p-2 p-4 p-8
      flex flex-row flex-col flex-wrap
      items-start items-center items-end items-stretch
      justify-start justify-center justify-end justify-between justify-around
      hidden block inline-block inline
      w-1/4 w-1/2 w-3/4 w-full
      h-1/4 h-1/2 h-3/4 h-full
      rounded rounded-sm rounded-md rounded-lg
      shadow shadow-md shadow-lg shadow-none
      text-sm text-base text-lg text-xl text-2xl
      font-thin font-light font-normal font-medium font-semibold font-bold font-black
      italic not-italic underline no-underline
      absolute relative fixed sticky
      top-0 top-1/2 bottom-0 bottom-1/2 left-0 left-1/2 right-0 right-1/2
      grid grid-cols-1 grid-cols-2 grid-cols-3 grid-cols-4
      gap-0 gap-1 gap-2 gap-4 gap-8
      hover:bg-red-500 hover:bg-green-500 hover:bg-blue-500 hover:bg-yellow-500
      active:bg-red-500 active:bg-green-500 active:bg-blue-500 active:bg-yellow-500
      focus:ring focus:ring-0 focus:ring-2 focus:ring-offset-2
      overflow-hidden overflow-auto overflow-scroll overflow-visible
      whitespace-nowrap whitespace-normal whitespace-pre
      cursor-pointer cursor-default cursor-wait cursor-move
    ].freeze

    attr_reader :class_samples, :sample_indices

    def initialize(num_samples: 500, avg_classes: 5, std_dev_classes: 2, num_operations: 10)
      @class_samples = generate_class_samples(num_samples, avg_classes, std_dev_classes)
      @sample_indices = Array.new(num_operations) { rand(0...@class_samples.size) }
    end

    def sample_class_names(index)
      @class_samples[@sample_indices[index % @sample_indices.size]]
    end

    private

    def generate_class_samples(num_samples, avg_classes, std_dev_classes)
      [].tap do |unique_samples|
        while unique_samples.size < num_samples
          num_classes = clamp_value(random_gaussian(avg_classes, std_dev_classes).round, 0, TAILWIND_CLASSES.size)
          sample_classes = TAILWIND_CLASSES.sample(num_classes, random: SecureRandom).sort
          unique_samples << sample_classes unless unique_samples.include?(sample_classes)
        end
      end
    end

    def random_gaussian(mean, std_dev)
      theta = 2 * Math::PI * rand
      rho = Math.sqrt(-2 * Math.log(rand))
      scale = std_dev * rho
      mean + scale * Math.cos(theta)
    end

    def clamp_value(value, min, max)
      [[value, min].max, max].min
    end
  end
end

# Benchmarking
[500, 800].each do |num_samples|
  puts "--------------- #{num_samples} samples ---------------"
  sampler = TailwindMerge::Sampler.new(
    num_samples: num_samples,
    num_operations: 2_000_000
  )
  Benchmark.ips do |benchmark|
    [TailwindMerge::Merger, TailwindMerge::FreezeMerger, TailwindMerge::DupMerger].each do |merger_class|
      benchmark.report(merger_class.name.split("::").last) do |iterations|
        merger_instance = merger_class.new
        (0...iterations).each { |i| merger_instance.merge(sampler.sample_class_names(i)) }
      end
    end

    benchmark.compare!
  end
end

[gjtorikian]

The cache can never recover because the value is not recalculated.

Gah, whoops!

The downside is, that .freeze is a breaking change. But then again, it probably mostly breaks code that had unnoticed bugs, so it seems justified.

I agree. Besides which, this gem is still pre 1.0, so per semver just like this can be made.

Thanks for the PR! I'll release this as a new patch release shortly.