From 276c39361b8ac13b0604716c815de9db24cdcc57 Mon Sep 17 00:00:00 2001 From: Claire Date: Tue, 25 Apr 2023 16:51:38 +0200 Subject: [PATCH] Fix anonymous visitors getting a session cookie on first visit (#24584) --- app/controllers/api/base_controller.rb | 1 - app/controllers/application_controller.rb | 18 ++++++-- .../concerns/web_app_controller_concern.rb | 4 ++ app/controllers/media_controller.rb | 1 - app/controllers/media_proxy_controller.rb | 1 - app/views/layouts/application.html.haml | 2 +- .../application_controller_spec.rb | 19 -------- spec/requests/anonymous_cookies_spec.rb | 44 +++++++++++++++++++ 8 files changed, 64 insertions(+), 26 deletions(-) create mode 100644 spec/requests/anonymous_cookies_spec.rb diff --git a/app/controllers/api/base_controller.rb b/app/controllers/api/base_controller.rb index 56bf69c4021..2629ab782fc 100644 --- a/app/controllers/api/base_controller.rb +++ b/app/controllers/api/base_controller.rb @@ -8,7 +8,6 @@ class Api::BaseController < ApplicationController include AccessTokenTrackingConcern include ApiCachingConcern - skip_before_action :store_current_location skip_before_action :require_functional!, unless: :whitelist_mode? before_action :require_authenticated_user!, if: :disallow_unauthenticated_api_access? diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 5fec3bf1735..f966b18ab59 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -20,6 +20,7 @@ class ApplicationController < ActionController::Base helper_method :sso_account_settings helper_method :whitelist_mode? helper_method :body_class_string + helper_method :skip_csrf_meta_tags? rescue_from ActionController::ParameterMissing, Paperclip::AdapterRegistry::NoHandlerError, with: :bad_request rescue_from Mastodon::NotPermittedError, with: :forbidden @@ -36,7 +37,7 @@ class ApplicationController < ActionController::Base service_unavailable end - before_action :store_current_location, except: :raise_not_found, unless: :devise_controller? + before_action :store_referrer, except: :raise_not_found, if: :devise_controller? before_action :require_functional!, if: :user_signed_in? before_action :set_cache_control_defaults @@ -57,14 +58,25 @@ class ApplicationController < ActionController::Base !authorized_fetch_mode? end - def store_current_location - store_location_for(:user, request.url) unless [:json, :rss].include?(request.format&.to_sym) + def store_referrer + return if request.referer.blank? + + redirect_uri = URI(request.referer) + return if redirect_uri.path.start_with?('/auth') + + stored_url = redirect_uri.to_s if redirect_uri.host == request.host && redirect_uri.port == request.port + + store_location_for(:user, stored_url) end def require_functional! redirect_to edit_user_registration_path unless current_user.functional? end + def skip_csrf_meta_tags? + false + end + def after_sign_out_path_for(_resource_or_scope) if ENV['OMNIAUTH_ONLY'] == 'true' && ENV['OIDC_ENABLED'] == 'true' '/auth/auth/openid_connect/logout' diff --git a/app/controllers/concerns/web_app_controller_concern.rb b/app/controllers/concerns/web_app_controller_concern.rb index 7f2fbae78e2..6cd32a377c6 100644 --- a/app/controllers/concerns/web_app_controller_concern.rb +++ b/app/controllers/concerns/web_app_controller_concern.rb @@ -10,6 +10,10 @@ module WebAppControllerConcern vary_by 'Accept, Accept-Language, Cookie' end + def skip_csrf_meta_tags? + current_user.nil? + end + def set_app_body_class @body_classes = 'app-body' end diff --git a/app/controllers/media_controller.rb b/app/controllers/media_controller.rb index 133564ee7ef..99a3f3431de 100644 --- a/app/controllers/media_controller.rb +++ b/app/controllers/media_controller.rb @@ -3,7 +3,6 @@ class MediaController < ApplicationController include Authorization - skip_before_action :store_current_location skip_before_action :require_functional!, unless: :whitelist_mode? before_action :authenticate_user!, if: :whitelist_mode? diff --git a/app/controllers/media_proxy_controller.rb b/app/controllers/media_proxy_controller.rb index f29b69a24a4..1b5486c1226 100644 --- a/app/controllers/media_proxy_controller.rb +++ b/app/controllers/media_proxy_controller.rb @@ -6,7 +6,6 @@ class MediaProxyController < ApplicationController include Redisable include Lockable - skip_before_action :store_current_location skip_before_action :require_functional! before_action :authenticate_user!, if: :whitelist_mode? diff --git a/app/views/layouts/application.html.haml b/app/views/layouts/application.html.haml index 7b9434d6f3c..3fa5fef09b1 100755 --- a/app/views/layouts/application.html.haml +++ b/app/views/layouts/application.html.haml @@ -30,7 +30,7 @@ = stylesheet_pack_tag current_theme, media: 'all', crossorigin: 'anonymous' = javascript_pack_tag 'common', crossorigin: 'anonymous' = javascript_pack_tag "locale_#{I18n.locale}", crossorigin: 'anonymous' - = csrf_meta_tags + = csrf_meta_tags unless skip_csrf_meta_tags? %meta{ name: 'style-nonce', content: request.content_security_policy_nonce } = stylesheet_link_tag '/inert.css', skip_pipeline: true, media: 'all', id: 'inert-style' diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb index bc6c6c0c5e5..c4710f34956 100644 --- a/spec/controllers/application_controller_spec.rb +++ b/spec/controllers/application_controller_spec.rb @@ -132,25 +132,6 @@ describe ApplicationController, type: :controller do include_examples 'respond_with_error', 422 end - describe 'before_action :store_current_location' do - it 'stores location for user if it is not devise controller' do - routes.draw { get 'success' => 'anonymous#success' } - get 'success' - expect(controller.stored_location_for(:user)).to eq '/success' - end - - context do - controller Devise::SessionsController do - end - - it 'does not store location for user if it is devise controller' do - @request.env['devise.mapping'] = Devise.mappings[:user] - get 'create' - expect(controller.stored_location_for(:user)).to be_nil - end - end - end - describe 'before_action :check_suspension' do before do routes.draw { get 'success' => 'anonymous#success' } diff --git a/spec/requests/anonymous_cookies_spec.rb b/spec/requests/anonymous_cookies_spec.rb new file mode 100644 index 00000000000..427f54e449c --- /dev/null +++ b/spec/requests/anonymous_cookies_spec.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +require 'rails_helper' + +context 'when visited anonymously' do + around do |example| + old = ActionController::Base.allow_forgery_protection + ActionController::Base.allow_forgery_protection = true + + example.run + + ActionController::Base.allow_forgery_protection = old + end + + describe 'account pages' do + it 'do not set cookies' do + alice = Fabricate(:account, username: 'alice', display_name: 'Alice') + _status = Fabricate(:status, account: alice, text: 'Hello World') + + get '/@alice' + + expect(response.cookies).to be_empty + end + end + + describe 'status pages' do + it 'do not set cookies' do + alice = Fabricate(:account, username: 'alice', display_name: 'Alice') + status = Fabricate(:status, account: alice, text: 'Hello World') + + get short_account_status_url(alice, status) + + expect(response.cookies).to be_empty + end + end + + describe 'the /about page' do + it 'does not set cookies' do + get '/about' + + expect(response.cookies).to be_empty + end + end +end