diff --git a/README.md b/README.md index be7a669..180a3dc 100644 --- a/README.md +++ b/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| diff --git a/lib/faraday/http_cache.rb b/lib/faraday/http_cache.rb index c0ea00a..3e4f070 100644 --- a/lib/faraday/http_cache.rb +++ b/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,7 +270,7 @@ 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 @@ -278,6 +278,41 @@ def store(response) 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| diff --git a/lib/faraday/http_cache/response.rb b/lib/faraday/http_cache/response.rb index d7eeb19..2f9025d 100644 --- a/lib/faraday/http_cache/response.rb +++ b/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' diff --git a/lib/faraday/http_cache/strategies/base_strategy.rb b/lib/faraday/http_cache/strategies/base_strategy.rb index 2f56dcc..a96de48 100644 --- a/lib/faraday/http_cache/strategies/base_strategy.rb +++ b/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) diff --git a/spec/http_cache_spec.rb b/spec/http_cache_spec.rb index 0021173..1a12270 100644 --- a/spec/http_cache_spec.rb +++ b/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 diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 2611303..5d101e8 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -16,6 +16,7 @@ require 'support/test_app' require 'support/test_server' +require 'support/json_gadget' server = TestServer.new diff --git a/spec/strategies/by_url_spec.rb b/spec/strategies/by_url_spec.rb index e8fdbe9..4530094 100644 --- a/spec/strategies/by_url_spec.rb +++ b/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 diff --git a/spec/strategies/by_vary_spec.rb b/spec/strategies/by_vary_spec.rb index 54a61a8..32dfd6c 100644 --- a/spec/strategies/by_vary_spec.rb +++ b/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 diff --git a/spec/support/json_gadget.rb b/spec/support/json_gadget.rb new file mode 100644 index 0000000..c232495 --- /dev/null +++ b/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 diff --git a/spec/support/test_app.rb b/spec/support/test_app.rb index 9b492fa..0d637f1 100644 --- a/spec/support/test_app.rb +++ b/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