From 23e32a4b3031d1da8b911e0145d61b4dd47c4f96 Mon Sep 17 00:00:00 2001 From: Claire Date: Wed, 8 Nov 2023 11:31:05 +0100 Subject: [PATCH] Fix format-dependent redirects being cached regardless of requested format (#27632) --- config/routes.rb | 23 +++++++++++++++++++---- spec/requests/cache_spec.rb | 16 +++++++++++----- 2 files changed, 30 insertions(+), 9 deletions(-) diff --git a/config/routes.rb b/config/routes.rb index 3735e42012..3adda3b822 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -3,6 +3,18 @@ require 'sidekiq_unique_jobs/web' require 'sidekiq-scheduler/web' +class RedirectWithVary < ActionDispatch::Routing::PathRedirect + def build_response(req) + super.tap do |response| + response.headers['Vary'] = 'Origin, Accept' + end + end +end + +def redirect_with_vary(path) + RedirectWithVary.new(301, path) +end + Rails.application.routes.draw do # Paths of routes on the web app that to not require to be indexed or # have alternative format representations requiring separate controllers @@ -90,10 +102,13 @@ Rails.application.routes.draw do confirmations: 'auth/confirmations', } - get '/users/:username', to: redirect('/@%{username}'), constraints: lambda { |req| req.format.nil? || req.format.html? } - get '/users/:username/following', to: redirect('/@%{username}/following'), constraints: lambda { |req| req.format.nil? || req.format.html? } - get '/users/:username/followers', to: redirect('/@%{username}/followers'), constraints: lambda { |req| req.format.nil? || req.format.html? } - get '/users/:username/statuses/:id', to: redirect('/@%{username}/%{id}'), constraints: lambda { |req| req.format.nil? || req.format.html? } + # rubocop:disable Style/FormatStringToken - those do not go through the usual formatting functions and are not safe to correct + get '/users/:username', to: redirect_with_vary('/@%{username}'), constraints: lambda { |req| req.format.nil? || req.format.html? } + get '/users/:username/following', to: redirect_with_vary('/@%{username}/following'), constraints: lambda { |req| req.format.nil? || req.format.html? } + get '/users/:username/followers', to: redirect_with_vary('/@%{username}/followers'), constraints: lambda { |req| req.format.nil? || req.format.html? } + get '/users/:username/statuses/:id', to: redirect_with_vary('/@%{username}/%{id}'), constraints: lambda { |req| req.format.nil? || req.format.html? } + # rubocop:enable Style/FormatStringToken + get '/authorize_follow', to: redirect { |_, request| "/authorize_interaction?#{request.params.to_query}" } resources :accounts, path: 'users', only: [:show], param: :username do diff --git a/spec/requests/cache_spec.rb b/spec/requests/cache_spec.rb index 1e31c28ca4..19232fce68 100644 --- a/spec/requests/cache_spec.rb +++ b/spec/requests/cache_spec.rb @@ -124,7 +124,7 @@ describe 'Caching behavior' do expect(response.cookies).to be_empty end - it 'sets public cache control' do + it 'sets public cache control', :aggregate_failures do # expect(response.cache_control[:max_age]&.to_i).to be_positive expect(response.cache_control[:public]).to be_truthy expect(response.cache_control[:private]).to be_falsy @@ -141,11 +141,8 @@ describe 'Caching behavior' do end shared_examples 'non-cacheable error' do - it 'does not return HTTP success' do + it 'does not return HTTP success and does not have cache headers', :aggregate_failures do expect(response).to_not have_http_status(200) - end - - it 'does not have cache headers' do expect(response.cache_control[:public]).to be_falsy end end @@ -180,6 +177,15 @@ describe 'Caching behavior' do end context 'when anonymously accessed' do + describe '/users/alice' do + it 'redirects with proper cache header', :aggregate_failures do + get '/users/alice' + + expect(response).to redirect_to('/@alice') + expect(response.headers['Vary']&.split(',')&.map { |x| x.strip.downcase }).to include('accept') + end + end + TestEndpoints::ALWAYS_CACHED.each do |endpoint| describe endpoint do before { get endpoint }