diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 49eb83581..cefdb39e9 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -1,4 +1,5 @@ class ApplicationController < ActionController::Base + skip_forgery_protection if: -> { SessionLoader.new(request).has_api_authentication? } helper :pagination before_action :reset_current_user before_action :set_current_user @@ -83,6 +84,8 @@ class ApplicationController < ActionController::Base render_error_page(500, exception, message: "The database timed out running your query.") when ActionController::BadRequest render_error_page(400, exception) + when ActionController::InvalidAuthenticityToken + render_error_page(403, exception) when ActiveRecord::RecordNotFound render_error_page(404, exception, message: "That record was not found.") when ActionController::RoutingError @@ -109,8 +112,11 @@ class ApplicationController < ActionController::Base @backtrace = Rails.backtrace_cleaner.clean(@exception.backtrace) format = :html unless format.in?(%i[html json xml js atom]) + # if InvalidAuthenticityToken was raised, CurrentUser isn't set so we have to use the blank layout. + layout = CurrentUser.user.present? ? "default" : "blank" + DanbooruLogger.log(@exception, expected: @expected) - render "static/error", status: status, formats: format + render "static/error", layout: layout, status: status, formats: format end def authentication_failed @@ -157,8 +163,7 @@ class ApplicationController < ActionController::Base end def set_current_user - session_loader = SessionLoader.new(session, cookies, request, params) - session_loader.load + SessionLoader.new(request).load end def reset_current_user diff --git a/app/logical/session_loader.rb b/app/logical/session_loader.rb index d62f4748f..ea3f6cdfb 100644 --- a/app/logical/session_loader.rb +++ b/app/logical/session_loader.rb @@ -3,23 +3,23 @@ class SessionLoader attr_reader :session, :cookies, :request, :params - def initialize(session, cookies, request, params) - @session = session - @cookies = cookies + def initialize(request) @request = request - @params = params + @session = request.session + @cookies = request.cookie_jar + @params = request.parameters end def load CurrentUser.user = User.anonymous CurrentUser.ip_addr = request.remote_ip - if session[:user_id] + if has_api_authentication? + load_session_for_api + elsif session[:user_id] load_session_user elsif cookie_password_hash_valid? load_cookie_user - else - load_session_for_api end set_statement_timeout @@ -30,6 +30,10 @@ class SessionLoader DanbooruLogger.initialize(request, session, CurrentUser.user) end + def has_api_authentication? + request.authorization.present? || params[:login].present? || params[:api_key].present? || params[:password_hash].present? + end + private def set_statement_timeout @@ -40,21 +44,21 @@ private def load_session_for_api if request.authorization authenticate_basic_auth - elsif params[:login].present? && params[:api_key].present? authenticate_api_key(params[:login], params[:api_key]) - elsif params[:login].present? && params[:password_hash].present? authenticate_legacy_api_key(params[:login], params[:password_hash]) + else + raise AuthenticationFailure end end - + def authenticate_basic_auth credentials = ::Base64.decode64(request.authorization.split(' ', 2).last || '') login, api_key = credentials.split(/:/, 2) authenticate_api_key(login, api_key) end - + def authenticate_api_key(name, api_key) CurrentUser.user = User.authenticate_api_key(name, api_key) @@ -62,7 +66,7 @@ private raise AuthenticationFailure.new end end - + def authenticate_legacy_api_key(name, password_hash) CurrentUser.user = User.authenticate_hash(name, password_hash) diff --git a/test/functional/application_controller_test.rb b/test/functional/application_controller_test.rb index 7a5f4e63c..fed3cf21d 100644 --- a/test/functional/application_controller_test.rb +++ b/test/functional/application_controller_test.rb @@ -40,6 +40,12 @@ class ApplicationControllerTest < ActionDispatch::IntegrationTest setup do @user = create(:user, password: "password") @api_key = ApiKey.generate!(@user) + + ActionController::Base.allow_forgery_protection = true + end + + teardown do + ActionController::Base.allow_forgery_protection = false end context "using http basic auth" do @@ -54,6 +60,14 @@ class ApplicationControllerTest < ActionDispatch::IntegrationTest get edit_user_path(@user), headers: { HTTP_AUTHORIZATION: basic_auth_string } assert_response 401 end + + should "succeed for non-GET requests without a CSRF token" do + assert_changes -> { @user.reload.enable_safe_mode }, from: false, to: true do + basic_auth_string = "Basic #{::Base64.encode64("#{@user.name}:#{@api_key.key}")}" + put user_path(@user), headers: { HTTP_AUTHORIZATION: basic_auth_string }, params: { user: { enable_safe_mode: "true" } }, as: :json + assert_response :success + end + end end context "using the api_key parameter" do @@ -72,6 +86,13 @@ class ApplicationControllerTest < ActionDispatch::IntegrationTest get edit_user_path(@user), params: { login: @user.name, api_key: "bad" } assert_response 401 end + + should "succeed for non-GET requests without a CSRF token" do + assert_changes -> { @user.reload.enable_safe_mode }, from: false, to: true do + put user_path(@user), params: { login: @user.name, api_key: @api_key.key, user: { enable_safe_mode: "true" } }, as: :json + assert_response :success + end + end end context "using the password_hash parameter" do @@ -90,6 +111,13 @@ class ApplicationControllerTest < ActionDispatch::IntegrationTest get edit_user_path(@user), params: { login: @user.name, password_hash: "bad" } assert_response 401 end + + should "succeed for non-GET requests without a CSRF token" do + assert_changes -> { @user.reload.enable_safe_mode }, from: false, to: true do + put user_path(@user), params: { login: @user.name, password_hash: User.sha1("password"), user: { enable_safe_mode: "true" } }, as: :json + assert_response :success + end + end end context "without any authentication" do @@ -98,6 +126,25 @@ class ApplicationControllerTest < ActionDispatch::IntegrationTest assert_redirected_to new_session_path(url: edit_user_path(@user)) end end + + context "with cookie-based authentication" do + should "not allow non-GET requests without a CSRF token" do + # get the csrf token from the login page so we can login + get new_session_path + assert_response :success + token = css_select("form input[name=authenticity_token]").first["value"] + + # login + post session_path, params: { authenticity_token: token, name: @user.name, password: "password" } + assert_redirected_to posts_path + + # try to submit a form with cookies but without the csrf token + put user_path(@user), headers: { HTTP_COOKIE: headers["Set-Cookie"] }, params: { user: { enable_safe_mode: "true" } } + assert_response 403 + assert_equal("ActionController::InvalidAuthenticityToken", css_select("p").first.content) + assert_equal(false, @user.reload.enable_safe_mode) + end + end end context "on session cookie authentication" do