diff --git a/app/assets/javascripts/discourse/lib/ajax.js.es6 b/app/assets/javascripts/discourse/lib/ajax.js.es6 index e4ca8df52ed..1cec5148e74 100644 --- a/app/assets/javascripts/discourse/lib/ajax.js.es6 +++ b/app/assets/javascripts/discourse/lib/ajax.js.es6 @@ -40,6 +40,12 @@ function handleRedirect(data) { } } +export function updateCsrfToken() { + return ajax("/session/csrf").then(result => { + Discourse.Session.currentProp("csrfToken", result.csrf); + }); +} + /** Our own $.ajax method. Makes sure the .then method executes in an Ember runloop for performance reasons. Also automatically adjusts the URL to support installs @@ -157,10 +163,7 @@ export function ajax() { !Discourse.Session.currentProp("csrfToken") ) { promise = new Ember.RSVP.Promise((resolve, reject) => { - ajaxObj = $.ajax(Discourse.getURL("/session/csrf"), { - cache: false - }).done(result => { - Discourse.Session.currentProp("csrfToken", result.csrf); + ajaxObj = updateCsrfToken().then(() => { performAjax(resolve, reject); }); }); diff --git a/app/assets/javascripts/discourse/models/login-method.js.es6 b/app/assets/javascripts/discourse/models/login-method.js.es6 index a9166633f4f..e00bd96308c 100644 --- a/app/assets/javascripts/discourse/models/login-method.js.es6 +++ b/app/assets/javascripts/discourse/models/login-method.js.es6 @@ -1,4 +1,5 @@ import computed from "ember-addons/ember-computed-decorators"; +import { updateCsrfToken } from "discourse/lib/ajax"; const LoginMethod = Ember.Object.extend({ @computed @@ -30,8 +31,10 @@ const LoginMethod = Ember.Object.extend({ } if (reconnect || fullScreenLogin || this.full_screen_login) { - document.cookie = "fsl=true"; - window.location = authUrl; + LoginMethod.buildPostForm(authUrl).then(form => { + document.cookie = "fsl=true"; + form.submit(); + }); } else { this.set("authenticate", name); const left = this.lastX - 400; @@ -44,31 +47,51 @@ const LoginMethod = Ember.Object.extend({ authUrl += authUrl.includes("?") ? "&" : "?"; authUrl += "display=popup"; } + LoginMethod.buildPostForm(authUrl).then(form => { + const windowState = window.open( + "about:blank", + "auth_popup", + `menubar=no,status=no,height=${height},width=${width},left=${left},top=${top}` + ); - const windowState = window.open( - authUrl, - "_blank", - "menubar=no,status=no,height=" + - height + - ",width=" + - width + - ",left=" + - left + - ",top=" + - top - ); + form.target = "auth_popup"; + form.submit(); - const timer = setInterval(() => { - if (!windowState || windowState.closed) { - clearInterval(timer); - this.set("authenticate", null); - } - }, 1000); + const timer = setInterval(() => { + // If the process is aborted, reset state in this window + if (!windowState || windowState.closed) { + clearInterval(timer); + this.set("authenticate", null); + } + }, 1000); + }); } } } }); +LoginMethod.reopenClass({ + buildPostForm(url) { + // Login always happens in an anonymous context, with no CSRF token + // So we need to fetch it before sending a POST request + return updateCsrfToken().then(() => { + const form = document.createElement("form"); + form.setAttribute("style", "display:none;"); + form.setAttribute("method", "post"); + form.setAttribute("action", url); + + const input = document.createElement("input"); + input.setAttribute("name", "authenticity_token"); + input.setAttribute("value", Discourse.Session.currentProp("csrfToken")); + form.appendChild(input); + + document.body.appendChild(form); + + return form; + }); + } +}); + let methods; export function findAll() { diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index b23cc593df0..c35ae5a1c98 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -18,6 +18,10 @@ class Users::OmniauthCallbacksController < ApplicationController # will not have a CSRF token, however the payload is all validated so its safe skip_before_action :verify_authenticity_token, only: :complete + def confirm_request + self.class.find_authenticator(params[:provider]) + end + def complete auth = request.env["omniauth.auth"] raise Discourse::NotFound unless request.env["omniauth.auth"] diff --git a/app/views/users/omniauth_callbacks/confirm_request.html.erb b/app/views/users/omniauth_callbacks/confirm_request.html.erb new file mode 100644 index 00000000000..6abfe3cf0fb --- /dev/null +++ b/app/views/users/omniauth_callbacks/confirm_request.html.erb @@ -0,0 +1,11 @@ +
+

<%= t('login.omniauth_confirm_title', provider:(t "js.login.#{params[:provider]}.name")) %>

+
+ + <%= form_tag do %> + + <%= button_tag(type: "submit", class: "btn btn-primary") do %> + <%= SvgSprite.raw_svg('fa-plug') %><%= t 'login.omniauth_confirm_button' %> + <% end %> + <% end %> +
diff --git a/config/initializers/009-omniauth.rb b/config/initializers/009-omniauth.rb index 6acc2856429..415e3b38803 100644 --- a/config/initializers/009-omniauth.rb +++ b/config/initializers/009-omniauth.rb @@ -7,3 +7,4 @@ require "middleware/omniauth_bypass_middleware" Rails.application.config.middleware.use Middleware::OmniauthBypassMiddleware OmniAuth.config.logger = Rails.logger +OmniAuth.config.allowed_request_methods = [:post] diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index ba940c74ae9..5c4d88421de 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -2226,6 +2226,8 @@ en: something_already_taken: "Something went wrong, perhaps the username or email is already registered. Try the forgot password link." omniauth_error: "Sorry, there was an error authorizing your account. Perhaps you did not approve authorization?" omniauth_error_unknown: "Something went wrong processing your log in, please try again." + omniauth_confirm_title: "Log in using %{provider}" + omniauth_confirm_button: "Continue" authenticator_error_no_valid_email: "No email addresses associated with %{account} are allowed. You may need to configure your account with a different email address." new_registrations_disabled: "New account registrations are not allowed at this time." password_too_long: "Passwords are limited to 200 characters." diff --git a/config/routes.rb b/config/routes.rb index 68b1b00fe93..fb0cda2cf23 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -599,6 +599,7 @@ Discourse::Application.routes.draw do end end + get "/auth/:provider", to: "users/omniauth_callbacks#confirm_request" match "/auth/:provider/callback", to: "users/omniauth_callbacks#complete", via: [:get, :post] match "/auth/failure", to: "users/omniauth_callbacks#failure", via: [:get, :post] get "/associate/:token", to: "users/associate_accounts#connect_info", constraints: { token: /\h{32}/ } diff --git a/lib/csrf_token_verifier.rb b/lib/csrf_token_verifier.rb new file mode 100644 index 00000000000..1c1842419d9 --- /dev/null +++ b/lib/csrf_token_verifier.rb @@ -0,0 +1,28 @@ +# frozen_string_literal: true + +# Provides a way to check a CSRF token outside of a controller +class CSRFTokenVerifier + include ActiveSupport::Configurable + include ActionController::RequestForgeryProtection + + # Use config from ActionController::Base + config.each_key do |configuration_name| + undef_method configuration_name + define_method configuration_name do + ActionController::Base.config[configuration_name] + end + end + + def call(env) + @request = ActionDispatch::Request.new(env.dup) + + unless verified_request? + raise ActionController::InvalidAuthenticityToken + end + end + + private + + attr_reader :request + delegate :params, :session, to: :request +end diff --git a/lib/middleware/omniauth_bypass_middleware.rb b/lib/middleware/omniauth_bypass_middleware.rb index e039489310f..8f4fdf27805 100644 --- a/lib/middleware/omniauth_bypass_middleware.rb +++ b/lib/middleware/omniauth_bypass_middleware.rb @@ -1,5 +1,7 @@ # 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 @@ -19,6 +21,9 @@ class Middleware::OmniauthBypassMiddleware end @omniauth.before_request_phase do |env| + # Check for CSRF token + CSRFTokenVerifier.new.call(env) + # If the user is trying to reconnect to an existing account, store in session request = ActionDispatch::Request.new(env) request.session[:auth_reconnect] = !!request.params["reconnect"] diff --git a/spec/requests/associate_accounts_spec.rb b/spec/requests/associate_accounts_spec.rb index 41b199c838d..3224750d820 100644 --- a/spec/requests/associate_accounts_spec.rb +++ b/spec/requests/associate_accounts_spec.rb @@ -38,7 +38,7 @@ RSpec.describe Users::AssociateAccountsController do sign_in(user) # Reconnect flow: - get "/auth/google_oauth2?reconnect=true" + post "/auth/google_oauth2?reconnect=true" expect(response.status).to eq(302) expect(session[:auth_reconnect]).to eq(true) diff --git a/spec/requests/omniauth_callbacks_controller_spec.rb b/spec/requests/omniauth_callbacks_controller_spec.rb index c5a093f82c7..82a424b0bb6 100644 --- a/spec/requests/omniauth_callbacks_controller_spec.rb +++ b/spec/requests/omniauth_callbacks_controller_spec.rb @@ -84,6 +84,41 @@ RSpec.describe Users::OmniauthCallbacksController do SiteSetting.enable_google_oauth2_logins = true end + describe "request" do + it "should error for non existant authenticators" do + post "/auth/fake_auth" + expect(response.status).to eq(404) + get "/auth/fake_auth" + expect(response.status).to eq(403) + end + + it "should only start auth with a POST request" do + post "/auth/google_oauth2" + expect(response.status).to eq(302) + get "/auth/google_oauth2" + expect(response.status).to eq(200) + end + + context "with CSRF protection enabled" do + before { ActionController::Base.allow_forgery_protection = true } + after { ActionController::Base.allow_forgery_protection = false } + + it "should be CSRF protected" do + post "/auth/google_oauth2" + expect(response.status).to eq(422) + + post "/auth/google_oauth2", params: { authenticity_token: "faketoken" } + expect(response.status).to eq(422) + + get "/session/csrf.json" + token = JSON.parse(response.body)["csrf"] + + post "/auth/google_oauth2", params: { authenticity_token: token } + expect(response.status).to eq(302) + end + end + end + context "without an `omniauth.auth` env" do it "should return a 404" do get "/auth/eviltrout/callback" @@ -348,7 +383,7 @@ RSpec.describe Users::OmniauthCallbacksController do end it "doesn't attempt redirect to external origin" do - get "/auth/google_oauth2?origin=https://example.com/external" + post "/auth/google_oauth2?origin=https://example.com/external" get "/auth/google_oauth2/callback" expect(response.status).to eq 302 @@ -359,7 +394,7 @@ RSpec.describe Users::OmniauthCallbacksController do end it "redirects to internal origin" do - get "/auth/google_oauth2?origin=http://test.localhost/t/123" + post "/auth/google_oauth2?origin=http://test.localhost/t/123" get "/auth/google_oauth2/callback" expect(response.status).to eq 302 @@ -370,7 +405,7 @@ RSpec.describe Users::OmniauthCallbacksController do end it "redirects to relative origin" do - get "/auth/google_oauth2?origin=/t/123" + post "/auth/google_oauth2?origin=/t/123" get "/auth/google_oauth2/callback" expect(response.status).to eq 302 @@ -381,7 +416,7 @@ RSpec.describe Users::OmniauthCallbacksController do end it "redirects with query" do - get "/auth/google_oauth2?origin=/t/123?foo=bar" + post "/auth/google_oauth2?origin=/t/123?foo=bar" get "/auth/google_oauth2/callback" expect(response.status).to eq 302 @@ -392,7 +427,7 @@ RSpec.describe Users::OmniauthCallbacksController do end it "removes authentication_data cookie on logout" do - get "/auth/google_oauth2?origin=https://example.com/external" + post "/auth/google_oauth2?origin=https://example.com/external" get "/auth/google_oauth2/callback" provider = log_in_user(Fabricate(:user)) @@ -440,7 +475,7 @@ RSpec.describe Users::OmniauthCallbacksController do it 'should not reconnect normally' do # Log in normally - get "/auth/google_oauth2" + post "/auth/google_oauth2" expect(response.status).to eq(302) expect(session[:auth_reconnect]).to eq(false) @@ -450,7 +485,7 @@ RSpec.describe Users::OmniauthCallbacksController do # Log into another user OmniAuth.config.mock_auth[:google_oauth2].uid = "123456" - get "/auth/google_oauth2" + post "/auth/google_oauth2" expect(response.status).to eq(302) expect(session[:auth_reconnect]).to eq(false) @@ -462,7 +497,7 @@ RSpec.describe Users::OmniauthCallbacksController do it 'should redirect to associate URL if parameter supplied' do # Log in normally - get "/auth/google_oauth2?reconnect=true" + post "/auth/google_oauth2?reconnect=true" expect(response.status).to eq(302) expect(session[:auth_reconnect]).to eq(true) @@ -477,7 +512,7 @@ RSpec.describe Users::OmniauthCallbacksController do UserAssociatedAccount.find_by(user_id: user.id).destroy # Reconnect flow: - get "/auth/google_oauth2?reconnect=true" + post "/auth/google_oauth2?reconnect=true" expect(response.status).to eq(302) expect(session[:auth_reconnect]).to eq(true)