fix: Parse JSON cache entries safely and stop sharing authenticated responses

README.md

@@ -62,8 +62,10 @@ you might see errors like:
 Response could not be serialized: "\xC3" from ASCII-8BIT to UTF-8. Try using Marshal to serialize.
 ```
 
-For full unicode support, or if you expect to be dealing with images, you can use the stdlib
-[Marshal][marshal] instead. Alternatively you could use another json library like `oj` or `yajl-ruby`.
+For full unicode support, or if you expect to be dealing with images, you can use another json
+library like `oj` or `yajl-ruby`, or the stdlib [Marshal][marshal]. Only pick Marshal when you fully
+trust the cache store: `Marshal.load` will instantiate any object found in the data, while the
+default `JSON` serializer parses entries into plain hashes and never instantiates classes.
 
 ```ruby
 client = Faraday.new do |builder|
@@ -214,9 +216,11 @@ The `max-age`, `must-revalidate`, `proxy-revalidate`, `s-maxage` and
 
 ### Shared vs. non-shared caches
 
-By default, the middleware acts as a "shared cache" per RFC 2616. This means it does not cache
-responses with `Cache-Control: private`. This behavior can be changed by passing in the
-`:shared_cache` configuration option:
+By default, the middleware acts as a "shared cache" per RFC 9111. This means it does not cache
+responses with `Cache-Control: private`, and it only stores and reuses responses to requests that
+carried an `Authorization` header when the response explicitly allows it with `public`,
+`must-revalidate` or `s-maxage` (RFC 9111 section 3.5). This behavior can be changed by passing in
+the `:shared_cache` configuration option:
 
 ```ruby
 client = Faraday.new do |builder|

lib/faraday/http_cache.rb

@@ -196,7 +196,7 @@ def should_delete?(status, method)
     def process(env)
       entry = @strategy.read(@request)
 
-      return fetch(env) if entry.nil?
+      return fetch(env) if entry.nil? || !reusable?(entry)
 
       if entry.fresh? && !@request.no_cache?
         response = entry.to_response(env)
@@ -270,14 +270,49 @@ def trace(operation)
     #
     # Returns nothing.
     def store(response)
-      if shared_cache? ? response.cacheable_in_shared_cache? : response.cacheable_in_private_cache?
+      if storable?(response)
         trace :store
         @strategy.write(@request, response)
       else
         trace :uncacheable
       end
     end
 
+    # Internal: Checks if the response may be stored by this cache instance.
+    # A shared cache also refuses responses to requests that carried an
+    # 'Authorization' header unless the response explicitly allows it
+    # (RFC 9111 section 3.5), so what is never stored is never served to
+    # another caller.
+    #
+    # response - a 'Faraday::HttpCache::Response' instance.
+    #
+    # Returns true or false.
+    def storable?(response)
+      return response.cacheable_in_private_cache? unless shared_cache?
+      return false if authorization_bearing? && !response.shared_cache_authorized?
+
+      response.cacheable_in_shared_cache?
+    end
+
+    # Internal: Checks if a stored entry may be served for the current request.
+    # Entries written by earlier versions of this middleware may be responses
+    # to authenticated requests that a shared cache must not reuse; such an
+    # entry is treated as a miss and replaced.
+    #
+    # entry - a 'Faraday::HttpCache::Response' read from the strategy.
+    #
+    # Returns true or false.
+    def reusable?(entry)
+      return true unless shared_cache? && authorization_bearing?
+
+      entry.shared_cache_authorized?
+    end
+
+    # Internal: Checks if the current request carries an 'Authorization' header.
+    def authorization_bearing?
+      !@request.headers['Authorization'].nil?
+    end
+
     def delete(request, response)
       headers = %w[Location Content-Location]
       headers.each do |header|

lib/faraday/http_cache/response.rb

@@ -98,6 +98,22 @@ def cacheable_in_private_cache?
         cacheable?(false)
       end
 
+      # Internal: Checks if a shared cache may reuse this response for requests
+      # other than the one that carried an 'Authorization' header.
+      #
+      # RFC 9111 section 3.5: a shared cache must not use a cached response to
+      # a request with an 'Authorization' header to satisfy any subsequent
+      # request unless the response carries a 'Cache-Control' directive that
+      # explicitly allows it. The directives with that effect are
+      # 'must-revalidate', 'public' and 's-maxage'.
+      #
+      # Returns true if one of those directives is present.
+      def shared_cache_authorized?
+        cache_control.public? ||
+          cache_control.must_revalidate? ||
+          !cache_control.shared_max_age.nil?
+      end
+
       # Internal: Gets the response age in seconds.
       #
       # Returns the 'Age' header if present, or subtracts the response 'date'

lib/faraday/http_cache/strategies/base_strategy.rb

@@ -29,7 +29,9 @@ class BaseStrategy
         # @option options [Faraday::HttpCache::MemoryStore, nil] :store - a cache
         #   store object that should respond to 'read', 'write', and 'delete'.
         # @option options [#dump#load] :serializer - an object that should
-        #   respond to 'dump' and 'load'.
+        #   respond to 'dump' and 'load'. 'load' must never instantiate classes
+        #   named by the data, since the cached entries contain response headers
+        #   sent by the origin server.
         # @option options [Logger, nil] :logger - an object to be used to emit warnings.
         def initialize(options = {})
           @cache = options[:store] || Faraday::HttpCache::MemoryStore.new
@@ -80,7 +82,12 @@ def deserialize_entry(*objects)
         end
 
         def deserialize_object(object)
-          @serializer.load(object).transform_keys(&:to_sym)
+          # JSON.load enables create_additions, so a `json_class` key in the
+          # entry would instantiate that class. Response headers are stored
+          # verbatim, which lets an origin server plant such a key. JSON.parse
+          # only ever builds plain Ruby objects.
+          loaded = @serializer.equal?(::JSON) ? ::JSON.parse(object) : @serializer.load(object)
+          loaded.transform_keys(&:to_sym)
         end
 
         def warn(message)

spec/http_cache_spec.rb

@@ -121,6 +121,48 @@
       expect(logger).to receive(:debug) { |&block| expect(block.call).to eq('HTTP Cache: [GET /private] miss, uncacheable') }
       client.get('private')
     end
+
+    describe 'responses to requests with an "Authorization" header' do
+      def get_as(user, path = 'authenticated')
+        client.get(path) { |request| request.headers['Authorization'] = "Bearer #{user}" }
+      end
+
+      it 'does not serve one caller the response cached for another' do
+        alice = get_as('alice')
+        bob = get_as('bob')
+
+        expect(alice.body).to eq('1:Bearer alice')
+        expect(bob.body).to eq('2:Bearer bob')
+      end
+
+      it 'logs that the response is uncacheable' do
+        expect(logger).to receive(:debug) { |&block| expect(block.call).to eq('HTTP Cache: [GET /authenticated] miss, uncacheable') }
+        get_as('alice')
+      end
+
+      it 'caches responses that are explicitly marked as public' do
+        get_as('alice', 'authenticated-public')
+        bob = get_as('bob', 'authenticated-public')
+
+        expect(bob.body).to eq('1:Bearer alice')
+      end
+
+      it 'does not serve entries stored before the authorization check existed' do
+        store = Faraday::HttpCache::MemoryStore.new
+        clients = [false, true].map do |shared|
+          Faraday.new(url: ENV['FARADAY_SERVER']) do |stack|
+            stack.use Faraday::HttpCache, store: store, shared_cache: shared
+            stack.adapter ENV['FARADAY_ADAPTER'].to_sym
+          end
+        end
+        private_client, shared_client = clients
+
+        private_client.get('authenticated') { |request| request.headers['Authorization'] = 'Bearer alice' }
+        bob = shared_client.get('authenticated') { |request| request.headers['Authorization'] = 'Bearer bob' }
+
+        expect(bob.body).to eq('2:Bearer bob')
+      end
+    end
   end
 
   describe 'when acting as a private cache' do
@@ -135,6 +177,13 @@
       expect(logger).to receive(:debug) { |&block| expect(block.call).to eq('HTTP Cache: [GET /private] miss, store') }
       client.get('private')
     end
+
+    it 'caches responses to requests with an "Authorization" header' do
+      client.get('authenticated') { |request| request.headers['Authorization'] = 'Bearer alice' }
+      bob = client.get('authenticated') { |request| request.headers['Authorization'] = 'Bearer bob' }
+
+      expect(bob.body).to eq('1:Bearer alice')
+    end
   end
 
   it 'does not cache responses with a explicit no-store directive' do

spec/spec_helper.rb

@@ -16,6 +16,7 @@
 
 require 'support/test_app'
 require 'support/test_server'
+require 'support/json_gadget'
 
 server = TestServer.new
 

spec/strategies/by_url_spec.rb

@@ -16,6 +16,20 @@
   let(:strategy) { described_class.new(store: cache) }
   subject { strategy }
 
+  describe 'deserializing entries' do
+    let(:response) { double(serializable_hash: { response_headers: { 'json_class' => 'JsonGadget' } }) }
+
+    before { JsonGadget.invocations.clear }
+
+    it 'never instantiates classes named by the cached data' do
+      strategy.write(request, response)
+      cached = strategy.read(request)
+
+      expect(JsonGadget.invocations).to be_empty
+      expect(cached.payload[:response_headers]['json_class']).to eq('JsonGadget')
+    end
+  end
+
   describe 'Cache configuration' do
     it 'uses a MemoryStore by default' do
       expect(Faraday::HttpCache::MemoryStore).to receive(:new).and_call_original

spec/strategies/by_vary_spec.rb

@@ -23,6 +23,20 @@
   let(:strategy) { described_class.new(store: cache) }
   subject { strategy }
 
+  describe 'deserializing entries' do
+    let(:response_payload) { { response_headers: { 'Vary' => vary, 'json_class' => 'JsonGadget' } } }
+
+    before { JsonGadget.invocations.clear }
+
+    it 'never instantiates classes named by the cached data' do
+      strategy.write(request, response)
+      cached = strategy.read(request)
+
+      expect(JsonGadget.invocations).to be_empty
+      expect(cached.payload[:response_headers]['json_class']).to eq('JsonGadget')
+    end
+  end
+
   describe 'storing responses' do
     shared_examples 'A strategy with serialization' do
       it 'writes the response object to the underlying cache' do

spec/support/json_gadget.rb

@@ -0,0 +1,14 @@
+# frozen_string_literal: true
+
+# A class that records every attempt to build it through JSON.load's
+# create_additions hook, so specs can assert cached entries never do that.
+class JsonGadget
+  def self.invocations
+    @invocations ||= []
+  end
+
+  def self.json_create(attributes)
+    invocations << attributes
+    new
+  end
+end

spec/support/test_app.rb

@@ -85,6 +85,14 @@ class TestApp < Sinatra::Base
     halt 405
   end
 
+  get '/authenticated' do
+    [200, { 'Cache-Control' => 'max-age=200' }, "#{increment_counter}:#{env['HTTP_AUTHORIZATION']}"]
+  end
+
+  get '/authenticated-public' do
+    [200, { 'Cache-Control' => 'public, max-age=200' }, "#{increment_counter}:#{env['HTTP_AUTHORIZATION']}"]
+  end
+
   get '/private' do
     [200, { 'Cache-Control' => 'private, max-age=100' }, increment_counter]
   end