diff --git a/app/assets/javascripts/discourse/controllers/create-account.js.es6 b/app/assets/javascripts/discourse/controllers/create-account.js.es6 index 1c8ecb254a1..0db34a4e949 100644 --- a/app/assets/javascripts/discourse/controllers/create-account.js.es6 +++ b/app/assets/javascripts/discourse/controllers/create-account.js.es6 @@ -24,7 +24,6 @@ export default Ember.Controller.extend( login: Ember.inject.controller(), complete: false, - accountPasswordConfirm: 0, accountChallenge: 0, formSubmitted: false, rejectedEmails: Ember.A([]), @@ -191,8 +190,36 @@ export default Ember.Controller.extend( @on("init") fetchConfirmationValue() { return ajax(userPath("hp.json")).then(json => { + this._challengeDate = new Date(); + // remove 30 seconds for jitter, make sure this works for at least + // 30 seconds so we don't have hard loops + this._challengeExpiry = parseInt(json.expires_in, 10) - 30; + if (this._challengeExpiry < 30) { + this._challengeExpiry = 30; + } + + const confirmation = document.getElementById( + "new-account-confirmation" + ); + if (confirmation) { + confirmation.value = json.value; + } + + // Chrome autocomplete is buggy per: + // https://bugs.chromium.org/p/chromium/issues/detail?id=987293 + // work around issue while leaving a semi useable honeypot for + // bots that are running full Chrome + if (confirmation && navigator.userAgent.indexOf("Chrome") > 0) { + const newConfirmation = document.createElement("input"); + + newConfirmation.type = "text"; + newConfirmation.id = "new-account-confirmation"; + newConfirmation.value = json.value; + + confirmation.parentNode.replaceChild(newConfirmation, confirmation); + } + this.setProperties({ - accountPasswordConfirm: json.value, accountChallenge: json.challenge .split("") .reverse() @@ -201,85 +228,102 @@ export default Ember.Controller.extend( }); }, + performAccountCreation() { + const attrs = this.getProperties( + "accountName", + "accountEmail", + "accountPassword", + "accountUsername", + "accountChallenge" + ); + + attrs["accountPasswordConfirm"] = document.getElementById( + "new-account-confirmation" + ).value; + + const userFields = this.userFields; + const destinationUrl = this.get("authOptions.destination_url"); + + if (!Ember.isEmpty(destinationUrl)) { + $.cookie("destination_url", destinationUrl, { path: "/" }); + } + + // Add the userfields to the data + if (!Ember.isEmpty(userFields)) { + attrs.userFields = {}; + userFields.forEach( + f => (attrs.userFields[f.get("field.id")] = f.get("value")) + ); + } + + this.set("formSubmitted", true); + return Discourse.User.createAccount(attrs).then( + result => { + this.set("isDeveloper", false); + if (result.success) { + // invalidate honeypot + this._challengeExpiry = 1; + + // Trigger the browser's password manager using the hidden static login form: + const $hidden_login_form = $("#hidden-login-form"); + $hidden_login_form + .find("input[name=username]") + .val(attrs.accountUsername); + $hidden_login_form + .find("input[name=password]") + .val(attrs.accountPassword); + $hidden_login_form + .find("input[name=redirect]") + .val(userPath("account-created")); + $hidden_login_form.submit(); + } else { + this.flash( + result.message || I18n.t("create_account.failed"), + "error" + ); + if (result.is_developer) { + this.set("isDeveloper", true); + } + if ( + result.errors && + result.errors.email && + result.errors.email.length > 0 && + result.values + ) { + this.rejectedEmails.pushObject(result.values.email); + } + if ( + result.errors && + result.errors.password && + result.errors.password.length > 0 + ) { + this.rejectedPasswords.pushObject(attrs.accountPassword); + } + this.set("formSubmitted", false); + $.removeCookie("destination_url"); + } + }, + () => { + this.set("formSubmitted", false); + $.removeCookie("destination_url"); + return this.flash(I18n.t("create_account.failed"), "error"); + } + ); + }, + actions: { externalLogin(provider) { this.login.send("externalLogin", provider); }, createAccount() { - const attrs = this.getProperties( - "accountName", - "accountEmail", - "accountPassword", - "accountUsername", - "accountPasswordConfirm", - "accountChallenge" - ); - const userFields = this.userFields; - const destinationUrl = this.get("authOptions.destination_url"); - - if (!Ember.isEmpty(destinationUrl)) { - $.cookie("destination_url", destinationUrl, { path: "/" }); - } - - // Add the userfields to the data - if (!Ember.isEmpty(userFields)) { - attrs.userFields = {}; - userFields.forEach( - f => (attrs.userFields[f.get("field.id")] = f.get("value")) + if (new Date() - this._challengeDate > 1000 * this._challengeExpiry) { + this.fetchConfirmationValue().then(() => + this.performAccountCreation() ); + } else { + this.performAccountCreation(); } - - this.set("formSubmitted", true); - return Discourse.User.createAccount(attrs).then( - result => { - this.set("isDeveloper", false); - if (result.success) { - // Trigger the browser's password manager using the hidden static login form: - const $hidden_login_form = $("#hidden-login-form"); - $hidden_login_form - .find("input[name=username]") - .val(attrs.accountUsername); - $hidden_login_form - .find("input[name=password]") - .val(attrs.accountPassword); - $hidden_login_form - .find("input[name=redirect]") - .val(userPath("account-created")); - $hidden_login_form.submit(); - } else { - this.flash( - result.message || I18n.t("create_account.failed"), - "error" - ); - if (result.is_developer) { - this.set("isDeveloper", true); - } - if ( - result.errors && - result.errors.email && - result.errors.email.length > 0 && - result.values - ) { - this.rejectedEmails.pushObject(result.values.email); - } - if ( - result.errors && - result.errors.password && - result.errors.password.length > 0 - ) { - this.rejectedPasswords.pushObject(attrs.accountPassword); - } - this.set("formSubmitted", false); - $.removeCookie("destination_url"); - } - }, - () => { - this.set("formSubmitted", false); - $.removeCookie("destination_url"); - return this.flash(I18n.t("create_account.failed"), "error"); - } - ); } } } diff --git a/app/assets/javascripts/discourse/templates/modal/create-account.hbs b/app/assets/javascripts/discourse/templates/modal/create-account.hbs index 4615fb74bf4..ad5dacdfc82 100644 --- a/app/assets/javascripts/discourse/templates/modal/create-account.hbs +++ b/app/assets/javascripts/discourse/templates/modal/create-account.hbs @@ -83,7 +83,7 @@ - {{input type="password" value=accountPasswordConfirm id="new-account-confirmation" autocomplete="new-password"}} + {{input value=accountChallenge id="new-account-challenge"}} diff --git a/app/controllers/users_controller.rb b/app/controllers/users_controller.rb index 5ffc97b7121..a696aa4ab12 100644 --- a/app/controllers/users_controller.rb +++ b/app/controllers/users_controller.rb @@ -414,6 +414,9 @@ class UsersController < ApplicationController authentication.finish activation.finish + secure_session[HONEYPOT_KEY] = nil + secure_session[CHALLENGE_KEY] = nil + # save user email in session, to show on account-created page session["user_created_message"] = activation.message session[SessionController::ACTIVATE_USER_KEY] = user.id @@ -467,7 +470,14 @@ class UsersController < ApplicationController end def get_honeypot_value - render json: { value: honeypot_value, challenge: challenge_value } + secure_session.set(HONEYPOT_KEY, honeypot_value, expires: 1.hour) + secure_session.set(CHALLENGE_KEY, challenge_value, expires: 1.hour) + + render json: { + value: honeypot_value, + challenge: challenge_value, + expires_in: SecureSession.expiry + } end def password_reset @@ -660,7 +670,6 @@ class UsersController < ApplicationController security_keys_enabled = email_token_user&.security_keys_enabled? second_factor_token = params[:second_factor_token] second_factor_method = params[:second_factor_method].to_i - security_key_credential = params[:security_key_credential] confirm_email = false @security_key_required = security_keys_enabled @@ -1368,22 +1377,21 @@ class UsersController < ApplicationController render json: success_json end - private + HONEYPOT_KEY ||= 'HONEYPOT_KEY' + CHALLENGE_KEY ||= 'CHALLENGE_KEY' + + protected def honeypot_value - Digest::SHA1::hexdigest("#{Discourse.current_hostname}:#{GlobalSetting.safe_secret_key_base}")[0, 15] + secure_session[HONEYPOT_KEY] ||= SecureRandom.hex end def challenge_value - challenge = $redis.get('SECRET_CHALLENGE') - unless challenge && challenge.length == 16 * 2 - challenge = SecureRandom.hex(16) - $redis.set('SECRET_CHALLENGE', challenge) - end - - challenge + secure_session[CHALLENGE_KEY] ||= SecureRandom.hex end + private + def respond_to_suspicious_request if suspicious?(params) render json: { diff --git a/lib/secure_session.rb b/lib/secure_session.rb index bced4807d90..6b8efe57c1e 100644 --- a/lib/secure_session.rb +++ b/lib/secure_session.rb @@ -6,15 +6,36 @@ class SecureSession @prefix = prefix end + def self.expiry + @expiry ||= 1.hour.to_i + end + + def self.expiry=(val) + @expiry = val + end + + def set(key, val, expires: nil) + expires ||= SecureSession.expiry + $redis.setex(prefixed_key(key), SecureSession.expiry.to_i, val.to_s) + true + end + def [](key) - $redis.get("#{@prefix}#{key}") + $redis.get(prefixed_key(key)) end def []=(key, val) if val == nil - $redis.del("#{@prefix}#{key}") + $redis.del(prefixed_key(key)) else - $redis.setex("#{@prefix}#{key}", 1.hour, val.to_s) + $redis.setex(prefixed_key(key), SecureSession.expiry.to_i, val.to_s) end + val + end + + private + + def prefixed_key(key) + "#{@prefix}#{key}" end end diff --git a/spec/requests/users_controller_spec.rb b/spec/requests/users_controller_spec.rb index 26a952fca5d..40bc5322eb5 100644 --- a/spec/requests/users_controller_spec.rb +++ b/spec/requests/users_controller_spec.rb @@ -5,6 +5,41 @@ require 'rails_helper' describe UsersController do let(:user) { Fabricate(:user) } + describe "#full account registration flow" do + it "will correctly handle honeypot and challenge" do + + get '/u/hp.json' + expect(response.status).to eq(200) + + json = JSON.parse(response.body) + + params = { + email: 'jane@jane.com', + name: 'jane', + username: 'jane', + password_confirmation: json['value'], + challenge: json['challenge'].reverse, + password: SecureRandom.hex + } + + secure_session = SecureSession.new(session["secure_session_id"]) + + expect(secure_session[UsersController::HONEYPOT_KEY]).to eq(json["value"]) + expect(secure_session[UsersController::CHALLENGE_KEY]).to eq(json["challenge"]) + + post '/u.json', params: params + + expect(response.status).to eq(200) + + jane = User.find_by(username: 'jane') + + expect(jane.email).to eq('jane@jane.com') + + expect(secure_session[UsersController::HONEYPOT_KEY]).to eq(nil) + expect(secure_session[UsersController::CHALLENGE_KEY]).to eq(nil) + end + end + describe '#perform_account_activation' do let(:token) do return @token if @token.present? @@ -1020,22 +1055,15 @@ describe UsersController do shared_examples 'honeypot fails' do it 'should not create a new user' do + User.any_instance.expects(:enqueue_welcome_message).never + expect { post "/u.json", params: create_params }.to_not change { User.count } - expect(response.status).to eq(200) - end - it 'should not send an email' do - User.any_instance.expects(:enqueue_welcome_message).never - post "/u.json", params: create_params expect(response.status).to eq(200) - end - it 'should say it was successful' do - post "/u.json", params: create_params json = JSON::parse(response.body) - expect(response.status).to eq(200) expect(json["success"]).to eq(true) # should not change the session @@ -3361,7 +3389,6 @@ describe UsersController do end it 'succeeds on correct password' do - session = {} ApplicationController.any_instance.stubs(:secure_session).returns("confirmed-password-#{user.id}" => "true") post "/users/create_second_factor_totp.json"