Fix format-dependent redirects being cached regardless of requested format (#27634)

This commit is contained in:
Claire 2023-11-13 17:58:00 +01:00 committed by GitHub
parent 74dd325112
commit eea2654236
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
2 changed files with 30 additions and 9 deletions

View file

@ -3,6 +3,18 @@
require 'sidekiq_unique_jobs/web' require 'sidekiq_unique_jobs/web'
require 'sidekiq-scheduler/web' require 'sidekiq-scheduler/web'
class RedirectWithVary < ActionDispatch::Routing::PathRedirect
def serve(...)
super.tap do |_, headers, _|
headers['Vary'] = 'Origin, Accept'
end
end
end
def redirect_with_vary(path)
RedirectWithVary.new(301, path)
end
Rails.application.routes.draw do Rails.application.routes.draw do
# Paths of routes on the web app that to not require to be indexed or # Paths of routes on the web app that to not require to be indexed or
# have alternative format representations requiring separate controllers # have alternative format representations requiring separate controllers
@ -90,10 +102,13 @@ Rails.application.routes.draw do
confirmations: 'auth/confirmations', confirmations: 'auth/confirmations',
} }
get '/users/:username', to: redirect('/@%{username}'), 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/following', to: redirect('/@%{username}/following'), constraints: lambda { |req| req.format.nil? || req.format.html? } get '/users/:username', to: redirect_with_vary('/@%{username}'), 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/following', to: redirect_with_vary('/@%{username}/following'), 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? } 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}" } get '/authorize_follow', to: redirect { |_, request| "/authorize_interaction?#{request.params.to_query}" }
resources :accounts, path: 'users', only: [:show], param: :username do resources :accounts, path: 'users', only: [:show], param: :username do

View file

@ -124,7 +124,7 @@ describe 'Caching behavior' do
expect(response.cookies).to be_empty expect(response.cookies).to be_empty
end 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[:max_age]&.to_i).to be_positive
expect(response.cache_control[:public]).to be_truthy expect(response.cache_control[:public]).to be_truthy
expect(response.cache_control[:private]).to be_falsy expect(response.cache_control[:private]).to be_falsy
@ -141,11 +141,8 @@ describe 'Caching behavior' do
end end
shared_examples 'non-cacheable error' do 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) expect(response).to_not have_http_status(200)
end
it 'does not have cache headers' do
expect(response.cache_control[:public]).to be_falsy expect(response.cache_control[:public]).to be_falsy
end end
end end
@ -182,6 +179,15 @@ describe 'Caching behavior' do
end end
context 'when anonymously accessed' do 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| TestEndpoints::ALWAYS_CACHED.each do |endpoint|
describe endpoint do describe endpoint do
before { get endpoint } before { get endpoint }