diff --git a/Gemfile.lock b/Gemfile.lock index 434d37556b8..ae13bfe7786 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -298,19 +298,20 @@ GEM oj (3.16.9) bigdecimal (>= 3.0) ostruct (>= 0.2) - omniauth (1.9.2) + omniauth (2.1.2) hashie (>= 3.4.6) - rack (>= 1.6.2, < 3) + rack (>= 2.2.3) + rack-protection omniauth-facebook (9.0.0) omniauth-oauth2 (~> 1.2) - omniauth-github (1.4.0) - omniauth (~> 1.5) - omniauth-oauth2 (>= 1.4.0, < 2.0) - omniauth-google-oauth2 (0.8.2) + omniauth-github (2.0.0) + omniauth (~> 2.0) + omniauth-oauth2 (~> 1.7.1) + omniauth-google-oauth2 (1.0.1) jwt (>= 2.0) oauth2 (~> 1.1) - omniauth (~> 1.1) - omniauth-oauth2 (>= 1.6) + omniauth (~> 2.0) + omniauth-oauth2 (~> 1.7.1) omniauth-oauth (1.2.1) oauth omniauth (>= 1.0, < 3) diff --git a/config/initializers/009-omniauth.rb b/config/initializers/009-omniauth.rb index c8bfbfafb7d..9b8bdbb7cf4 100644 --- a/config/initializers/009-omniauth.rb +++ b/config/initializers/009-omniauth.rb @@ -1,8 +1,50 @@ # frozen_string_literal: true -require "openssl" - require "middleware/omniauth_bypass_middleware" Rails.application.config.middleware.use Middleware::OmniauthBypassMiddleware OmniAuth.config.logger = Rails.logger +OmniAuth.config.silence_get_warning = true + +OmniAuth.config.request_validation_phase = nil # We handle CSRF checks in before_request_phase +OmniAuth.config.before_request_phase do |env| + request = ActionDispatch::Request.new(env) + + # Check for CSRF token in POST requests + CSRFTokenVerifier.new.call(env) if request.request_method.downcase.to_sym != :get + + # If the user is trying to reconnect to an existing account, store in session + request.session[:auth_reconnect] = !!request.params["reconnect"] + + # If the client provided an origin, store in session to redirect back + request.session[:destination_url] = request.params["origin"] +end + +OmniAuth.config.on_failure do |env| + exception = env["omniauth.error"] + + # OmniAuth 2 doesn't give us any way to know for sure whether a failure was due to an + # explicit fail! call, or a rescued exception. But, this check is a pretty good guess: + is_rescued_error = exception&.message&.to_sym == env["omniauth.error.type"] + + next OmniAuth::FailureEndpoint.call(env) if !is_rescued_error # let the default behavior handle it + + case exception + when OAuth::Unauthorized + # OAuth1 (i.e. Twitter) makes a web request during the setup phase + # If it fails, Omniauth does not handle the error. Handle it here + env["omniauth.error.type"] = "request_error" + when JWT::InvalidIatError + # Happens for openid-connect (including google) providers, when the server clock is wrong + env["omniauth.error.type"] = "invalid_iat" + when CSRFTokenVerifier::InvalidCSRFToken + # Happens when CSRF token is missing from request + env["omniauth.error.type"] = "csrf_detected" + else + # default omniauth behavior is to redirect to /auth/failure with error.message in the URL + # We don't want to leak that kind of unhandled exception info, so re-raise it + raise exception + end + + OmniAuth::FailureEndpoint.call(env) +end diff --git a/lib/middleware/omniauth_bypass_middleware.rb b/lib/middleware/omniauth_bypass_middleware.rb index 8ac2ff6cc9c..19bd0a85ec2 100644 --- a/lib/middleware/omniauth_bypass_middleware.rb +++ b/lib/middleware/omniauth_bypass_middleware.rb @@ -1,70 +1,54 @@ # frozen_string_literal: true -require "csrf_token_verifier" - # omniauth loves spending lots cycles in its magic middleware stack # this middleware bypasses omniauth middleware and only hits it when needed class Middleware::OmniauthBypassMiddleware - class AuthenticatorDisabled < StandardError + module OmniAuthStrategyCompatPatch + def callback_url + result = super + if script_name.present? && result.include?("#{script_name}#{script_name}") + result = result.gsub("#{script_name}#{script_name}", script_name) + Discourse.deprecate <<~MESSAGE + OmniAuth strategy '#{name}' included duplicate script_name in callback url. It's likely the callback_url method is concatenating `script_name` with `callback_path`. + OmniAuth v2 includes the `script_name` in the `callback_path` automatically, so the manual `script_name` call can be removed. + This issue has been automatically corrected, but the strategy should be updated to ensure subfolder compatibility with future versions of Discourse. + MESSAGE + end + result + end + end + + class PatchedOmniAuthBuilder < OmniAuth::Builder + def use(strategy, *args, **kwargs, &block) + if !strategy.ancestors.include?(OmniAuthStrategyCompatPatch) + strategy.prepend(OmniAuthStrategyCompatPatch) + end + super(strategy, *args, **kwargs, &block) + end end def initialize(app, options = {}) @app = app - - OmniAuth.config.before_request_phase do |env| - request = ActionDispatch::Request.new(env) - - # Check for CSRF token in POST requests - CSRFTokenVerifier.new.call(env) if request.request_method.downcase.to_sym != :get - - # If the user is trying to reconnect to an existing account, store in session - request.session[:auth_reconnect] = !!request.params["reconnect"] - - # If the client provided an origin, store in session to redirect back - request.session[:destination_url] = request.params["origin"] - end end def call(env) - if env["PATH_INFO"].start_with?("/auth") - begin - # When only one provider is enabled, assume it can be completely trusted, and allow GET requests - only_one_provider = - !SiteSetting.enable_local_logins && Discourse.enabled_authenticators.length == 1 + return @app.call(env) unless env["PATH_INFO"].start_with?("/auth") - allow_get = only_one_provider || !SiteSetting.auth_require_interaction + # When only one provider is enabled, assume it can be completely trusted, and allow GET requests + only_one_provider = + !SiteSetting.enable_local_logins && Discourse.enabled_authenticators.length == 1 - OmniAuth.config.allowed_request_methods = allow_get ? %i[get post] : [:post] + allow_get = only_one_provider || !SiteSetting.auth_require_interaction - omniauth = - OmniAuth::Builder.new(@app) do - Discourse.enabled_authenticators.each do |authenticator| - authenticator.register_middleware(self) - end - end + OmniAuth.config.allowed_request_methods = allow_get ? %i[get post] : [:post] - omniauth.call(env) - rescue AuthenticatorDisabled => e - # Authenticator is disabled, pretend it doesn't exist and pass request to app - @app.call(env) - rescue OAuth::Unauthorized => e - # OAuth1 (i.e. Twitter) makes a web request during the setup phase - # If it fails, Omniauth does not handle the error. Handle it here - env["omniauth.error.type"] ||= "request_error" - Rails.logger.error "Authentication failure! request_error: #{e.class}, #{e.message}" - OmniAuth::FailureEndpoint.call(env) - rescue JWT::InvalidIatError => e - # Happens for openid-connect (including google) providers, when the server clock is wrong - env["omniauth.error.type"] ||= "invalid_iat" - Rails.logger.error "Authentication failure! invalid_iat: #{e.class}, #{e.message}" - OmniAuth::FailureEndpoint.call(env) - rescue CSRFTokenVerifier::InvalidCSRFToken => e - # Happens when CSRF token is missing from request - env["omniauth.error.type"] ||= "csrf_detected" - OmniAuth::FailureEndpoint.call(env) + omniauth = + PatchedOmniAuthBuilder.new(@app) do + Discourse.enabled_authenticators.each do |authenticator| + authenticator.register_middleware(self) + end end - else - @app.call(env) - end + + omniauth.call(env) end end