Discource-C
/
discourse
mirror of https://github.com/discourse/discourse.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

SECURITY: Require POST with CSRF token for OmniAuth request phase

This commit is contained in:
David Taylor 2019-08-08 11:57:28 +01:00
parent 7bd54eaceb
commit 3b8c468832
11 changed files with 147 additions and 34 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

11
app/assets/javascripts/discourse/lib/ajax.js.es6
View File

@ -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);
});
});

63
app/assets/javascripts/discourse/models/login-method.js.es6
View File

@ -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() {

4
app/controllers/users/omniauth_callbacks_controller.rb
View File

@ -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"]

11
app/views/users/omniauth_callbacks/confirm_request.html.erb Normal file
View File

@ -0,0 +1,11 @@
<div id='simple-container'>
<h2><%= t('login.omniauth_confirm_title', provider:(t "js.login.#{params[:provider]}.name")) %></h2>
<br/>
<%= form_tag do %>
<input type="hidden" name="full_screen_login" value="true">
<%= button_tag(type: "submit", class: "btn btn-primary") do %>
<%= SvgSprite.raw_svg('fa-plug') %><%= t 'login.omniauth_confirm_button' %>
<% end %>
<% end %>
</div>

1
config/initializers/009-omniauth.rb
View File

@ -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]

2
config/locales/server.en.yml
View File

@ -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."

1
config/routes.rb
View File

@ -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}/ }

28
lib/csrf_token_verifier.rb Normal file
View File

@ -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

5
lib/middleware/omniauth_bypass_middleware.rb
View File

@ -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"]

2
spec/requests/associate_accounts_spec.rb
View File

@ -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)

53
spec/requests/omniauth_callbacks_controller_spec.rb
View File

@ -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)