From 36f5ac729812a26d54137316248e7cb712145234 Mon Sep 17 00:00:00 2001 From: Claire Date: Sun, 23 Apr 2023 22:27:24 +0200 Subject: [PATCH] Fix caching logic with regards to Accept-Language, Cookie, and Signature (#24604) --- app/controllers/accounts_controller.rb | 2 +- app/controllers/api/base_controller.rb | 5 -- app/controllers/application_controller.rb | 6 ++ app/controllers/concerns/cache_concern.rb | 14 +++++ .../concerns/web_app_controller_concern.rb | 2 + .../follower_accounts_controller.rb | 2 +- .../following_accounts_controller.rb | 2 +- app/controllers/statuses_controller.rb | 4 +- app/controllers/tags_controller.rb | 2 +- spec/controllers/accounts_controller_spec.rb | 4 +- spec/controllers/statuses_controller_spec.rb | 60 +++++++++---------- spec/controllers/tags_controller_spec.rb | 4 +- 12 files changed, 62 insertions(+), 45 deletions(-) diff --git a/app/controllers/accounts_controller.rb b/app/controllers/accounts_controller.rb index 7cdc40fb157493..4034d014af4ec2 100644 --- a/app/controllers/accounts_controller.rb +++ b/app/controllers/accounts_controller.rb @@ -7,7 +7,7 @@ class AccountsController < ApplicationController include AccountControllerConcern include SignatureAuthentication - vary_by -> { public_fetch_mode? ? 'Accept' : 'Accept, Signature' } + vary_by -> { public_fetch_mode? ? 'Accept, Accept-Language, Cookie' : 'Accept, Accept-Language, Cookie, Signature' } before_action :require_account_signature!, if: -> { request.format == :json && authorized_fetch_mode? } diff --git a/app/controllers/api/base_controller.rb b/app/controllers/api/base_controller.rb index d62416c53d85e6..21a79f89268154 100644 --- a/app/controllers/api/base_controller.rb +++ b/app/controllers/api/base_controller.rb @@ -12,7 +12,6 @@ class Api::BaseController < ApplicationController before_action :require_authenticated_user!, if: :disallow_unauthenticated_api_access? before_action :require_not_suspended! - before_action :set_cache_control_defaults protect_from_forgery with: :null_session @@ -148,10 +147,6 @@ def authorize_if_got_token!(*scopes) doorkeeper_authorize!(*scopes) if doorkeeper_token end - def set_cache_control_defaults - response.cache_control.replace(private: true, no_store: true) - end - def disallow_unauthenticated_api_access? ENV['DISALLOW_UNAUTHENTICATED_API_ACCESS'] == 'true' || Rails.configuration.x.whitelist_mode end diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index fb01abb935db43..c06aae10b0e195 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -38,6 +38,8 @@ class ApplicationController < ActionController::Base before_action :store_current_location, except: :raise_not_found, unless: :devise_controller? before_action :require_functional!, if: :user_signed_in? + before_action :set_cache_control_defaults + skip_before_action :verify_authenticity_token, only: :raise_not_found def raise_not_found @@ -152,4 +154,8 @@ def respond_with_error(code) format.json { render json: { error: Rack::Utils::HTTP_STATUS_CODES[code] }, status: code } end end + + def set_cache_control_defaults + response.cache_control.replace(private: true, no_store: true) + end end diff --git a/app/controllers/concerns/cache_concern.rb b/app/controllers/concerns/cache_concern.rb index ffede5839d8571..dae1dad1b0ab53 100644 --- a/app/controllers/concerns/cache_concern.rb +++ b/app/controllers/concerns/cache_concern.rb @@ -163,6 +163,20 @@ def vary_by(value) end end + included do + after_action :enforce_cache_control! + end + + # Prevents high-entropy headers such as `Cookie`, `Signature` or `Authorization` + # from being used as cache keys, while allowing to `Vary` on them (to not serve + # anonymous cached data to authenticated requests when authentication matters) + def enforce_cache_control! + vary = response.headers['Vary']&.split&.map { |x| x.strip.downcase } + return unless vary.present? && %w(cookie authorization signature).any? { |header| vary.include?(header) && request.headers[header].present? } + + response.cache_control.replace(private: true, no_store: true) + end + def render_with_cache(**options) raise ArgumentError, 'Only JSON render calls are supported' unless options.key?(:json) || block_given? diff --git a/app/controllers/concerns/web_app_controller_concern.rb b/app/controllers/concerns/web_app_controller_concern.rb index f28786f63e25e1..7f2fbae78e2a84 100644 --- a/app/controllers/concerns/web_app_controller_concern.rb +++ b/app/controllers/concerns/web_app_controller_concern.rb @@ -6,6 +6,8 @@ module WebAppControllerConcern included do prepend_before_action :redirect_unauthenticated_to_permalinks! before_action :set_app_body_class + + vary_by 'Accept, Accept-Language, Cookie' end def set_app_body_class diff --git a/app/controllers/follower_accounts_controller.rb b/app/controllers/follower_accounts_controller.rb index 3068c07e32f7b3..a260af7bd56f58 100644 --- a/app/controllers/follower_accounts_controller.rb +++ b/app/controllers/follower_accounts_controller.rb @@ -5,7 +5,7 @@ class FollowerAccountsController < ApplicationController include SignatureVerification include WebAppControllerConcern - vary_by -> { public_fetch_mode? ? 'Accept' : 'Accept, Signature' } + vary_by -> { public_fetch_mode? ? 'Accept, Accept-Language, Cookie' : 'Accept, Accept-Language, Cookie, Signature' } before_action :require_account_signature!, if: -> { request.format == :json && authorized_fetch_mode? } diff --git a/app/controllers/following_accounts_controller.rb b/app/controllers/following_accounts_controller.rb index 022884193d65e1..dfdda64da85261 100644 --- a/app/controllers/following_accounts_controller.rb +++ b/app/controllers/following_accounts_controller.rb @@ -5,7 +5,7 @@ class FollowingAccountsController < ApplicationController include SignatureVerification include WebAppControllerConcern - vary_by -> { public_fetch_mode? ? 'Accept' : 'Accept, Signature' } + vary_by -> { public_fetch_mode? ? 'Accept, Accept-Language, Cookie' : 'Accept, Accept-Language, Cookie, Signature' } before_action :require_account_signature!, if: -> { request.format == :json && authorized_fetch_mode? } diff --git a/app/controllers/statuses_controller.rb b/app/controllers/statuses_controller.rb index 07927d119f6000..f1b2bc350d3466 100644 --- a/app/controllers/statuses_controller.rb +++ b/app/controllers/statuses_controller.rb @@ -6,7 +6,7 @@ class StatusesController < ApplicationController include Authorization include AccountOwnedConcern - vary_by -> { public_fetch_mode? ? 'Accept' : 'Accept, Signature' } + vary_by -> { public_fetch_mode? ? 'Accept, Accept-Language, Cookie' : 'Accept, Accept-Language, Cookie, Signature' } before_action :require_account_signature!, only: [:show, :activity], if: -> { request.format == :json && authorized_fetch_mode? } before_action :set_status @@ -30,7 +30,7 @@ def show end format.json do - expires_in 3.minutes, public: @status.distributable? && public_fetch_mode? + expires_in 3.minutes, public: true if @status.distributable? && public_fetch_mode? render_with_cache json: @status, content_type: 'application/activity+json', serializer: ActivityPub::NoteSerializer, adapter: ActivityPub::Adapter end end diff --git a/app/controllers/tags_controller.rb b/app/controllers/tags_controller.rb index 4afc1aca5f5cc1..3a1b3058733485 100644 --- a/app/controllers/tags_controller.rb +++ b/app/controllers/tags_controller.rb @@ -7,7 +7,7 @@ class TagsController < ApplicationController PAGE_SIZE = 20 PAGE_SIZE_MAX = 200 - vary_by -> { public_fetch_mode? ? 'Accept' : 'Accept, Signature' } + vary_by -> { public_fetch_mode? ? 'Accept, Accept-Language, Cookie' : 'Accept, Accept-Language, Cookie, Signature' } before_action :require_account_signature!, if: -> { request.format == :json && authorized_fetch_mode? } before_action :authenticate_user!, if: :whitelist_mode? diff --git a/spec/controllers/accounts_controller_spec.rb b/spec/controllers/accounts_controller_spec.rb index 0cdccd70d16e45..53fc75659a20a5 100644 --- a/spec/controllers/accounts_controller_spec.rb +++ b/spec/controllers/accounts_controller_spec.rb @@ -218,8 +218,8 @@ expect(response.media_type).to eq 'application/activity+json' end - it 'returns public Cache-Control header' do - expect(response.headers['Cache-Control']).to include 'public' + it 'returns private Cache-Control header' do + expect(response.headers['Cache-Control']).to include 'private' end it 'renders account' do diff --git a/spec/controllers/statuses_controller_spec.rb b/spec/controllers/statuses_controller_spec.rb index f0a40707fea1cb..4ac6a68bb2c869 100644 --- a/spec/controllers/statuses_controller_spec.rb +++ b/spec/controllers/statuses_controller_spec.rb @@ -16,7 +16,7 @@ end it 'returns Vary header' do - expect(response.headers['Vary']).to include 'Accept' + expect(response.headers['Vary']).to include 'Accept, Accept-Language, Cookie' end it 'returns public Cache-Control header' do @@ -84,7 +84,7 @@ end it 'returns Vary header' do - expect(response.headers['Vary']).to eq 'Accept' + expect(response.headers['Vary']).to eq 'Accept, Accept-Language, Cookie' end it 'returns public Cache-Control header' do @@ -109,7 +109,7 @@ end it 'returns Vary header' do - expect(response.headers['Vary']).to eq 'Accept' + expect(response.headers['Vary']).to eq 'Accept, Accept-Language, Cookie' end it_behaves_like 'cacheable response' @@ -208,11 +208,11 @@ end it 'returns Vary header' do - expect(response.headers['Vary']).to eq 'Accept' + expect(response.headers['Vary']).to eq 'Accept, Accept-Language, Cookie' end - it 'returns no Cache-Control header' do - expect(response.headers).to_not include 'Cache-Control' + it 'returns private Cache-Control header' do + expect(response.headers['Cache-Control']).to include 'private' end it 'renders status' do @@ -233,11 +233,11 @@ end it 'returns Vary header' do - expect(response.headers['Vary']).to eq 'Accept' + expect(response.headers['Vary']).to eq 'Accept, Accept-Language, Cookie' end - it 'returns public Cache-Control header' do - expect(response.headers['Cache-Control']).to include 'public' + it 'returns private Cache-Control header' do + expect(response.headers['Cache-Control']).to include 'private' end it 'returns Content-Type header' do @@ -272,11 +272,11 @@ end it 'returns Vary header' do - expect(response.headers['Vary']).to eq 'Accept' + expect(response.headers['Vary']).to eq 'Accept, Accept-Language, Cookie' end - it 'returns no Cache-Control header' do - expect(response.headers).to_not include 'Cache-Control' + it 'returns private Cache-Control header' do + expect(response.headers['Cache-Control']).to include 'private' end it 'renders status' do @@ -297,7 +297,7 @@ end it 'returns Vary header' do - expect(response.headers['Vary']).to eq 'Accept' + expect(response.headers['Vary']).to eq 'Accept, Accept-Language, Cookie' end it 'returns private Cache-Control header' do @@ -359,11 +359,11 @@ end it 'returns Vary header' do - expect(response.headers['Vary']).to eq 'Accept' + expect(response.headers['Vary']).to eq 'Accept, Accept-Language, Cookie' end - it 'returns no Cache-Control header' do - expect(response.headers).to_not include 'Cache-Control' + it 'returns private Cache-Control header' do + expect(response.headers['Cache-Control']).to include 'private' end it 'renders status' do @@ -384,7 +384,7 @@ end it 'returns Vary header' do - expect(response.headers['Vary']).to eq 'Accept' + expect(response.headers['Vary']).to eq 'Accept, Accept-Language, Cookie' end it 'returns private Cache-Control header' do @@ -472,11 +472,11 @@ end it 'returns Vary header' do - expect(response.headers['Vary']).to eq 'Accept' + expect(response.headers['Vary']).to eq 'Accept, Accept-Language, Cookie' end - it 'returns no Cache-Control header' do - expect(response.headers).to_not include 'Cache-Control' + it 'returns private Cache-Control header' do + expect(response.headers['Cache-Control']).to include 'private' end it 'renders status' do @@ -497,7 +497,7 @@ end it 'returns Vary header' do - expect(response.headers['Vary']).to eq 'Accept' + expect(response.headers['Vary']).to eq 'Accept, Accept-Language, Cookie' end it_behaves_like 'cacheable response' @@ -534,11 +534,11 @@ end it 'returns Vary header' do - expect(response.headers['Vary']).to eq 'Accept' + expect(response.headers['Vary']).to eq 'Accept, Accept-Language, Cookie' end - it 'returns no Cache-Control header' do - expect(response.headers).to_not include 'Cache-Control' + it 'returns private Cache-Control header' do + expect(response.headers['Cache-Control']).to include 'private' end it 'renders status' do @@ -559,7 +559,7 @@ end it 'returns Vary header' do - expect(response.headers['Vary']).to eq 'Accept' + expect(response.headers['Vary']).to eq 'Accept, Accept-Language, Cookie' end it 'returns private Cache-Control header' do @@ -621,11 +621,11 @@ end it 'returns Vary header' do - expect(response.headers['Vary']).to eq 'Accept' + expect(response.headers['Vary']).to eq 'Accept, Accept-Language, Cookie' end - it 'returns no Cache-Control header' do - expect(response.headers).to_not include 'Cache-Control' + it 'returns private Cache-Control header' do + expect(response.headers['Cache-Control']).to include 'private' end it 'renders status' do @@ -646,7 +646,7 @@ end it 'returns Vary header' do - expect(response.headers['Vary']).to eq 'Accept' + expect(response.headers['Vary']).to eq 'Accept, Accept-Language, Cookie' end it 'returns private Cache-Control header' do @@ -827,7 +827,7 @@ end it 'returns Vary header' do - expect(response.headers['Vary']).to eq 'Accept' + expect(response.headers['Vary']).to eq 'Accept, Accept-Language, Cookie' end it 'returns public Cache-Control header' do diff --git a/spec/controllers/tags_controller_spec.rb b/spec/controllers/tags_controller_spec.rb index 9287a6ab523f6c..7a07801be70c6c 100644 --- a/spec/controllers/tags_controller_spec.rb +++ b/spec/controllers/tags_controller_spec.rb @@ -21,7 +21,7 @@ end it 'returns Vary header' do - expect(response.headers['Vary']).to eq 'Accept' + expect(response.headers['Vary']).to eq 'Accept, Accept-Language, Cookie' end it 'returns public Cache-Control header' do @@ -37,7 +37,7 @@ end it 'returns Vary header' do - expect(response.headers['Vary']).to eq 'Accept' + expect(response.headers['Vary']).to eq 'Accept, Accept-Language, Cookie' end it 'returns public Cache-Control header' do