Commit 432c1402 authored by Sean McGivern's avatar Sean McGivern

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.
parent 56bc7fa4
...@@ -20,11 +20,11 @@ class ApplicationController < ActionController::Base ...@@ -20,11 +20,11 @@ class ApplicationController < ActionController::Base
before_action :authenticate_user!, except: [:route_not_found] before_action :authenticate_user!, except: [:route_not_found]
before_action :enforce_terms!, if: :should_enforce_terms? before_action :enforce_terms!, if: :should_enforce_terms?
before_action :validate_user_service_ticket! 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 :ldap_security_check
before_action :sentry_context before_action :sentry_context
before_action :default_headers 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 :configure_permitted_parameters, if: :devise_controller?
before_action :require_email, unless: :devise_controller? before_action :require_email, unless: :devise_controller?
before_action :active_user_check, unless: :devise_controller? before_action :active_user_check, unless: :devise_controller?
...@@ -451,8 +451,8 @@ class ApplicationController < ActionController::Base ...@@ -451,8 +451,8 @@ class ApplicationController < ActionController::Base
response.headers['Page-Title'] = URI.escape(page_title('GitLab')) response.headers['Page-Title'] = URI.escape(page_title('GitLab'))
end end
def peek_request? def html_request?
request.path.start_with?('/-/peek') request.format.html?
end end
def json_request? def json_request?
...@@ -462,7 +462,7 @@ class ApplicationController < ActionController::Base ...@@ -462,7 +462,7 @@ class ApplicationController < ActionController::Base
def should_enforce_terms? def should_enforce_terms?
return false unless Gitlab::CurrentSettings.current_application_settings.enforce_terms return false unless Gitlab::CurrentSettings.current_application_settings.enforce_terms
!(peek_request? || devise_controller?) html_request? && !devise_controller?
end end
def set_usage_stats_consent_flag def set_usage_stats_consent_flag
......
...@@ -4,15 +4,18 @@ module ConfirmEmailWarning ...@@ -4,15 +4,18 @@ module ConfirmEmailWarning
extend ActiveSupport::Concern extend ActiveSupport::Concern
included do included do
before_action :set_confirm_warning, if: -> { Feature.enabled?(:soft_email_confirmation) } before_action :set_confirm_warning, if: :show_confirm_warning?
end end
protected protected
def show_confirm_warning?
html_request? && request.get? && Feature.enabled?(:soft_email_confirmation)
end
def set_confirm_warning def set_confirm_warning
return unless current_user return unless current_user
return if current_user.confirmed? return if current_user.confirmed?
return if peek_request? || json_request? || !request.get?
email = current_user.unconfirmed_email || current_user.email email = current_user.unconfirmed_email || current_user.email
......
# frozen_string_literal: true # frozen_string_literal: true
module UploadsActions module UploadsActions
extend ActiveSupport::Concern
include Gitlab::Utils::StrongMemoize include Gitlab::Utils::StrongMemoize
include SendFileUpload include SendFileUpload
UPLOAD_MOUNTS = %w(avatar attachment file logo header_logo favicon).freeze 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 def create
uploader = UploadService.new(model, params[:file], uploader_class).execute uploader = UploadService.new(model, params[:file], uploader_class).execute
...@@ -64,6 +69,18 @@ module UploadsActions ...@@ -64,6 +69,18 @@ module UploadsActions
private 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 def uploader_class
raise NotImplementedError raise NotImplementedError
end end
......
...@@ -90,14 +90,6 @@ describe ApplicationController do ...@@ -90,14 +90,6 @@ describe ApplicationController do
let(:format) { :html } let(:format) { :html }
it_behaves_like 'setting gon variables' 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 end
context 'with json format' do context 'with json format' do
...@@ -105,6 +97,12 @@ describe ApplicationController do ...@@ -105,6 +97,12 @@ describe ApplicationController do
it_behaves_like 'not setting gon variables' it_behaves_like 'not setting gon variables'
end end
context 'with atom format' do
let(:format) { :atom }
it_behaves_like 'not setting gon variables'
end
end end
describe 'session expiration' do describe 'session expiration' do
......
...@@ -228,10 +228,10 @@ describe UploadsController do ...@@ -228,10 +228,10 @@ describe UploadsController do
user.block user.block
end 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" } 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
end end
...@@ -320,10 +320,10 @@ describe UploadsController do ...@@ -320,10 +320,10 @@ describe UploadsController do
end end
context "when not signed in" do 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" } 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
end end
...@@ -343,10 +343,10 @@ describe UploadsController do ...@@ -343,10 +343,10 @@ describe UploadsController do
project.add_maintainer(user) project.add_maintainer(user)
end 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" } 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
end end
...@@ -439,10 +439,10 @@ describe UploadsController do ...@@ -439,10 +439,10 @@ describe UploadsController do
user.block user.block
end 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" } 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
end end
...@@ -526,10 +526,10 @@ describe UploadsController do ...@@ -526,10 +526,10 @@ describe UploadsController do
end end
context "when not signed in" do 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" } 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
end end
...@@ -549,10 +549,10 @@ describe UploadsController do ...@@ -549,10 +549,10 @@ describe UploadsController do
project.add_maintainer(user) project.add_maintainer(user)
end 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" } 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
end end
......
# 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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment