From 7f1d932342c4cd63a63623de7ae0887c14a736a3 Mon Sep 17 00:00:00 2001
From: Sean McGivern <sean@gitlab.com>
Date: Wed, 30 Oct 2019 15:23:56 +0000
Subject: [PATCH] Only inject gon variables and perform redirects for HTML
Instead of excluding XHRs for these actions, we only want to perform
them when we're serving an HTML page. If we're serving an image or an
Atom feed, they are mostly useless:
1. Gon variables can't be used by an image.
2. Redirects won't be seen if an image is embedded in another page.
---
app/controllers/application_controller.rb | 10 +++---
.../concerns/confirm_email_warning.rb | 7 ++--
app/controllers/concerns/uploads_actions.rb | 17 +++++++++
.../application_controller_spec.rb | 14 ++++----
spec/controllers/uploads_controller_spec.rb | 24 ++++++-------
spec/requests/user_avatar_spec.rb | 36 +++++++++++++++++++
6 files changed, 81 insertions(+), 27 deletions(-)
create mode 100644 spec/requests/user_avatar_spec.rb
diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb
index 85602aa72deb3..13b226a6a7106 100644
--- a/app/controllers/application_controller.rb
+++ b/app/controllers/application_controller.rb
@@ -20,11 +20,11 @@ class ApplicationController < ActionController::Base
before_action :authenticate_user!, except: [:route_not_found]
before_action :enforce_terms!, if: :should_enforce_terms?
before_action :validate_user_service_ticket!
- before_action :check_password_expiration
+ before_action :check_password_expiration, if: :html_request?
before_action :ldap_security_check
before_action :sentry_context
before_action :default_headers
- before_action :add_gon_variables, unless: [:peek_request?, :json_request?]
+ before_action :add_gon_variables, if: :html_request?
before_action :configure_permitted_parameters, if: :devise_controller?
before_action :require_email, unless: :devise_controller?
before_action :active_user_check, unless: :devise_controller?
@@ -455,8 +455,8 @@ def set_page_title_header
response.headers['Page-Title'] = URI.escape(page_title('GitLab'))
end
- def peek_request?
- request.path.start_with?('/-/peek')
+ def html_request?
+ request.format.html?
end
def json_request?
@@ -466,7 +466,7 @@ def json_request?
def should_enforce_terms?
return false unless Gitlab::CurrentSettings.current_application_settings.enforce_terms
- !(peek_request? || devise_controller?)
+ html_request? && !devise_controller?
end
def set_usage_stats_consent_flag
diff --git a/app/controllers/concerns/confirm_email_warning.rb b/app/controllers/concerns/confirm_email_warning.rb
index 86df001066536..32e1a46e580ff 100644
--- a/app/controllers/concerns/confirm_email_warning.rb
+++ b/app/controllers/concerns/confirm_email_warning.rb
@@ -4,15 +4,18 @@ module ConfirmEmailWarning
extend ActiveSupport::Concern
included do
- before_action :set_confirm_warning, if: -> { Feature.enabled?(:soft_email_confirmation) }
+ before_action :set_confirm_warning, if: :show_confirm_warning?
end
protected
+ def show_confirm_warning?
+ html_request? && request.get? && Feature.enabled?(:soft_email_confirmation)
+ end
+
def set_confirm_warning
return unless current_user
return if current_user.confirmed?
- return if peek_request? || json_request? || !request.get?
email = current_user.unconfirmed_email || current_user.email
diff --git a/app/controllers/concerns/uploads_actions.rb b/app/controllers/concerns/uploads_actions.rb
index b87779c22d316..023c41821da1b 100644
--- a/app/controllers/concerns/uploads_actions.rb
+++ b/app/controllers/concerns/uploads_actions.rb
@@ -1,11 +1,16 @@
# frozen_string_literal: true
module UploadsActions
+ extend ActiveSupport::Concern
include Gitlab::Utils::StrongMemoize
include SendFileUpload
UPLOAD_MOUNTS = %w(avatar attachment file logo header_logo favicon).freeze
+ included do
+ prepend_before_action :set_request_format_from_path_extension
+ end
+
def create
uploader = UploadService.new(model, params[:file], uploader_class).execute
@@ -64,6 +69,18 @@ def authorize
private
+ # From ActionDispatch::Http::MimeNegotiation. We have an initializer that
+ # monkey-patches this method out (so that repository paths don't guess a
+ # format based on extension), but we do want this behaviour when serving
+ # uploads.
+ def set_request_format_from_path_extension
+ path = request.headers['action_dispatch.original_path'] || request.headers['PATH_INFO']
+
+ if match = path&.match(/\.(\w+)\z/)
+ request.format = match.captures.first
+ end
+ end
+
def uploader_class
raise NotImplementedError
end
diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb
index bb7f5ec2b28fa..7893b0812c77f 100644
--- a/spec/controllers/application_controller_spec.rb
+++ b/spec/controllers/application_controller_spec.rb
@@ -90,14 +90,6 @@ def index
let(:format) { :html }
it_behaves_like 'setting gon variables'
-
- context 'for peek requests' do
- before do
- request.path = '/-/peek'
- end
-
- it_behaves_like 'not setting gon variables'
- end
end
context 'with json format' do
@@ -105,6 +97,12 @@ def index
it_behaves_like 'not setting gon variables'
end
+
+ context 'with atom format' do
+ let(:format) { :atom }
+
+ it_behaves_like 'not setting gon variables'
+ end
end
describe 'session expiration' do
diff --git a/spec/controllers/uploads_controller_spec.rb b/spec/controllers/uploads_controller_spec.rb
index 1bcf3bb106b86..f35babc1b5636 100644
--- a/spec/controllers/uploads_controller_spec.rb
+++ b/spec/controllers/uploads_controller_spec.rb
@@ -228,10 +228,10 @@
user.block
end
- it "redirects to the sign in page" do
+ it "responds with status 401" do
get :show, params: { model: "user", mounted_as: "avatar", id: user.id, filename: "dk.png" }
- expect(response).to redirect_to(new_user_session_path)
+ expect(response).to have_gitlab_http_status(401)
end
end
@@ -320,10 +320,10 @@
end
context "when not signed in" do
- it "redirects to the sign in page" do
+ it "responds with status 401" do
get :show, params: { model: "project", mounted_as: "avatar", id: project.id, filename: "dk.png" }
- expect(response).to redirect_to(new_user_session_path)
+ expect(response).to have_gitlab_http_status(401)
end
end
@@ -343,10 +343,10 @@
project.add_maintainer(user)
end
- it "redirects to the sign in page" do
+ it "responds with status 401" do
get :show, params: { model: "project", mounted_as: "avatar", id: project.id, filename: "dk.png" }
- expect(response).to redirect_to(new_user_session_path)
+ expect(response).to have_gitlab_http_status(401)
end
end
@@ -439,10 +439,10 @@
user.block
end
- it "redirects to the sign in page" do
+ it "responds with status 401" do
get :show, params: { model: "group", mounted_as: "avatar", id: group.id, filename: "dk.png" }
- expect(response).to redirect_to(new_user_session_path)
+ expect(response).to have_gitlab_http_status(401)
end
end
@@ -526,10 +526,10 @@
end
context "when not signed in" do
- it "redirects to the sign in page" do
+ it "responds with status 401" do
get :show, params: { model: "note", mounted_as: "attachment", id: note.id, filename: "dk.png" }
- expect(response).to redirect_to(new_user_session_path)
+ expect(response).to have_gitlab_http_status(401)
end
end
@@ -549,10 +549,10 @@
project.add_maintainer(user)
end
- it "redirects to the sign in page" do
+ it "responds with status 401" do
get :show, params: { model: "note", mounted_as: "attachment", id: note.id, filename: "dk.png" }
- expect(response).to redirect_to(new_user_session_path)
+ expect(response).to have_gitlab_http_status(401)
end
end
diff --git a/spec/requests/user_avatar_spec.rb b/spec/requests/user_avatar_spec.rb
new file mode 100644
index 0000000000000..9451674161c9d
--- /dev/null
+++ b/spec/requests/user_avatar_spec.rb
@@ -0,0 +1,36 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe 'Loading a user avatar' do
+ let(:user) { create(:user, :with_avatar) }
+
+ context 'when logged in' do
+ # The exact query count will vary depending on the 2FA settings of the
+ # instance, group, and user. Removing those extra 2FA queries in this case
+ # may not be a good idea, so we just set up the ideal case.
+ before do
+ stub_application_setting(require_two_factor_authentication: true)
+
+ login_as(create(:user, :two_factor))
+ end
+
+ # One each for: current user, avatar user, and upload record
+ it 'only performs three SQL queries' do
+ get user.avatar_url # Skip queries on first application load
+
+ expect(response).to have_gitlab_http_status(200)
+ expect { get user.avatar_url }.not_to exceed_query_limit(3)
+ end
+ end
+
+ context 'when logged out' do
+ # One each for avatar user and upload record
+ it 'only performs two SQL queries' do
+ get user.avatar_url # Skip queries on first application load
+
+ expect(response).to have_gitlab_http_status(200)
+ expect { get user.avatar_url }.not_to exceed_query_limit(2)
+ end
+ end
+end
--
GitLab